Add an opt-in allowlist of iframe embed providers, on by default for new installs - #480
Conversation
There was a problem hiding this comment.
Pull request overview
Restricts iframe embeds to approved providers using shared scrubber and CSP configuration.
Changes:
- Adds configurable provider, host, path, and attribute allowlists.
- Enforces restrictions during sanitization and through CSP.
- Adds model, helper, controller, and integration 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 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/embed_provider.rb |
Defines providers and matching rules. |
app/models/html_scrubber.rb |
Filters iframes and attributes. |
config/initializers/content_security_policy.rb |
Generates frame-src policy. |
test/models/embed_provider_test.rb |
Tests provider matching and configuration. |
test/helpers/pages_helper_test.rb |
Tests iframe sanitization. |
test/controllers/pages_controller_test.rb |
Tests rendered page behavior. |
test/integration/content_security_policy_test.rb |
Tests CSP response headers. |
💡 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: cd44482e5d
ℹ️ 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".
4037c10 to
ba8981f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba8981fb08
ℹ️ 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".
Authored book content is rendered to unauthenticated readers, but the
HtmlScrubber allowed <iframe src=…> from any origin. Narrow that to an
allowlist of approved embed providers, enforced at both author time (the
scrubber) and render time (the CSP frame-src directive).
Both enforcement points read from a single provider table (EmbedProvider)
so they can't drift:
* scrubber — keeps an <iframe> only when its src matches a provider's
host and path shape, and strips every attribute the provider doesn't
permit (no srcdoc, sandbox, name, or on* handlers ride along).
* CSP — frame-src is derived from the same table.
Ships with YouTube, Vimeo, Loom, and Google Maps enabled by default, each
pinned to its embed path shape. Self-hosted operators extend the table per
install via the WRITEBOOK_EMBED_PROVIDERS environment variable (JSON),
which widens the scrubber allowance and the CSP directive together. There
is no raw-iframe escape hatch: an embed is permitted only via a vetted
provider entry.
ba8981f to
ff2c5bd
Compare
|
🤖 Rebased onto main @ Conflict: Changes:
Verified by hand ( On the open thread at Tests: Review: no independent adversarial round on this push — the review agent pool was saturated. The 2026-08-26 Codex review from the GitHub connector is the only outside review on record. Still a product call. The card is tagged PRODUCT DECISION and this PR doesn't settle it: the default provider set, and what happens to already-published books whose embeds this strips. There's no audit, no author warning and no grandfather window here — an off-allowlist embed disappears on the next render, and the iframe's fallback text becomes visible body text. |
The leaf and book fragments wrap scrubbed page content, so a fragment cached before a policy change keeps serving markup the scrubber would now strip: an iframe from a since-removed provider, or attributes the provider no longer permits. CSP allows the host, so nothing downstream catches it. Key both fragments on HtmlScrubber.cache_version — a scrubber policy version plus a digest of the effective provider table (hosts, path prefix, attributes) — so a WRITEBOOK_EMBED_PROVIDERS edit or a scrubber tightening re-renders through the scrubber instead of hitting the stale fragment.
A configured path_prefix of "/.", "/./" or "//" passed validation (it starts with a slash and is longer than one character) but a browser resolves each to "/", so the entry became a whole-host allowance rather than the path shape the operator wrote. Canonicalize the prefix before accepting it: collapse duplicate slashes, drop the trailing slash, and drop the entry when any segment is "." or ".." or nothing but the root remains. The canonical form is what gets stored and matched against.
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. |
|
Pushed two fixes from the adversarial review, as separate commits: Stale fragment caches — 38d1df2. Root-equivalent path prefixes — bde9da8.
The nine open inline threads are all against |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bde9da869a
ℹ️ 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".
Matching is first-match-wins, so two entries overlapping on host and path but differing in attributes are a different policy in either order. Sorting the signatures hid that, so a reordered config kept serving fragments scrubbed under the previous order.
Clear WRITEBOOK_EMBED_PROVIDERS for every test and restore it afterwards, so a value in the operator's shell neither fails the default-table assertions nor gets dropped for the rest of the run. Reuse the welcome page fixture in the iframe show tests.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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 loop converged at |
…rated list Enforcing an allowlist by default would strip embeds that existing books render today. Instead, embeds stay as they are — any origin, attributes scrubbed, no Content-Security-Policy — until a provider table is configured, and then the scrubber and frame-src are both derived from it. The table comes from WRITEBOOK_EMBED_PROVIDERS first, then the account's embed_providers setting, else nothing. FirstRun seeds the curated defaults into the account, so a fresh install starts on YouTube, Vimeo, Loom and Google Maps, while an install upgraded from before the setting keeps a null row and stays permissive. An existing install opts in by setting the environment variable or the account setting. frame-src moves from the initializer to ApplicationController so that no directive at all is sent while permissive; an empty frame-src would block every frame. The fragment cache version names the permissive mode, so configuring an allowlist re-renders cached pages through the scrubber.
jeremy
changed the title
|
Reworked per the product decision on the card: the allowlist is now opt-in, and only new installs start on the curated list —
|
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
Policy resolution is repeatedly recomputed per request and can amplify invalid configuration warnings into production log flooding.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Balanced
The scrubber, the frame-src directive and the fragment cache key each read the table several times per request, and every read re-validated the entries and re-logged any rejected one. Keep the last resolution keyed by its source entries, so a config typo is logged once and a changed configuration is still picked up on the next read.
987ee49 to
4c8c3db
Compare
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
Environment JSON is reparsed and invalid configuration is repeatedly logged on every policy lookup, risking excessive overhead and log volume.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Balanced
The environment JSON was still parsed, and an unparsable value still logged, on every read ahead of the memoized table. Key the resolution on the raw environment value and the account row instead, so both the parse and the validation happen once per distinct configuration.
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
Policy resolution repeatedly loads and deserializes the account configuration for every leaf cache key, causing N+1 work on uncached books.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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 loop converged at |
What & why
Authored book content is rendered to unauthenticated readers, and
HtmlScrubberkeeps an<iframe src=…>from any origin. That is wider than authoring needs (a video or a map embed) and means every reader's session can load an arbitrary third-party frame chosen by the author.This adds an opt-in allowlist of embed providers. Nothing changes for an existing install until it opts in; a fresh install starts on a curated list.
Behavior by install
main, noContent-Security-Policy.accounts.embed_providersas NULL; nothing reads a NULL row as a policy.FirstRun.create!seedsEmbedProvider::DEFAULTSinto the account row (app/models/first_run.rb) — the one seam where a fresh install initializes state.WRITEBOOK_EMBED_PROVIDERS(env) orAccount#embed_providers(DB).Precedence
WRITEBOOK_EMBED_PROVIDERSoverridesAccount#embed_providersoverrides permissive. Whichever source applies is the whole table — the env var does not extend the account's list, it replaces it. Tested inEmbedProviderTest(env-only, account-only, env-over-account, neither, no account row) andContentSecurityPolicyTest.Opting in on an existing install
WRITEBOOK_EMBED_PROVIDERS='[…]'— a JSON array of provider entries (a single object is also accepted).EmbedProvider::DEFAULTSis in this exact form, so the curated list isbin/rails runner 'puts EmbedProvider::DEFAULTS.to_json'.bin/rails runner 'Account.first.update!(embed_providers: EmbedProvider::DEFAULTS)'. There is no admin UI for it in this PR; the setting exists so a new install has somewhere to start, and it is where a future settings page would write.Invalid JSON in the env var is logged and ignored (the account setting still applies). A configured table whose entries are all invalid is configured-but-empty: every iframe is stripped and
frame-src 'none'is sent — it does not fall through to a wider source.[]is therefore also the way to disable iframes entirely.Once configured: two enforcement points, one table
app/models/embed_provider.rbis the one table both legs read:{ name, hosts, path_prefix, attributes }.HtmlScrubberkeeps an<iframe>only when itssrcmatches a provider's host and path shape, and strips every attribute the provider doesn't permit. Anything else is stripped.ApplicationControllersets aframe-srcdirective derived from the same table (content_security_policy if: -> { EmbedProvider.configured? }). It lives on the controller rather than the initializer so that no directive is sent while permissive — aframe-srcwith an empty source list would block every frame. Onlyframe-srcis set; the rest of the policy is left unrestricted.A src is accepted only when it is
https, on the default port, with an exact host (case-insensitive), and a segment-boundary path match. A valid host with the wrong path (youtube.com/watch?v=…), a lookalike host, userinfo confusion (youtube.com@evil.com), protocol-relative /data:/http:URLs, an explicit non-443 port, and dot-segment or encoded traversal past the prefix are all rejected.Provider entries
[ { "name": "Wistia", "hosts": ["fast.wistia.net"], "path_prefix": "/embed/" } ]hosts(required) — exact DNS hostnames (string or array). Wildcards, whitespace, and IP literals are rejected.path_prefix(required) — permitted path, matched on a segment boundary. Canonicalized before use (duplicate slashes collapsed, trailing slash dropped); an entry whose prefix contains a./..segment or reduces to/(/.,/./,//) is dropped, since a browser resolves those to the root.attributes(optional) — iframe attributes to retain; defaults tosrc width height allowfullscreen frameborder title loading. Always intersected with that master list, so config can never reintroducesrcdoc,sandbox,name,on*,style,allow, orreferrerpolicy.Matching is first-match-wins across the table.
Fragment caches
The
leaves/_leafandbooks/showfragments wrap scrubbed content, so their keys includeHtmlScrubber.cache_version— a scrubber policy version plusEmbedProvider.cache_version, which is"permissive"or a digest of the effective table in resolution order. Configuring an allowlist over a permissive install, editing the table, or tightening the scrubber re-renders through the scrubber instead of serving a fragment cached under the old policy. Tested both ways inBooksControllerTestwith fragment caching on.Tests
main, and no CSP header is sent (HtmlScrubberTest,PagesHelperTest,PagesControllerTest,ContentSecurityPolicyTest).EmbedProviderTest).FirstRunTest).frame-srcreflects exactly the configured table, an env table replaces the account's without a restart, and an all-invalid table emits'none'.Full suite, RuboCop, and Brakeman are green.