rishabhdaim commented on code in PR #2983:
URL: https://github.com/apache/jackrabbit-oak/pull/2983#discussion_r3496900331
##########
oak-http/src/test/java/org/apache/jackrabbit/oak/http/AuthorizationFieldTest.java:
##########
@@ -64,9 +64,73 @@ public void testNoScheme() throws LoginException {
@Test
public void testColonInPassword() throws LoginException {
String b64 = "Basic " +
Base64.getEncoder().encodeToString("foo:bar:qux".getBytes(StandardCharsets.UTF_8));
- SimpleCredentials credentials
=AuthorizationField.valueOf(Collections.enumeration(List.of(b64)));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of(b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar:qux", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testMissingColon() throws LoginException {
+ String b64 = "Basic " +
Base64.getEncoder().encodeToString("foobarqux".getBytes(StandardCharsets.UTF_8));
+ AuthorizationField.valueOf(Collections.enumeration(List.of(b64)));
+ }
+
+ @Test
+ public void testSchemeCase() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("BaSiC " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test
+ public void testMoreWhitespace() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testNonSpWhitespace() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic \t " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testBrokenBase64() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ // insert a single SP into the base64 sequence
+ b64 = b64.substring(0,5) + " " + b64.substring(5);
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic \t " + b64)));
+ assertEquals("foo", credentials.getUserID());
Review Comment:
Dead code — `@Test(expected=LoginException.class)` means JUnit exits on the
throw; these assertions are never reached. Same issue in `testNonSpWhitespace`
(lines 98–99), and pre-existing in `testInvalidBase64` (lines 54–55) and
`testMoreBrokenBase64` (lines 117–118).
##########
oak-http/src/test/java/org/apache/jackrabbit/oak/http/AuthorizationFieldTest.java:
##########
@@ -64,9 +64,73 @@ public void testNoScheme() throws LoginException {
@Test
public void testColonInPassword() throws LoginException {
String b64 = "Basic " +
Base64.getEncoder().encodeToString("foo:bar:qux".getBytes(StandardCharsets.UTF_8));
- SimpleCredentials credentials
=AuthorizationField.valueOf(Collections.enumeration(List.of(b64)));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of(b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar:qux", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testMissingColon() throws LoginException {
+ String b64 = "Basic " +
Base64.getEncoder().encodeToString("foobarqux".getBytes(StandardCharsets.UTF_8));
+ AuthorizationField.valueOf(Collections.enumeration(List.of(b64)));
+ }
+
+ @Test
+ public void testSchemeCase() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("BaSiC " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test
+ public void testMoreWhitespace() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testNonSpWhitespace() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic \t " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testBrokenBase64() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ // insert a single SP into the base64 sequence
+ b64 = b64.substring(0,5) + " " + b64.substring(5);
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic \t " + b64)));
Review Comment:
`"Basic \t " + b64` — the `\t` (0x09 < 0x20) fires the control-char guard
before the Base64 decoder is reached. This test exercises the same path as
`testNonSpWhitespace`, not broken base64. The `catch
(IllegalArgumentException)` branch in `AuthorizationField.java:69` is left
untested.
Fix: change prefix to `"Basic "`.
##########
oak-http/src/test/java/org/apache/jackrabbit/oak/http/AuthorizationFieldTest.java:
##########
@@ -64,9 +64,73 @@ public void testNoScheme() throws LoginException {
@Test
public void testColonInPassword() throws LoginException {
String b64 = "Basic " +
Base64.getEncoder().encodeToString("foo:bar:qux".getBytes(StandardCharsets.UTF_8));
- SimpleCredentials credentials
=AuthorizationField.valueOf(Collections.enumeration(List.of(b64)));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of(b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar:qux", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testMissingColon() throws LoginException {
+ String b64 = "Basic " +
Base64.getEncoder().encodeToString("foobarqux".getBytes(StandardCharsets.UTF_8));
+ AuthorizationField.valueOf(Collections.enumeration(List.of(b64)));
+ }
+
+ @Test
+ public void testSchemeCase() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("BaSiC " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test
+ public void testMoreWhitespace() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testNonSpWhitespace() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic \t " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testBrokenBase64() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ // insert a single SP into the base64 sequence
+ b64 = b64.substring(0,5) + " " + b64.substring(5);
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic \t " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test(expected = LoginException.class)
+ public void testMoreBrokenBase64() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
+ b64 = b64.substring(0,5) + "=" + b64.substring(5);
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic " + b64)));
+ assertEquals("foo", credentials.getUserID());
+ assertEquals("bar", new String(credentials.getPassword()));
+ }
+
+ @Test
+ public void testMoreNonAscii() throws LoginException {
+ String b64 =
Base64.getEncoder().encodeToString("test:123\u00a3".getBytes(StandardCharsets.UTF_8));
+ SimpleCredentials credentials =
AuthorizationField.valueOf(Collections.enumeration(List.of("Basic " + b64)));
+ assertEquals("test", credentials.getUserID());
+ assertEquals("123\u00a3", new String(credentials.getPassword()));
+ }
+
+ @Test
+ public void Basic64NoPadding() throws LoginException {
Review Comment:
nit: should be `testBase64NoPadding` — all other methods use the `testXxx`
naming convention.
##########
oak-http/src/main/java/org/apache/jackrabbit/oak/http/AuthorizationField.java:
##########
@@ -44,11 +45,30 @@ public static SimpleCredentials valueOf(Enumeration<String>
values) throws Login
return parseCredentials(field);
}
- private static SimpleCredentials parseCredentials(String fieldValue)
throws LoginException {
- if (fieldValue.startsWith("Basic ")) {
- String[] basic =
- Base64.decode(fieldValue.substring("Basic
".length())).split(":");
- return new SimpleCredentials(basic[0], basic[1].toCharArray());
+ private static SimpleCredentials parseCredentials(String rawFieldValue)
throws LoginException {
+ boolean hasControls = rawFieldValue.chars().anyMatch(c -> c < ' ');
+ if (hasControls) {
+ throw new LoginException("Control characters are not allowed");
+ }
+
+ String fieldValue = rawFieldValue.trim().replaceAll(" +", " ");
+
+ if (fieldValue.toLowerCase(Locale.ENGLISH).startsWith("basic ")) {
+ String token68 = fieldValue.substring("basic ".length());
+ try {
+ String decoded = new
String(Base64.getDecoder().decode(token68), StandardCharsets.UTF_8);
+ int colon = decoded.indexOf(':');
+ if (colon < 0) {
+ throw new LoginException(
+ "Malformed Basic credentials: missing ':'
separator");
+ }
+ String userId = decoded.substring(0, colon);
+ String password = decoded.substring(colon + 1);
+
+ return new SimpleCredentials(userId, password.toCharArray());
+ } catch (IllegalArgumentException ex) {
+ throw new LoginException(ex.getMessage());
Review Comment:
`ex.getMessage()` may include an attacker-controlled byte offset in server
logs (CWE-532). Use a static message:
```java
throw new LoginException("Invalid Base64 encoding in Basic credentials");
```
--
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]
