fix(core): route direct font fetches through the browser's --proxy-server

packages/client/src/proxy.js

@@ -86,17 +86,20 @@ 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 envProxyUrl = (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 || (!!envProxyUrl && !hostnameMatches(
     stripQuotesAndSpaces(process.env.no_proxy || process.env.NO_PROXY)
-    , href(options));
+    , 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 (envProxyUrl && typeof envProxyUrl === 'string') { envProxyUrl = stripQuotesAndSpaces(envProxyUrl); }
+  let proxyUrl = override || envProxyUrl;
 
   if (shouldProxy) {
     proxyUrl = new URL(proxyUrl);
@@ -128,10 +131,16 @@ 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}`);
+    logger('client:proxy').debug(`Proxying request: ${href(options)}`);
 
     // modify the request for proxying
     request.path = href(options);
@@ -168,13 +177,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)}`);
 
@@ -192,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}`);
 
@@ -210,6 +225,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();
@@ -225,12 +246,17 @@ 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).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)
@@ -244,6 +270,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 +281,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 {

packages/client/src/utils.js

@@ -124,15 +124,16 @@ 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];
 
   // 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,

packages/client/test/unit/proxy.test.js

@@ -8,12 +8,25 @@ 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);
       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'
+      }));
+    });
+
     it('should return undefined if no proxy is set', () => {
       delete process.env.http_proxy;
       const options = { protocol: 'http:', hostname: 'example.com' };
@@ -82,6 +95,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 = {};

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',
@@ -420,6 +425,52 @@ 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 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('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('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/);
+            // 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 () => {
             process.env.NO_PROXY = 'localhost';
 

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
@@ -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`.
@@ -639,12 +641,38 @@ export class Network {
       cb(err, address, family);
     });
 
-    let { body, status, headers: responseHeaders } = await makeRequest(
-      request.url, { buffer: true, headers, lookup }, (body, res) => ({
+    // 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);
+    let fetch = (proxy, timeout) => makeRequest(
+      request.url, { buffer: true, headers, lookup, proxy, timeout }, (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 {
+        // 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, 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);
+        response = await fetch();
+      }
+    } else {
+      response = await fetch();
+    }
+
+    return { ...response, remoteAddresses };
   }
 }
 
@@ -952,8 +980,10 @@ 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();
+  // 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);