-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix(cid): localhost should only be considered as proxy if prefix is c or v. #1289
Conversation
Do the unit tests still go through the same code path? |
ea294e7
to
2651400
Compare
@cramforce with this current code, i can't 100% guarantee that. i think there could certainly be an indirect use of the im thinking of just removing the changes i made and doing this: if (url.origin.indexOf('http://localhost:') != 0) {
assert(prefix == 'c' || prefix == 'v',
'Unknown path prefix in url %s', url.href);
} in |
2651400
to
fe56bb8
Compare
I'm confused by the changeset that GH is showing now. Otherwise it was fine. |
This LGTM. What's the |
@cramforce i dont think i was confident that the previous code wouldn't be forcing the tests into the not sure what the v/c stands for, @cramforce can you shed some light on that one? |
a641501
to
21563f5
Compare
if (!(mode.localDev && | ||
(prefix == 'examples.build' || prefix == 'examples'))) { | ||
// whitelist localhost | ||
const localDev = location.hostname == 'localhost' || |
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.
I think what we discussed was to instead make #isProxyOrigin
only return true if the assert would not fail.
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.
@cramforce reverted to original change. PTAL
21563f5
to
271534f
Compare
271534f
to
484067b
Compare
// List of well known proxy hosts. New proxies must be added here | ||
// to generate correct tokens. | ||
return (url.origin == 'https://cdn.ampproject.org' || | ||
url.origin.indexOf('http://localhost:') == 0); | ||
(url.origin.indexOf('http://localhost:') == 0 && | ||
(prefix == 'c' || prefix == 'v'))); |
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.
@cramforce PTAL. changed to check if localhost has prefix c/v
LGTM |
fix(cid): localhost should only be considered as proxy if prefix is c or v.
No description provided.