Repository navigation
Inventory Trusted Types violations in system tests - #3194
Conversation
Serve a report-only require-trusted-types-for 'script' policy in the test environment, on its own header so the app policy is untouched. A listener injected over CDP records each violation into sessionStorage; teardown drains it, and the run groups violations by sink and source location into tmp/trusted_types_violations.json, which CI uploads as an artifact. No test fails over a violation: this inventories the DOM sinks to migrate before any Trusted Types enforcement is designed.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Configured endpoints can send violations outside CI, and listener cleanup failures can turn inventory collection into test errors.
2 open findings
What changed in this PR
Adds a system-test inventory of Trusted Types violations to inform future enforcement without changing the enforcing policy.
Changes:
- Enables separate report-only middleware in tests.
- Collects violations and groups them by sink and source.
- Tests header behavior and uploads CI inventories.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| test/test_helpers/trusted_types_violations_test_helper.rb | Captures violations and writes grouped inventories. |
| test/middleware/trusted_types_report_only_test.rb | Tests header stacking and response filtering. |
| test/integration/static_csp_test.rb | Checks separation from the enforcing policy. |
| test/application_system_test_case.rb | Includes inventory collection. |
| config/initializers/content_security_policy.rb | Adds optional report-only middleware. |
| config/environments/test.rb | Enables middleware for tests. |
| .github/workflows/test.yml | Uploads inventory artifacts. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9407b10071
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… them off the network Drain and unregister the listener inside the window the test started in, so a test that switches windows neither errors in teardown nor loses that window's records; a vanished window warns rather than failing the test. Send the inventory policy without a report URI, so an inherited CSP_REPORT_URI can't post test violations anywhere, and write the inventory even when it's empty.
A listener left registered survives Capybara's reset, so every later test would record each violation twice.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Review summary at 3739578. CI is green on all four test jobs, and each uploads its inventory artifact. Codex found no major issues on this head, and Copilot approved it. No open threads. Declined, with reasoning:
Holding: not merged, and auto-merge isn't armed. #3092 is stacked on this branch. |
Inventory Trusted Types violations in system tests
Before anything about Trusted Types goes to production, this lists the DOM sinks fizzy and its libraries write raw strings to, using only the system tests. Nothing is enforced, no test fails over a violation, and no report leaves the CI box.
This is track 1 of the Security Hardening card's Trusted Types work. #3092 is reworked and stacked on it as track 2: a sampled report-only pilot in production, which waits on a Trust & Compliance review.
What changes
TrustedTypesReportOnlymiddleware (config/initializers/content_security_policy.rb), carried over from Trusted Types report-only pilot: 1% of SaaS page loads, collected first-party #3092. It sendsrequire-trusted-types-for 'script'on its ownContent-Security-Policy-Report-Onlyheader, so the app policy is unchanged byte for byte. It stacks as a second header field when the app policy is itself report-only. It's on only whenconfig.x.content_security_policy.trusted_types_report_onlyis set, and onlyconfig/environments/test.rbsets it. Here it carries noreport-uri, so an inheritedCSP_REPORT_URIcan't post test violations anywhere.TrustedTypesViolationsTestHelper(test/test_helpers/trusted_types_violations_test_helper.rb), included inApplicationSystemTestCase:Page.addScriptToEvaluateOnNewDocumentrecords each Trusted Typessecuritypolicyviolation(sample, source file:line:col, page path) insessionStorage, which survives full page loads.before_teardowndrains it intotmp/trusted_types_violations.jsonl, inside the window the test started in. It has to bebefore_teardown:teardowncallbacks run after Capybara has reset the session to a blank page.Minitest.after_rungroups the violations by sink and by source (origin and asset digest stripped) intotmp/trusted_types_violations.json.sessionStorage, and writes after the final drain.trusted-types-violations-{oss,saas}-{sqlite,mysql}) from each test job (actions/upload-artifactv7.0.2, pinned by SHA).Inventory: system tests (OSS, SQLite, Chrome 152 headless, thelio, 9407b10)
40 runs, 0 failures. 3,164 violations at 21 call sites. All four CI jobs at that head (OSS and SaaS, SQLite and MySQL) agree: 21 sites, 3,149 to 3,167 violations. Re-run at 6fa278f: 3,152 violations, the same 21 sites.
Element innerHTMLDOMParser parseFromStringServiceWorkerContainer register(TrustedScriptURL)HTMLScriptElement src(TrustedScriptURL)Element insertAdjacentHTMLTop sources:
controllers/local_time_controller.js:93lexxy.js:20960turbo.min.js:5:2806(Drive visits, frame loads)lexxy.js:8056lexxy.js:9153lexxy.js:8063turbo.min.js:5:11182(stream render)turbo.min.js:19:32785(Turbo's service worker registration)controllers/syntax_highlight_controller.js:33,34mermaid.tiny.jshelpers/mermaid_helpers.js:77turbo.min.js:19:44067(streamupdateclearing its target)activity_pagination_test.rb:27)The other lexxy sites account for 22 to 62 each: toolbar buttons, table controls and icons. Turbo shows up at four sites (Drive and frame
parseFromString, stream-renderinnerHTML, streamupdate, service-worker registration): 537 violations. That's not all of Turbo's sinks. Itsdata-turbo-submits-withtext swap and missing-frame rendering also write raw strings, and no system test reaches them. Either way, enforcement can't come before Turbo has a policy hook, whatever fizzy's own code does. The first-party sinks arelocal_time_controller,syntax_highlight_controllerandmermaid_helpers.The system suite has only 9 test files, so this is a floor, not a census. Paths no system test visits don't appear. The artifact lists the pages and tests behind each site.
Verification
test/middleware/trusted_types_report_only_test.rbruns the middleware underRack::Lint: sole policy, stacking beside an existing report-only policy, non-HTML untouched.test/integration/static_csp_test.rb: the enforcing policy carries no Trusted Types directive, and the report-only header carries exactlyrequire-trusted-types-for 'script'.PARALLEL_WORKERS=1 bin/rails test:system, OSS/SQLite): 40 runs, 0 failures. The inventory above came from that run.teardownread a blank page. A scratch test confirmed the listener itself recorded violations, which pointed at the drain. 9407b10 moves it tobefore_teardown.ensure, so it still happens when draining fails. A listener left registered survives Capybara's reset and double-counts every later test. Scratch pair on thelio at 3739578: a test that leaves an unhandled alert open warns and passes, and the next test records exactly 1 of 1 writes.Risk and rollback
test.rbsets the flag.execute_script. If Chrome stops supporting CDP from Selenium,setupraises and system tests error loudly rather than silently losing the inventory.tmp/.What it unblocks