Implement URL validation and security checks in ClientHttpRedirect by YoussefAhmed256 · Pull Request #26395 · jenkinsci/jenkins

YoussefAhmed256 · 2026-03-02T22:38:27Z

Fixes #26387 >

Testing done

Screenshots (UI changes only)

Before

After

Proposed changelog entries

/label skip-changelog

Proposed changelog category

/label

Proposed upgrade guidelines

N/A

Submitter checklist

The issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
UI changes do not introduce regressions when enforcing the current default rules of Content Security Policy Plugin. In particular, new or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label 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 as lts-candidate to be considered.

Copilot

Pull request overview

Adds URL scheme validation to ClientHttpRedirect to prevent client-side redirects to non-HTTP(S) schemes (per #26387), and introduces unit tests covering allowed vs blocked URL patterns.

Changes:

Add scheme/URL safety checks to ClientHttpRedirect.generateResponse and block unsafe redirects with a 403 error.
Extend class-level Javadoc to document the new redirect restrictions.
Add a new JUnit test class exercising allowed (http/https, relative) and blocked (javascript/data/file/custom) redirect targets.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
core/src/main/java/jenkins/util/ClientHttpRedirect.java	Introduces redirect URL validation and blocks unsafe schemes before emitting the client-side redirect HTML.
test/src/test/java/jenkins/util/ClientHttpRedirectTest.java	Adds coverage for allowed/blocked redirect URLs, including mixed-case schemes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-03T05:23:34Z

core/src/main/java/jenkins/util/ClientHttpRedirect.java

 public record ClientHttpRedirect(String redirectUrl) implements HttpResponse {
+
+    private static boolean isSafeToRedirectTo(@NonNull String url) {
+        if (Util.isSafeToRedirectTo(url)) {
+            return true;
+        }
+
+        String urlLower = url.toLowerCase(Locale.ENGLISH);
+        return urlLower.startsWith("http://") || urlLower.startsWith("https://");
+    }
+
    @Override
    public void generateResponse(StaplerRequest2 req, StaplerResponse2 rsp, Object o) throws IOException, ServletException {
+        if (!isSafeToRedirectTo(redirectUrl)) {
+            throw hudson.util.HttpResponses.error(403,


redirectUrl can be null (record component is unannotated) but generateResponse and isSafeToRedirectTo assume non-null and will NPE. Consider annotating the record component with @NonNull and enforcing it in a compact constructor (or otherwise handle null safely).

Copilot · 2026-03-03T05:23:34Z

test/src/test/java/jenkins/util/ClientHttpRedirectTest.java

+        writer.flush();
+        String output = baos.toString();
+        assert output.length() > 0;
+    }


These tests use Java assert statements, which are skipped unless the JVM is started with -ea; this can make the test suite pass without actually verifying behavior. Use JUnit assertions (e.g., Assertions.assertTrue/assertFalse/assertEquals) instead.

Copilot · 2026-03-03T05:23:35Z

test/src/test/java/jenkins/util/ClientHttpRedirectTest.java

+        try {
+            redirect.generateResponse(req, rsp, null);
+            throw new AssertionError("Should have thrown an exception");
+        } catch (Exception e) {
+            assert e.getMessage().contains("Unsafe redirect blocked");
+        }
+    }


The blocked-URL tests use manual try/catch plus throw new AssertionError(...), and then assert on the exception message. Prefer Assertions.assertThrows to assert the expected exception type and inspect the message; it produces clearer failures and avoids false positives from unexpected exception types.

Copilot · 2026-03-03T05:23:35Z

core/src/main/java/jenkins/util/ClientHttpRedirect.java

+        }
+
        rsp.setContentType("text/html;charset=UTF-8");
        Util.printRedirect(req.getContextPath(), redirectUrl, redirectUrl, rsp.getWriter());


Util.printRedirect writes the message argument into the HTML body without escaping (see hudson.Util#printRedirect). Passing redirectUrl as the message here can therefore lead to HTML/JS injection if a caller ever supplies an attacker-controlled value. Use an HTML-escaped message (or a fixed string) rather than the raw URL.

Suggested change

Util.printRedirect(req.getContextPath(), redirectUrl, redirectUrl, rsp.getWriter());

Util.printRedirect(req.getContextPath(), redirectUrl, Util.escape(redirectUrl), rsp.getWriter());

MarkEWaite · 2026-03-03T05:27:01Z

Closing this pull request, since you did not describe any of the testing that was done and did not complete the submitter portion of the pull request checklist, nor did you include screenshots showing the result of the failed redirect.

The tests that were written look promising, but they use outdated techniques like assert n and try { stmt; throw exception }. They should use assertThrows and assertEquals so that they follow the same pattern as other tests in the repository.

You are welcome to create a new pull request with those issues resolved. You should also review the comments from GitHub Copilot and include them in your new pull request.

YoussefAhmed256 · 2026-03-03T05:30:08Z

Closing this pull request, since you did not describe any of the testing that was done and did not complete the submitter portion of the pull request checklist, nor did you include screenshots showing the result of the failed redirect.

The tests that were written look promising, but they use outdated techniques like assert n and try { stmt; throw exception }. They should use assertThrows and assertEquals so that they follow the same pattern as other tests in the repository.

You are welcome to create a new pull request with those issues resolved. You should also review the comments from GitHub Copilot and include them in your new pull request.

i will resolve all these issues then back again with a new pull request
thanks for your help

Implement URL validation and security checks in ClientHttpRedirect

9e99862

comment-ops-bot bot added the skip-changelog Should not be shown in the changelog label Mar 2, 2026

MarkEWaite requested a review from Copilot March 3, 2026 05:19

Copilot started reviewing on behalf of MarkEWaite March 3, 2026 05:19 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

MarkEWaite closed this Mar 3, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement URL validation and security checks in ClientHttpRedirect#26395

Implement URL validation and security checks in ClientHttpRedirect#26395
YoussefAhmed256 wants to merge 1 commit intojenkinsci:masterfrom
YoussefAhmed256:security/client-http-redirect-validation

YoussefAhmed256 commented Mar 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

MarkEWaite commented Mar 3, 2026

Uh oh!

YoussefAhmed256 commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	Util.printRedirect(req.getContextPath(), redirectUrl, redirectUrl, rsp.getWriter());
	Util.printRedirect(req.getContextPath(), redirectUrl, Util.escape(redirectUrl), rsp.getWriter());

Uh oh!

Conversation

YoussefAhmed256 commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing done

Screenshots (UI changes only)

Before

After

Proposed changelog entries

Proposed changelog category

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

MarkEWaite commented Mar 3, 2026

Uh oh!

YoussefAhmed256 commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YoussefAhmed256 commented Mar 2, 2026 •

edited

Loading