search

Brightspace / D2L.Security.OAuth2

Brightspace OAuth 2.0 for C#
Apache License 2.0
7 stars 16 forks source link

US75330 - adding check for tokens with parts that are non-Base64Url encoded strings. #57

Closed neverendingqs closed 8 years ago

neverendingqs commented 8 years ago

Workaround until https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/482 is resolved.

omsmith commented 8 years ago

Is this something we actually ran into?

neverendingqs commented 8 years ago

Is this something we actually ran into?

Ran into it while testing changes for https://git.dev.d2l/projects/CORE/repos/lp/pull-requests/2978/diff#framework/web/D2L.LP.Web/Rest/Errors/ExceptionHandlers/OAuth2ValidationErrorHandler.cs

It was returning 500 on bad access tokens, and we wanted to change that to return 401. We did it by handling ValidationExceptions, but this issue causes ArgumentExceptions, which means we were still getting 500's for certain badly formed tokens.

omsmith commented 8 years ago

So, yeah I knew this was a deficiency of the CanReadToken (that it wouldn't catch invalid parts), but most libraries are the same way. Similarly, if the base64 isn't representing valid json it would also proceed to fail despite these checks saying it should be good.

This is now just re-doing some of the work (decoding and not using the output) that's going to be done over again in ReadToken. So, if anything I might suggest just not using JwtSecurityTokenHandler in this case if we're looking to handle these cases more specifically.

j3parker commented 8 years ago

I'm not really worried about this error case because it's very obscure. This is a good example of test-driven-design leading to more complicated code with no pay-off. That being said, it degrades gracefully and there is a link in the code to the GitHub issue and its pretty obvious for how to test if this is still relevant so I'm not opposed to it being merged.

neverendingqs commented 8 years ago

The original reason for changing this was a UX concern (500 on certain malformed access tokens). After another discussion with the PO, we're okay if this case (non-Base64Url encoded strings) isn't covered.

Since we're adding complexity with little gain, I'm opting to just close this PR without merging unless there are any concerns.

j3parker commented 8 years ago

Yeah, it's hard to imagine why this would happen in practice. If JwtSecurityTokenHandlers behaviour changes we'll get it covered for free anyway.

brentschmaltz commented 8 years ago

@neverendingqs @j3parker nice catch. We will touch this up in our next minor release.