fix: correct post-increment in fetchJson redirect counter#24911
fix: correct post-increment in fetchJson redirect counter#24911enjoykumawat wants to merge 2 commits intogoogle-gemini:mainfrom
Conversation
redirectCount++ evaluates to the current value before incrementing, so every recursive call received redirectCount=0. The guard (redirectCount >= 10) never triggered, allowing infinite redirect loops that cause a stack overflow crash. Change to redirectCount + 1 so each recursive call receives the correctly incremented value, matching the pattern already used in the sibling downloadFile function. Fixes google-gemini#21007
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 an infinite recursion vulnerability in the fetchJson utility. By fixing an incorrect increment operation, the function now properly tracks redirect counts, preventing potential stack overflow crashes when encountering redirect loops. 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-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 fixes a bug in the fetchJson function where the redirect count was not correctly incremented due to the use of a post-increment operator, and adds a corresponding unit test to verify that the function rejects after 10 redirects. A security review identified a critical vulnerability where the GITHUB_TOKEN is leaked to untrusted domains during redirects; it is recommended to strip the Authorization header for cross-origin requests to prevent credential exfiltration.
| return reject(new Error('No location header in redirect response')); | ||
| } | ||
| fetchJson<T>(res.headers.location, redirectCount++) | ||
| fetchJson<T>(res.headers.location, redirectCount + 1) |
There was a problem hiding this comment.
The fetchJson function includes the GITHUB_TOKEN in the Authorization header for all requests. When following redirects (301/302), the function recursively calls itself with the new URL from the location header, which results in the token being sent to the new destination. If a trusted host redirects to an untrusted domain, the sensitive GitHub token will be leaked to the external party.
Remediation: Implement a check to ensure that the Authorization header is only included if the target URL (including redirect locations) belongs to a trusted domain (e.g., github.com). For cross-origin redirects, the token should be stripped. This aligns with the repository requirement to sanitize environment usage and prevent exfiltration of sensitive data.
References
- Sanitize the environment used for variable expansion in HTTP headers to prevent malicious extensions from exfiltrating sensitive system environment variables.
When following 301/302 redirects, the GITHUB_TOKEN was forwarded to the redirect destination without checking the hostname. A trusted GitHub URL redirecting to an untrusted domain would leak the token. Pin the trusted hostname from the initial request URL and only include the Authorization header when the current request hostname matches it. Cross-origin redirect destinations receive no credentials.
|
Addressed the security feedback: added hostname pinning so the |
|
Good point on the token leak. Addressed in the latest commit:
Added a test covering the cross-origin redirect case to verify the token is not forwarded. |
|
I approved #24896 instead |
2 participants
Problem
fetchJsoninpackages/cli/src/config/extensions/github_fetch.tsuses post-increment (redirectCount++) when passing the counter to the recursive call on line 34. Post-increment evaluates to the current value before incrementing, so every recursive call receivesredirectCount = 0. The guardif (redirectCount >= 10)always evaluates0 >= 10 → false, meaning the"Too many redirects"error is unreachable. A server returning redirect loops causes infinite recursion until a Node.js stack overflow crash.Fixes #21007 (also reported as #24893)
Root cause
The sibling
downloadFilefunction ingithub.tsalready usesredirectCount + 1— this fix bringsfetchJsonin line with the established pattern.Changes
packages/cli/src/config/extensions/github_fetch.ts: one-character fix (redirectCount++→redirectCount + 1)packages/cli/src/config/extensions/github_fetch.test.ts: regression test — verifies that 11 consecutive 302 redirects result in"Too many redirects"rejectionTesting
All 9 tests pass, including the new redirect-limit test.