-
Notifications
You must be signed in to change notification settings - Fork 103
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
[security] More backslash fixes #197
Conversation
assume(parsed.hostname).equals('github.com'); | ||
assume(parsed.pathname).equals('/foo/bar'); | ||
|
||
url = 'https:/\/\/\github.com/foo/bar'; |
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.
@3rd-Eden did you mean /\\/\\/\\
here? 3 characters are currently unnecessarily escaped.
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.
It's just testing that literally any slash (forward/backward) or combination of both is allowed.
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.
Ok, then it should be url = 'https:/\\/\\/\\github.com/foo/bar'
.
// to always have a / | ||
// | ||
if (url.pathname.charAt(0) !== '/' && url.hostname) { | ||
url.pathname = '/' + url.pathname; |
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 cause break change, see below demo code:
// assume this code exec in page http://cone-cf8b5c0e.app-dev.alipay.net/cone/strategy
const { pathname} = url('/cone/operate');
This PR cause pathname change from 'cone/operate' to '/cone/cone/operate'
i don't know of pass '/cone/operate' is a valid argument, if no, i think throw error is a better way
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.
Could you create an issue about this so we can track it? We do known issue with relative paths atm see #200 so it might be related to this bug.
CVE-2021-27515 was assigned to this PR. |
As per title, it seems that the previous security fix released in 1.4.5 only partially fixed the issue, with this adjustment to the regular expression we now have parity with the browser built-in URL parser as well. This change also exposed an issue where we didn't default pathnames to / when nothing was supplied in URL's.
That should now be resolved as well.