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
Respect openid_connect_content_security_form_action_enabled configuration only on client-side redirects #10603
Respect openid_connect_content_security_form_action_enabled configuration only on client-side redirects #10603
Conversation
…tion on client-side redirects changelog: Internal, OpenID Connect, Respect openid_connect_content_security_form_action_enabled configuration on client-side redirects
27b900e
to
7543202
Compare
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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like this suggestion, and honestly i think that name is fine? it's more clear what's happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a method and shuffled things a bit into a concern in 1e09a8a
3ee3e6c
to
febf556
Compare
febf556
to
1e09a8a
Compare
🛠 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_enabled
will 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_enabled
is 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.