This is an automated email from the ASF dual-hosted git repository. reta pushed a commit to branch 3.6.x-fixes in repository https://gitbox.apache.org/repos/asf/cxf.git
commit 362aeab192a3d724e6a1280b8c5bea62076ba619 Author: Javid Khan <[email protected]> AuthorDate: Thu Jul 2 05:44:52 2026 +0530 validate redirect origin in oidc rp sign-in completion (#3241) * validate redirect origin in oidc rp sign-in completion Signed-off-by: dxbjavid <[email protected]> * harden redirect state in oidc rp filter rather than completion service * derive same-origin base from request uri info instead of message context * add filter test for state with scheme but no authority Signed-off-by: Javid Khan <[email protected]> --------- Signed-off-by: dxbjavid <[email protected]> Signed-off-by: Javid Khan <[email protected]> (cherry picked from commit d36cff46abfda921b72b15bd01116017cea4c5d7) --- rt/rs/security/sso/oidc/pom.xml | 6 ++ .../oidc/rp/OidcRpAuthenticationFilter.java | 25 +++++ .../oidc/rp/OidcRpAuthenticationFilterTest.java | 106 +++++++++++++++++++++ 3 files changed, 137 insertions(+) diff --git a/rt/rs/security/sso/oidc/pom.xml b/rt/rs/security/sso/oidc/pom.xml index b882d52f416..ad0cff2c102 100644 --- a/rt/rs/security/sso/oidc/pom.xml +++ b/rt/rs/security/sso/oidc/pom.xml @@ -62,6 +62,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> diff --git a/rt/rs/security/sso/oidc/src/main/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationFilter.java b/rt/rs/security/sso/oidc/src/main/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationFilter.java index 8f4add8a600..d99410609f3 100644 --- a/rt/rs/security/sso/oidc/src/main/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationFilter.java +++ b/rt/rs/security/sso/oidc/src/main/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationFilter.java @@ -116,8 +116,33 @@ public class OidcRpAuthenticationFilter implements ContainerRequestFilter { rc.setEntityStream(new ByteArrayInputStream(StringUtils.toBytesUTF8(body))); } + // The "state" carried here is read back by the sign-in completion service and returned + // as a redirect Location, so a caller-supplied value collides with the redirect query + // the filter itself writes. Anything that is not within this application's own origin is + // dropped, otherwise completion would become an open redirect. + String location = requestState.getFirst("state"); + if (location != null && !isSameOrigin(rc, location)) { + requestState.remove("state"); + } return requestState; } + private boolean isSameOrigin(ContainerRequestContext rc, String location) { + final URI uri; + try { + uri = URI.create(location); + } catch (IllegalArgumentException ex) { + return false; + } + if (uri.getScheme() == null && uri.getAuthority() == null) { + // a path-only reference is resolved by the browser against the current request + return true; + } + URI base = rc.getUriInfo().getAbsolutePath(); + return uri.getScheme() != null + && uri.getScheme().equalsIgnoreCase(base.getScheme()) + && uri.getAuthority() != null + && uri.getAuthority().equalsIgnoreCase(base.getAuthority()); + } public void setRedirectUri(String redirectUri) { this.redirectUri = redirectUri; } diff --git a/rt/rs/security/sso/oidc/src/test/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationFilterTest.java b/rt/rs/security/sso/oidc/src/test/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationFilterTest.java new file mode 100644 index 00000000000..8b2eeba7729 --- /dev/null +++ b/rt/rs/security/sso/oidc/src/test/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationFilterTest.java @@ -0,0 +1,106 @@ +/** + * 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.Method; +import java.net.URI; + +import jakarta.ws.rs.container.ContainerRequestContext; +import jakarta.ws.rs.core.MultivaluedHashMap; +import jakarta.ws.rs.core.MultivaluedMap; +import jakarta.ws.rs.core.UriInfo; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class OidcRpAuthenticationFilterTest { + + private static final URI ABSOLUTE_PATH = URI.create("https://app.example.com:8080/services/rp/complete";); + + @Test + public void testDropsCrossOriginState() { + MultivaluedMap<String, String> state = requestState("https://evil.example.com/phish";); + assertFalse(state.containsKey("state")); + } + + @Test + public void testDropsProtocolRelativeState() { + MultivaluedMap<String, String> state = requestState("//evil.example.com/phish"); + assertFalse(state.containsKey("state")); + } + + @Test + public void testDropsUserinfoHostConfusionState() { + MultivaluedMap<String, String> state = requestState("https://app.example.com:[email protected]/phish";); + assertFalse(state.containsKey("state")); + } + + @Test + public void testDropsNoAuthority() { + MultivaluedMap<String, String> state = requestState("http:/"); + assertFalse(state.containsKey("state")); + } + + @Test + public void testKeepsSameOriginState() { + MultivaluedMap<String, String> state = requestState("https://app.example.com:8080/services/protected";); + assertTrue(state.containsKey("state")); + assertEquals("https://app.example.com:8080/services/protected";, state.getFirst("state")); + } + + @Test + public void testKeepsRelativeState() { + MultivaluedMap<String, String> state = requestState("/services/protected"); + assertTrue(state.containsKey("state")); + assertEquals("/services/protected", state.getFirst("state")); + } + + private MultivaluedMap<String, String> requestState(String stateLocation) { + MultivaluedMap<String, String> query = new MultivaluedHashMap<>(); + query.putSingle("state", stateLocation); + + UriInfo uriInfo = mock(UriInfo.class); + when(uriInfo.getQueryParameters(true)).thenReturn(query); + when(uriInfo.getAbsolutePath()).thenReturn(ABSOLUTE_PATH); + + ContainerRequestContext rc = mock(ContainerRequestContext.class); + when(rc.getUriInfo()).thenReturn(uriInfo); + when(rc.getMediaType()).thenReturn(null); + + return invokeToRequestState(new OidcRpAuthenticationFilter(), rc); + } + + @SuppressWarnings("unchecked") + private static MultivaluedMap<String, String> invokeToRequestState(OidcRpAuthenticationFilter filter, + ContainerRequestContext rc) { + try { + Method method = OidcRpAuthenticationFilter.class.getDeclaredMethod("toRequestState", + ContainerRequestContext.class); + method.setAccessible(true); + return (MultivaluedMap<String, String>)method.invoke(filter, rc); + } catch (ReflectiveOperationException ex) { + throw new IllegalStateException(ex); + } + } +}
