panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

fix at_hash validation for servers that provide null at_hash at refresh #532

Closed terion-name closed 2 years ago

terion-name commented 2 years ago

Some openid servers (namely Authentik) don't provide at_hash on refresh and instead of dropping the key from payload — are setting it to null.

Example of idToken upon refresh from authentik:

{
  iss: 'https://auth.domain.local/application/o/profiler/',
  sub: '49127....,
  aud: 'c503...,
  exp: 1667651121,
  iat: 1665059121,
  auth_time: 1664880227,
  acr: '[goauthentik.io/providers/oauth2/default](http://goauthentik.io/providers/oauth2/default)',
  c_hash: null,
  nonce: null,
  at_hash: null, // <----- problem
  email: '...',
  email_verified: true,
  name: '...',
  given_name: '...',
  family_name: '',
  preferred_username: '...',
  nickname: '...',
  groups: [ '...']
}

This is breaking validation. This commit ads null check

terion-name commented 2 years ago

It is perfectly fine if they drop the hash. But the conform behaviour is to omit the claim entirely, not setting it to null.

Adding workarounds for non-confirmity is not something openid-client generally does.

Where is it stated explicitly that at_hash should be dropped at all, not null?

Authentik is kinda one of major identity providers out there, do you think they failed the spec conform and an issue should be opened there?

To clarify: this happens only on refresh. Auth response contains at_hash, refresh — doesn't

panva commented 2 years ago

Where is it stated explicitly that at_hash should be dropped at all, not null?

Its definition says it's optional and that the at_hash value is a case sensitive string. There is no optionality that would allow null which is an explicit JSON type / value.

do you think they failed the spec conform and an issue should be opened there?

yes.

terion-name commented 2 years ago

@panva they commited a fix for this, thanx