[Fix] safeFetch redirect SSRF regression tests#524
Conversation
advancedresearcharray
requested review from
HiddenPuppy,
mvanhorn and
samzong
as code owners
|
Hi @advancedresearcharray, DetailsInstructions for interacting with me using comments are available here. |
Summary of ChangesHello, 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 introduces comprehensive regression tests for 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 the 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 counterproductive. 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. 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 adds three new unit tests to safe-fetch.test.ts to verify redirect handling in safeFetch. The feedback correctly points out that two of these tests mock an unrealistic scenario where net.fetch resolves with a 302 status code despite being configured with redirect: 'error'. Since net.fetch would actually reject with a TypeError in practice, these tests are redundant and should be removed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| it('rejects redirect response pointing to private host without following Location', async () => { | ||
| assertNotPrivateHostMock.mockResolvedValue(null); | ||
| netFetchMock.mockResolvedValue(mockResponse(new ArrayBuffer(0), 302, { Location: 'http://127.0.0.1/admin' })); | ||
|
|
||
| await expect(safeFetch('https://cdn.example.com/img.png')).rejects.toThrow('302'); | ||
| expect(assertNotPrivateHostMock).toHaveBeenCalledTimes(1); | ||
| expect(assertNotPrivateHostMock).toHaveBeenCalledWith('cdn.example.com'); | ||
| }); | ||
|
|
||
| it('rejects redirect response to another HTTPS URL without following Location', async () => { | ||
| assertNotPrivateHostMock.mockResolvedValue(null); | ||
| netFetchMock.mockResolvedValue( | ||
| mockResponse(new ArrayBuffer(0), 302, { Location: 'https://other.example.com/img.png' }), | ||
| ); | ||
|
|
||
| await expect(safeFetch('https://cdn.example.com/img.png')).rejects.toThrow('302'); | ||
| expect(assertNotPrivateHostMock).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
When redirect: 'error' is configured, net.fetch (like standard fetch) will reject the promise with a TypeError when encountering a redirect, rather than resolving with a 302 response.
By mocking netFetchMock.mockResolvedValue(...) with a 302 status, these tests simulate an impossible scenario where net.fetch resolves with a redirect response despite redirect: 'error' being set. In this simulated scenario, safeFetch only rejects because of the fallback if (!res.ok) check, which is already covered by the 'rejects on non-ok HTTP status' test.
Since the actual redirect-blocking behavior is already covered by:
'passes redirect:error to net.fetch to prevent SSRF bypass via 3xx'(verifying the option is passed)'propagates fetch error when redirect:error blocks a 3xx before response'(verifying that the rejection propagates)
These two tests are redundant and mock an unrealistic behavior. They should be removed to avoid confusion.
Add behavioral tests confirming safeFetch rejects 3xx responses pointing at private or alternate HTTPS hosts without following Location, and propagates redirect:error fetch failures. Closes clawwork-ai#406 Signed-off-by: root <root@array-fleet-ops.array.local>
advancedresearcharray
force-pushed
the
fix/406-safefetch-redirect-regression
branch
from
bd943b6 to
7711a4c
Compare
|
Thanks @advancedresearcharray! /approve |
|
I encountered an error while processing your command:
DetailsInstructions for interacting with me using comments are available here. |
|
Branch is mergeable again — the earlier /approve merge failure was transient. Ready for re-merge when convenient. |
|
Thanks @samzong for the review and merge! Re the Gemini review on the two 302 mock tests: they intentionally match the verification steps in #406 (reject a mocked |
2 participants
Summary
redirect: 'error'onnet.fetch) landed in fix(net): stream safeFetch body with incremental maxSize enforcement #487; this PR adds the behavioral regression tests requested in [Bug] safeFetch follows redirects without re-running SSRF check #406.safeFetchrejects 302 responses pointing athttp://127.0.0.1/adminwithout followingLocation.safeFetchrejects 302 responses to alternate HTTPS URLs without a second fetch hop.redirect: 'error'fetch failures propagate instead of silently following redirects.Test plan
pnpm checkpassesvitest run test/safe-fetch.test.ts— 19 tests passCloses #406