Repository navigation
fix: reject newlines in redirect locations - #19812
FrankChen021 merged 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens RedirectFilter against HTTP response splitting by rejecting redirect Location header values that contain literal carriage return (\r) or line feed (\n) characters after the redirect URL is fully constructed.
Changes:
- Add CR/LF validation for the redirect target prior to setting the
Locationresponse header (returning HTTP 400 on invalid values). - Preserve normal redirects and percent-encoded
%0D/%0Asequences without decoding them. - Add unit tests covering normal redirects, encoded newline sequences, and literal CR/LF rejection cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/src/main/java/org/apache/druid/server/http/RedirectFilter.java | Rejects redirect targets containing literal CR/LF before setting the Location header. |
| server/src/test/java/org/apache/druid/server/http/RedirectFilterTest.java | Adds tests for normal redirects, encoded newline preservation, and CR/LF rejection behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 2 of 2 changed files.
This is an automated review by Codex GPT-5.6-Sol
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
server/src/main/java/org/apache/druid/server/http/RedirectFilter.java:90
- The
validatedLocationreplacement is redundant because the code already returned on any\\r/\\n, sovalidatedLocationwill always be identical tolocation. Consider combining validation + 'SAST-friendly' sanitization into a single step (compute the replaced string first, then compare to the original and reject if changed). This removes dead/redundant logic while preserving the stated goal of making the validation obvious to security tooling.
final String location = url.toString();
if (location.indexOf('\r') >= 0 || location.indexOf('\n') >= 0) {
response.sendError(HttpServletResponse.SC_BAD_REQUEST);
return;
}
// String.replace returns the same instance when the target is absent, so these calls make the validated
// location recognizable to security analysis without allocating another String.
final String validatedLocation = location.replace('\r', ' ').replace('\n', ' ');
7623cb1 to
5ef9b4a
Compare
5ef9b4a to
c4e98d8
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 2 of 2 changed files. The previous baseline was unavailable locally; the full current diff was reviewed.
This is an automated review by Codex GPT-5.6-Luna(max)
What changed
Locationresponse header.%0D/%0AURL data.Why
RedirectFiltercopied a URL derived from request data directly into theLocationheader.java.net.URLpermits literal CR/LF characters, which could allow HTTP response splitting in servlet containers that do not reject them independently.Impact
Legitimate redirect targets retain their exact URL representation. Requests that would produce a raw newline in the response header are rejected before the response status or header is set.
Validation
mvn -ntp test -pl server -am -Dtest=org.apache.druid.server.http.RedirectFilterTest -Dsurefire.failIfNoSpecifiedTests=false -Pskip-static-checks -Dweb.console.skip=true -T1Cmvn -ntp test -pl server -Dtest=org.apache.druid.server.http.RedirectFilterTest -Dweb.console.skip=true -Pskip-static-checksmvn -ntp checkstyle:check -pl server -Dweb.console.skip=true