Enforce FEP-ef61 proof policy for portable objects - #968
Conversation
Add a policy-aware verifier for FEP-ef61 portable objects. It applies FEP-2277 core-type classification, requires every proof to use a DID from the portable object's cryptographic origin, and verifies the full proof set while reusing the canonicalized document digest. Document malformed-input handling, collection and nested-object trust boundaries, and telemetry behavior. Cover URI variants, proof policy failures, multiple proofs, JSON-LD aliases, and malformed inputs. Fixes fedify-dev#832 fedify-dev#288 fedify-dev#968 Assisted-by: Codex:gpt-5.6-sol Assisted-by: Claude Code:claude-fable-5
✅ Deploy Preview for fedify-json-schema canceled.
|
|
@codex review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesPortable proof verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PortableVerifier
participant DocumentLoader
participant ProofVerifier
Caller->>PortableVerifier: portable JSON-LD object
PortableVerifier->>PortableVerifier: validate ID and proof policy
PortableVerifier->>DocumentLoader: resolve verification method
DocumentLoader-->>PortableVerifier: verification key
PortableVerifier->>ProofVerifier: verify proof with cached digest
ProofVerifier-->>PortableVerifier: verification result
PortableVerifier-->>Caller: structured result
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a94557207e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dahlia
moved this to In review
in STF 2025–2026 Milestone 7: Advanced Features and Standards (FEP-ef61, new FEPs)
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Require every portable proof to expand to exactly one verification method before decoding or resolving keys. This prevents a valid first DID from hiding a second non-DID or cross-authority method from the FEP-ef61 policy check. fedify-dev#968 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6afef27594
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Resolve the active JSON-LD context before constructing the proofless JCS message, including type-scoped contexts and aliased @type terms. Use the preloaded-only loader so this lookup cannot fetch attacker-controlled remote contexts. fedify-dev#968 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab86306e49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fedify/src/sig/proof.ts`:
- Around line 498-536: Cache the resolved proof-property names produced by
getProofPropertyNames, keyed by the document’s `@context` value (including the
no-context case as appropriate), so repeated createProofMessageDigests calls
reuse completed context processing. Add bounded eviction such as a small LRU for
attacker-controlled context values, while preserving the existing returned names
and context-resolution behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6f42c00-9fd4-4720-b5d5-477e5b430c8d
📒 Files selected for processing (2)
packages/fedify/src/sig/proof.test.tspackages/fedify/src/sig/proof.ts
Honor JSON-LD prefix mappings when identifying proof properties to remove from the proofless JCS message. This keeps compact IRIs aligned with direct aliases and expanded predicates. fedify-dev#968 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f21b470db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Record remote contexts loaded during portable object expansion and replay only those responses when identifying proof property aliases. This keeps caller-defined aliases verifiable without letting digest construction fetch any new attacker-controlled context. fedify-dev#968 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78a16594b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/fedify/src/sig/proof.ts (1)
841-919: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftFull JSON-LD expansion (and potential network context fetch) runs before the cheap portable-ID check.
verifyPortableObjectProof()callsexpandPortableObjectRoot()— which drives a fulljsonld.expand()pass, including any remote@contextfetches viaoptions.contextLoader— unconditionally, before checking whetherroot["@id"]even matches the portableap(?:+ef61)?://scheme (Line 906-908). Every ordinaryhttps://object routed through this policy layer (a stated regression-test case: "non-portable HTTP(S) objects") pays the cost of full expansion and remote-context resolution just to be told it isn't portable.For a security-facing gate meant to sit in front of arbitrary inbound federation traffic, this means attacker-supplied objects with heavy/attacker-controlled
@contextchains force expensive work before being rejected as non-portable, which is a resource-exhaustion concern if this contextLoader isn't otherwise capacity-limited.Consider a cheap pre-check on the raw (un-expanded)
jsonLd.id/jsonLd["@id"]to short-circuit obviously non-portable input before invokingexpandPortableObjectRoot(), falling back to full expansion only when the fast check is ambiguous (e.g., custom aliasing of the id term).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fedify/src/sig/proof.ts` around lines 841 - 919, Update verifyPortableObjectProof to inspect the raw jsonLd["`@id`"] when it is a string and immediately return notPortableObject for IDs that are clearly not prefixed with ap:// or ap+ef61://, before calling expandPortableObjectRoot. Preserve full expansion for ambiguous inputs, including aliased or absent ID terms, so valid portable objects continue through the existing expanded-root validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/fedify/src/sig/proof.ts`:
- Around line 841-919: Update verifyPortableObjectProof to inspect the raw
jsonLd["`@id`"] when it is a string and immediately return notPortableObject for
IDs that are clearly not prefixed with ap:// or ap+ef61://, before calling
expandPortableObjectRoot. Preserve full expansion for ambiguous inputs,
including aliased or absent ID terms, so valid portable objects continue through
the existing expanded-root validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f82f6f4-1f8b-4a9a-8299-876a584a5667
📒 Files selected for processing (2)
packages/fedify/src/sig/proof.test.tspackages/fedify/src/sig/proof.ts
Inspect an explicit JSON-LD @id before expanding remote contexts. This avoids unnecessary attacker-controlled context work for obviously non-portable objects while preserving expansion for aliased or absent IDs. fedify-dev#968 (review) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e126686525
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Require every functional Data Integrity Proof property to expand to exactly one non-list value before decoding. This prevents appended values from being ignored by singular accessors while the original signature still verifies. fedify-dev#968 (comment) fedify-dev#968 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2chanhaeng
left a comment
2chanhaeng
left a comment
There was a problem hiding this comment.
LGTM! Got some suggestions.
Reuse the JSON-LD node guard in verification so TypeScript carries the exact record contract into digest creation. This removes the local assertion without changing runtime rejection. fedify-dev#968 (comment) Assisted-by: Codex:gpt-5.6-sol
Use an explicit verb to describe why the complete proof set is validated before any key resolution begins. fedify-dev#968 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54fa3c7cb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Require each expanded portable proof to declare exactly the DataIntegrityProof type before decoding. This prevents a missing or appended type from being ignored while the reconstructed proof configuration still verifies. fedify-dev#968 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
sij411
left a comment
sij411
left a comment
There was a problem hiding this comment.
I read verify proof spec and Fep-8b32 , then checked that the specs requires the verifier to copy the entire proof and remove only proofValue before canonicalizing it. So fields such as expires, domain, and challenge are part of the signed proof configs. Also Fep-8b32 says expired proofs should be ignored and another authentication method tried. however, the PR instead reconstructs a fixed config containing only cryptosuite, verificationMethod, proofPurpose, created. so if you add expires: "2000-01-01T00:00:00Z" to an already signed proof, it still makes verifyPortableObjectProof() return verified: true. could verification preserve and authenticate the complete received proof options? if some proof options are intentionally unsupported, they should at least rejected instead of being accepted without authentications.
Preserve every received proof option except proofValue when building the JCS verification input so expiry, domain, challenge, nonce, and extension fields cannot be changed without invalidating the signature. Resolve raw and typed proofs by content rather than array position, parse raw candidates once, and fail closed when unresolved contexts could hide aliased proof options. Retain compatibility for literal proof fields on documents with unrelated remote contexts. Add domain and challenge expectations to VerifyProofOptions, document the behavior, and cover malformed, expired, aliased, multi-proof, and context edge cases. fedify-dev#968 (review) Assisted-by: Claude Code:claude-fable-5 Assisted-by: Codex:gpt-5.6-sol
sij411
left a comment
sij411
left a comment
There was a problem hiding this comment.
await t.step("rejects a tampered duplicate proof", async () => {
const signed = await signPortableJsonLd(unsignedObject);
const proof = signed.proof as Record<string, unknown>;
const result = await verifyPortableObjectProof({
...signed,
proof: [
proof,
{
...structuredClone(proof),
// Added after the signature was generated:
expires: "3000-01-01T00:00:00Z",
},
],
}, options);
// Currently fails because the second parsed proof is matched
// to the first raw proof and result.verified is true.
assertFalse(result.verified);
});This is the short regression reproduces an issue i found in this commit 30a4978. i think we need another way to handle this when multiple matching candidates are given.
Bind each parsed proof to one raw occurrence so duplicate typed projections cannot reuse a configuration that omits appended proof options. Compare normalized configurations when occurrence identity is ambiguous. Retain fetched raw documents for remote proof references and canonicalize portable reference URLs. This preserves valid remote proofs while still rejecting tampered duplicates. fedify-dev#968 (review) Assisted-by: Claude Code:claude-fable-5 Assisted-by: Codex:gpt-5.6-sol
|
@sij411 Thanks for the reproducer. The typed projection was indeed allowing the first raw proof occurrence to be reused. Fixed in 0c10e0d. Verification now allocates each raw occurrence at most once, compares normalized configurations when standalone verification cannot identify an occurrence, and retains the fetched raw document for referenced proofs. This keeps equivalent JSON-LD representations and valid remote proofs working while authenticating appended options such as I added regression coverage for tampered duplicates, equivalent duplicate representations, HTTPS and portable remote references, and embedded-plus-referenced duplicates. The |
3 participants
FEP-ef61 moves authentication for portable objects away from the HTTP origin that served a document. A valid FEP-8b32 signature is therefore necessary but not sufficient: the signing DID must be the cryptographic origin encoded in the portable ID. Putting that rule inside
verifyProof()would mix one FEP's identity policy into a general cryptographic primitive, so this PR adds a policy layer above it.Closes #832. Part of #288. Nested portable object verification remains tracked by #938.
Why this is a separate policy layer
verifyPortableObjectProof()expands the top-level JSON-LD node before applying the FEP-2277 core-type classification. FEP-2277 classifies an object by its shape rather than its declaredtype, and expansion makes compact terms, aliases, and fully expanded properties follow the same path. Portableap:andap+ef61:IDs are then compared with each proof'sverificationMethodthrough the shared FEP-fe34 origin helpers.The helper parses and checks the complete proof set before resolving any key. Every verification method must be a DID URL, and every DID must match the portable ID's authority. This ordering prevents an earlier proof from causing an attacker-controlled key fetch when a later proof already makes the document unacceptable. Once the policy checks pass, every proof is verified through the existing
verifyProof()path.Multiple proofs share the canonicalized document digest. This keeps verification work proportional to the document size plus the number of proofs, instead of canonicalizing and hashing the full document again for each proof. Proof-specific configuration and signatures are still checked independently, and successful keys remain in proof order.
Failure and trust boundaries
Expected policy outcomes use a tagged result rather than exceptions. Callers can distinguish a non-portable document, an unsigned collection, a missing or invalid proof, an unsupported verification method, and a DID mismatch. Structurally malformed JSON-LD roots and malformed portable IDs still throw
TypeError, since they cannot be interpreted as policy results.Unsigned portable collections remain a separate outcome because FEP-ef61 permits them only under a gateway trust policy that this verifier cannot decide. Verification methods, legacy public keys, and links are also reported as unsupported FEP-2277 core types instead of being treated as ordinary portable objects.
The helper intentionally verifies only the top-level secured document. An outer JCS proof covers the bytes of an embedded proof, but it does not establish that the nested signer controls the nested portable ID. Compound-document verification is still unsettled upstream, so this PR does not silently choose a recursive proof-removal rule or claim that an outer proof authenticates its descendants. That work remains tracked by #938.
Documentation and observability
The manual in docs/manual/send.md shows how to handle malformed remote input, unsigned collections, and policy failures without confusing them with successful verification. It also states the top-level verification boundary. docs/manual/opentelemetry.md records that the wrapper emits one verification measurement for each attempted proof and none when policy preflight rejects the document before cryptographic work. CHANGES.md describes the new public surface for Fedify 2.4.0.
Testing
The tests in packages/fedify/src/sig/proof.test.ts cover FEP-2277 precedence, compact and expanded JSON-LD properties, URI spelling and authority variants, caller-resolved DID methods, malformed proofs, cross-DID signatures, whole-set policy preflight, signed and unsigned collections, multiple proofs, digest reuse, nested-object boundaries, and OpenTelemetry measurements.
The following checks passed:
mise run check-each fedifymise run check:fixture-usagemise run test-each fedifymise run check:mdmise run docs:build