Implement replaceUrl protocol for safe URL replacements #9175
Conversation
| const replaceUrlParam = this.params_['replaceUrl']; | ||
| if (ampdoc.isSingleDoc() && | ||
| replaceUrlParam && | ||
| this.win.history.replaceState) { |
There was a problem hiding this comment.
#replaceState check should be unnecessary, supported in everything but IE 9.
There was a problem hiding this comment.
I believe this is still an issue. Last time we tried to remove this check, we saw lots of errors. Feel free to file a bug for a deeper cleanup. But it will have to first start in the history-impl.js with error sampling for when it's not available.
| const url = parseUrl(this.win.location.href); | ||
| const replaceUrl = parseUrl( | ||
| removeFragment(replaceUrlParam) + this.win.location.hash); | ||
| if (url.origin == replaceUrl.origin && |
There was a problem hiding this comment.
compare case-insensitive ( I believe origin when coming from navigator will be lowercase but not when coming from this.params_['replaceUrl'] (don't see parseUrl or getSourceOrigin lowercase anything)
There was a problem hiding this comment.
I believe by spec, HTMLAnchorElement must lowercase origins.
There was a problem hiding this comment.
huh didn't know our parseUrl uses <a>. very cool!
test/functional/test-viewer.js
Outdated
| expect(windowApi.history.replaceState).to.be.calledWith({}, '', | ||
| 'http://www.example.com/two' + fragment); | ||
| expect(ampdoc.getUrl()) | ||
| .to.equal('http://www.example.com/two' + fragment); |
There was a problem hiding this comment.
I expected this to be 'http://www.example.com/two&b=1' + fragment. Looking at the code, I don't see how it is not, sort of confused. We remove fragment but not query string params and url.href should include query string params.
There was a problem hiding this comment.
For that to happen, & should be encoded. Basically, the whole fragment is parsed first using query parameter parse, which will end up with two params: #repalceUrl=http://www.example.com/two AND b=1. Only replaceUrl is used for this, so this test demonstrates that b=1 is ignored.
There was a problem hiding this comment.
got it. Might be worth adding a test for fragment = '#replaceUrl=http://www.example.com/two%25b=2&b=1'; to expect http://www.example.com/two&b=2 then.
test/functional/test-viewer.js
Outdated
| expect(windowApi.history.replaceState).to.be.calledWith({}, '', | ||
| 'http://www.example.com/two' + fragment); | ||
| expect(ampdoc.getUrl()) | ||
| .to.equal('http://www.example.com/two' + fragment); |
There was a problem hiding this comment.
got it. Might be worth adding a test for fragment = '#replaceUrl=http://www.example.com/two%25b=2&b=1'; to expect http://www.example.com/two&b=2 then.
Closes #9044.
/cc @ericfs @alf7