SNOW-3704231: Reassemble fragmented localhost callback requests

Author: sfc-gh-rkowalski

CHANGELOG.md

@@ -10,6 +10,7 @@
   - Fixed flaky `DatabaseMetaDataIT.testGetPrimarykeys` and `SnowflakeDriverIT.testConstraints` (eventually-consistent PK/FK constraint metadata after DDL) by retrying the constraint metadata lookups until visible. Replaced the live, concurrent stress tests `DatabaseMetaDataLatestIT.test[No]PatternSearchAllowedForPrimaryAndForeignKeys` (which timed out on slow CI runners) with deterministic coverage in the new `DatabaseMetadataWiremockLatestIT`, which validates the `getTables`/`getPrimaryKeys`/`getImportedKeys` SHOW-result mapping and the `enablePatternSearch` pattern-vs-literal behavior using stubbed responses (no live account or metadata-propagation dependency). Test-only change with no driver behavior impact (snowflakedb/snowflake-jdbc#2673).
   - Fixed the Loader API (`StreamLoader.setVectorColumns`) throwing `Loader$ConnectionError: ... Result set has been closed` when the `getColumns` metadata lookup returned no rows. The result set returned by `getColumns` closes itself once `next()` runs out of matching rows, so the unchecked `rs.next()` followed by `rs.getString(...)` raised "Result set has been closed"; the result is now read only when `rs.next()` returns a row. This intermittently aborted concurrent loads (e.g. `FlatfileReadMultithreadIT`) when the `SHOW COLUMNS` metadata query raced with concurrent DML on the same table (snowflakedb/snowflake-jdbc#2674).
   - Fixed `uploadStream`/`downloadStream` failing with SQL compilation errors when the stage reference contains non-ASCII characters (e.g. Japanese schema names). Stage references in internally generated PUT/GET commands are now wrapped in single quotes when they contain characters that require quoting per Snowflake SQL syntax (SNOW-3713887).
+  - Fixed `authenticator=externalbrowser` login crashing with `StringIndexOutOfBoundsException` when the browser opened an empty preconnect socket or delivered a large SSO request across multiple reads; the localhost callback server now ignores empty connections and reassembles fragmented requests (snowflakedb/snowflake-jdbc#2687).
 
 - v4.3.1
   - Fixed GCS-backed internal stage PUT failing with opaque `invalid_gcs_credentials` in SPCS pods on GCP: the GCS SDK's Application Default Credentials (ADC) probe was reaching out to `metadata.google.internal` which is unreachable inside SPCS; explicit credentials are now always set when a `GCS_ACCESS_TOKEN` is present, suppressing the ADC probe entirely. Also fixed `GCSAccessStrategyAwsSdk` rejecting custom GCS endpoints that lack an `https://` scheme prefix (e.g. bare `storage.me-central2.rep.googleapis.com`), mirroring the existing handling in `GCSDefaultAccessStrategy`. The catch-all in `setupGCSClient` now chains and logs the original exception instead of swallowing it (snowflakedb/snowflake-jdbc#2664).

src/main/java/net/snowflake/client/internal/core/SessionUtilExternalBrowser.java

@@ -295,28 +295,22 @@ void authenticate() throws SFException, SnowflakeSQLException {
       }
 
       while (true) {
-        Socket socket = ssocket.accept(); // start accepting the request
-        try {
+        try (Socket socket = ssocket.accept()) {
+          // Bound reads so a client that connects then stalls mid-request cannot hang the server.
+          socket.setSoTimeout(getBrowserResponseTimeout());
           BufferedReader in =
               new BufferedReader(new InputStreamReader(socket.getInputStream(), UTF8_CHARSET));
-          char[] buf = new char[16384];
-          int strLen = in.read(buf);
-          if (strLen < 0) {
-            // The browser frequently opens speculative/preconnect sockets to the localhost
-            // callback server and closes them without sending any data. The read then returns -1
-            // (EOF). Previously this produced `new String(buf, 0, -1)` and crashed with
-            // StringIndexOutOfBoundsException before the real request could be handled. Ignore such
-            // empty connections and keep listening. See SNOW-3704231.
+          String request = readRequest(in);
+          if (request.isEmpty()) {
+            // Browser preconnect/speculative socket closed without sending data. See SNOW-3704231.
             logger.debug("Received empty request on localhost callback socket. Ignoring.");
             continue;
           }
-          String[] rets = new String(buf, 0, strLen).split("\r\n");
+          String[] rets = request.split("\r\n");
           if (!processOptions(rets, socket)) {
             processSamlToken(rets, socket);
             break;
           }
-        } finally {
-          socket.close();
         }
       }
     } catch (SocketTimeoutException e) {
@@ -337,6 +331,25 @@ void authenticate() throws SFException, SnowflakeSQLException {
     }
   }
 
+  /**
+   * Reads a full HTTP request from the localhost callback socket, reassembling fragments until the
+   * end-of-headers marker ({@code \r\n\r\n}) or EOF. Returns an empty string if the peer closed the
+   * connection without sending data (e.g. a browser preconnect socket). See SNOW-3704231.
+   */
+  private static String readRequest(BufferedReader in) throws IOException {
+    char[] buf = new char[16384];
+    StringBuilder request = new StringBuilder();
+    int strLen;
+    while ((strLen = in.read(buf)) >= 0) {
+      request.append(buf, 0, strLen);
+      // Stop at end of headers; the browser holds the connection open awaiting our response.
+      if (request.indexOf("\r\n\r\n") >= 0) {
+        break;
+      }
+    }
+    return request.toString();
+  }
+
   private boolean processOptions(String[] rets, Socket socket) throws IOException {
     String targetLine = null;
     String userAgent = null;

src/test/java/net/snowflake/client/internal/core/SessionUtilExternalBrowserTest.java

@@ -13,6 +13,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.net.InetAddress;
 import java.net.ServerSocket;
 import java.net.Socket;
 import java.net.SocketTimeoutException;
@@ -21,9 +22,10 @@
 import java.nio.charset.StandardCharsets;
 import java.sql.Connection;
 import java.sql.ResultSet;
+import java.time.Duration;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.stream.IntStream;
+import java.util.concurrent.atomic.AtomicReference;
 import net.snowflake.client.AbstractDriverIT;
 import net.snowflake.client.api.datasource.SnowflakeDataSource;
 import net.snowflake.client.api.datasource.SnowflakeDataSourceFactory;
@@ -262,69 +264,67 @@ public void testInvalidSSOUrl() {
   }
 
   /**
-   * SNOW-3704231: the localhost callback server used to assume a single socket read returned the
-   * whole HTTP request. Browsers routinely open speculative/preconnect sockets to the callback port
-   * and close them without sending any data, so {@code in.read(buf)} returns -1 (EOF) and {@code
-   * new String(buf, 0, -1)} threw StringIndexOutOfBoundsException before the real request was ever
-   * processed. The server must ignore such empty connections and keep listening.
-   *
-   * @throws Throwable if any error occurs
-   */
-  @Test
-  public void testAuthenticateIgnoresEmptyPreconnectSocket() throws Throwable {
-    final SFLoginInput loginInput = initMockLoginInput();
-
-    try (MockedStatic<HttpUtil> mockedHttpUtil = mockStatic(HttpUtil.class)) {
-      mockSsoUrlResponse(mockedHttpUtil);
-
-      // First connection sends no data (browser preconnect closed immediately) -> read() == -1.
-      // Second connection carries the real token.
-      Socket emptySocket = mockSocket(new ByteArrayInputStream(new byte[0]));
-      Socket tokenSocket =
-          mockSocket(
-              toStream(
-                  String.format(
-                      "GET /?token=%s&confirm=1 HTTP/1.1\r\n" + "USER-AGENT: snowflake client",
-                      FakeSessionUtilExternalBrowser.MOCK_SAML_TOKEN)));
-
-      SessionUtilExternalBrowser sub =
-          new SequencedFakeSessionUtilExternalBrowser(
-              loginInput, mockServerSocket(emptySocket, tokenSocket));
-      sub.authenticate();
-      assertEquals(FakeSessionUtilExternalBrowser.MOCK_SAML_TOKEN, sub.getToken());
-    }
-  }
-
-  /**
-   * SNOW-3704231: large SSO requests (big tokens plus Sec-Fetch and Sec-GPC headers) fill the
-   * request line and headers with several kilobytes of data. This confirms the callback server
-   * still parses the token out of such a large request.
-   *
-   * @throws Throwable if any error occurs
+   * SNOW-3704231: end-to-end over real OS sockets. Reproduces the customer's HAR: a browser
+   * preconnect socket that closes without sending data, followed by a large (~3.8 KB) request
+   * delivered in two TCP fragments. Exercises the real ServerSocket/Socket + BufferedReader decode
+   * path, not Mockito mocks.
    */
   @Test
-  public void testAuthenticateHandlesLargeRequest() throws Throwable {
+  public void testAuthenticateOverRealSocket() throws Throwable {
     final SFLoginInput loginInput = initMockLoginInput();
+    // A real (non-zero) timeout so the test fails fast instead of hanging if the fix regresses.
+    when(loginInput.getBrowserResponseTimeout()).thenReturn(Duration.ofSeconds(30));
 
-    try (MockedStatic<HttpUtil> mockedHttpUtil = mockStatic(HttpUtil.class)) {
+    try (MockedStatic<HttpUtil> mockedHttpUtil = mockStatic(HttpUtil.class);
+        ServerSocket serverSocket = new ServerSocket(0, 0, InetAddress.getByName("localhost"))) {
       mockSsoUrlResponse(mockedHttpUtil);
+      int port = serverSocket.getLocalPort();
 
-      // A large token (mimicking a real SSO token) plus large privacy headers, ~4 KB total.
-      String largeToken = IntStream.range(0, 3000).mapToObj(i -> "A").reduce("", String::concat);
-      String request =
+      String largeToken = new String(new char[3800]).replace('\0', 'A');
+      byte[] requestBytes =
           String.format(
-              "GET /?token=%s&confirm=1 HTTP/1.1\r\n"
-                  + "USER-AGENT: snowflake client\r\n"
-                  + "Sec-Fetch-Site: none\r\n"
-                  + "Sec-GPC: 1\r\n"
-                  + "\r\n",
-              largeToken);
-
-      Socket socket = mockSocket(toStream(request));
+                  "GET /?token=%s&confirm=1 HTTP/1.1\r\n"
+                      + "USER-AGENT: snowflake client\r\n"
+                      + "Sec-Fetch-Site: none\r\n"
+                      + "Sec-GPC: 1\r\n"
+                      + "\r\n",
+                  largeToken)
+              .getBytes(StandardCharsets.UTF_8);
+
+      AtomicReference<Throwable> clientError = new AtomicReference<>();
+      Thread browser =
+          new Thread(
+              () -> {
+                try {
+                  // 1) Speculative/preconnect socket that closes without sending data (the crash).
+                  new Socket("localhost", port).close();
+                  // 2) Real request split into two TCP fragments to force a partial first read.
+                  try (Socket s = new Socket("localhost", port)) {
+                    OutputStream out = s.getOutputStream();
+                    int half = requestBytes.length / 2;
+                    out.write(requestBytes, 0, half);
+                    out.flush();
+                    Thread.sleep(100);
+                    out.write(requestBytes, half, requestBytes.length - half);
+                    out.flush();
+                    // Drain the driver's HTTP response so it can finish cleanly.
+                    s.getInputStream().read(new byte[4096]);
+                  }
+                } catch (Throwable t) {
+                  clientError.set(t);
+                }
+              });
+      browser.setDaemon(true);
+      browser.start();
 
       SessionUtilExternalBrowser sub =
-          new SequencedFakeSessionUtilExternalBrowser(loginInput, mockServerSocket(socket));
+          new SequencedFakeSessionUtilExternalBrowser(loginInput, serverSocket);
       sub.authenticate();
+
+      browser.join(10_000);
+      if (clientError.get() != null) {
+        throw new AssertionError("Browser client thread failed", clientError.get());
+      }
       assertEquals(largeToken, sub.getToken());
     }
   }
@@ -352,25 +352,6 @@ private static void mockSsoUrlResponse(MockedStatic<HttpUtil> mockedHttpUtil) th
                 new HashMap<>()));
   }
 
-  private static InputStream toStream(String data) {
-    return new ByteArrayInputStream(data.getBytes(StandardCharsets.UTF_8));
-  }
-
-  private static Socket mockSocket(InputStream in) throws IOException {
-    Socket socket = mock(Socket.class);
-    when(socket.getInputStream()).thenReturn(in);
-    when(socket.getOutputStream())
-        .thenReturn(new FakeSessionUtilExternalBrowser.NullOutputStream());
-    return socket;
-  }
-
-  private static ServerSocket mockServerSocket(Socket first, Socket... rest) throws IOException {
-    ServerSocket serverSocket = mock(ServerSocket.class);
-    when(serverSocket.getLocalPort()).thenReturn(12345);
-    when(serverSocket.accept()).thenReturn(first, rest);
-    return serverSocket;
-  }
-
   /**
    * Mock HttpUtil and SFLoginInput
    *
@@ -431,11 +412,7 @@ public void testExternalBrowserTimeout() throws Exception {
   }
 }
 
-/**
- * SessionUtilExternalBrowser whose ServerSocket is supplied by the test, so that the sequence of
- * accepted connections (e.g. an empty preconnect socket followed by the real request) can be
- * controlled.
- */
+/** SessionUtilExternalBrowser with a test-supplied ServerSocket to control accepted connections. */
 class SequencedFakeSessionUtilExternalBrowser extends SessionUtilExternalBrowser {
   private final ServerSocket serverSocket;