Respect openid_connect_content_security_form_action_enabled configuration only on client-side redirects#10603
Merged
mitchellhenke merged 2 commits intomainfrom May 13, 2024
Conversation
…tion on client-side redirects changelog: Internal, OpenID Connect, Respect openid_connect_content_security_form_action_enabled configuration on client-side redirects
mitchellhenke
force-pushed
the
mitchellhenke/oidc-js-redirect-csp-respect
branch
from
27b900e to
7543202
Compare
zachmargolis
approved these changes
May 10, 2024
Sgtpluck
approved these changes
May 10, 2024
Comment on lines
+12
to
+16
| return if !IdentityConfig.store.openid_connect_content_security_form_action_enabled && | ||
| oidc_redirect_method( | ||
| issuer: authorize_form.service_provider.issuer, | ||
| user_uuid: current_user&.uuid, | ||
| ) != 'server_side' |
Contributor
There was a problem hiding this comment.
this is a really long line and it's repeated, what if we made an named helper for this (with a better name than I am proposing)
Suggested change
| return if !IdentityConfig.store.openid_connect_content_security_form_action_enabled && | |
| oidc_redirect_method( | |
| issuer: authorize_form.service_provider.issuer, | |
| user_uuid: current_user&.uuid, | |
| ) != 'server_side' | |
| return if csp_disabled_and_not_server_side? |
Contributor
There was a problem hiding this comment.
i like this suggestion, and honestly i think that name is fine? it's more clear what's happening
Contributor
Author
There was a problem hiding this comment.
Added a method and shuffled things a bit into a concern in 1e09a8a
mitchellhenke
force-pushed
the
mitchellhenke/oidc-js-redirect-csp-respect
branch
from
3ee3e6c to
febf556
Compare
mitchellhenke
force-pushed
the
mitchellhenke/oidc-js-redirect-csp-respect
branch
from
febf556 to
1e09a8a
Compare
zachmargolis
approved these changes
May 13, 2024
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
🛠 Summary of changes
Following on the long history of #10036, #9790, #9755, #9669, this PR intends to align the behavior of the overly configurable OIDC redirect behavior with
openid_connect_content_security_form_action_enabled.Currently,
openid_connect_content_security_form_action_enabledwill enable/disable entirely without respect for whether it is needed. This is because the original implementation intended to cut over all redirects to client-side, and then later disable and remove the form-action Content Security Policy (CSP). This is no longer feasible in the near-term. The changes here intend to disable the form-action CSP if and only ifopenid_connect_content_security_form_action_enabledis disabled and the OIDC redirect method is notserver_side. This will allow us to remove the form-action CSP behavior more selectively while maintaining backwards compatibility with service providers that are not able to move to client-side redirects at this point.A more detailed explanation is available here and here.