fix(router): fix = not parsed in router segment name by DW8Reaper · Pull Request #47332 · angular/angular

DW8Reaper · 2022-09-02T05:12:05Z

fix router segment name parsing to allow segements to container an unscaped = character. Currently if you have a url like /some-site/folder=/some-file then then middle segment "folder=" will stop parsing at the = sign and register that part of the path as just "folder"

Fixes #21381

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

As outlined in this issue (#21381) when you have a url where a path segment contains an = character angular will parse that segment as only the characters before the =. For example password/de/MDAtMNTk= the last segment will be parsed in angular as only MDAtMNTk without the = character

Issue Number: 21381

What is the new behavior?

After this change you may include any number of = characters in a segment name and correspondingly define those as route paths in your route configuration.

Does this PR introduce a breaking change?

Yes
No

I think this PR could potentially be a breaking change since someone might have specifically handled cases where = was ignored by angular. Suppose they had path segments that were base 64 encoded strings for example IMz3bg==, those could possibly end with one or two = characters. It is therefore conceivable that someone might have implemented a workaround where if they received the id as IMz3bg they could infer based on the missing bit count that two == characters should be added. In that case if angular now accepted = their resource names could suddenly become IMz3bg====

A counter argument would be that if you had logic to determine the number of missing bits if angular now parsed the = there wouldn't be any missing bits and your logic would not trigger

Other information

Based off of comments in the issue here (#21381 (comment)) and (#21381 (comment)) I looked into RFC 3986 and RFC 1630 and it would seem that there isn't any specific limitation on using = in a segment name. Consider this excerpt from RFC 3986:

For example, the semicolon (";") and equals ("=") reserved characters are often used to delimit parameters and parameter values applicable to that segment. The comma (",") reserved character is often used for similar purposes. For example, one URI producer might use a segment such as "name;v=1.1" to indicate a reference to version 1.1 of "name", whereas another might use a segment such as "name,1.1" to indicate the same. Parameter types may be defined by scheme-specific semantics, but in most cases the syntax of a parameter is specific to the implementation of the URI's dereferencing algorithm.

This seems to clearly indicate that you can use = and that is further substantiated by angulars special handling of matric parameters which uses the name;v=1.1 syntax from this excerpt

DW8Reaper · 2022-09-07T03:11:59Z

I did some testing in a sample app and when I have my routing configured as

const routes: Routes = [
  {path: 'another=test', component: TestComponent}
];

then it correctly loads TestComponent when I go to http://localhost;4200/another=test where previously it would have tried to got to a route another.

There is one thing that may be unexpected and that is after routing completes the new URL will encode the = so the URL becomes http://localhost:4200/another%3Dtest (that route also works though if you try load it). So now I'm wondering if that result after loading the page should keep the = unencoded it kinda seems like it should any opinion on that?

pkozlowski-opensource · 2022-09-12T18:30:05Z

@DW8Reaper sorry for the delay on reviewing / merging this one. It is likely that this change introduces a breaking change so we need to run some internal tests and go over their results.

atscott · 2022-09-19T16:14:39Z

Note: This is related to the internal issue b/234604699. These issues are related but actually solve distinctly separate parts of the parsing.

The solution here parses = in the segment path but specifically excludes it from matrix params.
The internal issue requires the = to be parsed in the matrix param value but does not attempt to parse it elsewhere in the path.

We'll need to evaluate both scenarios and may want to include both in the single solution. The matrix proposal does not appear to take a stance on the = character in parameter values (i.e. /a/b;myKey=myValueWithAn=InTheMiddle).

DW8Reaper · 2022-09-19T17:19:04Z

@atscott I'd be happy to update mine if you can provide some details on where the other place was that this was changed on your side. Of course if it is simpler to just update things internally that makes sense to and then I can decline this PR

atscott · 2022-09-19T17:22:38Z

@DW8Reaper I think essentially you just need to update const valueMatch = matchMatrixParamSegments(this.remaining); to be const valueMatch = matchSegments(this.remaining);. We can leave this open and give you the credit for the fix. We're testing the change internally, but that'll take a fair bit of time to roll out safely.

DW8Reaper · 2022-09-19T18:14:36Z

Cool I can do that, not sure if you saw my other comment @atscott about it not preserving the = when the page navigation completes instead it the % escapes it.

Should I see if I can fix that as well to make this more complete?

atscott · 2022-09-19T18:23:19Z

Cool I can do that, not sure if you saw my other comment @atscott about it not preserving the = when the page navigation completes instead it the % escapes it.

Should I see if I can fix that as well to make this more complete?

I didn't see that. Yea, that seems like something we might want to address at the same time if possible.

DW8Reaper · 2022-10-02T02:53:57Z

@atscott sorry that it took a while to update this PR but I have changed it so that if now will keep the = sign after routing in the URL bar of the browser

DW8Reaper · 2022-10-17T19:38:00Z

@atscott just following up is there anything else I need to do from my side for this PR?

atscott · 2022-10-17T20:03:16Z

@DW8Reaper No, I'm just waiting to hear back about the results from the internal experiment.

michaelgriscom · 2022-11-22T16:08:25Z

packages/router/src/url_tree.ts

As alluded to in #47332 (comment) we've got an internal bug using = in matrix parameter values that would be convenient to fix in this PR, all that's needed is to leave this line as calling matchSegments

That will also make it worth renaming matchMatrixParamSegments to add more precision, maybe something like matchMatrixKeySegment

packages/router/test/url_serializer.spec.ts

michaelgriscom · 2022-12-19T20:28:16Z

packages/router/src/url_tree.ts

After some additional discussion with @atscott we think it would be better to leave this change for a separate PR, i.e. this PR will handle parsing = in router segments and leave them encoded (the same way = are currently handled in query parameters), and in a different PR we can look into leaving them unencoded (sorry for the back and forth!)

Man I hope I got this straight I think all an all after you comments I was supposed to remove the URL encoding part then which I did by just reverting that whole second commit.

I wasn't sure if I was still supposed to remove the second call to matchMatrixParamSegments in the parseParam function from this comment:

As alluded to in #47332 (comment) we've got an internal bug using = in matrix parameter values that would be convenient to fix in this PR, all that's needed is to leave this line as calling matchSegments

That will also make it worth renaming matchMatrixParamSegments to add more precision, maybe something like matchMatrixKeySegment

Sorry for the delay! I was down with the flu

Yep these are the only changes left to do now:

Change the second matchMatrixParamSegments call to matchSegments (which will let = be an allowed character in matrix parameter values)

With (1), matchMatrixParamSegments will only be used for matrix keys so it is probably worth renaming it to matchMatrixKeySegment

A test to cover the functionality mentioned in (1), I mentioned one option in this comment: https://github.com/angular/angular/pull/47332/files#r1029601032

After those I think the PR will be good to go!

Cool I'll update it then but it'll probably only be in about 2 weeks I'm traveling and don't have my notebook with me

DW

fix router segment name parsing to allow segements to container an unscaped = character. Currently if you have a url like /some-site/folder=/some-file then then middle segment "folder=" will stop parsing at the = sign and register that part of the path as just "folder" Fixes angular#21381

This reverts commit 2279f4d4620eba083a9832ed096890b69a25ec42. Reverting that commit based off PR feedback that this change should only affect the parsing of sergments and node encoding of the url

… name"

packages/router/test/url_serializer.spec.ts

michaelgriscom

Looks great!

@atscott this looks ready to merge to me

atscott · 2023-04-10T16:01:30Z

@michaelgriscom Great, thanks for helping out on the review here! I'll take one last look and run a global presubmit

ngbot · 2023-04-10T21:26:44Z

I see that you just added the action: merge label, but the following checks are still failing:
status "ci/angular: size" is failing
2 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

atscott · 2023-04-10T21:26:48Z

Green TGP

AndrewKushnir · 2023-04-11T16:16:34Z

This PR was merged into the repository by commit ee816e1.

…47332) This reverts commit 2279f4d4620eba083a9832ed096890b69a25ec42. Reverting that commit based off PR feedback that this change should only affect the parsing of sergments and node encoding of the url PR Close #47332

fix router segment name parsing to allow segements to container an unscaped = character. Currently if you have a url like /some-site/folder=/some-file then then middle segment "folder=" will stop parsing at the = sign and register that part of the path as just "folder" Fixes #21381 PR Close #47332

…47332) This reverts commit 2279f4d4620eba083a9832ed096890b69a25ec42. Reverting that commit based off PR feedback that this change should only affect the parsing of sergments and node encoding of the url PR Close #47332

angular-automatic-lock-bot · 2023-05-12T00:08:09Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pullapprove bot requested a review from AndrewKushnir September 2, 2022 05:12

AndrewKushnir added area: router target: major This PR is targeted for the next major release flag: breaking change labels Sep 2, 2022

ngbot bot added this to the Backlog milestone Sep 2, 2022

AndrewKushnir added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 2, 2022

ngbot bot modified the milestone: Backlog Sep 2, 2022

AndrewKushnir assigned atscott Sep 2, 2022

AndrewKushnir requested a review from atscott September 2, 2022 17:03

AndrewKushnir unassigned atscott Sep 2, 2022

pullapprove bot removed the request for review from atscott September 2, 2022 17:03

pkozlowski-opensource self-requested a review September 3, 2022 21:21

AndrewKushnir requested a review from atscott September 12, 2022 18:29

pkozlowski-opensource added the action: global presubmit The PR is in need of a google3 global presubmit label Sep 12, 2022

pullapprove bot removed the request for review from atscott September 12, 2022 18:46

angular-robot bot removed the flag: breaking change label Oct 2, 2022

DW8Reaper force-pushed the router-equals branch from 1cbe11a to 34d8fc0 Compare October 4, 2022 02:07

michaelgriscom suggested changes Dec 19, 2022

View reviewed changes

DW8Reaper force-pushed the router-equals branch from 34d8fc0 to 7bf9688 Compare January 18, 2023 03:04

DW8Reaper added 4 commits April 7, 2023 22:50

fixup! fix(router): fix = not parsed in router segment name

28d8ca1

revert "fixup! fix(router): fix = not parsed in router segment name"

6b35bf8

This reverts commit 2279f4d4620eba083a9832ed096890b69a25ec42. Reverting that commit based off PR feedback that this change should only affect the parsing of sergments and node encoding of the url

fixup! revert "fixup! fix(router): fix = not parsed in router segment…

d9b8e69

… name"

DW8Reaper force-pushed the router-equals branch from 7bf9688 to d9b8e69 Compare April 8, 2023 03:20

DW8Reaper commented Apr 8, 2023

View reviewed changes

packages/router/test/url_serializer.spec.ts Show resolved Hide resolved

michaelgriscom approved these changes Apr 10, 2023

View reviewed changes

atscott approved these changes Apr 10, 2023

View reviewed changes

atscott added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: global presubmit The PR is in need of a google3 global presubmit labels Apr 10, 2023

AndrewKushnir removed request for AndrewKushnir and pkozlowski-opensource April 11, 2023 15:02

AndrewKushnir added target: rc This PR is targeted for the next release-candidate and removed target: major This PR is targeted for the next major release labels Apr 11, 2023

AndrewKushnir closed this in 748c33c Apr 11, 2023

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 12, 2023

Conversation

DW8Reaper commented Sep 2, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

DW8Reaper commented Sep 7, 2022

Uh oh!

pkozlowski-opensource commented Sep 12, 2022

Uh oh!

atscott commented Sep 19, 2022

Uh oh!

DW8Reaper commented Sep 19, 2022

Uh oh!

atscott commented Sep 19, 2022

Uh oh!

DW8Reaper commented Sep 19, 2022

Uh oh!

atscott commented Sep 19, 2022

Uh oh!

DW8Reaper commented Oct 2, 2022

Uh oh!

DW8Reaper commented Oct 17, 2022

Uh oh!

atscott commented Oct 17, 2022

Uh oh!

michaelgriscom Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelgriscom Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

DW8Reaper Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

michaelgriscom Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

DW8Reaper Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelgriscom left a comment

Choose a reason for hiding this comment

Uh oh!

atscott commented Apr 10, 2023

Uh oh!

ngbot bot commented Apr 10, 2023

Uh oh!

atscott commented Apr 10, 2023

Uh oh!

AndrewKushnir commented Apr 11, 2023

Uh oh!

angular-automatic-lock-bot bot commented May 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants