Send request bodies to Sentry, minus card content
Reverses the send_default_pii decision in the previous commit: hrefs carry
opaque UIDs and the IPs are tailnet addresses, so the body is worth having
on a 404. Card content is the exception a write path would introduce, so
before_send redacts it, including the case where Sentry truncated the body
at 16KB and left a BEGIN:VCARD with no END.

Assisted-by: Claude Opus 5 via Claude Code
change wklxumvpsrzuxqytmwxkyrptvuwppwno
commit 307045b61d92be4deb52ed993cb5021904483aa9
author Alpha Chen <alpha@kejadlen.dev>
date
parent wlzovsry
diff --git a/AGENTS.md b/AGENTS.md
index 7620b00..63ab759 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -68,6 +68,9 @@ before regenerating.
   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.
 - `config.ru` must stay ASCII-only; a test enforces it, because boot crashes
   under a C locale otherwise. Watch for em dashes in comments.
 - `RUBYOPT=--enable-frozen-string-literal` is set in `.ramekin/config.kdl`.
diff --git a/README.md b/README.md
index 9b6a898..088a1cb 100644
--- a/README.md
+++ b/README.md
@@ -47,7 +47,9 @@ Requests the server cannot answer — a 404, or a crash — are kept under
 `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 event without the request body.
+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.
 
 ## The minimal set macOS Contacts needs
 
diff --git a/config.ru b/config.ru
index 1c6a4d1..c6ab6b3 100644
--- a/config.ru
+++ b/config.ru
@@ -7,6 +7,7 @@ require "sentry-ruby"
 
 $LOAD_PATH.unshift(Pathname.new(__dir__) / "lib")
 require "pro_tacts/web"
+require "pro_tacts/sentry_scrubber"
 
 config = ProTacts.config
 
@@ -22,14 +23,15 @@ Sentry.init do |sentry|
   # Get breadcrumbs from logs
   sentry.breadcrumbs_logger = [:sentry_logger, :http_logger]
 
-  # Off keeps the request body, query string, cookies, and client IP out of
-  # events. None of that is sensitive today: a read-only server's request
-  # bodies carry opaque contact UIDs, not card content, and the IPs are
-  # tailnet addresses. It is off because log/unhandled keeps the same bodies
-  # in better shape (see ProTacts::UnhandledRequests), and because a write
-  # path would put whole vCards in them. Headers still go, including
-  # Tailscale-User-Login, which says who hit the 404.
-  sentry.send_default_pii = false
+  # On: request bodies are worth having on a 404, and nothing else this
+  # sends is sensitive. Hrefs carry opaque contact UIDs, not names, and the
+  # IPs are tailnet addresses.
+  sentry.send_default_pii = true
+
+  # The one thing that must not leave the machine is card content, which a
+  # write path would put directly in a PUT body. Full bodies are kept
+  # locally either way, see ProTacts::UnhandledRequests.
+  sentry.before_send = ProTacts::SentryScrubber
 
   # Trace all the things!
   sentry.traces_sample_rate = 1.0
diff --git a/lib/pro_tacts/sentry_scrubber.rb b/lib/pro_tacts/sentry_scrubber.rb
new file mode 100644
index 0000000..b2961e7
--- /dev/null
+++ b/lib/pro_tacts/sentry_scrubber.rb
@@ -0,0 +1,56 @@
+module ProTacts
+  # Redacts card content from request bodies on their way to Sentry.
+  #
+  # Everything else in a request is fine to send: hrefs carry opaque contact
+  # UIDs, and the IPs are tailnet addresses. Card content is the exception,
+  # and a write path would put it straight in a PUT body.
+  #
+  # Sentry truncates bodies at 16KB (RequestInterface::MAX_BODY_LIMIT), so a
+  # card can arrive with its BEGIN and no END. The trailing alternation
+  # redacts to the end of the body in that case rather than missing the
+  # match; a card with an END redacts only to its own END, leaving the rest
+  # of the body intact.
+  module SentryScrubber
+    # (?~exp) is Ruby's absence operator: the run of characters not
+    # containing exp. Says "up to the first END:VCARD" more directly than a
+    # lazy quantifier, whose stopping point depends on alternation order.
+    VCARD = /BEGIN:VCARD(?~END:VCARD)(?:END:VCARD|\z)/mi
+    VCARD_CONTENT_TYPE = %r{\Atext/vcard}i
+    REDACTED = "[vcard redacted]".freeze
+
+    # Sentry discards the event unless a Sentry::ErrorEvent comes back, so
+    # this returns the event it was handed either way.
+    def self.call(event, _hint = nil)
+      request = event.request if event.respond_to?(:request)
+      return event unless request
+
+      request.data =
+        if vcard_body?(request)
+          REDACTED
+        else
+          redact(request.data)
+        end
+
+      event
+    end
+
+    # A card PUT is a card whether or not it parses, so the content type is
+    # enough on its own.
+    def self.vcard_body?(request)
+      headers = request.headers
+      return false unless headers.respond_to?(:[])
+
+      headers["Content-Type"].to_s.match?(VCARD_CONTENT_TYPE)
+    end
+
+    # Form bodies arrive as a params hash rather than a string.
+    def self.redact(data)
+      case data
+      when String then data.gsub(VCARD, REDACTED)
+      when Hash then data.transform_values { redact(it) }
+      when Array then data.map { redact(it) }
+      else data
+      end
+    end
+  end
+end
diff --git a/test/pro_tacts/test_sentry_scrubber.rb b/test/pro_tacts/test_sentry_scrubber.rb
new file mode 100644
index 0000000..a99d42a
--- /dev/null
+++ b/test/pro_tacts/test_sentry_scrubber.rb
@@ -0,0 +1,125 @@
+require_relative "../test_helper"
+require "sentry-ruby"
+
+require "pro_tacts/sentry_scrubber"
+
+class SentryScrubberTest < Minitest::Test
+  CARD = <<~VCARD.chomp
+    BEGIN:VCARD
+    VERSION:3.0
+    UID:AB12C345
+    FN:Real Person
+    TEL:+1-555-1234
+    END:VCARD
+  VCARD
+
+  # Stands in for a Sentry event: the scrubber only reaches for #request,
+  # and the request only needs a body and headers.
+  Event = Struct.new(:request)
+  Request = Struct.new(:data, :headers)
+
+  def scrub(data, headers = {})
+    event = Event.new(Request.new(data, headers))
+    ProTacts::SentryScrubber.call(event, nil)
+    event.request.data
+  end
+
+  def test_a_card_is_redacted
+    refute_includes scrub(CARD), "Real Person"
+  end
+
+  def test_a_card_is_replaced_rather_than_emptied
+    assert_includes scrub(CARD), "vcard redacted"
+  end
+
+  # The redaction has to stop at END:VCARD, not run to the end of the body,
+  # or a card anywhere in a request would take the rest of it along.
+  def test_a_card_embedded_in_xml_is_redacted_without_eating_what_follows
+    scrubbed = scrub("<address-data>#{CARD}</address-data><href>/keep/me</href>")
+
+    refute_includes scrubbed, "Real Person"
+    assert_includes scrubbed, "<address-data>"
+    assert_includes scrubbed, "</address-data>"
+    assert_includes scrubbed, "<href>/keep/me</href>"
+  end
+
+  def test_content_between_two_cards_survives
+    scrubbed = scrub("#{CARD}\nMIDDLE\n#{CARD.sub('Real Person', 'Other Person')}\nTRAILING")
+
+    refute_includes scrubbed, "Real Person"
+    refute_includes scrubbed, "Other Person"
+    assert_includes scrubbed, "MIDDLE"
+    assert_includes scrubbed, "TRAILING"
+  end
+
+  # Only the unterminated card runs to the end; the complete one before it
+  # still stops at its own END:VCARD.
+  def test_a_complete_card_before_a_truncated_one_stops_at_its_end
+    scrubbed = scrub("#{CARD}\nMIDDLE\nBEGIN:VCARD\nFN:Trunc")
+
+    refute_includes scrubbed, "Real Person"
+    refute_includes scrubbed, "Trunc"
+    assert_includes scrubbed, "MIDDLE"
+  end
+
+  def test_several_cards_are_all_redacted
+    scrubbed = scrub("#{CARD}\n#{CARD.sub('Real Person', 'Other Person')}")
+
+    refute_includes scrubbed, "Real Person"
+    refute_includes scrubbed, "Other Person"
+  end
+
+  # Sentry truncates at 16KB, so a card can arrive without its END line.
+  def test_a_truncated_card_is_redacted
+    refute_includes scrub("BEGIN:VCARD\nVERSION:3.0\nFN:Real Pers"), "Real Pers"
+  end
+
+  def test_lowercase_markers_are_redacted
+    refute_includes scrub(CARD.downcase), "real person"
+  end
+
+  # The href in a multiget names a contact by UID, which is not a secret and
+  # is the useful part of a 404 report.
+  def test_hrefs_are_left_alone
+    body = "<href>/dav/addressbook/AB12C345-6789.vcf</href>"
+
+    assert_equal body, scrub(body)
+  end
+
+  def test_a_vcard_content_type_redacts_the_whole_body
+    scrubbed = scrub("garbled but still a card", "Content-Type" => "text/vcard; charset=utf-8")
+
+    assert_equal ProTacts::SentryScrubber::REDACTED, scrubbed
+  end
+
+  def test_form_bodies_are_walked
+    scrubbed = scrub({ "card" => CARD, "id" => "AB12C345" })
+
+    refute_includes scrubbed.fetch("card"), "Real Person"
+    assert_equal "AB12C345", scrubbed.fetch("id")
+  end
+
+  def test_a_nil_body_is_left_alone
+    assert_nil scrub(nil)
+  end
+
+  def test_an_event_without_a_request_is_returned_unchanged
+    event = Event.new(nil)
+
+    assert_same event, ProTacts::SentryScrubber.call(event, nil)
+  end
+
+  # Guards the field names against a sentry-ruby upgrade: this is the real
+  # interface Sentry builds from a Rack env, not a stand-in.
+  def test_it_scrubs_a_real_sentry_request_interface
+    env = Rack::MockRequest.env_for("/dav/addressbook/", method: "PUT", input: CARD)
+    env["CONTENT_TYPE"] = "text/xml"
+    request = Sentry::RequestInterface.new(env: env, send_default_pii: true, rack_env_whitelist: [])
+
+    assert_includes request.data, "Real Person", "precondition: Sentry captured the body"
+
+    ProTacts::SentryScrubber.call(Event.new(request), nil)
+
+    refute_includes request.data, "Real Person"
+  end
+end