fix(node-fetch): redirect with correct status codes#2049
Merged
Conversation
Contributor
|
Warning Rate limit exceeded@ardatan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughA patch was applied to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Fetch
participant Utils
participant RedirectHandler
Client->>+Fetch: Send fetch request
Fetch->>+Utils: Call shouldRedirect(status)
Utils-->>-Fetch: Return true/false
alt Redirect Allowed (true)
Fetch->>RedirectHandler: Handle redirect (check 'follow' vs 'error')
RedirectHandler-->>Fetch: Return new or error response
Fetch-->>Client: Deliver final response
else Redirect Not Allowed
Fetch-->>Client: Return error without redirect
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
✅
|
Contributor
✅
|
Contributor
Contributor
✅
|
Contributor
✅
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/node-fetch/src/fetchNodeHttp.ts (1)
84-108: 🛠️ Refactor suggestionEnhance redirect handling with additional safeguards.
The redirect implementation needs additional safeguards:
- Missing redirect count limit to prevent infinite loops
- Missing check for same-origin redirects (if required)
- Generic error message for redirect=error case
Consider this enhanced implementation:
+const MAX_REDIRECT = 20; +let redirectCount = 0; if (nodeResponse.headers.location && shouldRedirect(nodeResponse.statusCode)) { if (fetchRequest.redirect === 'error') { - const redirectError = new Error('Redirects are not allowed'); + const redirectError = new Error( + `Redirect response received with status ${nodeResponse.statusCode} but 'redirect: error' was set` + ); reject(redirectError); nodeResponse.resume(); return; } if (fetchRequest.redirect === 'follow') { + if (redirectCount >= MAX_REDIRECT) { + reject(new Error(`Maximum redirect count (${MAX_REDIRECT}) exceeded`)); + nodeResponse.resume(); + return; + } + redirectCount++; const redirectedUrl = new PonyfillURL( nodeResponse.headers.location, fetchRequest.parsedUrl || fetchRequest.url, ); const redirectResponse$ = fetchNodeHttp( new PonyfillRequest(redirectedUrl, fetchRequest), );
🧹 Nitpick comments (3)
packages/node-fetch/tests/redirect.spec.ts (1)
6-59: Enhance test coverage with additional redirection scenarios.The test suite is well-structured but could benefit from additional test cases to ensure robust redirect handling:
- Test the
redirect=manualoption- Test maximum redirect limit (prevent infinite redirects)
- Test both relative and absolute redirect URLs
- Test redirect loop detection
Here's a suggested implementation for additional test cases:
describe('Redirections', () => { runTestsForEachFetchImpl((_, { fetchAPI }) => { const redirectionStatusCodes = [301, 302, 303, 307, 308]; const nonRedirectionLocationStatusCodes = [200, 201, 204, 304]; + const maxRedirects = 20; // Match the limit in fetchCurl.ts const requestListener = jest.fn((req: IncomingMessage, res: ServerResponse) => { if (req.url?.startsWith('/status-')) { const [_, statusCode] = req.url.split('-'); res.writeHead(Number(statusCode), { Location: '/redirected', }); res.end(); } else if (req.url === '/redirected') { res.writeHead(200); res.end('redirected'); + } else if (req.url === '/redirect-loop') { + res.writeHead(301, { + Location: '/redirect-loop', + }); + res.end(); + } else if (req.url === '/absolute-redirect') { + res.writeHead(301, { + Location: `http://localhost:${addressInfo.port}/redirected`, + }); + res.end(); } }); + it('should handle redirect=manual option', async () => { + const res = await fetchAPI.fetch( + `http://localhost:${addressInfo.port}/status-301`, + { redirect: 'manual' } + ); + expect(res.status).toBe(301); + expect(res.headers.get('Location')).toBe('/redirected'); + }); + it('should detect redirect loops', async () => { + await expect( + fetchAPI.fetch(`http://localhost:${addressInfo.port}/redirect-loop`) + ).rejects.toThrow(/maximum.*redirects/i); + }); + it('should handle absolute redirect URLs', async () => { + const res = await fetchAPI.fetch( + `http://localhost:${addressInfo.port}/absolute-redirect` + ); + expect(res.status).toBe(200); + expect(await res.text()).toBe('redirected'); + });packages/node-fetch/src/utils.ts (1)
102-104: Enhance the shouldRedirect utility function.While the function correctly implements HTTP redirect status code checks, it could be improved for better maintainability and documentation.
Consider this enhanced implementation:
+/** HTTP redirect status codes as per RFC 7231 and RFC 7238 */ +export const REDIRECT_STATUS = { + MOVED_PERMANENTLY: 301, + FOUND: 302, + SEE_OTHER: 303, + TEMPORARY_REDIRECT: 307, + PERMANENT_REDIRECT: 308, +} as const; + +/** + * Determines if the given HTTP status code indicates a redirect response + * @param {number} [status] - HTTP status code + * @returns {boolean} true if the status code indicates a redirect + */ export function shouldRedirect(status?: number): boolean { - return status === 301 || status === 302 || status === 303 || status === 307 || status === 308; + if (typeof status !== 'number') return false; + return Object.values(REDIRECT_STATUS).includes(status); }packages/node-fetch/src/fetchCurl.ts (1)
64-65: Consider exposing redirect count for consistency.While libcurl handles redirect limits internally, consider exposing the redirect count to maintain consistency with the fetchNodeHttp implementation.
curlHandle.setOpt('FOLLOWLOCATION', fetchRequest.redirect === 'follow'); curlHandle.setOpt('MAXREDIRS', 20); + // Track redirect count for consistency with fetchNodeHttp + let redirectCount = 0; + curlHandle.setOpt('HEADERFUNCTION', (header: Buffer) => { + const headerStr = header.toString(); + if (headerStr.startsWith('HTTP/')) { + redirectCount++; + } + return header.length; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/few-papayas-fry.md(1 hunks)packages/node-fetch/src/fetchCurl.ts(2 hunks)packages/node-fetch/src/fetchNodeHttp.ts(2 hunks)packages/node-fetch/src/utils.ts(1 hunks)packages/node-fetch/tests/redirect.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: alpha / snapshot
- GitHub Check: server (undici)
- GitHub Check: unit / node 18
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: server (ponyfill)
- GitHub Check: lint
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: esm
- GitHub Check: server (native)
- GitHub Check: e2e / azure-function
- GitHub Check: type check
- GitHub Check: e2e / aws-lambda
- GitHub Check: prettier
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
.changeset/few-papayas-fry.md (1)
1-6: LGTM! The changeset is well-structured.The changeset correctly specifies a patch version bump for the bug fix and includes a clear description that aligns with the PR objectives.
Contributor
✅
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes ardatan/graphql-mesh#8349
Summary by CodeRabbit
Bug Fixes
Tests