- Issue created by @tm01xx
- 🇳🇱Netherlands groendijk
Maybe consider not only look at paths but somewhere you can define roles in which case other rules are applied. Most easy fix would be to just add unsafe inline when a user role is in the defined roles.
- 🇳🇿New Zealand jweowu
You can use
hook_seckit_options_alter()to modify the settings however you wish based on any arbitrary criteria, certainly including "this is an admin path". For the Drupal 8+ versions, you'll need ✨ Provide hook_seckit_options_alter() D8 Needs review to provide the hook. - 🇳🇱Netherlands groendijk
Nice, Thanks @jweowu! Think this issue can be closed now? Agree @tm01xx?
- First commit to issue fork.
- 🇨🇦Canada gurpreet_chahal
We had a similar requirement at our agency. I've added the following patch and it works to ignore /admin routes.
Although, I believe it's not an ideal solution to ignore /admin paths from being ignored from CSP checks, but since one might be dealing with multiple modules and tons on inline scripts, it would still act as a shortcut/hack to ignore the CSP violations for admin.diff --git a/config/install/seckit.settings.yml b/config/install/seckit.settings.yml index 89278fb8d..9628b99a0 100644 --- a/config/install/seckit.settings.yml +++ b/config/install/seckit.settings.yml @@ -52,3 +52,4 @@ seckit_various: referrer_policy: FALSE referrer_policy_policy: 'no-referrer-when-downgrade' disable_autocomplete: FALSE + csp_disable_for_authenticated_users: FALSE diff --git a/config/schema/seckit.schema.yml b/config/schema/seckit.schema.yml index a51f2163b..3e27e53f7 100644 --- a/config/schema/seckit.schema.yml +++ b/config/schema/seckit.schema.yml @@ -177,3 +177,7 @@ seckit.settings: disable_autocomplete: type: boolean label: 'Disable autocomplete' + csp_disable_for_authenticated_users: + type: boolean + label: 'Disable CSP for Authenticated users' + diff --git a/src/EventSubscriber/SecKitEventSubscriber.php b/src/EventSubscriber/SecKitEventSubscriber.php index 72ef923f5..8fe253bcc 100644 --- a/src/EventSubscriber/SecKitEventSubscriber.php +++ b/src/EventSubscriber/SecKitEventSubscriber.php @@ -214,6 +214,7 @@ public function seckitCsp() { $csp_report_uri = $this->config->get('seckit_xss.csp.report-uri'); $csp_upgrade_req = $this->config->get('seckit_xss.csp.upgrade-req'); $add_nonce = $this->config->get('seckit_xss.csp.nonce'); + $csp_disable_for_authenticated_users = $this->config->get('seckit_various.csp_disable_for_authenticated_users'); // $csp_policy_uri = $this->config->get('seckit_xss.csp.policy-uri'); // Prepare directives. $directives = []; @@ -280,6 +281,9 @@ public function seckitCsp() { // } // send HTTP response header if directives were prepared. if ($directives) { + if ($csp_disable_for_authenticated_users && \Drupal::currentUser()->isAuthenticated()) { + return; + } if ($csp_report_only) { // Use report-only mode. $this->response->headers->set('Content-Security-Policy-Report-Only', $directives); diff --git a/src/Form/SecKitSettingsForm.php b/src/Form/SecKitSettingsForm.php index cda4f02a7..9c3c84101 100644 --- a/src/Form/SecKitSettingsForm.php +++ b/src/Form/SecKitSettingsForm.php @@ -731,6 +731,14 @@ public function buildForm(array $form, FormStateInterface $form_state) { '#description' => $this->t('Prevent the browser from populating login/registration form fields using its autocomplete functionality. This as populated fields may contain sensitive information, facilitating unauthorized access.'), ]; + // Ignore for authenticated users + $form['seckit_various']['csp_disable_for_authenticated_users'] = [ + '#type' => 'checkbox', + '#default_value' => $config->get('seckit_various.csp_disable_for_authenticated_users'), + '#title' => 'Disable CSP directives for logged-in users', + '#description' => $this->t("Disable for authenticated users"), + ]; + return parent::buildForm($form, $form_state); } - 🇳🇿New Zealand jweowu
> I've added the following patch and it works to ignore /admin routes.
It seems to be ignoring authenticated users, rather than just /admin routes.
In any case, I would again recommend using ✨ Provide hook_seckit_options_alter() D8 Needs review to implement conditional behaviour. (That feature should be merged at some point.)
- 🇨🇦Canada gurpreet_chahal
Plz find the updated patch, the earlier patch was dependent on the nonce_patch patch, i.e. https://www.drupal.org/files/issues/2023-06-19/3245008.patch →
The updated patch works with a clean version of the module, without any other installed patches.diff --git a/config/install/seckit.settings.yml b/config/install/seckit.settings.yml index a0595773a..8e4db5b82 100644 --- a/config/install/seckit.settings.yml +++ b/config/install/seckit.settings.yml @@ -48,3 +48,4 @@ seckit_various: referrer_policy: FALSE referrer_policy_policy: 'no-referrer-when-downgrade' disable_autocomplete: FALSE + csp_disable_for_authenticated_users: FALSE \ No newline at end of file diff --git a/config/schema/seckit.schema.yml b/config/schema/seckit.schema.yml index d0ef25bb4..62f8366a7 100644 --- a/config/schema/seckit.schema.yml +++ b/config/schema/seckit.schema.yml @@ -162,3 +162,6 @@ seckit.settings: disable_autocomplete: type: boolean label: 'Disable autocomplete' + csp_disable_for_authenticated_users: + type: boolean + label: 'Disable CSP for authenticated users' diff --git a/src/EventSubscriber/SecKitEventSubscriber.php b/src/EventSubscriber/SecKitEventSubscriber.php index 8870d1abc..1f5b59d67 100644 --- a/src/EventSubscriber/SecKitEventSubscriber.php +++ b/src/EventSubscriber/SecKitEventSubscriber.php @@ -223,6 +223,7 @@ public function seckitCsp() { $csp_connect_src = $this->config->get('seckit_xss.csp.connect-src'); $csp_report_uri = $this->config->get('seckit_xss.csp.report-uri'); $csp_upgrade_req = $this->config->get('seckit_xss.csp.upgrade-req'); + $csp_disable_for_authenticated_users = $this->config->get('seckit_various.csp_disable_for_authenticated_users'); // $csp_policy_uri = $this->config->get('seckit_xss.csp.policy-uri'); // Prepare directives. $directives = []; @@ -285,6 +286,9 @@ public function seckitCsp() { // } // send HTTP response header if directives were prepared. if ($directives) { + if ($csp_disable_for_authenticated_users && \Drupal::currentUser()->isAuthenticated()) { + return; + } if ($csp_report_only) { // Use report-only mode. $this->response->headers->set('Content-Security-Policy-Report-Only', $directives); diff --git a/src/Form/SecKitSettingsForm.php b/src/Form/SecKitSettingsForm.php index e334b0fb5..b7c8f27be 100644 --- a/src/Form/SecKitSettingsForm.php +++ b/src/Form/SecKitSettingsForm.php @@ -743,6 +743,15 @@ public function buildForm(array $form, FormStateInterface $form_state) { '#description' => $this->t('Prevent the browser from populating login/registration form fields using its autocomplete functionality. This as populated fields may contain sensitive information, facilitating unauthorized access.'), ]; + // Ignore for authenticated users + $form['seckit_various']['csp_disable_for_authenticated_users'] = [ + '#type' => 'checkbox', + '#default_value' => $config->get('seckit_various.csp_disable_for_authenticated_users'), + '#title' => 'Disable CSP directives for logged-in users', + '#description' => $this->t("Disable for authenticated users"), + ]; + + return parent::buildForm($form, $form_state); } - 🇳🇿New Zealand jweowu
I'm inclined to close this as "won't fix" in favour of ✨ Provide hook_seckit_options_alter() D8 Needs review being the general/recommended approach to conditional settings.
And I think if we did want a specific
csp_disable_for_authenticated_usersfeature, we'd want it to be a permission so that it could not only be applied to authenticated users, but trivially also to any other more specific role(s), without future scope creep to support that.What do others think?
- 🇳🇱Netherlands groendijk
I agree @jweowu. Allowing to disable CSP for all authenticated users creates an invisible security gap — one that's easy to overlook and hard to audit if a extra role is added in the future. If it has to be added, it must be with specific role(s).
We've implemented the "Disable-CSP-for-specific-roles-option" for some clients with the hook. So I guess it could be a nice-to-have in the future.
- 🇳🇿New Zealand jweowu
Ok, closing.
If anyone wishes to pursue this as a new feature, then a permission-based approach is my suggestion. Feel free to re-open this issue (or make a new one) if you intend to implement that approach.
Otherwise ✨ Provide hook_seckit_options_alter() D8 Needs review can be used for any conditional behaviour.