card form: a field in a shape no form sends is a 400
CardForm skipped an address row that was not a hash, read new_address
as a list as well as the hash the add dialog posts, and skipped a phone
or address row whose own line would not read. The routes now hold the
form's fields to their shapes with typecast_params, whose Error the
error handler answers as a 400, and a row read from a line that will
not read raises.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kqo9sKAi4DnKCQkrtYoAwM
diff --git a/lib/pro_tacts/admin/card_form.rb b/lib/pro_tacts/admin/card_form.rb
index 1b475bd..728ccf6 100644
--- a/lib/pro_tacts/admin/card_form.rb
+++ b/lib/pro_tacts/admin/card_form.rb
@@ -179,13 +179,11 @@ module ProTacts
#: (Array[Contact::Phone | Contact::Email] rows, String field, String name, VCard vcard, Hash[String, untyped] params) -> VCard
def self.edited_values(rows, field, name, vcard, params)
edits = params[field]
- if edits.is_a?(Hash)
+ if edits
rows.each do |row|
- # The property is nil only for a line that would not read,
- # and these rows read from lines that did; the guard is the
- # type's honesty, not a reachable case (n_line's own shape).
- property = row.line.property
- next if property.nil?
+ # These rows read from lines that did (Contact#phones), so
+ # a nil is a broken assumption.
+ property = row.line.property or raise "a #{field} row read from a line that will not read"
submitted = edits[row.line.digest]
next if submitted.nil? || submitted.to_s.strip == row.value.strip
@@ -210,17 +208,16 @@ module ProTacts
# one (Admin::ContactsEdit). Removal is the reader's own rule
# (Contact#address_of): a row blank throughout, po box included,
# removes the line, where a partially blanked one keeps it —
- # partial blanks are legal empty components.
+ # partial blanks are legal empty components. Every row is a hash,
+ # the shape the form posts and Web#card_form_params holds a
+ # request to.
#: (Contact contact, VCard vcard, Hash[String, untyped] params) -> VCard
def self.edited_addresses(contact, vcard, params)
rows = params["address"]
- if rows.is_a?(Hash)
+ if rows
contact.addresses.each do |address|
- property = address.line.property
- next if property.nil?
-
submitted = rows[address.line.digest]
- next if !submitted.is_a?(Hash) || address_unchanged?(address, submitted)
+ next if submitted.nil? || address_unchanged?(address, submitted)
line = address_line(address, submitted)
vcard = vcard.substitute(address.line.digest, line ? [line] : [])
@@ -233,12 +230,13 @@ module ProTacts
# after an empty one — so the rows arrive as a hash of hashes
# and read out by their values, several adds in one pass
# included.
- submitted = params["new_address"]
- added = submitted.is_a?(Hash) ? submitted.values : Array(submitted) #: Array[untyped]
- vcard.insert(added.filter_map { address_line(nil, it) })
+ added = params["new_address"] #: Hash[String, Hash[String, untyped]]?
+ return vcard if added.nil?
+
+ vcard.insert(added.values.filter_map { address_line(nil, it) })
end
- #: (Contact::Address address, untyped submitted) -> bool
+ #: (Contact::Address address, Hash[String, untyped] submitted) -> bool
def self.address_unchanged?(address, submitted)
Contact::ADDRESS_COMPONENTS.all? { |name|
submitted[name].to_s.strip == address.public_send(name).to_s.strip
@@ -256,10 +254,8 @@ module ProTacts
# unescape-then-re-escape not being byte-stable. A nil address is
# an added row: seven fresh positions, po box empty, a bare ADR
# header.
- #: (Contact::Address? address, untyped submitted) -> String?
+ #: (Contact::Address? address, Hash[String, untyped] submitted) -> String?
def self.address_line(address, submitted)
- return unless submitted.is_a?(Hash)
-
property = address && address.line.property
if property.nil?
components = [""]
diff --git a/lib/pro_tacts/web.rb b/lib/pro_tacts/web.rb
index b581f3b..5efa92a 100644
--- a/lib/pro_tacts/web.rb
+++ b/lib/pro_tacts/web.rb
@@ -83,12 +83,19 @@ module ProTacts
# DAV client and curl, and no page a current browser sends.
plugin :sec_fetch_site_csrf, allow_missing: true, csrf_failure: :empty_403
- # The identity gate's refusal, as an exception mapped to a
- # response: ProxyAuth.login raises MissingLogin for a request that
- # names nobody, and this answers it with the 401. `classes:` keeps
- # the plugin from touching any other raise — an unexpected failure
- # still propagates to Sentry::Rack::CaptureExceptions outside the
- # app, where it is reported, so this handler must not widen.
+ # The shape checks a form's own fields are held to (#card_form_params),
+ # raising TypecastParams::Error for a POST no page here sends, which
+ # the error handler below answers as a 400.
+ plugin :typecast_params
+
+ # The two refusals raised as exceptions and mapped to a response.
+ # ProxyAuth.login raises MissingLogin for a request that names
+ # nobody, and this answers it with the 401; typecast_params raises
+ # its Error for a form field in a shape no page here posts, and
+ # this answers it with the 400. `classes:` keeps the plugin from
+ # touching any other raise — an unexpected failure still
+ # propagates to Sentry::Rack::CaptureExceptions outside the app,
+ # where it is reported, so this handler must not widen.
#
# A 401 must carry a challenge (RFC 9110 section 15.5.2), and
# `Proxy-Identity` is no registered scheme: it tells the client
@@ -100,12 +107,18 @@ module ProTacts
# it hands an attacker nothing: the proxy overwrites the header on
# every request, so knowing which one it is buys no way to forge
# it.
- plugin :error_handler, classes: [ProxyAuth::MissingLogin] do |_e|
- response.status = 401
- response["WWW-Authenticate"] = "Proxy-Identity"
+ plugin :error_handler, classes: [ProxyAuth::MissingLogin, Roda::RodaPlugins::TypecastParams::Error] do |e|
response["Content-Type"] = "text/plain"
- "Unauthorized: no login on the #{ProxyAuth::HEADER} header.\n" \
- "The proxy in front of this app writes that header.\n"
+ case e
+ when ProxyAuth::MissingLogin
+ response.status = 401
+ response["WWW-Authenticate"] = "Proxy-Identity"
+ "Unauthorized: no login on the #{ProxyAuth::HEADER} header.\n" \
+ "The proxy in front of this app writes that header.\n"
+ else
+ response.status = 400
+ "Bad Request: #{e.param_name} is not the shape this app's forms send.\n"
+ end
end
# The router's trunk: the identity gate, the static files, and the
@@ -148,6 +161,23 @@ module ProTacts
self.class.store
end
+ # The params Admin::CardForm reads, held to the shapes the contact
+ # and group editors post: each phone, email and address field a
+ # hash keyed by its row's digest, every address row and added
+ # address a hash of components. Any other shape is a POST made by
+ # hand, refused here as a 400 (the error handler) rather than read
+ # around in CardForm. An absent field is a form that rendered no
+ # such rows, and passes.
+ #: () -> Hash[String, untyped]
+ def card_form_params
+ typecast_params.Hash(%w[phone email])
+ %w[address new_address].each do |key|
+ rows = typecast_params.Hash(key)
+ rows&.each_key { typecast_params[key].Hash!(it) }
+ end
+ request.params
+ end
+
# One step of a request, timed as a span inside its request's span
# so a slow screen names its slow step. The name is the step's whole
# name; a span carries counts and never a value a person typed or a
diff --git a/lib/pro_tacts/web/contacts.rb b/lib/pro_tacts/web/contacts.rb
index d5a6110..bc5d1d9 100644
--- a/lib/pro_tacts/web/contacts.rb
+++ b/lib/pro_tacts/web/contacts.rb
@@ -200,7 +200,7 @@ module ProTacts
return refuse.call(contact, notice: "This contact's card carries its own birthday spelling; nothing was saved.")
end
- store.save_edit(id, Admin::CardForm.contact_card(contact, first, middle, last, r.params), birthday:)
+ store.save_edit(id, Admin::CardForm.contact_card(contact, first, middle, last, card_form_params), birthday:)
r.redirect land || "/contacts/#{id}", 303
end
diff --git a/lib/pro_tacts/web/import.rb b/lib/pro_tacts/web/import.rb
index d95f346..8e88f08 100644
--- a/lib/pro_tacts/web/import.rb
+++ b/lib/pro_tacts/web/import.rb
@@ -329,7 +329,7 @@ module ProTacts
Admin::ImportGroups.new(
groups: choices,
joined: joined || [*ticked, lot&.id, *tagged.map(&:id)].compact.uniq,
- named: named || [(group unless lot || group.empty?), *(tags - tagged.map(&:name))].compact.uniq,
+ named: named || [(group unless lot), *(tags - tagged.map(&:name))].compact.uniq,
was:,
)
end
diff --git a/sig/gems/roda.rbs b/sig/gems/roda.rbs
index 762e1f6..1f91258 100644
--- a/sig/gems/roda.rbs
+++ b/sig/gems/roda.rbs
@@ -30,6 +30,12 @@ class Roda
# Sec-Fetch-Site the plugin was not configured to allow.
def check_sec_fetch_site!: () -> void
+ # The typecast_params plugin's: the request's params behind
+ # conversion methods that raise TypecastParams::Error for a value of
+ # the wrong shape. Untyped like the params themselves: a conversion
+ # method's answer depends on the type it names.
+ def typecast_params: () -> untyped
+
def request: () -> RodaRequest
def response: () -> RodaResponse
@@ -82,7 +88,18 @@ class Roda
def write: (String body) -> void
end
+ class RodaError < StandardError
+ end
+
module RodaPlugins
def self.register_plugin: (Symbol name, Module plugin) -> void
+
+ module TypecastParams
+ # A submitted value in a shape the conversion refuses; the
+ # error handler answers it as a 400.
+ class Error < RodaError
+ def param_name: () -> String?
+ end
+ end
end
end
diff --git a/test/pro_tacts/admin/test_contacts_pages.rb b/test/pro_tacts/admin/test_contacts_pages.rb
index 27d42e1..dab3b08 100644
--- a/test/pro_tacts/admin/test_contacts_pages.rb
+++ b/test/pro_tacts/admin/test_contacts_pages.rb
@@ -2115,6 +2115,25 @@ class AdminContactsPagesTest < Minitest::Test
end
end
+ # The editor posts every address field as a hash of components; any
+ # other shape is a POST made by hand, refused as a 400 before the
+ # save reads it (Web#card_form_params), the card untouched.
+ def test_an_address_field_in_a_shape_the_form_never_sends_is_a_400
+ with_contacts({"ada" => ADA}) do |store|
+ contact = store.contact("ada")
+ digest = contact.addresses.first.line.digest
+ [{address: {digest => "5 Analytical Way"}},
+ {address: "5 Analytical Way"},
+ {new_address: ["5 Analytical Way"]},
+ {phone: "+1-555-0199"}].each do |field|
+ post "/contacts/ada", {first: "Ada", last: "Lovelace", etag: contact.etag}.merge(field)
+
+ assert_equal 400, last_response.status, field
+ assert_equal contact.etag, store.contact("ada").etag
+ end
+ end
+ end
+
# The header is the line's own, kept byte for byte — the phones'
# own rule, over the parameters an ADR line can carry.
def test_a_saved_address_keeps_its_lines_own_parameters