-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BUGIX release] [Fixes #14925] remove duplicate /
in pathname
#14961
Conversation
@stefanpenner is this a regression? does this happen when there are multiple epsilon segments? |
I don't believe it is a regression, rather browsers hard-error (but in the past they did not).
I'm not totally sure. Is your question if we should only strip some excess Which lead me to believe we may just want to strip unwanted |
Chatting with @krisselden this is more likely an issue in RR not collapsing these "epsilon" segments. |
Actually. This appears to be unrelated to RR (route recognizer), and should be thought of as more of a "cleanup" of user input (from location.pathname) |
👍 |
@@ -101,7 +101,8 @@ export default EmberObject.extend({ | |||
// remove baseURL and rootURL from start of path | |||
let url = path | |||
.replace(new RegExp(`^${baseURL}(?=/|$)`), '') | |||
.replace(new RegExp(`^${rootURL}(?=/|$)`), ''); | |||
.replace(new RegExp(`^${rootURL}(?=/|$)`), '') | |||
.replace(/\/\//g,'/'); // remove extra slashes |
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.
Should this be anchored? If not, can you add a test showing that something like /foo//bar/
is actually supported?
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'm open to suggestions, I can glady add that test, or anchor it. thoughts?
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.
lets just anchor at the end, we can always make it more permissive if we end up finding a need for it...
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.
sounds good
This avoids a DOMException, which interprets duplicate `/` as a security violation and prevents the associated history state modifications.
86c9c05
to
fe88d54
Compare
Updated as per @rwjblue's comment. We should be good to go if CI agrees. |
@rwjblue should I backport or are you? I don't want to confuse the "process". |
[Fixes #14925]
This avoids a DOMException, which interprets duplicate
/
as a security violation and prevents the associated history state modifications.error in question:
I'm not totally sure this is the appropriate fix, so maybe someone else should sanity check this one.