Skip to content

Commit 67c78d9

Browse files
glasstigerclaude
andcommitted
Harden issuer-path scope and fix review nits
Follow-up to the OIDC device-flow review, four minor items: - isEndpointUnderIssuerPath now scans for an encoded path separator at every decode level, not just the raw string, and rejects a literal backslash. A split encoding such as %2%66 (which resolves to %2f then '/') and a backslash folded to '/' by decodePathSegments previously passed the single-pass pre-scan and could make a deeper endpoint masquerade as being under the pinned issuer path while a different raw path travelled on the wire. The origin pin already kept credentials on the trusted host, so this closes a defense-in-depth gap, not an exploit. - close()'s Javadoc no longer claims a hard one-HTTP-timeout bound: a DeviceCodePrompt that blocks in promptUser (the default browser launch) holds the lock while it runs, so a racing close() waits it out too. - testOutOfRangePollIntervalAndExpiryAreClamped now asserts the exact clamped values (60s interval, 1800s device-code lifetime) instead of bounds 5x and 2x looser than the real maxima. - Move the JsonLexer hasEscape field to its alphabetical position. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 15067f7 commit 67c78d9

3 files changed

Lines changed: 74 additions & 19 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/auth/OidcDeviceAuth.java

Lines changed: 45 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,9 @@
8989
* {@link OidcAuthException} rather than polling until the device code expires. Cancellation is seen
9090
* between polls (within ~100ms while waiting out an interval); a poll already in flight is not
9191
* interrupted, so the abort - and {@link #close()} - can take up to one HTTP request timeout (see
92-
* {@link Builder#httpTimeoutMillis(int)}), still far short of the device-code lifetime.
92+
* {@link Builder#httpTimeoutMillis(int)}), still far short of the device-code lifetime (a
93+
* {@link DeviceCodePrompt} that blocks in {@code promptUser}, such as the default browser launch, can
94+
* extend that wait by however long it runs).
9395
* <p>
9496
* Instances are interactive and hold a network connection; close them when done. Token state is
9597
* in-memory only and does not survive a process restart.
@@ -380,8 +382,12 @@ public void clearCache() {
380382
* between polls (within ~100ms while waiting out a poll interval); a poll request already in flight
381383
* is not interrupted, so {@code close()} acquires the lock - and returns - only once that request
382384
* finishes or times out, i.e. after at most one HTTP request timeout
383-
* (see {@link Builder#httpTimeoutMillis(int)}), not the full device-code lifetime. Idempotent. After
384-
* close, {@link #getToken()} and {@link #clearCache()} throw.
385+
* (see {@link Builder#httpTimeoutMillis(int)}), not the full device-code lifetime. The exception is a
386+
* {@link DeviceCodePrompt} that blocks in {@code promptUser} - for example the default
387+
* {@link DeviceCodePrompt#openBrowser()} prompt while it hands the verification URL to the OS browser,
388+
* which is not bounded by the HTTP timeout: the flow holds the lock across that one-off prompt, so a
389+
* racing {@code close()} waits it out too. Idempotent. After close, {@link #getToken()} and
390+
* {@link #clearCache()} throw.
385391
*/
386392
@Override
387393
public void close() {
@@ -574,6 +580,37 @@ private static OidcAuthException endpointNotUnderIssuer(String label, String url
574580
.put("explicitly with OidcDeviceAuth.builder()");
575581
}
576582

583+
private static boolean endpointPathHasEncodedSeparator(String rawEndpointPath) {
584+
// Scan for a literal backslash (decodePathSegments folds it to '/') or a percent-encoded path
585+
// separator - %2f ('/'), %5c ('\'), or an encoded percent %25 that gates a split or double encoding
586+
// such as %2%66 or %252f - at every decode level, not just the raw string. A separator that only
587+
// emerges after the server unescapes more than once would pass a single-pass scan yet split one
588+
// segment in two, letting .../realms/acme%2%66evil/token slip the issuer-path scope. A real OIDC
589+
// endpoint path encodes none of these. Bounded like decodePathSegments; a real path needs 0-1 passes.
590+
String decoded = rawEndpointPath;
591+
for (int pass = 0; pass < 10; pass++) {
592+
for (int i = 0, n = decoded.length(); i < n; i++) {
593+
char c = decoded.charAt(i);
594+
if (c == '\\') {
595+
return true;
596+
}
597+
if (c == '%' && i + 2 < n) {
598+
char a = decoded.charAt(i + 1);
599+
char b = decoded.charAt(i + 2);
600+
if ((a == '2' && (b == 'f' || b == 'F' || b == '5')) || (a == '5' && (b == 'c' || b == 'C'))) {
601+
return true;
602+
}
603+
}
604+
}
605+
String next = percentDecodeOnce(decoded);
606+
if (next.equals(decoded)) {
607+
break;
608+
}
609+
decoded = next;
610+
}
611+
return false;
612+
}
613+
577614
private static void fetchJson(Endpoint endpoint, String path, ClientTlsConfiguration tlsConfig, JsonParser parser, String reachError, String parseError) {
578615
HttpClient client = endpoint.isTls
579616
? HttpClientFactory.newTlsInstance(HTTP_CONFIG, tlsConfig)
@@ -661,18 +698,11 @@ private static boolean isEndpointUnderIssuerPath(String endpointUrl, String issu
661698
}
662699
String[] baseSegs = decodePathSegments(basePath.substring(0, baseEnd));
663700
String rawEndpointPath = pathOnly(endpointUrl);
664-
// reject a percent-encoded path separator - %2f ('/'), %5c ('\'), or a double-encoded form flagged by
665-
// an encoded percent %25 (e.g. %252f). decodePathSegments resolves it before the segment comparison,
666-
// so it would split one path segment in two and could let .../realms/acme%2fevil/token slip the
667-
// issuer-path scope. A real OIDC endpoint path never encodes a separator.
668-
for (int i = 0, n = rawEndpointPath.length() - 2; i < n; i++) {
669-
if (rawEndpointPath.charAt(i) == '%') {
670-
char a = rawEndpointPath.charAt(i + 1);
671-
char b = rawEndpointPath.charAt(i + 2);
672-
if ((a == '2' && (b == 'f' || b == 'F' || b == '5')) || (a == '5' && (b == 'c' || b == 'C'))) {
673-
return false;
674-
}
675-
}
701+
// A real OIDC endpoint path never encodes a path separator or uses a backslash; reject either before
702+
// the segment comparison, since decodePathSegments resolves them and would split one path segment in
703+
// two, letting .../realms/acme%2fevil/token (or its split/backslash forms) slip the issuer-path scope.
704+
if (endpointPathHasEncodedSeparator(rawEndpointPath)) {
705+
return false;
676706
}
677707
String[] endpointSegs = decodePathSegments(rawEndpointPath);
678708
// a "." or ".." segment is rejected outright: the server normalizes it away, so a naive prefix test

core/src/main/java/io/questdb/client/cutlass/json/JsonLexer.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,11 +60,11 @@ public class JsonLexer implements Mutable, Closeable {
6060
private long cache;
6161
private int cacheCapacity;
6262
private int cacheSize = 0;
63+
private boolean hasEscape = false;
6364
private boolean ignoreNext = false;
6465
private int objDepth = 0;
6566
private int position = 0;
6667
private boolean quoted = false;
67-
private boolean hasEscape = false;
6868
private int state = S_START;
6969
private boolean useCache = false;
7070

core/src/test/java/io/questdb/client/test/cutlass/auth/OidcDeviceAuthTest.java

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1918,6 +1918,21 @@ public void testIssuerPathScopingRejectsSiblingRealm() throws Exception {
19181918
});
19191919
}
19201920

1921+
@Test(timeout = 30_000)
1922+
public void testIssuerPathScopingRejectsSplitEncodedAndBackslashSeparators() throws Exception {
1923+
// hardening: an encoded path separator can hide behind a SPLIT encoding (%2%66 -> %2f -> '/') or a
1924+
// double encoding (%252f), and a literal backslash is folded to '/' by decodePathSegments. Each lets an
1925+
// extra segment masquerade as being under the issuer path while a different raw path travels on the
1926+
// wire, so isEndpointUnderIssuerPath must reject them. Only the path is scoped; the origin matches here.
1927+
String issuer = "https://idp.example.com/realms/acme";
1928+
// a genuine sub-path endpoint stays accepted
1929+
Assert.assertTrue(invokeIsEndpointUnderIssuerPath("https://idp.example.com/realms/acme/protocol/token", issuer));
1930+
// split, double, and literal-backslash separators all resolve to a deeper /realms/acme/evil and are rejected
1931+
Assert.assertFalse(invokeIsEndpointUnderIssuerPath("https://idp.example.com/realms/acme%2%66evil/token", issuer));
1932+
Assert.assertFalse(invokeIsEndpointUnderIssuerPath("https://idp.example.com/realms/acme%252fevil/token", issuer));
1933+
Assert.assertFalse(invokeIsEndpointUnderIssuerPath("https://idp.example.com/realms/acme\\evil/token", issuer));
1934+
}
1935+
19211936
@Test(timeout = 30_000)
19221937
public void testLargeSplitTokenValueParsesWithConfiguredLexerSizing() throws Exception {
19231938
assertMemoryLeak(() -> {
@@ -2230,9 +2245,10 @@ public void testOutOfRangePollIntervalAndExpiryAreClamped() throws Exception {
22302245
Assert.assertEquals("ACCESS-CLAMP", auth.getToken());
22312246
DeviceAuthorizationChallenge challenge = shown.get();
22322247
Assert.assertNotNull(challenge);
2233-
// the absurd interval/expires_in are clamped to the documented maxima
2234-
Assert.assertTrue("interval=" + challenge.getIntervalSeconds(), challenge.getIntervalSeconds() <= 300);
2235-
Assert.assertTrue("expiresIn=" + challenge.getExpiresInSeconds(), challenge.getExpiresInSeconds() <= 3600);
2248+
// the absurd interval/expires_in are clamped to the documented maxima: the poll interval to
2249+
// MAX_POLL_INTERVAL_SECONDS (60) and the device-code lifetime to MAX_DEVICE_CODE_TTL_SECONDS (1800)
2250+
Assert.assertEquals(60, challenge.getIntervalSeconds());
2251+
Assert.assertEquals(1800, challenge.getExpiresInSeconds());
22362252
}
22372253
});
22382254
}
@@ -3180,6 +3196,15 @@ private static long readExpiresAtMillis(OidcDeviceAuth auth) throws Exception {
31803196
return f.getLong(auth);
31813197
}
31823198

3199+
// isEndpointUnderIssuerPath is a private static security check (it scopes a /settings-advertised endpoint
3200+
// to the pinned issuer's path); the client is an open module, so reflection reaches it without widening
3201+
// production visibility for the test
3202+
private static boolean invokeIsEndpointUnderIssuerPath(String endpointUrl, String issuer) throws Exception {
3203+
Method m = OidcDeviceAuth.class.getDeclaredMethod("isEndpointUnderIssuerPath", String.class, String.class);
3204+
m.setAccessible(true);
3205+
return (boolean) m.invoke(null, endpointUrl, issuer);
3206+
}
3207+
31833208
// isLoopbackHost is a private static security classifier (it gates the plaintext-channel MITM pin); the
31843209
// client is an open module, so reflection reaches it without widening production visibility for the test
31853210
private static boolean invokeIsLoopbackHost(String host) throws Exception {

0 commit comments

Comments
 (0)