Narrow authored iframe embeds to an approved-provider allowlist - #466
Conversation
Writebook's HtmlScrubber allowed <iframe> with an arbitrary src in authored markdown, rendered to unauthenticated book readers (the DG4 residue after the WB-1 srcdoc neutralization). An author could embed any origin — content injection / clickjacking / third-party-origin trust in every reader's session. Introduces EmbedAllowlist as the single source of truth for permitted embed hosts, enforced twice: - Author-time (HtmlScrubber): keep an <iframe> only when its src host is on the allowlist; strip it otherwise, and minimize a surviving iframe to a vetted attribute set (drop srcdoc, on* handlers, name, sandbox overrides; filter the `allow` attribute down to a safe feature-token set). - Render-time (CSP frame-src): consume the same EmbedAllowlist so a provider the scrubber keeps is one the browser will load — the two enforcement points cannot drift. The host set is per-install configurable through the same CSP_EXTRA_FRAME_SRC ENV the CSP branch already tokenizes (Writebook is a self-hosted ONCE product), plus a shipped default provider list. PRODUCT DECISION (flagged in EmbedAllowlist): the default provider list, the per-provider attribute policy, grandfathering of existing off-allowlist embeds, and any raw-iframe escape hatch are product/authoring calls an owner must confirm before this enforces. Stacked on security/xss-campaign-writebook-csp-report-only. Spike for the XSS counteroffensive Tier C.
There was a problem hiding this comment.
Pull request overview
Introduces a shared iframe-provider allowlist used by HTML sanitization and CSP configuration.
Changes:
- Filters iframe hosts, attributes, and permissions.
- Adds default and environment-configured providers.
- Adds scrubber, CSP, and controller coverage.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/html_scrubber.rb |
Filters authored iframe elements and attributes. |
config/initializers/content_security_policy.rb |
Defines the allowlist and configures frame-src. |
test/models/html_scrubber_test.rb |
Tests iframe sanitization. |
test/models/embed_allowlist_test.rb |
Tests provider matching and configuration. |
test/integration/csp_nonce_test.rb |
Verifies CSP provider sources. |
test/controllers/pages_controller_test.rb |
Tests rendered iframe filtering. |
💡 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: de14d6d3d3
ℹ️ 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".
…ictions Review round on the allowlist's edges: - A CSP source expression narrower than a plain https origin (a path, a port) no longer widens into a bare-host scrubber match: it feeds frame-src verbatim and admits no iframes, so the scrubber can only ever accept what the CSP source actually allows. - A "*." pattern now covers subdomains only, never the apex, mirroring CSP host-source semantics; list the apex separately when both are wanted. - A surviving iframe allow attribute keeps each permitted directive whole instead of truncating it to the feature name - "autoplay 'none'" cut to "autoplay" would grant precisely what the author withheld. Also corrects the enforcement-point description: scrubbing happens at display time on every content render path (stored Markdown stays raw, so the allowlist applies retroactively), not at author/storage time.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (6)
config/initializers/content_security_policy.rb:156
allows?ignores the URL port. A CSP source such ashttps://www.youtube.commatches only the default HTTPS port, while this acceptshttps://www.youtube.com:8443/...; because CSP is currently report-only, that frame can load despite falling outside the browser policy. Reject non-default HTTPS ports to preserve the claimed equivalence.
return false unless uri && ALLOWED_SCHEMES.include?(uri.scheme) && uri.host.present?
config/initializers/content_security_policy.rb:207
- Removing
http://here treats an HTTP-only CSP source as a scrubber host:CSP_EXTRA_FRAME_SRC=http://maps.examplemakes the scrubber accepthttps://maps.example/..., althoughframe-srccontains only the HTTP source. Leave the HTTP prefix intact so the existing://check rejects it.
token = token.delete_prefix("https://").delete_prefix("http://")
config/initializers/content_security_policy.rb:97
HtmlScrubberapplies this allowlist at display time throughsanitize_content, not at author time. Update this stale wording to match the implementation and the display-time documentation below.
# EmbedAllowlist — the same source of truth HtmlScrubber uses at author-time —
config/initializers/content_security_policy.rb:117
- This states that
frame-srcrefuses disallowed loads, but this initializer still setscontent_security_policy_report_only = trueat line 224. In the current branch it only reports violations; clarify that refusal begins after enforcement is enabled.
# - browser-side, by the CSP frame-src directive below, which refuses to load
# a frame from an origin not on this list.
test/integration/csp_nonce_test.rb:89
- This comment repeats the stale author-time description, but the scrubber runs at display time through
sanitize_content. Correcting it avoids documenting the wrong enforcement lifecycle in the test.
# origins, so the render-time policy agrees with the author-time scrubber.
test/models/embed_allowlist_test.rb:39
- The test name says the wildcard matches the apex, but the test correctly asserts that CSP wildcard semantics reject the apex. Rename it so failures describe the expected behavior accurately.
test "wildcard ENV hosts match the apex and any subdomain" do
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ac0220387
ℹ️ 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".
A portless CSP source matches only the scheme's default port, so a kept iframe on an allowlisted host at :8443 would be one the browser then refuses to load. This closes the last dimension of the CSP host-source grammar in the scrubber's matcher - scheme, host, wildcard, path, and now port all align with what frame-src will actually permit.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
config/initializers/content_security_policy.rb:100
frame-srcis still report-only: this initializer setscontent_security_policy_report_only = trueat line 228, so the browser reports violations but does not provide the second enforcement point claimed by the PR. Either enable enforcement as part of this rollout or describe this control as report-only until that happens.
policy.frame_src :self, *::EmbedAllowlist.frame_src_sources
config/initializers/content_security_policy.rb:97
- This still says the scrubber operates at author time, but
HtmlScrubberis invoked bysanitize_contentduring display, as the updated module documentation and PR description now state. Use “display time” here to avoid documenting the previously corrected behavior incorrectly.
# EmbedAllowlist — the same source of truth HtmlScrubber uses at author-time —
test/controllers/pages_controller_test.rb:35
- This creates a new page and leaf for a read-only rendering assertion even though
leaves(:welcome_page)is available. Use the existing fixture and update its body, which follows the repository's fixture convention and avoids unnecessary records.
get leafable_path(sample_page_leaf(%(<div id="test"><iframe src="https://www.youtube.com/embed/abc"></iframe></div>)))
test/integration/csp_nonce_test.rb:89
- This comment calls the scrubber author-time even though sanitization occurs at display time. Update it so the test documents the actual enforcement point.
# origins, so the render-time policy agrees with the author-time scrubber.
test/models/embed_allowlist_test.rb:46
- The test name says the wildcard matches the apex, but the assertions correctly require the apex not to match. Rename it so failures describe the intended CSP semantics accurately.
test "wildcard ENV hosts match the apex and any subdomain" do
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15a9fe38cc
ℹ️ 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".
| end | ||
|
|
||
| test "show keeps an allowlisted-provider iframe" do | ||
| get leafable_path(sample_page_leaf(%(<div id="test"><iframe src="https://www.youtube.com/embed/abc"></iframe></div>))) |
There was a problem hiding this comment.
This new controller test calls sample_page_leaf, which persists another Page and Leaf via the exact books(...).press Page.new(...) pattern that the repository testing convention says to replace with an existing fixture. Reuse leaves(:welcome_page) and set up its body for this scenario instead of creating an additional record.
AGENTS.md reference: AGENTS.md:L5-L6
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not doing this — sample_page_leaf is this file's own pre-existing setup idiom (the neighboring tables test and others use it), and the two new tests follow it. Switching just these two to leaves(:welcome_page) plus a body rewrite would introduce a second setup pattern into the file while still mutating shared state per test, which is no closer to the convention's aim. Leaving unresolved for a human call.
Round three on the embed attribute policy and source matching: - An authored iframe sandbox survives minimization. The attribute can only ever remove privileges relative to no attribute at all, so stripping it UNsandboxed frames the author had locked down - the one direction a sanitizer must never move. - referrerpolicy is filtered to non-leaking values; authored unsafe-url (or the downgrade-tolerant default spelled out) no longer ships the reader's full URL to the embed provider. - An explicit :443 in CSP_EXTRA_FRAME_SRC counts as the plain https origin it is (CSP matches it like an omitted port) instead of being rejected by the port check, so such a source feeds both frame-src and the scrubber.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/controllers/pages_controller_test.rb:35
- This new test also creates a page through
sample_page_leafinstead of reusing the existing page fixture, contrary to the repository's fixture convention.
get leafable_path(sample_page_leaf(%(<div id="test"><iframe src="https://www.youtube.com/embed/abc"></iframe></div>)))
config/initializers/content_security_policy.rb:97
- This still describes
HtmlScrubberas author-time enforcement, but it runs fromsanitize_contentduring rendering. That contradicts the corrected display-time design documented below and in the PR description.
# EmbedAllowlist — the same source of truth HtmlScrubber uses at author-time —
test/controllers/pages_controller_test.rb:28
- This new test creates another page through
sample_page_leaf, contrary to the testing convention to reuse existing fixtures. Update the existing page fixture's body before requesting it, consistent withtest/controllers/leafables_controller_test.rb:49.
This issue also appears on line 35 of the same file.
get leafable_path(sample_page_leaf(%(<div id="test"><iframe src="http://example.com"></iframe></div>)))
test/integration/csp_nonce_test.rb:89
- This comment repeats the outdated author-time description. The scrubber is applied at display time, so the comment should use that terminology to match the implementation and PR description.
# origins, so the render-time policy agrees with the author-time scrubber.
test/models/embed_allowlist_test.rb:46
- The test name says the wildcard matches the apex, but the assertions correctly require the apex to be rejected. Rename it so failures report the behavior actually under test.
test "wildcard ENV hosts match the apex and any subdomain" do
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42c0f6722b
ℹ️ 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".
A CSP source's root path matches every path on the host, so "https://embed.example/" - the shape you get copying an origin from a browser - is the whole host, not a path restriction. Normalize it away like the explicit :443; deeper paths still feed frame-src only.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
config/initializers/content_security_policy.rb:97
- This still describes the scrubber as author-time enforcement, but
HtmlScrubberruns fromsanitize_contentwhile rendering and stored Markdown remains raw. Calling it author-time contradicts both the implementation and the PR's corrected display-time design; use “display time” here.
# EmbedAllowlist — the same source of truth HtmlScrubber uses at author-time —
test/integration/csp_nonce_test.rb:89
- The scrubber is invoked at display time, not author time. This comment now contradicts the test's render path and the documented design.
# origins, so the render-time policy agrees with the author-time scrubber.
test/models/embed_allowlist_test.rb:46
- The test name says wildcard hosts match the apex, while the assertions correctly verify that CSP wildcard semantics exclude it. Rename the test so failures describe the expected behavior accurately.
test "wildcard ENV hosts match the apex and any subdomain" do
|
Codex Review: Didn't find any major issues. 🎉 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". |
Closes the
DG4residue:HtmlScrubberallowed<iframe src>from any origin in authored markdown, rendered to unauthenticated book readers.Introduces
EmbedAllowlistas the single source of truth for permitted embed hosts, enforced twice:HtmlScrubber): every content render path passes throughsanitize_content, which keeps an<iframe>only when itssrchost is on the allowlist and minimizes a survivor to a vetted attribute set (dropsrcdoc,on*,name,sandboxoverrides; filterallowto a safe feature set, preserving each permitted directive's authored allowlist syntax). Stored Markdown stays raw, so the list applies retroactively to existing content.frame-src): consumes the sameEmbedAllowlist, so a provider the scrubber keeps is one the browser will load — the two enforcement points cannot drift. Scrubber matching mirrors CSP source semantics:*.hostcovers subdomains only (never the apex), and a source narrower than a plain https origin (path, port) feedsframe-srcverbatim while admitting no iframes.The host set is per-install configurable via the same
CSP_EXTRA_FRAME_SRCENV the CSP branch tokenizes (Writebook is a self-hosted ONCE product), plus a shipped default provider list.EmbedAllowlist): the default provider list, per-provider attribute policy, grandfathering of existing off-allowlist embeds, and any raw-iframe escape hatch are product/authoring calls an owner must confirm before this enforces.Stacked on
security/xss-campaign-writebook-csp-report-only. Tests green (embed allowlist, scrubber, CSP, pages controller). Draft spike for the XSS counteroffensive Tier C.