From c2f52c2522972991c73c20a7c73c2cb30a11cea4 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 6 Oct 2026 14:58:49 +0530 Subject: [PATCH 01/11] fix(core): route direct font fetches through the browser's --proxy-server Font responses are re-fetched from Node (makeDirectRequest) because browser bodies can be badly encoded. That Node request only honoured HTTP(S)_PROXY, so when Chrome was launched with `--proxy-server` (e.g. the visual scanner sending traffic through privoxy to a BrowserStack Local tunnel), every font on a tunnel-only host failed with `getaddrinfo ENOTFOUND` and was dropped from the snapshot while the rest of the page captured fine. directFetch now resolves the proxy Chrome itself would use for the URL from the launch args (`--proxy-server`, per-scheme rules, `--proxy-bypass-list` incl. implicit loopback bypass and `<-loopback>`), and @percy/client's request() accepts an explicit `proxy` that takes precedence over the env vars, NO_PROXY and PAC. SOCKS/direct rules fall back to the existing env behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/client/src/proxy.js | 30 ++++++---- packages/client/src/utils.js | 8 ++- packages/client/test/unit/proxy.test.js | 19 ++++++ packages/client/test/unit/request.test.js | 10 ++++ packages/core/src/network.js | 11 +++- packages/core/src/utils.js | 52 ++++++++++++++++ packages/core/test/discovery.test.js | 59 +++++++++++++++++++ packages/core/test/unit/utils.test.js | 72 ++++++++++++++++++++++- 8 files changed, 245 insertions(+), 16 deletions(-) diff --git a/packages/client/src/proxy.js b/packages/client/src/proxy.js index 21c4b1b7e..0be357c22 100644 --- a/packages/client/src/proxy.js +++ b/packages/client/src/proxy.js @@ -86,15 +86,16 @@ export function href(options) { (path || `${pathname || ''}${search || ''}${hash || ''}`); }; -// Returns the proxy URL for a set of request options -export function getProxy(options) { - let proxyUrl = (options.protocol === 'https:' && +// Returns the proxy URL for a set of request options. An explicit `override` proxy URL is +// used as-is, without consulting the proxy env vars or NO_PROXY; the caller owns bypassing. +export function getProxy(options, override) { + let proxyUrl = override || (options.protocol === 'https:' && (process.env.https_proxy || process.env.HTTPS_PROXY)) || (process.env.http_proxy || process.env.HTTP_PROXY); - let shouldProxy = !!proxyUrl && !hostnameMatches( + let shouldProxy = !!override || (!!proxyUrl && !hostnameMatches( stripQuotesAndSpaces(process.env.no_proxy || process.env.NO_PROXY) - , href(options)); + , href(options))); if (proxyUrl && typeof proxyUrl === 'string') { proxyUrl = stripQuotesAndSpaces(proxyUrl); } @@ -128,8 +129,14 @@ export class ProxyHttpAgent extends http.Agent { // needed for https proxies httpsAgent = new https.Agent({ keepAlive: true }); + // an optional `proxy` URL takes precedence over the proxy env vars + constructor({ proxy, ...options } = {}) { + super(options); + this.proxy = proxy; + } + addRequest(request, options) { - let proxy = getProxy(options); + let proxy = getProxy(options, this.proxy); if (!proxy) return super.addRequest(request, options); logger('client:proxy').debug(`Proxying request: ${options.href}`); @@ -168,13 +175,15 @@ export class ProxyHttpAgent extends http.Agent { // Proxified https agent export class ProxyHttpsAgent extends https.Agent { - constructor(options) { + // an optional `proxy` URL takes precedence over the proxy env vars + constructor({ proxy, ...options } = {}) { // default keep-alive super({ keepAlive: true, ...options }); + this.proxy = proxy; } createConnection(options, callback) { - let proxy = getProxy(options); + let proxy = getProxy(options, this.proxy); if (!proxy) return super.createConnection(options, callback); logger('client:proxy').debug(`Proxying request: ${href(options)}`); @@ -244,6 +253,7 @@ export function proxyAgentFor(url, options) { let cache = (proxyAgentFor.cache ||= new Map()); let { protocol, hostname } = new URL(url); let cachekey = `${protocol}//${hostname}`; + if (options?.proxy) cachekey += ` via ${options.proxy}`; // If we already have a cached agent, return it if (cache.has(cachekey)) { @@ -254,8 +264,8 @@ export function proxyAgentFor(url, options) { let agent; const pacUrl = process.env.PERCY_PAC_FILE_URL; - // If PAC URL is provided, use PAC proxy - if (pacUrl) { + // If PAC URL is provided, use PAC proxy (an explicit proxy takes precedence) + if (pacUrl && !options?.proxy) { logger('client:proxy').info(`Using PAC file from: ${pacUrl}`); agent = createPacAgent(pacUrl, options); } else { diff --git a/packages/client/src/utils.js b/packages/client/src/utils.js index f5a3927b1..7fe84ab7f 100644 --- a/packages/client/src/utils.js +++ b/packages/client/src/utils.js @@ -124,7 +124,8 @@ const RETRY_ERROR_CODES = [ // and any received error details. Server 500 errors are retried up to 5 times at 50ms intervals by // default, and 404 errors may also be optionally retried. If a callback is provided, it is called // with the parsed response body and response details. If the callback returns a value, that value -// will be returned in the final resolved promise instead of the response body. +// will be returned in the final resolved promise instead of the response body. A `proxy` URL +// routes the request through that proxy regardless of the proxy env vars. export async function request(url, options = {}, callback) { // accept `request(url, callback)` if (typeof options === 'function') [options, callback] = [{}, options]; @@ -132,7 +133,7 @@ export async function request(url, options = {}, callback) { // gather request options let { body, headers, retries, retryNotFound, - interval, noProxy, buffer, meta = {}, ...requestOptions + interval, noProxy, proxy, buffer, meta = {}, ...requestOptions } = options; let { protocol, hostname, port, pathname, search, hash } = new URL(url); @@ -150,7 +151,8 @@ export async function request(url, options = {}, callback) { // combine request options Object.assign(requestOptions, { - agent: requestOptions.agent || (!noProxy && proxyAgentFor(url)) || null, + agent: requestOptions.agent || (proxy && proxyAgentFor(url, { proxy })) || + (!noProxy && proxyAgentFor(url)) || null, path: pathname + search + hash, protocol, hostname, diff --git a/packages/client/test/unit/proxy.test.js b/packages/client/test/unit/proxy.test.js index 7bc93a695..aa83f6524 100644 --- a/packages/client/test/unit/proxy.test.js +++ b/packages/client/test/unit/proxy.test.js @@ -14,6 +14,16 @@ describe('proxy', () => { expect(proxy).toBeInstanceOf(Object); }); + it('should prefer an explicit proxy over env and NO_PROXY', () => { + process.env.no_proxy = '*'; + const options = { protocol: 'https:', hostname: 'example.com' }; + expect(getProxy(options)).toBeUndefined(); + expect(getProxy(options, 'http://other.com:3128')).toEqual(jasmine.objectContaining({ + host: 'other.com', port: '3128' + })); + delete process.env.no_proxy; + }); + it('should return undefined if no proxy is set', () => { delete process.env.http_proxy; const options = { protocol: 'http:', hostname: 'example.com' }; @@ -82,6 +92,15 @@ describe('proxy', () => { expect(agent).toBeInstanceOf(PacProxyAgent); }); + it('should create a separately cached agent for an explicit proxy, ignoring PAC', () => { + const url = 'https://example.com'; + const agent = proxyAgentFor(url, { proxy: 'http://localhost:8118' }); + expect(agent).toBeInstanceOf(ProxyHttpsAgent); + expect(agent.proxy).toBe('http://localhost:8118'); + expect(proxyAgentFor(url, { proxy: 'http://localhost:8118' })).toBe(agent); + expect(proxyAgentFor(url)).toBeInstanceOf(PacProxyAgent); + }); + it('logs an error and throws when proxy agent creation fails', () => { const url = 'http://example.com'; const options = {}; diff --git a/packages/client/test/unit/request.test.js b/packages/client/test/unit/request.test.js index 5666ce002..5b14c7401 100644 --- a/packages/client/test/unit/request.test.js +++ b/packages/client/test/unit/request.test.js @@ -420,6 +420,16 @@ describe('Unit / Request', () => { ]); }); + it('proxies requests through an explicit `proxy` option over env and NO_PROXY', async () => { + delete process.env[env]; + process.env.NO_PROXY = 'localhost'; + + await expectAsync(server.request('/test', { proxy: proxy.address })) + .toBeResolvedTo('test proxied'); + await expectAsync(server.request('/test')) + .toBeResolvedTo('test'); + }); + it('does not proxy requests matching NO_PROXY', async () => { process.env.NO_PROXY = 'localhost'; diff --git a/packages/core/src/network.js b/packages/core/src/network.js index d5ef21f5b..dff2e9705 100644 --- a/packages/core/src/network.js +++ b/packages/core/src/network.js @@ -2,7 +2,7 @@ import { request as makeRequest } from '@percy/client/utils'; import logger from '@percy/logger'; import mime from 'mime-types'; import dns from 'dns'; -import { AbortError, DefaultMap, createResource, hostnameMatches, normalizeURL, waitFor, decodeAndEncodeURLWithLogging, handleIncorrectFontMimeType, executeDomainValidation, isMetadataTarget, isMetadataIP } from './utils.js'; +import { AbortError, DefaultMap, createResource, hostnameMatches, normalizeURL, waitFor, decodeAndEncodeURLWithLogging, handleIncorrectFontMimeType, executeDomainValidation, isMetadataTarget, isMetadataIP, browserProxyFor } from './utils.js'; export const MAX_RESOURCE_SIZE = 25 * (1024 ** 2) * 0.63; // 25MB, 0.63 factor for accounting for base64 encoding // CDP returns binary bodies via Network.getResponseBody as base64 in the JSON-RPC @@ -639,8 +639,15 @@ export class Network { cb(err, address, family); }); + // Take the same route as the browser: when Chrome was launched with `--proxy-server`, hosts + // may only resolve through that proxy (e.g. privoxy in front of a BrowserStack Local tunnel). + // The proxy then resolves the target, so as on the browser path (where remoteIPAddress is the + // proxy's) the connected-IP metadata gate only sees the proxy address. + let proxy = browserProxyFor(this.page.session?.browser?.args, request.url); + if (proxy) this.log.debug('- Requesting directly through the browser proxy', this.meta); + let { body, status, headers: responseHeaders } = await makeRequest( - request.url, { buffer: true, headers, lookup }, (body, res) => ({ + request.url, { buffer: true, headers, lookup, proxy }, (body, res) => ({ body, status: res.statusCode, headers: res.headers })); diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index 2bea8fed0..4034d1cff 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -152,6 +152,58 @@ export function isMetadataIP(remoteIP) { return matchMetadataHost(remoteIP); } +// Returns the proxy URL Chrome would route `url` through, given the browser's launch `args` +// (`--proxy-server` and `--proxy-bypass-list`), so Node-side fetches made on the browser's behalf +// take the same route. Hosts reachable only through that proxy (e.g. a BrowserStack Local tunnel) +// otherwise fail DNS from Node. Only http(s) proxies are supported: SOCKS and `direct://` rules, +// like a bypassed host, return undefined and leave the fetch to the proxy env vars. +export function browserProxyFor(args, url) { + // Chrome honours the last occurrence of a repeated switch + let flag = name => [].concat(args ?? []).reverse() + .find(arg => arg.startsWith(`--${name}=`))?.slice(name.length + 3); + + let server = flag('proxy-server'); + if (!server) return; + + let { protocol, hostname, port } = new URL(url); + let scheme = protocol.slice(0, -1); + + // rules are `[=][,...]` separated by `;` + let rules = server.split(';').map(rule => rule.trim()); + let rule = rules.find(rule => rule.startsWith(`${scheme}=`)) ?? rules.find(rule => !rule.includes('=')); + if (!rule) return; + + let proxy = rule.replace(/^\w+=/, '').split(',')[0].trim(); + if (!proxy.includes('://')) proxy = `http://${proxy}`; + if (!/^https?:\/\//.test(proxy)) return; + + port ||= scheme === 'https' ? '443' : '80'; + if (bypassesBrowserProxy(flag('proxy-bypass-list'), hostname, port)) return; + return proxy; +} + +// Mirrors Chrome's `--proxy-bypass-list` matching: `,`/`;` separated host globs with an optional +// scheme and port, `.host` meaning `*.host`, `` for dotless hosts, and loopback hosts +// bypassed implicitly unless the list contains `<-loopback>`. +function bypassesBrowserProxy(list = '', hostname, port) { + let rules = list.split(/[,;]/).map(rule => rule.trim()).filter(Boolean); + + if (!rules.includes('<-loopback>') && + /^(localhost|127(\.\d+){3}|\[::1\])$|\.localhost$/.test(hostname)) return true; + + return rules.some(rule => { + if (rule === '') return !hostname.includes('.'); + if (rule === '<-loopback>') return false; + + let [, host, rulePort] = rule.replace(/^\w+:\/\//, '').match(/^(.+?)(?::(\d+))?$/); + if (rulePort && rulePort !== port) return false; + if (host.startsWith('.')) host = `*${host}`; + + let glob = host.toLowerCase().replace(/[.+?^${}()|[\]\\]/g, '\\$&').replace(/\*/g, '.*'); + return new RegExp(`^${glob}$`).test(hostname); + }); +} + // Throws when the URL points at a cloud instance-metadata endpoint. Used to // refuse navigating the top-level snapshot URL to such a target. export function assertNotMetadataTarget(rawUrl) { diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js index 4d64606ce..297a371da 100644 --- a/packages/core/test/discovery.test.js +++ b/packages/core/test/discovery.test.js @@ -2649,6 +2649,65 @@ describe('Discovery', () => { }); }); + describe('with a browser proxy', () => { + // `tunnel.test` never resolves (RFC 2606), so like a host behind a BrowserStack Local + // tunnel it is reachable only through the proxy. The test server doubles as that proxy: + // it routes absolute-form requests (`GET http://tunnel.test/font.woff`) by path. + const proxiedFontDOM = dedent` + + + + + +

Hello Percy!

+ ${' '.repeat(1000)} + + + `; + + beforeEach(async () => { + await percy.stop(true); + + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1, + allowedHostnames: ['tunnel.test'], + launchOptions: { args: ['--proxy-server=http://localhost:8000'] } + } + }); + + percy.loglevel('debug'); + }); + + it('re-fetches fonts through the proxy the browser was launched with', async () => { + await percy.snapshot({ + name: 'proxied font snapshot', + url: 'http://localhost:8000', + domSnapshot: proxiedFontDOM + }); + + await percy.idle(); + + expect(logger.stderr).toContain( + '[percy:core:discovery] - Requesting directly through the browser proxy' + ); + expect(logger.stderr).not.toContain(jasmine.stringContaining('ENOTFOUND')); + expect(captured[0]).toEqual(jasmine.arrayContaining([ + jasmine.objectContaining({ + id: sha256hash(''), + attributes: jasmine.objectContaining({ + 'resource-url': 'http://tunnel.test/font.woff' + }) + }) + ])); + }); + }); + describe('resource caching', () => { let snapshot = async n => { await percy.snapshot({ diff --git a/packages/core/test/unit/utils.test.js b/packages/core/test/unit/utils.test.js index cbd708f78..cf7d9e484 100644 --- a/packages/core/test/unit/utils.test.js +++ b/packages/core/test/unit/utils.test.js @@ -6,10 +6,80 @@ import { yieldAll, DefaultMap, redactSecrets, - base64encode + base64encode, + browserProxyFor } from '../../src/utils.js'; describe('Unit / Utils', () => { + describe('browserProxyFor', () => { + const proxy = 'http://127.0.0.1:8118'; + const args = (server, bypass) => [ + '--headless', `--proxy-server=${server}`, + ...(bypass != null ? [`--proxy-bypass-list=${bypass}`] : []) + ]; + + it('returns undefined without a --proxy-server arg', () => { + expect(browserProxyFor(undefined, 'https://a.com')).toBeUndefined(); + expect(browserProxyFor(['--headless'], 'https://a.com')).toBeUndefined(); + }); + + it('returns the proxy for http and https urls', () => { + expect(browserProxyFor(args(proxy), 'https://site.example/font.woff')).toBe(proxy); + expect(browserProxyFor(args(proxy), 'http://site.example/font.woff')).toBe(proxy); + }); + + it('uses the last occurrence of a repeated switch', () => { + expect(browserProxyFor([...args('http://first:1'), ...args(proxy)], 'https://a.com')).toBe(proxy); + }); + + it('defaults a bare host:port to an http proxy and takes the first fallback', () => { + expect(browserProxyFor(args('127.0.0.1:8118,direct://'), 'https://a.com')).toBe(proxy); + expect(browserProxyFor(args('https://secure:443'), 'https://a.com')).toBe('https://secure:443'); + }); + + it('picks per-scheme rules, falling back to a scheme-less rule', () => { + let server = 'https=127.0.0.1:8118;ftp=ftp:21'; + expect(browserProxyFor(args(server), 'https://a.com')).toBe(proxy); + expect(browserProxyFor(args(server), 'http://a.com')).toBeUndefined(); + expect(browserProxyFor(args('ftp=ftp:21;127.0.0.1:8118'), 'http://a.com')).toBe(proxy); + }); + + it('returns undefined for socks and direct proxies', () => { + expect(browserProxyFor(args('socks5://127.0.0.1:1080'), 'https://a.com')).toBeUndefined(); + expect(browserProxyFor(args('direct://'), 'https://a.com')).toBeUndefined(); + }); + + it('bypasses loopback hosts unless <-loopback> is listed', () => { + for (let url of ['http://localhost:8000', 'http://127.0.0.1', 'http://[::1]', 'http://app.localhost']) { + expect(browserProxyFor(args(proxy), url)).toBeUndefined(); + expect(browserProxyFor(args(proxy, '<-loopback>'), url)).toBe(proxy); + } + }); + + it('honours the bypass list', () => { + // the scanner's real bypass list + let bypass = '<-loopback>,ws.pusherapp.com,.pusher.com,ssl.gstatic.com,.google.com'; + expect(browserProxyFor(args(proxy, bypass), 'https://ws.pusherapp.com/x')).toBeUndefined(); + expect(browserProxyFor(args(proxy, bypass), 'https://js.pusher.com/x')).toBeUndefined(); + expect(browserProxyFor(args(proxy, bypass), 'https://fonts.google.com/x')).toBeUndefined(); + expect(browserProxyFor(args(proxy, bypass), 'https://fonts.gstatic.com/x')).toBe(proxy); + expect(browserProxyFor(args(proxy, bypass), 'https://nestscheme-sit6.uk.tapue.com/f.woff2')).toBe(proxy); + }); + + it('matches bypass globs, schemes, ports and ', () => { + expect(browserProxyFor(args(proxy, '*.cdn.com'), 'https://a.cdn.com')).toBeUndefined(); + expect(browserProxyFor(args(proxy, '*cdn.com'), 'https://mycdn.com')).toBeUndefined(); + expect(browserProxyFor(args(proxy, '*'), 'https://any.where')).toBeUndefined(); + expect(browserProxyFor(args(proxy, 'https://a.com'), 'https://a.com')).toBeUndefined(); + expect(browserProxyFor(args(proxy, 'a.com:443'), 'https://a.com')).toBeUndefined(); + expect(browserProxyFor(args(proxy, 'a.com:8443'), 'https://a.com')).toBe(proxy); + expect(browserProxyFor(args(proxy, 'a.com:80'), 'http://a.com')).toBeUndefined(); + expect(browserProxyFor(args(proxy, ''), 'http://intranet')).toBeUndefined(); + expect(browserProxyFor(args(proxy, ''), 'http://a.com')).toBe(proxy); + expect(browserProxyFor(args(proxy, ' ; a.com'), 'https://b.com')).toBe(proxy); + }); + }); + describe('generatePromise', () => { it('accepts a generator and returns a promise', async () => { let gen = (function*(done) { while (!done) done = yield; })(); From b671272d3bd64ebc7706870937ac5c443662622c Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 6 Oct 2026 19:12:11 +0530 Subject: [PATCH 02/11] fix(core): match proxy bypass globs without a dynamic RegExp Address CodeQL and semgrep findings on the browser-proxy change: - semgrep (detect-non-literal-regexp): bypass-list globs are now matched by a linear `*` wildcard matcher instead of building a RegExp from user input. - CodeQL (polynomial regex on library input): an explicit proxy override is used verbatim; only env-sourced proxy URLs go through stripQuotesAndSpaces. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/client/src/proxy.js | 3 ++- packages/core/src/utils.js | 20 +++++++++++++++++--- packages/core/test/unit/utils.test.js | 4 ++++ 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/packages/client/src/proxy.js b/packages/client/src/proxy.js index 0be357c22..500682ee1 100644 --- a/packages/client/src/proxy.js +++ b/packages/client/src/proxy.js @@ -97,7 +97,8 @@ export function getProxy(options, override) { stripQuotesAndSpaces(process.env.no_proxy || process.env.NO_PROXY) , href(options))); - if (proxyUrl && typeof proxyUrl === 'string') { proxyUrl = stripQuotesAndSpaces(proxyUrl); } + // only env values may carry stray quotes/spaces; an explicit override is used verbatim + if (!override && proxyUrl && typeof proxyUrl === 'string') { proxyUrl = stripQuotesAndSpaces(proxyUrl); } if (shouldProxy) { proxyUrl = new URL(proxyUrl); diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index 4034d1cff..10508ba87 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -198,12 +198,26 @@ function bypassesBrowserProxy(list = '', hostname, port) { let [, host, rulePort] = rule.replace(/^\w+:\/\//, '').match(/^(.+?)(?::(\d+))?$/); if (rulePort && rulePort !== port) return false; if (host.startsWith('.')) host = `*${host}`; - - let glob = host.toLowerCase().replace(/[.+?^${}()|[\]\\]/g, '\\$&').replace(/\*/g, '.*'); - return new RegExp(`^${glob}$`).test(hostname); + return globMatches(host.toLowerCase(), hostname); }); } +// Returns true when `subject` matches `glob`, where `*` matches any run of characters. A linear +// greedy matcher with single-star backtracking, so user-supplied patterns need no RegExp. +function globMatches(glob, subject) { + let [g, s, star, mark] = [0, 0, -1, 0]; + + while (s < subject.length) { + if (glob[g] === '*') [star, mark] = [g++, s]; + else if (glob[g] === subject[s]) [g, s] = [g + 1, s + 1]; + else if (star !== -1) [g, s] = [star + 1, ++mark]; + else return false; + } + + while (glob[g] === '*') g++; + return g === glob.length; +} + // Throws when the URL points at a cloud instance-metadata endpoint. Used to // refuse navigating the top-level snapshot URL to such a target. export function assertNotMetadataTarget(rawUrl) { diff --git a/packages/core/test/unit/utils.test.js b/packages/core/test/unit/utils.test.js index cf7d9e484..8613b32ff 100644 --- a/packages/core/test/unit/utils.test.js +++ b/packages/core/test/unit/utils.test.js @@ -70,6 +70,10 @@ describe('Unit / Utils', () => { expect(browserProxyFor(args(proxy, '*.cdn.com'), 'https://a.cdn.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, '*cdn.com'), 'https://mycdn.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, '*'), 'https://any.where')).toBeUndefined(); + expect(browserProxyFor(args(proxy, 'cdn.*'), 'https://cdn.example.com')).toBeUndefined(); + expect(browserProxyFor(args(proxy, 'CDN.*.com'), 'https://cdn.a.b.com')).toBeUndefined(); + expect(browserProxyFor(args(proxy, '*.cdn.com'), 'https://cdn.com')).toBe(proxy); + expect(browserProxyFor(args(proxy, 'cdn.*.net'), 'https://cdn.a.com')).toBe(proxy); expect(browserProxyFor(args(proxy, 'https://a.com'), 'https://a.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, 'a.com:443'), 'https://a.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, 'a.com:8443'), 'https://a.com')).toBe(proxy); From 3b71d9dce0ab09154266928ca686ec215b2b25f9 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 6 Oct 2026 19:31:22 +0530 Subject: [PATCH 03/11] fix: mirror more of Chrome's proxy rules; stop false tunnel-close errors browserProxyFor (review feedback): - use the `socks=` mapping as the fallback for schemes without their own (an http proxy there is honoured; a scheme-less one is SOCKS4, unsupported) - implicitly bypass link-local hosts (169.254/16, fe80::/10) like loopback - enforce scheme-restricted bypass rules and match CIDR rules (IPv4/IPv6) against IP-literal hosts ProxyHttpsAgent: detach the CONNECT handshake's error/close listeners once the tunnel is established. Previously the normal keep-alive teardown of a proxied https connection logged "Proxying request ... failed: Connection closed while sending request to upstream proxy" plus a network warning and re-invoked the connection callback. Seen on every proxied run, including HTTP_PROXY on master. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/client/src/proxy.js | 2 + packages/client/test/unit/request.test.js | 10 ++++ packages/core/src/utils.js | 64 ++++++++++++++++++----- packages/core/test/unit/utils.test.js | 30 ++++++++++- 4 files changed, 90 insertions(+), 16 deletions(-) diff --git a/packages/client/src/proxy.js b/packages/client/src/proxy.js index 500682ee1..0d3a1b7ee 100644 --- a/packages/client/src/proxy.js +++ b/packages/client/src/proxy.js @@ -235,6 +235,8 @@ export class ProxyHttpsAgent extends https.Agent { )); } + // the tunnel is established; its later close (e.g. keep-alive teardown) is not a failure + socket.off('error', handleError).off('close', handleClose); options.socket = socket; options.servername = options.hostname; // callback not passed in so not to be added as a listener diff --git a/packages/client/test/unit/request.test.js b/packages/client/test/unit/request.test.js index 5b14c7401..5fb4d1d5a 100644 --- a/packages/client/test/unit/request.test.js +++ b/packages/client/test/unit/request.test.js @@ -430,6 +430,16 @@ describe('Unit / Request', () => { .toBeResolvedTo('test'); }); + it('does not report an error when an established proxy tunnel later closes', async () => { + await expectAsync(server.request('/test')).toBeResolvedTo('test proxied'); + // the keep-alive connection to the proxy is torn down after a successful request + await proxy.close(); + await new Promise(r => setTimeout(r, 50)); + + expect(logger.stderr).not.toContain(jasmine.stringContaining( + 'Connection closed while sending request to upstream proxy')); + }); + it('does not proxy requests matching NO_PROXY', async () => { process.env.NO_PROXY = 'localhost'; diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index 10508ba87..b081ebada 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -2,6 +2,7 @@ import EventEmitter from 'events'; import { sha256hash, request } from '@percy/client/utils'; import { camelcase, merge } from '@percy/config/utils'; import YAML from 'yaml'; +import net from 'net'; import path from 'path'; import url from 'url'; import { readFileSync } from 'fs'; @@ -156,7 +157,8 @@ export function isMetadataIP(remoteIP) { // (`--proxy-server` and `--proxy-bypass-list`), so Node-side fetches made on the browser's behalf // take the same route. Hosts reachable only through that proxy (e.g. a BrowserStack Local tunnel) // otherwise fail DNS from Node. Only http(s) proxies are supported: SOCKS and `direct://` rules, -// like a bypassed host, return undefined and leave the fetch to the proxy env vars. +// like a bypassed host, return undefined and leave the fetch to the proxy env vars. Only the first +// proxy of a fallback list is used; Chrome's failover to later entries is not mirrored. export function browserProxyFor(args, url) { // Chrome honours the last occurrence of a repeated switch let flag = name => [].concat(args ?? []).reverse() @@ -168,40 +170,74 @@ export function browserProxyFor(args, url) { let { protocol, hostname, port } = new URL(url); let scheme = protocol.slice(0, -1); - // rules are `[=][,...]` separated by `;` + // rules are `[=][,...]` separated by `;`, where the `socks=` + // mapping is the fallback for schemes without a mapping of their own let rules = server.split(';').map(rule => rule.trim()); - let rule = rules.find(rule => rule.startsWith(`${scheme}=`)) ?? rules.find(rule => !rule.includes('=')); + let mapping = name => rules.find(rule => rule.toLowerCase().startsWith(`${name}=`)); + let rule = mapping(scheme) ?? rules.find(rule => !rule.includes('=')) ?? mapping('socks'); if (!rule) return; + // a proxy without a scheme is http, or SOCKS4 within the `socks=` mapping let proxy = rule.replace(/^\w+=/, '').split(',')[0].trim(); - if (!proxy.includes('://')) proxy = `http://${proxy}`; + if (!proxy.includes('://')) proxy = `${rule === mapping('socks') ? 'socks4' : 'http'}://${proxy}`; if (!/^https?:\/\//.test(proxy)) return; port ||= scheme === 'https' ? '443' : '80'; - if (bypassesBrowserProxy(flag('proxy-bypass-list'), hostname, port)) return; + if (bypassesBrowserProxy(flag('proxy-bypass-list'), { scheme, hostname, port })) return; return proxy; } -// Mirrors Chrome's `--proxy-bypass-list` matching: `,`/`;` separated host globs with an optional -// scheme and port, `.host` meaning `*.host`, `` for dotless hosts, and loopback hosts -// bypassed implicitly unless the list contains `<-loopback>`. -function bypassesBrowserProxy(list = '', hostname, port) { +// Mirrors Chrome's `--proxy-bypass-list` matching: `,`/`;` separated rules that are either an IP +// range in CIDR notation or a host glob with an optional scheme and port (`.host` meaning +// `*.host`), plus `` for dotless hosts. Loopback and link-local hosts are bypassed +// implicitly unless the list contains `<-loopback>`. +function bypassesBrowserProxy(list = '', { scheme, hostname, port }) { let rules = list.split(/[,;]/).map(rule => rule.trim()).filter(Boolean); - if (!rules.includes('<-loopback>') && - /^(localhost|127(\.\d+){3}|\[::1\])$|\.localhost$/.test(hostname)) return true; + if (!rules.includes('<-loopback>') && ( + /^(localhost|127(\.\d+){3}|169\.254(\.\d+){2}|\[::1\])$|\.localhost$/.test(hostname) || + /^\[fe[89ab][0-9a-f]:/.test(hostname) + )) return true; return rules.some(rule => { if (rule === '') return !hostname.includes('.'); if (rule === '<-loopback>') return false; - let [, host, rulePort] = rule.replace(/^\w+:\/\//, '').match(/^(.+?)(?::(\d+))?$/); - if (rulePort && rulePort !== port) return false; + let host = rule.toLowerCase(); + let sep = host.indexOf('://'); + if (sep !== -1) { + if (host.slice(0, sep) !== scheme) return false; + host = host.slice(sep + 3); + } + + if (host.includes('/')) return cidrMatches(host, hostname); + + let colon = host.lastIndexOf(':'); + if (colon > host.lastIndexOf(']') && /^\d+$/.test(host.slice(colon + 1))) { + if (host.slice(colon + 1) !== port) return false; + host = host.slice(0, colon); + } + if (host.startsWith('.')) host = `*${host}`; - return globMatches(host.toLowerCase(), hostname); + return globMatches(host, hostname); }); } +// Returns true when the IP-literal `hostname` falls within the `cidr` range (IPv4 or IPv6) +function cidrMatches(cidr, hostname) { + let [ip, prefix] = cidr.split('/'); + let address = hostname.replace(/^\[/, '').replace(/\]$/, ''); + let family = net.isIP(ip); + let bits = Number(prefix); + + if (!family || net.isIP(address) !== family) return false; + if (!Number.isInteger(bits) || bits < 0 || bits > (family === 4 ? 32 : 128)) return false; + + let range = new net.BlockList(); + range.addSubnet(ip, bits, `ipv${family}`); + return range.check(address, `ipv${family}`); +} + // Returns true when `subject` matches `glob`, where `*` matches any run of characters. A linear // greedy matcher with single-star backtracking, so user-supplied patterns need no RegExp. function globMatches(glob, subject) { diff --git a/packages/core/test/unit/utils.test.js b/packages/core/test/unit/utils.test.js index 8613b32ff..d820fbc6d 100644 --- a/packages/core/test/unit/utils.test.js +++ b/packages/core/test/unit/utils.test.js @@ -44,13 +44,22 @@ describe('Unit / Utils', () => { expect(browserProxyFor(args('ftp=ftp:21;127.0.0.1:8118'), 'http://a.com')).toBe(proxy); }); + it('falls back to the socks= mapping for schemes without their own', () => { + expect(browserProxyFor(args('http=first:1;socks=http://127.0.0.1:8118'), 'https://a.com')).toBe(proxy); + // a scheme-less proxy in the socks= mapping is SOCKS4, which is unsupported + expect(browserProxyFor(args('http=first:1;SOCKS=127.0.0.1:1080'), 'https://a.com')).toBeUndefined(); + }); + it('returns undefined for socks and direct proxies', () => { expect(browserProxyFor(args('socks5://127.0.0.1:1080'), 'https://a.com')).toBeUndefined(); expect(browserProxyFor(args('direct://'), 'https://a.com')).toBeUndefined(); }); - it('bypasses loopback hosts unless <-loopback> is listed', () => { - for (let url of ['http://localhost:8000', 'http://127.0.0.1', 'http://[::1]', 'http://app.localhost']) { + it('bypasses loopback and link-local hosts unless <-loopback> is listed', () => { + for (let url of [ + 'http://localhost:8000', 'http://127.0.0.1', 'http://[::1]', 'http://app.localhost', + 'http://169.254.1.10', 'http://[fe80::1]', 'http://[febf::1]' + ]) { expect(browserProxyFor(args(proxy), url)).toBeUndefined(); expect(browserProxyFor(args(proxy, '<-loopback>'), url)).toBe(proxy); } @@ -81,6 +90,23 @@ describe('Unit / Utils', () => { expect(browserProxyFor(args(proxy, ''), 'http://intranet')).toBeUndefined(); expect(browserProxyFor(args(proxy, ''), 'http://a.com')).toBe(proxy); expect(browserProxyFor(args(proxy, ' ; a.com'), 'https://b.com')).toBe(proxy); + expect(browserProxyFor(args(proxy, '[2001:db8::1]:8080'), 'http://[2001:db8::1]:8080')).toBeUndefined(); + }); + + it('only bypasses scheme-restricted rules for that scheme', () => { + expect(browserProxyFor(args(proxy, 'https://a.com'), 'http://a.com')).toBe(proxy); + expect(browserProxyFor(args(proxy, 'HTTP://a.com'), 'http://a.com')).toBeUndefined(); + }); + + it('matches CIDR rules against IP-literal hosts', () => { + expect(browserProxyFor(args(proxy, '192.168.1.0/24'), 'http://192.168.1.20')).toBeUndefined(); + expect(browserProxyFor(args(proxy, '192.168.1.0/24'), 'http://192.168.2.1')).toBe(proxy); + expect(browserProxyFor(args(proxy, '192.168.1.0/24'), 'http://a.com')).toBe(proxy); + expect(browserProxyFor(args(proxy, '2001:db8::/32'), 'http://[2001:db8::1]')).toBeUndefined(); + expect(browserProxyFor(args(proxy, '192.168.0.0/16'), 'http://[2001:db8::1]')).toBe(proxy); + expect(browserProxyFor(args(proxy, 'not-an-ip/8'), 'http://10.0.0.1')).toBe(proxy); + expect(browserProxyFor(args(proxy, '10.0.0.0/33'), 'http://10.0.0.1')).toBe(proxy); + expect(browserProxyFor(args(proxy, '10.0.0.0/x'), 'http://10.0.0.1')).toBe(proxy); }); }); From 8f5bd6e0d5482285d1feb302648863f17efbea3d Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 6 Oct 2026 19:39:39 +0530 Subject: [PATCH 04/11] fix: address CodeQL flow and review follow-ups on browser proxy - getProxy: keep the explicit override out of the env-value quote stripping entirely (separate variable), so CodeQL's js/polynomial-redos flow no longer runs through code changed here. Behaviour is unchanged. - cidrMatches: guard net.BlockList (Node >= 14.18); CIDR rules are left unmatched on older 14.x instead of throwing. - globMatches: support `?` like Chrome's MatchPattern. - tests: an established tunnel failing mid-response rejects the request without an unhandled error; move no_proxy cleanup into afterEach. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/client/src/proxy.js | 7 ++++--- packages/client/test/unit/proxy.test.js | 5 ++++- packages/client/test/unit/request.test.js | 13 +++++++++++++ packages/core/src/utils.js | 10 +++++++--- packages/core/test/unit/utils.test.js | 2 ++ 5 files changed, 30 insertions(+), 7 deletions(-) diff --git a/packages/client/src/proxy.js b/packages/client/src/proxy.js index 0d3a1b7ee..c4d092468 100644 --- a/packages/client/src/proxy.js +++ b/packages/client/src/proxy.js @@ -89,16 +89,17 @@ export function href(options) { // Returns the proxy URL for a set of request options. An explicit `override` proxy URL is // used as-is, without consulting the proxy env vars or NO_PROXY; the caller owns bypassing. export function getProxy(options, override) { - let proxyUrl = override || (options.protocol === 'https:' && + let envProxyUrl = (options.protocol === 'https:' && (process.env.https_proxy || process.env.HTTPS_PROXY)) || (process.env.http_proxy || process.env.HTTP_PROXY); - let shouldProxy = !!override || (!!proxyUrl && !hostnameMatches( + let shouldProxy = !!override || (!!envProxyUrl && !hostnameMatches( stripQuotesAndSpaces(process.env.no_proxy || process.env.NO_PROXY) , href(options))); // only env values may carry stray quotes/spaces; an explicit override is used verbatim - if (!override && proxyUrl && typeof proxyUrl === 'string') { proxyUrl = stripQuotesAndSpaces(proxyUrl); } + if (envProxyUrl && typeof envProxyUrl === 'string') { envProxyUrl = stripQuotesAndSpaces(envProxyUrl); } + let proxyUrl = override || envProxyUrl; if (shouldProxy) { proxyUrl = new URL(proxyUrl); diff --git a/packages/client/test/unit/proxy.test.js b/packages/client/test/unit/proxy.test.js index aa83f6524..3ca3e92f8 100644 --- a/packages/client/test/unit/proxy.test.js +++ b/packages/client/test/unit/proxy.test.js @@ -8,6 +8,10 @@ describe('proxy', () => { process.env.http_proxy = 'http://proxy.com:8080'; }); + afterEach(() => { + delete process.env.no_proxy; + }); + it('should return proxy object if proxy is set', () => { const options = { protocol: 'http:', hostname: 'example.com' }; const proxy = getProxy(options); @@ -21,7 +25,6 @@ describe('proxy', () => { expect(getProxy(options, 'http://other.com:3128')).toEqual(jasmine.objectContaining({ host: 'other.com', port: '3128' })); - delete process.env.no_proxy; }); it('should return undefined if no proxy is set', () => { diff --git a/packages/client/test/unit/request.test.js b/packages/client/test/unit/request.test.js index 5fb4d1d5a..0431f5e04 100644 --- a/packages/client/test/unit/request.test.js +++ b/packages/client/test/unit/request.test.js @@ -440,6 +440,19 @@ describe('Unit / Request', () => { 'Connection closed while sending request to upstream proxy')); }); + it('rejects without crashing when an established proxy tunnel fails mid-response', async () => { + server.reply('/hang', (req, res) => { + res.writeHead(200, { 'Content-Length': '1000' }); + res.write('partial'); // never finishes + }); + + let pending = server.request('/hang', { retries: 0 }); + await new Promise(r => setTimeout(r, 100)); + await proxy.close(); + + await expectAsync(pending).toBeRejected(); + }); + it('does not proxy requests matching NO_PROXY', async () => { process.env.NO_PROXY = 'localhost'; diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index b081ebada..3729bdc04 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -230,6 +230,9 @@ function cidrMatches(cidr, hostname) { let family = net.isIP(ip); let bits = Number(prefix); + // net.BlockList needs Node >= 14.18; without it CIDR rules are left unmatched + /* istanbul ignore next: CI runs a Node version that has net.BlockList */ + if (typeof net.BlockList !== 'function') return false; if (!family || net.isIP(address) !== family) return false; if (!Number.isInteger(bits) || bits < 0 || bits > (family === 4 ? 32 : 128)) return false; @@ -238,14 +241,15 @@ function cidrMatches(cidr, hostname) { return range.check(address, `ipv${family}`); } -// Returns true when `subject` matches `glob`, where `*` matches any run of characters. A linear -// greedy matcher with single-star backtracking, so user-supplied patterns need no RegExp. +// Returns true when `subject` matches `glob`, where `*` matches any run of characters and `?` any +// single one (as in Chrome's MatchPattern). A linear greedy matcher with single-star +// backtracking, so user-supplied patterns need no RegExp. function globMatches(glob, subject) { let [g, s, star, mark] = [0, 0, -1, 0]; while (s < subject.length) { if (glob[g] === '*') [star, mark] = [g++, s]; - else if (glob[g] === subject[s]) [g, s] = [g + 1, s + 1]; + else if (glob[g] === '?' || glob[g] === subject[s]) [g, s] = [g + 1, s + 1]; else if (star !== -1) [g, s] = [star + 1, ++mark]; else return false; } diff --git a/packages/core/test/unit/utils.test.js b/packages/core/test/unit/utils.test.js index d820fbc6d..625e4ddd1 100644 --- a/packages/core/test/unit/utils.test.js +++ b/packages/core/test/unit/utils.test.js @@ -83,6 +83,8 @@ describe('Unit / Utils', () => { expect(browserProxyFor(args(proxy, 'CDN.*.com'), 'https://cdn.a.b.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, '*.cdn.com'), 'https://cdn.com')).toBe(proxy); expect(browserProxyFor(args(proxy, 'cdn.*.net'), 'https://cdn.a.com')).toBe(proxy); + expect(browserProxyFor(args(proxy, 'cdn?.com'), 'https://cdn1.com')).toBeUndefined(); + expect(browserProxyFor(args(proxy, 'cdn?.com'), 'https://cdn12.com')).toBe(proxy); expect(browserProxyFor(args(proxy, 'https://a.com'), 'https://a.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, 'a.com:443'), 'https://a.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, 'a.com:8443'), 'https://a.com')).toBe(proxy); From 698e3dc6621f40f0638c2dc2a1a16faef154466d Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 6 Oct 2026 19:56:30 +0530 Subject: [PATCH 05/11] fix(core): reject malformed CIDR prefixes in proxy bypass rules Number('') is 0, so a rule like `10.0.0.0/` became /0 and bypassed the proxy for every IPv4 literal; '1e1', '0x8', ' 8' and extra '/' segments were also accepted. Only a plain decimal prefix (and a single '/') is now valid. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/core/src/utils.js | 8 +++++--- packages/core/test/unit/utils.test.js | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index 3729bdc04..e9224ca96 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -225,16 +225,18 @@ function bypassesBrowserProxy(list = '', { scheme, hostname, port }) { // Returns true when the IP-literal `hostname` falls within the `cidr` range (IPv4 or IPv6) function cidrMatches(cidr, hostname) { - let [ip, prefix] = cidr.split('/'); + let [ip, prefix, ...extra] = cidr.split('/'); let address = hostname.replace(/^\[/, '').replace(/\]$/, ''); let family = net.isIP(ip); - let bits = Number(prefix); + // only a plain decimal prefix is valid; Number() would read '' as 0 (matching everything) + // and also accept forms like '1e1' or '0x8' + let bits = !extra.length && /^\d{1,3}$/.test(prefix) ? Number(prefix) : NaN; // net.BlockList needs Node >= 14.18; without it CIDR rules are left unmatched /* istanbul ignore next: CI runs a Node version that has net.BlockList */ if (typeof net.BlockList !== 'function') return false; if (!family || net.isIP(address) !== family) return false; - if (!Number.isInteger(bits) || bits < 0 || bits > (family === 4 ? 32 : 128)) return false; + if (!(bits <= (family === 4 ? 32 : 128))) return false; let range = new net.BlockList(); range.addSubnet(ip, bits, `ipv${family}`); diff --git a/packages/core/test/unit/utils.test.js b/packages/core/test/unit/utils.test.js index 625e4ddd1..7ea5dd9dc 100644 --- a/packages/core/test/unit/utils.test.js +++ b/packages/core/test/unit/utils.test.js @@ -109,6 +109,10 @@ describe('Unit / Utils', () => { expect(browserProxyFor(args(proxy, 'not-an-ip/8'), 'http://10.0.0.1')).toBe(proxy); expect(browserProxyFor(args(proxy, '10.0.0.0/33'), 'http://10.0.0.1')).toBe(proxy); expect(browserProxyFor(args(proxy, '10.0.0.0/x'), 'http://10.0.0.1')).toBe(proxy); + // malformed prefixes must not degrade to /0 (which would match every address) + for (let rule of ['10.0.0.0/', '10.0.0.0/1e1', '10.0.0.0/0x8', '10.0.0.0/ 8', '10.0.0.0/8/9']) { + expect(browserProxyFor(args(proxy, rule), 'http://10.0.0.1')).withContext(rule).toBe(proxy); + } }); }); From cc848a48e34cf6d8aa2e47cc118298237074a85d Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 6 Oct 2026 20:38:50 +0530 Subject: [PATCH 06/11] test(core): cover trailing-star glob match in proxy bypass rules Restores 100% statement coverage of packages/core/src/utils.js: the leftover-'*' loop in globMatches only runs when the host is exhausted before trailing stars, e.g. 'a.com**' against 'a.com'. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/core/test/unit/utils.test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/core/test/unit/utils.test.js b/packages/core/test/unit/utils.test.js index 7ea5dd9dc..60f8b528c 100644 --- a/packages/core/test/unit/utils.test.js +++ b/packages/core/test/unit/utils.test.js @@ -80,6 +80,8 @@ describe('Unit / Utils', () => { expect(browserProxyFor(args(proxy, '*cdn.com'), 'https://mycdn.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, '*'), 'https://any.where')).toBeUndefined(); expect(browserProxyFor(args(proxy, 'cdn.*'), 'https://cdn.example.com')).toBeUndefined(); + // trailing stars left over once the host is exhausted still match + expect(browserProxyFor(args(proxy, 'a.com**'), 'https://a.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, 'CDN.*.com'), 'https://cdn.a.b.com')).toBeUndefined(); expect(browserProxyFor(args(proxy, '*.cdn.com'), 'https://cdn.com')).toBe(proxy); expect(browserProxyFor(args(proxy, 'cdn.*.net'), 'https://cdn.a.com')).toBe(proxy); From 06019a6ed6854d984f68cc2785913193aa9a4003 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 6 Oct 2026 21:26:25 +0530 Subject: [PATCH 07/11] fix(core): fall back to the default route when the browser proxy fails Routing the direct font fetch through Chrome's `--proxy-server` regressed setups that worked before: an authenticated proxy (Chrome answers the 407 with discovery.authorization, the Node fetch cannot), with or without HTTPS_PROXY=http://user:pass@... for the CLI, and proxies that refuse a non-browser client. Verified with real builds: fonts captured on master, dropped on this branch. When the fetch through the browser proxy fails at the proxy (407, or no response from the target at all), retry via the pre-change route (env proxy or direct), so anything that fetched before still does. Error responses from the target are not retried. Also fix ProxyHttpAgent logging "Proxying request: undefined". Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/client/src/proxy.js | 2 +- packages/core/src/network.js | 25 ++++++++++-- packages/core/test/discovery.test.js | 60 ++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/packages/client/src/proxy.js b/packages/client/src/proxy.js index c4d092468..6a78e5bae 100644 --- a/packages/client/src/proxy.js +++ b/packages/client/src/proxy.js @@ -140,7 +140,7 @@ export class ProxyHttpAgent extends http.Agent { addRequest(request, options) { let proxy = getProxy(options, this.proxy); if (!proxy) return super.addRequest(request, options); - logger('client:proxy').debug(`Proxying request: ${options.href}`); + logger('client:proxy').debug(`Proxying request: ${href(options)}`); // modify the request for proxying request.path = href(options); diff --git a/packages/core/src/network.js b/packages/core/src/network.js index dff2e9705..7f353c1ee 100644 --- a/packages/core/src/network.js +++ b/packages/core/src/network.js @@ -644,14 +644,31 @@ export class Network { // The proxy then resolves the target, so as on the browser path (where remoteIPAddress is the // proxy's) the connected-IP metadata gate only sees the proxy address. let proxy = browserProxyFor(this.page.session?.browser?.args, request.url); - if (proxy) this.log.debug('- Requesting directly through the browser proxy', this.meta); - - let { body, status, headers: responseHeaders } = await makeRequest( + let fetch = proxy => makeRequest( request.url, { buffer: true, headers, lookup, proxy }, (body, res) => ({ body, status: res.statusCode, headers: res.headers })); - return { body, status, headers: responseHeaders, remoteAddresses }; + let response; + if (proxy) { + this.log.debug('- Requesting directly through the browser proxy', this.meta); + + try { + response = await fetch(proxy); + } catch (error) { + // The browser can answer a proxy's auth challenge (with discovery.authorization) but this + // fetch cannot, and the proxy may refuse a non-browser client. When the proxy itself fails + // (407, or no response from the target at all), fall back to the pre-browser-proxy route + // (env proxy or direct) so anything that fetched before still does. + if (error.response && error.response.statusCode !== 407) throw error; + this.log.debug(`- Browser proxy failed (${error.message.split('\n')[0]}), retrying via the default route`, this.meta); + response = await fetch(); + } + } else { + response = await fetch(); + } + + return { ...response, remoteAddresses }; } } diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js index 297a371da..2adfc3b67 100644 --- a/packages/core/test/discovery.test.js +++ b/packages/core/test/discovery.test.js @@ -2706,6 +2706,66 @@ describe('Discovery', () => { }) ])); }); + + describe('when the proxy fails the direct fetch', () => { + // localhost is proxied too (<-loopback>), so both the browser and the direct font fetch + // reach the test server as a proxy; only the direct fetch sends `sec-fetch-user: ?1` + const loopbackFontDOM = proxiedFontDOM.replace('http://tunnel.test/font.woff', 'http://localhost:8000/lb-font.woff'); + let directFetches; + + beforeEach(async () => { + await percy.stop(true); + directFetches = 0; + + percy = await Percy.start({ + token: 'PERCY_TOKEN', + snapshot: { widths: [1000] }, + discovery: { + concurrency: 1, + launchOptions: { args: ['--proxy-server=http://localhost:8000', '--proxy-bypass-list=<-loopback>'] } + } + }); + + percy.loglevel('debug'); + }); + + it('falls back to the default route when the proxy requires auth', async () => { + // like an authenticated proxy: the browser got through, the direct fetch gets a 407 + server.reply('/lb-font.woff', req => req.headers['sec-fetch-user'] === '?1' && directFetches++ === 0 + ? [407, { 'Proxy-Authenticate': 'Basic' }, 'proxy auth required'] + : [200, 'font/woff', '']); + + await percy.snapshot({ name: 'proxy 407 snapshot', url: 'http://localhost:8000', domSnapshot: loopbackFontDOM }); + await percy.idle(); + + expect(directFetches).toEqual(2); + expect(logger.stderr).toContain(jasmine.stringMatching( + /- Browser proxy failed \(407 Proxy Authentication Required\), retrying via the default route/)); + expect(captured[0]).toEqual(jasmine.arrayContaining([ + jasmine.objectContaining({ + id: sha256hash(''), + attributes: jasmine.objectContaining({ 'resource-url': 'http://localhost:8000/lb-font.woff' }) + }) + ])); + }); + + it('does not fall back on an error response from the target', async () => { + server.reply('/lb-font.woff', req => req.headers['sec-fetch-user'] === '?1' && ++directFetches + ? [404, 'text/plain', 'not found'] + : [200, 'font/woff', '']); + + await percy.snapshot({ name: 'proxy 404 snapshot', url: 'http://localhost:8000', domSnapshot: loopbackFontDOM }); + await percy.idle(); + + expect(directFetches).toEqual(1); + expect(logger.stderr).not.toContain(jasmine.stringContaining('retrying via the default route')); + expect(captured[0]).not.toEqual(jasmine.arrayContaining([ + jasmine.objectContaining({ + attributes: jasmine.objectContaining({ 'resource-url': 'http://localhost:8000/lb-font.woff' }) + }) + ])); + }); + }); }); describe('resource caching', () => { From cb12c4c9ec72cdcc7f8ec49e2bf3590d71bc252e Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 6 Oct 2026 22:56:10 +0530 Subject: [PATCH 08/11] fix(core): bound the browser-proxy attempt so the fallback stays reachable A proxy that accepts the connection and then stalls would hang the font re-fetch (which has no overall timeout), so the default-route fallback was never reached. The proxied attempt now uses DIRECT_FETCH_TIMEOUT as an idle timeout (30s under PERCY_GZIP) via a shared directFetchTimeout() helper; a timeout has no response, so it falls back like a 407. Also make the "no fallback on a 404" test's handler explicit. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/core/src/network.js | 15 +++++++----- packages/core/test/discovery.test.js | 34 +++++++++++++++++++++++++--- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/packages/core/src/network.js b/packages/core/src/network.js index 7f353c1ee..93be1d36c 100644 --- a/packages/core/src/network.js +++ b/packages/core/src/network.js @@ -31,6 +31,8 @@ const ABORTED_MESSAGE = 'Request was aborted by browser'; const RESPONSE_RECEIVED_TIMEOUT = 2000; // Cap idle() impact when a host accepts the TCP connection then stalls during a direct fetch. const DIRECT_FETCH_TIMEOUT = 5000; +// Under PERCY_GZIP the size ceiling can reach tens of MB; 5s is too short. +const directFetchTimeout = () => process.env.PERCY_GZIP ? DIRECT_FETCH_TIMEOUT_WITH_GZIP : DIRECT_FETCH_TIMEOUT; // Stable, machine-readable codes for abort errors thrown from this module. // Consumers should prefer `error.code` over string matching on `error.message`. @@ -644,8 +646,8 @@ export class Network { // The proxy then resolves the target, so as on the browser path (where remoteIPAddress is the // proxy's) the connected-IP metadata gate only sees the proxy address. let proxy = browserProxyFor(this.page.session?.browser?.args, request.url); - let fetch = proxy => makeRequest( - request.url, { buffer: true, headers, lookup, proxy }, (body, res) => ({ + let fetch = (proxy, timeout) => makeRequest( + request.url, { buffer: true, headers, lookup, proxy, timeout }, (body, res) => ({ body, status: res.statusCode, headers: res.headers })); @@ -654,11 +656,13 @@ export class Network { this.log.debug('- Requesting directly through the browser proxy', this.meta); try { - response = await fetch(proxy); + // an idle timeout, so a proxy that accepts the connection and then stalls still leaves + // the fallback reachable (the font re-fetch otherwise has no timeout at all) + response = await fetch(proxy, directFetchTimeout()); } catch (error) { // The browser can answer a proxy's auth challenge (with discovery.authorization) but this // fetch cannot, and the proxy may refuse a non-browser client. When the proxy itself fails - // (407, or no response from the target at all), fall back to the pre-browser-proxy route + // (407, or no response from the target at all, including a stall), fall back to the pre-browser-proxy route // (env proxy or direct) so anything that fetched before still does. if (error.response && error.response.statusCode !== 407) throw error; this.log.debug(`- Browser proxy failed (${error.message.split('\n')[0]}), retrying via the default route`, this.meta); @@ -976,8 +980,7 @@ async function captureResourceDirectly(network, request, session) { let url = originURL(request); let meta = { ...network.meta, url }; - // Under PERCY_GZIP the size ceiling can reach tens of MB; 5s is too short. - let timeoutMs = process.env.PERCY_GZIP ? DIRECT_FETCH_TIMEOUT_WITH_GZIP : DIRECT_FETCH_TIMEOUT; + let timeoutMs = directFetchTimeout(); try { log.debug('- Requesting resource directly (responseReceived timeout fallback)', meta); diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js index 2adfc3b67..3ac5e88b2 100644 --- a/packages/core/test/discovery.test.js +++ b/packages/core/test/discovery.test.js @@ -2749,10 +2749,38 @@ describe('Discovery', () => { ])); }); + it('falls back to the default route when the proxy stalls', async () => { + let release; + server.reply('/lb-font.woff', req => { + if (req.headers['sec-fetch-user'] === '?1' && directFetches++ === 0) { + // accept the request through the proxy, then never answer it + return new Promise(resolve => (release = resolve)); + } + return [200, 'font/woff', '']; + }); + + await percy.snapshot({ name: 'proxy stall snapshot', url: 'http://localhost:8000', domSnapshot: loopbackFontDOM }); + await percy.idle(); + release([200, 'font/woff', '']); + + expect(directFetches).toEqual(2); + expect(logger.stderr).toContain(jasmine.stringMatching( + /- Browser proxy failed \(Request to http:\/\/localhost:8000\/lb-font\.woff timed out after 5000ms\), retrying via the default route/)); + expect(captured[0]).toEqual(jasmine.arrayContaining([ + jasmine.objectContaining({ + id: sha256hash(''), + attributes: jasmine.objectContaining({ 'resource-url': 'http://localhost:8000/lb-font.woff' }) + }) + ])); + }); + it('does not fall back on an error response from the target', async () => { - server.reply('/lb-font.woff', req => req.headers['sec-fetch-user'] === '?1' && ++directFetches - ? [404, 'text/plain', 'not found'] - : [200, 'font/woff', '']); + server.reply('/lb-font.woff', req => { + // the browser gets the font; the direct fetch gets a 404 from the target + if (req.headers['sec-fetch-user'] !== '?1') return [200, 'font/woff', '']; + directFetches++; + return [404, 'text/plain', 'not found']; + }); await percy.snapshot({ name: 'proxy 404 snapshot', url: 'http://localhost:8000', domSnapshot: loopbackFontDOM }); await percy.idle(); From 57a9c3acd8b4673d263bdec5b2f90a457a58724a Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 6 Oct 2026 23:36:03 +0530 Subject: [PATCH 09/11] fix(core): leave time for the default-route retry on worker-path fetches captureResourceDirectly races the whole direct fetch against one DIRECT_FETCH_TIMEOUT. Through a browser proxy, a stalled proxied attempt uses that same timeout before falling back, so the outer race fired first and the fallback never ran. When a browser proxy applies, budget for both attempts (2x); without one the budget is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/core/src/network.js | 3 +++ packages/core/test/discovery.test.js | 33 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/packages/core/src/network.js b/packages/core/src/network.js index 93be1d36c..415114853 100644 --- a/packages/core/src/network.js +++ b/packages/core/src/network.js @@ -981,6 +981,9 @@ async function captureResourceDirectly(network, request, session) { let meta = { ...network.meta, url }; let timeoutMs = directFetchTimeout(); + // through a browser proxy, directFetch may spend one timeout on the proxied attempt before + // falling back to the default route; budget for both so the fallback is not cut off + if (browserProxyFor(network.page.session?.browser?.args, request.url)) timeoutMs *= 2; try { log.debug('- Requesting resource directly (responseReceived timeout fallback)', meta); diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js index 3ac5e88b2..55a541288 100644 --- a/packages/core/test/discovery.test.js +++ b/packages/core/test/discovery.test.js @@ -2774,6 +2774,39 @@ describe('Discovery', () => { ])); }); + it('leaves time for the fallback when a worker-path direct fetch stalls at the proxy', async () => { + // drop the CDP response so the resource goes through captureResourceDirectly, whose + // overall timeout must cover the stalled proxied attempt plus the default-route retry + spyOn(percy.browser, '_handleMessage').and.callFake(function(data) { + let parsed; try { parsed = JSON.parse(data); } catch { /* binary frame */ } + if (parsed?.method === 'Network.responseReceived' && + parsed.params?.response?.url?.endsWith('/lb-style.css')) return; + this._handleMessage.and.originalFn.call(this, data); + }); + + let release; + server.reply('/lb-style.css', req => { + if (req.headers['sec-fetch-user'] === '?1' && directFetches++ === 0) { + return new Promise(resolve => (release = resolve)); + } + return [200, 'text/css', 'p { color: purple; }']; + }); + + let dom = 'x'; + await percy.snapshot({ name: 'proxy stall worker snapshot', url: 'http://localhost:8000', domSnapshot: dom }); + await percy.idle(); + release([200, 'text/css', 'p { color: purple; }']); + + expect(directFetches).toEqual(2); + expect(logger.stderr).toContain(jasmine.stringMatching(/- Browser proxy failed \(.*timed out after 5000ms\), retrying via the default route/)); + expect(logger.stderr).not.toContain(jasmine.stringContaining('Direct fetch timed out')); + expect(captured[0]).toEqual(jasmine.arrayContaining([ + jasmine.objectContaining({ + attributes: jasmine.objectContaining({ 'resource-url': 'http://localhost:8000/lb-style.css' }) + }) + ])); + }); + it('does not fall back on an error response from the target', async () => { server.reply('/lb-font.woff', req => { // the browser gets the font; the direct fetch gets a 404 from the target From 9bb7a9f0b8b9038841112fb59a6709959836fc56 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Wed, 7 Oct 2026 10:38:15 +0530 Subject: [PATCH 10/11] fix(client): time out the CONNECT handshake to a silent https proxy Node arms a request's `timeout` only once the request has a socket, and ProxyHttpsAgent hands the socket over after the proxy answers CONNECT. A proxy that accepts the connection and never answers therefore hung the request forever. For the browser-proxy font fetch that meant the fallback never ran and the snapshot failed waiting for the network to idle. Apply the request's timeout to the handshake and clear it once the tunnel is established. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/client/src/proxy.js | 11 ++++++++++- packages/client/test/unit/request.test.js | 12 ++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/client/src/proxy.js b/packages/client/src/proxy.js index 6a78e5bae..96467087c 100644 --- a/packages/client/src/proxy.js +++ b/packages/client/src/proxy.js @@ -221,6 +221,12 @@ export class ProxyHttpsAgent extends https.Agent { new Error('Connection closed while sending request to upstream proxy') ); + // a request's `timeout` only starts once it has a socket, which is handed over after the + // CONNECT reply; bound the handshake itself so a silent proxy cannot hang the request + let handleTimeout = () => handleError(Object.assign(new Error( + `Request to ${href(options)} timed out after ${options.timeout}ms waiting for the proxy` + ), { code: 'ETIMEDOUT' })); + let buffer = ''; let handleData = data => { buffer += data.toString(); @@ -237,13 +243,16 @@ export class ProxyHttpsAgent extends https.Agent { } // the tunnel is established; its later close (e.g. keep-alive teardown) is not a failure - socket.off('error', handleError).off('close', handleClose); + socket.off('error', handleError).off('close', handleClose).off('timeout', handleTimeout); + socket.setTimeout(0); options.socket = socket; options.servername = options.hostname; // callback not passed in so not to be added as a listener callback(null, super.createConnection(options)); }; + if (options.timeout) socket.setTimeout(options.timeout, handleTimeout); + // send and handle the connect message socket .on('error', handleError) diff --git a/packages/client/test/unit/request.test.js b/packages/client/test/unit/request.test.js index 0431f5e04..8a69d5388 100644 --- a/packages/client/test/unit/request.test.js +++ b/packages/client/test/unit/request.test.js @@ -83,6 +83,9 @@ function createProxyServer({ type, port, ...options }) { return res.writeHead(403).end(); } + // accept the request, then never answer it + if (options.stall) return; + (proto === 'http' ? http : https).request(url.href, { method, headers, rejectUnauthorized: false }).on('response', remote => { @@ -111,6 +114,8 @@ function createProxyServer({ type, port, ...options }) { return client.end(); } + if (options.stall) return; + let socket = net.connect({ rejectUnauthorized: false, host: 'localhost', @@ -453,6 +458,13 @@ describe('Unit / Request', () => { await expectAsync(pending).toBeRejected(); }); + it('times out a request when the proxy never answers', async () => { + proxy.options.stall = true; + + await expectAsync(server.request('/test', { timeout: 100, retries: 0 })) + .toBeRejectedWithError(/timed out after 100ms/); + }); + it('does not proxy requests matching NO_PROXY', async () => { process.env.NO_PROXY = 'localhost'; From ec1812cae86d21bb4e820d8c17f51fae339c0035 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Wed, 7 Oct 2026 11:01:14 +0530 Subject: [PATCH 11/11] fix(client): handle a CONNECT tunnel failure only once Destroying the socket re-emits 'error' and 'close', so a failed handshake called the createConnection callback and logged the failure up to three times. Ignore everything after the first failure. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/client/src/proxy.js | 4 ++++ packages/client/test/unit/request.test.js | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/packages/client/src/proxy.js b/packages/client/src/proxy.js index 96467087c..b4db80e64 100644 --- a/packages/client/src/proxy.js +++ b/packages/client/src/proxy.js @@ -203,7 +203,11 @@ export class ProxyHttpsAgent extends https.Agent { // start the proxy connection and setup listeners let socket = proxy.connect(); + // destroying the socket re-emits 'error' and 'close'; only the first failure counts + let failed = false; let handleError = err => { + if (failed) return; + failed = true; socket.destroy(err); logger('client:proxy').error(`Proxying request ${href(options)} failed: ${err}`); diff --git a/packages/client/test/unit/request.test.js b/packages/client/test/unit/request.test.js index 8a69d5388..97adfddbe 100644 --- a/packages/client/test/unit/request.test.js +++ b/packages/client/test/unit/request.test.js @@ -463,6 +463,12 @@ describe('Unit / Request', () => { await expectAsync(server.request('/test', { timeout: 100, retries: 0 })) .toBeRejectedWithError(/timed out after 100ms/); + // let the destroyed socket emit its trailing 'error' and 'close' + await new Promise(r => setTimeout(r, 50)); + + // https requests go through a CONNECT tunnel, whose failure is handled only once + expect(logger.stderr.filter(line => /Proxying request .* failed/.test(line))) + .toHaveSize(serverType === 'https' ? 1 : 0); }); it('does not proxy requests matching NO_PROXY', async () => {