vcard: a line carries one property, not a list
The array existed for a bare CR packing two content lines into one
line's bytes, a shape no recorded macOS session sends
(test/fixtures/macos-exchange); Parser now refuses it and Store reports
it, so the assumption is checked rather than held. The unfold comment's
claim that Contacts folds at 75 octets goes with it — the same capture
shows a 443-octet line sent unbroken.
Assisted-by: Claude Opus 5 via Claude Code
diff --git a/db/migrations/002_birthdays.rb b/db/migrations/002_birthdays.rb
index 2b136f6..dac5f5f 100644
--- a/db/migrations/002_birthdays.rb
+++ b/db/migrations/002_birthdays.rb
@@ -44,12 +44,11 @@ Sequel.migration do
# card's BDAY lines take — only the modeled single line moves.
case ProTacts::VCard.new(row.fetch(:vcard)).lines.partition { it.names?("BDAY") }
in [[line], others]
- # Only a line holding exactly one BDAY property can move it
- # out: the line's bytes move as one, and a lone CR can pack a
- # second content line into them.
- next unless line.properties.one?
- property = line.properties.fetch(0)
- next unless property.name.casecmp?("BDAY")
+ # Only a line that reads as a BDAY can move it out: the line's
+ # bytes move as one, and a line holding anything else — or
+ # nothing this parser will read — stays where it is.
+ property = line.property
+ next unless property&.name&.casecmp?("BDAY")
birthday = ProTacts::Birthday.from_property(property)
next if birthday.nil?
diff --git a/lib/pro_tacts/store.rb b/lib/pro_tacts/store.rb
index 12238d6..8872724 100644
--- a/lib/pro_tacts/store.rb
+++ b/lib/pro_tacts/store.rb
@@ -219,9 +219,11 @@ module ProTacts
# see was the user's deletion, an unseen model row survives as
# nothing a client ever saw, and what nobody recognizes is
# reported rather than lost in silence.
+ card = VCard.new(vcard)
+ report_broken_assumptions(card)
existing = birthday_of(id)
birthday, stored =
- case VCard.new(vcard).lines.partition { it.names?("BDAY") }
+ case card.lines.partition { it.names?("BDAY") }
in [[line], others]
report_unrecognized_bday_lines([line])
property = bday_of(line)
@@ -234,7 +236,7 @@ module ProTacts
carried, lost = carried_and_lost_bday_lines(stored_vcard(id))
kept = existing && !existing.served? ? existing : nil
report_lost_bday_lines(lost)
- [kept, VCard.new(vcard).insert(carried)]
+ [kept, card.insert(carried)]
in [lines, _]
# More than one BDAY: cardinality-broken data, kept verbatim
# and reported like any other unrecognized line.
@@ -380,19 +382,17 @@ module ProTacts
row && row.fetch(:vcard).to_s
end
- # The BDAY a decision may be made of: the line's one property,
- # when the line holds exactly one and it names BDAY. Found by name,
- # not position, and only when alone, because the line is the unit a
- # rewrite or a move to the model carries — a BDAY sharing its bytes
- # with another content line (a lone CR packs two into one physical
- # line) cannot leave without taking the rest with it. Nil for any
- # other shape, the empty line a failed read leaves included.
+ # The BDAY a decision may be made of: the line's property, when it
+ # has one and it names BDAY. Found by name rather than position,
+ # and safe to act on because the line is the unit a rewrite or a
+ # move to the model carries and a line holds at most one property
+ # (VCard::Line) — so moving its bytes moves this and nothing else.
+ # Nil for any other shape, the empty line a failed read leaves
+ # included.
#: (VCard::Line line) -> VCard::Property?
def bday_of(line)
- return nil unless line.properties.one?
-
- property = line.properties.fetch(0)
- property.name.casecmp?("BDAY") ? property : nil
+ property = line.property
+ property if property&.name&.casecmp?("BDAY")
end
# A stored card's BDAY lines in two piles: the verbatim lines a
@@ -425,6 +425,11 @@ module ProTacts
#: (Array[VCard::Line] lines) -> void
def report_unrecognized_bday_lines(lines)
unrecognized = lines.count { |line|
+ # A line that broke a parser assumption is not an unrecognized
+ # BDAY, it is a line this server declined to read at all —
+ # report_broken_assumptions says so, and more precisely.
+ next false if line.error.is_a?(VCard::Parser::BrokenAssumption)
+
property = bday_of(line)
property.nil? || (!Birthday.rendered?(property) && !Birthday.unrendered_value?(property.value))
}
@@ -449,6 +454,25 @@ module ProTacts
)
end
+ # The assumption report, louder than the two above it: a submitted
+ # card that breaks one of the parser's assumptions
+ # (VCard::Parser::BrokenAssumption) is not bad input, it is news
+ # that the assumption is wrong and every simplification resting on
+ # it is now suspect. Nothing else would say so — the card is stored
+ # and served either way, and only its index entry is lost. The
+ # message carries no card content (ProTacts::SentryScrubber's
+ # line); the admin view shows the card raw.
+ #: (VCard card) -> void
+ def report_broken_assumptions(card)
+ broken = card.lines.count { it.error.is_a?(VCard::Parser::BrokenAssumption) }
+ return if broken.zero?
+
+ Sentry.capture_message(
+ "a submitted card broke #{broken} parser assumption(s) about what macOS Contacts sends",
+ level: :error,
+ )
+ end
+
# The etag comes back out of the card rather than out of a column, so
# a row can never carry one that disagrees with the bytes beside it.
# The card is composed with its birthday first, so both halves
diff --git a/lib/pro_tacts/vcard.rb b/lib/pro_tacts/vcard.rb
index e7b659a..58fb859 100644
--- a/lib/pro_tacts/vcard.rb
+++ b/lib/pro_tacts/vcard.rb
@@ -3,7 +3,7 @@ module ProTacts
# they came from, so a caller asks questions of either without
# re-parsing, and moves a property without a byte around it moving
# too. #lines walks the card once into Line values — a logical line's
- # properties beside the exact bytes that carried them — and is the
+ # property beside the exact bytes that carried it — and is the
# enumeration the questions that move bytes are asked over;
# #properties folds the same walk down to structure, and #insert
# puts lines back in. vCard 3.0 (RFC 2426): escape and fold, the
@@ -51,16 +51,21 @@ module ProTacts
Property = Data.define(:group, :name, :parameters, :value)
# One logical line of a card, parsed beside the exact bytes that
- # carried it: `properties` is every content line the logical line
- # read (usually one — a lone CR can end one mid-physical-line, and
- # a blank line reads as none), `error` the ParseError the first
- # failure raised, nil when it read, and `verbatim` the line itself,
- # folds and terminator included, so what moves between cards moves
- # unchanged.
+ # carried it: `property` is the content line it read, nil when it
+ # read none (a blank line, or one that would not read), `error` the
+ # ParseError the failure raised, nil when it read, and `verbatim`
+ # the line itself, folds and terminator included, so what moves
+ # between cards moves unchanged.
+ #
+ # One property, not a list of them: a logical line can only hold a
+ # second content line when a bare CR ends the first, and Parser
+ # refuses that outright (Parser::BrokenAssumption). That is what
+ # lets a caller move a line's bytes knowing it moves exactly one
+ # property.
#
# The signature lives in sig/pro_tacts/vcard.rbs with Property's.
# @rbs skip
- Line = Data.define(:properties, :verbatim, :error)
+ Line = Data.define(:property, :verbatim, :error)
# Reopened rather than defined in the block above: steep reads a
# define block's self as this class's, not the constant's, so the
@@ -145,7 +150,7 @@ module ProTacts
def properties
return @properties if defined?(@properties)
- @properties = lines.any? { it.error } ? nil : lines.flat_map { it.properties }
+ @properties = lines.any? { it.error } ? nil : lines.filter_map { it.property }
end
# Whether the parsed properties carry the envelope RFC 2426 section
diff --git a/lib/pro_tacts/vcard/parser.rb b/lib/pro_tacts/vcard/parser.rb
index 28a86e6..caca4f6 100644
--- a/lib/pro_tacts/vcard/parser.rb
+++ b/lib/pro_tacts/vcard/parser.rb
@@ -27,6 +27,19 @@ module ProTacts
class Parser
# @rbs @scanner: StringScanner
+ # Something this parser was built to assume it would never read.
+ # Not a judgment on the card: the assumptions are about macOS
+ # Contacts, the only client this address book serves
+ # (test/fixtures/macos-exchange is the evidence for each), and a
+ # card can break one while being a perfectly good vCard. Raising
+ # is how the assumption gets checked instead of merely held.
+ #
+ # A ParseError so it rides the same rails as any card that will
+ # not read — the bytes are still served, an index entry is what
+ # is lost — and its own class so Store can report it, a wrong
+ # assumption being news rather than ordinary bad input.
+ class BrokenAssumption < ParseError; end
+
# The content line, RFC 2426 section 4:
#
# contentline = [group "."] name *(";" param) ":" value CRLF
@@ -70,7 +83,7 @@ module ProTacts
error = lines.filter_map { it.error }.first
raise error if error
- lines.flat_map { it.properties }
+ lines.filter_map { it.property }
end
# The card's logical lines, each parsed beside the exact bytes it
@@ -85,24 +98,30 @@ module ProTacts
# VCard.fold's inverse, and the first thing a read does: RFC 2426
# section 2.6 has a content line unfolded before it is read. It
- # runs over the whole line rather than token by token because a
- # fold can land anywhere in it — Contacts folds at 75 octets
- # without regard for what it splits — so there is no boundary to
- # do it at.
+ # runs over the whole line rather than token by token because the
+ # RFC puts no constraint on where a fold lands — mid-token and
+ # mid-escape included — so there is no boundary to do it at.
+ #
+ # The recorded macOS session folds nothing: it sent a 443-octet
+ # X-ADDRESSING-GRAMMAR line and an 81-octet ADR unbroken, both
+ # past the 75-octet limit, with no continuation in the card
+ # (test/fixtures/macos-exchange/10-put-contact-edit). That is one
+ # capture of one card, and the PHOTO bodies that would settle it
+ # went unpromoted, so this stays general rather than becoming
+ # another macOS assumption.
#: (String line) -> String
def self.unfold(line)
line.gsub(FOLD, "")
end
- # One logical line into a Line: every content line it read, or
- # the error the first failure raised — the prefix that did read
- # is dropped to match parse, which declines a line wholesale by
- # raising. A blank line is neither: no properties, no error.
+ # One logical line into a Line: the content line it read, or the
+ # error the failure raised. A blank line is neither: no property,
+ # no error.
#: (String logical_line) -> Line
def self.line_of(logical_line)
- Line.new(properties: new(logical_line).parse, verbatim: logical_line, error: nil)
+ Line.new(property: new(logical_line).parse, verbatim: logical_line, error: nil)
rescue ParseError => error
- Line.new(properties: [], verbatim: logical_line, error:)
+ Line.new(property: nil, verbatim: logical_line, error:)
end
# The card's logical lines: a physical line and the continuations
@@ -134,18 +153,23 @@ module ProTacts
@scanner = StringScanner.new(self.class.unfold(logical_line))
end
- # Every content line in this logical line, in order — usually one,
- # though a lone CR can end a content line mid-physical-line, and a
- # blank line has none.
- #: () -> Array[Property]
+ # The one content line this logical line carries, or nil when it
+ # carries none — a blank line reads as nothing.
+ #
+ # A second content line means a bare CR ended the first (see
+ # LINE_BREAK), and that is refused rather than read: it is a line
+ # ending no recorded macOS session sends, and reading it would
+ # leave a property whose bytes cannot move without taking another
+ # property's along.
+ #: () -> Property?
def parse
- properties = [] #: Array[Property]
- until @scanner.eos?
- next if @scanner.skip(LINE_BREAK)
+ nil while @scanner.skip(LINE_BREAK)
+ return nil if @scanner.eos?
+
+ property = content_line
+ return property if @scanner.eos?
- properties << content_line
- end
- properties
+ raise BrokenAssumption, "a bare CR packed a second content line into one line #{here}"
end
private
diff --git a/sig/pro_tacts/vcard.rbs b/sig/pro_tacts/vcard.rbs
index 5be6970..c760424 100644
--- a/sig/pro_tacts/vcard.rbs
+++ b/sig/pro_tacts/vcard.rbs
@@ -13,11 +13,11 @@ module ProTacts
end
class Line
- attr_reader properties: Array[Property]
+ attr_reader property: Property?
attr_reader verbatim: String
attr_reader error: ParseError?
- def self.new: (properties: Array[Property], verbatim: String, error: ParseError?) -> instance
+ def self.new: (property: Property?, verbatim: String, error: ParseError?) -> instance
end
end
end
diff --git a/test/pro_tacts/test_store.rb b/test/pro_tacts/test_store.rb
index 526f235..f213996 100644
--- a/test/pro_tacts/test_store.rb
+++ b/test/pro_tacts/test_store.rb
@@ -549,11 +549,12 @@ class StoreTest < Minitest::Test
# rewrite that omits the BDAY carries them across or loses them
# (docs/macos-contacts.md, "A birthday the client cannot render is
# dropped from the card").
- # A BDAY sharing its line's bytes with another content line — a lone
- # CR packs two into one physical line — is not a birthday any single
- # decision can be made of: taking the first property alone would drop
- # the bytes beside it unwitnessed, so the line stays verbatim, the
- # model empties, and the arrival is reported.
+ # A BDAY sharing its line's bytes with another content line — a bare
+ # CR packs two into one physical line — is a shape the parser is
+ # built to assume macOS never sends, so it is refused rather than
+ # read: the line stays verbatim, the model empties, and the broken
+ # assumption is reported as itself rather than as an odd BDAY, which
+ # is what would send anyone reading it looking in the wrong place.
def test_a_bday_sharing_its_line_arrives_whole_and_reported
shared = AIDEN.sub("END:VCARD\r\n", "BDAY:1985-04-12\rNOTE:b\r\nEND:VCARD\r\n")
@@ -563,6 +564,7 @@ class StoreTest < Minitest::Test
assert_equal shared, store.contact("aiden").vcard
assert_nil birthday_row(store, "aiden")
assert_equal 1, messages.length
+ assert_match "broke 1 parser assumption", messages.fetch(0)
end
end
diff --git a/test/pro_tacts/vcard/test_lines.rb b/test/pro_tacts/vcard/test_lines.rb
index 808e179..1d4bd55 100644
--- a/test/pro_tacts/vcard/test_lines.rb
+++ b/test/pro_tacts/vcard/test_lines.rb
@@ -54,7 +54,7 @@ class VCardLinesTest < Minitest::Test
def test_lines_yield_the_cards_lines_parsed_beside_verbatim
lines = VCARD.new(CARD).lines
- assert_equal %w[BEGIN VERSION FN UID END], lines.map { it.properties.first&.name }
+ assert_equal %w[BEGIN VERSION FN UID END], lines.map { it.property&.name }
assert_equal CARD, lines.map(&:verbatim).join
end
@@ -63,7 +63,7 @@ class VCardLinesTest < Minitest::Test
bdays, others = VCARD.new(born).lines.partition { it.names?("BDAY") }
assert_equal 1, bdays.length
- assert_equal "1985-04-12", bdays.fetch(0).properties.first&.value
+ assert_equal "1985-04-12", bdays.fetch(0).property&.value
assert_equal "BDAY:1985-04-12\r\n", bdays.fetch(0).verbatim
assert_equal CARD, others.map(&:verbatim).join
end
@@ -74,7 +74,7 @@ class VCardLinesTest < Minitest::Test
folded = CARD.sub("UID:ada\r\n", "UID:ada\r\nBDAY:1985-04-\r\n 12\r\n")
bday = VCARD.new(folded).lines.find { it.names?("BDAY") }
- assert_equal "1985-04-12", bday&.properties&.first&.value
+ assert_equal "1985-04-12", bday&.property&.value
assert_equal "BDAY:1985-04-\r\n 12\r\n", bday&.verbatim
end
@@ -93,7 +93,7 @@ class VCardLinesTest < Minitest::Test
unparsed = VCARD.new("BDAY;=;:\r\n").lines.first
assert_kind_of VCARD::ParseError, unparsed.error
- assert_empty unparsed.properties
+ assert_nil unparsed.property
assert unparsed.names?("BDAY")
assert_equal "BDAY;=;:\r\n", unparsed.verbatim
end
diff --git a/test/pro_tacts/vcard/test_parser.rb b/test/pro_tacts/vcard/test_parser.rb
index 4e1e039..1ea1a98 100644
--- a/test/pro_tacts/vcard/test_parser.rb
+++ b/test/pro_tacts/vcard/test_parser.rb
@@ -153,29 +153,46 @@ class VCardParserTest < Minitest::Test
card = "BEGIN:VCARD\r\nBDAY:1985-04-\r\n 12\r\nEND:VCARD\r\n"
lines = ProTacts::VCard::Parser.lines(card)
- assert_equal %w[BEGIN BDAY END], lines.map { it.properties.first&.name }
- assert_equal "1985-04-12", lines.fetch(1).properties.first&.value
+ assert_equal %w[BEGIN BDAY END], lines.map { it.property&.name }
+ assert_equal "1985-04-12", lines.fetch(1).property&.value
assert_equal "BDAY:1985-04-\r\n 12\r\n", lines.fetch(1).verbatim
assert_equal card, lines.map(&:verbatim).join
end
- # A lone CR ends a content line inside a physical line, so one
- # logical line can read as two properties — the walk keeps both
- # beside the bytes that carried them, where the old lenient read
- # kept only the first.
- def test_a_logical_line_can_read_as_several_content_lines
+ # A bare CR ends a content line without ending a physical line, so
+ # it packs two properties into one line's bytes. No recorded macOS
+ # session sends one, and this server is built on that: the line is
+ # refused, its bytes kept, so nothing downstream can move a property
+ # and take another property's bytes along.
+ def test_a_bare_cr_packing_two_content_lines_is_refused
line = ProTacts::VCard::Parser.lines("FN:A\rNOTE:n\r\n").fetch(0)
- assert_equal %w[FN NOTE], line.properties.map { it.name }
- assert_nil line.error
- assert_equal %w[FN NOTE], parse("FN:A\rNOTE:n\r\n").map { it.name }
+ assert_kind_of ProTacts::VCard::Parser::BrokenAssumption, line.error
+ assert_nil line.property
+ assert_equal "FN:A\rNOTE:n\r\n", line.verbatim
+
+ assert_raises(ProTacts::VCard::Parser::BrokenAssumption) { parse("FN:A\rNOTE:n\r\n") }
+ end
+
+ # A bare CR that ends the only content line on its line takes no
+ # second property with it, so there is nothing to refuse.
+ def test_a_bare_cr_ending_a_line_alone_still_reads
+ assert_equal %w[FN], parse("FN:A\r").map { it.name }
+ end
+
+ # BrokenAssumption is a ParseError, so a caller that declines a card
+ # it cannot read declines this one too without knowing the
+ # difference — the class is for reporting, not for control flow.
+ def test_a_broken_assumption_is_a_parse_error
+ assert_operator ProTacts::VCard::Parser::BrokenAssumption, :<, ProTacts::VCard::ParseError
+ assert_nil ProTacts::VCard.new("BEGIN:VCARD\rEND:VCARD\r\n").properties
end
def test_a_line_that_will_not_read_carries_its_error_and_keeps_its_bytes
line = ProTacts::VCard::Parser.lines("BDAY;=;:\r\n").fetch(0)
assert_kind_of ProTacts::VCard::ParseError, line.error
- assert_empty line.properties
+ assert_nil line.property
assert_equal "BDAY;=;:\r\n", line.verbatim
end
@@ -198,7 +215,7 @@ class VCardParserTest < Minitest::Test
lines = ProTacts::VCard::Parser.lines("FN:A\r\n\r\nUID:b\r\n")
assert_equal ["FN:A\r\n", "\r\n", "UID:b\r\n"], lines.map(&:verbatim)
- assert_empty lines.fetch(1).properties
+ assert_nil lines.fetch(1).property
assert_nil lines.fetch(1).error
assert_equal %w[FN UID], parse("FN:A\r\n\r\nUID:b\r\n").map { it.name }
end