-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Invalid URL Fix: Decoding manually for some reserved characters #1682
Changes from all commits
fc5d0ef
f9d8a4c
f9f377a
609493a
34d40df
dc0aa9a
f485e59
60292f9
0a2b5ee
e2b3ae3
8bcf155
ca4273f
7d4c7ac
c92176c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. | ||
// 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -375,6 +375,76 @@ export function base64encode(content) { | |
.toString('base64'); | ||
} | ||
|
||
const RESERVED_CHARACTERS = { | ||
'%3A': ':', | ||
'%23': '#', | ||
'%24': '$', | ||
'%26': '&', | ||
'%2B': '+', | ||
'%2C': ',', | ||
'%2F': '/', | ||
'%3B': ';', | ||
'%3D': '=', | ||
'%3F': '?', | ||
'%40': '@' | ||
}; | ||
|
||
function _replaceReservedCharactersWithPlaceholder(url) { | ||
let result = url; | ||
let matchedPattern = {}; | ||
let placeHolderCount = 0; | ||
for (let key of Object.keys(RESERVED_CHARACTERS)) { | ||
let regex = new RegExp(key, 'g'); | ||
if (regex.test(result)) { | ||
let placeholder = `__PERCY_PLACEHOLDER_${placeHolderCount}__`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using |
||
result = result.replace(regex, placeholder); | ||
matchedPattern[placeholder] = key; | ||
placeHolderCount++; | ||
} | ||
} | ||
return { url: result, matchedPattern }; | ||
} | ||
|
||
function _replacePlaceholdersWithReservedCharacters(matchedPattern, url) { | ||
let result = url; | ||
for (let [key, value] of Object.entries(matchedPattern)) { | ||
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 | ||
// are not encoded | ||
// Eg: [abc] -> gets encoded to %5Babc%5D | ||
// ab c -> ab%20c | ||
export function decodeAndEncodeURLWithLogging(url, logger, options = {}) { | ||
// In case the url is partially encoded, then directly using encodeURI() | ||
// 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; | ||
try { | ||
let { url: placeholderURL, matchedPattern } = _replaceReservedCharactersWithPlaceholder(url); | ||
let decodedURL = decodeURI(placeholderURL); | ||
let encodedURL = encodeURI(decodedURL); | ||
encodedURL = _replacePlaceholdersWithReservedCharacters(matchedPattern, encodedURL); | ||
return encodedURL; | ||
} catch (error) { | ||
logger.debug(error, meta); | ||
if (error.name === 'URIError' && shouldLogWarning) { | ||
logger.warn(warningMessage); | ||
} | ||
return url; | ||
} | ||
} | ||
|
||
export function snapshotLogName(name, meta) { | ||
if (meta?.snapshot?.testCase) { | ||
return `testCase: ${meta.snapshot.testCase}, ${name}`; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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']); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
network.js