fix: drain redirect response and resolve relative URLs in downloadFile#25375
fix: drain redirect response and resolve relative URLs in downloadFile#25375KevinZhao wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
The `downloadFile` function in `github.ts` has the same two issues that were fixed in `fetchJson` (google-gemini#24896): 1. The redirect response body is not consumed, which can cause socket exhaustion under repeated redirects (Node.js keeps the socket open until the response is drained or garbage collected). 2. `res.headers.location` is passed directly to the recursive call without resolving against the current URL. Relative `Location` headers (e.g. `/path/to/file`) would fail with `ERR_INVALID_URL` since `https.get` expects an absolute URL. Both fixes mirror the approach already applied to `fetchJson`.
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 addresses two critical issues in the downloadFile function within github.ts, ensuring consistent behavior with previously applied fixes in fetchJson. By properly draining response streams and resolving relative redirect URLs, the changes improve network resource management and reliability when handling redirected download requests. 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. Footnotes
|
gemini-cli
Bot
added
priority/p3
![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 updates the downloadFile function in the GitHub extension to properly drain response bodies using res.resume() during redirects and to correctly resolve redirect URLs. Review feedback identifies missing resource cleanup in error paths that could lead to socket leaks and a high-severity security risk where the Authorization header might be leaked to untrusted domains during redirects.
| ); | ||
| } | ||
| downloadFile(res.headers.location, dest, options, redirectCount + 1) | ||
| res.resume(); // Drain response body to free the socket |
There was a problem hiding this comment.
While adding res.resume() here is correct, the same logic is missing in the error paths within this redirect block. Specifically, if the redirect count is exceeded (line 538) or if the Location header is missing (line 542), the response body is not drained, leading to a socket leak. To ensure robust resource management, the response should be resumed in all cases where the body is not consumed.
| downloadFile(res.headers.location, dest, options, redirectCount + 1) | ||
| res.resume(); // Drain response body to free the socket | ||
| const redirectUrl = new URL(res.headers.location, url).toString(); | ||
| downloadFile(redirectUrl, dest, options, redirectCount + 1) |
There was a problem hiding this comment.
Following redirects blindly while re-adding the Authorization header poses a security risk. If the request is redirected to a different domain (e.g., a CDN like S3 where GitHub release assets are often stored), the GitHub token will be leaked to that third party. According to GitHub's security best practices, the Authorization header should only be sent to trusted GitHub domains (e.g., github.com or api.github.com) and should be stripped when following redirects to external hosts. The implementation should be updated to only include the Authorization header if the target URL is on a trusted domain. Note that since the token is added at the start of the downloadFile function (lines 528-531), the fix should be applied there. This same issue likely exists in the fetchJson function in github_fetch.ts.
1 participant
Follow-up to #24896 — applies the same two fixes to the sibling function
downloadFileingithub.ts.Problem
downloadFilehas the same redirect handling issues that were fixed infetchJson(#24896):Socket leak: The redirect response body is not consumed. Node.js keeps the underlying TCP socket open until the response stream is drained or garbage collected, which can cause socket exhaustion under repeated redirects.
Relative redirect URLs:
res.headers.locationis passed directly to the recursive call without resolving against the current URL. RelativeLocationheaders (e.g./path/to/file) would fail sincehttps.getexpects an absolute URL.Changes
res.resume()before following redirect to drain the response body and release the socketnew URL(res.headers.location, url).toString()to resolve relativeLocationheaders against the current URLBoth fixes mirror the approach already applied to
fetchJsonin #24896.Test plan
downloadFileis used for extension downloads (GitHub release assets), which commonly return 302 redirects to CDN URLs