-
Notifications
You must be signed in to change notification settings - Fork 91
Narrow authored iframe embeds to an approved-provider allowlist #466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
de14d6d
6ac0220
15a9fe3
42c0f67
9d045dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,64 @@ | ||
| class HtmlScrubber < Rails::Html::PermitScrubber | ||
| # Attributes preserved on a surviving <iframe>. Everything else — srcdoc, on* | ||
| # handlers, name, sandbox overrides, arbitrary allow — is dropped so only the | ||
| # vetted embed shape survives. See EmbedAllowlist for the host allowlist. | ||
| IFRAME_ATTRIBUTES = %w[ src width height allowfullscreen loading referrerpolicy title frameborder allow ].freeze | ||
|
jeremy marked this conversation as resolved.
Outdated
|
||
|
|
||
| # Feature-policy tokens permitted in a surviving iframe `allow` attribute. Any | ||
| # other requested capability (camera, microphone, geolocation, …) is dropped. | ||
| IFRAME_ALLOW_TOKENS = %w[ | ||
| accelerometer autoplay clipboard-write encrypted-media fullscreen | ||
| gyroscope picture-in-picture web-share | ||
| ].freeze | ||
|
|
||
| def initialize | ||
| super | ||
| self.tags = Rails::Html::WhiteListSanitizer.allowed_tags + %w[ | ||
| audio details summary iframe options table tbody td th thead tr video source mark | ||
| ] | ||
| end | ||
|
|
||
| # Keep an <iframe> only when its src host is on the embed allowlist; other tags | ||
| # fall through to the permitted-tag check. | ||
| def keep_node?(node) | ||
| if iframe?(node) | ||
| EmbedAllowlist.allows?(node["src"]) | ||
| else | ||
| super | ||
| end | ||
| end | ||
|
|
||
| # Minimize a surviving <iframe> to the vetted attribute set; other tags keep | ||
| # the default attribute scrubbing. | ||
| def scrub_attributes(node) | ||
| if iframe?(node) | ||
| minimize_iframe(node) | ||
| else | ||
| super | ||
| end | ||
| end | ||
|
|
||
| private | ||
| def iframe?(node) | ||
| node.element? && node.name == "iframe" | ||
| end | ||
|
|
||
| def minimize_iframe(node) | ||
| node.attribute_nodes.each do |attr| | ||
| name = attr.name.downcase | ||
| if IFRAME_ATTRIBUTES.include?(name) | ||
| node[attr.name] = filtered_allow(attr.value) if name == "allow" | ||
| else | ||
| attr.remove | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def filtered_allow(value) | ||
| value.to_s.split(";") | ||
| .map { |directive| directive.strip.split(/\s+/).first.to_s.downcase } | ||
|
jeremy marked this conversation as resolved.
Outdated
|
||
| .select { |token| IFRAME_ALLOW_TOKENS.include?(token) } | ||
| .uniq | ||
| .join("; ") | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,10 +24,17 @@ class PagesControllerTest < ActionDispatch::IntegrationTest | |
| assert_select "#test", html: %(<div style="text-align:center;">Hello</div>) | ||
| end | ||
|
|
||
| test "show with iframes" do | ||
| test "show strips an off-allowlist iframe" do | ||
| get leafable_path(sample_page_leaf(%(<div id="test"><iframe src="http://example.com"></iframe></div>))) | ||
|
|
||
| assert_select "#test", html: %(<iframe src="http://example.com"></iframe>) | ||
| assert_select "#test", html: %() | ||
| assert_select "iframe", false | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new controller test calls AGENTS.md reference: AGENTS.md:L5-L6 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not doing this — |
||
|
|
||
| assert_select "#test iframe[src=?]", "https://www.youtube.com/embed/abc" | ||
| end | ||
|
|
||
| test "show with tables in the markdown" do | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| require "test_helper" | ||
|
|
||
| class EmbedAllowlistTest < ActiveSupport::TestCase | ||
| test "default providers are allowed with no ENV set" do | ||
| with_env "CSP_EXTRA_FRAME_SRC" => nil do | ||
| assert EmbedAllowlist.allows?("https://www.youtube.com/embed/abc123") | ||
| assert EmbedAllowlist.allows?("https://player.vimeo.com/video/123") | ||
| assert EmbedAllowlist.allows?("https://www.loom.com/embed/xyz") | ||
| end | ||
| end | ||
|
|
||
| test "off-allowlist hosts are rejected" do | ||
| with_env "CSP_EXTRA_FRAME_SRC" => nil do | ||
| assert_not EmbedAllowlist.allows?("https://evil.example/embed") | ||
| assert_not EmbedAllowlist.allows?("https://notyoutube.com/embed") | ||
| end | ||
| end | ||
|
|
||
| test "non-https and non-absolute srcs are rejected" do | ||
| with_env "CSP_EXTRA_FRAME_SRC" => nil do | ||
| assert_not EmbedAllowlist.allows?("http://www.youtube.com/embed/x"), "http is rejected" | ||
| assert_not EmbedAllowlist.allows?("//www.youtube.com/embed/x"), "protocol-relative is rejected" | ||
| assert_not EmbedAllowlist.allows?("/local/page"), "relative is rejected" | ||
| assert_not EmbedAllowlist.allows?("javascript:alert(1)"), "javascript: is rejected" | ||
| assert_not EmbedAllowlist.allows?(nil) | ||
| assert_not EmbedAllowlist.allows?("") | ||
| end | ||
| end | ||
|
|
||
| test "a per-install ENV host is honored, from the same source CSP frame-src reads" do | ||
| with_env "CSP_EXTRA_FRAME_SRC" => "https://maps.example.test https://forms.example.test" do | ||
| assert EmbedAllowlist.allows?("https://maps.example.test/embed") | ||
| assert EmbedAllowlist.allows?("https://forms.example.test/f/1") | ||
| # and the same host appears in the CSP frame-src source list — no drift. | ||
| assert_includes EmbedAllowlist.frame_src_sources, "https://maps.example.test" | ||
| end | ||
| end | ||
|
|
||
| test "wildcard ENV hosts match the apex and any subdomain" do | ||
| with_env "CSP_EXTRA_FRAME_SRC" => "https://*.example.test" do | ||
| assert EmbedAllowlist.allows?("https://example.test/x") | ||
| assert EmbedAllowlist.allows?("https://deep.sub.example.test/x") | ||
| assert_not EmbedAllowlist.allows?("https://example.test.evil.com/x") | ||
| end | ||
| end | ||
|
|
||
| test "frame_src_sources always includes the default provider origins" do | ||
| with_env "CSP_EXTRA_FRAME_SRC" => nil do | ||
| assert_includes EmbedAllowlist.frame_src_sources, "https://www.youtube.com" | ||
| assert_includes EmbedAllowlist.frame_src_sources, "https://player.vimeo.com" | ||
| end | ||
| end | ||
|
|
||
| private | ||
| def with_env(vars) | ||
| original = {} | ||
| vars.each_key { |k| original[k] = ENV.key?(k) ? ENV[k] : :__unset__ } | ||
| vars.each { |k, v| v.nil? ? ENV.delete(k) : ENV[k] = v } | ||
| yield | ||
| ensure | ||
| original.each { |k, v| v == :__unset__ ? ENV.delete(k) : ENV[k] = v } | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| require "test_helper" | ||
|
|
||
| class HtmlScrubberTest < ActiveSupport::TestCase | ||
| test "keeps an allowlisted-provider iframe" do | ||
| html = %(<p>hi</p><iframe src="https://www.youtube.com/embed/abc"></iframe>) | ||
| out = scrub(html) | ||
|
|
||
| assert_includes out, "<iframe" | ||
| assert_includes out, %(src="https://www.youtube.com/embed/abc") | ||
| end | ||
|
|
||
| test "strips an off-allowlist iframe entirely" do | ||
| html = %(<p>before</p><iframe src="https://evil.example/x"></iframe><p>after</p>) | ||
| out = scrub(html) | ||
|
|
||
| assert_not_includes out, "<iframe" | ||
| assert_not_includes out, "evil.example" | ||
| assert_includes out, "before" | ||
| assert_includes out, "after" | ||
| end | ||
|
|
||
| test "strips an iframe with no src" do | ||
| assert_not_includes scrub(%(<iframe srcdoc="<b>x</b>"></iframe>)), "<iframe" | ||
| end | ||
|
|
||
| test "minimizes attributes on a surviving iframe" do | ||
| html = <<~HTML | ||
| <iframe src="https://player.vimeo.com/video/1" | ||
| width="640" height="360" allowfullscreen | ||
| srcdoc="<script>alert(1)</script>" | ||
| onload="steal()" name="x" sandbox=""></iframe> | ||
| HTML | ||
| out = scrub(html) | ||
|
|
||
| assert_includes out, %(src="https://player.vimeo.com/video/1") | ||
| assert_includes out, "width", "vetted attributes survive" | ||
| assert_not_includes out, "srcdoc", "srcdoc is dropped" | ||
| assert_not_includes out, "onload", "on* handlers are dropped" | ||
| assert_not_includes out, "sandbox", "author sandbox override is dropped" | ||
| assert_not_includes out, %(name="x"), "arbitrary attributes are dropped" | ||
| end | ||
|
|
||
| test "filters the iframe allow attribute to a safe token set" do | ||
| html = %(<iframe src="https://www.youtube.com/embed/x" allow="fullscreen; camera; microphone; autoplay"></iframe>) | ||
| out = scrub(html) | ||
|
|
||
| assert_match %r{allow="[^"]*fullscreen}, out | ||
| assert_match %r{allow="[^"]*autoplay}, out | ||
| assert_not_includes out, "camera" | ||
| assert_not_includes out, "microphone" | ||
| end | ||
|
|
||
| test "honors a per-install ENV provider" do | ||
| with_env "CSP_EXTRA_FRAME_SRC" => "https://maps.example.test" do | ||
| out = scrub(%(<iframe src="https://maps.example.test/embed"></iframe>)) | ||
| assert_includes out, "maps.example.test" | ||
| end | ||
| end | ||
|
|
||
| private | ||
| def scrub(html) | ||
| Loofah.fragment(html).scrub!(HtmlScrubber.new).to_html | ||
| end | ||
|
|
||
| def with_env(vars) | ||
| original = {} | ||
| vars.each_key { |k| original[k] = ENV.key?(k) ? ENV[k] : :__unset__ } | ||
| vars.each { |k, v| v.nil? ? ENV.delete(k) : ENV[k] = v } | ||
| yield | ||
| ensure | ||
| original.each { |k, v| v == :__unset__ ? ENV.delete(k) : ENV[k] = v } | ||
| end | ||
| end |
Uh oh!
There was an error while loading. Please reload this page.