Normalize outgoing JSON-LD for Pixelfed interop#721
Conversation
Move outgoing JSON-LD compatibility adjustments into a dedicated layer so send and proof creation use the same wire representation. Keep the existing public audience workaround and add attachment array preservation for Pixelfed compatibility, while skipping attachment rewrites that would change JSON-LD semantics. Document the distinction from activity transformers and cover the new normalization behavior in send and proof tests. pixelfed/pixelfed#6588 Assisted-by: Codex:gpt-5.5
dahlia
added
type/bug
|
@codex review |
issues-auto-labeler
Bot
added
activitypub/pixelfed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds outgoing JSON‑LD normalization that ensures ActivityStreams Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ContextImpl
participant Middleware
participant Normalizer
participant Signer
participant FanoutQueue
participant Remote
Client->>ContextImpl: sendActivity(activity, options)
ContextImpl->>Middleware: sendActivityInternal(activity, { ..., normalizeExistingProofs? })
Middleware->>Normalizer: normalizeOutgoingActivityJsonLd(compactJsonLd, contextLoader)
alt normalization applied (no existing proof OR proofCreated OR normalizeExistingProofs)
Normalizer-->>Middleware: normalizedJsonLd
Middleware->>Signer: createProof(normalizedJsonLd)
else skip normalization (preserve existing proof bytes)
Middleware->>Signer: forward originalWireBytes or createProof(original)
end
Signer-->>Middleware: signedActivity
Middleware->>FanoutQueue: enqueue FanoutMessage (includes normalizeExistingProofs flag)
FanoutQueue->>Middleware: dequeue/deliver per recipient
Middleware->>Remote: POST signedActivity (attachments normalized as arrays)
Remote-->>Middleware: response
Middleware-->>ContextImpl: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Introduces an outgoing JSON-LD normalization layer for ActivityPub interoperability (notably Pixelfed), ensuring the JSON-LD bytes that get signed match what is sent on the wire.
Changes:
- Added
normalizeOutgoingActivityJsonLd()to apply outgoing JSON-LD wire-format compatibility fixes (public audience + attachment array shape). - Applied outgoing JSON-LD normalization consistently in both the send pipeline and
eddsa-jcs-2022proof creation/verification fallback paths. - Added tests + documentation + changelog entry covering the new behavior and regressions.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fedify/src/compat/outgoing-jsonld.ts | New outgoing JSON-LD normalization layer, including Pixelfed attachment array workaround. |
| packages/fedify/src/compat/outgoing-jsonld.test.ts | Unit coverage for attachment wrapping, semantics-guarding, and safety cases. |
| packages/fedify/src/sig/proof.ts | Normalizes compact JSON-LD before signing; verification tries both on-wire and normalized forms. |
| packages/fedify/src/sig/proof.test.ts | Regression coverage ensuring proof bytes align with outgoing normalized JSON-LD. |
| packages/fedify/src/federation/middleware.ts | Normalizes JSON-LD before LD signature creation and sending. |
| packages/fedify/src/federation/middleware.test.ts | Regression coverage ensuring posted activities preserve attachment as an array. |
| docs/manual/send.md | Documents the post-serialization, internal outgoing JSON-LD compatibility step. |
| CHANGES.md | Changelog entry for Pixelfed interoperability improvement and reference links. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a compatibility layer to improve interoperability with ActivityPub implementations like Pixelfed by ensuring that outgoing activity 'attachment' fields are serialized as arrays, even when containing a single item. The changes include a new utility module for JSON-LD normalization, updates to the federation middleware and signature verification logic to incorporate these normalization steps, and comprehensive regression tests. I have no further feedback to provide as the reviewer's comment regarding performance refactoring correctly identifies that such optimizations should be deferred to a separate PR to maintain scope and reduce risk.
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Skip the JSON-LD canonicalization equivalence check when scalar attachment wrapping runs under an ActivityStreams-only context without nested context scopes. Also make the fallback log messages context neutral, since the helper can run from proof verification as well as send paths. References fedify-dev#721 (comment) References fedify-dev#721 (comment) Assisted-by: Codex:gpt-5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements internal JSON-LD wire-format compatibility fixes to improve interoperability with ActivityPub implementations like Pixelfed, which require the attachment field to be an array even for single items. It introduces a normalizeOutgoingActivityJsonLd utility that wraps scalar attachments while ensuring JSON-LD semantics are preserved via canonicalization. The federation middleware and signature modules have been updated to apply these fixes before activities are signed and sent. I have no feedback to provide as the review comment suggested improvements that were already implemented in the code.
![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: 895f30ed47
ℹ️ 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".
Keep the fast path semantics-preserving by leaving JSON-LD list objects unwrapped, since array-wrapping list objects can change their meaning. Broaden the known-safe context check to allow additional preloaded contexts alongside ActivityStreams, and avoid allocating a replacement object until the attachment walker actually changes a subtree. References fedify-dev#721 (comment) References fedify-dev#721 (comment) References fedify-dev#721 (comment) Assisted-by: Codex:gpt-5
|
@codex review |
|
/gemini review |
Share the preloaded-only context loader between outgoing JSON-LD compatibility helpers so security-sensitive fallback behavior cannot drift. Keep the proof verification fallback on that restricted loader instead of reusing a caller-provided context loader for inbound JSON-LD. Also document the attachment normalization loader contract, add safe correlation metadata to the semantic-divergence warning, and tighten the pathological nesting regression assertion. fedify-dev#721 (comment) fedify-dev#721 (comment) fedify-dev#721 (comment) fedify-dev#721 (comment) fedify-dev#721 (comment) Assisted-by: Codex:gpt-5
Add a public sendActivity option for callers that pre-sign activities with Fedify and need the outgoing compact JSON-LD to match the normalized bytes covered by the proof. Preserve existing proofs by default, and document that trade-off in the send path, queue message type, and manual. fedify-dev#721 (comment) fedify-dev#721 (comment) fedify-dev#721 (comment) fedify-dev#721 (comment) fedify-dev#721 (comment) Assisted-by: Codex:gpt-5
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
Review feedback addressed.
|
@codex review |
|
/gemini review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fedify/src/compat/outgoing-jsonld.ts`:
- Around line 123-146: Add a startup-time invariant check that verifies none of
the preloaded contexts in KNOWN_SAFE_CONTEXT_URLS redefine the "attachment" term
(or if present, that it maps exactly to the ActivityStreams definition:
"@type":"@id"), and fail-fast if any context violates this; implement this check
alongside module initialization so hasKnownSafeContext (and the jsonld.canonize
fast-path) remains safe. Locate the check to run before the fast-path relies on
KNOWN_SAFE_CONTEXT_URLS (reference symbols: KNOWN_SAFE_CONTEXT_URLS,
AS_CONTEXT_URL, hasKnownSafeContext, and the jsonld.canonize fast-path) and
ensure the startup assertion enumerates each preloaded context, inspects its
term definition for "attachment", and throws/logs an error if a conflicting
definition is found.
In `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 3749-3780: Add a queued-fanout regression: create a pre-signed
activity (reuse signedWithNormalizedProof and actorEdKey) and call
ctx.sendActivity with fanout: "force" and normalizeExistingProofs: true so the
activity is queued rather than direct-sent; then simulate/replay the worker
delivery of the persisted FanoutMessage (the code path that consumes
FanoutMessage) and assert that after worker delivery the posted payload still
has object.attachment as an array
(assertEquals(Array.isArray(postedSignedObject.attachment), true)) and that
proof verification still succeeds (assertEquals(verified, ["proof"])), ensuring
the fanout serialization/propagation preserves the option and the normalized
proof.
🪄 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
Run ID: 72aad9b2-9c22-47b3-a0e7-6fdf1a41fa4f
📒 Files selected for processing (11)
docs/manual/send.mdpackages/fedify/src/compat/outgoing-jsonld.test.tspackages/fedify/src/compat/outgoing-jsonld.tspackages/fedify/src/compat/preloaded-context-loader.tspackages/fedify/src/compat/public-audience.tspackages/fedify/src/federation/context.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.tspackages/fedify/src/federation/queue.tspackages/fedify/src/sig/proof.test.tspackages/fedify/src/sig/proof.ts
There was a problem hiding this comment.
Code Review
This pull request introduces internal JSON-LD wire-format compatibility fixes to improve interoperability with platforms like Pixelfed, specifically by ensuring attachment fields are serialized as arrays. It adds a new normalizeExistingProofs option to sendActivity() to allow opting into these fixes for pre-signed activities and implements a restricted preloadedOnlyDocumentLoader for secure normalization. Feedback identifies a potential issue in the recursive traversal logic where inline @context objects might be processed, potentially leading to unintended modifications of term definitions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cc2fbc3ff
ℹ️ 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".
Enforce the preloaded context invariant that keeps the attachment term compatible with ActivityStreams before using the known-safe fast path. Also keep JSON-LD value payloads out of the nested-context detector, fix the newly added option's release annotation, and cover queued fanout for pre-signed activities that opt into existing-proof normalization. Addresses review comments: fedify-dev#721 (comment) fedify-dev#721 (comment) fedify-dev#721 (comment) fedify-dev#721 (comment) Assisted-by: Codex:gpt-5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fedify/src/compat/outgoing-jsonld.ts`:
- Around line 51-64: assertPreloadedAttachmentContextInvariant currently only
examines objects yielded by getContextObjects() and misses scoped `@context`
nested inside term definitions; update the invariant to recursively descend into
any term definition objects and collect/visit their nested "@context" (or extend
getContextObjects to do this) so that any redefinition of the "attachment" term
inside scoped contexts is detected; specifically, when iterating context objects
in assertPreloadedAttachmentContextInvariant, for each property that is an
object check for and recurse into its "@context" (mirroring hasNestedContext's
traversal) before testing with isActivityStreamsAttachmentTerm so
wrapScalarAttachments's fast path remains safe.
🪄 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
Run ID: 1f57ee10-48ba-4d5e-8e3e-fcfa9ea7de63
📒 Files selected for processing (4)
packages/fedify/src/compat/outgoing-jsonld.test.tspackages/fedify/src/compat/outgoing-jsonld.tspackages/fedify/src/federation/context.tspackages/fedify/src/federation/middleware.test.ts
There was a problem hiding this comment.
Code Review
This pull request introduces internal JSON-LD wire-format compatibility fixes to improve interoperability with ActivityPub implementations like Pixelfed, specifically by ensuring attachment fields are serialized as arrays. It adds the normalizeExistingProofs option to SendActivityOptions, allowing these fixes to be applied to pre-signed activities. The implementation includes safeguards such as traversal depth limits and semantic preservation checks via JSON-LD canonicalization. I have no feedback to provide.
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
Build the known-safe preloaded context set by filtering contexts that keep ActivityStreams attachment semantics intact instead of failing at module load. The check now descends into scoped context definitions so future preloaded contexts cannot silently bypass canonicalization when they redefine attachment locally. Pass the restricted preloaded-only loader explicitly in proof verification fallbacks so inbound JSON-LD normalization cannot start depending on a network-capable default after future refactors. Addresses review comments: fedify-dev#721 (comment) fedify-dev#721 (comment) fedify-dev#721 (comment) Assisted-by: Codex:gpt-5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces internal JSON-LD wire-format compatibility fixes to improve interoperability with platforms like Pixelfed, specifically by ensuring attachment fields are serialized as arrays. It also adds a normalizeExistingProofs option to SendActivityOptions to allow these fixes to be applied to pre-signed activities. The implementation includes security measures such as a restricted document loader to prevent SSRF and traversal depth limits to mitigate DoS risks. I have no feedback to provide.
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ 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". |
Summary
as:Public.attachmentas an array when compact JSON-LD would otherwise collapse a single item to a scalar.Rationale
Fedify already had an interoperability workaround for Lemmy because compacting public addressing to
as:Publiccan cause problems with some implementations. Pixelfed has a similar representation-level interoperability issue withattachment: when a JSON-LD processor compacts a single-element array, the result can become a scalar value, but Pixelfed expectsattachmentto remain an array.This cannot be handled by activity transformers because they operate on
Activityobjects rather than the serialized JSON-LD document. The new compatibility layer runs after JSON-LD serialization, where Fedify can adjust the outgoing document shape without changing the vocabulary object model.The
attachmentworkaround is guarded by JSON-LD canonicalization. Fedify compares the document before and after wrapping scalar attachments and skips the rewrite if the canonicalized semantics would change, which avoids altering custom local contexts that redefineattachment.Testing
deno test --check --doc --allow-all --unstable-kv --trace-leaks --parallel packages/fedify/src/compat/outgoing-jsonld.test.ts packages/fedify/src/sig/proof.test.ts packages/fedify/src/federation/middleware.test.ts.mise run check-each fedify.mise run test-each fedify.References
References pixelfed/pixelfed#6588.