fix: make require_state skip verification of state #181
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stanhu
force-pushed
the
sh-fix-require-state
branch
from
a8678e0 to
328d87a
Compare
In #127, `require_state` was introduced because according to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` During review, the `require_state` parameter was modified to verify `state` as long as `stored_state` was present. However, `stored_state` always holds at least a random value, so when `require_state` were `false` and if an OpenID provider did not relay the `state` value, authentication would halt with a "Invalid 'state' parameter" error. This commit updates it so that if `require_state` is set to `false`, the `state` parameter is never checked at all.
stanhu
force-pushed
the
sh-fix-require-state
branch
from
328d87a to
58a6d80
Compare
bufferoverflow
approved these changes
Jun 28, 2024
There was a problem hiding this comment.
Thanks @stanhu , especially for the specification reference.
amessinger
approved these changes
Jun 28, 2024
BobbyMcWho
approved these changes
Jun 28, 2024
stanhu
added a commit
that referenced
this pull request
Jul 3, 2024
According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` In #181 we attempted to make `require_state` skip the `state` verification if it were `true`, but this was reverted for two reasons: 1. If identity providers make direct requests to the callback phase with a valid token, no `state` is passed in the request, so `require_state` to fail the request would break the existing flow. 2. If `state` isn't used, it shouldn't be sent in the first place. `send_state` will now disable the sending of a `state` in the authorize phase.
stanhu
added a commit
that referenced
this pull request
Jul 3, 2024
This reverts #181 and adds a `send_state` parameter instead to address #174. According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` In #181 we attempted to make `require_state` skip the `state` verification if it were `true`, but this was reverted for two reasons: 1. If identity providers make direct requests to the callback phase with a valid token, no `state` is passed in the request, so `require_state` to fail the request would break the existing flow. 2. If `state` isn't used, it shouldn't be sent in the first place. `send_state` will now disable the sending of a `state` in the authorize phase.
stanhu
added a commit
that referenced
this pull request
Jul 3, 2024
This reverts #181 and adds a `send_state` parameter instead to address #174. According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` In #181 we attempted to make `require_state` skip the `state` verification if it were `true`, but this was reverted for two reasons: 1. If identity providers make direct requests to the callback phase with a valid token, no `state` is passed in the request. If `require_state` were `true`, this change fails the request and breaks existing flows. 2. If `state` isn't sent in the first place, it should not be verified. `send_state` will now disable the sending of a `state` in the authorize phase.
stanhu
added a commit
that referenced
this pull request
Jul 3, 2024
This reverts #181 and adds a `send_state` parameter instead to address #174. According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` In #181 we attempted to make `require_state` skip the `state` verification if it were `true`, but this was reverted for two reasons: 1. If identity providers make direct requests to the callback phase with a valid token, no `state` is passed in the request. If `require_state` were `true`, this change fails the request and breaks existing flows. 2. If `state` isn't sent in the first place, it should not be verified. `send_state` will now disable the sending of a `state` in the authorize phase.
stanhu
added a commit
that referenced
this pull request
Jul 3, 2024
This reverts #181 and adds a `send_state` parameter instead to address #174. According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1, `state` is recommended but not required: ``` state RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. ``` In #181 we attempted to make `require_state` skip the `state` verification if it were `true`, but this was reverted for two reasons: 1. If identity providers make direct requests to the callback phase with a valid token, no `state` is passed in the request. If `require_state` were `true`, this change fails the request and breaks existing flows. 2. If `state` isn't sent in the first place, it should not be verified. `send_state` will now disable the sending of a `state` in the authorize phase.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #127,
require_statewas introduced because according to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1,stateis recommended but not required:During review, the
require_stateparameter was modified to verifystateas long asstored_statewas present. However,stored_statealways holds at least a random value, so whenrequire_statewerefalseand if an OpenID provider did not relay thestatevalue, authentication would halt with a "Invalid 'state' parameter" error.This commit updates it so that if
require_stateis set tofalse, thestateparameter is never checked at all.Closes #174