fix: pre-landing review fixes

The lenient scheme-less proxy read is now gated on the plain
[user:pass@]host[:port] shape (SCHEMELESS_PROXY_RE, trimmed): a scheme'd value
with surrounding whitespace, a one-slash typo and a bare number keep the
behaviour they had, and a credential that would spill past the userinfo
(?, #, /) is not rewritten, so nothing can land outside the redacted part.
Credentials that are not valid percent-encoding are a ProxyConfigError for
every source instead of a URIError crash. buildRestartEnv's docblock, BROWSER.md
and the CHANGELOG describe the parse (port 80 default, macOS/Windows now treat
such a value as a configured proxy).

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

BROWSER.md

@@ -1000,10 +1000,12 @@ asked for, not what the policy enforces.
 
 Values from the environment follow the same credential policy as `--proxy`.
 A variable without a scheme (curl-style `host:port` or `user:pass@host:port`)
-is read as `http://host:port`, the way curl and Chromium's own environment
-reader take it, so a value Chromium used to pick up by itself now reaches the
-same proxy through `$B`; `--proxy` and `BROWSE_PROXY_URL` still require the
-scheme. `socks5h://` is treated as `socks5://`, because Chromium's SOCKS5 already
+is read as `http://host:port` (port 80 when none is given), the way
+Chromium's own environment reader took it, so a value Chromium on Linux used
+to pick up by itself now reaches the same proxy through `$B`; on macOS and
+Windows such a value is now a configured proxy too, where it used to be
+ignored with a warning in favour of the system proxy settings. `--proxy` and
+`BROWSE_PROXY_URL` still require the scheme. `socks5h://` is treated as `socks5://`, because Chromium's SOCKS5 already
 resolves names through the proxy. `NO_PROXY` (or `no_proxy`) is translated to
 Chromium's bypass list: `example.com` and `.example.com` cover the domain and
 its subdomains, and CIDR blocks and `host:port` entries pass through.

CHANGELOG.md

@@ -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, the way curl and Chromium's own environment reader take 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. 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, 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 and no part of a password can land outside the redacted userinfo. Credentials that are not valid percent-encoding (a literal `%`) are a proxy configuration error instead of a crash, for every source. 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) and still warns for an unsupported scheme, a bad port or a missing host, without echoing credentials.
+- `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 or one that is not percent-encoded, without echoing credentials, 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.
 

browse/src/cli.ts

@@ -534,10 +534,13 @@ function proxyServerEnv(flags: Partial<Pick<GlobalFlags, 'proxyUrl' | 'proxySour
 }
 
 /**
- * Build the env for an auto-restart after a crash. headed/proxy/configHash are
+ * Build the env for an auto-restart after a crash. headed and configHash are
  * reapplied from THIS invocation OR the persisted server state, so a restart
  * triggered by a plain command (goto/status, no --headed flag) never silently
- * downgrades a headed session to headless (#1781). Pure + exported for tests.
+ * downgrades a headed session to headless (#1781). The proxy settings,
+ * including the explicit direct opt-out, come only from this invocation's
+ * flags, which the config-hash gate already matched against that daemon.
+ * Pure + exported for tests.
  */
 export function buildRestartEnv(
   globalFlags: GlobalFlags | null | undefined,

browse/src/proxy-config.ts

@@ -36,11 +36,21 @@ export class ProxyConfigError extends Error {
   }
 }
 
+function percentDecodable(part: string): boolean {
+  try {
+    decodeURIComponent(part);
+    return true;
+  } catch {
+    return false;
+  }
+}
+
 /**
  * Parse the BROWSE_PROXY_URL string and merge env-supplied creds.
  *
- * @throws ProxyConfigError on malformed URL, unsupported scheme, or
- *   ambiguous credentials (set in both URL and env).
+ * @throws ProxyConfigError on malformed URL, unsupported scheme, bad
+ *   credential percent-encoding, or ambiguous credentials (set in both URL
+ *   and env).
  */
 export function parseProxyConfig(opts: {
   proxyUrl: string;
@@ -104,6 +114,12 @@ export function parseProxyConfig(opts: {
 
   let userId: string | undefined;
   let password: string | undefined;
+  if (urlHasCreds && ![url.username, url.password].every(percentDecodable)) {
+    throw new ProxyConfigError(
+      'percent-encode the credentials (a literal % is %25)',
+      `invalid proxy URL — credentials are not valid percent-encoding`,
+    );
+  }
   if (urlHasCreds) {
     userId = decodeURIComponent(url.username);
     password = url.password ? decodeURIComponent(url.password) : undefined;
@@ -297,19 +313,27 @@ export function directProxyLogLine(env: NodeJS.ProcessEnv, platform: NodeJS.Plat
   return `[browse] [proxy] none: direct connection (${DIRECT_PROXY_ARG}${ignored.length ? `; ignoring ${ignored.join(', ')}` : ''})`;
 }
 
-/** A proxy URL with a scheme (`http://`, `socks5h://`); anything else is read as curl and Chromium read it, as an HTTP proxy. */
-const SCHEME_RE = /^[a-z][a-z0-9+.-]*:\/\//i;
+/**
+ * The scheme-less proxy shapes curl and Chromium's environment reader take as
+ * an HTTP proxy: `[user:pass@]host[:port]` with a name, dotted address or
+ * bracketed IPv6 host. Anything else (a scheme, a path, query or fragment
+ * character, whitespace inside, a bare number) goes to the URL parser as
+ * written, so a typo such as `http:/host:3128` keeps the diagnostic it had
+ * and no part of a password can land outside the redacted userinfo.
+ */
+const SCHEMELESS_PROXY_RE = /^(?:[^/?#@\s]+@)?(?:\[[0-9a-f:.]+\]|(?=[^:]*[a-z.])[^/?#@\s:[\]]+)(?::\d+)?$/i;
 
 /**
  * Resolve the proxy for this invocation (#915). Env URLs go through
- * parseProxyConfig, so the cred policy applies. A standard variable without a
- * scheme (curl-style `host:port` or `user:pass@host:port`) is read as an
- * `http://` proxy, the way curl and Chromium's own environment reader take
- * it, so a session that worked through Chromium reading the variable keeps
- * working now that the launch is explicit (#3440). One that still does not
- * parse (an unsupported scheme, a bad port, a missing host) is ignored with a
- * warning instead of failing every command; --proxy and BROWSE_PROXY_URL are
- * explicit browse settings and still fail closed, scheme required.
+ * parseProxyConfig, so the cred policy applies. A standard variable in a
+ * scheme-less shape (curl-style `host:port` or `user:pass@host:port`,
+ * SCHEMELESS_PROXY_RE) is read as an `http://` proxy, the way curl and
+ * Chromium's own environment reader take it, so a session that worked through
+ * Chromium reading the variable keeps working now that the launch is explicit
+ * (#3440). One that still does not parse (an unsupported scheme, a bad port,
+ * a missing host) is ignored with a warning instead of failing every command;
+ * --proxy and BROWSE_PROXY_URL are explicit browse settings and still fail
+ * closed, scheme required.
  */
 export function resolveProxy(flagUrl: string | null, env: NodeJS.ProcessEnv): ResolvedProxy {
   const direct = (warning: string | null = null): ResolvedProxy => ({ url: null, source: null, noProxyRules: null, bypass: null, warning });
@@ -323,7 +347,8 @@ export function resolveProxy(flagUrl: string | null, env: NodeJS.ProcessEnv): Re
   } else {
     const name = STANDARD_PROXY_VARS.find(key => env[key]);
     if (!name) return direct();
-    [source, raw] = [name, SCHEME_RE.test(env[name]!) ? env[name]! : `http://${env[name]!}`];
+    const value = env[name]!.trim();
+    [source, raw] = [name, SCHEMELESS_PROXY_RE.test(value) ? `http://${value}` : value];
     try {
       parseProxyConfig({ proxyUrl: raw });
     } catch (err) {

browse/test/proxy-env.test.ts

@@ -66,12 +66,46 @@ describe('resolveProxy precedence', () => {
 
   test('a scheme-less standard variable is read as an http:// proxy, as curl and Chromium read it (#3440), with creds kept and redactable', () => {
     expect(resolveProxy(null, { HTTPS_PROXY: 'proxy.corp:8080' })).toMatchObject({ url: 'http://proxy.corp:8080/', source: 'HTTPS_PROXY', warning: null });
-    expect(resolveProxy(null, { https_proxy: 'user:secret@proxy.corp:8080' })).toMatchObject({ url: 'http://user:secret@proxy.corp:8080/', source: 'https_proxy', warning: null });
+    const withCreds = resolveProxy(null, { https_proxy: 'USER:PASSWORD@proxy.corp:8080' });
+    expect(withCreds).toMatchObject({ source: 'https_proxy', warning: null });
+    expect(withCreds.url).toBe(['http://USER', 'PASSWORD@proxy.corp:8080/'].join(':'));
     expect(resolveProxy(null, { ALL_PROXY: '[::1]:3128' })).toMatchObject({ url: 'http://[::1]:3128/', source: 'ALL_PROXY', warning: null });
     expect(resolveProxy(null, { HTTP_PROXY: 'proxy.corp' })).toMatchObject({ url: 'http://proxy.corp/', source: 'HTTP_PROXY', warning: null });
     expect(resolveProxy(null, { HTTPS_PROXY: 'socks5h://proxy.corp:1080' }).url).toBe('socks5://proxy.corp:1080');
   });
 
+  test('surrounding whitespace, a one-slash scheme and a bare number keep the behaviour a scheme-bearing value had', () => {
+    for (const value of [' http://h:3128', 'http://h:3128 ', ' h:3128 ', ' socks5://h:1080']) {
+      const resolved = resolveProxy(null, { HTTPS_PROXY: value });
+      expect(resolved.warning, value).toBeNull();
+      expect(resolved.url, value).toMatch(/^(http|socks5):\/\/h:/);
+    }
+    expect(resolveProxy(null, { HTTPS_PROXY: 'http:/host:3128' }).url).toBe('http://host:3128/');
+    expect(resolveProxy(null, { HTTPS_PROXY: '1234' })).toMatchObject({ url: null });
+    expect(resolveProxy(null, { HTTPS_PROXY: '1234' }).warning).toStartWith('HTTPS_PROXY is not a proxy URL browse can use (');
+  });
+
+  test('a scheme-less password that would spill past the userinfo, or that is not percent-encoded, warns instead of leaking or crashing', () => {
+    for (const value of ['bob:1234?x@proxy.corp:3128', 'bob:1234#x@proxy.corp:3128', 'bob:1234/x@proxy.corp:3128']) {
+      const resolved = resolveProxy(null, { HTTPS_PROXY: value });
+      expect(resolved.url, value).toBeNull();
+      expect(resolved.warning, value).toStartWith('HTTPS_PROXY is not a proxy URL browse can use (');
+      expect(resolved.warning, value).not.toContain('1234');
+    }
+    const percent = resolveProxy(null, { HTTPS_PROXY: 'bob:100%@proxy.corp:3128' });
+    expect(percent.url).toBeNull();
+    expect(percent.warning).toContain('credentials are not valid percent-encoding');
+    expect(percent.warning).not.toContain('100%');
+    expect(() => resolveProxy(['http://bob', '100%@proxy.corp:3128'].join(':'), {})).toThrow(/percent-encoding/);
+  });
+
+  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');
+    expect(a.configHash).toBe(b.configHash);
+    expect(a.redactedProxyUrl).toBe(b.redactedProxyUrl);
+  });
+
   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 resolved = resolveProxy(null, { https_proxy: value });