From 2c42736f9155d544af93a44a08d3729d2b02d2ce Mon Sep 17 00:00:00 2001 From: Lace Date: Wed, 3 Sep 2025 22:35:42 +1000 Subject: [PATCH 1/2] Fix redirect failures caused by closing the server too early --- src/LaunchServer.lua | 80 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 71 insertions(+), 9 deletions(-) diff --git a/src/LaunchServer.lua b/src/LaunchServer.lua index e6aca20f5d..d2d68cf30a 100644 --- a/src/LaunchServer.lua +++ b/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 From 367d561dbbdb356654264bab9309fed250c64e8a Mon Sep 17 00:00:00 2001 From: Lace Date: Wed, 3 Sep 2025 22:50:53 +1000 Subject: [PATCH 2/2] Correct formatting in function doc comment --- src/LaunchServer.lua | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/LaunchServer.lua b/src/LaunchServer.lua index d2d68cf30a..f5413c6fc5 100644 --- a/src/LaunchServer.lua +++ b/src/LaunchServer.lua @@ -95,15 +95,15 @@ ConPrintf("Opening URL: %s", url) OpenURL(url) --- 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 +--- @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*, +--- @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. +--- @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