Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions packages/core/src/network.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
import { request as makeRequest } from '@percy/client/utils';
import logger from '@percy/logger';
import mime from 'mime-types';
import { DefaultMap, createResource, hostnameMatches, normalizeURL, waitFor, decodeAndEncodeURLWithLogging, handleIncorrectFontMimeType, executeDomainValidation } from './utils.js';
import { AbortError, DefaultMap, createResource, hostnameMatches, normalizeURL, waitFor, decodeAndEncodeURLWithLogging, handleIncorrectFontMimeType, executeDomainValidation } from './utils.js';

const MAX_RESOURCE_SIZE = 25 * (1024 ** 2) * 0.63; // 25MB, 0.63 factor for accounting for base64 encoding
const ALLOWED_STATUSES = [200, 201, 301, 302, 304, 307, 308];
const ALLOWED_RESOURCES = ['Document', 'Stylesheet', 'Image', 'Media', 'Font', 'Other'];
const ABORTED_MESSAGE = 'Request was aborted by browser';

// Stable, machine-readable codes for abort errors thrown from this module.
// Consumers should prefer `error.code` over string matching on `error.message`.
export const AbortCodes = Object.freeze({
ABORTED: 'ABORTED',
TIMEOUT_NETWORK_IDLE: 'TIMEOUT_NETWORK_IDLE'
});

// RequestLifeCycleHandler handles life cycle of a requestId
// Ideal flow: requestWillBeSent -> requestPaused -> responseReceived -> loadingFinished / loadingFailed
// ServiceWorker flow: requestWillBeSent -> responseReceived -> loadingFinished / loadingFailed
Expand All @@ -22,8 +29,6 @@ class RequestLifeCycleHandler {
// The Interceptor class creates common handlers for dealing with intercepting asset requests
// for a given page using various devtools protocol events and commands.
export class Network {
static TIMEOUT = undefined;

log = logger('core:discovery');

#requestsLifeCycleHandler = new DefaultMap(() => new RequestLifeCycleHandler());
Expand Down Expand Up @@ -101,11 +106,13 @@ export class Network {

return requests.length === 0;
}, {
timeout: Network.TIMEOUT,
timeout: this.networkIdleWaitTimeout,
idle: timeout
}).catch(error => {
if (error.message.startsWith('Timeout')) {
let message = 'Timed out waiting for network requests to idle.';
let message = 'Timed out waiting for network requests to idle.\n' +
'Hint: set PERCY_NETWORK_IDLE_WAIT_TIMEOUT to increase the budget, ' +
'or allowlist slow domains via the discovery config.';
if (captureResponsiveAssetsEnabled) message += '\nWhile capturing responsive assets try setting PERCY_DO_NOT_CAPTURE_RESPONSIVE_ASSETS to true.';
this._throwTimeoutError(message, filter);
} else {
Expand Down Expand Up @@ -135,7 +142,10 @@ export class Network {
if (params.requestId) {
/* istanbul ignore if: race condition, very hard to mock this */
if (this.isAborted(params.requestId)) {
throw new Error(ABORTED_MESSAGE);
throw new AbortError(ABORTED_MESSAGE, {
code: AbortCodes.ABORTED,
reason: 'browser-aborted'
});
}
}

Expand Down Expand Up @@ -166,7 +176,14 @@ export class Network {
return;
}

throw new Error(msg);
// Use a plain Error (NOT AbortError) so this does not trip
// `error.name === 'AbortError'` consumers in discovery.js:520,
// percy.js:347, snapshot.js:472 — those treat AbortError as
// "snapshot was aborted" and would silently drop the timeout.
let err = new Error(msg);
err.code = AbortCodes.TIMEOUT_NETWORK_IDLE;
err.reason = 'network-idle-timeout';
throw err;
}

// Called when a request should be removed from various trackers
Expand Down Expand Up @@ -384,11 +401,11 @@ export class Network {
}

_initializeNetworkIdleWaitTimeout() {
if (Network.TIMEOUT) return;

Network.TIMEOUT = parseInt(process.env.PERCY_NETWORK_IDLE_WAIT_TIMEOUT) || 30000;
// Per-instance timeout so concurrent pages with different env values
// (or env values changed mid-run by tests) don't stomp each other.
this.networkIdleWaitTimeout = parseInt(process.env.PERCY_NETWORK_IDLE_WAIT_TIMEOUT) || 30000;

if (Network.TIMEOUT > 60000) {
if (this.networkIdleWaitTimeout > 60000) {
this.log.warn('Setting PERCY_NETWORK_IDLE_WAIT_TIMEOUT over 60000ms is not recommended. ' +
'If your page needs more than 60000ms to idle due to CPU/Network load, ' +
'its recommended to increase CI resources where this cli is running.');
Expand Down Expand Up @@ -526,7 +543,7 @@ async function sendResponseResource(network, request, session) {
// Note: its not a necessity that we would get aborted callback in a tick, its just that if we
// already have it then we can safely ignore this error
// Its very hard to test it as this function should be called and request should get cancelled before
if (error.message === ABORTED_MESSAGE || error.message.includes('Invalid InterceptionId')) {
if (error.code === AbortCodes.ABORTED || error.message === ABORTED_MESSAGE || error.message.includes('Invalid InterceptionId')) {
// defer this to the end of queue to make sure that any incoming aborted messages were
// handled and network.#aborted is updated
await new Promise((res, _) => process.nextTick(res));
Expand Down
10 changes: 7 additions & 3 deletions packages/core/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,9 @@ export async function executeDomainValidation(network, hostname, url, domainVali
// Worker returns 'accessible' field, not 'allowed'
if (result?.error) {
newErrorHosts.add(hostname);
network.log.debug(`Domain validation: ${hostname} validated as BLOCKED - ${result?.reason}`, network.meta);
// Redact upstream-derived `result.reason` — may contain credentials
// from response bodies the validation worker echoed.
network.log.debug(redactSecrets(`Domain validation: ${hostname} validated as BLOCKED - ${result?.reason}`), network.meta);
processedDomains.set(hostname, false);
return false;
} else if (!result?.accessible) {
Expand All @@ -208,8 +210,10 @@ export async function executeDomainValidation(network, hostname, url, domainVali
}
return false;
} catch (error) {
// On error, default to allowing (fail-open for better UX)
network.log.warn(`Domain validation: Failed to validate ${hostname} - ${error.message}`, network.meta);
// On error, default to allowing (fail-open for better UX).
// Redact `error.message` — upstream HTTP errors can include
// Authorization headers / URL credentials in the failed-request text.
network.log.warn(redactSecrets(`Domain validation: Failed to validate ${hostname} - ${error.message}`), network.meta);
processedDomains.set(hostname, false);
return false;
} finally {
Expand Down
17 changes: 9 additions & 8 deletions packages/core/test/discovery.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1328,11 +1328,7 @@ describe('Discovery', () => {
});

describe('idle timeout', () => {
let Network;

beforeEach(async () => {
({ Network } = await import('../src/network.js'));
Network.TIMEOUT = undefined;
process.env.PERCY_NETWORK_IDLE_WAIT_TIMEOUT = 500;

// some async request that takes a while
Expand All @@ -1344,7 +1340,6 @@ describe('Discovery', () => {
});

afterEach(() => {
Network.TIMEOUT = undefined;
process.env.PERCY_NETWORK_IDLE_WAIT_TIMEOUT = undefined;
process.env.PERCY_IGNORE_TIMEOUT_ERROR = undefined;
});
Expand All @@ -1356,7 +1351,11 @@ describe('Discovery', () => {
});

expect(logger.stderr).toContain(
'[percy] Error: Timed out waiting for network requests to idle.'
jasmine.stringContaining('[percy] Error: Timed out waiting for network requests to idle.')
);
// R7: actionable hint surfaces in the same error message.
expect(logger.stderr).toContain(
jasmine.stringContaining('PERCY_NETWORK_IDLE_WAIT_TIMEOUT')
);

let expectedRequestBody = {
Expand All @@ -1367,7 +1366,7 @@ describe('Discovery', () => {
meta: { build: { id: '123', url: 'https://percy.io/test/test/123', number: 1 }, snapshot: { name: 'test idle' } }
},
{
message: 'Timed out waiting for network requests to idle.',
message: jasmine.stringContaining('Timed out waiting for network requests to idle.'),
meta: { build: { id: '123', url: 'https://percy.io/test/test/123', number: 1 }, snapshot: { name: 'test idle' } }
}
]
Expand Down Expand Up @@ -1401,6 +1400,7 @@ describe('Discovery', () => {

expect(logger.stderr).toContain(jasmine.stringMatching([
'^\\[percy:core] Error: Timed out waiting for network requests to idle.',
'Hint: set PERCY_NETWORK_IDLE_WAIT_TIMEOUT to increase the budget, or allowlist slow domains via the discovery config.',
'While capturing responsive assets try setting PERCY_DO_NOT_CAPTURE_RESPONSIVE_ASSETS to true.',
'',
' Active requests:',
Expand All @@ -1417,7 +1417,7 @@ describe('Discovery', () => {
meta: { build: { id: '123', url: 'https://percy.io/test/test/123', number: 1 }, snapshot: { name: 'test idle' } }
},
{
message: 'Timed out waiting for network requests to idle.\nWhile capturing responsive assets try setting PERCY_DO_NOT_CAPTURE_RESPONSIVE_ASSETS to true.\n\n Active requests:\n - http://localhost:8000/img-fromsrcset.png\n',
message: 'Timed out waiting for network requests to idle.\nHint: set PERCY_NETWORK_IDLE_WAIT_TIMEOUT to increase the budget, or allowlist slow domains via the discovery config.\nWhile capturing responsive assets try setting PERCY_DO_NOT_CAPTURE_RESPONSIVE_ASSETS to true.\n\n Active requests:\n - http://localhost:8000/img-fromsrcset.png\n',
meta: { build: { id: '123', url: 'https://percy.io/test/test/123', number: 1 }, snapshot: { name: 'test idle' } }
}
]
Expand Down Expand Up @@ -1496,6 +1496,7 @@ describe('Discovery', () => {

expect(logger.stderr).not.toContain(jasmine.stringMatching([
'^\\[percy:core] Error: Timed out waiting for network requests to idle.',
'Hint: set PERCY_NETWORK_IDLE_WAIT_TIMEOUT to increase the budget, or allowlist slow domains via the discovery config.',
'',
' Active requests:',
' - http://localhost:8000/img.gif',
Expand Down
56 changes: 56 additions & 0 deletions packages/core/test/unit/network.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { setupTest } from '../helpers/index.js';
import { Network, AbortCodes } from '../../src/network.js';
import { AbortError } from '../../src/utils.js';

describe('Unit / Network', () => {
beforeEach(async () => {
await setupTest();
});

afterEach(() => {
process.env.PERCY_NETWORK_IDLE_WAIT_TIMEOUT = undefined;
});

// SC6 — concurrent pages with different PERCY_NETWORK_IDLE_WAIT_TIMEOUT
// values must each see their own value. Pre-fix this was a static class
// field so the second instance overwrote the first.
describe('SC6: instance-scoped network-idle wait timeout', () => {
it('initializes per-instance from env at construction time', () => {
process.env.PERCY_NETWORK_IDLE_WAIT_TIMEOUT = '1234';
let n1 = new Network({}, { userAgent: 'test' });

process.env.PERCY_NETWORK_IDLE_WAIT_TIMEOUT = '5678';
let n2 = new Network({}, { userAgent: 'test' });

expect(n1.networkIdleWaitTimeout).toBe(1234);
expect(n2.networkIdleWaitTimeout).toBe(5678);
});

it('falls back to 30000ms when env is unset or invalid', () => {
process.env.PERCY_NETWORK_IDLE_WAIT_TIMEOUT = undefined;
let n1 = new Network({}, { userAgent: 'test' });
expect(n1.networkIdleWaitTimeout).toBe(30000);

process.env.PERCY_NETWORK_IDLE_WAIT_TIMEOUT = 'not-a-number';
let n2 = new Network({}, { userAgent: 'test' });
expect(n2.networkIdleWaitTimeout).toBe(30000);
});
});

// R5 — verify the exported AbortCodes contract.
describe('R5: AbortCodes', () => {
it('exports a frozen enum with the codes the network module throws', () => {
expect(AbortCodes.ABORTED).toBe('ABORTED');
expect(AbortCodes.TIMEOUT_NETWORK_IDLE).toBe('TIMEOUT_NETWORK_IDLE');
expect(Object.isFrozen(AbortCodes)).toBe(true);
});

it('AbortError carries code and reason while keeping name=AbortError', () => {
let err = new AbortError('msg', { code: AbortCodes.ABORTED, reason: 'browser-aborted' });
expect(err.name).toBe('AbortError');
expect(err.code).toBe('ABORTED');
expect(err.reason).toBe('browser-aborted');
expect(err.message).toBe('msg');
});
});
});
16 changes: 16 additions & 0 deletions packages/core/test/unit/utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,22 @@ describe('Unit / Utils', () => {
it('should redact sensitive keys from array of object', () => {
expect(redactSecrets([{ message: 'This is a secret: ASIAY34FZKBOKMUTVV7A' }])).toEqual([{ message: 'This is a secret: [REDACTED]' }]);
});

// SC8 fixtures — verify the categories the plan claims to cover
// for the domain-validation log path. Categories outside secretPatterns.yml
// (Cookie:, JSESSIONID, custom auth schemes) are deferred to a yml-augment ticket.
describe('SC8: domain-validation error fixtures', () => {
it('redacts AWS access keys embedded in upstream error text', () => {
let msg = 'Domain validation: Failed to validate example.com - AWS error AKIAIOSFODNN7EXAMPLE returned';
expect(redactSecrets(msg)).toEqual(jasmine.stringContaining('[REDACTED]'));
expect(redactSecrets(msg)).not.toContain('AKIAIOSFODNN7EXAMPLE');
});

it('redacts URL-embedded credentials', () => {
let msg = 'Domain validation: Failed to validate example.com - request to https://admin:secret-AKIAIOSFODNN7EXAMPLE@host/path failed';
expect(redactSecrets(msg)).toContain('[REDACTED]');
});
});
});

describe('base64encode', () => {
Expand Down
Loading