feat: Add -A/--authorized-fetch option to fedify inbox command
#474
Conversation
Summary of ChangesHello @notJoon, 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 security of 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-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 successfully adds the -A/--authorized-fetch flag to enforce HTTP signature verification. The implementation is logical, with changes correctly propagated through the CLI, federation, and handler layers. I've included a couple of review comments on the new test file to suggest improvements for code clarity and to enhance test coverage for an important edge case related to this new security feature.
| import { MemoryKvStore } from "./kv.ts"; | ||
| import { createFederation } from "./middleware.ts"; | ||
|
|
||
| test("handleInbox() with requireHttpSignature option", async () => { |
There was a problem hiding this comment.
The tests for requireHttpSignature are good, but they don't seem to cover a specific edge case. The current tests use an unsigned activity body, so activity is null inside handleInbox before the HTTP signature check. This means the tests only validate the activity == null part of the condition activity == null || (requireHttpSignature ?? false).
It would be beneficial to add a test case for when an activity is successfully parsed from a Linked Data Signature (so activity != null), but requireHttpSignature: true is also set. This would ensure that HTTP signature verification is correctly enforced even when a valid LD-Signature is present.
A suggested test flow would be:
- Create a request with a body containing a valid LD-Signed activity.
- Call
handleInboxwith this request andrequireHttpSignature: true. It should fail with a 401 because there's no HTTP signature. - Sign the same request with an HTTP signature.
- Call
handleInboxagain. It should now succeed with a 202.
This would provide more complete coverage for this new security feature.
| recipient: null, | ||
| context: unsignedContext, | ||
| inboxContextFactory(_activity) { | ||
| return createInboxContext({ ...unsignedContext, clone: undefined }); |
There was a problem hiding this comment.
The clone: undefined property is unnecessary and potentially confusing. The options parameter of createInboxContext is of type RequestContextOptions, which does not have a clone property. The unsignedContext object also does not have a clone property, so there is no need to explicitly set it to undefined. This should be removed for clarity. This applies to all similar occurrences in this file.
| return createInboxContext({ ...unsignedContext, clone: undefined }); | |
| return createInboxContext({ ...unsignedContext }); |
|
The latest push to this pull request has been published to JSR and npm as a pre-release:
|
|
The docs for this pull request have been published: |
There was a problem hiding this comment.
Thank you for working on this feature! The -A/--authorized-fetch option for fedify inbox will be very helpful for testing. However, I'd like to suggest a different implementation approach that better aligns with Fedify's existing access control architecture.
I believe using .authorize() is the more “Fedify way” to implement this feature. Would you be open to refactoring the implementation to use the existing callback API? I'm happy to provide more guidance or examples if needed.
| * By default, this is `false` | ||
| * @since 2.0.0 | ||
| */ | ||
| requireHttpSignature?: boolean; |
There was a problem hiding this comment.
The current implementation adds a new requireHttpSignature field to FederationFetchOptions and InboxHandlerParameters, which duplicates functionality that already exists in Fedify's .authorize() callback API.
According to our access control documentation, the standard way to enable authorized fetch in Fedify is through the .authorize() method:
federation
.setActorDispatcher("/users/{identifier}", async (ctx, identifier) => {
// ...
})
.authorize(async (ctx, identifier) => {
const signedKeyOwner = await ctx.getSignedKeyOwner();
if (signedKeyOwner == null) return false;
return !await isBlocked(identifier, signedKeyOwner);
});The .authorize() callback provides a flexible, dispatcher-level mechanism for access control that:
- Follows Fedify's established patterns
- Allows fine-grained control (per-actor, per-collection, etc.)
- Is already documented and understood by users
- Supports complex authorization logic beyond simple HTTP signature checks
Instead of adding a new field that bypasses the existing API, I suggest implementing the -A/--authorized-fetch option by registering an .authorize() callback in the CLI code:
// In packages/cli/src/inbox.tsx
federation
.setInboxListeners("/users/{identifier}/inbox", "/inbox")
.authorize(async (ctx, identifier) => {
if (!command.authorizedFetch) return true; // Allow all if -A not set
const signedKeyOwner = await ctx.getSignedKeyOwner();
return signedKeyOwner != null; // Only allow signed requests
});2 participants
Summary
Adds the
-A/--authorized-fetchflag to thefedify inboxcommand, enabling HTTP signature verification enforcement for all incoming requests.Related Issue
-A/--authorized-fetchoption tofedify inboxcommand #229Changes
-A/--authorized-fetchflag to thefedify inboxcommandrequireHttpSignatureoption toFederationFetchOptionsinterfaceChecklist
deno task test-allon your machine?Testing
cd packages/cli deno run --allow-all src/mod.ts inbox -A --no-tunnelSend unsigned request:
logs