Fix OAuth2 redirect timeouts caused by closing the server too early (#1285)

* Fix redirect failures caused by closing the server too early

* Correct formatting in function doc comment

Author: redgoldlace

src/LaunchServer.lua

@@ -2,11 +2,10 @@
 local url = ...
 local socket = require("socket")
 local server = assert(socket.bind("*", 49082) or socket.bind("*", 49083) or socket.bind("*", 49084))
-server:settimeout(30)
 local host, port = server:getsockname()
 ConPrintf("Server started on %s:%s", host, port)
 
-local redirect_uri= string.format(
+local redirect_uri = string.format(
 	"http://localhost:%d", port
 )
 ConPrintf("Redirect URI: %s", redirect_uri)
@@ -95,18 +94,35 @@ local commonResponseEnd = [[
 ConPrintf("Opening URL: %s", url)
 OpenURL(url)
 
-local code, state
-local client = server:accept()
-if client then
-	client:settimeout(10)
+--- Handle an incoming socket connection, to complete an OAuth redirect.
+--- @param client table @The socket connection to handle, as returned by `server:accept()`.
+--- @param attempt number @The number of attempts made to handle an incoming connection. This is used for logging
+--- purposes, since spurious issues can be difficult to identify otherwise.
+--- @return boolean shouldRetry @Whether we should wait for another connection. If false, we've successfully responded
+--- to a HTTP request. Note that, for the purposes of this function, we don't care whether authorization was *granted*,
+--- just that the process itself was completed and the user was redirected as intended.
+--- @return string? code @The OAuth authorization code. This is exchanged for an access token and refresh token later.
+--- @return string? state @The OAuth state string. This is a sentinel value used to ensure that a request hasn't been
+--- forged.
+function handleConnection(client, attempt)
+	local shouldRetry, code, state = true, nil, nil
+
 	local request, err = client:receive("*l")
-	
-	if not err and request then
+	if err then
+		ConPrintf("Attempt %d to handle incoming connection failed: %s", attempt, err)
+	elseif request then
 		local response
 		local _, _, method, path, version = request:find("^(%S+)%s(%S+)%s(%S+)")
 		if method ~= "GET" then
-			return
+			ConPrintf(
+				"Attempt %d to handle incoming connection received an invalid HTTP request: non-GET method %s",
+				attempt,
+				method
+			)
+
+			return true
 		end
+
 		local queryParams = {}
 		for k, v in path:gmatch("([^&=?]+)=([^&=?]+)") do
 			queryParams[k] = v:gsub("%%(%x%x)", function(hex)
@@ -129,10 +145,56 @@ if client then
 			]] .. commonResponseEnd
 		end
 
+		shouldRetry = false
+		if attempt ~= 1 then
+			ConPrintf("Attempt %d to handle incoming connection received a valid HTTP request", attempt)
+		end
+
 		-- Send HTTP Response
 		--ConPrintf("Sending response: %s", response)
 		client:send(response)
 	end
+
+	return shouldRetry, code, state
+end
+
+-- Misbehaving software (think VPNs, anything network-related, even OS services) will occasionally attempt to connect
+-- to newly-opened sockets for one reason or another. Previously, PoB only waited for one connection, and gave up
+-- immediately if something went wrong.
+--
+-- This would result in a sequence of events roughly like this:
+--   1. PoB opens a socket
+--   2. A misbehaving piece of software connects to the socket, sends nothing, then terminates the connection
+--   3. PoB tries to read from the socket, receives an error since the connection is terminated, and closes the server
+--   4. OAuth authorization succeeds, but by the time the user is redirected back to PoB, the server is already closed
+--   5. PoB never receives the OAuth redirect, and doesn't have any of the information necessary to use the API
+--
+-- To avoid this, we instead allow for any number of incoming connections, and simply stop listening for them once
+-- either a) 30 seconds have elapsed or b) we've received a legitimate HTTP request and responded to it.
+--
+-- Unfortunately, this still isn't perfect: in theory, two applications (such as a browser, and something else) could
+-- attempt to establish a connection at the same time. In the future, this could be refactored to perform non-blocking
+-- IO, so that it can operate concurrently, but hopefully that isn't necessary.
+local attempt = 1
+local stopAt = os.time() + 30
+local shouldRetry, code, state = true, nil, nil
+while (os.time() < stopAt) and shouldRetry do
+	-- `settimeout`` applies only to individual operations, but we're more concerned with not spending more than 30
+	-- seconds *total* waiting, so we adjust with each iteration as necessary.
+	local remainingTime = math.max(0, stopAt - os.time())
+	server:settimeout(remainingTime)
+
+	local client = server:accept()
+	if not client then
+		goto retry
+	end
+
+	client:settimeout(5)
+	shouldRetry, code, state = handleConnection(client, attempt)
 	client:close()
+
+	:: retry ::
+	attempt = attempt + 1
 end
+server:close()
 return code, state, port