-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
[JENKINS-75905] Include update site URL in signature verification in error messages #25980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
bcc10db to
fb54c7d
Compare
fb54c7d to
1ce2a8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adhamahmad, LGTM!
Thank you for screenshots and covering it by adding unit test!
MarkEWaite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request. It looks good in general.
I have a question to clarify the text substitution, since I was unable to find any way to generate an exception message that would require the substitution.
I also offered two optional suggestions. You are welcome to ignore them if you don't find them to be helpful.
annotation Use the non-deprecated getJsonSignatureValidator Add @nonnull annotation to the Throwable parameter in JSONSignatureValidator#getRootCauseMessage.
|
Hey @MarkEWaite, Thanks for the suggestions, I’ve applied the requested changes. Regarding the text substitution logic: I added it cause I notcied that the error message shown in case of signature verification errors include escaped HTML characters and the literal default word:
I assumed that the "update site … Path" message pattern was common, and that replacing that segment would both ensure the update site URL is included and avoid the escaped In the automated test, the actual error message does not match that pattern. The message produced is: Since this message does not contain the "update site … Path" sequence, the substitution branch is not executed, and the code correctly falls back to appending the URL. Since the escaped HTML content already exists before this code runs, I am unsure whether handling it here is appropriate or whether it should be left as it is or addressed elsewhere. Please advise on the preferred approach. |
I like that technique. That's a creative way to avoid showing a poorly formatted string to the user. It handles the issue as reported and you also handle the more general case. I think that your solution is a very reasonable choice. In both cases, the URL of the update site is shown and the stack trace is not shown. |
MarkEWaite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.
/label ready-for-merge
|
Congratulations on getting your very first Jenkins core pull request merged 🎉🥳 |

Fixes #16784
This change improves the readability of signature verification error messages in the plugin manager UI.
Testing done
Screenshots (UI changes only)
Before
After
Proposed changelog entries
Proposed changelog category
/label bug
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
N/A
Before the changes are marked as
ready-for-merge:Maintainer checklist
If the change needs additional upgrade steps from users, theupgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).If it would make sense to backport the change to LTS, be a Bug or Improvement, and either the issue or pull request must be labeled aslts-candidateto be considered.