Migrated CSP from 'seckit' to 'drupal/csp' with per-request nonce.#166
Migrated CSP from 'seckit' to 'drupal/csp' with per-request nonce.#166AlexSkrypnyk wants to merge 2 commits intodevelopfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds and enables the Drupal CSP module, defines default CSP policies and reporting, injects per-request CSP nonces into page attachments, adjusts environment-specific CSP behavior, and adds/updates tests to assert CSP header presence and nonce handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Drupal as Drupal App
participant CSP as CSP Module
participant Report as /report-csp-violation
rect rgba(220,230,250,0.5)
Browser->>Drupal: HTTP request (page)
end
rect rgba(220,250,220,0.5)
Drupal->>CSP: Request nonce & policy
CSP-->>Drupal: nonce + header value
Drupal->>Drupal: do_base_page_attachments merges nonce, attaches library
Drupal-->>Browser: HTTP response with Content-Security-Policy header (includes nonce)
end
rect rgba(250,240,220,0.5)
Browser->>Report: (on violation) POST violation report
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/default/csp.settings.yml`:
- Around line 24-27: The CSP currently permits inline styles via the style-src
flags entry containing "unsafe-inline"; update the policy by removing
"unsafe-inline" from the style-src flags and migrate to a nonce- or hash-based
approach: replace the flags entry with either a runtime-generated nonce token
(e.g., add a style-src "nonce-{nonce}" placeholder in the config) or with
specific style hashes, implement server-side nonce generation (inject the same
nonce into the CSP header and into any inline <style> or style attributes in
templates), and ensure the configuration keys referenced (style-src, base,
flags) are updated to emit the nonce/hash value so the CSP header and rendered
pages remain in sync.
- Line 3: The YAML key "directives" currently uses braces with extra spaces ("{
}"); update the value for the directives key to use a compact empty mapping by
replacing "{ }" with "{}" so the line reads "directives: {}" for consistent
formatting while keeping the same semantics.
- Around line 8-60: Add the missing CSP directives under the top-level
"directives" mapping: introduce a new "base-uri" directive with "base: self" and
a new "form-action" directive with "base: self" (or a more restrictive source
list if needed). Update the same structure used by existing directives (like
"default-src", "script-src", "style-src") so "base-uri: { base: self }" and
"form-action: { base: self }" appear alongside them to lock down allowed base
URLs and permitted form submission targets.
In `@tests/behat/features/csp.feature`:
- Around line 11-23: Add a regression Behat scenario that verifies CSP nonce
rotation by making two consecutive requests to the homepage and asserting the
"Content-Security-Policy" header contains a nonce on each request and that the
nonce values differ; reuse the existing steps ("Given I am an anonymous user" /
"Given I am logged in as a user with the \"administrator\" role", "When I go to
the homepage", "Then the response header \"Content-Security-Policy\" should
contain the value \"'nonce-\"") but extend the scenario to capture the full
nonce substring from the header on the first request, perform a second request,
capture the second nonce, and assert the two nonce strings are not equal to
ensure rotation for both anonymous and authenticated cases.
In `@web/modules/custom/do_base/do_base.module`:
- Around line 37-38: The current code overwrites any existing CSP nonce
contributions by assigning $attachments['#attached']['csp_nonce']['script']
directly; change it to merge/appended behavior so other hooks are
preserved—check/ensure $attachments['#attached']['csp_nonce']['script'] is an
array and append Csp::POLICY_UNSAFE_INLINE (e.g., via array_merge or []
assignment) instead of direct assignment, and still add the 'csp/nonce' library
to $attachments['#attached']['library'] so the nonce support remains registered.
In `@web/sites/default/includes/modules/settings.csp.php`:
- Around line 11-12: Update the misleading comment to accurately describe what
the code does: it does not disable CSP entirely but only disables the
"upgrade-insecure-requests" directive. Edit the comment above the
$config['csp.settings']['enforce']['directives']['upgrade-insecure-requests']
assignment so it states that only the upgrade-insecure-requests directive is
being turned off (e.g., because the site is not served over HTTPS), rather than
claiming CSP is disabled locally and in CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 42627c96-fa4f-49e1-ba12-7ebb0b3368e5
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
composer.jsonconfig/default/core.extension.ymlconfig/default/csp.settings.ymlconfig/default/seckit.settings.ymltests/behat/features/csp.featuretests/behat/features/seckit.featuretests/phpunit/Drupal/EnvironmentSettingsTest.phpweb/modules/custom/do_base/do_base.moduleweb/sites/default/includes/modules/settings.csp.php
| directives: | ||
| default-src: | ||
| base: self | ||
| script-src: | ||
| base: self | ||
| sources: | ||
| - https://www.googletagmanager.com | ||
| - https://www.gstatic.com | ||
| - https://www.recaptcha.net | ||
| - https://www.google.com | ||
| - https://cdnjs.cloudflare.com | ||
| - https://cdn.jsdelivr.net/gh/cferdinandi/tabby@12.0.3/dist/js/tabby.min.js | ||
| - https://unpkg.com/@popperjs/core@2.11.6/dist/umd/popper.js | ||
| - https://unpkg.com/tippy.js@6.3.7/dist/tippy.umd.js | ||
| object-src: | ||
| base: none | ||
| style-src: | ||
| base: self | ||
| flags: | ||
| - unsafe-inline | ||
| sources: | ||
| - https://cdnjs.cloudflare.com/ajax/libs/highlight.js/ | ||
| - https://fonts.googleapis.com/ | ||
| - https://cdn.jsdelivr.net/gh/cferdinandi/tabby@12.0.3/dist/css/tabby-ui.min.css | ||
| - https://cdnjs.cloudflare.com/ajax/libs/codemirror/5.65.12/codemirror.css | ||
| - https://unpkg.com/tippy.js@6.3.7/dist/tippy.css | ||
| - https://cdnjs.cloudflare.com/ajax/libs/select2/4.0.13/css/select2.min.css | ||
| img-src: | ||
| base: self | ||
| sources: | ||
| - 'data:' | ||
| media-src: | ||
| base: self | ||
| frame-src: | ||
| base: self | ||
| sources: | ||
| - https://www.youtube.com | ||
| - https://www.recaptcha.net | ||
| - https://www.google.com | ||
| frame-ancestors: | ||
| base: none | ||
| font-src: | ||
| base: self | ||
| sources: | ||
| - https://fonts.gstatic.com | ||
| connect-src: | ||
| base: self | ||
| sources: | ||
| - https://www.googletagmanager.com | ||
| - https://www.google-analytics.com | ||
| - https://www.recaptcha.net | ||
| - https://www.google.com | ||
| upgrade-insecure-requests: true |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add base-uri and form-action directives to strengthen CSP.
The CSP configuration is missing two important security directives:
-
base-uri: Controls allowed base URLs for relative URL resolution. Without this directive, an attacker could inject a
<base>tag to redirect relative URLs to malicious endpoints. Recommend addingbase-uri: { base: self }to restrict base URLs to the same origin. -
form-action: Restricts where forms can be submitted. Without this directive, forms could potentially be submitted to attacker-controlled endpoints. Recommend adding
form-action: { base: self }(or a more restrictive policy based on your requirements).
These directives provide defense-in-depth protection against base tag injection and form hijacking attacks.
🛡️ Proposed additions
connect-src:
base: self
sources:
- https://www.googletagmanager.com
- https://www.google-analytics.com
- https://www.recaptcha.net
- https://www.google.com
+ base-uri:
+ base: self
+ form-action:
+ base: self
upgrade-insecure-requests: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| directives: | |
| default-src: | |
| base: self | |
| script-src: | |
| base: self | |
| sources: | |
| - https://www.googletagmanager.com | |
| - https://www.gstatic.com | |
| - https://www.recaptcha.net | |
| - https://www.google.com | |
| - https://cdnjs.cloudflare.com | |
| - https://cdn.jsdelivr.net/gh/cferdinandi/tabby@12.0.3/dist/js/tabby.min.js | |
| - https://unpkg.com/@popperjs/core@2.11.6/dist/umd/popper.js | |
| - https://unpkg.com/tippy.js@6.3.7/dist/tippy.umd.js | |
| object-src: | |
| base: none | |
| style-src: | |
| base: self | |
| flags: | |
| - unsafe-inline | |
| sources: | |
| - https://cdnjs.cloudflare.com/ajax/libs/highlight.js/ | |
| - https://fonts.googleapis.com/ | |
| - https://cdn.jsdelivr.net/gh/cferdinandi/tabby@12.0.3/dist/css/tabby-ui.min.css | |
| - https://cdnjs.cloudflare.com/ajax/libs/codemirror/5.65.12/codemirror.css | |
| - https://unpkg.com/tippy.js@6.3.7/dist/tippy.css | |
| - https://cdnjs.cloudflare.com/ajax/libs/select2/4.0.13/css/select2.min.css | |
| img-src: | |
| base: self | |
| sources: | |
| - 'data:' | |
| media-src: | |
| base: self | |
| frame-src: | |
| base: self | |
| sources: | |
| - https://www.youtube.com | |
| - https://www.recaptcha.net | |
| - https://www.google.com | |
| frame-ancestors: | |
| base: none | |
| font-src: | |
| base: self | |
| sources: | |
| - https://fonts.gstatic.com | |
| connect-src: | |
| base: self | |
| sources: | |
| - https://www.googletagmanager.com | |
| - https://www.google-analytics.com | |
| - https://www.recaptcha.net | |
| - https://www.google.com | |
| upgrade-insecure-requests: true | |
| directives: | |
| default-src: | |
| base: self | |
| script-src: | |
| base: self | |
| sources: | |
| - https://www.googletagmanager.com | |
| - https://www.gstatic.com | |
| - https://www.recaptcha.net | |
| - https://www.google.com | |
| - https://cdnjs.cloudflare.com | |
| - https://cdn.jsdelivr.net/gh/cferdinandi/tabby@12.0.3/dist/js/tabby.min.js | |
| - https://unpkg.com/@popperjs/core@2.11.6/dist/umd/popper.js | |
| - https://unpkg.com/tippy.js@6.3.7/dist/tippy.umd.js | |
| object-src: | |
| base: none | |
| style-src: | |
| base: self | |
| flags: | |
| - unsafe-inline | |
| sources: | |
| - https://cdnjs.cloudflare.com/ajax/libs/highlight.js/ | |
| - https://fonts.googleapis.com/ | |
| - https://cdn.jsdelivr.net/gh/cferdinandi/tabby@12.0.3/dist/css/tabby-ui.min.css | |
| - https://cdnjs.cloudflare.com/ajax/libs/codemirror/5.65.12/codemirror.css | |
| - https://unpkg.com/tippy.js@6.3.7/dist/tippy.css | |
| - https://cdnjs.cloudflare.com/ajax/libs/select2/4.0.13/css/select2.min.css | |
| img-src: | |
| base: self | |
| sources: | |
| - 'data:' | |
| media-src: | |
| base: self | |
| frame-src: | |
| base: self | |
| sources: | |
| - https://www.youtube.com | |
| - https://www.recaptcha.net | |
| - https://www.google.com | |
| frame-ancestors: | |
| base: none | |
| font-src: | |
| base: self | |
| sources: | |
| - https://fonts.gstatic.com | |
| connect-src: | |
| base: self | |
| sources: | |
| - https://www.googletagmanager.com | |
| - https://www.google-analytics.com | |
| - https://www.recaptcha.net | |
| - https://www.google.com | |
| base-uri: | |
| base: self | |
| form-action: | |
| base: self | |
| upgrade-insecure-requests: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/default/csp.settings.yml` around lines 8 - 60, Add the missing CSP
directives under the top-level "directives" mapping: introduce a new "base-uri"
directive with "base: self" and a new "form-action" directive with "base: self"
(or a more restrictive source list if needed). Update the same structure used by
existing directives (like "default-src", "script-src", "style-src") so
"base-uri: { base: self }" and "form-action: { base: self }" appear alongside
them to lock down allowed base URLs and permitted form submission targets.
There was a problem hiding this comment.
Not applying in this PR. The scope of this change is an intentional 1:1 migration of the existing 'seckit' policy to 'drupal/csp' - no source list is widened or narrowed, and no new directive is introduced. 'base-uri' and 'form-action' were not present in the previous policy and adding them here risks breaking known-good flows (e.g., third-party form submissions) without testing. Tracking as a follow-up hardening step.
There was a problem hiding this comment.
@AlexSkrypnyk, understood — keeping this PR as a strict 1:1 migration is the right call. Introducing untested directive changes mid-migration is a good way to accidentally break third-party form flows or other integrations. The follow-up hardening step makes sense once the migrated policy is confirmed stable.
Would you like me to open a GitHub issue to track the addition of base-uri and form-action directives as a follow-up CSP hardening task?
| style-src: | ||
| base: self | ||
| flags: | ||
| - unsafe-inline |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider migrating away from unsafe-inline for styles.
The unsafe-inline flag in style-src allows arbitrary inline styles, which degrades CSP protection. While this is a common trade-off in Drupal implementations where inline styles are prevalent, it opens the door to CSS-based attacks and potential data exfiltration.
Consider migrating to nonce-based or hash-based styles in the long term to strengthen the security posture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/default/csp.settings.yml` around lines 24 - 27, The CSP currently
permits inline styles via the style-src flags entry containing "unsafe-inline";
update the policy by removing "unsafe-inline" from the style-src flags and
migrate to a nonce- or hash-based approach: replace the flags entry with either
a runtime-generated nonce token (e.g., add a style-src "nonce-{nonce}"
placeholder in the config) or with specific style hashes, implement server-side
nonce generation (inject the same nonce into the CSP header and into any inline
<style> or style attributes in templates), and ensure the configuration keys
referenced (style-src, base, flags) are updated to emit the nonce/hash value so
the CSP header and rendered pages remain in sync.
There was a problem hiding this comment.
Not applying in this PR. 'style-src' with 'unsafe-inline' is inherited from the previous 'seckit' policy and kept unchanged to preserve 1:1 behaviour during the migration. Drupal core and CivicTheme emit inline styles that would need hashes or nonces assigned across many render paths. Tracking as a follow-up to migrate 'style-src' to nonces once an audit of inline styles is complete.
There was a problem hiding this comment.
@AlexSkrypnyk, understood — preserving 1:1 parity with the existing seckit policy during migration is a reasonable rationale for keeping unsafe-inline in style-src for now, and a full audit across Drupal core + CivicTheme render paths is the right prerequisite before switching to nonces or hashes.
Would you like me to open a follow-up GitHub issue to track the style-src unsafe-inline → nonce/hash migration? I can include a link back to this PR and comment for context.
| Scenario: CSP header contains a nonce for anonymous users | ||
| Given I am an anonymous user | ||
| When I go to the homepage | ||
| Then the response status code should be 200 | ||
| And the response header "Content-Security-Policy" should contain the value "'nonce-" | ||
|
|
||
| @api | ||
| Scenario: CSP header contains a nonce for authenticated users | ||
| Given I am logged in as a user with the "administrator" role | ||
| When I go to the homepage | ||
| Then the response status code should be 200 | ||
| And the response header "Content-Security-Policy" should contain the value "'nonce-" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a regression test for nonce rotation across requests.
Current checks confirm nonce presence but not that the nonce changes per request.
Suggested scenario addition
+ `@api`
+ Scenario: CSP nonce changes between requests
+ Given I am an anonymous user
+ When I go to the homepage
+ Then capture the response header "Content-Security-Policy" value as "csp_header_1"
+ When I go to the homepage
+ Then capture the response header "Content-Security-Policy" value as "csp_header_2"
+ And the extracted nonce from "csp_header_1" should not equal the extracted nonce from "csp_header_2"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/behat/features/csp.feature` around lines 11 - 23, Add a regression
Behat scenario that verifies CSP nonce rotation by making two consecutive
requests to the homepage and asserting the "Content-Security-Policy" header
contains a nonce on each request and that the nonce values differ; reuse the
existing steps ("Given I am an anonymous user" / "Given I am logged in as a user
with the \"administrator\" role", "When I go to the homepage", "Then the
response header \"Content-Security-Policy\" should contain the value
\"'nonce-\"") but extend the scenario to capture the full nonce substring from
the header on the first request, perform a second request, capture the second
nonce, and assert the two nonce strings are not equal to ensure rotation for
both anonymous and authenticated cases.
There was a problem hiding this comment.
Not applying in this PR. The steps in the suggestion are not provided by the DrupalExtension Mink step library used here, so adopting the scenario would require a custom FeatureContext step to extract the nonce from a header and compare two captured values across requests. Nonce rotation is already guaranteed by the 'drupal/csp' 'Nonce' service, which regenerates the random value on each request, and the module ships its own 'NonceSettingTest' functional coverage. Keeping this out of scope here.
There was a problem hiding this comment.
@AlexSkrypnyk, understood — that's a fair call. The drupal/csp module's built-in NonceSettingTest functional coverage already guarantees per-request nonce rotation, and adding the scenario here would require custom FeatureContext step definitions outside the scope of this PR. Thanks for the clear explanation!
✏️ Learnings added
Learnt from: AlexSkrypnyk
Repo: drevops/website PR: 166
File: tests/behat/features/csp.feature:11-23
Timestamp: 2026-04-22T09:43:11.276Z
Learning: In the `drevops/website` repository, Behat tests use the DrupalExtension Mink step library. Custom header-capture and cross-request value comparison steps are not available out of the box and would require a custom FeatureContext implementation. Avoid suggesting Behat scenarios that rely on such non-standard steps unless a custom FeatureContext already exists in the project.
Learnt from: AlexSkrypnyk
Repo: drevops/website PR: 166
File: tests/behat/features/csp.feature:11-23
Timestamp: 2026-04-22T09:43:11.276Z
Learning: In `drevops/website`, CSP per-request nonce rotation is guaranteed by the `drupal/csp` module's `Nonce` service and is covered by the module's own `NonceSettingTest` functional test. There is no need to add a Behat regression scenario for nonce rotation in `tests/behat/features/csp.feature`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #166 +/- ##
========================================
Coverage 87.73% 87.73%
========================================
Files 11 11
Lines 163 163
========================================
Hits 143 143
Misses 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…larified settings comment.
|
Code coverage (GitHub Actions) |
2 participants
Summary
Production was reporting CSP violations caused by BigPipe's inline
<script>tags being blocked under the strict policy previously emitted byseckit. The root cause is thatseckithas no mechanism to emit a per-request nonce - its CSP header is static and cannot include a nonce value that changes on every response. Moving CSP header ownership todrupal/cspsolves this: the module is nonce-aware by design and integrates with Drupal's render pipeline.seckitremains active for every other security header it manages (HSTS, Expect-CT, Feature-Policy, clickjacking, Referrer-Policy, X-Content-Type-Options).Changes
Dependency
drupal/csp:^2.1via composer (resolved to2.2.3).Configuration
cspmodule incore.extension.yml.config/default/csp.settings.ymlwith anenforcepolicy mirroring the previously-shippedseckitpolicy 1:1, plus a per-request nonce onscript-srcandscript-src-elem, areport-uripointing at the existing/report-csp-violationroute, andupgrade-insecure-requests.seckit.settings.seckit_xss.csp.checkbox: falseto stopseckitfrom emitting its own CSP header. Two modules emitting CSP simultaneously causes browsers to intersect both policies, which blocks more than either policy intends.Runtime behaviour
web/modules/custom/do_base/do_base.module- newhook_page_attachments()attaches acsp_noncesource on every page so the header always includes a nonce, and adds thecsp/noncelibrary sodrupalSettings.csp.nonceis available to JS.web/sites/default/includes/modules/settings.csp.php- disablesupgrade-insecure-requestson CI and LOCAL environments. This mirrors the existingsettings.seckit.phppattern; without it, local HTTP subresources fail because the browser upgrades them to HTTPS on a domain with no valid certificate.Tests
tests/behat/features/csp.featurewith three scenarios: anonymous-user nonce, admin-user nonce, and preservation of all previously allowed sources.tests/behat/features/seckit.feature- removed CSP-header assertions (now owned bycsp.feature); kept HSTS, Expect-CT, Feature-Policy, From-Origin, Referrer-Policy, and X-Content-Type-Options assertions.tests/phpunit/Drupal/EnvironmentSettingsTest.php- added$config['csp.settings']['enforce']['directives']['upgrade-insecure-requests'] = FALSEto the three CI/LOCAL environment tests (testEnvironmentLocal,testEnvironmentLocalContainer,testEnvironmentGha).Before / After
Who owns which header:
Policy shape change:
Summary by CodeRabbit
New Features
Chores / Configuration
Tests