Use Roda's sec_fetch_site_csrf in place of CrossSite
Roda ships the Sec-Fetch-Site check as a plugin, so the hand-written
module goes. allow_missing keeps DAV clients and curl, which send no
Sec-Fetch-Site, working. What goes with it is the Origin fallback for
browsers too old to send the header, which the family's are not.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013VNdx5qiRSWADBoVfDRcrd
diff --git a/AGENTS.md b/AGENTS.md
index 90dccc8..e02b0aa 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -95,9 +95,10 @@ no `sync:*` group; a test that needs everyone's book makes it.
for the fixture book in `demo.ru` and nowhere else: never in
`config.ru`, and never in front of real data.
- That identity is ambient, so a page on another site could submit a
- form as whoever's device opened it. `CrossSite` refuses a write the
- browser marks as cross-site before any route runs; a form's POST
- from this app's own pages is same-origin and passes.
+ form as whoever's device opened it. Roda's `sec_fetch_site_csrf`
+ refuses a write the browser marks as cross-site before any route
+ runs; a form's POST from this app's own pages is same-origin and
+ passes, and so does a DAV client, which sends no Sec-Fetch-Site.
- Failed DAV exchanges are written whole to `log/exchange.log` under the
id Sentry carries as the `exchange` tag. Look there first when a
client asks for something, and promote one with
diff --git a/lib/pro_tacts/cross_site.rb b/lib/pro_tacts/cross_site.rb
deleted file mode 100644
index 50739fc..0000000
--- a/lib/pro_tacts/cross_site.rb
+++ /dev/null
@@ -1,51 +0,0 @@
-require "uri"
-
-module ProTacts
- # Whether a request is a write another site's page made the browser
- # send. The identity is ambient — the proxy writes the login for
- # whichever tailnet device is asking (ProxyAuth), so no cookie is
- # involved and SameSite has nothing to hold back — and a page open on
- # a family member's device can submit a form here, which the browser
- # sends without a preflight and the app would apply as that member.
- # DAV's PUT and DELETE already need a preflight nothing answers, so
- # it is the admin's POSTs this refuses in practice; every method that
- # changes something is asked, so a verb added later is covered too.
- #
- # The browser says where a request came from: Sec-Fetch-Site on every
- # browser that sends it, and Origin on the older ones that do not.
- # A request carrying neither is no browser's — a DAV client, curl —
- # and passes, being nothing a page could have sent.
- module CrossSite
- # The methods that change nothing, and so have nothing to forge.
- READS = %w[GET HEAD OPTIONS PROPFIND REPORT].freeze #: Array[String]
-
- # Sec-Fetch-Site's answers for a request this site's own page sent,
- # or one a person typed or bookmarked (`none`).
- OWN = %w[same-origin none].freeze #: Array[String]
-
- #: (Rack::env env) -> bool
- def self.write?(env)
- return false if READS.include?(env["REQUEST_METHOD"])
-
- site = env["HTTP_SEC_FETCH_SITE"]
- return !OWN.include?(site) unless site.nil?
-
- origin = env["HTTP_ORIGIN"]
- return false if origin.nil?
-
- foreign?(origin, env["HTTP_HOST"].to_s)
- end
-
- # An Origin naming a host other than the one asked, `null` (an
- # opaque origin, a sandboxed frame's) included, whose URI has no
- # host at all. Both are read as URIs so the port falls away from
- # each the same way. One that does not parse is no page of ours
- # either.
- #: (String origin, String host) -> bool
- def self.foreign?(origin, host)
- URI(origin).host != URI("//#{host}").host
- rescue URI::InvalidURIError
- true
- end
- end
-end
diff --git a/lib/pro_tacts/web.rb b/lib/pro_tacts/web.rb
index 34b2e0c..e55dde9 100644
--- a/lib/pro_tacts/web.rb
+++ b/lib/pro_tacts/web.rb
@@ -8,7 +8,6 @@ require "rack/rewindable_input"
require "roda"
require "pro_tacts/contact"
-require "pro_tacts/cross_site"
require "pro_tacts/exchange_log"
require "pro_tacts/refusal_alerts"
require "pro_tacts/store"
@@ -91,6 +90,15 @@ module ProTacts
# it hands an attacker nothing: the proxy overwrites the header on
# every request, so knowing which one it is buys no way to forge
# it.
+ # The login is ambient — the proxy writes it for whichever tailnet
+ # device is asking — so no cookie is involved and SameSite holds
+ # nothing back: a page on another site could submit a form here and
+ # the app would apply it as whoever's device opened it. A write the
+ # browser marks as anything but same-origin gets an empty 403. One
+ # carrying no Sec-Fetch-Site at all passes, because that is every
+ # DAV client and curl, and no page a current browser sends.
+ plugin :sec_fetch_site_csrf, allow_missing: true, csrf_failure: :empty_403
+
plugin :error_handler, classes: [ProxyAuth::MissingLogin] do |_e|
response.status = 401
response["WWW-Authenticate"] = "Proxy-Identity"
@@ -109,14 +117,9 @@ module ProTacts
# nobody and the error handler above answers it.
@login = ProxyAuth.login(r.env)
- # A write another site's page sent, refused before any route
- # can apply it (CrossSite).
- if CrossSite.write?(r.env)
- response.status = 403
- response["Content-Type"] = "text/plain"
- response.write("Forbidden: a write from another site's page.\n")
- r.halt
- end
+ # A write another site's page sent, refused before any route can
+ # apply it (the plugin's comment above).
+ check_sec_fetch_site!
r.public
r.hash_branches
diff --git a/sig/gems/roda.rbs b/sig/gems/roda.rbs
index e235b49..5282214 100644
--- a/sig/gems/roda.rbs
+++ b/sig/gems/roda.rbs
@@ -26,6 +26,10 @@ class Roda
def self.hash_branch: (String segment) { (RodaRequest) [self: instance] -> untyped } -> void
| (String | Symbol namespace, String segment) { (RodaRequest) [self: instance] -> untyped } -> void
+ # The sec_fetch_site_csrf plugin's: halts a write whose
+ # Sec-Fetch-Site the plugin was not configured to allow.
+ def check_sec_fetch_site!: () -> void
+
def request: () -> RodaRequest
def response: () -> RodaResponse
diff --git a/test/pro_tacts/test_web.rb b/test/pro_tacts/test_web.rb
index 6a7f477..1a1f60b 100644
--- a/test/pro_tacts/test_web.rb
+++ b/test/pro_tacts/test_web.rb
@@ -229,34 +229,25 @@ class WebTest < Minitest::Test
end
end
- # A browser too old to send Sec-Fetch-Site still sends Origin.
- def test_a_write_from_another_origin_is_refused
- header "Origin", "https://evil.example"
+ # A page on a sibling subdomain is another site's too.
+ def test_a_same_site_write_is_refused
+ header "Sec-Fetch-Site", "same-site"
post "/contacts", first: "Mallory"
assert_equal 403, last_response.status
end
- def test_a_write_from_this_origin_is_applied
- with_contacts({}) do
- header "Origin", "http://example.org"
-
+ # No Sec-Fetch-Site is every DAV client, whose write goes through.
+ def test_a_write_without_sec_fetch_site_is_applied
+ with_contacts({}) do |store|
post "/contacts", first: "Grace"
assert_equal 303, last_response.status
+ assert_equal ["Grace"], store.contacts.map(&:name)
end
end
- # A sandboxed frame's opaque origin names no host at all.
- def test_a_write_from_an_opaque_origin_is_refused
- header "Origin", "null"
-
- post "/contacts", first: "Mallory"
-
- assert_equal 403, last_response.status
- end
-
# A cross-site read changes nothing, and a link followed from
# elsewhere is how a person reaches the dashboard.
def test_a_cross_site_read_is_answered