SNOW-3704231: Fix externalbrowser auth crash on empty localhost callback socket#2687
Open
sfc-gh-rkowalski wants to merge 3 commits into
Open
SNOW-3704231: Fix externalbrowser auth crash on empty localhost callback socket#2687sfc-gh-rkowalski wants to merge 3 commits into
sfc-gh-rkowalski wants to merge 3 commits into
Conversation
b7b43ba to
48396cd
Compare
ef4b3c1 to
07b5bc5
Compare
…ack socket
The localhost SSO callback loop assumed a single socket read returned the
whole HTTP request:
int strLen = in.read(buf);
String[] rets = new String(buf, 0, strLen).split("\r\n");
BufferedReader.read() returns -1 at EOF, i.e. when a socket is accepted but
closed without sending any data. new String(buf, 0, -1) then throws
StringIndexOutOfBoundsException before the real request could be handled.
Browsers routinely open speculative/preconnect sockets to the callback port
and close them without sending data, so the loop crashed on that empty
connection instead of processing the following request carrying the token.
Guard against strLen < 0 and keep listening for the real request.
Adds unit tests covering the empty preconnect connection and a large SSO
request.
Address review follow-ups on the externalbrowser callback reader: - Cap the reassembled request at MAX_REQUEST_LENGTH (1 MiB) so a misbehaving local client that advertises a huge Content-Length or never sends the end-of-headers marker can no longer buffer unboundedly toward OOM. The old single-read code was implicitly bounded to 16 KiB. - Skip the per-read UTF-8 re-encode when no body is expected (Content-Length absent/zero) and gate the exact byte count behind the cheap char-count check, avoiding the O(n^2) substring+getBytes on the GET/preflight path. - Drop the unreachable length guard in bodyByteLength (bodyStart is always within the accumulated request) and the redundant Math.max(0, ...) clamp in getContentLength. - Extract the duplicated large-token test fixture into a LARGE_TOKEN constant.
58ad4d8 to
78941c2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
SNOW-3704231
JDBC
authenticator=externalbrowserSSO fails withStringIndexOutOfBoundsExceptioninSessionUtilExternalBrowser.authenticate. The browser completes SSO and redirects tohttp://localhost:<port>/?token=..., but the localhost callback server crashes before sending a response, so the login never completes. Blocks browser-based SSO from DBeaver and custom Java apps on the customer's Linux workstation (SnowSQL, which uses a different callback, works).Root cause
The callback loop assumed a single socket read returned the whole HTTP request:
This breaks in two ways:
Empty / preconnect connection (the reported crash).
BufferedReader.read()returns-1at EOF — i.e. when a socket is accepted but closed without sending any data.new String(buf, 0, -1)then throws exactly the exception from the ticket:Modern browsers routinely open speculative/preconnect sockets to the callback port and close them without sending data. The loop
accept()s that empty connection first and crashes before the real request (carrying the token) is ever processed.Fragmented request. A large SSO request (big token plus
Sec-Fetch/Sec-GPCheaders, ~3900 bytes in the HAR) can arrive across multiple TCP reads. A single read then returns only the first fragment, so the truncated request line fails to parse (Invalid HTTP request. No token is given from the browser).Fix
A new
readRequest(BufferedReader)helper:\r\n\r\n) and, when the request carries a body, until the fullContent-Lengthbytes have arrived — so a POST callback whose token body lands in a TCP segment separate from the headers (the console-login SAML flow) is no longer truncated.Content-Lengthis a byte count, so the check compares the body's UTF-8 byte length, not its char count. With no body (Content-Lengthabsent/zero, e.g. a GET callback or CORS preflight) it stops at the blank line so it never blocks while the browser awaits the response.Reads on the accepted socket are bounded with
socket.setSoTimeout(getBrowserResponseTimeout())so a client that stalls mid-request surfaces the existing timeout error instead of hanging, and the accept loop now uses try-with-resources to close the socket.readRequest/getContentLengthare instance (not static) methods on purpose:SSOConnectionTestdrives the flow insidemockStatic(SessionUtilExternalBrowser.class), which would stub a static helper to returnnull.Test
Two end-to-end tests in
SessionUtilExternalBrowserTestdrive the realauthenticate()loop against a realServerSocket, with a background "browser" thread writing over actual TCP (a shareddriveRealSocketCallbackhelper) — the realServerSocket/Socket+BufferedReaderUTF-8 decode path, not Mockito socket mocks:testAuthenticateOverRealSocketreproduces the HAR: a preconnect socket closed without data, followed by a ~3.8 KB GET request delivered in two fragments. Fails on the pre-fix code with the exact ticket exception (StringIndexOutOfBoundsException ... length 16384) and passes with the fix.testAuthenticatePostBodyOverRealSocketsends a POST with the token in the body, delivered in a fragment separate from the headers. Fails on the pre-fix code withNo token is given from the browser(the read stopped at\r\n\r\nbefore the body arrived) and passes with the fix.CHANGELOG.mdupdated.Pre-review self checklist
masterbranchmvn -P check-style validate)mvn verifyand inspecttarget/japicmp/japicmp.html)SNOW-XXXX: