Two guards that looked like they held (v0.3.2)

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

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

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)]

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
 

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,

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

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
 

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