Skip to content

Normalize fragment encoding to avoid duplicate route fires (#4132) #4303

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.

Sign up for GitHub

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

Open
BigBalli wants to merge 1 commit into
base: master
Choose a base branch
from
+38 −4
Open

Normalize fragment encoding to avoid duplicate route fires (#4132) #4303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions backbone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1885,7 +1885,9 @@
this._wantsPushState = !!this.options.pushState;
this._hasPushState = !!(this.history && this.history.pushState);
this._usePushState = this._wantsPushState && this._hasPushState;
this.fragment = this.getFragment();
// Store the initial fragment in decoded form for consistency
// with what navigate()/loadUrl() write (#4132).
this.fragment = this.decodeFragment(this.getFragment());

// Normalize root to always include a leading and trailing slash.
this.root = ('/' + this.root + '/').replace(rootStripper, '/');
Expand Down Expand Up @@ -1979,12 +1981,19 @@
// Checks the current URL to see if it has changed, and if it has,
// calls `loadUrl`, normalizing across the hidden iframe.
checkUrl: function(e) {
var current = this.getFragment();
// Compare in decoded form. Firefox returns `location.href` with
// percent-encoded non-ASCII characters in the hash, while Chrome
// returns them decoded. Without normalising here, navigating to a
// URL like `#search/大阪` from inside the app stores the decoded
// form in `this.fragment` while the next `hashchange` event reads
// back the encoded form, so checkUrl thinks the URL changed and
// fires the route a second time. See #4132, #4085, #3941.
var current = this.decodeFragment(this.getFragment());

Comment on lines +1988 to 1992
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decodeFragment uses decodeURI, which throws URIError on malformed percent-escapes (e.g. a user manually enters a hash containing a stray % or incomplete UTF-8 sequence). By decoding unconditionally in start/checkUrl/loadUrl now, hashChange navigation that previously tolerated such URLs can start crashing the router. Consider wrapping the decode in a try/catch (returning the original fragment on failure) or adding a safe decode helper used by these call sites so invalid fragments don’t break history handling.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@paulfalgout paulfalgout Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it's suggesting:

decodeFragment: function(fragment) {
  var raw = fragment;
  fragment = fragment.replace(/%25/g, '%2525');
  try {
    return decodeURI(fragment);
  } catch (e) {
    return raw;
  }
}

I suppose it could be breaking.. although seems unlikely

// If the user pressed the back button, the iframe's hash will have
// changed and we should use that for comparison.
if (current === this.fragment && this.iframe) {
current = this.getHash(this.iframe.contentWindow);
current = this.decodeFragment(this.getHash(this.iframe.contentWindow));
}

if (current === this.fragment) {
Expand All @@ -2001,7 +2010,11 @@
loadUrl: function(fragment) {
// If the root doesn't match, no routes can match either.
if (!this.matchRoot()) return this.notfound();
fragment = this.fragment = this.getFragment(fragment);
// Always store the fragment in its decoded form so that the
// value cached in `this.fragment` is consistent with what
// `navigate()` writes — see checkUrl for the cross-browser
// rationale (#4132).
fragment = this.fragment = this.decodeFragment(this.getFragment(fragment));
Copy link
Copy Markdown
Collaborator

@paulfalgout paulfalgout Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

fragment = this.getFragment(fragment);
this.fragment = this.decodeFragment(fragment);

Decode only the cached fragment for comparison; don’t decode the fragment you match routes against, or you change routing semantics.

return _.some(this.handlers, function(handler) {
if (handler.route.test(fragment)) {
handler.callback(fragment);
Expand Down
21 changes: 21 additions & 0 deletions test/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,27 @@
Backbone.history.start({pushState: true});
});

QUnit.test('#4132 - checkUrl does not double-fire on encoded non-ASCII hash.', function(assert) {
assert.expect(1);
var count = 0;
var history = new Backbone.History;
history._wantsHashChange = true;
history._hasPushState = false;
history._usePushState = false;
history.matchRoot = function() { return true; };
history.handlers = [{
route: /^search\/(.+)$/,
callback: function() { count++; }
}];
// Simulate Firefox returning the hash percent-encoded in
// location.href even though navigate() previously stored the
// decoded form on this.fragment.
history.location = {href: 'http://example.com/#search/%E5%A4%A7%E9%98%AA'};
history.fragment = 'search/大阪';
history.checkUrl();
assert.strictEqual(count, 0, 'route handler must not be re-fired');
});

QUnit.test('Router#execute receives callback, args, name.', function(assert) {
assert.expect(3);
location.replace('http://example.com#foo/123/bar?x=y');
Expand Down
Loading