Refuse REPORT types this server does not implement
An unsupported report used to fall through to the multiget branch and come
back as an empty 207, which reads as success and is not a status
UnhandledRequests records, so the ask left no trace to implement from.
Assisted-by: Claude Opus 5 via Claude Code
diff --git a/AGENTS.md b/AGENTS.md
index 498a48e..5fecc46 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -66,10 +66,10 @@ before regenerating.
a bare `curl` against `rake dev` is refused until you pass one. The
security of that rests on the app being reachable only through
`tailscale serve` — never bind it to anything but localhost.
-- Unanswered requests (404s and 5xx) are written to `log/unhandled` in the
- fixture layout. When implementing something a client asked for, look there
- first — and strip the identifying headers before promoting a capture into
- `test/fixtures`.
+- Unanswered requests (404s, app-level 403s, and 5xx) are written to
+ `log/unhandled` in the fixture layout. When implementing something a
+ client asked for, look there first — and strip the identifying headers
+ before promoting a capture into `test/fixtures`.
- Anything added to a request body must be assumed to reach Sentry. Card
content is redacted by `ProTacts::SentryScrubber`; a new kind of sensitive
field would need its own rule there.
diff --git a/README.md b/README.md
index 088a1cb..14478ed 100644
--- a/README.md
+++ b/README.md
@@ -42,11 +42,12 @@ while the app is reachable through serve alone — bind it to localhost.
Tailscale documents two cases that carry no identity and so cannot get in:
Funnel traffic, which is public, and traffic from tagged devices.
-Requests the server cannot answer — a 404, or a crash — are kept under
-`log/unhandled`, one directory per distinct request, in the same layout as
-`test/fixtures/macos-exchange`. A client asking for something unimplemented
-therefore leaves behind enough to implement it, and the capture can be
-promoted to a fixture by copying it and stripping the identifying headers.
+Requests the server cannot answer — a 404, a refused report, or a crash —
+are kept under `log/unhandled`, one directory per distinct request, in the
+same layout as `test/fixtures/macos-exchange`. A client asking for
+something unimplemented therefore leaves behind enough to implement it, and
+the capture can be promoted to a fixture by copying it and stripping the
+identifying headers.
Sentry gets the request body too, minus any card content, which
`ProTacts::SentryScrubber` redacts on the way out — hrefs and tailnet IPs
are not secrets, but the cards themselves never leave the machine.
diff --git a/lib/pro_tacts/unhandled_requests.rb b/lib/pro_tacts/unhandled_requests.rb
index c556b90..d064fcf 100644
--- a/lib/pro_tacts/unhandled_requests.rb
+++ b/lib/pro_tacts/unhandled_requests.rb
@@ -19,10 +19,14 @@ module ProTacts
# 404 is the missing-functionality signal: a client asked for something
# this server does not route. 5xx is kept because Sentry now reports
- # those without a body, and a crash is hard to read without one.
+ # those without a body, and a crash is hard to read without one. 403 is
+ # the routed-but-unimplemented case, today an unsupported REPORT type —
+ # safe to capture because TailscaleAuth sits above this middleware and
+ # refuses unauthenticated requests before they reach it, so every 403
+ # arriving here came from the app.
#: (Integer status) -> bool
def self.capture?(status)
- status == 404 || status >= 500
+ status == 403 || status == 404 || status >= 500
end
#: (Rack::_App app, directory: Pathname | String) -> void
diff --git a/lib/pro_tacts/web.rb b/lib/pro_tacts/web.rb
index 7b3aa4c..0887315 100644
--- a/lib/pro_tacts/web.rb
+++ b/lib/pro_tacts/web.rb
@@ -220,7 +220,11 @@ module ProTacts
root = doc.root
raise ArgumentError, "REPORT body is not XML" if root.nil?
- if root.name == "sync-collection"
+ # Each branch renders the whole response body: a Roda route
+ # block cannot return early, so the unsupported case has to be
+ # a value like the others rather than a return.
+ case root.name
+ when "sync-collection"
# DAV:sync-collection (RFC 6578 section 3.2). The warm-sync ask
# is etag-only; a changed etag sends the client back through
# multiget, so no address-data here.
@@ -231,13 +235,13 @@ module ProTacts
# contact with no token. macOS resyncs the whole collection
# anyway, so it works; a client that trusts the token would
# break. Fixing it is the incremental-sync work in the backlog.
- responses = addressbook.contacts.map { etag_response(it) }
- else
+ multistatus(addressbook.contacts.map { etag_response(it) })
+ when "addressbook-multiget"
# CARDDAV:addressbook-multiget (RFC 6352 section 8.7); the
# address-data the client asks for is section 10.4.
wants_cards = doc.xpath("//address-data").any?
- responses = doc.xpath("//href").map { it.text }.map { |requested|
+ multistatus(doc.xpath("//href").map { it.text }.map { |requested|
id = requested[%r{\A/dav/addressbook/([^/]+)\.vcf\z}, 1]
contact = id && addressbook.contacts.find { it.id == id }
@@ -246,15 +250,30 @@ module ProTacts
else
missing_response(requested)
end
- }
+ })
+ else
+ # The DAV:supported-report precondition on REPORT (RFC 3253
+ # section 3.6) — the report asked for has to be one the
+ # resource supports. Answering an unsupported report with an
+ # empty 207 reads to the client as a successful empty result,
+ # and to us as nothing at all: 207 is not a status
+ # UnhandledRequests captures, so the one signal that a client
+ # wanted something unimplemented never fired.
+ #
+ # 403 with the precondition named in a DAV:error body is the
+ # marshalling RFC 4918 section 16 defines, and 403 is its
+ # "will always fail, do not repeat" case. addressbook-query
+ # (RFC 6352 section 8.6) is the report this rejects today;
+ # macOS Contacts has never sent one.
+ response.status = 403
+
+ <<~XML
+ <?xml version="1.0" encoding="UTF-8"?>
+ <d:error xmlns:d="DAV:">
+ <d:supported-report/>
+ </d:error>
+ XML
end
-
- <<~XML
- <?xml version="1.0" encoding="UTF-8"?>
- <d:multistatus xmlns:d="DAV:" xmlns:card="urn:ietf:params:xml:ns:carddav">
- #{responses.join}
- </d:multistatus>
- XML
end
r.get String do |filename|
@@ -285,6 +304,16 @@ module ProTacts
@addressbook ||= Addressbook.load(ProTacts.config.contacts_dir)
end
+ #: (Array[String] responses) -> String
+ def multistatus(responses)
+ <<~XML
+ <?xml version="1.0" encoding="UTF-8"?>
+ <d:multistatus xmlns:d="DAV:" xmlns:card="urn:ietf:params:xml:ns:carddav">
+ #{responses.join}
+ </d:multistatus>
+ XML
+ end
+
#: (String id) -> String
def contact_href(id)
"/dav/addressbook/#{id}.vcf"
diff --git a/test/pro_tacts/test_unhandled_requests.rb b/test/pro_tacts/test_unhandled_requests.rb
index 4e3ea54..a5818d7 100644
--- a/test/pro_tacts/test_unhandled_requests.rb
+++ b/test/pro_tacts/test_unhandled_requests.rb
@@ -61,6 +61,17 @@ class UnhandledRequestsTest < Minitest::Test
assert_equal 1, captures.size
end
+ # An unsupported REPORT type. TailscaleAuth refuses unauthenticated
+ # requests above this middleware, so a 403 reaching it is always the app
+ # saying it routed the request and will not serve it.
+ def test_a_403_from_the_app_is_captured
+ @stub.status = 403
+
+ request "/dav/addressbook/", method: "REPORT", input: "<addressbook-query/>"
+
+ assert_equal 1, captures.size
+ end
+
def test_capture_records_the_request_verbatim
@stub.status = 404
diff --git a/test/pro_tacts/test_web.rb b/test/pro_tacts/test_web.rb
index 92b8a04..5bc18db 100644
--- a/test/pro_tacts/test_web.rb
+++ b/test/pro_tacts/test_web.rb
@@ -192,6 +192,17 @@ class WebTest < Minitest::Test
XML
end
+ def addressbook_query
+ <<~XML
+ <?xml version="1.0" encoding="UTF-8"?>
+ <C:addressbook-query xmlns:C="urn:ietf:params:xml:ns:carddav">
+ <A:prop xmlns:A="DAV:">
+ <A:getetag/>
+ </A:prop>
+ </C:addressbook-query>
+ XML
+ end
+
def test_listing_and_multiget_serve_every_contact_on_disk
with_contacts({
"aiden.kdl" => "name \"Aiden\"",
@@ -253,6 +264,31 @@ class WebTest < Minitest::Test
end
end
+ # RFC 6352 section 8.6 defines addressbook-query, which this server does
+ # not implement. Before the DAV:supported-report precondition was enforced
+ # it fell through to the multiget branch, found no hrefs, and answered with
+ # an empty 207 that read as a successful empty address book.
+ def test_unsupported_report_is_refused_rather_than_answered_emptily
+ directory = ProTacts.config.unhandled_dir
+ FileUtils.rm_rf(directory)
+
+ with_contacts({"aiden.kdl" => "name \"Aiden\""}) do
+ request "/dav/addressbook/", method: "REPORT", input: addressbook_query
+
+ assert_equal 403, last_response.status
+ assert_includes last_response.body, "<d:supported-report/>"
+ refute_includes last_response.body, "multistatus"
+
+ # The refusal is what makes the ask visible; an empty 207 left nothing.
+ captured = Pathname.new(directory).glob("*/request").map(&:read)
+
+ assert_equal 1, captured.size
+ assert_includes captured.first, "addressbook-query"
+ end
+ ensure
+ FileUtils.rm_rf(directory)
+ end
+
def test_etags_agree_across_listing_multiget_and_get
with_contacts({"aiden.kdl" => "name \"Aiden\""}) do
request "/dav/addressbook/", method: "PROPFIND", "HTTP_DEPTH" => "1", input: etag_only_propfind