-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Use client-side redirects in admin monitors for Chrome #26207
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
base: master
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR introduces a client-side redirect HttpResponse and switches selected administrative monitors to use it so that redirects to jenkins.io documentation continue to work in Chrome when CSP form-action is enforced.
Changes:
- Added
jenkins.util.ClientHttpRedirect, anHttpResponseimplementation that performs a redirect via HTML meta refresh and a JS helper, usingUtil.printRedirect. - Exposed a convenience API
HttpResponses.clientRedirectTo(String url)for client-side redirects. - Updated three administrative monitors (
JavaVersionRecommendationAdminMonitor,DefaultCrumbIssuer.ExcludeSessionIdAdministrativeMonitor, andReverseProxySetupMonitor) to use client-side redirects instead of server-sideHttpRedirect/sendRedirectwhen sending users to external documentation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/jenkins/util/ClientHttpRedirect.java | Introduces a reusable HttpResponse that renders an HTML+JS redirect page via Util.printRedirect, avoiding CSP form-action issues in Chrome for form submissions redirecting off-site. |
| core/src/main/java/hudson/util/HttpResponses.java | Adds clientRedirectTo(String url) as a public helper returning ClientHttpRedirect, making client-side redirects discoverable to core and plugin code. |
| core/src/main/java/jenkins/monitor/JavaVersionRecommendationAdminMonitor.java | Changes the “learn more” path in doAct to use ClientHttpRedirect for the Java support documentation link while keeping the dismiss path using HttpResponses.forwardToPreviousPage(). |
| core/src/main/java/hudson/security/csrf/DefaultCrumbIssuer.java | Refactors the ExcludeSessionIdAdministrativeMonitor#doAct to return HttpResponse, using ClientHttpRedirect for the CSRF documentation link and HttpResponses.forwardToPreviousPage() for dismissal. |
| core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java | Updates the admin monitor action doAct to use ClientHttpRedirect for the broken reverse proxy documentation, preserving the existing internal /manage redirect via HttpResponses.redirectViaContextPath. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Kevin-CB
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.
I tested the changes, redirection works fine.
|
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. /label ready-for-merge |
Fixes #26001
I considered multiple options to address this issue:
www.jenkins.io, so that would be easy.Locationheader to client-side JS/metatag.The first one is easy but limited: Any URLs outside www.jenkins.io would be unsupported. Even if there aren't many problems like this across plugins, relaxing rules does not set a good example for plugin developers.
The second one was my favorite until I found #26205. It's still my favorite option for plugins though, and has as additional benefit that it's straightforward to open the link in a new window.
The third seems unnecessarily complicated on the view, so I dismissed it quickly.
This implementation uses the fourth option. For use in plugins, they would need to increase the core dependency, or reimplement the
HttpResponsecallingUtil#printRedirectthemselves. The latter seems reasonable, with just using a (currently broken looking) link probably the easiest.FWIW there is currently no caller for
HttpResponses#clientRedirectTo, so technically not ideal. It seems like a natural addition to that API though, and mirrors existing methods. Adding it there would help discoverability.In terms of plugins, a quick GH code search found just:
Even if my search is not complete, I would expect this to be a problem in few plugins, and it's fairly easily resolved with a link.
Testing done
Clicked the buttons in Chrome once the monitors showed:
jetty:run, since it fails with "HTTP ERROR 400 Ambiguous URI path separator".hudson.security.csrf.DefaultCrumbIssuer.EXCLUDE_SESSION_ID = truein script consoleJavaVersionRecommendationAdminMonitorso it's already due.I skipped autotests, because the new code is just a minor adapter around existing code doing the actual work.
Screenshots (UI changes only)
n/a
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
@mention
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered.