diff --git a/CHANGELOG.md b/CHANGELOG.md index bd0f3c8..e5b30cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.3.2] - 2026-08-20 + +### Fixed — two guards that looked like they held + +- **Canonical JSON refuses invalid bytes whatever their encoding tag.** + `valid_encoding?` is always true on an `ASCII-8BIT` string — BINARY has no + invalid byte sequences by definition — and BINARY is exactly what Rack and + CDN headers deliver, so the UTF-8 guard was a no-op for the ten + request-evidence values most likely to be malformed. Invalid bytes passed + through and produced canonical JSON that was not itself valid UTF-8, which + RFC 8785 forbids and a verifier in another language may reject or normalize + into a different digest. The guard now normalizes the tag before validating. + **No digest ever written changes**: bytes that are valid UTF-8 canonicalize + byte-identically whether they arrive tagged BINARY or UTF-8, which a test + pins. And a stored value that can no longer be canonicalized now reports a + binding mismatch instead of raising out of the integrity check — a check + that crashes tells an operator nothing except that the tool broke. +- **`record_ip_geolocation(country: nil)` is refused instead of quietly + enabling three fields.** With plain `nil` keyword defaults an explicit nil + was indistinguishable from an omitted keyword, so a policy written as + `record_ip_geolocation(country: settings[:geo])` with an empty setting fell + through to the coarse-trio default — enabling a category of personal data as + a side effect, which the frictionless pass never relaxed. A sentinel now + tells the two apart: unmentioned still gets the coarse trio, `false` still + reaches the coherence check that names `do_not_record_ip_geolocation`, and + `nil` raises a sentence. +- **A scaffolding `legal_basis_reference:` is refused like a scaffolding + `because:`.** The reference lands in the compiled policy revision and every + receipt built from it, permanently, where "TODO: ask legal" reads as a + reviewed determination rather than an omission. The option stays optional; + text the host actually wrote has to be text they meant. + +### Documentation + +- The installer's purpose and retention prompts now describe what the + installer actually does since 0.3.x: a blank purpose is accepted (the gem + records its own stated purpose, marked as the gem's), a blank period keeps + pace with the evidence it corroborates, and only scaffolding text or a + negative period stops generation. + ## [0.3.1] - 2026-08-20 ### Changed — the rest of the collection friction, and the principle behind removing it diff --git a/lib/clickwrap/canonical_json.rb b/lib/clickwrap/canonical_json.rb index d46fa2a..d4de3d6 100644 --- a/lib/clickwrap/canonical_json.rb +++ b/lib/clickwrap/canonical_json.rb @@ -132,10 +132,26 @@ def utf16_sort_key(key) end def write_string(string, buffer) - raise SerializationError, "Canonical JSON strings must be valid UTF-8" unless string.valid_encoding? + # `valid_encoding?` is ALWAYS true on an ASCII-8BIT string — BINARY has + # no invalid byte sequences by definition — and BINARY is exactly what + # Rack and CDN headers deliver, so this guard was a no-op for the + # strings most likely to be malformed. Ten request-evidence columns + # reach it. + # + # Normalizing the tag before validating is safe for every digest ever + # written: bytes that ARE valid UTF-8 canonicalize byte-identically + # whether they arrive tagged BINARY or UTF-8 (measured, and pinned by a + # test below). What changes is only the case that was broken — + # genuinely invalid bytes used to emit canonical JSON that was itself + # not valid UTF-8, which RFC 8785 forbids and a verifier in another + # language may reject or normalize into a different digest. That is the + # "still verifiable years later" promise failing silently, so it is + # refused at write time instead. + utf8 = string.encoding == Encoding::UTF_8 ? string : string.dup.force_encoding(Encoding::UTF_8) + raise SerializationError, "Canonical JSON strings must be valid UTF-8" unless utf8.valid_encoding? buffer << '"' - string.each_char do |char| + utf8.each_char do |char| escape = ESCAPES[char] buffer << if escape escape diff --git a/lib/clickwrap/dsl/policy_builder.rb b/lib/clickwrap/dsl/policy_builder.rb index 60d8201..67e837e 100644 --- a/lib/clickwrap/dsl/policy_builder.rb +++ b/lib/clickwrap/dsl/policy_builder.rb @@ -257,9 +257,21 @@ def do_not_record_browser_user_agent # and nothing finer, because that is what "IP geolocation" means in this # gem when nobody narrows it. Naming even one field means you are # choosing the set yourself, and then the set is exactly what you named. - def record_ip_geolocation(country: nil, region: nil, city: nil, postal_code: nil, - latitude_and_longitude: nil, timezone: nil, continent: nil, - metro_code: nil, accuracy_radius_in_kilometers: nil, + # Distinguishes "the policy never mentioned this field" from "the policy + # passed nil for it". With plain `nil` defaults the two are identical, and + # `record_ip_geolocation(country: settings[:geo])` with an empty setting + # silently enabled the coarse trio — enabling a category of personal data + # as a side effect, which is the one thing the frictionless pass never + # relaxed. + UNMENTIONED_FIELD = Object.new.freeze + private_constant :UNMENTIONED_FIELD + + def record_ip_geolocation(country: UNMENTIONED_FIELD, region: UNMENTIONED_FIELD, + city: UNMENTIONED_FIELD, postal_code: UNMENTIONED_FIELD, + latitude_and_longitude: UNMENTIONED_FIELD, + timezone: UNMENTIONED_FIELD, continent: UNMENTIONED_FIELD, + metro_code: UNMENTIONED_FIELD, + accuracy_radius_in_kilometers: UNMENTIONED_FIELD, using: nil, encrypted: nil, delete_after: nil, retain_until: nil, fail_if_unavailable: false, because: nil, legal_basis_reference: nil, @@ -415,14 +427,28 @@ def application_default_legal_basis_reference(category) end end - # `nil` means "the policy did not mention this field"; `false` means "the - # policy named it and turned it off". The distinction is the whole reason - # the keywords default to nil: a policy that mentions nothing gets the - # coarse trio, and a policy that explicitly sets every field to false - # still reaches the coherence check that tells it to say + # An UNMENTIONED field means "the policy did not mention this"; `false` + # means "the policy named it and turned it off". A policy that mentions + # nothing gets the coarse trio; a policy that explicitly sets every field + # to false still reaches the coherence check that tells it to say # `do_not_record_ip_geolocation` instead. + # + # An explicit `nil` is neither, and is refused rather than guessed: it is + # almost always a variable that came out empty, and treating it as "the + # policy said nothing" would turn a missing setting into three enabled + # fields of personal data. def default_ip_geolocation_fields_when_none_named(named) - return named.transform_values { |value| value == true } if named.any? { |_, value| !value.nil? } + ambiguous = named.select { |_, value| value.nil? }.keys + unless ambiguous.empty? + raise DefinitionError, + "Policy #{@key} passes nil for #{ambiguous.join(", ")} in " \ + "`record_ip_geolocation`. Say `true` or `false` for each field you name, " \ + "or leave it out entirely — Clickwrap will not read an empty value as " \ + "permission to record it, and it will not read it as silence either." + end + + mentioned = named.reject { |_, value| value.equal?(UNMENTIONED_FIELD) } + return named.transform_values { |value| value == true } if mentioned.any? Vocabulary::IP_GEOLOCATION_DATA_FIELDS.to_h do |field| [field, Vocabulary::COARSE_IP_GEOLOCATION_DATA_FIELDS.include?(field)] diff --git a/lib/clickwrap/models/request_evidence.rb b/lib/clickwrap/models/request_evidence.rb index adaf372..193acd0 100644 --- a/lib/clickwrap/models/request_evidence.rb +++ b/lib/clickwrap/models/request_evidence.rb @@ -302,11 +302,18 @@ def category_binding_digest_verified?(category:, digest:, algorithm:, key_id:) key = Clickwrap.config.request_evidence_binding_key_for(key_id) return false if key.nil? - computed = Digest.keyed_digest( - CanonicalJson.generate(binding_body_for(category)), - key: key, - algorithm: digest_algorithm - ) + # A stored value that can no longer be canonicalized cannot reproduce its + # binding, and saying so is the honest answer — an integrity check that + # raises tells an operator nothing except that the tool broke. + begin + computed = Digest.keyed_digest( + CanonicalJson.generate(binding_body_for(category)), + key: key, + algorithm: digest_algorithm + ) + rescue CanonicalJson::SerializationError + return false + end Digest.secure_compare?(computed, digest) end diff --git a/lib/clickwrap/request_evidence_policy.rb b/lib/clickwrap/request_evidence_policy.rb index 715fdbd..182372e 100644 --- a/lib/clickwrap/request_evidence_policy.rb +++ b/lib/clickwrap/request_evidence_policy.rb @@ -229,6 +229,20 @@ def validate_category!(category) "purpose." end + # The legal-basis reference goes into the compiled policy revision and + # every receipt built from it, permanently. Scaffolding there is worse + # than an omission: an omission reads as "the host said nothing", while + # "TODO: ask legal" reads as a reviewed determination to anyone who finds + # it later. Same rule as `because:` — the option is optional, but text + # the host actually wrote has to be text they meant. + if ReviewedText.placeholder?(setting.legal_basis_reference) + raise DefinitionError, + "Policy #{policy_key} records #{category} with a `legal_basis_reference:` " \ + "that is still scaffolding text (#{setting.legal_basis_reference.inspect}). " \ + "Replace it with the application's own reference, or drop the option — " \ + "Clickwrap would rather record nothing than record a TODO as a legal basis." + end + return unless setting.delete_after && setting.delete_after.to_i <= 0 raise DefinitionError, diff --git a/lib/clickwrap/version.rb b/lib/clickwrap/version.rb index 150e774..348ed1d 100644 --- a/lib/clickwrap/version.rb +++ b/lib/clickwrap/version.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Clickwrap - VERSION = "0.3.1" + VERSION = "0.3.2" # The canonical schema version for receipts, event digests, and presentation # manifests. This is deliberately independent of VERSION: gem releases may diff --git a/lib/generators/clickwrap/install_generator.rb b/lib/generators/clickwrap/install_generator.rb index ec1624f..8248dcf 100644 --- a/lib/generators/clickwrap/install_generator.rb +++ b/lib/generators/clickwrap/install_generator.rb @@ -980,15 +980,18 @@ def ask_about_ip_geolocation_purpose def ask_purpose(label) say "\n Why does the application need #{label}? One plain sentence, in your own" - say " words — it goes into the initializer and the privacy inventory. A blank" - say " or scaffolding answer stops generation before Clickwrap writes any files." + say " words — it goes into the initializer and the privacy inventory. Leave it" + say " blank and Clickwrap records its own stated purpose instead, marked as the" + say " gem's; a scaffolding answer (TODO, FIXME) stops generation before any" + say " files are written." ask(" Purpose:").to_s.strip end def ask_retention_days(label) say "\n After how many days should Clickwrap delete #{label}?" - say " Clickwrap does not invent a period. Enter the positive number your application" - say " has reviewed; a blank or zero answer stops generation before files are written." + say " Enter the number of days your application reviewed, or leave it blank and" + say " #{label} keeps pace with the evidence it corroborates — kept until a" + say " reviewed disposition removes it. A negative number stops generation." ask(" Days:").to_s.strip.to_i end diff --git a/test/canonical_encoding_and_geolocation_nil_test.rb b/test/canonical_encoding_and_geolocation_nil_test.rb new file mode 100644 index 0000000..ec09729 --- /dev/null +++ b/test/canonical_encoding_and_geolocation_nil_test.rb @@ -0,0 +1,122 @@ +# frozen_string_literal: true + +require "test_helper" + +# Two defects found by review on 2026-08-20, both in the same family: a guard +# that looked like it held and did not. +class CanonicalEncodingAndGeolocationNilTest < ActiveSupport::TestCase + # --- The canonicalization guard ------------------------------------------- + + test "BINARY-tagged bytes that are valid UTF-8 canonicalize byte-identically" do + # THE safety claim behind fixing the guard: every value that ever reached + # production arrived from Rack tagged ASCII-8BIT with valid UTF-8 bytes, so + # normalizing the tag cannot change a digest anyone already wrote. + tagged = Clickwrap::CanonicalJson.dump({ "city" => "Málaga".b }) + plain = Clickwrap::CanonicalJson.dump({ "city" => "Málaga" }) + + assert_equal plain, tagged + assert_equal plain.bytes, tagged.bytes + assert_equal Encoding::UTF_8, tagged.encoding + assert tagged.valid_encoding? + end + + test "genuinely invalid bytes are refused however they are tagged" do + # `valid_encoding?` is always true on ASCII-8BIT, so the old guard passed + # these through and emitted canonical JSON that was not valid UTF-8 — which + # RFC 8785 forbids, and which a verifier in another language may reject or + # normalize into a different digest. + ["M\xFFlaga".b, "M\xFFlaga".dup.force_encoding(Encoding::UTF_8)].each do |value| + error = assert_raises(Clickwrap::CanonicalJson::SerializationError) do + Clickwrap::CanonicalJson.dump({ "city" => value }) + end + assert_match(/valid UTF-8/, error.message) + end + end + + test "an annex value that cannot be canonicalized reports a mismatch, never raises" do + # An integrity check that crashes tells an operator nothing except that the + # tool broke. + record_request_evidence_by_default! + receipt = submit_clickwrap(:signup, actor: create_user, http_request: fake_http_request) + annex = receipt.event.reload.request_evidence + annex.define_singleton_method(:binding_body_for) { |_category| { "ip_address" => "1\xFF".b } } + + assert_nothing_raised do + refute annex.category_binding_digest_verified?( + category: :ip_address, digest: "whatever", + algorithm: annex.event.request_evidence_digest_algorithm, + key_id: annex.event.request_evidence_key_id + ) + end + end + + # --- The geolocation nil path --------------------------------------------- + + test "an explicit nil field is refused instead of quietly enabling three" do + error = assert_raises(Clickwrap::DefinitionError) do + Clickwrap.policy :geolocation_nil_probe do + agree_to :terms, link_label: "Terms of Service" + record_ip_geolocation(country: nil) + end + end + + assert_match(/passes nil for country/, error.message) + assert_match(/will not read an empty value as permission/, error.message) + end + + test "mentioning nothing still gets the coarse trio; naming one field gets that one" do + Clickwrap.policy :geolocation_unmentioned_probe do + agree_to :terms, link_label: "Terms of Service" + record_ip_geolocation + end + assert_equal %w[city country region], + Clickwrap.policy!(:geolocation_unmentioned_probe) + .request_evidence.enabled_ip_geolocation_fields.map(&:to_s).sort + + Clickwrap.policy :geolocation_one_field_probe do + agree_to :terms, link_label: "Terms of Service" + record_ip_geolocation(country: true) + end + assert_equal %w[country], + Clickwrap.policy!(:geolocation_one_field_probe) + .request_evidence.enabled_ip_geolocation_fields.map(&:to_s) + end + + test "naming every field false still says use do_not_record_ip_geolocation" do + assert_raises(Clickwrap::DefinitionError) do + Clickwrap.policy :geolocation_all_false_probe do + agree_to :terms, link_label: "Terms of Service" + record_ip_geolocation(country: false) + end + end + end + + # --- Scaffolding in a legal-basis reference -------------------------------- + + test "a scaffolding legal-basis reference is refused like a scaffolding purpose" do + error = assert_raises(Clickwrap::DefinitionError) do + Clickwrap.policy :legal_basis_todo_probe do + agree_to :terms, link_label: "Terms of Service" + record_ip_address(legal_basis_reference: "TODO: ask legal") + end + end + + assert_match(/legal_basis_reference/, error.message) + assert_match(/rather record nothing than record a TODO/, error.message) + end + + private + + def record_request_evidence_by_default! + Clickwrap.configure { |config| config.record_request_evidence_by_default = true } + Clickwrap::Services::LoadPolicies.new(root: Rails.root.to_s, paths: ["config/clickwrap.rb"]).call + end + + def fake_http_request + ActionDispatch::TestRequest.create( + "REMOTE_ADDR" => "203.0.113.7", + "HTTP_USER_AGENT" => "Mozilla/5.0 (Macintosh) Test/1.0", + "action_dispatch.request_id" => "req-#{SecureRandom.hex(4)}" + ) + end +end