Skip to content

Fall back when Linked Data signing hits JSON-LD processing errors#842

Merged
dahlia merged 3 commits into
fedify-dev:2.0-maintenancefrom
notJoon:fix/issue-824
Jun 26, 2026
Merged

Fall back when Linked Data signing hits JSON-LD processing errors#842
dahlia merged 3 commits into
fedify-dev:2.0-maintenancefrom
notJoon:fix/issue-824

Conversation

@notJoon

@notJoon notJoon commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #824

This changes outbound activity delivery to continue without an optional Linked Data Signature when JSON-LD canonicalization/processing fails during signing. These failures are logged as warnings.

Other signing failures, such as key, configuration, or programming errors, still fail.

AI Usage

Codex (GPT-5.5) was used after the implementation for self-review.

@netlify

netlify Bot commented Jun 25, 2026

Copy link
Copy Markdown

Deploy Preview for fedify-json-schema ready!

Name Link
🔨 Latest commit aa439f6
🔍 Latest deploy log https://app.netlify.com/projects/fedify-json-schema/deploys/6a3dc70a05b3d70008e26454
😎 Deploy Preview https://deploy-preview-842--fedify-json-schema.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a89e9b2c-d1e5-430a-87b3-1b7f1aa7cabb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds JSON-LD signing error classification and changes outbound RSA Linked Data signing to log matching failures and continue without a Linked Data signature. Also adds a changelog note for the release.

Changes

Linked Data signature fallback

Layer / File(s) Summary
JSON-LD error classifier
packages/fedify/src/federation/middleware.ts
Adds an internal helper that recognizes JSON-LD processing failures tied to Linked Data signature creation.
RSA signature fallback
packages/fedify/src/federation/middleware.ts, CHANGES.md
Wraps signJsonLd(...) in a guarded try/catch that logs matching JSON-LD processing errors and continues sending without a Linked Data signature; the changelog records the release note.

Sequence Diagram(s)

sequenceDiagram
  participant sendActivity
  participant signJsonLd
  participant jsonld.canonize
  participant isLinkedDataSignatureJsonLdProcessingError
  sendActivity->>signJsonLd: create RSA Linked Data signature
  signJsonLd->>jsonld.canonize: canonize JSON-LD
  jsonld.canonize-->>signJsonLd: throw processing error
  signJsonLd-->>sendActivity: raise error
  sendActivity->>isLinkedDataSignatureJsonLdProcessingError: classify caught error
  isLinkedDataSignatureJsonLdProcessingError-->>sendActivity: true for matching JSON-LD failures
  Note over sendActivity: log warning and continue without Linked Data signature
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • fedify-dev/fedify#721: Both PRs modify the outbound/proof signing path in packages/fedify/src/federation/middleware.ts.

Suggested labels

type/bug, component/federation, component/signatures

Suggested reviewers

  • sij411
  • 2chanhaeng
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: falling back when Linked Data signing hits JSON-LD processing errors.
Description check ✅ Passed The description matches the implemented fallback behavior and the stated distinction between recoverable and fatal signing errors.
Linked Issues check ✅ Passed The code now logs JSON-LD signing failures and continues delivery without a Linked Data Signature, matching issue #824.
Out of Scope Changes check ✅ Passed The changelog note is relevant documentation and no unrelated code changes are evident.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request ensures that outbound activity delivery does not abort when Linked Data Signature creation fails during JSON-LD canonicalization. Instead, it logs a warning and continues delivery without the signature. The review feedback points out a potential runtime crash in the new isLinkedDataSignatureJsonLdProcessingError helper function, where calling .startsWith() on potentially undefined properties (error.message or error.name) could throw a TypeError. Using optional chaining is recommended to ensure robust error handling.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread packages/fedify/src/federation/middleware.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/federation/middleware.ts`:
- Around line 425-439: Update the JSON-LD error classification in the middleware
so signing/configuration failures remain fatal: the logic in federation
middleware should inspect the actual nested failure on error.cause for
jsonld.InvalidUrl instead of error.details.cause, and it should not treat
UnsafeJsonLdError as retryable/continuable. Adjust the predicate around the
existing jsonld.InvalidUrl handling and the final fallback in the middleware
helper that classifies errors so Unsupported JSON-LD keyword and related
jsonld.* failures still propagate as aborting errors rather than
warning-and-continuing.
🪄 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: cc7a4393-9304-47fe-826f-15296c8769d8

📥 Commits

Reviewing files that changed from the base of the PR and between 667c8bf and 544ff02.

📒 Files selected for processing (2)
  • CHANGES.md
  • packages/fedify/src/federation/middleware.ts

Comment thread packages/fedify/src/federation/middleware.ts Outdated
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 30.76923% with 27 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
packages/fedify/src/federation/middleware.ts 30.76% 26 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
packages/fedify/src/federation/middleware.ts 90.04% <30.76%> (-5.64%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

isInvalidUrlTypeError(error)));
}

function isLinkedDataSignatureJsonLdProcessingError(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be simplified by avoiding access to unknown fields throughout the body, and handling it with a single structural type. just a thought

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to split it into two functions: a user-defined type guard which takes an unknown value, and an actual predicate function which takes a proper structural type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied the suggestion. thanks! cf43e96

@dahlia dahlia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change the target of this pull request to 2.0-maintenance rather than main?

@notJoon notJoon changed the base branch from main to 2.0-maintenance June 26, 2026 01:38
@notJoon notJoon marked this pull request as draft June 26, 2026 01:39
@dahlia

dahlia commented Jun 26, 2026

Copy link
Copy Markdown
Member

Codex (GPT-5.5) was used after the implementation for self-review.

So, could you put Assisted-by: Codex:gpt-5.5 trailers in your commit messages? See also our AI usage policy!

Assisted-by: Codex:gpt-5.5
@notJoon notJoon marked this pull request as ready for review June 26, 2026 04:44
@notJoon

notJoon commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@dahlia I’ve updated all the points you mentioned. Could you please check them? Thank you!

@notJoon notJoon requested a review from dahlia June 26, 2026 04:48

@dahlia dahlia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

@dahlia dahlia merged commit e620494 into fedify-dev:2.0-maintenance Jun 26, 2026
16 of 17 checks passed
@notJoon notJoon deleted the fix/issue-824 branch June 26, 2026 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linked Data signing should not crash outbound activity delivery on canonization depth errors

2 participants