-
-
Notifications
You must be signed in to change notification settings - Fork 89
Add -A/--authorized-fetch option to fedify inbox command #527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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
|
|
/gemini review |
There was a problem hiding this comment.
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 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.
| "--authorized-fetch", | ||
| { | ||
| description: | ||
| message`Enable authorized fetch mode. Incoming requests without valid HTTP signatures with 401 Unauthorized will be rejected.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for the --authorized-fetch option is slightly confusing. It could be rephrased for better clarity. The current phrasing "Incoming requests without valid HTTP signatures with 401 Unauthorized will be rejected" could be misinterpreted. A clearer phrasing would be "Incoming requests without valid HTTP signatures will be rejected with 401 Unauthorized."
message`Enable authorized fetch mode. Incoming requests without valid HTTP signatures will be rejected with 401 Unauthorized.`,
There was a problem hiding this comment.
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 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.
| if (authorizedFetchEnabled) { | ||
| const key = await verifyRequest(request, { | ||
| documentLoader: federationDocumentLoader, | ||
| }); | ||
| if (key == null) { | ||
| logger.error( | ||
| "Unauthorized request: HTTP Signature verification failed for {method} {path}", | ||
| { method: request.method, path: pathname }, | ||
| ); | ||
| return new Response("Unauthorized: Invalid or missing HTTP signature", { | ||
| status: 401, | ||
| headers: { "Content-Type": "text/plain; charset=utf-8" }, | ||
| }); | ||
| } | ||
| logger.debug("HTTP Signature verified: {keyId}", { keyId: key.id?.href }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation introduces a performance issue due to double verification of HTTP signatures. The verifyRequest call here performs the first verification. However, the subsequent call to federation.fetch() will trigger another verification inside its handleInbox logic because the federation object is configured with skipSignatureVerification: false by default. This is inefficient as it can lead to redundant network requests for public keys.
To fix this, you could either:
- (Recommended) Refactor
inbox.tsxto create thefederationobject insiderunInbox, allowing you to setskipSignatureVerification: !command.authorizedFetch. This would make the manual verification here unnecessary, as the framework would handle it. - (Workaround) Temporarily set
(federation as any).skipSignatureVerification = true;after a successful manual verification and reset it in afinallyblock around thefederation.fetch()call. This is less ideal as it relies on internal properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dahlia Hi, I need your opinion on this!
| "--authorized-fetch", | ||
| { | ||
| description: | ||
| message`Enable authorized fetch mode. Incoming requests without valid HTTP signatures with 401 Unauthorized will be rejected.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current wording of the description is a bit ambiguous and could be misinterpreted. For better clarity, I suggest rephrasing it to make it clear that the requests are rejected with a 401 status.
message`Enable authorized fetch mode. Incoming requests without valid HTTP signatures will be rejected with 401 Unauthorized.`,
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
Summary
-A/--authorized-fetchoption tofedify inboxcommand401 UnauthorizedChanges
-A/--authorized-fetchCLI option to inbox commandverifyRequest()in the fetch handlerCloses #229
🤖 Generated with Claude Code