Skip to content
This repository has been archived by the owner on Jan 23, 2024. It is now read-only.

Negative tests of "authorize" endpoint should include INVALID_AUD #155

Open
jmandel opened this issue Mar 2, 2019 · 3 comments
Open

Negative tests of "authorize" endpoint should include INVALID_AUD #155

jmandel opened this issue Mar 2, 2019 · 3 comments

Comments

@jmandel
Copy link
Contributor

jmandel commented Mar 2, 2019

I just realized this is a behavior the S4S Test Suite never checked for -- but it's important for API providers to get right to prevent certain attacks (i.e., a malicious server whose /metadata claims association with a legitimate authorization server).

@arscan
Copy link
Contributor

arscan commented Mar 3, 2019

Thanks Josh.

Is the behavior of the authorization server well-specified in standards in this error case? By my reading of the Error Response portion of OAuth 2.0 spec, it seems that a compliant behavior would be to redirect the client browser to the redirect URI (since it isn't an issue with the redirect URI or client id), with an error param that matches one of the legal values, and a state param.

Since the aud param was added by the SMART App Launch Framework spec from what I can tell (not seeing it in OAuth 2.0 spec), it isn't clear what that error code must be though. Looks like the SMART App Launcher passes error=bad_audience&error_description=Bad%20audience%20value, though that error value isn't technically valid according to the OAuth 2.0 spec. It is also missing the state parameter, which is problematic for Inferno as a client because that is how we reattach users to a running test session.

Did I get that right?

@jmandel
Copy link
Contributor Author

jmandel commented Mar 3, 2019

I think your assessment is correct and that the SMART launcher needs some work to ensure that it is compliant (and as far as the launcher goes, there are some intentionally strange behaviors, like we don't validate client secrets for example).

The key issue is to make sure that a server does not issue a token if a correct aud wasn't supplied.

@arscan
Copy link
Contributor

arscan commented Mar 4, 2019

Ok, I understand the issue. Will see how we can add a test for this. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants