Send the account login through the system proxy - #500
Conversation
The login request went through requestBoundedTextViaNode, the raw Node https transport the auth service requires (#76), which never read the system proxy or HTTP_PROXY/HTTPS_PROXY. Every other launcher request already inherited the proxy through Electron's net.request, so a network that requires one only broke login. Resolves the proxy for the login URL through Electron's own session (session.defaultSession.resolveProxy), the same source net.request reads from, and tunnels an HTTP proxy answer with a plain CONNECT before writing the request. DIRECT keeps the prior path byte for byte. HTTPS_PROXY/HTTP_PROXY are read as a fallback only when the session itself answers DIRECT, with NO_PROXY respected. A SOCKS answer, an HTTPS-secured proxy answer, or a proxy that answers a CONNECT with 407 all fail the login up front with a fixed reason token (proxy-unsupported, proxy-auth-required) instead of the generic failure; both join the network-unreachable family, whose sentence already tells a player to check a proxy. Every log line stays a fixed token, never the resolved host.
Zaldaryon
left a comment
There was a problem hiding this comment.
Changes requested. The direct and Electron proxy paths are covered by focused tests, but the environment fallback still changes proxy semantics. The required checks are green. I added comments for the scheme and NO_PROXY handling, and the tests should cover an HTTPS target through CONNECT before this is ready.
HTTPS_PROXY/HTTP_PROXY's fallback used to read a proxy URL and rebuild it as a bare "PROXY host:port" string, discarding the scheme it just parsed. An https proxy then received a plaintext CONNECT instead of one sent over its own TLS connection, and a socks5 URL was silently treated as an HTTP proxy rather than refused. parseProxyUrl (src/domain/net/proxy.ts) now reads that URL's scheme directly: http and https both come back tunnelable, with a new "https" kind marking a proxy connectThroughProxy must reach over TLS before it ever sends the CONNECT; socks comes back the same recognised-but-unimplemented shape a SOCKS PAC answer already gets. Chromium's own PAC answers are untouched, parseProxyResolution still refuses an "HTTPS" PAC entry the same as before. Covered in tests/ipc/networkProxy.test.ts with a self-signed certificate generated at test time (tests/ipc/helpers/tlsFixtures.ts, no new dependency): one test proves the https-proxy CONNECT is actually sent over TLS, the other proves a socks5 HTTPS_PROXY is refused up front instead of dialing out as if it were HTTP.
hostMatchesNoProxy only ever received the request's bare hostname, so a ported entry such as auth.vintagestory.at:443 never equalled it and the bypass never triggered; the login went through the proxy regardless of NO_PROXY. It is also pure decision logic with no host dependency, so it belongs in the domain layer next to the rest of this issue's proxy parsing, not in src/ipc/network.ts. Moved to src/domain/net/proxy.ts as matchesNoProxy(url, noProxy): an entry without a port still bypasses every port for that host, matching curl; an entry with one only bypasses the URL's own effective port (defaulted the same way a missing one already is elsewhere in this file). The wildcard, suffix and leading-dot behaviour, case-insensitivity and whitespace tolerance are unchanged. tests/domain/net/proxy.test.ts covers a bare host, a host with a port (matching and not, including a mismatched explicit port), a leading-dot suffix, the * entry, spaces around an entry, and a case difference, as direct unit tests now that the logic lives in domain rather than needing a running proxy to exercise.
Every existing proxy test used a plain http target, the one case requestBoundedTextViaNode already skips its TLS wrap for, so the wrap connectThroughProxy puts around a tunnelled socket for a real, https login was never exercised end to end. A break there would have shipped unnoticed. startSecureOrigin (tests/ipc/networkProxy.test.ts) runs a real https server on a self-signed certificate from tests/ipc/helpers/tlsFixtures.ts, trusted for this test only through the ca-merging mock that file installs on node:tls/node:https. The new test tunnels a POST through a plain HTTP proxy to that origin and checks the login body arrives intact on the far side of the TLS-wrapped tunnel, not just that a response comes back. Confirmed against a real regression: temporarily skipping the TLS wrap in connectThroughProxy made this test fail with the tunnelled socket's plain HTTP request landing on a TLS-only server, then passed again once the wrap was restored.
|
Pushed three commits, one per point.
PR body has a Review round 1 section with file:line and gate figures. Ready for another look. |
d3fc84f to
ebb67ab
Compare
Zaldaryon
left a comment
There was a problem hiding this comment.
Reviewed the current head. The proxy implementation now preserves supported proxy schemes, handles port qualified NO_PROXY entries, and covers HTTP CONNECT plus TLS on both sides of the tunnel. The required checks pass in the Ubuntu and Windows environments, including typecheck, lint, the full test matrix, build, and the gate test. Approving.
What the player sees
A network that requires a proxy no longer breaks login by itself. The login request now goes through the same proxy every other launcher request already used, and a proxy this launcher cannot route through (an authenticated proxy, or a SOCKS proxy) fails with a message that names a proxy as the cause, the same sentence a plain connection failure already shows, instead of a generic "check your connection or firewall".
Why
Login posts through
requestBoundedTextViaNode, a raw Nodehttpstransport kept only because the auth service rejects Electron'snet.requestfetch metadata (#76). That transport set no agent and read no proxy, whilenet.request(every other call) inherits the system proxy from Electron on its own. On a network that requires a proxy, everything else in the launcher worked and only login failed.Decisions
session.defaultSession.resolveProxy), the same sourcenet.requestreads, so it follows the OS settings and Chromium switches the same way the rest of the launcher does. Parsing that PAC-style answer is a pure function,parseProxyResolution(src/domain/net/proxy.ts).CONNECT(node:http's ownrequest, thennode:tlsupgrading the returned socket for anhttps:target): no new dependency.DIRECTkeeps the prior path byte for byte.HTTPS_PROXY/HTTP_PROXYare read as a fallback only when the session itself answersDIRECT, withNO_PROXYrespected for the login host.CONNECTwith 407 fails the login with a fixed reason instead of a prompt this launcher does not have; a SOCKS proxy has no client here and fails the same way, whether Chromium's own session resolved it orHTTPS_PROXY/HTTP_PROXYnamed one directly. Anhttps:proxy URL from the environment fallback tunnels over its own TLS connection before the CONNECT (since review round 1); Chromium's ownHTTPS-scheme PAC answer still comes back unsupported, since a PAC answer never carries more than a host and a port to act on. Every one of these joins the existingnetwork-unreachablefamily, whose sentence already tells a player to check a proxy.proxy-used,proxy-direct,proxy-unsupported,proxy-auth-required), never the resolved host.Testing
parseProxyResolutionagainst every PAC shape (PROXY,SOCKS4/SOCKS5,HTTPS,DIRECT, a fallback list, garbage, empty).CONNECT-accepting proxy stub relays the login through the tunnel and gets the real response back with the same headers and body; a proxy answering 407 maps toproxy-auth-requiredbefore the origin is ever reached; a SOCKS answer maps toproxy-unsupportedbefore any socket opens;DIRECTbypasses the tunnel;HTTPS_PROXYis used only as aDIRECTfallback, andNO_PROXYstill bypasses it.network-unreachablefamily sentence, the one both new reason tokens join, is confirmed to name a proxy; no new locale string was needed, en-US and fr-FR already carried the wording.git status --short.Review round 1
Addressed in the order raised:
src/ipc/network.ts:350,parseProxyUrlnow insrc/domain/net/proxy.ts:58).HTTPS_PROXY/HTTP_PROXYno longer get flattened to a barePROXY host:port: the URL's scheme is kept, sohttp:andhttps:both tunnel overCONNECT(connectThroughProxy,src/ipc/network.ts:403, now sends the CONNECT itself overnode:https'srequestrather thannode:http's when the proxy issecure), and asocks:/socks4:/socks5:URL comes back the same recognised-but-unimplemented shape a SOCKS PAC answer already gets instead of being dialed as if it were HTTP. Test:tests/ipc/networkProxy.test.ts:287, a self-signed-certificate proxy proving the CONNECT itself goes out over TLS, and asocks5://value refused up front rather than reaching for a socket.matchesNoProxy, moved out ofsrc/ipc/network.tsintosrc/domain/net/proxy.ts:95since it never touched Electron or a socket). An entry can now carry an optional:port, bypassing only when both the host and the target's own effective port agree; a port-less entry still bypasses every port for that host, unchanged. Test:tests/domain/net/proxy.test.ts:48, covering a bare host, a ported host (matching and mismatched), a leading-dot suffix,*, spaces around an entry, and a case difference.tests/ipc/networkProxy.test.ts:320). A new self-signed-certificate origin, tunnelled through a plain HTTP proxy, proves the login body actually crosses the TLS wrapconnectThroughProxyputs around the tunnelled socket, not just that some response comes back. Confirmed against a real regression: temporarily skipping that wrap failed this test.Gate:
typecheckclean,lint:ci0 errors (14 pre-existing warnings),format:checkclean,test:coverage4453 passed / 2 skipped, thresholds cleared (statements 94.29%, branches 90.47%, functions 95.01%, lines 95.98%).Closes #481.