Sign portable compound activities with one proof - #1073
Conversation
Fedify's activity signer created one Object Integrity Proof for each Ed25519 key, so a sender with several Ed25519 keys emitted a proof set. The map-local compound-proof profile that Fedify's inbox applies to documents containing FEP-ef61 portable maps accepts exactly one direct proof per map, so Fedify rejected such activities from Fedify itself. The three places that sign outgoing activities now share one helper with an explicit rule for documents that contain portable maps: - An activity that already carries a proof is kept as is, including when it is sent by actor identifier. - A single Ed25519 key signs the activity, as before. - With several Ed25519 keys, a portable activity is signed only by the key whose ID is a DID URL for the activity's DID, the only proof FEP-ef61 accepts. If no key or several keys qualify, or a non-portable activity embeds portable maps, sendActivity() rejects with an actionable TypeError before delivering or enqueueing. A final check also refuses to deliver or enqueue a document with portable maps in which any map carries a proof set, for example after signObject() has been called twice. The producer-side scans are unbounded, so neither check depends on the inbox's traversal limits. Activities without portable maps are signed exactly as before. Fixes fedify-dev#1045 fedify-dev#288 fedify-dev#1041 Assisted-by: Claude Code:claude-opus-5-5 Assisted-by: Codex:gpt-6-astra
dahlia
added
component/federation
✅ Deploy Preview for fedify-json-schema canceled.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughOutgoing activity signing now selects a single eligible Ed25519 key for compound portable activities and rejects unsupported proof shapes. The send paths validate serialized activities before delivery or fanout. The manual and changelog describe the signing and rejection rules. ChangesCompound portable activity signing
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant ContextImpl
participant signOutgoingActivity
participant assertSupportedCompoundProofShape
participant Fanout
ContextImpl->>signOutgoingActivity: Sign activity with candidate keys
signOutgoingActivity-->>ContextImpl: Return signed activity and proof state
ContextImpl->>assertSupportedCompoundProofShape: Validate serialized activity
ContextImpl->>Fanout: Enqueue validated activity JSON-LD
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains in the supplied evidence; proceed with normal validation. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new rules reduce ambiguous signing of portable activities, but they affect every outgoing delivery mode. The available evidence does not fully establish how receiving inboxes handle every final signed payload. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 36.84% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 5 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
1 participant
Closes #1045.
Fedify's inbox accepts exactly one direct proof per map in a document that contains FEP-ef61 portable objects (#1041). The signer created one proof per Ed25519 key, so an actor with several keys produced activities that Fedify itself rejected.
Key selection
When the outgoing JSON contains a map identified by an
ap:orap+ef61:URI, Fedify now creates at most one proof. With several Ed25519 keys, a portable activity is signed by the one key whose ID is a DID URL for the activity's DID. FEP-ef61 accepts no proof from any other key.Fedify throws a
TypeErrorbefore delivery or enqueueing if no key matches, several keys match, or a non-portable activity embeds portable objects. In that last case, key order cannot tell which key the application meant, so Fedify does not fall back to the first one; callers pass explicit sender keys or pre-sign withsignObject(). An activity that already has a proof is left alone, even when sent by actor identifier, which used to append more proofs.Keys are matched by the ID the proof will name, not by
ActorKeyPair.keyId. Dispatched keys sign as…#multikey-N, which is never a DID URL, so a portable activity with several dispatched Ed25519 keys always fails.The three signing sites in packages/fedify/src/federation/middleware.ts now share one helper in packages/fedify/src/federation/outgoing-proof.ts. It serializes the activity only when there are two or more candidate keys, so the single-key path skips that serialization.
Final guard
Before delivery or enqueueing, Fedify also rejects a document with portable objects if any map carries a proof set, such as an object signed twice with
signObject(). The fanout path runs this check before enqueueing, so the error reaches the caller instead of the worker.Both JSON scans run without the inbox's traversal limits. Reusing the bounded inbox scan would let a limit hit hide a later proof array, and would make large ordinary activities look compound. Documents without portable objects are signed exactly as before, proof sets included.
Tests
The new tests cover immediate delivery, the outbox queue, forced fanout, the fanout worker, and the key pairs dispatcher. One delivers a Fedify-produced portable activity to a Fedify inbox and expects
202 Accepted. With both checks disabled, nine tests fail, including that one. The tests pass on Deno, Node.js, and Bun.Not covered
That end-to-end test uses Ed25519 keys only. With an RSA key, Fedify attaches a Linked Data Signature, and the inbox's compound check hashes the
signatureproperty along with the document, so the activity is rejected. FEP-8b32 says the Linked Data Signature must be removed before verifying proofs. That inbox bug is outside #1045 and needs a separate PR.