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

Implement RFC 9207 #1213

Open
bellebaum opened this issue Mar 22, 2022 · 8 comments
Open

Implement RFC 9207 #1213

bellebaum opened this issue Mar 22, 2022 · 8 comments
Labels
feature-request Improvements and additions to the library.

Comments

@bellebaum
Copy link

Recently, a new OAuth response parameter was defined in RFC 9207: iss

The basic idea is that if a server advertises authorization_response_iss_parameter_supported as true in its metadata (or we know of support via explicit configuration), the client should only accept the response if iss matches the server's issuer identifier.

Furthermore: This Client clears several OAuth/OpenID response parameters after login (e.g. code or state).
The following code should clear iss as well:

if (!options.preventClearHashAfterLogin) {
const href =
location.origin +
location.pathname +
location.search
.replace(/code=[^&\$]*/, '')
.replace(/scope=[^&\$]*/, '')
.replace(/state=[^&\$]*/, '')
.replace(/session_state=[^&\$]*/, '')
.replace(/^\?&/, '?')
.replace(/&$/, '')
.replace(/^\?$/, '')
.replace(/&+/g, '&')
.replace(/\?&/, '?')
.replace(/\?$/, '') +
location.hash;
history.replaceState(null, window.name, href);
}

At a minimum, this should free sites of having to manually clear the iss when using compliant servers. When properly implemented, some sites might even benefit from the mix-up countermeasure.

@jeroenheijmans jeroenheijmans added the feature-request Improvements and additions to the library. label Mar 23, 2022
@jeroenheijmans
Copy link
Collaborator

Interesting! Looking at the RFC it's currently "Proposed Standard" so even though not fully mature still good enough to already consider implementing it. Tagged your suggestion accordingly, as a feature-request.

@buchatsky
Copy link

buchatsky commented May 2, 2023

I've created the PR to fix this. The only thing that I'm not sure is reporting the error:

  1. What should be the argument of Promise.reject() - string or OAuthErrorEvent? (Usually in Angular the HttpErrorResponse type is used for remote errors and simple string - for local errors, so I return the string. It is also one if() case less in my error handler)
  2. Should the two error cases
    (this.authorizationResponseIssParameterSupported && iss !== this.issuer)
    (!this.authorizationResponseIssParameterSupported && iss)
    return the same or different messages? (I return the same message to simplify the code)

@bellebaum
Copy link
Author

I am unsure about the first question, but for the second I think it might make sense to not throw any error at all if we do not know of support for the spec, since an Authorization Server might throw all kinds of other parameters in there, and they may not be aware that a new spec suddenly popped up defining a new parameter in the IANA Registry.

Thanks for implementing this 👍

@buchatsky
Copy link

@bellebaum do you mean throwing an error in this case: (!this.authorizationResponseIssParameterSupported && iss) ?
RFC says:

Clients SHOULD discard authorization responses with the iss parameter from authorization servers that do not indicate their support for the parameter. However, there might be legitimate authorization servers that provide the iss parameter without indicating their support in their metadata. Local policy or configuration can determine whether to accept such responses

So, if authorization_response_iss_parameter_supported is not supplied in AS metadata but the 'iss' param returned by AS is really an issuer id, this can be fixed with setting OAuthService.authorizationResponseIssParameterSupported = true after loading a discovery document.
If 'iss' param is returned and is anything but an issuer id, issuer verification can be turned off by setting AuthConfig.skipIssuerCheck = true (but in this case verifying issuer in id_token claim will be also skipped).

@bellebaum
Copy link
Author

Not sure why the RFC says that, but I can live with it :)

@masumulu28
Copy link

Hi, what's the status of this issue ? Is this gonna merge ?

@bjpe
Copy link

bjpe commented Apr 25, 2024

Hi @manfredsteyer , are there any plans to fix this issue?

@olegdunkan
Copy link

Workaround for those who use Keycloak OpenId Provider
Turn on Exclude Issuer From Authentication Response in clients advance settings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Improvements and additions to the library.
Projects
None yet
Development

No branches or pull requests

6 participants