Skip to content

Commit 0493b4c

Browse files
glasstigerclaude
andcommitted
Sanitize HTTP status and probe text in errors
The PR sanitized the JSON error body a failed flush renders into a LineSenderException, but two adjacent untrusted-text paths in the same file still reached the message raw, so a hostile or proxied endpoint could splice ANSI/control/bidi bytes into a log line or terminal: - The "[http-status=...]" field was rendered with put(), not putAsPrintable(). The HTTP header parser copies the status-line token verbatim between the two spaces, and a non-3-char token bypasses the numeric status checks to reach the generic error path, so a crafted status line could carry an ESC. Route all four status renders through putAsPrintable(); the generic path is the exploitable one, the others are defensive. A normal 3-digit status renders unchanged. - Protocol-version detection concatenated the raw probe response body via String +. Build it through putAsPrintable() instead. Tests: - LineHttpSenderErrorResponseTest: a malformed status line carrying an ESC, and a protocol-probe error body with ESC/bidi/newline; both fail without the fix and pass with it. - DisplaySafeTest: direct coverage for the shared classifier (controls, format/bidi/BOM, supplementary tag chars and lone surrogates unsafe; printable ASCII and emoji safe) - it previously had none. - SenderBuilderErrorApiTest: the null-provider guard and the provider-then-username/password exclusion, neither tested before. - OidcDeviceAuthTest: a percent-encoded backslash (%5c/%5C) in the issuer-path scope check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c35d931 commit 0493b4c

5 files changed

Lines changed: 210 additions & 5 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/line/http/AbstractLineHttpSender.java

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,9 @@ public static AbstractLineHttpSender createLineSender(
334334
if (protocolVersion == PROTOCOL_VERSION_NOT_SET_EXPLICIT) {
335335
Misc.free(cli);
336336
if (lastErrorSink != null) {
337-
throw new LineSenderException("Failed to detect server line protocol version: " + lastErrorSink);
337+
// sanitize the raw server body before it reaches the exception message (and any log/terminal):
338+
// a hostile or proxied endpoint must not splice control, ANSI or bidi chars into the render
339+
throw new LineSenderException("Failed to detect server line protocol version: ").putAsPrintable(lastErrorSink);
338340
}
339341
throw new LineSenderException("Failed to detect server line protocol version");
340342
}
@@ -841,7 +843,7 @@ private void throwOnHttpErrorResponse(DirectUtf8Sequence statusCode, HttpClient.
841843
// an untrusted or proxied endpoint must not splice control, ANSI or bidi chars into the render
842844
ex = ex.put(": ").putAsPrintable(sink);
843845
}
844-
ex.put(" [http-status=").put(statusAscii).put(']');
846+
ex.put(" [http-status=").putAsPrintable(statusAscii).put(']');
845847
client.disconnect();
846848
throw ex;
847849
}
@@ -862,7 +864,7 @@ private void throwOnHttpErrorResponse(DirectUtf8Sequence statusCode, HttpClient.
862864
// an untrusted or proxied endpoint must not splice control, ANSI or bidi chars into the render
863865
LineSenderException ex = new LineSenderException("Could not flush buffer: ", retryable)
864866
.putAsPrintable(sink)
865-
.put(" [http-status=").put(statusCode.asAsciiCharSequence()).put(']');
867+
.put(" [http-status=").putAsPrintable(statusCode.asAsciiCharSequence()).put(']');
866868
client.disconnect();
867869
throw ex;
868870
}
@@ -1032,7 +1034,7 @@ public void onEvent(int code, CharSequence tag, int position) throws JsonExcepti
10321034
private void drainAndReset(LineSenderException sink, DirectUtf8Sequence httpStatus) {
10331035
assert state == State.INIT;
10341036

1035-
sink.putAsPrintable(messageSink).put(" [http-status=").put(httpStatus.asAsciiCharSequence());
1037+
sink.putAsPrintable(messageSink).put(" [http-status=").putAsPrintable(httpStatus.asAsciiCharSequence());
10361038
if (codeSink.length() != 0 || errorIdSink.length() != 0 || lineSink.length() != 0) {
10371039
if (errorIdSink.length() != 0) {
10381040
sink.put(", id: ").putAsPrintable(errorIdSink);
@@ -1074,7 +1076,7 @@ LineSenderException toException(Response chunkedRsp, DirectUtf8Sequence httpStat
10741076
}
10751077
// sanitize the raw server body before it reaches the exception message (and any log/terminal):
10761078
// an untrusted or proxied endpoint must not splice control, ANSI or bidi chars into the render
1077-
exception.putAsPrintable(jsonSink).put(" [http-status=").put(httpStatus.asAsciiCharSequence()).put(']');
1079+
exception.putAsPrintable(jsonSink).put(" [http-status=").putAsPrintable(httpStatus.asAsciiCharSequence()).put(']');
10781080
reset();
10791081
return exception;
10801082
}

core/src/test/java/io/questdb/client/test/SenderBuilderErrorApiTest.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,27 @@ public void testHttpTokenProviderIsMutuallyExclusiveWithOtherAuth() {
264264
}
265265
}
266266

267+
@Test
268+
public void testHttpTokenProviderNullRejectedAndExclusiveWithLaterUsernamePassword() {
269+
// a null provider is rejected up front
270+
try {
271+
Sender.builder(Sender.Transport.HTTP).address("localhost:9000").httpTokenProvider(null);
272+
Assert.fail("expected a null provider to be rejected");
273+
} catch (LineSenderException e) {
274+
Assert.assertTrue(e.getMessage(), e.getMessage().contains("token provider cannot be null"));
275+
}
276+
// the reverse of the mutual-exclusion case above: provider first, then username/password. This hits a
277+
// distinct guard in httpUsernamePassword(), which the provider-then-token / token-then-provider /
278+
// username-then-provider orderings above do not reach
279+
try {
280+
Sender.builder(Sender.Transport.HTTP).address("localhost:9000")
281+
.httpTokenProvider(() -> "dynamic").httpUsernamePassword("u", "p");
282+
Assert.fail("expected token-provider-already-configured");
283+
} catch (LineSenderException e) {
284+
Assert.assertTrue(e.getMessage(), e.getMessage().contains("token provider authentication is already configured"));
285+
}
286+
}
287+
267288
@Test
268289
public void testHttpTokenProviderAcceptedForWebSocket() {
269290
// the provider is supported over WebSocket (queried at each upgrade handshake): it must pass

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1863,6 +1863,10 @@ public void testIssuerPathScopingRejectsSplitEncodedAndBackslashSeparators() thr
18631863
Assert.assertFalse(invokeIsEndpointUnderIssuerPath("https://idp.example.com/realms/acme%2%66evil/token", issuer));
18641864
Assert.assertFalse(invokeIsEndpointUnderIssuerPath("https://idp.example.com/realms/acme%252fevil/token", issuer));
18651865
Assert.assertFalse(invokeIsEndpointUnderIssuerPath("https://idp.example.com/realms/acme\\evil/token", issuer));
1866+
// a percent-encoded backslash (%5c / %5C) is the encoded form of the literal '\' above; the scan
1867+
// rejects it at the encoded level too, before decodePathSegments would fold it to '/'
1868+
Assert.assertFalse(invokeIsEndpointUnderIssuerPath("https://idp.example.com/realms/acme%5cevil/token", issuer));
1869+
Assert.assertFalse(invokeIsEndpointUnderIssuerPath("https://idp.example.com/realms/acme%5Cevil/token", issuer));
18661870
}
18671871

18681872
@Test(timeout = 30_000)

core/src/test/java/io/questdb/client/test/cutlass/line/LineHttpSenderErrorResponseTest.java

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,38 @@ public class LineHttpSenderErrorResponseTest {
5151
// U+202E RIGHT-TO-LEFT OVERRIDE: reorders displayed text (visual spoofing)
5252
private static final char RLO = 0x202e;
5353

54+
@Test(timeout = 30_000)
55+
public void testProtocolDetectionErrorBodyControlAndBidiAreEscaped() throws Exception {
56+
assertMemoryLeak(() -> {
57+
// when the caller does not pin a protocol version, build() probes the server for one; a
58+
// non-success, non-404 probe response body is captured into the "Failed to detect server line
59+
// protocol version" exception. A hostile or proxied endpoint must not splice control, ANSI or
60+
// bidi chars into that message any more than into a flush error
61+
String errorBody = "probe denied " + ESC + "[2J forged\n" + RLO + "moc.live";
62+
try (MockOidcServer server = new MockOidcServer((method, path, body) -> MockOidcServer.chunkedJson(400, errorBody))) {
63+
try {
64+
// no protocolVersion(...) -> build() runs the detection probe; retryTimeoutMillis(0) makes
65+
// it give up after the first failed probe instead of retrying to a deadline
66+
Sender.builder(Sender.Transport.HTTP)
67+
.address("127.0.0.1:" + server.port())
68+
.retryTimeoutMillis(0)
69+
.build()
70+
.close();
71+
Assert.fail("expected protocol detection to fail and surface the server body");
72+
} catch (LineSenderException e) {
73+
String msg = e.getMessage();
74+
Assert.assertTrue(msg, msg.contains("Failed to detect server line protocol version"));
75+
Assert.assertTrue("visible text must be preserved: " + msg, msg.contains("probe denied"));
76+
Assert.assertTrue("the ESC must be escaped: " + msg, msg.contains("\\u001b"));
77+
Assert.assertTrue("the bidi override must be escaped: " + msg, msg.contains("\\u202e"));
78+
Assert.assertFalse("a raw ESC must not leak: " + msg, msg.indexOf(0x1b) >= 0);
79+
Assert.assertFalse("a raw newline must not leak: " + msg, msg.indexOf('\n') >= 0);
80+
Assert.assertFalse("a raw bidi override must not leak: " + msg, msg.indexOf(0x202e) >= 0);
81+
}
82+
}
83+
});
84+
}
85+
5486
@Test(timeout = 30_000)
5587
public void testServerAuthErrorBodyControlAndBidiAreEscaped() throws Exception {
5688
assertMemoryLeak(() -> {
@@ -83,6 +115,46 @@ public void testServerAuthErrorBodyControlAndBidiAreEscaped() throws Exception {
83115
});
84116
}
85117

118+
@Test(timeout = 30_000)
119+
public void testServerErrorStatusLineControlCharsAreEscaped() throws Exception {
120+
assertMemoryLeak(() -> {
121+
// the HTTP status-line token is echoed into the exception as "[http-status=...]". The header parser
122+
// copies it verbatim between the two spaces, so a hostile or proxied endpoint can smuggle control or
123+
// ANSI bytes there; a non-3-char token bypasses the numeric status checks and reaches the generic
124+
// error path, so the status render must escape them too, not just the body. A bidi override is a
125+
// multi-byte char the raw-response writer's US-ASCII encoding would drop, so this case uses an ESC;
126+
// the bidi cases above cover the body
127+
String body = "upstream error";
128+
// a malformed status code "400<ESC>[m" (6 chars, not 3) carries an ESC between the two spaces;
129+
// text/plain keeps it off the JSON parser, so it reaches the generic path that renders the status
130+
String rawResponse = "HTTP/1.1 400" + ESC + "[m FORGED\r\n"
131+
+ "Content-Type: text/plain\r\n"
132+
+ "Transfer-Encoding: chunked\r\n\r\n"
133+
+ Integer.toHexString(body.length()) + "\r\n" + body + "\r\n"
134+
+ "0\r\n\r\n";
135+
try (MockOidcServer server = new MockOidcServer((method, path, b) -> MockOidcServer.raw(rawResponse))) {
136+
try (Sender sender = Sender.builder(Sender.Transport.HTTP)
137+
.address("127.0.0.1:" + server.port())
138+
.protocolVersion(Sender.PROTOCOL_VERSION_V1)
139+
.disableAutoFlush()
140+
.build()) {
141+
sender.table("t").longColumn("v", 1L).atNow();
142+
try {
143+
sender.flush();
144+
Assert.fail("expected the server's error to surface as a LineSenderException");
145+
} catch (LineSenderException e) {
146+
String msg = e.getMessage();
147+
Assert.assertTrue(msg, msg.contains("Could not flush buffer"));
148+
// the ESC smuggled into the status token arrives escaped, never as a raw byte that
149+
// could drive an ANSI terminal sequence
150+
Assert.assertTrue("the status-line ESC must be escaped: " + msg, msg.contains("\\u001b"));
151+
Assert.assertFalse("a raw ESC must not leak from the status line: " + msg, msg.indexOf(0x1b) >= 0);
152+
}
153+
}
154+
}
155+
});
156+
}
157+
86158
@Test(timeout = 30_000)
87159
public void testServerJsonErrorBidiAndZeroWidthAreEscaped() throws Exception {
88160
assertMemoryLeak(() -> {
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
/*+*****************************************************************************
2+
* ___ _ ____ ____
3+
* / _ \ _ _ ___ ___| |_| _ \| __ )
4+
* | | | | | | |/ _ \/ __| __| | | | _ \
5+
* | |_| | |_| | __/\__ \ |_| |_| | |_) |
6+
* \__\_\\__,_|\___||___/\__|____/|____/
7+
*
8+
* Copyright (c) 2014-2019 Appsicle
9+
* Copyright (c) 2019-2026 QuestDB
10+
*
11+
* Licensed under the Apache License, Version 2.0 (the "License");
12+
* you may not use this file except in compliance with the License.
13+
* You may obtain a copy of the License at
14+
*
15+
* http://www.apache.org/licenses/LICENSE-2.0
16+
*
17+
* Unless required by applicable law or agreed to in writing, software
18+
* distributed under the License is distributed on an "AS IS" BASIS,
19+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
20+
* See the License for the specific language governing permissions and
21+
* limitations under the License.
22+
*
23+
******************************************************************************/
24+
25+
package io.questdb.client.test.std.str;
26+
27+
import io.questdb.client.std.str.DisplaySafe;
28+
import org.junit.Assert;
29+
import org.junit.Test;
30+
31+
/**
32+
* Direct coverage for {@link DisplaySafe}, the single source of truth for whether a code point may be shown
33+
* verbatim in a terminal or a log line. Both {@code Utf16Sink.putAsPrintable} and the OIDC display sanitizer
34+
* delegate to it, so a regression here would silently weaken every display-escaping path - yet the classifier
35+
* was previously exercised only transitively, for the few code points the integration tests happen to use.
36+
* <p>
37+
* The unsafe code points (controls, Unicode format chars, surrogates, bidi controls, the BOM) are written as
38+
* hex literals so this source stays pure ASCII and carries none of the chars it asserts on.
39+
*/
40+
public class DisplaySafeTest {
41+
42+
@Test
43+
public void testC0C1ControlsAndDelAreUnsafe() {
44+
// C0 (incl. TAB/LF/CR/ESC), DEL, and the C1 block: every ISO control must be escaped
45+
int[] unsafe = {0x00, 0x07, 0x08, 0x09, 0x0A, 0x0D, 0x1B, 0x1F, 0x7F, 0x80, 0x90, 0x9F};
46+
for (int cp : unsafe) {
47+
String hex = "0x" + Integer.toHexString(cp);
48+
Assert.assertFalse("control " + hex + " must be unsafe", DisplaySafe.isDisplaySafe(cp));
49+
Assert.assertTrue("control " + hex + " must be unsafe", DisplaySafe.isUnsafeForDisplay(cp));
50+
}
51+
}
52+
53+
@Test
54+
public void testFormatBidiAndBomAreUnsafe() {
55+
// Cf format chars and the explicit bidi/BOM set that reorder, hide, or mark text - including the
56+
// supplementary-plane "tag" chars that arrive as a surrogate pair and must be judged whole
57+
int[] unsafe = {
58+
0x00AD, // soft hyphen
59+
0x200B, // zero-width space
60+
0x200E, 0x200F, // LRM, RLM
61+
0x202A, 0x202B, 0x202C, 0x202D, 0x202E, // LRE, RLE, PDF, LRO, RLO
62+
0x2066, 0x2067, 0x2068, 0x2069, // LRI, RLI, FSI, PDI
63+
0xFEFF, // BOM / zero-width no-break space
64+
0xE0001, // language tag
65+
0xE0020, 0xE007F // tag space, cancel tag
66+
};
67+
for (int cp : unsafe) {
68+
String hex = "0x" + Integer.toHexString(cp);
69+
Assert.assertTrue("format " + hex + " must be unsafe", DisplaySafe.isUnsafeForDisplay(cp));
70+
Assert.assertFalse("format " + hex + " must be unsafe", DisplaySafe.isDisplaySafe(cp));
71+
}
72+
}
73+
74+
@Test
75+
public void testLoneSurrogatesAreUnsafe() {
76+
// a lone surrogate half has no displayable meaning; the code-point classifier must reject it
77+
int[] surrogates = {0xD800, 0xDBFF, 0xDC00, 0xDFFF};
78+
for (int cp : surrogates) {
79+
Assert.assertFalse("surrogate 0x" + Integer.toHexString(cp) + " must be unsafe", DisplaySafe.isDisplaySafe(cp));
80+
}
81+
}
82+
83+
@Test
84+
public void testPrintableAsciiIsSafe() {
85+
// the fast-path range 0x20..0x7e is the overwhelmingly common case and must always pass
86+
for (int cp = 0x20; cp <= 0x7e; cp++) {
87+
String hex = "0x" + Integer.toHexString(cp);
88+
Assert.assertTrue("printable ASCII " + hex + " must be safe", DisplaySafe.isDisplaySafe(cp));
89+
Assert.assertFalse("printable ASCII " + hex + " must be safe", DisplaySafe.isUnsafeForDisplay(cp));
90+
}
91+
}
92+
93+
@Test
94+
public void testPrintableSupplementaryCharsAreSafe() {
95+
// a normal supplementary char (emoji, CJK extension) is neither control, format, nor surrogate and
96+
// stays safe, so the classifier does not over-escape legitimate non-BMP text
97+
int[] safe = {
98+
0x1F600, // grinning face emoji
99+
0x1F4A9, // pile of poo
100+
0x20000 // CJK Extension B ideograph
101+
};
102+
for (int cp : safe) {
103+
Assert.assertTrue("supplementary 0x" + Integer.toHexString(cp) + " must be safe", DisplaySafe.isDisplaySafe(cp));
104+
}
105+
}
106+
}

0 commit comments

Comments
 (0)