Repository navigation
Conversation
rmartinc
requested changes
May 18, 2026
Contributor
There was a problem hiding this comment.
@kosiorkosa47 There are failures in the testsuite. Probably now the double encoded should be moved to false here.
Initially I see this OK, but the test should be changed to make it pass OK.
Contributor
Author
|
CI caught a cross-module integration test that needed updating alongside the regex change. My local verification for the original commit was scoped to the Pushed Verification:
For future security-pattern changes I'll widen the local test scope to include Side note, not in scope of this PR: there's a near-identical |
Contributor
Author
|
@rmartinc Thanks for the review — your suggestion at L359-L362 matches the fix in 884a769506 that I pushed a few minutes earlier (we crossed in the air). Same intent, same lines. CI re-running on the new commit now. |
rmartinc
approved these changes
May 19, 2026
Contributor
There was a problem hiding this comment.
Thanks @kosiorkosa47! LGTM!
@mposolda do you want to review too?
Fixes keycloak#48978 Extends the regex to cover encoded forms that previously bypassed detection: - %3B / %3b (encoded semicolon) - %09, %0A, %0D, %00 (control characters) - %252E (double-encoded dot) These encodings do not produce actual path traversal on conformant servers per RFC 3986 (percent-encoded characters are literals, not delimiters), but are semantically close enough to the patterns the regex was designed to block to warrant defense-in-depth coverage. The end-of-input anchor ($) is moved into the terminator class to collapse the two pattern alternatives into one, keeping the diff minimal. Test changes: - 8 new assertions covering encoded semicolons, control character terminators, and double-encoded dots. - 3 prior assertEquals flipped to assertNull (lines that previously asserted %252E%252E/, %252E%252E/#fragment, and ..%3Bsomething/ were allowed are now expected to be blocked). - 1 new negative test confirming %3B as legitimate path content (not following a parent-folder sequence) still resolves. Triple-encoded variants (e.g., %25252E) remain allowed; out of scope for this issue. Signed-off-by: Michał Kosiorek <michal.kosiorek@arklink.co>
Follow-up to 36b0b10dd2 — Base IT (6) CI run 8000 for keycloak#49000 caught a cross-module integration test that needed updating alongside the regex change. Local verification of the previous commit covered the services module (RedirectUtilsTest); testsuite/integration-arquillian was outside that scope, so the existing OAuthRedirectUriTest.testWildcard expectations for %252E%252E variants didn't flip with the regex. Four assertions in testWildcard flipped from true → false to match the Option A semantic introduced in 36b0b10dd2 (double-encoded dots are now blocked by UNSAFE_PATH_PATTERN): http://example.com/foo/%252E%252E/ http://example.com/foo/%252E%252E/?some_query_param=some_value http://example.com/foo/%252E%252E/?encodeTest=a%3Cb http://example.com/foo/%252E%252E/#encodeTest=a%3Cb Triple-encoded (%25252E) and septuple-encoded variants remain expected:true — recursive decoding is explicitly out of scope for keycloak#48978. Verified locally: - mvn -pl services -Dtest=RedirectUtilsTest test → 11/11 green. - Direct regex match against the four flipped URIs confirms UNSAFE_PATH_PATTERN matches each rawPath, mechanically equivalent to the verifyRedirectUri code path exercised by the arquillian test. Refs keycloak#48978 Signed-off-by: Michał Kosiorek <michal.kosiorek@arklink.co>
rmartinc
force-pushed
the
fix/48978-unsafe-path-pattern-encoded-terminators
branch
from
May 21, 2026 08:42
884a769 to
7b26bd2
Compare
Contributor
|
I'm rebasing this, because there were other changes in the same area. @mposolda If everything goes OK I will merge it. Last chance to take a look. 😄 |
rmartinc
pushed a commit
to rmartinc/keycloak
that referenced
this pull request
May 26, 2026
keycloak#49000) * Tighten UNSAFE_PATH_PATTERN against encoded path-traversal terminators Fixes keycloak#48978 Extends the regex to cover encoded forms that previously bypassed detection: - %3B / %3b (encoded semicolon) - %09, %0A, %0D, %00 (control characters) - %252E (double-encoded dot) These encodin 8000 gs do not produce actual path traversal on conformant servers per RFC 3986 (percent-encoded characters are literals, not delimiters), but are semantically close enough to the patterns the regex was designed to block to warrant defense-in-depth coverage. The end-of-input anchor ($) is moved into the terminator class to collapse the two pattern alternatives into one, keeping the diff minimal. Test changes: - 8 new assertions covering encoded semicolons, control character terminators, and double-encoded dots. - 3 prior assertEquals flipped to assertNull (lines that previously asserted %252E%252E/, %252E%252E/#fragment, and ..%3Bsomething/ were allowed are now expected to be blocked). - 1 new negative test confirming %3B as legitimate path content (not following a parent-folder sequence) still resolves. Triple-encoded variants (e.g., %25252E) remain allowed; out of scope for this issue. Signed-off-by: Michał Kosiorek <michal.kosiorek@arklink.co> * Update OAuthRedirectUriTest expectations for double-encoded dots Follow-up to 36b0b10dd2 — Base IT (6) CI run for keycloak#49000 caught a cross-module integration test that needed updating alongside the regex change. Local verification of the previous commit covered the services module (RedirectUtilsTest); testsuite/integration-arquillian was outside that scope, so the existing OAuthRedirectUriTest.testWildcard expectations for %252E%252E variants didn't flip with the regex. Four assertions in testWildcard flipped from true → false to match the Option A semantic introduced in 36b0b10dd2 (double-encoded dots are now blocked by UNSAFE_PATH_PATTERN): http://example.com/foo/%252E%252E/ http://example.com/foo/%252E%252E/?some_query_param=some_value http://example.com/foo/%252E%252E/?encodeTest=a%3Cb http://example.com/foo/%252E%252E/#encodeTest=a%3Cb Triple-encoded (%25252E) and septuple-encoded variants remain expected:true — recursive decoding is explicitly out of scope for keycloak#48978. Verified locally: - mvn -pl services -Dtest=RedirectUtilsTest test → 11/11 green. - Direct regex match against the four flipped URIs confirms UNSAFE_PATH_PATTERN matches each rawPath, mechanically equivalent to the verifyRedirectUri code path exercised by the arquillian test. Refs keycloak#48978 Signed-off-by: Michał Kosiorek <michal.kosiorek@arklink.co> --------- Signed-off-by: Michał Kosiorek <michal.kosiorek@arklink.co> (cherry picked from commit c3f34c3)
ahus1
pushed a commit
that referenced
this pull request
May 26, 2026
Fixes #48978 Extends the regex to cover encoded forms that previously bypassed detection: - %3B / %3b (encoded semicolon) - %09, %0A, %0D, %00 (control characters) - %252E (double-encoded dot) These encodings do not produce actual path traversal on conformant servers per RFC 3986 (percent-encoded characters are literals, not delimiters), but are semantically close enough to the patterns the regex was designed to block to warrant defense-in-depth coverage. The end-of-input anchor ($) is moved into the terminator class to collapse the two pattern alternatives into one, keeping the diff minimal. Test changes: - 8 new assertions covering encoded semicolons, control character terminators, and double-encoded dots. - 3 prior assertEquals flipped to assertNull (lines that previously asserted %252E%252E/, %252E%252E/#fragment, and ..%3Bsomething/ were allowed are now expected to be blocked). - 1 new negative test confirming %3B as legitimate path content (not following a parent-folder sequence) still resolves. Triple-encoded variants (e.g., %25252E) remain allowed; out of scope for this issue. * Update OAuthRedirectUriTest expectations for double-encoded dots Follow-up to 36b0b10dd2 — Base IT (6) CI run for #49000 caught a cross-module integration test that needed updating alongside the regex change. Local verification of the previous commit covered the services module (RedirectUtilsTest); testsuite/integration-arquillian was outside that scope, so the existing OAuthRedirectUriTest.testWildcard expectations for %252E%252E variants didn't flip with the regex. Four assertions in testWildcard flipped from true → false to match the Option A semantic introduced in 36b0b10dd2 (double-encoded dots are now blocked by UNSAFE_PATH_PATTERN): http://example.com/foo/%252E%252E/ http://example.com/foo/%252E%252E/?some_query_param=some_value http://example.com/foo/%252E%252E/?encodeTest=a%3Cb http://example.com/foo/%252E%252E/#encodeTest=a%3Cb Triple-encoded (%25252E) and septuple-encoded variants remain expected:true — recursive decoding is explicitly out of scope for #48978. Verified locally: - mvn -pl services -Dtest=RedirectUtilsTest test → 11/11 green. - Direct regex match against the four flipped URIs confirms UNSAFE_PATH_PATTERN matches each rawPath, mechanically equivalent to the verifyRedirectUri code path exercised by the arquillian test. Refs #48978 --------- (cherry picked from commit c3f34c3) Signed-off-by: Michał Kosiorek <michal.kosiorek@arklink.co> Co-authored-by: Kosiorkosa47 <mkosiorek121@gmail.com>
Closed
sashyo
pushed a commit
to tide-foundation/keycloak-IGA
that referenced
this pull request
Jul 20, 2026
keycloak#49000) * Tighten UNSAFE_PATH_PATTERN against encoded path-traversal terminators Fixes keycloak#48978 Extends the regex to cover encoded forms that previously bypassed detection: - %3B / %3b (encoded semicolon) - %09, %0A, %0D, %00 (control characters) - %252E (double-encoded dot) These encodings do not produce actual path traversal on conformant servers per RFC 3986 (percent-encoded characters are literals, not delimiters), but are semantically close enough to the patterns the regex was designed to block to warrant defense-in-depth coverage. The end-of-input anchor ($) is moved into the terminator class to collapse the two pattern alternatives into one, keeping the diff minimal. Test changes: - 8 new assertions covering encoded semicolons, control character terminators, and double-encoded dots. - 3 prior assertEquals flipped to assertNull (lines that previously asserted %252E%252E/, %252E%252E/#fragment, and ..%3Bsomething/ were allowed are now expected to be blocked). - 1 new negative test confirming %3B as legitimate path content (not following a parent-folder sequence) still resolves. Triple-encoded variants (e.g., %25252E) remain allowed; out of scope for this issue. Signed-off-by: Michał Kosiorek <michal.kosiorek@arklink.co> * Update OAuthRedirectUriTest expectations for double-encoded dots Follow-up to 36b0b10dd2 — Base IT (6) CI run for keycloak#49000 caught a cross-module integration test that needed updating alongside the regex change. Local verification of the previous commit covered the services module (RedirectUtilsTest); testsuite/integration-arquillian was outside that scope, so the existing OAuthRedirectUriTest.testWildcard expectations for %252E%252E variants didn't flip with the regex. Four assertions in testWildcard flipped from true → false to match the Option A semantic introduced in 36b0b10dd2 (double-encoded dots are now blocked by UNSAFE_PATH_PATTERN): http://example.com/foo/%252E%252E/ http://example.com/foo/%252E%252E/?some_query_param=some_value http://example.com/foo/%252E%252E/?encodeTest=a%3Cb http://example.com/foo/%252E%252E/#encodeTest=a%3Cb Triple-encoded (%25252E) and septuple-encoded variants remain expected:true — recursive decoding is explicitly out of scope for keycloak#48978. Verified locally: - mvn -pl services -Dtest=RedirectUtilsTest test → 11/11 green. - Direct regex match against the four flipped URIs confirms UNSAFE_PATH_PATTERN matches each rawPath, mechanically equivalent to the verifyRedirectUri code path exercised by the arquillian test. Refs keycloak#48978 Signed-off-by: Michał Kosiorek <michal.kosiorek@arklink.co> --------- Signed-off-by: Michał Kosiorek <michal.kosiorek@arklink.co>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #48978
Summary
Extends
UNSAFE_PATH_PATTERNinRedirectUtilsto cover the percent-encodedterminators and double-encoded dots that were noted in the issue as gaps:
%3B/%3b(encoded semicolon)%09,%0A,%0D,%00(control characters)%252E(double-encoded dot)The end-of-input anchor
$is moved into the terminator alternation, whichcollapses the two pattern alternatives into one and keeps the diff small.
Why
These encoded forms do not produce actual path traversal on conformant
servers (per RFC 3986, percent-encoded characters in a path are literals,
not delimiters — confirmed empirically against Tomcat 9, nginx 1.27,
Apache httpd 2.4, Spring Boot 2.2, Node.js Express 5, and an nginx ↔ Tomcat
proxy chain). However, they're semantically close enough to the patterns
the regex was designed to block, and #48978 calls for the defense-in-depth
coverage.
Approach (Option A vs Option B)
This PR implements Option A from the issue — regex extension. It is
deliberately the smaller of the two suggested changes; canonical path
normalization (Option B) would be the more robust long-term answer but
involves broader code changes. Happy to do Option B as a follow-up PR if
preferred — let me know.
Test changes (
RedirectUtilsTest.testEncodedRedirectUri)8 new
assertNullassertions for the encoded forms now blocked:3 prior
assertEqualsflipped toassertNull— these patterns werepreviously allowed and are now blocked by design:
https://keycloak.org/test/%252E%252E/https://keycloak.org/test/%252E%252E/#encodeTest=a%3Cbhttps://keycloak.org/test/..%3Bsomething/1 new
assertEquals(negative test) confirming%3Bas legitimatepath content (not preceded by
..) still resolves:"https://keycloak.org/test/file%3Bname/"Triple-encoded variants (
%25252E) remain allowed — out of scopefor this issue, would need recursive decoding (Option B territory).
Trade-off note for review
..%3Bsomething/is now blocked because the regex matches..%3Bassubstring regardless of what follows (matching the existing
..;something/behaviour for the literal-semicolon form, which is alsoblocked — see line 258). If the maintainers prefer the more permissive
semantic ("only block when the terminator is immediately followed by
end-of-segment"), the regex can be tightened with a lookahead. Happy to
do that variant if it's the preferred direction.
Verification
Local run:
Full module:
No regressions in the
servicesmodule test suite.DCO
Signed off in commit (
Signed-off-by: Michał Kosiorek <…>).