Preserve query parameters when redirecting requests #40490
Conversation
0140f31 to
f1b7f30
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.cluster.RealmInvalidationClusterTest#crudWithFailoverKeycloak CI - Store IT (mssql) |
f1b7f30 to
3374b32
Compare
services/src/main/java/org/keycloak/services/resources/ThemeResource.java
Fixed
Show fixed
Hide fixed
3374b32 to
47f1845
Compare
Also hardening the redirect against uncommon URLs Closes keycloak#40489 Signed-off-by: Alexander Schwartz <alexander.schwartz@gmx.net>
47f1845 to
2896fb4
Compare
| base = base.substring(0, base.length() - 1); | ||
| if (!uriInfo.getRequestUri().toURL().getPath().startsWith(base + UriBuilder.fromResource(ThemeResource.class) | ||
| .path("/{version}/").build(version).getPath())) { | ||
| log.debugf("No URL encoding should be necessary for the version, returning a 404: %s", uriInfo.getRequestUri().getPath()); |
There was a problem hiding this comment.
My understanding is that RestEasy will always return the version in decoded form. I'm struggling to understand scenarios where we need this check, can you provide an example?
There was a problem hiding this comment.
I am trying to prevent situations where someone passes in strange versions in, which then lead to open an open or half-open redirect.
Imagine a version like myversion/../../auth/realm/xxx which is then url-encoded as myversion%2F..%2F..%2Fauth%2Frealm%2Fxxx and passed in as the version URL fragment.
So I'd say everything that is encoded is possibly someone trying to exploit it. And as a version, we only use characters and numbers at the moment. I could add that as a regex check in addition, or as a replacement.
Still, IMHO we need the encoding check for the path.
There was a problem hiding this comment.
Thanks for explaining.
Do we need this first check still given that we only redirect if (!version.equals(Version.RESOURCES_VERSION) && !hasContentHash), won't L123 protect against the scenario you describe?
There was a problem hiding this comment.
Yes and no: For that branch, it is. But then there is also the part in line 148, and the fallthrough where there is a content hash and the resource is returned to the caller starting in line 156. For that item, I also don't want to accept just any version.
There was a problem hiding this comment.
In L148 we're not checking against the version provided in the url though, we're only comparing the etag and Version.RESOURCES_VERSION. So I don't think an encoded path version can be exploited in this case as a 304 is sent.
Either way, happy to keep the check if you prefer. I do think a regex validating the version would be more explicit though. Adding some additional comments explaining why we're performing these checks would be nice for other contributors.
| .path("/{version}/{themeType}/{themeName}/{path}") | ||
| // The 'path' can contain slashes, so encoding of slashes is set to false | ||
| .build(new Object[]{Version.RESOURCES_VERSION, themeType, themeName, path}, false) | ||
| redirectUri |
There was a problem hiding this comment.
Nitpick, but we can fix the formatting here to be a one-liner or:
return Response
.temporaryRedirect(redirectUri)
.build();Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
|
Thank you. Your review comments should now be addressed. |
| // The 'path' can contain slashes, so encoding of slashes is set to false | ||
| .build(new Object[]{Version.RESOURCES_VERSION, themeType, themeName, path}, false) | ||
| ).build(); | ||
| return Response.temporaryRedirect(redirectUri) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
|
I see the CI is failing ... looking at it now. |
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
|
@ryanemerson - ok, test is no fixed. Sorry for the noise. Please re-review. Thanks! |
|
@mhajas - can you please approve based on Ryan's review? Thanks! |
Closes #40489