Add anchors to valid chars in a redirect_uri

[orenmazor]

@thedrow @bjmc Hey guys, looks like there are some problems validating redirect_uris that have anchors in unexpected places (eg angular uris).

I extended the regex and added a really basic test.

thoughts?

[bjmc]

Why are the requirements expanding as a result of this?

[orenmazor]

I ran the test suite and it was failing because these guys were missing

[bjmc]

IMHO, requirements that are only needed for tests should go in a separate test_requirements.txt file, not part of the main requirements.txt

[orenmazor]

ahh didn't see the test_requirements. totally. fixing.

[bjmc]

Can this be parameterized to be several different test cases, rather than one test that makes multiple assertions?

[orenmazor]

totally. there were zero tests so I wasn't sure what strategy you guys would prefer, so I went with the laziest one. I'll split these guys up.

what we should do is get some kind of better list of good/bad urls and have them iterated on.

[kevin-brown]

What's accepted by pchar is formally defined in RFC 3986, Section 3.3 and it doesn't contain # or ?. This is because pchar defines what characters are allowed in the path, and the # defines the start of the fragment.

The problem that you're running into is that the fragment is not formally defined as a part of the absolute URI, This should not normally be a problem, since the redirect_uri must be a "URI" and not an "Absolute URI" per the specification, but oauthlib is validating it as an absolute URI.

But oauthlib is actually doing it correctly, since there is a note in RFC 6749, Section 3.1.2 which defines acceptable redirection endpoints, which explicitly says

The redirection endpoint URI MUST be an absolute URI as defined by [RFC3986] Section 4.3.

And later

The endpoint URI MUST NOT include a fragment component.

So, there is an inconsistency within the specification, and oauthlib opted to lean on the safer side. After looking through the errata, I don't believe it's been reported.

[bjmc]

You might check out this document, "The correct use of the state parameter in OAuth 2" by one of the authors of the RFC. It suggests encoding extra parameters your application needs into the state parameter that gets round-tripped during an OAuth2 authorization flow. Maybe that would be appropriate for your use-case?

[orenmazor]

@bjmc I feel like it's wrong to use the state param as a generic bucket when it should be encourated to be used for security (i.e. csrf style). but also this is a uri issue and not extra parameters type of thing.

@kevin-brown your comments make sense. but the long and the short of it is that angular uses these fragments for routing. as far as some more modern frameworks are concerned, routing requests via fragments is acceptable. which means that if I use oauthlib in my authentication server, people who use angular can't authenticate it.

so the question becomes, do we want this to be strictly adhering or do we allow a superset of functionality?

[bjmc]

@orenmazor It's not an either-or question. You can use state for csrf security and also encode extra information in the parameter. For example, a signed JWT that contains a nonce could also contain other information. If one of the spec authors is specifically saying it's intended to be used that way, I don't think it's wrong.

I don't know very much about Angular, but it seems like it can be configured to work with URLs without a fragment identifier?

[kevin-brown]

so the question becomes, do we want this to be strictly adhering or do we allow a superset of functionality?

I can't speak for the project owners, but I will just leave the description here.

A generic, spec-compliant, thorough implementation of the OAuth request-signing logic for python

[orenmazor]

@bjmc ah thats true. I kind of always wanted to keep state as small as possible but I guess nothing stops somebody from throwing as much of Genesis as you can fit in there :)

I don't know much about angular either, but as far as it's concerned, those are valid urls. I don't know that I can dictate to people who integrate with us how they should structure their routing, to be honest (tho I'd definitely like to)

[bjmc]

I obviously don't know the specifics of your situation with your users, but I think you'd be on pretty firm ground to say "The RFC says it has to be this way..."

FWIW, it seems like the practice in Angular-land is to have one page that's just responsible for handling OAuth2 callbacks, and then that will redirect back into the main application. That client-side redirect could be driven off something encoded in the state if they wanted.

There may even be library support so the app devs don't have to implement this all themselves.

[orenmazor]

@bjmc totally. the only problem with that link is that its for implicit grants, which isn't what we're doing here :(

the only other solution than updating the regex to allow more complex urls is to rely on the default redirect uri feature in oauthlib. the downside to this is that I'm then taking away their ability to have multiple possible redirect uris.

tbh the regex solution smells the rightest, as the likelyhood of fragment urls going away anytime soon is basically 0. but if you guys are a strong no on this then I'll need to solve the problem some other way.

[bjmc]

What are they wanting to do in Angular that isn't the implicit grant? A public Javascript client can't protect its client_secret so in that case they'll have to have some kind of back-end component, anyway.

Modifying pchar to violate the spec smells wrong to me. I'm not one of the project owners so it's not my place to give a final decision, but I'm pretty iffy on it.

[orenmazor]

What are they wanting to do in Angular that isn't the implicit grant? A public Javascript client can't protect its client_secret so in that case they'll have to have some kind of back-end component, anyway.

thats a great point actually. I assumed they're just piping the request to a backend, but if thats the case then maybe the backend can be reached directly.

[thedrow]

I agree with @bjmc I don't think this PR should be accepted. I'd rather stick to the spec as best as I can in order to avoid security issues and/or compatibility issues with other clients.

[orenmazor]

👍 cool beans

…

On Fri, Mar 31, 2017 at 4:59 AM Omer Katz ***@***.***> wrote: Closed #469 <#469>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#469 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXdF8P3MhuTmX4Zf_agSYm5Ws10esYLks5rrMBzgaJpZM4MtSN3> .