Move the UTF-8 relabel from the store to the route
Store#text existed because Sequel literalized values into UTF-8 SQL and
a binary card raised on the way in; the sqlite3 adapter binds now, and
the gem encodes every bound value to UTF-8, so the store's contract
enforces itself and the relabel belongs at the one boundary where Rack
hands over binary.

Assisted-by: GLM-5.3 via pi
change krvztmqztvkzzykpxzzmqokxnroounpz
commit dd3235c02ddf34503114a8c6b442c4e086c0681a
author Alpha Chen <alpha@kejadlen.dev>
date
parent zvsztwkt
diff --git a/AGENTS.md b/AGENTS.md
index fc1c112..2646d17 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -112,10 +112,13 @@ not a fixture; edit a `.vcf` to change what the replay serves.
   to forget, and no way to serve from a database a deploy left behind.
   Never edit a migration that has run anywhere; add the next one.
 - Those tables are `STRICT`, so every string column needs `text: true`:
-  Sequel's plain `String` is a varchar, which STRICT refuses. Sequel
-  also literalizes values into UTF-8 SQL, so a card or an id that
-  arrived as ASCII-8BIT has to go through `Store#text` first or it
-  raises on the first non-ASCII byte.
+  Sequel's plain `String` is a varchar, which STRICT refuses. Strings
+  reach the store as UTF-8 by contract — `Web#utf8` relabels Rack's
+  binary (the request body, path segments carrying non-ASCII) where the
+  wire meets the route — and the adapter enforces it: the sqlite3 gem
+  encodes every bound value to UTF-8, so binary-flagged bytes above 7
+  bits raise at the bind, while bytes that are not UTF-8 at all are
+  refused by SQLite on the insert.
 - The app is handed its store rather than reaching for one:
   `config.ru` builds it and sets `ProTacts::Web.store`, and a test does
   the same with a throwaway. There is no global `ProTacts.store`, so
diff --git a/lib/pro_tacts/store.rb b/lib/pro_tacts/store.rb
index dd1494b..7315b80 100644
--- a/lib/pro_tacts/store.rb
+++ b/lib/pro_tacts/store.rb
@@ -129,7 +129,7 @@ module ProTacts
     # nil.
     #: (String id) -> Contact?
     def contact(id)
-      contact_from(cards.where(id: text(id)).sole)
+      contact_from(cards.where(id:).sole)
     rescue Sequel::NoMatchingRow
       nil
     end
@@ -143,7 +143,7 @@ module ProTacts
     # cards sharing a UID is a corruption to raise on, not a choice.
     #: (String uid) -> String?
     def card_id_with_uid(uid)
-      card_properties.where(name: "UID", value: text(uid)).sole.fetch(:card_id).to_s
+      card_properties.where(name: "UID", value: uid).sole.fetch(:card_id).to_s
     rescue Sequel::NoMatchingRow
       nil
     end
@@ -152,9 +152,16 @@ module ProTacts
     # change-log entry a client's sync token counts on, carrying the etag
     # this card hashed to now, and the index rows read off the card. One
     # transaction, because the log entry cannot be rebuilt from anything.
+    #
+    # The strings are UTF-8 by contract, and the adapter holds the store
+    # to it: the sqlite3 gem encodes every bound value to UTF-8, so a
+    # binary-flagged byte above 7 bits raises at the bind, and bytes
+    # that are not UTF-8 at all stop at the insert, which SQLite refuses
+    # to store as text. Rack's binary is relabelled before this —
+    # Web#utf8, where the wire meets the route.
     #: (String id, String vcard) -> Contact
     def put(id, vcard)
-      contact = Contact.for(id: text(id), vcard: text(vcard))
+      contact = Contact.for(id:, vcard:)
       @database.transaction do
         cards
           .insert_conflict(target: :id, update: {vcard: Sequel[:excluded][:vcard], updated_at: NOW})
@@ -169,7 +176,6 @@ module ProTacts
     # it is gone. Returns whether there was anything to remove.
     #: (String id) -> bool
     def delete(id)
-      id = text(id)
       @database.transaction do
         deleted = cards.where(id:).delete.positive?
         record(id, "delete", nil) if deleted
@@ -223,22 +229,6 @@ module ProTacts
       @database[:card_parameters]
     end
 
-    # Anything that came off the wire is ASCII-8BIT, and Sequel writes a
-    # value into UTF-8 SQL, so a card with a byte above 7 bits raises
-    # Encoding::UndefinedConversionError on the way in. Ids are ASCII by
-    # Contact::ID_FORMAT and cards go out as `text/vcard; charset=utf-8`,
-    # so this relabels bytes rather than converting them.
-    #
-    # Relabelling is not validating, and deliberately so — the card is
-    # kept as it arrived. Bytes that are not UTF-8 at all do not slip
-    # through, though: SQLite refuses them on the insert, which is where
-    # a card that cannot honestly be served as UTF-8 should stop. There
-    # is a test pinning that.
-    #: (String value) -> String
-    def text(value)
-      value.encoding == Encoding::BINARY ? value.dup.force_encoding(Encoding::UTF_8) : value
-    end
-
     #: (String card_id, String action, String? etag) -> void
     def record(card_id, action, etag)
       change_log.insert(card_id:, action:, etag:)
diff --git a/lib/pro_tacts/web.rb b/lib/pro_tacts/web.rb
index 80da52d..5d6bc0e 100644
--- a/lib/pro_tacts/web.rb
+++ b/lib/pro_tacts/web.rb
@@ -317,7 +317,7 @@ module ProTacts
           # Read on its own rather than through the collection: serving
           # one href has no reason to load every other contact first.
           r.get String do |filename|
-            contact = store.contact(filename.delete_suffix(".vcf"))
+            contact = store.contact(utf8(filename.delete_suffix(".vcf")))
 
             # No match falls through to the empty-body 404 that the
             # not_found handler fills in.
@@ -384,11 +384,7 @@ module ProTacts
     # CARDDAV:valid-address-data even when its If-Match is stale too.
     #: (String id) -> String
     def write_card(id)
-      vcard = request.body.read
-      # Rack reads a body in as ASCII-8BIT; relabel rather than convert,
-      # the way Store#text does, so the charset check and the insert see
-      # the same bytes.
-      vcard = vcard.dup.force_encoding(Encoding::UTF_8) if vcard.encoding == Encoding::BINARY
+      vcard = utf8(request.body.read)
 
       # CARDDAV:supported-address-data (RFC 6352 section 6.3.2.1): what
       # arrived must be a vCard, and text/vcard is the one media type
@@ -449,6 +445,18 @@ module ProTacts
       nil
     end
 
+    # Where the wire's bytes become the app's text. Rack hands the
+    # request body in as ASCII-8BIT, and path segments carrying non-ASCII
+    # likewise (its spec's rule for CGI values), while everything past
+    # the route — the store, the parser, the responses — speaks UTF-8.
+    # Relabelling is not converting: the bytes are untouched, and a body
+    # that is not UTF-8 at all is caught by the charset check in
+    # write_card rather than here.
+    #: (String wire) -> String
+    def utf8(wire)
+      wire.encoding == Encoding::BINARY ? wire.dup.force_encoding(Encoding::UTF_8) : wire
+    end
+
     # A 412 whose body names the CardDAV precondition that failed, in
     # the DAV:error form RFC 4918 section 16 defines; the element names
     # are RFC 6352 section 6.3.2.1's, qualified by the card: namespace
diff --git a/test/pro_tacts/test_store.rb b/test/pro_tacts/test_store.rb
index e991873..7b57536 100644
--- a/test/pro_tacts/test_store.rb
+++ b/test/pro_tacts/test_store.rb
@@ -121,19 +121,22 @@ class StoreTest < Minitest::Test
     end
   end
 
-  # An id and a card off the wire are ASCII-8BIT, and Sequel writes
-  # values into UTF-8 SQL: a card with an accent in it raises on the way
-  # in unless the bytes are relabelled first.
-  def test_binary_bytes_off_the_wire_are_stored_as_text
+  # The store's contract is UTF-8, and the adapter enforces it: the
+  # sqlite3 gem encodes every bound value to UTF-8, so a binary-flagged
+  # string with a byte above 7 bits raises at the bind — loudly, at the
+  # contract violation, rather than being quietly relabelled here. The
+  # route is where Rack's binary becomes text (Web#utf8).
+  def test_binary_bytes_above_ascii_raise_at_the_bind
     accented = AIDEN.sub("Aiden", "Aiden Åberg")
 
-    with_store do |store|
-      store.put("aiden".b, accented.b)
+    with_store({"aiden" => AIDEN}) do |store|
+      assert_raises(Encoding::UndefinedConversionError) do
+        store.put("aiden".b, accented.b)
+      end
 
-      assert_equal accented, store.contact("aiden".b).vcard
-      assert_equal accented, store.contact("aiden").vcard
-      assert_equal Encoding::UTF_8, store.contact("aiden").vcard.encoding
-      assert_equal ProTacts::Contact.etag_for(accented), store.contact("aiden").etag
+      # Pure ASCII carries no such byte, so a binary-flagged id still
+      # finds its row.
+      assert_equal AIDEN, store.contact("aiden".b).vcard
     end
   end
 
@@ -211,11 +214,14 @@ class StoreTest < Minitest::Test
   # Relabelling bytes is not converting them, so a card whose bytes are
   # not UTF-8 at all is refused rather than stored: better than serving
   # it back as `text/vcard; charset=utf-8` while it is no such thing.
-  # Pinned here so that a later "fix" to Store#text has to be deliberate.
+  # Relabelled first, as the route would — a request this bad never
+  # reaches the store, and this pins the last line that catches it.
   def test_a_card_that_is_not_utf_8_is_refused
     with_store do |store|
+      invalid = "BEGIN:VCARD\r\nFN:\xFF\xFE\r\nEND:VCARD\r\n".dup.force_encoding(Encoding::UTF_8)
+
       assert_raises(Sequel::DatabaseError) do
-        store.put("bad", "BEGIN:VCARD\r\nFN:\xFF\xFE\r\nEND:VCARD\r\n".b)
+        store.put("bad", invalid)
       end
 
       assert_empty store.contacts
diff --git a/test/pro_tacts/test_web.rb b/test/pro_tacts/test_web.rb
index cdf570e..d90400c 100644
--- a/test/pro_tacts/test_web.rb
+++ b/test/pro_tacts/test_web.rb
@@ -365,6 +365,24 @@ class WebTest < Minitest::Test
     end
   end
 
+  # A body arrives flagged ASCII-8BIT — Rack's rule for request input —
+  # and becomes UTF-8 where the route meets the wire, so a card with
+  # non-ASCII in it stores and serves as the UTF-8 it is.
+  def test_put_of_a_binary_flagged_body_stores_as_utf_8
+    card = "BEGIN:VCARD\r\nVERSION:3.0\r\nFN:Aiden Åberg\r\nUID:new\r\nEND:VCARD\r\n"
+
+    with_contacts({}) do
+      put_request "new", card.dup.force_encoding(Encoding::BINARY),
+        "CONTENT_TYPE" => VCARD, "HTTP_IF_NONE_MATCH" => "*"
+
+      assert_equal 201, last_response.status
+
+      get "/dav/addressbook/new.vcf"
+      assert_equal Encoding::UTF_8, last_response.body.encoding
+      assert_equal card, last_response.body
+    end
+  end
+
   # A last segment that is not the <id>.vcf shape can address no
   # resource here, created or read — the 404 the GET handler gives it.
   def test_put_to_a_uri_that_cannot_address_a_card