From dcfeeeb35427bad5e2d70cbc114c86e0da191db2 Mon Sep 17 00:00:00 2001 From: Colm O hEigeartaigh Date: Fri, 22 May 2026 11:20:13 +0100 Subject: [PATCH] Removing control characters from OAuth2 realm and logs --- .../oauth2/services/AbstractTokenService.java | 5 +- .../oauth2/utils/AuthorizationUtils.java | 6 +- .../services/AbstractTokenServiceTest.java | 130 ++++++++++++++++++ .../oauth2/utils/AuthorizationUtilsTest.java | 88 ++++++++++++ .../src/test/resources/logging.properties | 74 ++++++++++ 5 files changed, 300 insertions(+), 3 deletions(-) create mode 100644 rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenServiceTest.java create mode 100644 rt/rs/security/oauth-parent/oauth2/src/test/resources/logging.properties diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenService.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenService.java index b9fa880fece..78c844d360a 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenService.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenService.java @@ -254,17 +254,18 @@ protected Client getClient(String clientId, String clientSecret, MultivaluedMap< return null; } Client client = null; + String sanitizedClientId = AuthorizationUtils.stripControlCharacters(clientId); try { client = getValidClient(clientId, clientSecret, params); } catch (OAuthServiceException ex) { - LOG.warning("No valid client found for clientId: " + clientId); + LOG.warning("No valid client found for clientId: " + sanitizedClientId); if (ex.getError() != null) { reportInvalidClient(ex.getError()); return null; } } if (client == null) { - LOG.warning("No valid client found for clientId: " + clientId); + LOG.warning("No valid client found for clientId: " + sanitizedClientId); reportInvalidClient(); } return client; diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/utils/AuthorizationUtils.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/utils/AuthorizationUtils.java index 24d5bcf3160..7e0c7e0a384 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/utils/AuthorizationUtils.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/utils/AuthorizationUtils.java @@ -106,7 +106,7 @@ public static void throwAuthorizationFailure(Set challenges, String real } if (sb.length() > 0) { if (realm != null) { - sb.append(" realm=\"").append(realm).append('"'); + sb.append(" realm=\"").append(stripControlCharacters(realm)).append('"'); } rb.header(HttpHeaders.WWW_AUTHENTICATE, sb.toString()); } @@ -115,4 +115,8 @@ public static void throwAuthorizationFailure(Set challenges, String real } throw ExceptionUtils.toNotAuthorizedException(cause, rb.build()); } + + public static String stripControlCharacters(String value) { + return value.replaceAll("[\\p{Cntrl}&&[^\\t]]", "_"); + } } diff --git a/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenServiceTest.java b/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenServiceTest.java new file mode 100644 index 00000000000..4e56b366987 --- /dev/null +++ b/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenServiceTest.java @@ -0,0 +1,130 @@ +/** + * 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.oauth2.services; + +import jakarta.ws.rs.core.MultivaluedMap; +import org.apache.cxf.jaxrs.impl.MetadataMap; +import org.apache.cxf.rs.security.oauth2.common.Client; +import org.apache.cxf.rs.security.oauth2.common.OAuthError; +import org.apache.cxf.rs.security.oauth2.provider.OAuthServiceException; +import org.apache.cxf.rs.security.oauth2.utils.OAuthConstants; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; + +public class AbstractTokenServiceTest { + + @Test + public void testGetClientReportsInvalidClientWhenClientNotFound() { + TestTokenService service = new TestTokenService(); + + InvalidClientException ex = assertThrows(InvalidClientException.class, + () -> service.callGetClient("missing-client", "secret", new MetadataMap<>())); + + assertNotNull(ex.getError()); + assertEquals(OAuthConstants.INVALID_CLIENT, ex.getError().getError()); + } + + @Test + public void testInvalidClientInvalidCharactersHandled() { + TestTokenService service = new TestTokenService(); + + String clientId = "alice\r\n\r\n\r\n[FAKE]+Admin+login+successful"; + InvalidClientException ex = assertThrows(InvalidClientException.class, + () -> service.callGetClient(clientId, "secret", new MetadataMap<>())); + + assertNotNull(ex.getError()); + assertEquals(OAuthConstants.INVALID_CLIENT, ex.getError().getError()); + } + + @Test + public void testGetClientReportsCustomErrorWhenProviderThrowsIt() { + TestTokenService service = new TestTokenService(); + OAuthError customError = new OAuthError(OAuthConstants.UNAUTHORIZED_CLIENT); + service.setException(new OAuthServiceException(customError)); + + InvalidClientException ex = assertThrows(InvalidClientException.class, + () -> service.callGetClient("client", "secret", new MetadataMap<>())); + + assertSame(customError, ex.getError()); + } + + @Test + public void testGetClientReturnsClientWhenFound() { + TestTokenService service = new TestTokenService(); + Client expected = new Client("client", "secret", true); + service.setClient(expected); + + Client actual = service.callGetClient("client", "secret", new MetadataMap<>()); + assertSame(expected, actual); + } + + private static final class TestTokenService extends AbstractTokenService { + private Client client; + private OAuthServiceException exception; + + Client callGetClient(String clientId, String clientSecret, MultivaluedMap params) { + return getClient(clientId, clientSecret, params); + } + + void setClient(Client client) { + this.client = client; + } + + void setException(OAuthServiceException exception) { + this.exception = exception; + } + + @Override + protected Client getValidClient(String clientId, String clientSecret, MultivaluedMap params) + throws OAuthServiceException { + if (exception != null) { + throw exception; + } + return client; + } + + @Override + protected void reportInvalidClient() { + throw new InvalidClientException(new OAuthError(OAuthConstants.INVALID_CLIENT)); + } + + @Override + protected void reportInvalidClient(OAuthError error) { + throw new InvalidClientException(error); + } + } + + private static class InvalidClientException extends RuntimeException { + private static final long serialVersionUID = 1L; + private final OAuthError error; + + InvalidClientException(OAuthError error) { + this.error = error; + } + + OAuthError getError() { + return error; + } + } +} \ No newline at end of file diff --git a/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/utils/AuthorizationUtilsTest.java b/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/utils/AuthorizationUtilsTest.java index f2e2864fd38..9f0cc0be46d 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/utils/AuthorizationUtilsTest.java +++ b/rt/rs/security/oauth-parent/oauth2/src/test/java/org/apache/cxf/rs/security/oauth2/utils/AuthorizationUtilsTest.java @@ -19,6 +19,7 @@ package org.apache.cxf.rs.security.oauth2.utils; import java.util.Arrays; +import java.util.Base64; import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashSet; @@ -100,6 +101,35 @@ public void testThrowAuthorizationFailureWithCause() { } } + @Test + public void testThrowAuthorizationFailureWithRealm() { + try { + AuthorizationUtils.throwAuthorizationFailure(Collections.singleton("Basic"), "oauth"); + fail("WebApplicationException expected"); + } catch (WebApplicationException ex) { + Response r = ex.getResponse(); + assertEquals(401, r.getStatus()); + String value = r.getHeaderString(HttpHeaders.WWW_AUTHENTICATE); + assertNotNull(value); + assertEquals("Basic realm=\"oauth\"", value); + } + } + + @Test + public void testThrowAuthorizationFailureWithRealmInvalidCharacters() { + String realm = "oauth\r\n\r\n\r\nX-Injected: true"; + try { + AuthorizationUtils.throwAuthorizationFailure(Collections.singleton("Basic"), realm); + fail("WebApplicationException expected"); + } catch (WebApplicationException ex) { + Response r = ex.getResponse(); + assertEquals(401, r.getStatus()); + String value = r.getHeaderString(HttpHeaders.WWW_AUTHENTICATE); + assertNotNull(value); + assertEquals("Basic realm=" + "\"" + AuthorizationUtils.stripControlCharacters(realm) + "\"", value); + } + } + @Test public void getAuthorizationParts() { String type = "type"; @@ -123,4 +153,62 @@ public void getAuthorizationParts() { AuthorizationUtils.getAuthorizationParts(mc, new HashSet<>(Arrays.asList("another", type)))); } + @Test + public void testGetAuthorizationPartsWithMultipleHeadersFails() { + Message m = new MessageImpl(); + m.put(Message.PROTOCOL_HEADERS, Collections.singletonMap(HttpHeaders.AUTHORIZATION, + Arrays.asList("Basic YWxpY2U6c2VjcmV0", "Bearer token"))); + MessageContext mc = new MessageContextImpl(m); + + try { + AuthorizationUtils.getAuthorizationParts(mc); + fail("WebApplicationException expected"); + } catch (WebApplicationException ex) { + assertEquals(401, ex.getResponse().getStatus()); + } + } + + @Test + public void testGetBasicAuthParts() { + String basicAuthData = Base64.getEncoder().encodeToString("alice:secret".getBytes()); + + String[] parts = AuthorizationUtils.getBasicAuthParts(basicAuthData); + assertArrayEquals(new String[] {"alice", "secret"}, parts); + } + + @Test + public void testGetBasicAuthPartsInvalidBase64() { + try { + AuthorizationUtils.getBasicAuthParts("not-base64"); + fail("WebApplicationException expected"); + } catch (WebApplicationException ex) { + assertEquals(401, ex.getResponse().getStatus()); + } + } + + @Test + public void testGetBasicAuthPartsMissingSeparator() { + String basicAuthData = Base64.getEncoder().encodeToString("alice".getBytes()); + + try { + AuthorizationUtils.getBasicAuthParts(basicAuthData); + fail("WebApplicationException expected"); + } catch (WebApplicationException ex) { + assertEquals(401, ex.getResponse().getStatus()); + } + } + + @Test + public void testGetBasicAuthUserInfo() { + String basicAuthData = Base64.getEncoder().encodeToString("alice:secret".getBytes()); + + Message m = new MessageImpl(); + m.put(Message.PROTOCOL_HEADERS, Collections.singletonMap(HttpHeaders.AUTHORIZATION, + Collections.singletonList("Basic " + basicAuthData))); + MessageContext mc = new MessageContextImpl(m); + + String[] userInfo = AuthorizationUtils.getBasicAuthUserInfo(mc); + assertArrayEquals(new String[] {"alice", "secret"}, userInfo); + } + } diff --git a/rt/rs/security/oauth-parent/oauth2/src/test/resources/logging.properties b/rt/rs/security/oauth-parent/oauth2/src/test/resources/logging.properties new file mode 100644 index 00000000000..4889366a3ab --- /dev/null +++ b/rt/rs/security/oauth-parent/oauth2/src/test/resources/logging.properties @@ -0,0 +1,74 @@ +# +# +# 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. +# +# +############################################################ +# Default Logging Configuration File +# +# You can use a different file by specifying a filename +# with the java.util.logging.config.file system property. +# For example java -Djava.util.logging.config.file=myfile +############################################################ + +############################################################ +# Global properties +############################################################ + +# "handlers" specifies a comma separated list of log Handler +# classes. These handlers will be installed during VM startup. +# Note that these classes must be on the system classpath. +# By default we only configure a ConsoleHandler, which will only +# show messages at the INFO and above levels. +#handlers= java.util.logging.ConsoleHandler + +# To also add the FileHandler, use the following line instead. +#handlers= java.util.logging.FileHandler, java.util.logging.ConsoleHandler + +# Default global logging level. +# This specifies which kinds of events are logged across +# all loggers. For any given facility this global level +# can be overriden by a facility specific level +# Note that the ConsoleHandler also has a separate level +# setting to limit messages printed to the console. +.level= INFO + +############################################################ +# Handler specific properties. +# Describes specific configuration info for Handlers. +############################################################ + +# default file output is in user's home directory. +java.util.logging.FileHandler.pattern = %h/java%u.log +java.util.logging.FileHandler.limit = 50000 +java.util.logging.FileHandler.count = 1 +java.util.logging.FileHandler.formatter = java.util.logging.XMLFormatter + +# Limit the message that are printed on the console to INFO and above. +java.util.logging.ConsoleHandler.level = INFO +java.util.logging.ConsoleHandler.formatter = java.util.logging.SimpleFormatter + + +############################################################ +# Facility specific properties. +# Provides extra control for each logger. +############################################################ + +# For example, set the com.xyz.foo logger to only log SEVERE +# messages: +#com.xyz.foo.level = SEVERE