vcard: a card with a NUL in it is not text
Sequel writes a card into its INSERT as a literal and SQLite's tokenizer stops at a NUL, so a card holding one failed the write with a Sequel::DatabaseError whose message carried the card's bytes to Sentry. A NUL is not in RFC 2425's VALUE-CHAR, so VCard.text? now asks for UTF-8 with no NUL: a PUT holding one is refused as valid-address-data, an upload as not text, and VCard.new raises without naming the bytes for anything else. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VAiyEheRBdzbBjZUDWZUPk
diff --git a/lib/pro_tacts/vcard.rb b/lib/pro_tacts/vcard.rb
index d8df766..9a3f63c 100644
--- a/lib/pro_tacts/vcard.rb
+++ b/lib/pro_tacts/vcard.rb
@@ -108,19 +108,34 @@ module ProTacts
"#{prefix}#{property.name}#{parameters.join}:"
end
- # A card is made of UTF-8 text, and refuses to be made of anything
- # else: bytes that are not valid UTF-8 raise here, at the boundary,
- # rather than leaking an ArgumentError out of whatever regex first
- # trips over them. Raising rather than answering, because no caller
- # is meant to be asking — a card's bytes come from a column that can
- # hold nothing else or from a body already decoded, and whether some
- # bytes are text at all is that decode's question, settled before
- # anything here sees them (ProTacts::Web's write_card). This is the
- # assertion under those callers, not a check any of them makes; the
- # store's bind holds the same line a third time below the card.
+ # Whether bytes are text a card can be made of: valid UTF-8, and no
+ # NUL. RFC 2425 section 5.8.3's VALUE-CHAR has no controls but
+ # whitespace in it, and the NUL is the one of them nothing here can
+ # carry: Sequel writes a card into its INSERT as a literal, SQLite's
+ # tokenizer stops at the NUL, and the error that raises carries the
+ # card's bytes in its message — to Sentry, which card content must
+ # never reach. The question the decodes ask (Web#write_card, the
+ # import's upload) before anything here sees the bytes.
+ #: (String bytes) -> bool
+ def self.text?(bytes)
+ bytes.valid_encoding? && !bytes.include?("\0")
+ end
+
+ # A card is made of text, and refuses to be made of anything else:
+ # bytes that are not (.text?) raise here, at the boundary, rather
+ # than leaking an ArgumentError out of whatever regex first trips
+ # over them or SQLite's out of the insert. Raising rather than
+ # answering, because no caller is meant to be asking — a card's
+ # bytes come from a column that can hold nothing else or from a body
+ # already decoded, and whether some bytes are text at all is that
+ # decode's question, settled before anything here sees them
+ # (ProTacts::Web's write_card). This is the assertion under those
+ # callers, not a check any of them makes; the store's bind holds the
+ # encoding a third time below the card. The message names neither
+ # the bytes nor where they went wrong, for Sentry's sake.
#: (String bytes, ?lines: Array[Parser::Line]?) -> void
def initialize(bytes, lines: nil)
- raise ArgumentError, "not valid UTF-8: a card is text" unless bytes.valid_encoding?
+ raise ArgumentError, "not text: a card is UTF-8 with no NUL" unless VCard.text?(bytes)
@bytes = bytes
@lines = lines if lines
diff --git a/lib/pro_tacts/web/dav.rb b/lib/pro_tacts/web/dav.rb
index 161c11b..9775637 100644
--- a/lib/pro_tacts/web/dav.rb
+++ b/lib/pro_tacts/web/dav.rb
@@ -399,7 +399,8 @@ module ProTacts
# label is checked honest in the same breath, because a
# force_encoding nobody validates is a lie every later reader
# inherits. That the bytes are not the UTF-8 the media type
- # promised is a fact about this body, not about any card, which is
+ # promised, or carry the NUL no card's text can (VCard.text?), is
+ # a fact about this body, not about any card, which is
# why it is settled here and VCard is never handed the question
# (CARDDAV:valid-address-data, RFC 6352 section 6.3.2.1). Left
# unsettled it would surface as a 500 out of SQLite on the insert,
@@ -407,7 +408,7 @@ module ProTacts
# Paths need no counterpart: Puma hands PATH_INFO over still
# percent-encoded, so an id off the wire is ASCII.
bytes = request.body.read.force_encoding(Encoding::UTF_8)
- return precondition(:valid_address_data) unless bytes.valid_encoding?
+ return precondition(:valid_address_data) unless VCard.text?(bytes)
# CARDDAV:valid-address-data again, on the card this time: the
# envelope RFC 2426 section 4 requires. That is the whole test. A
diff --git a/lib/pro_tacts/web/import.rb b/lib/pro_tacts/web/import.rb
index d417988..9d8941d 100644
--- a/lib/pro_tacts/web/import.rb
+++ b/lib/pro_tacts/web/import.rb
@@ -95,7 +95,7 @@ module ProTacts
# for the one other body this app reads: a multipart part arrives
# as binary.
bytes = upload.fetch(:tempfile).read.force_encoding(Encoding::UTF_8)
- return import_screen(notice: "#{name} is not UTF-8 text.") unless bytes.valid_encoding?
+ return import_screen(notice: "#{name} is not UTF-8 text.") unless VCard.text?(bytes)
begin
# A Monica export is read as the cards it composes, and from
diff --git a/test/pro_tacts/admin/test_import_pages.rb b/test/pro_tacts/admin/test_import_pages.rb
index 187ab0a..4ebe1cd 100644
--- a/test/pro_tacts/admin/test_import_pages.rb
+++ b/test/pro_tacts/admin/test_import_pages.rb
@@ -1165,6 +1165,17 @@ class AdminImportPagesTest < Minitest::Test
end
end
+ # A NUL is not text either (VCard.text?), and would leave nothing the
+ # store could write.
+ def test_a_file_holding_a_nul_stages_nothing
+ with_contacts({}) do |store|
+ upload(JANE.sub("Jane", "Ja\0ne"))
+
+ assert_includes last_response.body, "contacts.vcf is not UTF-8 text."
+ assert_empty store.changes
+ end
+ end
+
# A Monica export is read as the cards it composes, and is a walk like
# any other from there (Import::Monica).
def test_a_monica_export_opens_the_walk_on_its_contacts
diff --git a/test/pro_tacts/import/test_match.rb b/test/pro_tacts/import/test_match.rb
index 1860532..7d052e3 100644
--- a/test/pro_tacts/import/test_match.rb
+++ b/test/pro_tacts/import/test_match.rb
@@ -164,9 +164,9 @@ class ImportMatchTest < Minitest::Test
# Emails as a card might carry them: real addresses, ones sharing a
# local part or a domain, and the malformed — no @, nothing before it.
- def email = one_of(emails, sampled_from(["ada@example.com", "ADA@example.com", "@example.com", "ada", ""]), text(max_size: 8))
+ def email = one_of(emails, sampled_from(["ada@example.com", "ADA@example.com", "@example.com", "ada", ""]), text(max_size: 8, min_codepoint: 1))
- def phone = one_of(sampled_from(["+1 555 0100", "(555) 0100", "n/a", ""]), text(max_size: 8))
+ def phone = one_of(sampled_from(["+1 555 0100", "(555) 0100", "n/a", ""]), text(max_size: 8, min_codepoint: 1))
# A likeness is a share, between 0 for nothing alike and 1 for a value
# both share, whatever the cards hold — and so the ranking over them
@@ -174,8 +174,8 @@ class ImportMatchTest < Minitest::Test
def test_every_likeness_is_a_share
Hegel.test(test_cases: 1_000) do |tc|
lines = ->(name, values) { values.map { "#{name}:#{ProTacts::VCard.escape(it)}" } }
- arriving = contact("ada", tc.draw(text(max_size: 8)).gsub(/[\r\n]/, ""), *lines.("EMAIL", tc.draw(arrays(email, max_size: 2))), *lines.("TEL", tc.draw(arrays(phone, max_size: 2))))
- stored = contact("bob", tc.draw(text(max_size: 8)).gsub(/[\r\n]/, ""), *lines.("EMAIL", tc.draw(arrays(email, max_size: 2))), *lines.("TEL", tc.draw(arrays(phone, max_size: 2))))
+ arriving = contact("ada", tc.draw(text(max_size: 8, min_codepoint: 1)).gsub(/[\r\n]/, ""), *lines.("EMAIL", tc.draw(arrays(email, max_size: 2))), *lines.("TEL", tc.draw(arrays(phone, max_size: 2))))
+ stored = contact("bob", tc.draw(text(max_size: 8, min_codepoint: 1)).gsub(/[\r\n]/, ""), *lines.("EMAIL", tc.draw(arrays(email, max_size: 2))), *lines.("TEL", tc.draw(arrays(phone, max_size: 2))))
assert Match.likeness(arriving, stored).all? { (0.0..1.0).cover?(it) }, Match.likeness(arriving, stored).inspect
end
diff --git a/test/pro_tacts/import/test_vcf.rb b/test/pro_tacts/import/test_vcf.rb
index f635d82..de25279 100644
--- a/test/pro_tacts/import/test_vcf.rb
+++ b/test/pro_tacts/import/test_vcf.rb
@@ -362,7 +362,7 @@ class ImportVcfTest < Minitest::Test
# section 2.4.2 has a text value escaped, read back as the tags they
# were — blank ones and repeats aside, which name no group.
def test_categories_read_back_as_the_tags_written
- tag = one_of(text(max_size: 10), arrays(sampled_from([",", "\\", ";", " ", "a"]), max_size: 5).map(&:join))
+ tag = one_of(text(max_size: 10, min_codepoint: 1), arrays(sampled_from([",", "\\", ";", " ", "a"]), max_size: 5).map(&:join))
Hegel.test(test_cases: 1_000) do |tc|
tags = tc.draw(arrays(tag, min_size: 1, max_size: 5))
line = "CATEGORIES:#{tags.map { ProTacts::VCard.escape(it) }.join(",")}"
diff --git a/test/pro_tacts/test_card_diff.rb b/test/pro_tacts/test_card_diff.rb
index a0cf39a..f1df81d 100644
--- a/test/pro_tacts/test_card_diff.rb
+++ b/test/pro_tacts/test_card_diff.rb
@@ -80,7 +80,7 @@ class CardDiffTest < Minitest::Test
# carries some twice, beside lines drawn fresh.
COMMON = ["FN:Ada", "NOTE:one two", "TEL:+1-555-0100", "item1.EMAIL:ada@example.com"].freeze #: Array[String]
- def content = one_of(sampled_from(COMMON), from_regex('[A-Z]{1,4}:[^\r\n]{0,10}', fullmatch: true))
+ def content = one_of(sampled_from(COMMON), from_regex('[A-Z]{1,4}:[^\x00\r\n]{0,10}', fullmatch: true))
def contents = arrays(content, max_size: 8)
diff --git a/test/pro_tacts/test_store.rb b/test/pro_tacts/test_store.rb
index 38da5ec..c39d604 100644
--- a/test/pro_tacts/test_store.rb
+++ b/test/pro_tacts/test_store.rb
@@ -2526,10 +2526,8 @@ class StoreTest < Minitest::Test
# A card a write might carry: any name, and a TEL or two that may
# repeat, which a diff counts rather than sets. Any name but one
- # holding a NUL, which no write survives today: Sequel writes the
- # card into its INSERT as a literal, SQLite's tokenizer stops at the
- # NUL, and the write raises with the card's bytes in its message.
- # That is the store's open question, not this machine's.
+ # holding a NUL, which is not text a card can be made of
+ # (VCard.text?).
def card(tc, id)
name = ProTacts::VCard.escape(tc.draw(text(max_size: 10, min_codepoint: 1)))
phones = tc.draw(arrays(sampled_from(["TEL:+1 555 0100\r\n", "TEL:+1 555 0199\r\n"]), max_size: 3))
diff --git a/test/pro_tacts/test_vcard.rb b/test/pro_tacts/test_vcard.rb
index 256263e..f8573a3 100644
--- a/test/pro_tacts/test_vcard.rb
+++ b/test/pro_tacts/test_vcard.rb
@@ -195,7 +195,30 @@ class VCardTest < Minitest::Test
error = assert_raises(ArgumentError) { ProTacts::VCard.new(invalid) }
- assert_match "not valid UTF-8", error.message
+ assert_match "not text", error.message
+ end
+
+ # A NUL is no character a card is made of (RFC 2425's VALUE-CHAR),
+ # and the one the store cannot carry: wherever it falls, the bytes
+ # are not text, and the refusal's message carries none of them, so a
+ # report of it cannot leak the card into Sentry.
+ def test_a_nul_anywhere_makes_bytes_not_a_card
+ Hegel.test(test_cases: 1_000) do |tc|
+ text = tc.draw(text(max_size: 40, min_codepoint: 1))
+ at = tc.draw(integers(min_value: 0, max_value: text.length))
+ bytes = text.dup.insert(at, "\0")
+
+ error = assert_raises(ArgumentError) { ProTacts::VCard.new(bytes) }
+ assert_equal "not text: a card is UTF-8 with no NUL", error.message
+ end
+ end
+
+ # And everything else valid UTF-8 is text, controls and all: the
+ # refusal is the NUL's alone.
+ def test_utf_8_without_a_nul_is_text
+ Hegel.test(test_cases: 1_000) do |tc|
+ assert ProTacts::VCard.text?(tc.draw(text(max_size: 40, min_codepoint: 1)))
+ end
end
## The envelope
diff --git a/test/pro_tacts/test_web.rb b/test/pro_tacts/test_web.rb
index 1a1f60b..f7c1b09 100644
--- a/test/pro_tacts/test_web.rb
+++ b/test/pro_tacts/test_web.rb
@@ -534,6 +534,18 @@ class WebTest < Minitest::Test
end
end
+ # A NUL is not text a card is made of (VCard.text?), and refused
+ # here rather than at the insert that cannot hold it.
+ def test_put_of_a_card_holding_a_nul_names_its_precondition
+ with_contacts({}) do |store|
+ put_request "new", card("new", "Ne\0w"), "CONTENT_TYPE" => VCARD
+
+ assert_equal 412, last_response.status
+ assert_includes last_response.body, "<card:valid-address-data/>"
+ assert_empty store.changes
+ end
+ end
+
def test_put_of_something_that_is_not_a_card_names_its_precondition
with_contacts({}) do
put_request "new", "not a vCard at all", "CONTENT_TYPE" => VCARD
diff --git a/test/pro_tacts/vcard/test_lines.rb b/test/pro_tacts/vcard/test_lines.rb
index 46c084e..06ef6d9 100644
--- a/test/pro_tacts/vcard/test_lines.rb
+++ b/test/pro_tacts/vcard/test_lines.rb
@@ -53,7 +53,7 @@ class VCardLinesTest < Minitest::Test
error = assert_raises(ArgumentError) { VCARD.new(invalid) }
- assert_match "not valid UTF-8", error.message
+ assert_match "not text", error.message
end
# The envelope RFC 2426 section 4 requires, the one judgment a PUT
@@ -343,7 +343,7 @@ class VCardLinesTest < Minitest::Test
def content_line
composite { |dtc|
terminator = dtc.draw(sampled_from(["\r\n", "\n"]))
- value = dtc.draw(from_regex('[^\r\n]{0,12}', fullmatch: true))
+ value = dtc.draw(from_regex('[^\x00\r\n]{0,12}', fullmatch: true))
fold = dtc.draw(integers(min_value: 0, max_value: value.length))
folded = fold.zero? ? value : value.dup.insert(fold, "#{terminator} ")
dtc.draw(booleans(p: 0.1)) ? "garbage#{terminator}" : "#{dtc.draw(sampled_from(NAMES))}:#{folded}#{terminator}"
@@ -352,7 +352,7 @@ class VCardLinesTest < Minitest::Test
# Lines an edit hands in, bare or terminated, each naming `name`.
def edit_lines(name)
- line = tuples(from_regex('[^\r\n]{0,12}', fullmatch: true), sampled_from(["", "\r\n", "\n"]))
+ line = tuples(from_regex('[^\x00\r\n]{0,12}', fullmatch: true), sampled_from(["", "\r\n", "\n"]))
arrays(line, max_size: 3).map { |lines| lines.map { |value, terminator| "#{name}:#{value}#{terminator}" } }
end