Handle FetchError in getSignedKeyOwner() gracefully#589
Handle FetchError in getSignedKeyOwner() gracefully#589dahlia merged 2 commits intofedify-dev:mainfrom
FetchError in getSignedKeyOwner() gracefully#589Conversation
When a remote server has authorized fetch enabled and returns HTTP 401 for actor lookups, `RequestContext.getSignedKeyOwner()` previously let the `FetchError` propagate uncaught, resulting in a 500 Internal Server Error instead of treating the request as unauthenticated. This commit wraps the `getKeyOwner()` call in a try/catch so that a `FetchError` causes `getSignedKeyOwner()` to log a warning and return `null` (unauthenticated), while any other error is still re-thrown. A regression test and documentation updates are included. Fixes fedify-dev#473 Co-Authored-By: Claude <claude@anthropic.com>
dahlia
added
type/bug
Summary of ChangesHello @dahlia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug where Fedify servers would return a 500 Internal Server Error when interacting with remote servers that require authorized fetch (such as GoToSocial) and respond with an HTTP 401 during the key owner lookup. The implemented fix ensures that Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 gracefully handles FetchError in getSignedKeyOwner() by catching the error, logging a warning, and returning null instead of letting it propagate and cause a 500 Internal Server Error. This fixes an interoperability issue with servers that use authorized fetch. The change is well-implemented with a new regression test to cover the failure scenario and corresponding documentation updates to explain the new behavior. The code is clear, correct, and follows best practices.
There was a problem hiding this comment.
Pull request overview
Fixes a federation error-handling bug where RequestContext.getSignedKeyOwner() could throw an uncaught FetchError (notably on HTTP 401 from authorized-fetch servers), leading to a 500 response instead of treating the requester as unauthenticated.
Changes:
- Catch
FetchErrorduring key-owner lookup ingetSignedKeyOwner(), log a warning, and returnnull. - Add a regression test covering the 401/
FetchErrorcase. - Update authorized-fetch documentation and add a changelog entry for the fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/fedify/src/federation/middleware.ts | Handles FetchError from key-owner fetch by logging and returning null instead of throwing. |
| packages/fedify/src/federation/middleware.test.ts | Adds regression test ensuring getSignedKeyOwner() returns null on FetchError. |
| docs/manual/access-control.md | Documents authorized-fetch failure modes and the new null behavior. |
| CHANGES.md | Adds release note entry referencing the fix and linked issue/PR. |
Include the key owner URL ({keyOwner}) as a separate field in the
warning log message, in addition to the key ID ({keyId}), to make
it clearer which actor URL the fetch failed for when debugging
authorized-fetch interoperability issues.
fedify-dev#589 (comment)
Co-Authored-By: Claude <claude@anthropic.com>
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Summary
When a remote server has authorized fetch enabled, fetching the key owner actor requires a signed request—but
getSignedKeyOwner()made that fetch unsigned by default. If the remote server returned HTTP 401, the resultingFetchErrorpropagated uncaught up throughhandleActor(), causing Fedify to return a500 Internal Server Errorto the requester instead of treating them as unauthenticated.This bug broke all interaction with GoToSocial (and any other server with authorized fetch enabled) whenever an
AuthorizePredicatewas attached to an actor dispatcher, even a trivial one that allows all requests.Changes
RequestContext.getSignedKeyOwner()now catchesFetchErrorfrom the key owner lookup, logs a warning via LogTape, and returnsnull(i.e., treats the request as unauthenticated) instead of throwing. All other errors are still re-thrown.getSignedKeyOwner()returnsnull.null-return behavior.Notes
The proper solution for full interoperability with authorized-fetch servers is to use the instance actor pattern: pass an authenticated document loader to
getSignedKeyOwner({ documentLoader })so that the key owner fetch is signed. This is already supported and documented. This fix ensures that without the instance actor, the server no longer crashes with 500—it simply treats such requests as unauthenticated.Fixes #473.