Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion rt/rs/security/sso/oidc/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
</cxf.osgi.import>
<!-- this configures the surefire plugin to run your tests with the javaagent enabled -->
<!-- (openJPA loadtime weaving) -->
<cxf.surefire.fork.vmargs.extra>-javaagent:${org.apache.openjpa:openjpa:jar}</cxf.surefire.fork.vmargs.extra>
<cxf.surefire.fork.vmargs.extra>-javaagent:${org.apache.openjpa:openjpa:jar} -javaagent:${org.mockito:mockito-core:jar}</cxf.surefire.fork.vmargs.extra>
</properties>
<dependencies>
<dependency>
Expand All @@ -64,6 +64,12 @@
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>${cxf.mockito.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.hsqldb</groupId>
<artifactId>hsqldb</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

@reta reta Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coheigea need your advice please, the usage of state for redirect URL looks very suspicious, I haven't found any references in OIDC RP specs of such designation, is it correct or I am missing something? thank you

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. if you'd rather the validation lived in the filter where the value is first written, i'm happy to move it there.

Oh thanks for clarifying @dxbjavid , I think hardening filter would make sense, also making sure the state is not colliding with the request state (may be filtering here will help), thanks again

URI requestedUri = URI.create(UrlUtils.urlDecode(location));
if (isSameOrigin(requestedUri)) {
redirectUri = requestedUri;
}
}
if (redirectUri == null && defaultLocation != null) {
String basePath = (String)mc.get("http.base.path");
redirectUri = UriBuilder.fromUri(basePath).path(defaultLocation).build();
} else if (location != null) {
redirectUri = URI.create(UrlUtils.urlDecode(location));
}
if (redirectUri != null) {
return Response.seeOther(redirectUri).build();
}
return Response.ok(oidcContext).build();
}

// The location is taken from the request state, so it can only be trusted as long as it
// stays within this application's own origin. An absolute value pointing at a different
// host (or a protocol-relative "//host" reference) would turn sign-in completion into an
// open redirect, so anything that is not same-origin is ignored here.
private boolean isSameOrigin(URI location) {
if (location.getScheme() == null && location.getAuthority() == null) {
// a path-only reference is resolved by the browser against the current request
return true;
}
String basePath = (String)mc.get("http.base.path");
if (basePath == null) {
return false;
}
URI base = URI.create(basePath);
return location.getScheme() != null
&& location.getScheme().equalsIgnoreCase(base.getScheme())
&& location.getAuthority() != null
&& location.getAuthority().equalsIgnoreCase(base.getAuthority());
}

public void setDefaultLocation(String defaultLocation) {
this.defaultLocation = defaultLocation;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.cxf.rs.security.oidc.rp;

import java.lang.reflect.Field;
import java.net.URI;

import jakarta.ws.rs.core.MultivaluedHashMap;
import jakarta.ws.rs.core.MultivaluedMap;
import jakarta.ws.rs.core.Response;

import org.apache.cxf.jaxrs.ext.MessageContext;
import org.apache.cxf.rs.security.oauth2.client.ClientTokenContextManager;

import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class OidcRpAuthenticationServiceTest {

private static final String BASE_PATH = "https://app.example.com:8080/services/";

@Test
public void testRejectsCrossOriginRedirectFromState() {
Response response = complete("https://evil.example.com/phish");
assertEquals(200, response.getStatus());
assertNull(response.getLocation());
}

@Test
public void testRejectsProtocolRelativeRedirectFromState() {
Response response = complete("//evil.example.com/phish");
assertEquals(200, response.getStatus());
assertNull(response.getLocation());
}

@Test
public void testRejectsUserinfoHostConfusionFromState() {
Response response = complete("https://app.example.com:8080@evil.example.com/phish");
assertEquals(200, response.getStatus());
assertNull(response.getLocation());
}

@Test
public void testAllowsSameOriginRedirectFromState() {
Response response = complete("https://app.example.com:8080/services/protected");
assertEquals(303, response.getStatus());
assertEquals(URI.create("https://app.example.com:8080/services/protected"), response.getLocation());
}

@Test
public void testAllowsRelativeRedirectFromState() {
Response response = complete("/services/protected");
assertEquals(303, response.getStatus());
assertEquals(URI.create("/services/protected"), response.getLocation());
}

private Response complete(String stateLocation) {
MessageContext messageContext = mock(MessageContext.class);
when(messageContext.get("http.base.path")).thenReturn(BASE_PATH);

MultivaluedMap<String, String> state = new MultivaluedHashMap<>();
state.putSingle("state", stateLocation);

OidcClientTokenContext tokenContext = mock(OidcClientTokenContext.class);
when(tokenContext.getState()).thenReturn(state);

OidcRpAuthenticationService service = new OidcRpAuthenticationService();
service.setClientTokenContextManager(mock(ClientTokenContextManager.class));
setMessageContext(service, messageContext);

return service.completeAuthentication(tokenContext);
}

private static void setMessageContext(OidcRpAuthenticationService service, MessageContext messageContext) {
try {
Field field = OidcRpAuthenticationService.class.getDeclaredField("mc");
field.setAccessible(true);
field.set(service, messageContext);
} catch (ReflectiveOperationException ex) {
throw new IllegalStateException(ex);
}
}
}
Loading