improve: Also pass along a state (nonce) paramater. #1135
Conversation
bug: getodk#1134 It's not needed for CSRF protection because code_challenge does this, but the Authentik IDP will return &state= in the query string. This will trigger node-openid-client to fail a check.
openbrian
force-pushed
the
oidc-state-parameter
branch
from
06763fe
to
6776bab
Compare
lib/resources/oidc.js
Outdated
| @@ -102,15 +103,19 @@ module.exports = (service, endpoint) => { | |||
| const client = await getClient(); | |||
| const code_verifier = generators.codeVerifier(); // eslint-disable-line camelcase | |||
|
|
|||
| // State is not strictly required because PKCS (code_challenge) protects us from CSRF attacks. | |||
There was a problem hiding this comment.
It feels important to then mention why it's there! Maybe just add "but some IDPs always send it"
|
Thanks for digging into this and figuring out a solution, @openbrian! I've left one small suggestion. @alxndrsn could you please do a deeper review? |
| @@ -102,15 +103,22 @@ module.exports = (service, endpoint) => { | |||
| const client = await getClient(); | |||
| const code_verifier = generators.codeVerifier(); // eslint-disable-line camelcase | |||
|
|
|||
| // State is not strictly required because PKCS (code_challenge) protects us from CSRF attacks. | |||
| // See https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#name-countermeasures-4 | |||
| // We create a nonce (state) anyway because some IDPs (e.g. Authentik) will send the nonce | |||
There was a problem hiding this comment.
nonce is the name of a different value which can be used at various points in OAuth/OIDC. I think avoiding that word here would reduce chance of confusion.
There was a problem hiding this comment.
@openbrian just in case you missed this 🙂
Thanks for the PR! I think this is a bug in Authentik's implementation; they seem to be open source (https://github.com/goauthentik/authentik?) so perhaps it can be fixed on their side. That said, |
I've opened a potential PR at goauthentik/authentik#9735 |
bug: #1134
It's not needed for CSRF protection because code_challenge does this, but the Authentik IDP will return &state= in the query string. This will trigger node-openid-client to fail a check.
Closes #1134
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass OR confirm CircleCI build passesTesting the oidc way failed for me. I mean the tests would not kick off.