diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a52d12ee..db5c3e39 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -3,4 +3,13 @@ class ApplicationController < ActionController::Base # Only allow modern browsers supporting webp images, web push, badges, import maps, CSS nesting, and CSS :has. allow_browser versions: :modern + + # Render-time half of the iframe embed allowlist: once a provider table is + # configured, `frame-src` is derived from the same table HtmlScrubber reads so + # the two can't drift. Left unset while permissive, so nothing that renders + # today is blocked, and only `frame-src` is set — the rest of the policy is + # deliberately unrestricted. + content_security_policy if: -> { EmbedProvider.configured? } do |policy| + policy.frame_src(*EmbedProvider.csp_frame_sources) + end end diff --git a/app/models/account.rb b/app/models/account.rb index 75802c75..4807e1fb 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1,3 +1,8 @@ class Account < ApplicationRecord include Joinable + + # The iframe embed allowlist for this install, as EmbedProvider config entries. + # Nil leaves embeds permissive; FirstRun seeds the curated defaults for new + # installs, and an install upgraded from before the column keeps nil. + serialize :embed_providers, coder: JSON end diff --git a/app/models/embed_provider.rb b/app/models/embed_provider.rb new file mode 100644 index 00000000..2c364e3b --- /dev/null +++ b/app/models/embed_provider.rb @@ -0,0 +1,254 @@ +# Which third-party )) + + writes = 0 + ActiveSupport::Notifications.subscribed(->(*) { writes += 1 }, "write_fragment.action_controller") do + get book_slug_path(books(:handbook)) + end + assert_response :success + assert_select "iframe[src=?]", "https://x.example/embed/1" + assert_operator writes, :>, 0, "expected the page to be fragment cached" + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = EmbedProvider::DEFAULTS.to_json + get book_slug_path(books(:handbook)) + assert_response :success + assert_select "iframe", count: 0 + end + end + + test "show re-scrubs cached leaves when the allowlist narrows" do + with_fragment_caching do + leaves(:welcome_page).leafable.update!(body: %()) + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"hosts":["x.example"],"path_prefix":"/embed"}]) + get book_slug_path(books(:handbook)) + assert_select "iframe[src=?]", "https://x.example/embed/1" + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = EmbedProvider::DEFAULTS.to_json + get book_slug_path(books(:handbook)) + assert_select "iframe", count: 0 + end + end + test "show includes link to markdown format" do get book_slug_path(books(:handbook)) assert_response :success assert_select "link[rel=\"alternate\"][type=\"text/markdown\"][href=\"#{book_slug_path(books(:handbook), format: :md)}\"]" end + + private + def with_fragment_caching + perform_caching, cache_store = ActionController::Base.perform_caching, ActionController::Base.cache_store + ActionController::Base.perform_caching = true + ActionController::Base.cache_store = :memory_store + yield + ensure + ActionController::Base.perform_caching = perform_caching + ActionController::Base.cache_store = cache_store + end end diff --git a/test/controllers/pages_controller_test.rb b/test/controllers/pages_controller_test.rb index 0b0ccec8..e33690b5 100644 --- a/test/controllers/pages_controller_test.rb +++ b/test/controllers/pages_controller_test.rb @@ -30,6 +30,25 @@ class PagesControllerTest < ActionDispatch::IntegrationTest assert_select "#test", html: %() end + test "show keeps an approved-provider iframe under a configured allowlist" do + ENV["WRITEBOOK_EMBED_PROVIDERS"] = EmbedProvider::DEFAULTS.to_json + leaves(:welcome_page).leafable.update!(body: %(
)) + + get leafable_path(leaves(:welcome_page)) + + assert_select "#test iframe[src=?]", "https://www.youtube.com/embed/dQw4w9WgXcQ" + end + + test "show strips an off-allowlist iframe under a configured allowlist" do + ENV["WRITEBOOK_EMBED_PROVIDERS"] = EmbedProvider::DEFAULTS.to_json + leaves(:welcome_page).leafable.update!(body: %(
)) + + get leafable_path(leaves(:welcome_page)) + + assert_select "#test", html: "" + assert_select "#test iframe", count: 0 + end + test "show with tables in the markdown" do get leafable_path(sample_page_leaf(%(| name | food |\n| ---- | ---- |\n| Kevin | Pizza |))) diff --git a/test/helpers/pages_helper_test.rb b/test/helpers/pages_helper_test.rb new file mode 100644 index 00000000..583dbe49 --- /dev/null +++ b/test/helpers/pages_helper_test.rb @@ -0,0 +1,68 @@ +require "test_helper" + +class PagesHelperTest < ActionView::TestCase + test "sanitize_content keeps an iframe from any origin while embeds are permissive" do + html = %() + result = sanitize_content(html) + + assert_includes result, %(src="https://anything.example/embed/x") + assert_includes result, "allowfullscreen" + assert_not_includes result, "allow=" + assert_not_includes result, "onload" + end + + test "sanitize_content keeps an approved-provider iframe" do + with_allowlist do + html = %() + result = sanitize_content(html) + + assert_includes result, ") + assert_not_includes sanitize_content(html), ") + assert_not_includes sanitize_content(html), ") + result = sanitize_content(html) + + assert_includes result, ") + assert_not_includes sanitize_content(html), " "https://www.youtube.com/embed/dQw4w9WgXcQ", + "YouTube (nocookie)" => "https://www.youtube-nocookie.com/embed/dQw4w9WgXcQ", + "Vimeo" => "https://player.vimeo.com/video/76979871", + "Loom" => "https://www.loom.com/embed/0123456789abcdef", + "Google Maps" => "https://www.google.com/maps/embed?pb=!1m18!1m12!1m3" + }.each do |name, url| + test "#{name} embed is allowed" do + assert EmbedProvider.allows?(url), "expected #{url} to be allowed" + end + end + + test "bare and www hosts both allowed" do + assert EmbedProvider.allows?("https://youtube.com/embed/abc") + assert EmbedProvider.allows?("https://www.youtube.com/embed/abc") + end + + # --- disallowed origins ---------------------------------------------------- + + test "unknown origin is rejected" do + assert_not EmbedProvider.allows?("https://evil.com/embed/abc") + end + + test "valid host with wrong path shape is rejected" do + assert_not EmbedProvider.allows?("https://www.youtube.com/watch?v=dQw4w9WgXcQ") + assert_not EmbedProvider.allows?("https://player.vimeo.com/channels/staffpicks") + assert_not EmbedProvider.allows?("https://www.google.com/maps/place/foo") + end + + # --- allowlist bypass attempts -------------------------------------------- + + test "path-prefix boundary tricks are rejected" do + assert_not EmbedProvider.allows?("https://www.youtube.com/embedxyz/evil") + assert_not EmbedProvider.allows?("https://www.youtube.com/embedded") + end + + test "dot-segment and encoded traversal past the prefix are rejected" do + assert_not EmbedProvider.allows?("https://www.youtube.com/embed/../watch?v=x") + assert_not EmbedProvider.allows?("https://www.youtube.com/embed/%2e%2e/watch") + assert_not EmbedProvider.allows?("https://www.youtube.com/embed%2fx") + end + + test "explicit non-default port is rejected (CSP is host-only, implicit 443)" do + assert_not EmbedProvider.allows?("https://www.youtube.com:444/embed/x") + assert EmbedProvider.allows?("https://www.youtube.com:443/embed/x") + end + + test "protocol-relative url is rejected" do + assert_not EmbedProvider.allows?("//www.youtube.com/embed/abc") + end + + test "non-https schemes are rejected" do + assert_not EmbedProvider.allows?("http://www.youtube.com/embed/abc") + assert_not EmbedProvider.allows?("data:text/html,") + assert_not EmbedProvider.allows?("javascript:alert(1)") + end + + test "userinfo host confusion is rejected" do + assert_not EmbedProvider.allows?("https://www.youtube.com@evil.com/embed/abc") + assert_not EmbedProvider.allows?("https://evil.com@www.youtube.com/embed/abc") + end + + test "lookalike hostnames are rejected" do + assert_not EmbedProvider.allows?("https://notyoutube.com/embed/abc") + assert_not EmbedProvider.allows?("https://youtube.com.evil.com/embed/abc") + end + + test "host is matched case-insensitively" do + assert EmbedProvider.allows?("https://WWW.YOUTUBE.COM/embed/abc") + end + + test "trailing-dot host is rejected (CSP would not match it)" do + assert_not EmbedProvider.allows?("https://www.youtube.com./embed/abc") + end + + test "blank and malformed srcs are rejected" do + assert_not EmbedProvider.allows?(nil) + assert_not EmbedProvider.allows?("") + assert_not EmbedProvider.allows?("https://") + end + + # --- CSP frame-src derives from the same table ---------------------------- + + test "csp_frame_sources reflects exactly the configured table" do + assert_equal %w[ + https://youtube.com https://www.youtube.com + https://youtube-nocookie.com https://www.youtube-nocookie.com + https://player.vimeo.com + https://loom.com https://www.loom.com + https://google.com https://www.google.com + ], EmbedProvider.csp_frame_sources + end + + # --- config validation ----------------------------------------------------- + + test "default providers carry only the vetted attribute set" do + provider = EmbedProvider.match("https://www.youtube.com/embed/abc") + assert_equal EmbedProvider::PERMITTED_ATTRIBUTES, provider.attributes + %w[srcdoc sandbox name onload style allow referrerpolicy].each do |forbidden| + assert_not_includes provider.attributes, forbidden + end + end + + test "config cannot reintroduce forbidden attributes" do + ENV["WRITEBOOK_EMBED_PROVIDERS"] = + %([{"name":"X","hosts":["x.example"],"path_prefix":"/e","attributes":["src","srcdoc","sandbox","onload","style","allow","referrerpolicy"]}]) + + provider = EmbedProvider.match("https://x.example/e/1") + assert_equal %w[src], provider.attributes + end + + test "invalid environment json is ignored, the account setting still applies" do + ENV["WRITEBOOK_EMBED_PROVIDERS"] = "{not valid json" + assert EmbedProvider.allows?("https://www.youtube.com/embed/abc") + assert_not EmbedProvider.allows?("https://x.example/e/1") + end + + test "a rejected entry or unparsable value is logged once per distinct configuration, not per call" do + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"name":"typo-#{SecureRandom.hex(4)}","hosts":["*"],"path_prefix":"/e"}]) + log = capture_embed_provider_log do + 3.times { EmbedProvider.all } + 3.times { EmbedProvider.csp_frame_sources } + end + assert_equal 1, log.scan("ignoring invalid provider entry").size, log + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = "{not valid json #{SecureRandom.hex(4)}" + log = capture_embed_provider_log do + 3.times { EmbedProvider.configured? } + 3.times { EmbedProvider.all } + end + assert_equal 1, log.scan("not valid JSON").size, log + end + + test "a changed configuration is picked up without a restart" do + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"hosts":["a.example"],"path_prefix":"/a"}]) + assert EmbedProvider.allows?("https://a.example/a/1") + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"hosts":["b.example"],"path_prefix":"/b"}]) + assert_not EmbedProvider.allows?("https://a.example/a/1") + assert EmbedProvider.allows?("https://b.example/b/1") + + ENV.delete("WRITEBOOK_EMBED_PROVIDERS") + accounts(:signal).update!(embed_providers: [ { "hosts" => [ "c.example" ], "path_prefix" => "/c" } ]) + assert EmbedProvider.allows?("https://c.example/c/1") + assert_not EmbedProvider.allows?("https://b.example/b/1") + end + + test "a single provider object (not wrapped in an array) is accepted" do + ENV["WRITEBOOK_EMBED_PROVIDERS"] = + %({"name":"Wistia","hosts":["fast.wistia.net"],"path_prefix":"/embed/"}) + assert EmbedProvider.allows?("https://fast.wistia.net/embed/iframe/abc") + assert_includes EmbedProvider.csp_frame_sources, "https://fast.wistia.net" + end + + test "root-equivalent path prefixes are rejected and the table fails closed" do + %w[/ /. /./ // /embed/.. /embed/../ /embed/./.. embed].each do |prefix| + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"name":"x","hosts":["x.example"],"path_prefix":"#{prefix}"}]) + + assert EmbedProvider.configured?, prefix + assert_equal [ :none ], EmbedProvider.csp_frame_sources, prefix + assert_not EmbedProvider.allows?("https://x.example/anything"), prefix + assert_not EmbedProvider.allows?("https://x.example/./anything"), prefix + assert_not EmbedProvider.allows?("https://x.example//anything"), prefix + end + end + + test "a configured path prefix is stored in canonical form" do + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"name":"x","hosts":["x.example"],"path_prefix":"/embed//x/"}]) + + provider = EmbedProvider.match("https://x.example/embed/x/1") + assert_equal "/embed/x", provider.path_prefix + assert_not EmbedProvider.allows?("https://x.example/embed/xy") + assert_not EmbedProvider.allows?("https://x.example/embed/../x/1") + end + + test "wildcard, whitespace and over-broad config entries are rejected and the table fails closed" do + [ + %([{"name":"a","hosts":["*"],"path_prefix":"/e"}]), + %([{"name":"b","hosts":["*.example.com"],"path_prefix":"/e"}]), + %([{"name":"c","hosts":["x.example bad"],"path_prefix":"/e"}]), + %([{"name":"d","hosts":["x.example"],"path_prefix":"/"}]), + %([{"name":"e","hosts":["127.1"],"path_prefix":"/e"}]), + %([{"name":"f","hosts":["0x7f.1"],"path_prefix":"/e"}]) + ].each do |config| + ENV["WRITEBOOK_EMBED_PROVIDERS"] = config + # A fully-rejected entry leaves a configured but empty table — no wildcard, + # no whitespace, no IP-literal, and no path-prefix-of-"/" catch-all leaks + # in, and nothing falls back to a wider source. + assert_equal [ :none ], EmbedProvider.csp_frame_sources, config + assert_not EmbedProvider.allows?("https://x.example/anything"), config + end + end + + # --- fragment cache version tracks the effective policy ------------------- + + test "cache_version distinguishes permissive from configured and follows the table" do + configured = EmbedProvider.cache_version + assert_equal configured, EmbedProvider.cache_version + + accounts(:signal).update!(embed_providers: nil) + assert_equal "permissive", EmbedProvider.cache_version + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"name":"x","hosts":["x.example"],"path_prefix":"/e"}]) + with_host = EmbedProvider.cache_version + assert_not_equal configured, with_host + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"name":"x","hosts":["x.example"],"path_prefix":"/e","attributes":["src"]}]) + assert_not_equal with_host, EmbedProvider.cache_version + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"name":"x","hosts":["x.example"],"path_prefix":"/f"}]) + assert_not_equal with_host, EmbedProvider.cache_version + end + + test "cache_version follows resolution order, since match is first-match-wins" do + ENV["WRITEBOOK_EMBED_PROVIDERS"] = + %([{"hosts":["x.example"],"path_prefix":"/e","attributes":["src"]},{"hosts":["x.example"],"path_prefix":"/e"}]) + narrow_first = EmbedProvider.cache_version + assert_equal %w[src], EmbedProvider.match("https://x.example/e/1").attributes + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = + %([{"hosts":["x.example"],"path_prefix":"/e"},{"hosts":["x.example"],"path_prefix":"/e","attributes":["src"]}]) + assert_not_equal narrow_first, EmbedProvider.cache_version + assert_equal EmbedProvider::PERMITTED_ATTRIBUTES, EmbedProvider.match("https://x.example/e/1").attributes + end + + private + def capture_embed_provider_log + log = StringIO.new + capture = ActiveSupport::Logger.new(log) + Rails.logger.broadcast_to(capture) + yield + log.string + ensure + Rails.logger.stop_broadcasting_to(capture) + end +end diff --git a/test/models/first_run_test.rb b/test/models/first_run_test.rb index bd743e64..f72097ea 100644 --- a/test/models/first_run_test.rb +++ b/test/models/first_run_test.rb @@ -18,6 +18,22 @@ class FirstRunTest < ActiveSupport::TestCase end end + test "starts a new install on the curated embed providers" do + create_first_run_user + + assert_equal EmbedProvider::DEFAULTS, Account.sole.embed_providers + assert EmbedProvider.configured? + assert EmbedProvider.allows?("https://www.youtube.com/embed/abc") + assert_not EmbedProvider.allows?("https://anything.example/embed/abc") + end + + test "an install upgraded from before the setting stays permissive" do + Account.create!(name: "Upgraded") + + assert_nil Account.sole.embed_providers + assert_not EmbedProvider.configured? + end + test "creates a demo book" do assert_changes -> { Book.count }, to: 1 do create_first_run_user diff --git a/test/models/html_scrubber_test.rb b/test/models/html_scrubber_test.rb index af8c8711..605130bf 100644 --- a/test/models/html_scrubber_test.rb +++ b/test/models/html_scrubber_test.rb @@ -9,6 +9,30 @@ def render_and_scrub(markdown) scrub(ActionText::Markdown.renderer.call.render(markdown)) end + test "cache_version changes when the embed policy does" do + permissive = HtmlScrubber.cache_version + assert_includes permissive, EmbedProvider.cache_version + + ENV["WRITEBOOK_EMBED_PROVIDERS"] = %([{"hosts":["x.example"],"path_prefix":"/e"}]) + assert_not_equal permissive, HtmlScrubber.cache_version + end + + test "keeps an iframe from any origin while embeds are permissive, attributes still scrubbed" do + result = scrub(%()) + assert_includes result, %()), ")), ")) @@ -79,7 +103,7 @@ def render_and_scrub(markdown) end test "strips value-sensitive iframe attributes but keeps safe embed attributes" do - result = scrub(%()) + result = scrub(%()) assert_not_includes result, "allow=" assert_not_includes result, "referrerpolicy" assert_not_includes result, "sandbox" diff --git a/test/test_helper.rb b/test/test_helper.rb index 37e5e0a2..e3b9eb17 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -11,5 +11,10 @@ class TestCase fixtures :all include SessionTestHelper + + # Tests assert against the shipped provider table, so a per-install + # WRITEBOOK_EMBED_PROVIDERS in the shell must not leak in — nor out. + setup { @embed_providers_before = ENV.delete("WRITEBOOK_EMBED_PROVIDERS") } + teardown { ENV["WRITEBOOK_EMBED_PROVIDERS"] = @embed_providers_before } end end