dxbjavid commented on code in PR #3241:
URL: https://github.com/apache/cxf/pull/3241#discussion_r3460294388
##########
rt/rs/security/sso/oidc/src/main/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationService.java:
##########
@@ -57,18 +57,42 @@ public Response completeAuthentication(@Context
OidcClientTokenContext oidcConte
URI redirectUri = null;
MultivaluedMap<String, String> state = oidcContext.getState();
String location = state != null ? state.getFirst("state") : null;
- if (location == null && defaultLocation != null) {
+ if (location != null) {
Review Comment:
you're right that it isn't an OIDC concept, the spec treats state as opaque.
this is cxf's own reuse of the parameter name rather than anything from the RP
specs.
it comes from OidcRpAuthenticationFilter: when addRequestUriAsRedirectQuery
is set, the filter writes the original request uri into a query parameter it
happens to call state (redirectBuilder.queryParam("state",
rc.getUriInfo().getRequestUri().toString())), and completeAuthentication later
reads that same state back to send the browser to where it was originally
heading. so in normal use the value is always the application's own request
uri, which is why the legitimate case is same-origin.
the problem is that toRequestState copies every current request parameter
into the context state, so /rp/complete?state=https://evil.example against an
already-authenticated session is read back verbatim and returned as a 303
Location, an open redirect. the origin check just keeps that redirect within
the application's own scheme and authority. if you'd rather the validation
lived in the filter where the value is first written, i'm happy to move it
there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
