-
Notifications
You must be signed in to change notification settings - Fork 48
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
Conversation
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.
LGTM
@@ -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 }, |
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.
- We are adding a snapshot url there, to use it in
network.js
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
- Since snapshot.url also goes via network.js, so if snapshot.url is invalid the error message is already been shown before so no need to show again. Only show for assets urls
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Using __PERCY_PLACEHOLDER_0__
as a placeholder, as these alphanumerical and _
doesn't get encoded as it is a part of a valid URL and not a part of reserved characters.
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.
LGTM
Reference PR: #1664
# : /
, then we don't have an issue, but if user given an encoded value of these characters, it does get's not decoded, and while encoding the%
part of get encoded againChanges:
List of all reserved characters for which decodeURI, doesn't work.
Revisiting solution for the above issue with an update:
?
and#
, then the browser converts them to their encoded form.abd?s.html
is hosted locally then the browser opens this file ashttp://localhost:9000/abd%3Fs.html
, now once we decode it manually, it becomeshttp://localhost:9000/abd?s.html
that is a valid file path, but the browser is not able to recognise it and return 404, because for?
browser think it to be a query params, but it is actually a filename, so it only accepts the encoded value of it.?
and#
only out of all reserved characters. And only happens for if a file name contains these 2 reserved characters.Changes: