mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-01 14:34:42 +01:00
fix: pre-landing review fixes
Ship rerun review pass: proxy-direct-launch-args.test.ts joins the macOS named-regressions job so its omission arm (no switch without the opt-out) runs in CI; serverProxyEnv takes the platform so the state-file predicate is pinned per platform instead of mirroring process.platform; buildHeadedServerEnv's parameter type requires explicitDirect so a narrowed caller cannot drop the opt-out silently; BROWSER.md's precedence item 6 and the reuse sentence say what each platform does (a connect daemon gets the generic flags refusal); CHANGELOG notes the one-time refusal against a pre-release daemon. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
c08c586bbd
commit
bd2203291c
8 files changed
+19
-14
No files matched your search
@@ -146,7 +146,7 @@ jobs:
|
||||
resolved="$(realpath "$tmp")"
|
||||
echo "temp root: $tmp -> $resolved"
|
||||
test "$resolved" != "${tmp%/}"
|
||||
files=(test/gstack-design-detect.test.ts browse/test/chromium-profile-isolation.test.ts test/cso-git-hardening.test.ts test/plan-count-file-permission.test.ts test/spec-quality-gate-secret-sink.test.ts test/gstack-memory-ingest.test.ts test/regression-issue3024-zsh-sourced-helpers.test.ts test/codex-probe-subcommands.test.ts browse/test/proxy-env-daemon.test.ts)
|
||||
files=(test/gstack-design-detect.test.ts browse/test/chromium-profile-isolation.test.ts test/cso-git-hardening.test.ts test/plan-count-file-permission.test.ts test/spec-quality-gate-secret-sink.test.ts test/gstack-memory-ingest.test.ts test/regression-issue3024-zsh-sourced-helpers.test.ts test/codex-probe-subcommands.test.ts browse/test/proxy-env-daemon.test.ts browse/test/proxy-direct-launch-args.test.ts)
|
||||
for file in "${files[@]}"; do test -f "$file"; done
|
||||
bun test "${files[@]}"
|
||||
|
||||
|
||||
+5
-3
@@ -970,7 +970,8 @@ first one set wins:
|
||||
3. `HTTPS_PROXY`, then `https_proxy`
|
||||
4. `HTTP_PROXY`, then `http_proxy`
|
||||
5. `ALL_PROXY`, then `all_proxy`
|
||||
6. no proxy (a direct connection)
|
||||
6. no proxy configured by `$B` (a direct connection on Linux; the system proxy
|
||||
settings on macOS and Windows)
|
||||
|
||||
Whatever `$B` resolves is what Chromium gets, explicitly: a proxy is passed
|
||||
as `--proxy-server=<url>`, and a direct connection as
|
||||
@@ -985,8 +986,9 @@ never reads those variables; it follows the system proxy settings (PAC and
|
||||
WPAD included), and step 6 leaves it to them, so GStack Browser.app behind a
|
||||
corporate proxy keeps working. `BROWSE_PROXY_URL=direct` bypasses the system
|
||||
settings too, on every platform; on macOS and Windows it is part of the
|
||||
daemon's settings, so a running daemon started without it is not reused (the
|
||||
refusal below names `BROWSE_PROXY_URL`) and a crash restart keeps it. The
|
||||
daemon's settings, so a running daemon started without it is not reused (for
|
||||
a headless daemon the refusal below names `BROWSE_PROXY_URL`; a `connect`
|
||||
daemon gets the generic flags refusal) and a crash restart keeps it. The
|
||||
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://;
|
||||
|
||||
+1
-1
@@ -7,7 +7,7 @@
|
||||
### 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), 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 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/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.
|
||||
|
||||
|
||||
+1
-1
@@ -552,7 +552,7 @@ export function buildRestartEnv(
|
||||
* the same port, watchdog setting, proxy and config hash. Pure + exported for tests.
|
||||
*/
|
||||
export function buildHeadedServerEnv(
|
||||
globalFlags: Pick<GlobalFlags, 'proxyUrl' | 'configHash'> & Partial<Pick<GlobalFlags, 'proxySource' | 'proxyBypass'>>,
|
||||
globalFlags: Pick<GlobalFlags, 'proxyUrl' | 'configHash' | 'explicitDirect'> & Partial<Pick<GlobalFlags, 'proxySource' | 'proxyBypass'>>,
|
||||
): Record<string, string> {
|
||||
return {
|
||||
BROWSE_HEADED: '1',
|
||||
|
||||
@@ -235,12 +235,12 @@ export function withLoopbackBypass(rules: string[]): string {
|
||||
* caller other than the CLI set only BROWSE_PROXY_URL), and whether an
|
||||
* explicit direct bypasses the system proxy settings here (explicitDirect).
|
||||
*/
|
||||
export function serverProxyEnv(env: NodeJS.ProcessEnv): { url: string | undefined; source: string; bypass: string; direct: boolean } {
|
||||
export function serverProxyEnv(env: NodeJS.ProcessEnv, platform: NodeJS.Platform = process.platform): { url: string | undefined; source: string; bypass: string; direct: boolean } {
|
||||
return {
|
||||
url: env.BROWSE_PROXY_URL === 'direct' ? undefined : env.BROWSE_PROXY_URL || undefined,
|
||||
source: env.BROWSE_PROXY_SOURCE || 'BROWSE_PROXY_URL',
|
||||
bypass: env.BROWSE_PROXY_BYPASS || withLoopbackBypass(noProxyToChromiumRules(env.NO_PROXY || env.no_proxy)),
|
||||
direct: explicitDirect(env),
|
||||
direct: explicitDirect(env, platform),
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -92,7 +92,7 @@ function harness(opts: {
|
||||
let clock = 1_000_000, sleeps = 0, tick = 0, exiting = false, starts = 0;
|
||||
const calls = { startEnv: [] as Record<string, string>[], agents: [] as number[], log: [] as string[], warn: [] as string[], error: [] as string[] };
|
||||
const deps: HeadedSupervisorDeps = {
|
||||
env: buildHeadedServerEnv({ proxyUrl: 'socks5://127.0.0.1:9050', configHash: 'abc123' }),
|
||||
env: buildHeadedServerEnv({ proxyUrl: 'socks5://127.0.0.1:9050', configHash: 'abc123', explicitDirect: false }),
|
||||
tickMs: opts.tickMs ?? 30_000,
|
||||
backoffMs: [1000, 2000, 4000, 8000, 30000],
|
||||
daemonLog: '/state/browse-daemon.log',
|
||||
@@ -181,7 +181,7 @@ describe('runHeadedSupervisor (behavior)', () => {
|
||||
|
||||
describe('buildHeadedServerEnv', () => {
|
||||
test('omits proxy and config hash when this invocation has none', () => {
|
||||
expect(buildHeadedServerEnv({ proxyUrl: null, configHash: '' })).toEqual({ BROWSE_HEADED: '1', BROWSE_PORT: '34567', BROWSE_PARENT_PID: '0' });
|
||||
expect(buildHeadedServerEnv({ proxyUrl: null, configHash: '', explicitDirect: false })).toEqual({ BROWSE_HEADED: '1', BROWSE_PORT: '34567', BROWSE_PARENT_PID: '0' });
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -220,9 +220,12 @@ describe('explicitDirect (#3440 item 8)', () => {
|
||||
.toBe(computeConfigHash({ proxyUrl: 'http://p:1', headed: false }));
|
||||
});
|
||||
|
||||
test('the daemon reads the opt-out from its own environment for the state file', () => {
|
||||
expect(serverProxyEnv({ BROWSE_PROXY_URL: 'direct' }).direct).toBe(process.platform !== 'linux');
|
||||
expect(serverProxyEnv({}).direct).toBe(false);
|
||||
test('the daemon reads the opt-out from its own environment for the state file, on the platforms where it matters', () => {
|
||||
expect(serverProxyEnv({ BROWSE_PROXY_URL: 'direct' }, 'darwin').direct).toBe(true);
|
||||
expect(serverProxyEnv({ BROWSE_PROXY_URL: 'direct' }, 'win32').direct).toBe(true);
|
||||
expect(serverProxyEnv({ BROWSE_PROXY_URL: 'direct' }, 'linux').direct).toBe(false);
|
||||
expect(serverProxyEnv({}, 'darwin').direct).toBe(false);
|
||||
expect(serverProxyEnv({ BROWSE_PROXY_URL: 'http://p:1' }, 'darwin').direct).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -514,7 +514,7 @@ describe('Server auth security', () => {
|
||||
// parent-PID watchdog.
|
||||
const connectBlock = sliceBetween(CLI_SRC, 'Launching headed Chromium', 'Terminal agent started');
|
||||
expect(connectBlock).toContain('startServer(buildHeadedServerEnv(globalFlags))');
|
||||
expect(buildHeadedServerEnv({ proxyUrl: null, configHash: '' }).BROWSE_PARENT_PID).toBe('0');
|
||||
expect(buildHeadedServerEnv({ proxyUrl: null, configHash: '', explicitDirect: false }).BROWSE_PARENT_PID).toBe('0');
|
||||
});
|
||||
|
||||
// Regression: newtab returned 403 for scoped tokens because the tab ownership
|
||||
|
||||
Reference in new issue
Block a user