Add -A/--authorized-fetch option to fedify inbox command #527
Conversation
Summary of ChangesHello @sij411, 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 enhances the Highlights
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
|
issues-auto-labeler
bot
added
component/cli
|
/gemini review |
![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 new --authorized-fetch (-A) option to the fedify inbox command. This feature is useful for testing ActivityPub server implementations by enforcing that all incoming requests are signed with valid HTTP Signatures, rejecting unsigned or invalid ones with a 401 Unauthorized status. The implementation is straightforward, adding the CLI option and integrating verifyRequest into the fetch handler. The documentation has also been updated accordingly.
My review found a minor issue with the clarity of the CLI option's description, and I've provided a suggestion to improve it. Overall, this is a valuable addition for developers working with Fedify.
There was a problem hiding this comment.
Code Review
This pull request adds a useful --authorized-fetch option to the fedify inbox command for testing signed requests. The implementation is mostly correct, but there are a couple of areas for improvement.
First, the description for the new CLI option is slightly ambiguous and could be rephrased for better clarity.
More importantly, there's a performance issue where HTTP signatures are verified twice for each request when the new option is enabled. This happens once in the new logic and again within the federation.fetch() call. I've suggested a couple of ways to address this to avoid the redundant work.
Overall, a great addition for improving the testing capabilities of the CLI.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
…to feat/inbox-auth
Replace manual verifyRequest() in fetch handler with Fedify's authorization predicates on actor and collection dispatchers. This is the idiomatic Fedify approach and covers: - Actor fetches - Inbox POST (built-in) - Followers, following, outbox collections WebFinger and NodeInfo remain public, matching real-world authorized fetch implementations like Mastodon's secure mode. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new --authorized-fetch option to the fedify inbox command, which is a great feature for testing server-to-server authentication. The implementation is well-done, moving the federation setup into the runInbox function to accommodate the new conditional logic. The documentation is also clear and helpful. I have one suggestion to improve code maintainability by reducing duplication in the authorization logic.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new --authorized-fetch option to the fedify inbox command, which is a great addition for testing signed requests. The implementation correctly uses an authorize predicate to protect GET endpoints. The associated documentation is also clear and helpful. I've identified a couple of areas for improvement. First, the behavior for unsigned POST requests to the inbox when the flag is disabled doesn't seem to match the feature's intent, which can be fixed by configuring skipSignatureVerification. Second, there's an opportunity to improve robustness by managing the module-level state more carefully. Overall, this is a valuable feature with a solid implementation foundation.
…ication - Reset module-level state at the beginning of runInbox to avoid state leaking between calls (e.g., during testing) - Set skipSignatureVerification: !authorizedFetchEnabled to make inbox permissive when -A flag is off, and strict when enabled 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable --authorized-fetch option to the fedify inbox command, enabling testing of HTTP signature verification. The implementation is solid, correctly using skipSignatureVerification for inbox deliveries and an authorize predicate for resource fetching. A significant and commendable part of this PR is the refactoring of the runInbox function, which moves module-level logic into the function scope. This greatly improves the command's encapsulation, state management, and testability. I have one suggestion to optimize a repeated function call.
Replace manual verifyRequest() in fetch handler with Fedify's authorization predicates on actor and collection dispatchers. This is the idiomatic Fedify approach and covers: - Actor fetches - Inbox POST (built-in) - Followers, following, outbox collections WebFinger and NodeInfo remain public, matching real-world authorized fetch implementations like Mastodon's secure mode. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ication - Reset module-level state at the beginning of runInbox to avoid state leaking between calls (e.g., during testing) - Set skipSignatureVerification: !authorizedFetchEnabled to make inbox permissive when -A flag is off, and strict when enabled 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…to feat/inbox-auth
The actor endpoint must be publicly accessible so that other servers can fetch the public key to verify HTTP signatures. Requiring authorization on the actor endpoint breaks the trust chain for authorized fetch. Dispatchers with authorization (require HTTP signature when -A enabled): - setFollowersDispatcher: protects follower collection - setFollowingDispatcher: protects following collection - setOutboxDispatcher: protects outbox collection Dispatchers without authorization (publicly accessible): - setActorDispatcher: must be public for key fetching per instance actor specification References: - https://swicg.github.io/activitypub-http-signature/#instance-actor - https://seb.jambor.dev/posts/understanding-activitypub-part-4-threads/#the-instance-actor 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Store the result of ctx.getActorKeyPairs(identifier) in a variable to avoid calling it twice, improving performance and readability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Instead of removing .authorize() from the actor dispatcher entirely, use a dedicated instanceActor predicate that: - Returns true for identifier "i" (instance actor is always public) - Delegates to authorize() for other identifiers This approach keeps .authorize() on the actor dispatcher while ensuring the instance actor remains publicly accessible for key fetching per the ActivityPub HTTP Signature spec. Reference: https://swicg.github.io/activitypub-http-signature/#instance-actor 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add a separate instance actor that is always publicly accessible for key fetching, while the normal actor requires authorized fetch when -A is enabled: - i: normal actor, requires authorized fetch with -A - ia: instance actor, always public for key fetching Both actors share the same key pairs since they're ephemeral and controlled by the same CLI instance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
sij411
force-pushed
the
feat/inbox-auth
branch
from
b22b5ed to
d76a62c
Compare
2 participants
Summary
-A/--authorized-fetchoption tofedify inboxcommand401 UnauthorizedChanges
-A/--authorized-fetchCLI option to inbox commandverifyRequest()in the fetch handlerCloses #229
🤖 Generated with Claude Code