Restrict iframe embeds to an allowlist of approved providers (author scrubber + CSP frame-src) - #480
Restrict iframe embeds to an allowlist of approved providers (author scrubber + CSP frame-src)#480jeremy wants to merge 1 commit into
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.
| # but not "/embedded" or "/watch". Rejects a valid host used with the wrong | ||
| # path shape. | ||
| def path_allowed?(path) | ||
| path == path_prefix || path.start_with?("#{path_prefix}/") |
| PERMITTED_ATTRIBUTES = %w[ | ||
| src width height allowfullscreen frameborder allow title loading referrerpolicy style | ||
| ].freeze |
|
|
||
| class ContentSecurityPolicyTest < ActionDispatch::IntegrationTest | ||
| test "frame-src carries the approved embed providers" do | ||
| get new_session_url |
| hosts = Array(entry["hosts"] || entry["host"]).map { |host| host.to_s.strip.downcase }.reject(&:blank?) | ||
| path_prefix = entry["path_prefix"].to_s | ||
| return if hosts.empty? || path_prefix.blank? |
![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".
| node.remove_attribute(attr.name) unless permitted.include?(attr.name) | ||
| end | ||
| else |
There was a problem hiding this comment.
When authored content supplies a style attribute on an approved iframe, this override retains it verbatim and bypasses the CSS-value sanitization performed by PermitScrubber#scrub_attributes. Because style is in PERMITTED_ATTRIBUTES, an author can inject declarations such as fixed positioning and a high z-index to turn the frame into a full-page overlay; filter the attribute names as intended, but still run the sanitizer's CSS scrubber on retained styles.
Useful? React with 👍 / 👎.
| def path_allowed?(path) | ||
| path == path_prefix || path.start_with?("#{path_prefix}/") |
There was a problem hiding this comment.
When an approved host has another framable route, a source such as https://www.youtube.com/embed/../watch passes this raw-string prefix check, but browsers remove the dot segment and navigate to /watch. The CSP still permits the host, so this bypasses the provider's intended path-shape restriction; reject dot segments or canonicalize the URL path before applying the prefix check.
Useful? React with 👍 / 👎.
|
|
||
| class ContentSecurityPolicyTest < ActionDispatch::IntegrationTest | ||
| test "frame-src carries the approved embed providers" do | ||
| get new_session_url |
There was a problem hiding this comment.
This request neither crosses hosts nor tests a full absolute URL, so use new_session_path rather than new_session_url. The root AGENTS.md specifically requires _path helpers in controller and integration tests unless the host or full URL is under test.
AGENTS.md reference: AGENTS.md:L8-L9
Useful? React with 👍 / 👎.
| def allows?(uri) | ||
| hosts.include?(normalize_host(uri.host)) && path_allowed?(uri.path) |
There was a problem hiding this comment.
When an operator configures a provider reached on a nondefault HTTPS port, such as https://video.example:8443/embed/1, this check ignores uri.port and allows the iframe, while csp_sources emits only https://video.example, which CSP matches on the scheme's default port. The browser therefore blocks an embed that the shared provider table reports as allowed; either support ports in provider entries and include them in the CSP source or reject nondefault ports during matching.
Useful? React with 👍 / 👎.
cd44482 to
4037c10
Compare
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.
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".
| # may be framed. | ||
| Rails.application.configure do | ||
| config.content_security_policy do |policy| | ||
| policy.frame_src -> { EmbedProvider.csp_frame_sources.presence || [ :none ] } |
There was a problem hiding this comment.
When any response builds this policy, Rails resolves the proc as one source expression rather than flattening its return value, so returning an array serializes a header like frame-src ["https://youtube.com", ...] instead of space-separated origins. Browsers reject those bracketed/quoted expressions, causing every approved iframe to remain blocked; expand the dynamic list into individual CSP sources rather than returning an array from a single source proc.
Useful? React with 👍 / 👎.
What & why
Authored book content is rendered to unauthenticated readers, but
HtmlScrubberpermitted<iframe src=…>from any origin. That is a much wider capability than authoring actually needs (a video or a map embed), and it means every reader's session can load an arbitrary third-party frame chosen by the author.This narrows the capability to an allowlist of approved embed providers, enforced in two places that can't drift because they read from one table:
HtmlScrubberkeeps an<iframe>only when itssrcmatches an approved provider's host and path shape, and strips every attribute the provider doesn't permit. Anything else is stripped.Content-Security-Policyframe-srcdirective is derived from the same provider table, so the header and the scrubber always agree. Onlyframe-srcis set; the rest of the policy is intentionally left unrestricted so this change is scoped to which origins may be framed.Single source of truth
app/models/embed_provider.rbis the one table both legs read:{ name, hosts, path_prefix, attributes }. Adding a provider is a config edit, not a scrubber rewrite, and it widens the scrubber allowance and the CSP directive together.Default providers (shipped enabled)
youtube.com,www.youtube.com,youtube-nocookie.com,www.youtube-nocookie.com/embed/<id>player.vimeo.com/video/<id>loom.com,www.loom.com/embed/<id>google.com,www.google.com/maps/embedA src is accepted only when it is
https, on the default port, with an exact host (case-insensitive), and a segment-boundary path match. So a valid host used with the wrong path (youtube.com/watch?v=…), a lookalike host (youtube.com.evil.com), 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 (/embed/../watch,%2e%2e,%2f) are all rejected.Retained iframe attributes
A kept iframe carries only
src,width,height,allowfullscreen,frameborder,title,loading. Everything else is stripped — includingsrcdoc,sandbox,name,on*handlers,style(CSS exfil / overlay), andallow/referrerpolicy(delegating powerful features or leaking the URL). None of the retained attributes carries an author-controlled URI or CSS value.Per-install configuration (self-hosted)
Writebook is self-hosted, so the permitted hosts vary per install. Operators extend the table via the
WRITEBOOK_EMBED_PROVIDERSenvironment variable — a JSON array (a single object is also accepted) of provider entries:[ { "name": "Wistia", "hosts": ["fast.wistia.net"], "path_prefix": "/embed/" } ]hosts(required) — exact DNS hostnames to permit (string or array). Wildcards, whitespace, and IP literals are rejected.path_prefix(required) — permitted path, matched on a segment boundary; must be more specific than/.attributes(optional) — iframe attributes to retain; defaults to the vetted set. Values are always intersected with the master allowlist, so config can never reintroducesrcdoc,sandbox,name,on*,style,allow, orreferrerpolicy.Configured providers extend both the scrubber allowance and the CSP
frame-srcdirective at once. Invalid JSON or invalid entries are dropped (logged) and the defaults still apply.No escape hatch
There is deliberately no raw-iframe bypass. An embed is permitted only when a provider in the table vouches for it.
Rollout note
This narrows an existing authoring capability: any book currently embedding an off-allowlist origin will have that embed stripped on re-render. Operators who need those origins add them via
WRITEBOOK_EMBED_PROVIDERS. Auditing existing books' embeds before enforcing is a sensible operational follow-up.Tests
userinfo@host, path-prefix and dot-segment/encoded traversal, non-443 port, trailing-dot and IP-literal hosts,data:/http:, lookalike hosts, baresrcdoc) are rejected.frame-srcreflects exactly the table (defaults + an operator-extended entry), and config extension adds to both the scrubber and CSP; malformed/over-broad config is dropped without disturbing defaults.PagesControllerTest).Full suite, RuboCop, and Brakeman are green.