From fc5d0efc86cfceeba8784c2d9ed87ba1374fc352 Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 1 Aug 2024 01:08:19 +0530 Subject: [PATCH 01/13] fix: decoding manually for reserved characters --- packages/core/src/config.js | 5 +++ packages/core/src/discovery.js | 1 + packages/core/src/snapshot.js | 2 +- packages/core/src/utils.js | 26 ++++++++++++- packages/core/test/snapshot.test.js | 57 +++++++++++++++++++++++++++++ snapshot.yml | 3 ++ 6 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 snapshot.yml diff --git a/packages/core/src/config.js b/packages/core/src/config.js index 48d50b11b..a0267205b 100644 --- a/packages/core/src/config.js +++ b/packages/core/src/config.js @@ -50,6 +50,10 @@ export const configSchema = { type: 'boolean', default: true }, + modifySnapshotUrl: { + type: 'boolean', + default: true + }, disableShadowDOM: { type: 'boolean', default: false @@ -279,6 +283,7 @@ export const snapshotSchema = { percyCSS: { $ref: '/config/snapshot#/properties/percyCSS' }, enableJavaScript: { $ref: '/config/snapshot#/properties/enableJavaScript' }, cliEnableJavaScript: { $ref: '/config/snapshot#/properties/cliEnableJavaScript' }, + modifySnapshotUrl: { $ref: '/config/snapshot#/properties/modifySnapshotUrl' }, disableShadowDOM: { $ref: '/config/snapshot#/properties/disableShadowDOM' }, domTransformation: { $ref: '/config/snapshot#/properties/domTransformation' }, enableLayout: { $ref: '/config/snapshot#/properties/enableLayout' }, diff --git a/packages/core/src/discovery.js b/packages/core/src/discovery.js index fdc0d247c..e3ef80402 100644 --- a/packages/core/src/discovery.js +++ b/packages/core/src/discovery.js @@ -43,6 +43,7 @@ function debugSnapshotOptions(snapshot) { debugProp(snapshot, 'enableJavaScript'); debugProp(snapshot, 'cliEnableJavaScript'); debugProp(snapshot, 'disableShadowDOM'); + debugProp(snapshot, 'modifySnapshotUrl'); debugProp(snapshot, 'enableLayout'); debugProp(snapshot, 'domTransformation'); debugProp(snapshot, 'reshuffleInvalidTags'); diff --git a/packages/core/src/snapshot.js b/packages/core/src/snapshot.js index 37a841d0e..e0e99d8b4 100644 --- a/packages/core/src/snapshot.js +++ b/packages/core/src/snapshot.js @@ -102,7 +102,7 @@ function mapSnapshotOptions(snapshots, context) { // transform snapshot URL shorthand into an object if (typeof snapshot === 'string') snapshot = { url: snapshot }; - validateAndFixSnapshotUrl(snapshot); + if (snapshot.modifySnapshotUrl !== false && process.env.PERCY_MODIFY_SNAPSHOT_URL !== 'false') validateAndFixSnapshotUrl(snapshot); let url = validURL(snapshot.url, context?.baseUrl); // normalize the snapshot url and use it for the default name diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index ef4f9f638..9a5eafc88 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -375,6 +375,29 @@ export function base64encode(content) { .toString('base64'); } +const RESERVED_CHARACTERS = { + '%3A': ':', + '%23': '#', + '%24': '$', + '%26': '&', + '%2B': '+', + '%2C': ',', + '%2F': '/', + '%3B': ';', + '%3D': '=', + '%3F': '?', + '%40': '@' +}; + +function _replaceReservedCharacters(url) { + let result = url; + for (let [key, value] of Object.entries(RESERVED_CHARACTERS)) { + let regex = new RegExp(key, 'g'); + result = result.replace(regex, value); + } + return result; +} + // This function replaces invalid character that are not the // part of valid URI syntax with there correct encoded value. // Also, if a character is a part of valid URI syntax, those characters @@ -388,7 +411,8 @@ export function decodeAndEncodeURLWithLogging(url, logger, options = {}) { // correctly. const { meta, shouldLogWarning, warningMessage } = options; try { - let decodedURL = decodeURI(url); // This can throw error, so handle it will trycatch + let decodedURL = _replaceReservedCharacters(url); + decodedURL = decodeURI(decodedURL); let encodedURL = encodeURI(decodedURL); return encodedURL; } catch (error) { diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index 913facf46..fc2ea3bbf 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -180,6 +180,63 @@ describe('Snapshot', () => { sharedExpectModifiedSnapshotURL('http://localhost:8000/advanced-search/cf1a5848-f658-4939-be11-dctwo-%7B%20abc%20%5Bdbd%5D%20%7D/2253'); }); + it('do not double encode reserved URI character(%2F) in snapshot url', async () => { + let partiallyEncodedURL = 'http://localhost:8000/https%2F/abc[123].html'; + await percy.snapshot([ + { url: partiallyEncodedURL } + ]); + + sharedExpectModifiedSnapshotURL('http://localhost:8000/https//abc%5B123%5D.html'); + }); + + describe('does not modifies snapshot url', () => { + let partiallyEncodedURL = 'http://localhost:8000/https%2F/abc[123].html'; + const sharedExpect = () => { + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy:core:snapshot] Received snapshot: /https%2F/abc[123].html', + '[percy:core:snapshot] - url: http://localhost:8000/https%2F/abc[123].html' + ])); + + expect(logger.stderr).not.toEqual(jasmine.arrayContaining([ + '[percy:core:snapshot] Snapshot URL modified to: http://localhost:8000/https//abc%5B123%5D.html' + ])); + }; + + it('when modifySnapshotUrl set to false', async () => { + await percy.snapshot([ + { url: partiallyEncodedURL, modifySnapshotUrl: true } + ]); + + sharedExpect(); + }); + + describe('with PERCY_MODIFY_SNAPSHOT_URL=false', () => { + beforeEach(() => { + process.env.PERCY_MODIFY_SNAPSHOT_URL = 'false'; + }); + + afterEach(() => { + process.env.PERCY_MODIFY_SNAPSHOT_URL = 'true'; + }); + + it('uses original snapshot url', async () => { + await percy.snapshot([ + { url: partiallyEncodedURL } + ]); + + sharedExpect(); + }); + + fit('uses original snapshot url if modifySnapshotUrl = false ', async () => { + await percy.snapshot([ + { url: partiallyEncodedURL, modifySnapshotUrl: false } + ]); + + sharedExpect(); + }); + }); + }); + it('warns for invalid characters that can not get encoded properly', async () => { let givenURL = 'http://localhost:8000/advanced-search/cf1a5848%-f658-4939-be11-dct'; await percy.snapshot([ diff --git a/snapshot.yml b/snapshot.yml new file mode 100644 index 000000000..9e7c65f9a --- /dev/null +++ b/snapshot.yml @@ -0,0 +1,3 @@ +name: 'some screenshot' +url: http://localhost:9000/https%3A/abc[123].html +waitForTimeout: 3000 \ No newline at end of file From f9d8a4cb214249ca3d59628cbf005da2e42b29bc Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 1 Aug 2024 01:11:16 +0530 Subject: [PATCH 02/13] chore: typo fix --- packages/core/test/snapshot.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index fc2ea3bbf..76cd5fd52 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -227,7 +227,7 @@ describe('Snapshot', () => { sharedExpect(); }); - fit('uses original snapshot url if modifySnapshotUrl = false ', async () => { + it('uses original snapshot url if modifySnapshotUrl = false ', async () => { await percy.snapshot([ { url: partiallyEncodedURL, modifySnapshotUrl: false } ]); From f9f377a32c26b8ce37f6d0f61c9ba599d85e3472 Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 1 Aug 2024 01:26:35 +0530 Subject: [PATCH 03/13] test: test fix on percy.test.js and snapshot.test.js --- packages/core/test/percy.test.js | 3 ++- packages/core/test/snapshot.test.js | 4 ++-- packages/core/types/index.d.ts | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/core/test/percy.test.js b/packages/core/test/percy.test.js index 7632f54e1..8e5af0579 100644 --- a/packages/core/test/percy.test.js +++ b/packages/core/test/percy.test.js @@ -74,7 +74,8 @@ describe('Percy', () => { percyCSS: '', enableJavaScript: false, disableShadowDOM: false, - cliEnableJavaScript: true + cliEnableJavaScript: true, + modifySnapshotUrl: true }); }); diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index 76cd5fd52..36659a398 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -202,9 +202,9 @@ describe('Snapshot', () => { ])); }; - it('when modifySnapshotUrl set to false', async () => { + fit('when modifySnapshotUrl set to false', async () => { await percy.snapshot([ - { url: partiallyEncodedURL, modifySnapshotUrl: true } + { url: partiallyEncodedURL, modifySnapshotUrl: false } ]); sharedExpect(); diff --git a/packages/core/types/index.d.ts b/packages/core/types/index.d.ts index fecedd2e7..e004915f6 100644 --- a/packages/core/types/index.d.ts +++ b/packages/core/types/index.d.ts @@ -45,6 +45,7 @@ interface CommonSnapshotOptions { percyCSS?: string; enableJavaScript?: boolean; disableShadowDOM?: boolean; + modifySnapshotUrl?: boolean; enableLayout?: boolean; domTransformation?: string; reshuffleInvalidTags?: boolean; From 609493a467c99fb5cdaf48717bfb8aeee4bd4ffa Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 1 Aug 2024 01:27:45 +0530 Subject: [PATCH 04/13] chore: revert file --- snapshot.yml | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 snapshot.yml diff --git a/snapshot.yml b/snapshot.yml deleted file mode 100644 index 9e7c65f9a..000000000 --- a/snapshot.yml +++ /dev/null @@ -1,3 +0,0 @@ -name: 'some screenshot' -url: http://localhost:9000/https%3A/abc[123].html -waitForTimeout: 3000 \ No newline at end of file From 34d40dfd84a505be71aeb2d6e7acfd307eaa2190 Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 1 Aug 2024 01:31:58 +0530 Subject: [PATCH 05/13] test: test fix --- packages/core/test/snapshot.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index 36659a398..898e7467b 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -202,7 +202,7 @@ describe('Snapshot', () => { ])); }; - fit('when modifySnapshotUrl set to false', async () => { + it('when modifySnapshotUrl set to false', async () => { await percy.snapshot([ { url: partiallyEncodedURL, modifySnapshotUrl: false } ]); From dc0aa9ae680a0cecb881909bf830f494ce985b47 Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 1 Aug 2024 18:21:19 +0530 Subject: [PATCH 06/13] revert: Removing modifySnapshotUrl from snaphshot config --- packages/core/src/config.js | 5 ----- packages/core/src/discovery.js | 1 - packages/core/src/snapshot.js | 2 +- packages/core/test/percy.test.js | 3 +-- packages/core/test/snapshot.test.js | 16 ---------------- packages/core/types/index.d.ts | 1 - 6 files changed, 2 insertions(+), 26 deletions(-) diff --git a/packages/core/src/config.js b/packages/core/src/config.js index a0267205b..48d50b11b 100644 --- a/packages/core/src/config.js +++ b/packages/core/src/config.js @@ -50,10 +50,6 @@ export const configSchema = { type: 'boolean', default: true }, - modifySnapshotUrl: { - type: 'boolean', - default: true - }, disableShadowDOM: { type: 'boolean', default: false @@ -283,7 +279,6 @@ export const snapshotSchema = { percyCSS: { $ref: '/config/snapshot#/properties/percyCSS' }, enableJavaScript: { $ref: '/config/snapshot#/properties/enableJavaScript' }, cliEnableJavaScript: { $ref: '/config/snapshot#/properties/cliEnableJavaScript' }, - modifySnapshotUrl: { $ref: '/config/snapshot#/properties/modifySnapshotUrl' }, disableShadowDOM: { $ref: '/config/snapshot#/properties/disableShadowDOM' }, domTransformation: { $ref: '/config/snapshot#/properties/domTransformation' }, enableLayout: { $ref: '/config/snapshot#/properties/enableLayout' }, diff --git a/packages/core/src/discovery.js b/packages/core/src/discovery.js index e3ef80402..fdc0d247c 100644 --- a/packages/core/src/discovery.js +++ b/packages/core/src/discovery.js @@ -43,7 +43,6 @@ function debugSnapshotOptions(snapshot) { debugProp(snapshot, 'enableJavaScript'); debugProp(snapshot, 'cliEnableJavaScript'); debugProp(snapshot, 'disableShadowDOM'); - debugProp(snapshot, 'modifySnapshotUrl'); debugProp(snapshot, 'enableLayout'); debugProp(snapshot, 'domTransformation'); debugProp(snapshot, 'reshuffleInvalidTags'); diff --git a/packages/core/src/snapshot.js b/packages/core/src/snapshot.js index e0e99d8b4..1f04077d3 100644 --- a/packages/core/src/snapshot.js +++ b/packages/core/src/snapshot.js @@ -102,7 +102,7 @@ function mapSnapshotOptions(snapshots, context) { // transform snapshot URL shorthand into an object if (typeof snapshot === 'string') snapshot = { url: snapshot }; - if (snapshot.modifySnapshotUrl !== false && process.env.PERCY_MODIFY_SNAPSHOT_URL !== 'false') validateAndFixSnapshotUrl(snapshot); + if (process.env.PERCY_MODIFY_SNAPSHOT_URL !== 'false') validateAndFixSnapshotUrl(snapshot); let url = validURL(snapshot.url, context?.baseUrl); // normalize the snapshot url and use it for the default name diff --git a/packages/core/test/percy.test.js b/packages/core/test/percy.test.js index 8e5af0579..7632f54e1 100644 --- a/packages/core/test/percy.test.js +++ b/packages/core/test/percy.test.js @@ -74,8 +74,7 @@ describe('Percy', () => { percyCSS: '', enableJavaScript: false, disableShadowDOM: false, - cliEnableJavaScript: true, - modifySnapshotUrl: true + cliEnableJavaScript: true }); }); diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index 898e7467b..c6b21c6e0 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -202,14 +202,6 @@ describe('Snapshot', () => { ])); }; - it('when modifySnapshotUrl set to false', async () => { - await percy.snapshot([ - { url: partiallyEncodedURL, modifySnapshotUrl: false } - ]); - - sharedExpect(); - }); - describe('with PERCY_MODIFY_SNAPSHOT_URL=false', () => { beforeEach(() => { process.env.PERCY_MODIFY_SNAPSHOT_URL = 'false'; @@ -226,14 +218,6 @@ describe('Snapshot', () => { sharedExpect(); }); - - it('uses original snapshot url if modifySnapshotUrl = false ', async () => { - await percy.snapshot([ - { url: partiallyEncodedURL, modifySnapshotUrl: false } - ]); - - sharedExpect(); - }); }); }); diff --git a/packages/core/types/index.d.ts b/packages/core/types/index.d.ts index e004915f6..fecedd2e7 100644 --- a/packages/core/types/index.d.ts +++ b/packages/core/types/index.d.ts @@ -45,7 +45,6 @@ interface CommonSnapshotOptions { percyCSS?: string; enableJavaScript?: boolean; disableShadowDOM?: boolean; - modifySnapshotUrl?: boolean; enableLayout?: boolean; domTransformation?: string; reshuffleInvalidTags?: boolean; From f485e59ca7e8504708e3696f9d968fa5310c08df Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 1 Aug 2024 18:27:32 +0530 Subject: [PATCH 07/13] test: added 1 more test for multiple reserved character --- packages/core/test/snapshot.test.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index c6b21c6e0..a8e380987 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -189,6 +189,15 @@ describe('Snapshot', () => { sharedExpectModifiedSnapshotURL('http://localhost:8000/https//abc%5B123%5D.html'); }); + it('do not double encode if multiple reserved URI characters in snapshot url', async () => { + let partiallyEncodedURL = 'http://localhost:8000/https%2F/abc[123]%3A%3F_abc.html'; + await percy.snapshot([ + { url: partiallyEncodedURL } + ]); + + sharedExpectModifiedSnapshotURL('http://localhost:8000/https//abc%5B123%5D:?_abc.html'); + }); + describe('does not modifies snapshot url', () => { let partiallyEncodedURL = 'http://localhost:8000/https%2F/abc[123].html'; const sharedExpect = () => { From 0a2b5eec9b5118f0b1049d4dbe3b06c90d9bf917 Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Tue, 20 Aug 2024 18:31:06 +0530 Subject: [PATCH 08/13] feat: preserving reserved character from encoding --- packages/core/src/utils.js | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index 9a5eafc88..6b2b5a908 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -391,7 +391,23 @@ const RESERVED_CHARACTERS = { function _replaceReservedCharacters(url) { let result = url; + let matchedPattern = {}; + let placeHolderCount = 0; for (let [key, value] of Object.entries(RESERVED_CHARACTERS)) { + let regex = new RegExp(key, 'g'); + if (regex.test(result)) { + let placeholder = `__PERCY_PLACEHOLDER_${placeHolderCount}__` + result = result.replace(regex, placeholder); + matchedPattern[placeholder] = key; + placeHolderCount++; + } + } + return {result, matchedPattern}; +} + +function _decodeMatchedPattern(matchedPattern, url) { + let result = url; + for (let [key, value] of Object.entries(matchedPattern)) { let regex = new RegExp(key, 'g'); result = result.replace(regex, value); } @@ -409,11 +425,17 @@ export function decodeAndEncodeURLWithLogging(url, logger, options = {}) { // will encode those characters again. Therefore decodeURI once helps is decoding // partially encoded URL and then after encoding it again, full URL get encoded // correctly. - const { meta, shouldLogWarning, warningMessage } = options; + const { + meta, + shouldLogWarning, + warningMessage + } = options; try { - let decodedURL = _replaceReservedCharacters(url); - decodedURL = decodeURI(decodedURL); + let {result, matchedPattern} = _replaceReservedCharacters(url); + console.log("--> gojo matched ", matchedPattern, result); + let decodedURL = decodeURI(result); let encodedURL = encodeURI(decodedURL); + encodedURL = _decodeMatchedPattern(matchedPattern, encodedURL); return encodedURL; } catch (error) { logger.debug(error, meta); From e2b3ae31b76249a2f382900e5245525897fef164 Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 22 Aug 2024 03:50:20 +0530 Subject: [PATCH 09/13] test: test fix + only show warning for network logs if snapshot.url != request.url --- packages/core/src/discovery.js | 2 +- packages/core/src/network.js | 10 +++++++++- packages/core/src/snapshot.js | 18 +++++++++++++++++- packages/core/src/utils.js | 17 ++++++++--------- packages/core/test/snapshot.test.js | 9 ++++----- 5 files changed, 39 insertions(+), 17 deletions(-) diff --git a/packages/core/src/discovery.js b/packages/core/src/discovery.js index fdc0d247c..dc57ffad0 100644 --- a/packages/core/src/discovery.js +++ b/packages/core/src/discovery.js @@ -342,7 +342,7 @@ export function createDiscoveryQueue(percy) { authorization: snapshot.discovery.authorization, userAgent: snapshot.discovery.userAgent, captureMockedServiceWorker: snapshot.discovery.captureMockedServiceWorker, - meta: snapshot.meta, + meta: { ...snapshot.meta, snapshotURL: snapshot.url }, // enable network inteception intercept: { diff --git a/packages/core/src/network.js b/packages/core/src/network.js index 4598e024e..6eb38827e 100644 --- a/packages/core/src/network.js +++ b/packages/core/src/network.js @@ -1,7 +1,7 @@ import { request as makeRequest } from '@percy/client/utils'; import logger from '@percy/logger'; import mime from 'mime-types'; -import { DefaultMap, createResource, hostnameMatches, normalizeURL, waitFor } from './utils.js'; +import { DefaultMap, createResource, hostnameMatches, normalizeURL, waitFor, decodeAndEncodeURLWithLogging } from './utils.js'; const MAX_RESOURCE_SIZE = 25 * (1024 ** 2); // 25MB const ALLOWED_STATUSES = [200, 201, 301, 302, 304, 307, 308]; @@ -207,6 +207,14 @@ export class Network { // do not handle data urls if (request.url.startsWith('data:')) return; + // Browsers handle URL encoding leniently, but invalid characters can break tools like Jackproxy. + // This code checks for issues such as `%` and leading spaces and warns the user accordingly. + decodeAndEncodeURLWithLogging(request.url, this.log, { + meta: { ...this.meta, url: request.url }, + shouldLogWarning: request.url !== this.meta.snapshotURL, + warningMessage: `An invalid URL was detected for url: ${request.url} - the snapshot may fail on Percy. Please verify that your asset URL is valid.` + }); + if (this.intercept) { this.#pending.set(requestId, event); if (this.captureMockedServiceWorker) { diff --git a/packages/core/src/snapshot.js b/packages/core/src/snapshot.js index e00b71a6f..b687fa175 100644 --- a/packages/core/src/snapshot.js +++ b/packages/core/src/snapshot.js @@ -7,7 +7,8 @@ import { request, hostnameMatches, yieldTo, - snapshotLogName + snapshotLogName, + decodeAndEncodeURLWithLogging } from './utils.js'; import { JobData } from './wait-for-job.js'; @@ -24,6 +25,21 @@ function validURL(url, base) { } } +function validateAndFixSnapshotUrl(snapshot) { + let log = logger('core:snapshot'); + // encoding snapshot url, if contians invalid URI characters/syntax + let modifiedURL = decodeAndEncodeURLWithLogging(snapshot.url, log, { + meta: { snapshot: { name: snapshot.name || snapshot.url } }, + shouldLogWarning: true, + warningMessage: `Invalid URL detected for url: ${snapshot.url} - the snapshot may fail on Percy. Please confirm that your website URL is valid.` + }); + + if (modifiedURL !== snapshot.url) { + log.debug(`Snapshot URL modified to: ${modifiedURL}`); + snapshot.url = modifiedURL; + } +} + // used to deserialize regular expression strings const RE_REGEXP = /^\/(.+)\/(\w+)?$/; diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index 6b2b5a908..81fc06e38 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -389,23 +389,23 @@ const RESERVED_CHARACTERS = { '%40': '@' }; -function _replaceReservedCharacters(url) { +function _replaceReservedCharactersWithPlaceholder(url) { let result = url; let matchedPattern = {}; let placeHolderCount = 0; - for (let [key, value] of Object.entries(RESERVED_CHARACTERS)) { + for (let key of Object.keys(RESERVED_CHARACTERS)) { let regex = new RegExp(key, 'g'); if (regex.test(result)) { - let placeholder = `__PERCY_PLACEHOLDER_${placeHolderCount}__` + let placeholder = `__PERCY_PLACEHOLDER_${placeHolderCount}__`; result = result.replace(regex, placeholder); matchedPattern[placeholder] = key; placeHolderCount++; } } - return {result, matchedPattern}; + return { url: result, matchedPattern }; } -function _decodeMatchedPattern(matchedPattern, url) { +function _replacePlaceholdersWithReservedCharacters(matchedPattern, url) { let result = url; for (let [key, value] of Object.entries(matchedPattern)) { let regex = new RegExp(key, 'g'); @@ -431,11 +431,10 @@ export function decodeAndEncodeURLWithLogging(url, logger, options = {}) { warningMessage } = options; try { - let {result, matchedPattern} = _replaceReservedCharacters(url); - console.log("--> gojo matched ", matchedPattern, result); - let decodedURL = decodeURI(result); + let { url: placeholderURL, matchedPattern } = _replaceReservedCharactersWithPlaceholder(url); + let decodedURL = decodeURI(placeholderURL); let encodedURL = encodeURI(decodedURL); - encodedURL = _decodeMatchedPattern(matchedPattern, encodedURL); + encodedURL = _replacePlaceholdersWithReservedCharacters(matchedPattern, encodedURL); return encodedURL; } catch (error) { logger.debug(error, meta); diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index a8e380987..456b9afa3 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -180,13 +180,13 @@ describe('Snapshot', () => { sharedExpectModifiedSnapshotURL('http://localhost:8000/advanced-search/cf1a5848-f658-4939-be11-dctwo-%7B%20abc%20%5Bdbd%5D%20%7D/2253'); }); - it('do not double encode reserved URI character(%2F) in snapshot url', async () => { + it('do not encode reserved URI character(%2F) in snapshot url', async () => { let partiallyEncodedURL = 'http://localhost:8000/https%2F/abc[123].html'; await percy.snapshot([ { url: partiallyEncodedURL } ]); - sharedExpectModifiedSnapshotURL('http://localhost:8000/https//abc%5B123%5D.html'); + sharedExpectModifiedSnapshotURL('http://localhost:8000/https%2F/abc%5B123%5D.html'); }); it('do not double encode if multiple reserved URI characters in snapshot url', async () => { @@ -195,7 +195,7 @@ describe('Snapshot', () => { { url: partiallyEncodedURL } ]); - sharedExpectModifiedSnapshotURL('http://localhost:8000/https//abc%5B123%5D:?_abc.html'); + sharedExpectModifiedSnapshotURL('http://localhost:8000/https%2F/abc%5B123%5D%3A%3F_abc.html'); }); describe('does not modifies snapshot url', () => { @@ -237,8 +237,7 @@ describe('Snapshot', () => { ]); expect(logger.stderr).toEqual(jasmine.arrayContaining([ - `[percy:core:snapshot] Invalid URL detected for url: ${givenURL} - the snapshot may fail on Percy. Please confirm that your website URL is valid.`, - `[percy:core:discovery] An invalid URL was detected for url: ${givenURL} - the snapshot may fail on Percy. Please verify that your asset URL is valid.` + `[percy:core:snapshot] Invalid URL detected for url: ${givenURL} - the snapshot may fail on Percy. Please confirm that your website URL is valid.` ])); }); From 8bcf155d2e5815236d6b29d6b8d19bf8aa2c323b Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 22 Aug 2024 12:52:29 +0530 Subject: [PATCH 10/13] chore: null check for passing tests --- packages/core/src/network.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/network.js b/packages/core/src/network.js index 6eb38827e..b498027c1 100644 --- a/packages/core/src/network.js +++ b/packages/core/src/network.js @@ -211,7 +211,7 @@ export class Network { // This code checks for issues such as `%` and leading spaces and warns the user accordingly. decodeAndEncodeURLWithLogging(request.url, this.log, { meta: { ...this.meta, url: request.url }, - shouldLogWarning: request.url !== this.meta.snapshotURL, + shouldLogWarning: request.url !== this.meta?.snapshotURL, warningMessage: `An invalid URL was detected for url: ${request.url} - the snapshot may fail on Percy. Please verify that your asset URL is valid.` }); From ca4273fdf2ce4f5a45c6a59894245b2b7d348a59 Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 22 Aug 2024 13:04:35 +0530 Subject: [PATCH 11/13] test: test coverage --- packages/core/test/discovery.test.js | 45 +++++++++++++++++++++++++- packages/core/test/utils.test.js | 48 ++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 packages/core/test/utils.test.js diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js index b181add91..3b80dfccf 100644 --- a/packages/core/test/discovery.test.js +++ b/packages/core/test/discovery.test.js @@ -647,6 +647,49 @@ describe('Discovery', () => { ])); }); + it('debug error log only for invalid network url', async () => { + let err = new Error('Some Invalid Error'); + spyOn(global, 'decodeURI').and.callFake((url) => { + if (url === 'http://localhost:8000/style.css') { + throw err; + } + return url; + }); + + percy.loglevel('debug'); + await percy.snapshot({ + name: 'test snapshot', + url: 'http://localhost:8000', + domSnapshot: testDOM + }); + + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy:core:page] Navigate to: http://localhost:8000/', + '[percy:core:discovery] Handling request: http://localhost:8000/', + '[percy:core:discovery] - Serving root resource', + `[percy:core:discovery] ${err.stack}`, + '[percy:core:discovery] Handling request: http://localhost:8000/style.css', + '[percy:core:discovery] Handling request: http://localhost:8000/img.gif' + ])); + }); + + it('detect invalid network url', async () => { + percy.loglevel('debug'); + await percy.snapshot({ + name: 'test snapshot', + url: 'http://localhost:8000', + domSnapshot: testDOM.replace('style.css', 'http://localhost:404/%style.css') + }); + + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy:core:discovery] Handling request: http://localhost:8000/', + '[percy:core:discovery] - Serving root resource', + '[percy:core:discovery] An invalid URL was detected for url: http://localhost:404/%style.css - the snapshot may fail on Percy. Please verify that your asset URL is valid.', + '[percy:core:discovery] Handling request: http://localhost:404/%style.css', + '[percy:core:discovery] Handling request: http://localhost:8000/img.gif' + ])); + }); + it('allows setting a custom discovery user-agent', async () => { let userAgent; @@ -1389,7 +1432,7 @@ describe('Discovery', () => { }); describe('Dom serialisation', () => { - it('should contain valid dom serialization values', async () => { + fit('should contain valid dom serialization values', async () => { const page = await percy.browser.page(); await page.goto('http://localhost:8000'); await page.insertPercyDom(); diff --git a/packages/core/test/utils.test.js b/packages/core/test/utils.test.js new file mode 100644 index 000000000..2a43d7c8f --- /dev/null +++ b/packages/core/test/utils.test.js @@ -0,0 +1,48 @@ +import { decodeAndEncodeURLWithLogging } from '../src/utils.js'; +import { logger } from './helpers/index.js'; +import percyLogger from '@percy/logger'; + +describe('utils', () => { + let log; + beforeEach(async () => { + log = percyLogger(); + logger.reset(true); + await logger.mock({ level: 'debug' }); + }); + + describe('decodeAndEncodeURLWithLogging', () => { + it('does not warn invalid url when options params is null', async () => { + decodeAndEncodeURLWithLogging('https://abc.com/test%abc', log); + expect(logger.stderr).toEqual([]); + }); + + it('does not warn invalid url when shouldLogWarning = false', async () => { + decodeAndEncodeURLWithLogging('https://abc.com/test%abc', log, + { + shouldLogWarning: false + }); + + expect(logger.stderr).toEqual([]); + }); + + it('returns modified url', async () => { + const url = decodeAndEncodeURLWithLogging('https://abc.com/test[ab]c', log, + { + shouldLogWarning: false + }); + expect(logger.stderr).toEqual([]); + expect(url).toEqual('https://abc.com/test%5Bab%5Dc'); + }); + + it('warns if invalid url when shouldLogWarning = true', async () => { + decodeAndEncodeURLWithLogging( + 'https://abc.com/test%abc', + log, + { + shouldLogWarning: true, + warningMessage: 'some Warning Message' + }); + expect(logger.stderr).toEqual(['[percy] some Warning Message']); + }); + }); +}); From 7d4c7accff1995be1781af8d1bd14b6687989ff4 Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Thu, 22 Aug 2024 13:24:19 +0530 Subject: [PATCH 12/13] test: test fix --- packages/core/test/discovery.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js index 3b80dfccf..318a590f8 100644 --- a/packages/core/test/discovery.test.js +++ b/packages/core/test/discovery.test.js @@ -1432,7 +1432,7 @@ describe('Discovery', () => { }); describe('Dom serialisation', () => { - fit('should contain valid dom serialization values', async () => { + it('should contain valid dom serialization values', async () => { const page = await percy.browser.page(); await page.goto('http://localhost:8000'); await page.insertPercyDom(); From c92176cf8f1c97088dec5c655f780fee70e068d9 Mon Sep 17 00:00:00 2001 From: this-is-shivamsingh Date: Tue, 27 Aug 2024 12:34:13 +0530 Subject: [PATCH 13/13] chore: remvoving some excessive comments --- packages/core/src/network.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/network.js b/packages/core/src/network.js index b498027c1..66841152c 100644 --- a/packages/core/src/network.js +++ b/packages/core/src/network.js @@ -207,7 +207,7 @@ export class Network { // do not handle data urls if (request.url.startsWith('data:')) return; - // Browsers handle URL encoding leniently, but invalid characters can break tools like Jackproxy. + // Browsers handle URL encoding leniently. // This code checks for issues such as `%` and leading spaces and warns the user accordingly. decodeAndEncodeURLWithLogging(request.url, this.log, { meta: { ...this.meta, url: request.url },