Skip to content
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

Add support for multiline urls (with "%0D%0A" in them) #3432

Closed
1 task done
DmitryGonchar opened this issue May 10, 2017 · 4 comments · Fixed by ui-router/core#61
Closed
1 task done

Add support for multiline urls (with "%0D%0A" in them) #3432

DmitryGonchar opened this issue May 10, 2017 · 4 comments · Fixed by ui-router/core#61

Comments

@DmitryGonchar
Copy link

DmitryGonchar commented May 10, 2017

This is a:

  • Feature Request

My version of UI-Router is: 0.4.2

Feature Request

Problem description

This feature request is based on a real problem in a web app. I have already made it work in a fork and can make a PR which closes this issue.

We have "Login with Active Directory" functionality, login is done on a Microsoft website, and in case of an error, it redirects to the url like https://www.myapp.com/#/error=access_denied&error_description=AADSTS65005:+The+app+needs+access+to+a+service+(%22https://myapp.com/fdd80187-f97c-496d-9266-606316779c58%22)+that+your+organization+%22mstnd206987.onmicrosoft.com%22+has+not+subscribed+to+or+enabled.+Contact+your+IT+Admin+to+review+the+configuration+of+your+service+subscriptions.%0D%0ACorrelation+ID:+6c8ff940-b412-4519-a034-def6d0ba5adb%0D%0ATimestamp:+2017-05-05+11:49:04Z&state=f9c70076-af69-4f7f-8f87-7fc9f0d57922

We have a route/state for it in config, and it worked with ngRoute (it matched this url to template and controller, and the page was shown). But after we switched to ui-router, controller was not called, template's html was not requested from the server and no page was shown, page was completely blank.

Reason

The part of the url which causes problem is this one %0D%0ACorrelation+ID:..., or more specifically symbols %0D%0A. If I remove them from the url - everything works fine. These symbols are encoded "CRLF", so basically a new line.

I went to the source code and debugged why such urls are not matched to anything (tested with catch all route /error=*adError). The reason is that multiline urls are not matched (skipped) without multiline 'm' flag (because ui-router adds $ at the end of the regexp). If multiline flag is set - it works. But ui-router does not allow to set it via config.

Code lines for reference are:

function UrlMatcher(...) {
...
this.regexp = new RegExp(compiled, config.caseInsensitive ? 'i' : undefined);

Possible solution

Now, there are several ways to add multiline urls support:
a) allow to set it globally, as a "caseInsensitive" option you already have. I am not sure if it can break any other use cases - this is a question to you.
b) set it per state. This is how I did it for my project

.state({
 name: ,
 url: ,
 multilineUrl: true
});

Maybe we should allow both options.

So, the questions are:

  1. Should I send a PR for this?

  2. which option a) or b) should be done?

@christopherthielen
Copy link
Contributor

Thanks for the well described issue report.
Can you think of any way that enabling multiline matching would break matching in other apps?

@DmitryGonchar
Copy link
Author

DmitryGonchar commented May 27, 2017

@christopherthielen Well, it should not affect one line urls - their functioning should stay the same (as long as the global flag is not used, but it seems not to be used anywhere in the component).

The only difference I can think of is multiline url where only the first line should be matched, but cannot think of a real world example where this could be needed.

Actually, the problem seems to be not in the multiline flag missing, but in the "catch all" syntax.
Its regexp is .*, which does not match \n and \r. They should be included, so it can be [\s\S]* instead (to match any character), or (.|\n|\r)* at least.

I have checked - url and search params are calculated correctly if %0D%0A is inside param value. So, only catch all was not working.

@DmitryGonchar
Copy link
Author

@christopherthielen Should I send the PR? Works well in our app

@christopherthielen
Copy link
Contributor

Yes, please send a PR. The PR does not need a toggle. Let's process multi line by default and if people complain, we can add a toggle later.

DmitryGonchar added a commit to DmitryGonchar/core that referenced this issue Jun 16, 2017
urls with \r or \n symbols in them (%0D or %0A when encoded) are matched with catch-all syntax

close angular-ui/ui-router#3432
DmitryGonchar added a commit to DmitryGonchar/core that referenced this issue Jun 16, 2017
urls with \r or \n symbols in them (%0D or %0A when encoded) are matched with catch-all syntax

close angular-ui/ui-router#3432
christopherthielen pushed a commit to ui-router/core that referenced this issue Jun 16, 2017
urls with \r or \n symbols in them (%0D or %0A when encoded) are matched with catch-all syntax

closes angular-ui/ui-router#3432
wawyed pushed a commit to wawyed/core that referenced this issue Mar 3, 2018
urls with \r or \n symbols in them (%0D or %0A when encoded) are matched with catch-all syntax

closes angular-ui/ui-router#3432
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants