Preserve percent-encoded trailing whitespace in fragments (#4198)#4301
Preserve percent-encoded trailing whitespace in fragments (#4198)#4301BigBalli wants to merge 1 commit intojashkenas:masterfrom
Conversation
…4198) The route stripper used to remove trailing whitespace from any fragment, including the value returned by getPath(), which decodes percent-encoded characters first. As a result, a fragment ending in %20 (e.g. an item id with a literal trailing space) was silently chopped, breaking router lookups for legitimate parameters. Move the trailing whitespace strip onto the raw hash value in getHash(), where the original fix for jashkenas#1794 belongs (location.hash returning trailing whitespace in old browsers). The path branch through getPath() and any explicit fragment passed to getFragment() now preserve trailing whitespace, fixing jashkenas#4198 while keeping jashkenas#1794 covered. Updated existing jashkenas#1794/jashkenas#1820 tests to assert at the right level and added a regression test for jashkenas#4198.
jgonggrijp
left a comment
jgonggrijp
left a comment
There was a problem hiding this comment.
Despite its small size, I found this proposal very tough to review. I stared at the git blame and at #4198, #1794, #1795, #1820, #967, #1156, #1426 and #2566. After a lot of thought, I think the fix is not quite right. Let me clarify with the following table about the internals of getFragment:
history.navigate |
hashChange |
popState |
|
|---|---|---|---|
source of fragment |
user-provided | this.getHash |
this.getPath |
fragment still encoded? |
yes (assumed) | yes | no |
leading #//? |
maybe | no | no |
| are trailing spaces removed? (current situation) | yes | yes | yes |
| should trailing spaces be removed? | yes | yes | no |
| will trailing spaces be removed? (your proposal) | no | yes | no |
Your proposed change is effectively over-compensating by preserving the trailing spaces not only for the popState scenario, but also for the history.navigate scenario. This has prompted you to change existing tests that I think were actually justified or at least not problematic enough to warrant a breaking change.
I suggest that you make the following adjustments:
- Keep the new regression test for #4198.
- Undo the changes to the other tests.
- Undo the change to
routeStripperand drop the newtrailingSpaceStrippervariable. - Undo the change to
getHash. - In the
getFragmentfunction, change the line that readsfragment = this.getPath();toreturn this.getPath();.
The immediate return in getFragment will avoid applying routeStripper in the popState scenario. This preserves the spaces that should be preserved and skips stripping the leading / which getPath already removes, anyway.
There was a problem hiding this comment.
This is changing the meaning of an existing test. I know this has been done a lot in the past history of Backbone, but I think it is fundamentally wrong. Either, you preserve a test and make sure that the code still passes it in the face of new changes, or you remove a test entirely if you can defend that it was testing for the wrong thing. In the latter case, you replace it by a different test that more accurately tests what should be tested.
| QUnit.test('#1820 - Leading slash stripped, trailing space preserved.', function(assert) { | ||
| assert.expect(1); | ||
| var history = new Backbone.History; | ||
| assert.strictEqual(history.getFragment('fragment '), 'fragment'); | ||
| assert.strictEqual(history.getFragment('/fragment '), 'fragment '); | ||
| }); | ||
|
|
||
| QUnit.test('#1820 - Leading slash and trailing space.', function(assert) { | ||
| QUnit.test('#4198 - Percent-encoded trailing space preserved in path.', function(assert) { | ||
| assert.expect(1); | ||
| var history = new Backbone.History; | ||
| assert.strictEqual(history.getFragment('/fragment '), 'fragment'); |
There was a problem hiding this comment.
Again, this is changing the meaning of an existing test.
| history.getFragment(), | ||
| 'outbound/22130600/po/powithspacetest ' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This is a useful test to add. Please keep it in your proposal.
2 participants
Fixes #4198.
Problem
routeStripper(/^[#\/]|\s+$/g) was applied at the very end ofgetFragment(), aftergetPath()has already decoded the URL viadecodeFragment(). As a result, an encoded trailing space (%20) wasdecoded to a literal space and then stripped, silently truncating
legitimate parameter values:
Why the strip exists
The trailing-space strip was introduced in #1794 to work around old
browsers where
location.hashitself returned trailing whitespace. Thatfix only ever needed to apply to the raw hash value — not to the
already-decoded path.
Fix
routeStripperinto two regexes:routeStripper = /^[#\/]/— only the leading hash/slash, appliedin
getFragment()as before.trailingSpaceStripper = /\s+$/— applied insidegetHash()onthe raw hash value, preserving the original Trailing spaces in search query cause route to be fired twice #1794 fix at the level
where it actually matters.
getFragment()no longer touches trailing whitespace, so explicitfragments and decoded paths preserve their trailing characters.
Tests
#1794test to assert againstgetHash()(thecorrect level), and added an assertion that
getFragment('fragment ')preserves whitespace.
#1820test (leading slash + trailing space) toreflect that only the leading slash is stripped now.
#4198covering a percent-encodedtrailing space in the path.
npm run lintpasses.