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
<amp-pinterest> Sanitize URLs #13885
Conversation
@@ -58,9 +58,9 @@ export class PinWidget { | |||
const log = el.getAttribute('data-pin-log'); | |||
if (href) { | |||
if (shouldPop) { | |||
openWindowDialog(window, href, '_pinit', POP); | |||
openWindowDialog(window, encodeURI(href), '_pinit', POP); |
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.
This won't do it. I can still use a javascript:alert('PWNED')
, which'll pass right through. We likely want something like assertAbsoluteHttpOrHttpsUrl
.
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.
Taken this out in favor of assertAbsoluteHttpOrHttpsUrl
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.
@jridgewell PTAL?
@@ -57,6 +57,7 @@ export class PinWidget { | |||
const href = el.getAttribute('data-pin-href'); | |||
const log = el.getAttribute('data-pin-log'); | |||
if (href) { | |||
assertAbsoluteHttpOrHttpsUrl(href); |
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.
Asserting that the href is starting with http/https here.
Won't allow localhost - so testing 🤷
Also can one still force code in the latter part of the URL?
Can you please merge it in too? I don't have the required rights. |
Ping me when travis passes. |
🔔🔔🔔 |
* Encode all UR components to sanitize them * Assert that all URLs are http or https
Currently we call openWindowDialog on a click on an AMP widget. These
href
s should be sanitized before being passed to openWindowDialog.