mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-06 08:54:04 +01:00
fix: pre-landing review fixes
Ship adversarial pass: a crash restart re-emits BROWSE_PROXY_URL=direct from the daemon state only when the invocation has no flags, so the env it hands the new daemon never contradicts the config hash it also hands over; an explicit-direct session is refused against a daemon with no recorded config hash, like --proxy and --headed already were; the unparseable-variable hint says that the opt-out also bypasses the system proxy settings on macOS and Windows; BROWSER.md notes that a managed Chromium proxy policy still overrides the switch; CHANGELOG names both files that joined the macOS named-regressions job. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
bd2203291c
commit
c0ddffa3e2
5 files changed
+17
-12
No files matched your search
+6
-2
@@ -993,7 +993,10 @@ daemon log (`browse-daemon.log` next to the state file) records the
|
||||
effective mode each time the daemon starts, as `[proxy] using <url> from
|
||||
<source>`, `[proxy] none: direct connection (--proxy-server=direct://;
|
||||
ignoring HTTPS_PROXY)` (on macOS and Windows, `ignoring the system proxy
|
||||
settings`), or `[proxy] none: system proxy settings`.
|
||||
settings`), or `[proxy] none: system proxy settings`. A managed Chromium
|
||||
proxy policy (`ProxyMode`/`ProxySettings`) still overrides the switch; `$B`
|
||||
cannot see it, so on a managed machine the log line describes what `$B`
|
||||
asked for, not what the policy enforces.
|
||||
|
||||
Values from the environment follow the same credential policy as `--proxy`.
|
||||
`socks5h://` is treated as `socks5://`, because Chromium's SOCKS5 already
|
||||
@@ -1009,7 +1012,8 @@ where it came from, with credentials redacted.
|
||||
unsupported scheme, or a bad port. `$B` connects directly (on Linux,
|
||||
Chromium is told so and does not read the variable itself) instead of failing
|
||||
every command. Fix: `export HTTPS_PROXY=http://host:port` (or `socks5://`),
|
||||
or set `BROWSE_PROXY_URL=direct` to silence the warning. A bad `--proxy` or
|
||||
or set `BROWSE_PROXY_URL=direct` to silence the warning (on macOS and
|
||||
Windows that also makes Chromium bypass the system proxy settings). A bad `--proxy` or
|
||||
`BROWSE_PROXY_URL` is still an error, because those are explicit browse
|
||||
settings and a silent direct connection could expose your real IP address.
|
||||
- **`the running browse daemon was started with different proxy settings (<VAR> differs), so it was not reused.`**
|
||||
|
||||
+3
-3
@@ -7,16 +7,16 @@
|
||||
### Fixed
|
||||
- **`browse/src/browser-manager.ts`**: every launch path (headless `launch`, the `BROWSE_EXTENSIONS_DIR` persistent context, `launchHeaded`, `handoff`) adds `--proxy-server=direct://` when no proxy is configured and `forcesDirect()` says Chromium would otherwise pick one browse did not resolve, from one `proxyLaunchArgs()`. A configured proxy was already explicit (Playwright passes it as `--proxy-server`), so the environment can no longer win silently in either mode. `--no-proxy-server` was measured and rejected: chrome-headless-shell, the default headless build, ignores it and still took `HTTPS_PROXY`; the full Chromium honors both switches, and `direct://` works on both.
|
||||
- **`browse/src/proxy-config.ts`**: `forcesDirect(env, platform)` is true on Linux always, where the leak lives, and on macOS and Windows only with `BROWSE_PROXY_URL=direct`. Chromium there never read the proxy variables and follows the system proxy settings (PAC and WPAD included), so a user of GStack Browser.app behind a corporate proxy keeps that connectivity, and a PAC cannot be named through `BROWSE_PROXY_URL`. An explicit `direct` bypasses the system settings on every platform.
|
||||
- **`browse/src/cli.ts`, `browse/src/server.ts`**: on macOS and Windows the explicit `direct` is part of the daemon config hash (`explicitDirect()`), is recorded in the daemon state as `proxyDirect` and is re-emitted on a crash auto-restart, so a running daemon started on the system proxy is refused rather than reused by a session that opted out (the refusal names `BROWSE_PROXY_URL` and offers `browse stop` or unsetting it; a daemon started before this release meets that refusal once), and a restart triggered from a shell without the variable stays direct. On Linux the opt-out hashes like no proxy, as before, because every direct connection there already carries the switch.
|
||||
- **`browse/src/cli.ts`, `browse/src/server.ts`**: on macOS and Windows the explicit `direct` is part of the daemon config hash (`explicitDirect()`), is recorded in the daemon state as `proxyDirect` and is re-emitted on a crash auto-restart, so a running daemon started on the system proxy is refused rather than reused by a session that opted out (the refusal names `BROWSE_PROXY_URL` and offers `browse stop` or unsetting it; a daemon started before this release meets that refusal once, and a daemon with no recorded config hash is refused like one started without `--proxy`), and a restart with no invocation flags re-emits the opt-out from the daemon state; a restart driven by a session's own flags emits only what that session's hash says. The unparseable-variable warning's hint now says that `BROWSE_PROXY_URL=direct` also bypasses the system proxy settings on macOS and Windows. On Linux the opt-out hashes like no proxy, as before, because every direct connection there already carries the switch.
|
||||
- **`browse/src/server.ts`** logs the effective proxy mode each time the daemon starts, beside the existing `[proxy] using <url> from <source>` line: `[browse] [proxy] none: direct connection (--proxy-server=direct://; ignoring HTTPS_PROXY)` on Linux, naming each standard variable that was set and ignored; `(...; ignoring the system proxy settings)` for an explicit direct on macOS and Windows; and `[browse] [proxy] none: system proxy settings (Chromium's default on <platform>; BROWSE_PROXY_URL=direct connects directly)` when those platforms are left to the system.
|
||||
- **BROWSER.md, "Proxy from environment"**: says that whatever `$B` resolves is passed to Chromium explicitly, that on Linux a set proxy variable is ignored in direct mode, that macOS and Windows follow the system proxy settings unless `BROWSE_PROXY_URL=direct` is set (and that the opt-out is part of the daemon's settings there), and where the daemon log records the effective mode. The eMudhra/TLS trust section is unchanged.
|
||||
- **BROWSER.md, "Proxy from environment"**: says that whatever `$B` resolves is passed to Chromium explicitly, that on Linux a set proxy variable is ignored in direct mode, that macOS and Windows follow the system proxy settings unless `BROWSE_PROXY_URL=direct` is set (and that the opt-out is part of the daemon's settings there), where the daemon log records the effective mode, and that a managed Chromium proxy policy still overrides the switch. The eMudhra/TLS trust section is unchanged.
|
||||
|
||||
### Tests
|
||||
- `browse/test/proxy-direct-launch.test.ts` (Linux): two logging CONNECT proxies on loopback (`browse/test/helpers/connect-proxy.ts`) and a target name that does not resolve. With `HTTPS_PROXY` set and `BROWSE_PROXY_URL=direct`, the goto fails at DNS, no CONNECT reaches the proxy, the daemon log names the ignored variable and the state file has no proxy; this case fails on the unfixed tree with `ERR_TUNNEL_CONNECTION_FAILED` and a logged CONNECT. An unparseable `HTTPS_PROXY` (curl-style `host:port`) warns and still forces direct, so no CONNECT reaches the host Chromium itself would have parsed. Two controls: `HTTPS_PROXY` alone is the configured proxy and receives the CONNECT; `--proxy` beside `HTTPS_PROXY` sends the CONNECT only to the configured proxy. Each case asserts on the daemon-log slice its own invocation wrote.
|
||||
- `browse/test/proxy-direct-launch-paths.test.ts` (Linux, real Chromium under Xvfb): the `BROWSE_EXTENSIONS_DIR` persistent context, `launchHeaded()` and the `handoff()` relaunch each ignore an inherited `HTTPS_PROXY` (no CONNECT, `ERR_NAME_NOT_RESOLVED`), and a headed control with a configured proxy still sends the CONNECT to it.
|
||||
- `browse/test/proxy-config.test.ts`: `forcesDirect` per platform, and `directProxyLogLine` for the Linux variable list, the explicit-direct line on macOS/Windows, and the system-proxy line.
|
||||
- `browse/test/proxy-env.test.ts`, `browse/test/restart-env.test.ts`: the config hash separates an explicit direct from no proxy on macOS and Windows and not on Linux (`extractGlobalFlags` takes the platform); the mismatch refusal for both directions (explicit session against a system-proxy daemon, default session against an explicit-direct daemon); `buildRestartEnv` re-emits `BROWSE_PROXY_URL=direct` from the invocation or the persisted state, and a proxied invocation wins over it.
|
||||
- `browse/test/proxy-env-daemon.test.ts`: through the real CLI and daemon, `BROWSE_PROXY_URL=direct` is written to the daemon state as `proxyDirect` where it bypasses the system proxy and a default session is then refused with the `BROWSE_PROXY_URL differs` lines; on Linux the daemon is reused. The file joins the macOS named-regressions CI job so the refusal arm runs there. `buildHeadedServerEnv` carries the opt-out to the headed server, and BROWSER.md's section is checked against the real switch and log literals.
|
||||
- `browse/test/proxy-env-daemon.test.ts`: through the real CLI and daemon, `BROWSE_PROXY_URL=direct` is written to the daemon state as `proxyDirect` where it bypasses the system proxy and a default session is then refused with the `BROWSE_PROXY_URL differs` lines; on Linux the daemon is reused. This file and `proxy-direct-launch-args.test.ts` join the macOS named-regressions CI job, so the refusal arm and the "no switch without an opt-out" arm run on macOS. `buildHeadedServerEnv` carries the opt-out to the headed server, and BROWSER.md's section is checked against the real switch and log literals.
|
||||
- `browse/test/proxy-direct-launch-args.test.ts`: reads the real command line of the headless Chromium `BrowserManager` launched (from the pid it records) and asserts the switch is present on Linux and absent elsewhere without an opt-out, and present on every platform with `BROWSE_PROXY_URL=direct`; the per-platform expectations are written out, not read back from `forcesDirect`. Skipped on Windows, where no command line reader exists.
|
||||
|
||||
## [1.91.226.0] - 2026-10-26
|
||||
|
||||
+3
-3
@@ -539,7 +539,7 @@ export function buildRestartEnv(
|
||||
oldState: ServerState | null,
|
||||
): Record<string, string> {
|
||||
const env: Record<string, string> = proxyServerEnv(globalFlags);
|
||||
if (!globalFlags?.proxyUrl && oldState?.proxyDirect) env.BROWSE_PROXY_URL = 'direct';
|
||||
if (!globalFlags && oldState?.proxyDirect) env.BROWSE_PROXY_URL = 'direct';
|
||||
if (globalFlags?.headed || oldState?.mode === 'headed') env.BROWSE_HEADED = '1';
|
||||
const configHash = globalFlags?.configHash || oldState?.configHash;
|
||||
if (configHash) env.BROWSE_CONFIG_HASH = configHash;
|
||||
@@ -943,8 +943,8 @@ async function ensureServer(flags?: GlobalFlags): Promise<ServerState> {
|
||||
// Same path: existing daemon is plain (no flags) but caller passes
|
||||
// --proxy/--headed. Refuse for the same reason — apply explicitly via
|
||||
// disconnect+reconnect.
|
||||
if (desiredHash && !state.configHash && (flags?.proxyUrl || flags?.headed)) {
|
||||
console.error(`[browse] existing daemon was started without --proxy/--headed.`);
|
||||
if (desiredHash && !state.configHash && (flags?.proxyUrl || flags?.headed || flags?.explicitDirect)) {
|
||||
console.error(`[browse] existing daemon was started without --proxy/--headed${flags?.explicitDirect ? ' or BROWSE_PROXY_URL=direct' : ''}.`);
|
||||
console.error(`[browse] run 'browse disconnect' first to apply new flags.`);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
@@ -319,7 +319,7 @@ export function resolveProxy(flagUrl: string | null, env: NodeJS.ProcessEnv): Re
|
||||
} catch (err) {
|
||||
if (!(err instanceof ProxyConfigError)) throw err;
|
||||
return direct(`${source} is not a proxy URL browse can use (${err.message}; ${err.hint}), so browse connects directly. ` +
|
||||
`fix: export ${source}=http://host:port (or socks5://), or set BROWSE_PROXY_URL=direct to silence this. see ${PROXY_TROUBLESHOOTING}`);
|
||||
`fix: export ${source}=http://host:port (or socks5://), or set BROWSE_PROXY_URL=direct to silence this (on macOS and Windows that also makes Chromium bypass the system proxy settings). see ${PROXY_TROUBLESHOOTING}`);
|
||||
}
|
||||
}
|
||||
const parsed = parseProxyConfig({ proxyUrl: raw, envUser: env.BROWSE_PROXY_USER, envPass: env.BROWSE_PROXY_PASS, source });
|
||||
|
||||
@@ -38,9 +38,10 @@ describe("buildRestartEnv (#1781 headed persistence)", () => {
|
||||
expect(env.BROWSE_CONFIG_HASH).toBe("d");
|
||||
});
|
||||
|
||||
test("explicit direct falls back to persisted state, so a plain command's restart never flips to the system proxy", () => {
|
||||
expect(buildRestartEnv({} as any, { ...launchedState, proxyDirect: true } as any).BROWSE_PROXY_URL).toBe("direct");
|
||||
expect(buildRestartEnv({} as any, launchedState as any).BROWSE_PROXY_URL).toBeUndefined();
|
||||
test("explicit direct falls back to persisted state only when no invocation flags exist, so the env never contradicts the hash", () => {
|
||||
expect(buildRestartEnv(null, { ...launchedState, proxyDirect: true } as any).BROWSE_PROXY_URL).toBe("direct");
|
||||
expect(buildRestartEnv(null, launchedState as any).BROWSE_PROXY_URL).toBeUndefined();
|
||||
expect(buildRestartEnv({ explicitDirect: false, configHash: "plain" } as any, { ...launchedState, proxyDirect: true } as any).BROWSE_PROXY_URL).toBeUndefined();
|
||||
});
|
||||
|
||||
test("a proxied invocation wins over a persisted explicit direct", () => {
|
||||
|
||||
Reference in new issue
Block a user