github-advanced-security[bot] commented on code in PR #19567:
URL: https://github.com/apache/druid/pull/19567#discussion_r3974866349
##########
processing/src/test/java/org/apache/druid/java/util/http/client/FriendlyServersTest.java:
##########
@@ -388,11 +398,359 @@
StatusResponseHandler.getInstance()
).get().getStatus();
- Assertions.assertEquals(200, status.getCode());
+ Assertions.assertEquals(200, status.code());
}
}
finally {
lifecycle.stop();
}
}
+
+ /**
+ * Regression test for the case where the same {@link Request} (or a copy of
it) is sent more
+ * than once. Netty 4's HttpObjectEncoder advances the content's reader
index and releases
+ * {@link io.netty.handler.codec.http.FullHttpRequest} after encoding; if
{@link NettyHttpClient}
+ * handed the Request's stored {@link io.netty.buffer.ByteBuf} directly to
the encoder, the
+ * second send would either drain to zero bytes or hit an {@link
io.netty.util.IllegalReferenceCountException}.
+ *
+ * KerberosHttpClient hits this path: on a 401 it calls {@code
Request.copy()} (which deep-copies
+ * the content) and re-sends. This test exercises the same shape.
+ */
+ @Test
+ public void testRequestCanBeSentMultipleTimes() throws Exception
+ {
+ final ExecutorService exec = Executors.newCachedThreadPool();
+ final ServerSocket serverSocket = new ServerSocket(0);
+ final BlockingQueue<String> receivedBodies = new LinkedBlockingQueue<>();
+ // Keep-alive HTTP/1.1 loop: each accepted connection serves consecutive
requests until the
+ // peer disconnects. Needed because the client's channel pool reuses
connections, and a
+ // single-shot server would cause a write-to-closed-channel race on the
second send.
+ exec.submit(
+ () -> {
+ while (!Thread.currentThread().isInterrupted()) {
+ final Socket clientSocket;
+ try {
+ clientSocket = serverSocket.accept();
+ }
+ catch (IOException e) {
+ return;
+ }
+ exec.submit(() -> {
+ try (
+ Socket s = clientSocket;
+ BufferedReader in = new BufferedReader(
+ new InputStreamReader(s.getInputStream(),
StandardCharsets.UTF_8)
+ );
+ OutputStream out = s.getOutputStream()
+ ) {
+ while (true) {
+ String requestLine = in.readLine();
+ if (requestLine == null) {
+ return;
+ }
+ int contentLength = 0;
+ String header;
+ while (!(header = in.readLine()).equals("")) {
+ if
(header.toLowerCase(Locale.ROOT).startsWith("content-length:")) {
+ contentLength =
Integer.parseInt(header.substring("content-length:".length()).trim());
+ }
+ }
+ char[] body = new char[contentLength];
+ int read = 0;
+ while (read < contentLength) {
+ int n = in.read(body, read, contentLength - read);
+ if (n < 0) {
+ return;
+ }
+ read += n;
+ }
+ receivedBodies.put(new String(body, 0, read));
+ out.write("HTTP/1.1 200 OK\r\nContent-Length:
2\r\n\r\nok".getBytes(StandardCharsets.UTF_8));
+ out.flush();
+ }
+ }
+ catch (Exception ignored) {
+ // suppress
+ }
+ });
+ }
+ }
+ );
+
+ final Lifecycle lifecycle = new Lifecycle();
+ try {
+ final HttpClient client =
HttpClientInit.createClient(HttpClientConfig.builder().build(), lifecycle);
+ final URL url = new URL(StringUtils.format("http://localhost:%d/";,
serverSocket.getLocalPort()));
+ final byte[] payload = "hello-body".getBytes(StandardCharsets.UTF_8);
+
+ final Request request = new Request(HttpMethod.POST,
url).setContent(payload);
+
+ // First send.
+ Assertions.assertEquals(
+ 200,
+ client.go(request,
StatusResponseHandler.getInstance()).get().getStatus().code()
+ );
+
+ // KerberosHttpClient's retry pattern: copy the request and send the
copy. This must not
+ // observe a released ByteBuf nor an already-drained reader index on the
original.
+ Assertions.assertEquals(
+ 200,
+ client.go(request.copy(),
StatusResponseHandler.getInstance()).get().getStatus().code()
+ );
+
+ // And the original Request must still be sendable a third time on its
own.
+ Assertions.assertEquals(
+ 200,
+ client.go(request,
StatusResponseHandler.getInstance()).get().getStatus().code()
+ );
+
+ Assertions.assertEquals("hello-body", receivedBodies.poll(10,
TimeUnit.SECONDS));
+ Assertions.assertEquals("hello-body", receivedBodies.poll(10,
TimeUnit.SECONDS));
+ Assertions.assertEquals("hello-body", receivedBodies.poll(10,
TimeUnit.SECONDS));
+ }
+ finally {
+ exec.shutdownNow();
+ serverSocket.close();
+ lifecycle.stop();
+ }
+ }
+
+ /**
+ * Regression test for the case where a caller sets HTTP header names using
their conventional
+ * casing (e.g. {@code Host}, {@code Accept-Encoding}) rather than the
lowercase form Netty 4's
+ * {@link HttpHeaderNames} constants use. NettyHttpClient must not emit
duplicate headers when
+ * these overlap with defaults it would otherwise apply.
+ */
+ @Test
+ public void testCallerHostHeaderDoesNotDuplicate() throws Exception
+ {
+ final ExecutorService exec = Executors.newSingleThreadExecutor();
+ final ServerSocket serverSocket = new ServerSocket(0);
+ final BlockingQueue<java.util.List<String>> receivedHeaders = new
LinkedBlockingQueue<>();
+ exec.submit(
+ () -> {
+ while (!Thread.currentThread().isInterrupted()) {
+ try (
+ Socket clientSocket = serverSocket.accept();
+ BufferedReader in = new BufferedReader(
+ new InputStreamReader(clientSocket.getInputStream(),
StandardCharsets.UTF_8)
+ );
+ OutputStream out = clientSocket.getOutputStream()
+ ) {
+ in.readLine(); // request line
+ java.util.List<String> hdrs = new java.util.ArrayList<>();
+ String h;
+ while (!(h = in.readLine()).isEmpty()) {
+ hdrs.add(h);
+ }
+ receivedHeaders.put(hdrs);
+ out.write("HTTP/1.1 200 OK\r\nContent-Length:
2\r\n\r\nok".getBytes(StandardCharsets.UTF_8));
+ }
+ catch (Exception ignored) {
+ // suppress
+ }
+ }
+ }
+ );
+
+ final Lifecycle lifecycle = new Lifecycle();
+ try {
+ final HttpClient client =
HttpClientInit.createClient(HttpClientConfig.builder().build(), lifecycle);
+ final URL url = new URL(StringUtils.format("http://localhost:%d/";,
serverSocket.getLocalPort()));
+
+ // Caller sets Host and Accept-Encoding using conventional (title-case)
HTTP header names.
+ // Netty 4's HttpHeaderNames constants are lowercase; a naive
case-sensitive presence check
+ // against the caller's Multimap would miss these and cause
NettyHttpClient to add duplicates.
+ final Request request = new Request(HttpMethod.GET, url)
+ .setHeader("Host", "override.example.com")
+ .setHeader("Accept-Encoding", "identity");
+
+ Assertions.assertEquals(
+ 200,
+ client.go(request,
StatusResponseHandler.getInstance()).get().getStatus().code()
+ );
+
+ final java.util.List<String> hdrs = receivedHeaders.poll(10,
TimeUnit.SECONDS);
+ Assertions.assertNotNull(hdrs, "no headers received");
+
+ int hostCount = 0;
+ int acceptEncodingCount = 0;
+ for (String hdr : hdrs) {
+ final String name = hdr.substring(0,
hdr.indexOf(':')).toLowerCase(Locale.ROOT);
+ if ("host".equals(name)) {
+ hostCount++;
+ Assertions.assertTrue(
+ hdr.toLowerCase(Locale.ROOT).contains("override.example.com"),
+ "Host header should reflect caller value, was: " + hdr
+ );
+ } else if ("accept-encoding".equals(name)) {
+ acceptEncodingCount++;
+ Assertions.assertTrue(
+ hdr.toLowerCase(Locale.ROOT).contains("identity"),
+ "Accept-Encoding should be caller-supplied identity, was: " + hdr
+ );
+ }
+ }
+ Assertions.assertEquals(1, hostCount, "exactly one Host header on the
wire");
+ Assertions.assertEquals(1, acceptEncodingCount, "exactly one
Accept-Encoding header on the wire");
+ }
+ finally {
+ exec.shutdownNow();
+ serverSocket.close();
+ lifecycle.stop();
+ }
+ }
+
+ @Test
+ public void testValidationFailureDoesNotLeakChannel() throws Exception
+ {
+ final ExecutorService exec = Executors.newSingleThreadExecutor();
+ final ServerSocket serverSocket = new ServerSocket(0);
+ exec.submit(
+ () -> {
+ while (!Thread.currentThread().isInterrupted()) {
+ try (
+ Socket clientSocket = serverSocket.accept();
+ BufferedReader in = new BufferedReader(
+ new InputStreamReader(clientSocket.getInputStream(),
StandardCharsets.UTF_8)
+ );
+ OutputStream out = clientSocket.getOutputStream()
+ ) {
+ String line;
+ while ((line = in.readLine()) != null && !line.isEmpty()) {
+ // skip request headers
+ }
+ out.write("HTTP/1.1 200 OK\r\nContent-Length:
6\r\n\r\nhello!".getBytes(StandardCharsets.UTF_8));
+ out.flush();
+ }
+ catch (Exception ignored) {
+ if (serverSocket.isClosed()) {
+ return;
+ }
+ }
+ }
+ }
+ );
+
+ final Lifecycle lifecycle = new Lifecycle();
+ try {
+ final HttpClient client =
HttpClientInit.createClient(HttpClientConfig.builder().build(), lifecycle);
+ final URL url = new URL(StringUtils.format("http://localhost:%d/";,
serverSocket.getLocalPort()));
Review Comment:
## CodeQL / Deprecated method or constructor invocation
Invoking [URL.URL](1) should be avoided because it has been deprecated.
[Show more
details](https://github.com/apache/druid/security/code-scanning/11947)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
