test: give the wire-compat sample set a non-canonical sender identity - #367
Conversation
Every attribute value in the sample set was already canonical, so canonicalization was a no-op across the whole corpus and the gate could not tell canonical-values-on-the-wire from canonicalize-only-inside derive(). It would not have noticed a revert of either. The sender is the only role where a fixture can bite. RecipientHeader ::decaps is handed only the KEM ciphertext and the user secret key, so no reader derives from a recipient policy, and Header::new stores policy.to_hidden(), which blanks the value for any type outside HINT_TYPES. SignatureExt.pol, by contrast, is a full Policy and Unsealer::new derives from it. So: SENDER becomes " Sender@Sample.TEST " (trim and lowercase), the private sender policy gains pbdf.sidn-pbdf.mobilenumber.mobilenumber = "+31 (0)6 1234 5678" next to the existing fullname (the two policies are canonicalized by separate statements in the sealer, so one fixture leaves the other blind), and manifest() records .canonical() of each. The manifest is the expectation readers are checked against, so it promises the canonical form while the sealer is handed the raw one. pg-compat did not check the sender policy at all, while pg-compat-js has since #261; close that asymmetry. Manifest gains a `sender` field deserialized structurally, because each pinned pg-core is a distinct crate with a distinct Policy type. pg-core/tests/sample_sealer.rs had the same hole. Both halves get a gate-teeth case that rewrites the manifest's sender policy back to the raw value and requires a failure naming the mismatch, and the record is corrected in three places: Header::new's canonicalize is load-bearing for the hint a recipient is shown, not for what anyone derives, and the recipient side is not gate-observable. SCHEMA_VERSION, SEED, the five case names and the recipient policies are unchanged, and the non-privsig artifacts stay byte-identical because canonicalize(" Sender@Sample.TEST ") is the old value. Closes #355 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
CI note, so the red X on this PR is not read as this change: Build cryptify (arm64) in the Delivery workflow failed after 12s at the It is a runner-side flake, not a permissions problem and not this diff:
I cannot rerun it ( |
CI: the red
|
| job | login | time |
|---|---|---|
| Build cryptify (amd64) | Succeeded | 13:51:59 |
| Build (arm64) | Succeeded | 13:52:10 |
| Build cryptify (arm64) | denied | 13:52:24 |
The failure lands before the build step, and the diff touches no Docker, workflow, Cargo.toml or Cargo.lock file, so nothing in it can reach that login.
I changed no code for this, so the diff under review is byte-identical to before. CI was retriggered by closing and reopening the PR, which requeues everything because the trigger is a bare pull_request: and picks up the default reopened type. The same commit bc51b66 is now green: 36 pass, 4 skipping, no failures, with Build cryptify (arm64) among the passes.
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
VERDICT: request-changes
Rules Dobby 2, cycle 1 on bc51b66. Rule sweep over the rules that apply to a Rust test-fixture-plus-docs diff, merged with the review findings: one bug, two nits.
The fixture itself holds up. I re-ran both mutations one at a time on this HEAD and confirmed the restore between them (git diff pg-core/src/client/mod.rs empty after each):
| mutation | cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream |
|---|---|
canonical_signing_key -> key.clone() |
exit 101, head_reads_back_every_case ... FAILED, mem/alice: public signing policy (" Sender@Sample.TEST " vs "sender@sample.test") |
with_priv_signing_key's canonicalize() removed |
exit 101, same test, mem-privsig/alice: private signing policy ("+31 (0)6 1234 5678" vs "+31612345678") |
That is the fixture working, and it is also the bug: the CLAUDE.md bullet this PR rewrites still tells the next reader those runs stay green. The PR body says so under "One prediction in the ticket no longer holds" while CLAUDE.md says the opposite, and CLAUDE.md is the surface a later agent run loads automatically.
The two nits are both accuracy of a claim about the JS half, not the code the claim sits next to. Neither blocks on its own.
What the sweep checked and cleared: the PR title is conventional (test:, and pr-title.yml does run amannn/action-semantic-pull-request here), Closes #355 is a real closing keyword against the default branch, no workflow file is touched so no docs prose here claims a gate that does not exist, cargo fmt --all -- --check and cargo fmt --manifest-path pg-compat/Cargo.toml --all -- --check both pass, and the body is 1 em dash and 1 bold span in 1356 words. CLAUDE.md:18 is the only copy of the stale claim in the tree.
Two mutation sites, two teeth in the Rust half, and the sample set is non-canonical in both sender policies: that part of the ticket is fully covered.
Review follow-up on #367. CLAUDE.md's identity-canonicalization bullet still ended with "cargo test -p pg-core stays green under both reverts". That was true of the tree the ticket was written against, but this PR's own step-3 assertion in head_reads_back_every_case invalidates it: with each canonicalization site reverted alone, cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream exits 101 on mem/alice: public signing policy and mem-privsig/alice: private signing policy respectively. The bullet is what a maintainer reads to decide whether a red Test workspace (pg-core) can be a canonicalization regression, and as written it told them no. describe_policy's doc claimed it mirrors describePolicy so a mismatch reads the same in either half. Two things were wrong. The text differs: serde_json here has no preserve_order, so its object keys come out alphabetically while the JS half emits ts before con. More than cosmetic, the halves disagreed on what a mismatch *is* — this half sorts con and the JS half did not. Until this PR sender.private held one attribute so the order could not differ; it now holds two, and Wire compat is the one required check, so a future reader handing the conjunction back in another order would redden it for something this half rules a non-break. describePolicy now sorts con on the same grounds (code-unit order, not localeCompare, so the order is the same everywhere the gate runs), and the doc says what actually holds. The JS half also only had a gate tooth on sender.public. The sealer canonicalizes the two policies in separate statements, which is the argument the Rust half's second tooth rests on, and it applies verbatim here; the private comparison is additionally the branch behind reader.privateSignatureVisible, so the wasm reader is the only path that reaches it. Adds the mem-privsig tooth and a unit test for the reorder. Both new tests were mutated to confirm they bite: removing verify.mjs's private comparison and removing the new sort each turn exactly one red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
VERDICT: approve
Rules Dobby 2 — cycle 2, clean
Re-swept 9ccc934 against the rule corpus and verified the delta myself. No findings. Review Dobby 2's pass came back empty too; the three cycle-1 findings are fixed.
Rules checked
Selected 15 rules by changed-file type (Rust fixture/gate, Node test, docs) and ran them in three batches. Two candidates came back and both were refuted on verification:
- "the Rust private gate-teeth test cannot pass — only the wasm reader surfaces the private signature." That limitation is about the JS readers (
pg-jsvspg-wasm, gated onreader.privateSignatureVisible); Rustpg-corereaders returnVerificationResult.privatedirectly.a_manifest_promising_the_wrong_private_sender_value_is_reportedpasses. - "
26 passeddoes not rule out# cancelled" ([[node-test-cancelled-is-not-green]]). Checked the way the rule prescribes rather than taking the figure:# pass 26 / # fail 0 / # cancelled 0,npm testexit 0.
Verified independently
Both suites reproduce the PR body's figures exactly — pg-compat 18 passed, exit 0; pg-compat-js 26 pass, 0 cancelled, exit 0.
The headline claim — that the non-canonical fixture gives the gate teeth — I probed rather than read. Reducing canonical_signing_key to key.clone(), resealing, and running both halves:
pg-core 0.6.3: mem/alice: parse: incorrect signature
pg-core 0.6.3: mem/bob: parse: incorrect signature
... (all 5 cases x 2 recipients)
---- head_reads_back_every_case ----
assertion `left == right` failed: mem/alice: public signing policy
left: ... value: Some(" Sender@Sample.TEST ")
right: ... value: Some("sender@sample.test")
exit 101
So the "one prediction in the ticket no longer holds" section is accurate as written: the producer-side suite really does catch the revert now, one CI job earlier than #355 predicted. Restored with the inverse edit; git diff empty.
Spot-checks that could have been findings and were not:
newest_reader()usesreaders().first()—readers()really does list0.6.3before0.5.10, so "the newest pin" is accurate.check_sender's "aprivatethe reader did not surface is already reported as a presence mismatch by the caller" holds —lib.rs:411pushes that failure andcheck_senderis called unconditionally after it, so the branch cannot swallow a missing private policy.describe_policy's{t, v}+tsprojection drops nothing:Attributehas exactlyt/vandPolicyexactlyts/con.canonicalizeitself is pinned by literal vectors (identity.rs:444), so a manifest derived via.canonical()cannot hide a mutation of the rule table — the layering is right, not circular.scratch()being shared by both teeth tests is safe: it is only the child'scurrent_dir, not a write target.pg-compat/tests/gate_teeth.rsis genuinely wired into CI —build.yml:205runs the wholepg-compatsuite with no--testfilter.cargo fmt/clippyare listed for both changed Rust crates (pg-core,pg-compat);pg-compat-jsis not Rust.
No advisory or embargo reference in the PR or #355, so the draft flag is being lifted.
1 participant
Closes #355.
Every attribute value in the wire-compat sample set was already canonical, so canonicalization was a no-op across the whole corpus: the gate could not tell canonical-values-on-the-wire from canonicalize-only-inside-
derive(), and would not have noticed a revert of either. This gives it a non-canonical sender, teaches the Rust half to assert the sender policy the way the Node half already did, and proves both halves go red.What changed
pg-core/examples/seal-samples/sample_set.rsSENDERis now" Sender@Sample.TEST ", which exercises both halves of the email rule.private_sender_policy()keepspbdf.gemeente.personalData.fullnameand appendspbdf.sidn-pbdf.mobilenumber.mobilenumber = "+31 (0)6 1234 5678". Both policies are needed:canonical_signing_keyguards the public one andwith_priv_signing_keycanonicalizes the private one in a separate statement, so a fixture on one leaves the other as blind as before. The phone value covers the second canonicalization rule rather than the email rule twice, and(0)is the vector where dropping the parentheses but keeping the0yields+310612345678— an E.164-shaped number that dials nowhere.manifest()serializes.canonical()of each sender policy instead of the raw one. The manifest is the expectation readers are checked against, so it promises the canonical form while the sealer is handed the raw one, and that disagreement is what makes the fixture a test..canonical()rather than literals, so there is no second copy of the value to drift.SCHEMA_VERSION,SEED, the five case names and the recipient policies are untouched.pg-compat/src/lib.rsManifestgains asenderfield. The policies are held asserde_json::Valueand compared through a newdescribe_policy/describe_policy_mismatchpair, because each pinnedpg-coreis a distinct crate with a distinctPolicytype and naming one of them in the shared manifest would tie it to a single reader. Both halves sortcon, so a reader handing the conjunction back in another order is not a spurious failure:describe_policyhere anddescribePolicyinpg-compat-js/src/failures.mjs, which gained the sort in the review round below. The two messages name the same fields but are not the same text —serde_jsonhere has nopreserve_order, so its object keys come out alphabetically while the JS half emitstsbeforecon. Insidereader!,verify_casenow compares theVerificationResult'spublicagainstmanifest.sender.publicfor every case, and itsprivateagainstmanifest.sender.privatewhen the case isprivateSigning.pg-core/tests/sample_sealer.rshad the same hole inhead_reads_back_every_case; therePolicyis in scope, so the comparison is a plainassert_eq!.Three places said the wrong thing about
Header::new's.canonical()and are corrected: the doc comment atpg-core/src/client/header.rs, the identity-canonicalization bullet in rootCLAUDE.md, and thetest_policy_canonicalization_reaches_the_wirecomment inpg-core/src/identity.rs. SincePolicy::derivecanonicalizes internally, that call yields identical KEM identities either way; its only observable effect on the wire isto_hidden's displayed hint for aHINT_TYPESattribute. It is load-bearing for what a recipient is shown, not for what is derived, and the recipient side is therefore not gate-observable at all.The non-privsig artifacts are byte-identical
canonicalize(" Sender@Sample.TEST ")is exactly the oldsender@sample.test, so the public-half edit must not move any byte. Sealed a set onmain, sealed one on this branch, compared:The two
*-privsigcontainers change because the private policy gains a real attribute, which is expected. The onlymanifest.jsonchange is the new canonical mobile number:Mutation 1:
canonical_signing_keyreturns the key unchangedpg-core/src/client/mod.rs,canonical_signing_keyreduced tokey.clone(). Published readers derive the raw identity fromh_sig_ext.polwhile the signing key is canonical, so the header signature no longer verifies:The Node half, same set:
Mutation 2:
with_priv_signing_keydrops itscanonicalizepg-core/src/client/mod.rs, thepriv_sign_key.policy.canonicalize()line removed. Only the*-privsigcases fail, which is the point of having the fixture on both policies:Both reverts were applied one at a time and restored;
git diff pg-core/src/client/mod.rsis empty on this branch.Review round
Three points from the consolidated review, all addressed.
CLAUDE.mdstill claimedcargo test -p pg-corestays green under both reverts. True of the tree the ticket was written against, false at this HEAD, and this PR's own step-3 assertion is what makes it false — the section below already said so while the bullet said the opposite. That bullet is what a maintainer reads to decide whether a redTest workspace (pg-core)can be a canonicalization regression, so it now points athead_reads_back_every_caseand says the gate is still what proves a published reader rejects it. Re-measured here, each revert applied alone and restored before the next: both exit 101 oncargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream, onmem/alice: public signing policyandmem-privsig/alice: private signing policyrespectively.The two halves disagreed on what a mismatch is.
describe_policysortedcon;describePolicydid not. Until this PRsender.privateheld one attribute so the order could not differ — it now holds two, andWire compatis the one required check, so a future reader returning the conjunction in another order would have reddened it for something the Rust half rules a non-break.describePolicynow sorts too, on code-unit order rather thanlocaleCompareso the order is the same everywhere the gate runs. The doc comment claiming the two read identically was wrong on the text as well and now says what holds.The JS half had a tooth only on
sender.public. The sealer canonicalizes the two policies in separate statements, which is the argument the Rust half's second tooth rests on and applies verbatim here; the private comparison is also the branch behindreader.privateSignatureVisible, so the wasm reader is the only path that reaches it — the narrowest path in either half. Added themem-privsigtooth, plus a unit test for the reorder.Both new tests were mutated to confirm they bite rather than trusted: dropping
verify.mjs's private comparison turnsa manifest promising the wrong private sender value is reportedred, and dropping the new sort turnsa conjunction handed back in another order is not a mismatchred. One red each, nothing else moved, both restored.One prediction in the ticket no longer holds
The ticket notes that
cargo test -p pg-coreshould stay green under both reverts, because HEAD's ownderive_ibscanonicalizes and the failure is only visible to a published reader. That was true of the tree the ticket was written against. With step 3's assertion added,head_reads_back_every_casecatches both reverts too, because it now compares the recovered policy against the manifest's canonical one rather than only checking the plaintext and the private-signature presence:So a revert is now caught in
cargo test -p pg-coreas well as in the gate, one CI job earlier. Nothing was done to arrange this; flagging it because the ticket predicted otherwise.Gate teeth
The new assertion is proven rather than trusted, in both halves. Each copies the artifact directory, rewrites
manifest.json's sender policy back to the raw value, runs one reader against the copy, and requires a failure naming the mismatch.pg-compat-js/test/gate-teeth.test.mjs: one moretest(...)on top of the existingwithDamagedSet.pg-compat/tests/gate_teeth.rs: new, since this is the half whose assertion is brand new and therefore the least proven. It reusesread_manifest,run_case,readersand the child-process runner, and puts the damaged copy underCARGO_TARGET_TMPDIRso no dev-dependency is needed for a temp directory. Two cases: the public policy onmem, and the private one onmem-privsig, because the sealer canonicalizes them in separate statements.pg-compat/src/lib.rsalso gets three unit tests on the comparison itself: a reordered conjunction is not a mismatch, a raw value does not match its canonical form, and an absent attribute value is not an empty one.Acceptance check
All green on this branch:
No case ran into the abort path on the unmutated tree: every case exited through a reported message or a clean pass.
Notes
pg-compat/Cargo.toml's pins andCOMPATIBILITY.mdare untouched, per the scope fence. Nothing under.github/workflows/is touched either, and no dependency changed.pg-compat/README.mddocuments the artifact layout, and thesender.public/sender.privatebullets now say the manifest records the canonical form while the sealer is handed a non-canonical one. Without that, the next reader of the manifest has no way to know the disagreement is deliberate.Wire compatpath filter coverspg-core/**andpg-compat/**, so the required check does the real work on this PR rather than reporting a filtered pass.🤖 Generated with Claude Code