Skip to content

fix(github): evict a cached App JWT on rejection to stop fleet-wide token-minting poisoning - #2480

Merged
JSONbored merged 2 commits into
mainfrom
fix/app-jwt-eviction-2453
Jul 2, 2026
Merged

fix(github): evict a cached App JWT on rejection to stop fleet-wide token-minting poisoning#2480
JSONbored merged 2 commits into
mainfrom
fix/app-jwt-eviction-2453

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • createAppJwt (src/github/app.ts) caches the App-level JWT for ~8 minutes (APP_JWT_REUSE_MS) with no eviction path on rejection, unlike installation tokens — those are evicted and retried once by withInstallationTokenRetry (shipped in fix(github): evict a rejected installation token on PR-mutating calls #2406). A single GitHub-side rejection of the currently-cached App JWT (a transient validation hiccup, a clock-skew edge case, or a brief App suspend/reinstate, with env.GITHUB_APP_PRIVATE_KEY unchanged) kept getting reused by mintInstallationToken on every subsequent installation-token mint attempt, across every installation on the instance, because the function threw straight through on the first non-ok response from the access-tokens endpoint with no retry at all.
  • Adds expireCachedAppJwt (mirrors expireCachedInstallationToken's exact eviction pattern) and has mintInstallationToken evict + re-sign + retry once on a 401 from /app/installations/{id}/access_tokens, bounded the same way withInstallationTokenRetry already bounds installation-token retries — a persistently-bad key (or a genuinely revoked App) still fails cleanly after the one retry, not an infinite loop.
  • Extracted the POST call into a small requestInstallationTokenWithJwt helper so the retry can issue the identical request twice without duplicating the fetch/headers wiring.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage — new code is 100% line+branch covered (verified against coverage/lcov.info for the exact edited range in src/github/app.ts; file totals 181/181 lines, the 4 remaining uncovered branches are pre-existing, outside this PR's lines). Global 96.55%/95.53%.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

Full suite: 6062/6062 passing. Also ran npm run db:migrations:check (unaffected, green).

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. Added: a 401-then-success regression proving the eviction+retry recovers the mint, and a 401-then-401 regression proving the retry is bounded to exactly one attempt (no infinite loop) and still surfaces the failure.
  • API/OpenAPI/MCP behavior is updated and tested where needed. No OpenAPI/MCP surface changed.
  • UI changes — N/A, no UI changed.
  • Visible UI changes — N/A, no UI changed.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. No changelog edit.

Notes

@dosubot dosubot Bot added the size:M label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 07:14:58 UTC

2 files · 1 AI reviewer · no blockers · readiness 86/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change correctly factors the installation-token POST and adds a bounded 401 recovery path that evicts the cached App JWT, signs once again, and still surfaces the original failure path after the retry. The added tests exercise the recovered mint, the bounded retry failure, and the second eviction needed when the retried JWT is rejected too. The implementation is narrowly scoped to the local App-JWT mint path and does not change brokered token behavior.

Nits — 6 non-blocking
  • nit: src/github/app.ts:260 adds a very long explanatory comment in the hot path; the same reasoning is already in the PR and tests, so this should be shortened to the operational invariant.
  • nit: test/unit/github-app.test.ts:217 and test/unit/github-app.test.ts:244 keep long `Response.json` payloads on single lines, which is harder to scan than the surrounding test style.
  • nit: src/github/app.ts:263 retries without consuming or canceling the first 401 response body, which is not a correctness issue but would be cleaner before issuing the second request.
  • src/github/app.ts:260: trim the comment to the invariant: on 401, evict the cached App JWT, retry once, and evict again if the retry is also rejected.
  • src/github/app.ts:263: consider `await response.body?.cancel()` or reading the small error body before retrying so the first failed response is explicitly released.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (size label size:M; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 548 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 548 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 65 PR(s), 548 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Triage stale or unlinked PRs.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
@JSONbored JSONbored self-assigned this Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.95%. Comparing base (4101ca6) to head (39c96c2).
⚠️ Report is 35 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2480      +/-   ##
==========================================
+ Coverage   95.94%   95.95%   +0.01%     
==========================================
  Files         226      226              
  Lines       25361    25436      +75     
  Branches     9229     9247      +18     
==========================================
+ Hits        24333    24408      +75     
  Misses        417      417              
  Partials      611      611              
Files with missing lines Coverage Δ
src/github/app.ts 98.09% <100.00%> (+0.08%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

JSONbored added 2 commits July 2, 2026 00:05
…oken-minting poisoning

createAppJwt caches the App-level JWT for ~8 minutes with no eviction
path on rejection, unlike installation tokens (evicted + retried once
by withInstallationTokenRetry since #2406). A single GitHub-side
rejection of the currently-cached App JWT — a transient validation
hiccup, a clock-skew edge case, or a brief App suspend/reinstate —
kept getting reused by every subsequent installation-token mint
attempt across EVERY installation on the instance, since
mintInstallationToken threw straight through on the first non-ok
response with no retry at all.

Add expireCachedAppJwt (mirroring expireCachedInstallationToken) and
have mintInstallationToken evict + re-sign + retry once on a 401 from
the access-tokens endpoint, bounded the same way
withInstallationTokenRetry already bounds installation-token retries.

Fixes #2453
…jected

createAppJwt caches optimistically before GitHub validates the JWT. The
first-401 retry path evicted and re-signed once, but if that retried
JWT was ALSO rejected, the just-rejected JWT stayed cached and kept
poisoning every installation-token mint fleet-wide for up to
APP_JWT_REUSE_MS -- the exact failure mode the retry was meant to fix
(flagged by the gate's own review).

Evict again on a second 401 before throwing, so the next mint attempt
signs fresh instead of replaying the poisoned JWT.
@JSONbored
JSONbored force-pushed the fix/app-jwt-eviction-2453 branch from 6ccc1f2 to 39c96c2 Compare July 2, 2026 07:06
@JSONbored
JSONbored merged commit c82f6f2 into main Jul 2, 2026
9 checks passed
@JSONbored
JSONbored deleted the fix/app-jwt-eviction-2453 branch July 2, 2026 07:20
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant