mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-01 14:34:42 +01:00
fix(browse): keep a slash-less scheme spelling, refuse a bare ? or #, and tighten the proxy docs
A standard proxy variable whose userinfo opens with a known scheme (https:bob:pw@host:3128, which the URL parser reads as https://) is no longer taken as scheme-less credentials, so the protocol it names is kept. parseProxyConfig refuses a literal ? or # anywhere in the value, not only a non-empty query or fragment. The unparseable-variable test now puts the credential in every looped value, so its no-echo assertion is real; new cases pin the slash-less scheme spelling, the bare ?/#, an env-sourced trailing slash, and that mixed credential sources stay a hard error for the scheme-less spelling as for the http:// one (the cred-mixing refusal predates this branch and its test on main pins it, so it is documented rather than softened). CHANGELOG and BROWSER.md: single backslashes inside code spans, "a path other than a bare /", the socks5 bridge log lines in the effective-mode list, and the character list applies anywhere in the value. Decision: GStack Minder (2026-10-26), option B. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
0c33acefde
commit
8ca299f2c7
4 files changed
+38
-17
No files matched your search
+9
-6
@@ -993,7 +993,9 @@ 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`. A managed Chromium
|
||||
settings`), or `[proxy] none: system proxy settings` (an authenticated
|
||||
`socks5://` upstream logs its `[proxy] upstream test ok` and `[proxy] bridge
|
||||
listening on 127.0.0.1:<port>` lines instead). 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.
|
||||
@@ -1015,11 +1017,12 @@ where it came from, with credentials redacted.
|
||||
|
||||
- **`warning: HTTPS_PROXY is not a proxy URL browse can use (...)`.** The
|
||||
variable doesn't parse even as `http://` plus the value: an unsupported
|
||||
scheme (`ftp://`), a bad port, a missing host, a path, query or fragment
|
||||
(where a `DOMAIN\user` login or a `/` in a password ends up after URL
|
||||
parsing, so such values are refused for every source rather than printed
|
||||
unredacted; percent-encode `/ \ ? # @` in credentials), or credentials
|
||||
that are not valid percent-encoding. `$B` connects directly (on Linux,
|
||||
scheme (`ftp://`), a bad port, a missing host, a path other than a bare
|
||||
`/`, a query or a fragment (where a `DOMAIN\user` login or a `/` in a
|
||||
password ends up after URL parsing, so such values are refused for every
|
||||
source rather than printed unredacted; percent-encode `/ \ ? # @` in
|
||||
`http://` and `https://` credentials), or credentials that are not valid
|
||||
percent-encoding. `$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 (on macOS and
|
||||
|
||||
+2
-2
@@ -6,7 +6,7 @@
|
||||
|
||||
### Fixed
|
||||
- **`browse/src/browser-manager.ts`**: each of `BrowserManager`'s four Chromium launch sites (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. The cookie-import worker (`cookie-import-native-worker.ts`), which launches the user's own browser only to read its cookie store and never navigates, is unchanged.
|
||||
- **`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. A standard proxy variable without a scheme (curl-style `host:port` or `user:pass@host:port`) is now read as an `http://` proxy (port 80 when none is given), the way Chromium's own environment reader took it: before this change browse rejected it with a warning and Chromium on Linux read the variable anyway, so that session was proxied; with the launch now explicit, reading it the same way keeps those sessions on their proxy instead of sending them direct. On macOS and Windows the same value is now a configured proxy too, where it used to be ignored in favour of the system proxy settings, so a daemon started under the old warning meets the mismatch refusal once. Only the plain `[user:pass@]host[:port]` shape is read that way; a value with a scheme, a path, a `/`, `\\`, `?` or `#` in the credentials, or a bare number goes to the URL parser as written, so a typo such as `http:/host:3128` keeps its diagnostic. A proxy URL with a path, query or fragment is now refused for every source (`--proxy` included), because that is where a `DOMAIN\\user` login or a `/` in a password lands after URL parsing and only the userinfo is redacted; percent-encode those characters. Credentials that are not valid percent-encoding (a literal `%`) are a proxy configuration error instead of a crash, for every source. For a standard variable each of these is a warning and a direct connection, like any other unusable value. A value that does not parse even as `http://` (an unsupported scheme, a bad port, a missing host) still warns and connects directly; `--proxy` and `BROWSE_PROXY_URL` still require the scheme.
|
||||
- **`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. A standard proxy variable without a scheme (curl-style `host:port` or `user:pass@host:port`) is now read as an `http://` proxy (port 80 when none is given), the way Chromium's own environment reader took it: before this change browse rejected it with a warning and Chromium on Linux read the variable anyway, so that session was proxied; with the launch now explicit, reading it the same way keeps those sessions on their proxy instead of sending them direct. On macOS and Windows the same value is now a configured proxy too, where it used to be ignored in favour of the system proxy settings, so a daemon started under the old warning meets the mismatch refusal once. Only the plain `[user:pass@]host[:port]` shape is read that way; a value with a scheme (including the slash-less `https:user:pass@host:port`, which the URL parser reads as `https://`), a `/`, `\`, `?`, `#`, `@` or whitespace anywhere else in the value, or a bare number goes to the URL parser as written, so a typo such as `http:/host:3128` keeps its diagnostic. A proxy URL with a path other than a bare `/`, a query or a fragment (a literal `?` or `#` anywhere) is now refused for every source (`--proxy` included), because that is where a `DOMAIN\user` login or a `/` in a password lands after URL parsing and only the userinfo is redacted; percent-encode those characters (for `http://` and `https://`; a `socks5://` userinfo keeps a backslash as written). Credentials that are not valid percent-encoding (a literal `%`) are a proxy configuration error instead of a crash, for every source. For a standard variable each of these parse problems is a warning and a direct connection, like any other unusable value; credentials given both in the value and in `BROWSE_PROXY_USER`/`BROWSE_PROXY_PASS` stay the hard error they have always been, for the scheme-less spelling too. A value that does not parse even as `http://` (an unsupported scheme, a bad port, a missing host) still warns and connects directly; `--proxy` and `BROWSE_PROXY_URL` still require the scheme.
|
||||
- **`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 from the session's own flags, 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`; the mismatch refusal offers `BROWSE_PROXY_URL=direct` as a remedy only where it would match the running daemon, on Linux or against a daemon started with it, and otherwise says to unset the variable that differs). 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), 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.
|
||||
@@ -15,7 +15,7 @@
|
||||
- `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. A scheme-less `HTTPS_PROXY` (`host:port`) is the configured proxy and the CONNECT reaches it (this case fails on the tree before the lenient parse); an `HTTPS_PROXY` that does not parse even as `http://` (`ftp://`) warns and still forces direct, so no CONNECT reaches the proxy it named. 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) and the remedy an env-proxied session gets against a system-proxy daemon (unset the variable, not `BROWSE_PROXY_URL=direct`, which that daemon would refuse); `buildRestartEnv` re-emits `BROWSE_PROXY_URL=direct` from the invocation's flags only, never from the persisted state, and a proxied invocation wins over it; `resolveProxy` reads scheme-less standard variables as `http://` (with and without `user:pass@`, IPv6, no port, surrounding whitespace), leaves a one-slash scheme and a bare number to the URL parser, gives `h:3128` and `http://h:3128` the same config hash, warns for an unsupported scheme, a bad port, a missing host, a credential that would spill past the userinfo (`/`, `\\`, `?`, `#`, including a `DOMAIN\\user` login) or one that is not percent-encoded, without echoing credentials, refuses a proxy URL with a path, query or fragment for every source, and still fails closed for `--proxy` and `BROWSE_PROXY_URL`.
|
||||
- `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) and the remedy an env-proxied session gets against a system-proxy daemon (unset the variable, not `BROWSE_PROXY_URL=direct`, which that daemon would refuse); `buildRestartEnv` re-emits `BROWSE_PROXY_URL=direct` from the invocation's flags only, never from the persisted state, and a proxied invocation wins over it; `resolveProxy` reads scheme-less standard variables as `http://` (with and without `user:pass@`, IPv6, no port, surrounding whitespace), leaves a one-slash scheme and a bare number to the URL parser, gives `h:3128` and `http://h:3128` the same config hash, warns for an unsupported scheme, a bad port, a missing host, a credential that would spill past the userinfo (`/`, `\`, `?`, `#`, including a `DOMAIN\user` login) or one that is not percent-encoded, without echoing credentials, refuses a proxy URL with a path other than a bare `/`, a query or a fragment (a bare trailing `?` or `#` included) for every source, keeps the protocol of a slash-less `https:user:pass@host:port` spelling, refuses mixed credential sources for the scheme-less spelling as for the `http://` one, and still fails closed for `--proxy` and `BROWSE_PROXY_URL`.
|
||||
- `browse/test/proxy-env-daemon.test.ts`: through the real CLI and daemon, a scheme-less `HTTPS_PROXY` with credentials starts the daemon on that `http://` proxy, redacted in the start line and the state, and an `ftp://` value warns and falls back to direct; `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.
|
||||
|
||||
|
||||
@@ -48,9 +48,9 @@ function percentDecodable(part: string): boolean {
|
||||
/**
|
||||
* Parse the BROWSE_PROXY_URL string and merge env-supplied creds.
|
||||
*
|
||||
* @throws ProxyConfigError on malformed URL, unsupported scheme, bad
|
||||
* credential percent-encoding, or ambiguous credentials (set in both URL
|
||||
* and env).
|
||||
* @throws ProxyConfigError on malformed URL, unsupported scheme, a path
|
||||
* (other than a bare `/`), query or fragment, bad credential
|
||||
* percent-encoding, or ambiguous credentials (set in both URL and env).
|
||||
*/
|
||||
export function parseProxyConfig(opts: {
|
||||
proxyUrl: string;
|
||||
@@ -89,7 +89,7 @@ export function parseProxyConfig(opts: {
|
||||
// put there came from a value that was not a proxy URL (a backslash in a
|
||||
// DOMAIN\user login, a '/' in a password) and would otherwise be printed
|
||||
// in the clear, because only the userinfo is redacted.
|
||||
if ((url.pathname !== '' && url.pathname !== '/') || url.search || url.hash) {
|
||||
if ((url.pathname !== '' && url.pathname !== '/') || url.search || url.hash || /[?#]/.test(opts.proxyUrl)) {
|
||||
throw new ProxyConfigError(
|
||||
'expected scheme://[user:pass@]host:port with no path; percent-encode any / \\ ? # @ in the credentials',
|
||||
`invalid proxy URL — unexpected path, query or fragment`,
|
||||
@@ -332,8 +332,12 @@ export function directProxyLogLine(env: NodeJS.ProcessEnv, platform: NodeJS.Plat
|
||||
* parser as written, so a typo such as `http:/host:3128` keeps the
|
||||
* diagnostic it had; parseProxyConfig then rejects any path, query or
|
||||
* fragment, so no part of a password can land outside the redacted userinfo.
|
||||
* A userinfo that opens with a known scheme (`https:bob:pw@host:3128`, the
|
||||
* slash-less spelling the URL parser reads as `https://bob:pw@host:3128`)
|
||||
* is a scheme, not credentials, so it is excluded too.
|
||||
*/
|
||||
const SCHEMELESS_PROXY_RE = /^(?:[^/\\?#@\s]+@)?(?:\[[0-9a-f:.]+\]|(?=[^:]*[a-z.])[^/\\?#@\s:[\]]+)(?::\d+)?$/i;
|
||||
const LEADING_SCHEME_RE = /^(?:https?|socks5h?):/i;
|
||||
|
||||
/**
|
||||
* Resolve the proxy for this invocation (#915). Env URLs go through
|
||||
@@ -360,7 +364,7 @@ export function resolveProxy(flagUrl: string | null, env: NodeJS.ProcessEnv): Re
|
||||
const name = STANDARD_PROXY_VARS.find(key => env[key]);
|
||||
if (!name) return direct();
|
||||
const value = env[name]!.trim();
|
||||
[source, raw] = [name, SCHEMELESS_PROXY_RE.test(value) ? `http://${value}` : value];
|
||||
[source, raw] = [name, SCHEMELESS_PROXY_RE.test(value) && !LEADING_SCHEME_RE.test(value) ? `http://${value}` : value];
|
||||
try {
|
||||
parseProxyConfig({ proxyUrl: raw });
|
||||
} catch (err) {
|
||||
|
||||
@@ -100,16 +100,30 @@ describe('resolveProxy precedence', () => {
|
||||
});
|
||||
|
||||
test('a proxy URL with a path, query or fragment is refused for every source, so a DOMAIN\\user login never prints its password', () => {
|
||||
for (const value of [['http://CORP\\jdoe', '1234@proxy.corp:3128'].join(':'), 'http://proxy.corp:3128/path', 'http://proxy.corp:3128?x=1', 'socks5://proxy.corp:1080#f']) {
|
||||
for (const value of [['http://CORP\\jdoe', '1234@proxy.corp:3128'].join(':'), 'http://proxy.corp:3128/path', 'http://proxy.corp:3128?x=1', 'socks5://proxy.corp:1080#f', 'http://proxy.corp:3128?', 'http://proxy.corp:3128#']) {
|
||||
expect(() => resolveProxy(value, {}), value).toThrow(/unexpected path, query or fragment/);
|
||||
const resolved = resolveProxy(null, { HTTPS_PROXY: value });
|
||||
expect(resolved.url, value).toBeNull();
|
||||
expect(resolved.warning, value).not.toContain('1234');
|
||||
}
|
||||
expect(resolveProxy('http://proxy.corp:3128/', {}).url).toBe('http://proxy.corp:3128/');
|
||||
expect(resolveProxy(null, { HTTPS_PROXY: 'http://proxy.corp:3128/' })).toMatchObject({ url: 'http://proxy.corp:3128/', warning: null });
|
||||
expect(resolveProxy('socks5://proxy.corp:1080', {}).url).toBe('socks5://proxy.corp:1080');
|
||||
});
|
||||
|
||||
test('a userinfo that opens with a scheme is a slash-less URL, not scheme-less credentials, so the protocol it names is kept', () => {
|
||||
expect(resolveProxy(null, { HTTPS_PROXY: ['https:bob', 'pw@proxy.corp:3128'].join(':') }).url).toBe(['https://bob', 'pw@proxy.corp:3128/'].join(':'));
|
||||
const socks = resolveProxy(null, { HTTPS_PROXY: 'socks5:bob@proxy.corp:1080' });
|
||||
expect(socks.url).toBeNull();
|
||||
expect(socks.warning).toContain('missing host');
|
||||
});
|
||||
|
||||
test('a scheme-less standard variable with credentials beside BROWSE_PROXY_USER is refused like its http:// spelling: mixed credential sources stay a hard error', () => {
|
||||
for (const value of ['bob:pw@proxy.corp:3128', ['http://bob', 'pw@proxy.corp:3128'].join(':')]) {
|
||||
expect(() => resolveProxy(null, { HTTPS_PROXY: value, BROWSE_PROXY_USER: 'alice' }), value).toThrow(/pick one source/);
|
||||
}
|
||||
});
|
||||
|
||||
test('scheme-less and http:// spellings of the same proxy share a config hash, so either shell reuses the daemon', () => {
|
||||
const a = extractGlobalFlags(['status'], { HTTPS_PROXY: 'h:3128' }, 'linux');
|
||||
const b = extractGlobalFlags(['status'], { HTTPS_PROXY: 'http://h:3128' }, 'linux');
|
||||
@@ -118,15 +132,15 @@ describe('resolveProxy precedence', () => {
|
||||
});
|
||||
|
||||
test('a standard variable that does not parse even as http:// warns and falls back to direct, without echoing it', () => {
|
||||
for (const value of ['ftp://proxy.corp:21', 'http://proxy.corp:99999', 'proxy.corp:notaport', 'http://:8080']) {
|
||||
const cred = ['user', 'secret@'].join(':');
|
||||
for (const value of [`ftp://${cred}proxy.corp:21`, `http://${cred}proxy.corp:99999`, `${cred}proxy.corp:notaport`, `http://${cred}:8080`]) {
|
||||
const resolved = resolveProxy(null, { https_proxy: value });
|
||||
expect(resolved).toMatchObject({ url: null, source: null, bypass: null });
|
||||
expect(resolved.warning).toStartWith('https_proxy is not a proxy URL browse can use (');
|
||||
expect(resolved.warning).toContain('fix: export https_proxy=http://host:port (or socks5://), or set BROWSE_PROXY_URL=direct');
|
||||
expect(resolved.warning).toContain('BROWSER.md#proxy-from-environment');
|
||||
expect(resolved.warning).not.toContain('secret');
|
||||
expect(resolved.warning, value).not.toContain('secret');
|
||||
}
|
||||
expect(resolveProxy(null, { https_proxy: 'user:secret@proxy.corp:notaport' }).warning).not.toContain('secret');
|
||||
});
|
||||
|
||||
test('--proxy and BROWSE_PROXY_URL are explicit and still fail closed, scheme required', () => {
|
||||
|
||||
Reference in new issue
Block a user