Skip to content

fix(cache): stop forwarding the GitHub token when following redirects (closes #53) - #158

Merged
harsharajkumar-273 merged 1 commit into
harsharajkumar-273:mainfrom
SakethSumanBathini:fix/53-no-token-on-redirect
Jul 29, 2026
Merged

fix(cache): stop forwarding the GitHub token when following redirects (closes #53)#158
harsharajkumar-273 merged 1 commit into
harsharajkumar-273:mainfrom
SakethSumanBathini:fix/53-no-token-on-redirect

Conversation

@SakethSumanBathini

Copy link
Copy Markdown
Contributor

Summary

_downloadAsset follows redirects itself and was sending Authorization: token …
on every hop. GitHub release assets redirect to pre-signed S3 URLs, so the token was
going to a host that has no business seeing it.

Worth stating the order of severity differently from the issue: the 400 from S3 is
the symptom, but the actual problem is that a GitHub credential was being handed to a
third party. S3 rejecting the request is arguably the lucky part — a host that
accepted it would have received a working token silently.

Fixed by only sending the header when the target is GitHub.

Related Issue

Closes #53

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Refactor
  • Chore / dependency update

Changes Made

The host check is stricter than the one in the issue. hostname.endsWith('github.com')
also matches evilgithub.com, notgithub.com and mygithub.com — anyone able to get
a redirect pointed at a lookalike domain would still receive the token. So:

export const isGitHubHost = (hostname: string): boolean =>
  hostname === 'github.com' || hostname.endsWith('.github.com');

The dot is the whole point. Exported so the classification can be tested directly.

A second bug in the same function. A Location header is allowed to be relative
(RFC 9110), and the value was passed straight back into follow(). There,
url.startsWith('https') would be false for a bare path, so it would pick the http
module and try to request a URL with no host. Redirects are now resolved against the
URL that produced them, and the target is parsed once through new URL() — which also
means a malformed redirect rejects the promise instead of throwing out of the executor.

url.startsWith('https') was also replaced with target.protocol === 'https:', which
is what it was reaching for.

AI Usage

  • I did not use AI for any part of the code in this PR
  • I used AI for coding (specify which tool below) and I fully understand every change I made, including which functions I changed, why I changed them, and what side effects they could create

Used Claude for coding.

Testing

10 new tests in backend/tests/githubCacheStore.test.js, all passing.

The redirect tests run against two real local HTTP servers rather than mocks, so what
is asserted is the headers that actually went over a socket:

assert.equal(destinationHeaders.authorization, undefined,
  'the GitHub token was forwarded to the redirect target');

Covered: github.com and its subdomains accepted; evilgithub.com, notgithub.com
and mygithub.com rejected; objects.githubusercontent.com and
github-releases.s3.amazonaws.com rejected, since those are where a release asset
actually lands; github.com.evil.net rejected; User-Agent and Accept still sent;
the body written from the redirect target rather than the redirect; a relative
Location resolved correctly; a malformed URL rejected; and a redirect loop giving up
with Too many redirects.

Full backend suite: 69 tests, 65 pass, 4 fail — the four being buildQueue,
gitWorkspaceDraft, gitWorkspaceService and server.routes, which fail identically
on a clean main (59/55/4 there). npx tsc --noEmit reports nothing in this file.

Checklist

  • I was assigned to the issue before opening this PR
  • My branch is up to date with main
  • Code works locally and I have tested it
  • PR title follows Conventional Commits format
  • This PR description is written in my own words

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@SakethSumanBathini, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3f0fa0b8-d0c3-4849-885d-f868a8147552

📥 Commits

Reviewing files that changed from the base of the PR and between 2f06e6e and cabff32.

📒 Files selected for processing (2)
  • backend/src/services/githubCacheStore.ts
  • backend/tests/githubCacheStore.test.js
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@harsharajkumar-273 harsharajkumar-273 added ELUSOC Required Tracking ADVENTURER Intermediate (25 pts) labels Jul 29, 2026
@harsharajkumar-273
harsharajkumar-273 merged commit f45508d into harsharajkumar-273:main Jul 29, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ADVENTURER Intermediate (25 pts) ELUSOC Required Tracking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: S3 Authorization Header Leakage and Cache Restore Failure in GitHubCacheStore

2 participants