Fixed sending activities to path based redirects #324
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @allouis, 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 addresses a critical bug in the doubleKnock function that prevented it from correctly following HTTP redirects when the Location header specified a relative path. The changes ensure that all redirect URLs, whether absolute or relative, are properly resolved against the original request's context, significantly improving the reliability of activity sending. A new regression test has also been included to validate the fix.
Highlights
- Bug Fix: Path-Based Redirects: I've identified and fixed a bug in the
doubleKnockfunction where it failed to correctly handle HTTP redirects when theLocationheader provided a relative path (e.g.,/some/path) instead of a full URL. The previous implementation would not correctly resolve these relative paths, leading to failed requests. - Improved URL Resolution: The core change involves modifying the
createRedirectRequestfunction to use theURLconstructor with both thelocationheader value and the original request's URL as a base. This ensures that relative redirect paths are properly resolved into absolute URLs, allowingdoubleKnockto follow redirects correctly regardless of theLocationheader format. - Regression Test Added: A new regression test has been added to
fedify/sig/http.test.tsthat specifically reproduces the bug with path-based redirects and verifies that the fix correctly allowsdoubleKnockto complete the request successfully. This ensures the bug does not re-emerge in the future.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
github-actions
bot
added
component/federation
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in handling redirects where the Location header contains a relative path. The change in fedify/sig/http.ts to use the original request URL as a base for resolving the redirect location is the right approach. The addition of a regression test in fedify/sig/http.test.ts is excellent for preventing future regressions. I have one suggestion to make the new test even more robust.
|
The docs for this pull request have been published: |
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
Good catch. Thanks for your contribution! Could you make this pull request target at the 1.6-maintenance branch instead of the main branch?
allouis
force-pushed
the
handle-redirects
branch
from
729f2bb to
3fcf258
Compare
When the Location header contains just the path to redirect to Fedify will error with something like `TypeError Failed to parse URL from ...` This ensures that the path will be resolved relative to the original URL
|
The latest push to this pull request has been published to JSR and npm as a pre-release: |
|
This fix is shipped with v1.6.7 and v1.7.6 releases. |
ref fedify-dev/fedify#335 ref fedify-dev/fedify#324 This bump to fedify includes two fixes the the doubleKnock implementation. One which is an attempt to remedy the 'unusable' errors when cloning the request objects, and the other which handles relative paths in the Location header when recieving a redirect. Both of these errors cause significant noise in our production environment and cleaning them up should allow us to spot real errors a lot easier in future.
ref fedify-dev/fedify#335 ref fedify-dev/fedify#324 This bump to fedify includes two fixes the the doubleKnock implementation. One which is an attempt to remedy the 'unusable' errors when cloning the request objects, and the other which handles relative paths in the Location header when recieving a redirect. Both of these errors cause significant noise in our production environment and cleaning them up should allow us to spot real errors a lot easier in future.
2 participants
Summary
There is a bug when handling redirects in
doubleKnockwhen theLocationheader includes a path, rather than a full URLChanges
Benefits
This allows full URLs to continue to work, whilst paths will be resolved relative to the original
Checklist
deno task test-allon your machine?