Skip to content

fix(orb): cache brokered installation tokens - #1657

Merged
JSONbored merged 1 commit into
mainfrom
codex/fix-github-token-minting-vulnerability
Jun 28, 2026
Merged

fix(orb): cache brokered installation tokens#1657
JSONbored merged 1 commit into
mainfrom
codex/fix-github-token-minting-vulnerability

Conversation

@JSONbored

@JSONbored JSONbored commented Jun 28, 2026

Copy link
Copy Markdown
Owner

Summary

No issue because this is maintainer-side Orb broker hardening for an already shipped token-exchange path. This keeps repeated valid /v1/orb/token exchanges from forcing a fresh GitHub installation token mint while the previous encrypted cache entry is still safely fresh.

What changed

  • Reads cached_token_json from orb_enrollments and reuses a decryptable, non-expiring cached installation token when available.
  • Writes freshly minted installation tokens back to the enrollment row encrypted with TOKEN_ENCRYPTION_SECRET.
  • Keeps cache and last_token_at writes best-effort so bookkeeping failures do not fail an otherwise valid token exchange.
  • Adds integration coverage for cache hits, absent/expired/unreadable cache entries, cache-write failures, and timestamp-touch failures.

Migration note

migrations/0081_orb_enrollment_token_cache.sql is already on current main and adds orb_enrollments.cached_token_json, so this branch only needs the broker/runtime and regression-test changes.

Validation

  • npm ci
  • npx vitest run test/integration/orb-broker.test.ts
  • npm run typecheck
  • npm run test:coverage
  • npm run test:ci
  • npm audit --audit-level=moderate
  • git diff --check
  • npm run db:migrations:check

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.52%. Comparing base (2c6241b) to head (a6b036b).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1657   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files         204      204           
  Lines       22196    22218   +22     
  Branches     8025     8029    +4     
=======================================
+ Hits        21202    21224   +22     
  Misses        415      415           
  Partials      579      579           
Files with missing lines Coverage Δ
src/orb/broker.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb

loopover-orb Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review — held for maintainer review

2 files · 1 AI reviewers · 1 blocker · readiness 55/100 · CI green · unknown

⏸️ Held for maintainer review — Readiness score is below the configured threshold — Address the short explicit PR panel actions, then re-run the gate.

Review summary
The broker now reuses encrypted cached installation tokens after rechecking enrollment and installation eligibility, and falls back to minting when the cache is absent, stale, unreadable, or unavailable. The core runtime path is sound: cached tokens are not served for revoked enrollments or ineligible installations, and cache/write timestamp failures are intentionally fail-open for token exchange. The main gap is test hygiene and coverage around the actual expiry boundary and invalid cache shapes, not a functional blocker in the changed broker logic.

Concerns raised — review before merging

  • Readiness score is below the configured threshold — Address the short explicit PR panel actions, then re-run the gate.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewers, synthesized
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Review load ❌ 8/20 Readiness component derived from cached public PR metadata and labels; size label size:M.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 29 open PR(s), 15 likely reviewable, 14 unlinked.
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 80 PR(s), 290 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Nits — 5 non-blocking
  • nit: test/integration/orb-broker.test.ts should restore spies with `vi.restoreAllMocks()` in `afterEach`, because the `console.warn` and `db(e).prepare` spies created in these tests otherwise remain mocked beyond the test that installed them.
  • nit: src/orb/broker.ts:100 casts parsed cache JSON directly to the expected shape; malformed objects currently fall through via catch/null, but explicit shape checks would make the cache format contract easier to maintain.
  • nit: test/integration/orb-broker.test.ts:84 covers an expired cache entry only with intentionally bad ciphertext, so it does not independently prove that a decryptable token below the 10-minute remaining window is rejected.
  • Add `vi.restoreAllMocks()` to `test/integration/orb-broker.test.ts` afterEach alongside `vi.useRealTimers()` and `vi.unstubAllGlobals()`.
  • Add a regression case that writes a valid encrypted cached token expiring within `ORB_TOKEN_CACHE_MIN_REMAINING_MS` and verifies `brokerOrbToken` remints.
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: 80 PR(s), 290 issue(s).
  • Related work: Titles/paths share 8 meaningful terms. (PR #1661, PR #1662)
  • Related work: Titles/paths share 6 meaningful terms. (PR #1658, PR #1662)
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Review top overlaps.
  • Add scope summary.
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

The broker now reuses encrypted cached installation tokens after rechecking enrollment and installation eligibility, and falls back to minting when the cache is absent, stale, unreadable, or unavailable. The core runtime path is sound: cached tokens are not served for revoked enrollments or ineligible installations, and cache/write timestamp failures are intentionally fail-open for token exchange. The main gap is test hygiene and coverage around the actual expiry boundary and invalid cache shapes, not a functional blocker in the changed broker logic.

Nits (5)

  • nit: test/integration/orb-broker.test.ts should restore spies with `vi.restoreAllMocks()` in `afterEach`, because the `console.warn` and `db(e).prepare` spies created in these tests otherwise remain mocked beyond the test that installed them.
  • nit: src/orb/broker.ts:100 casts parsed cache JSON directly to the expected shape; malformed objects currently fall through via catch/null, but explicit shape checks would make the cache format contract easier to maintain.
  • nit: test/integration/orb-broker.test.ts:84 covers an expired cache entry only with intentionally bad ciphertext, so it does not independently prove that a decryptable token below the 10-minute remaining window is rejected.
  • Add `vi.restoreAllMocks()` to `test/integration/orb-broker.test.ts` afterEach alongside `vi.useRealTimers()` and `vi.unstubAllGlobals()`.
  • Add a regression case that writes a valid encrypted cached token expiring within `ORB_TOKEN_CACHE_MIN_REMAINING_MS` and verifies `brokerOrbToken` remints.

🟩 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 Jun 28, 2026
@JSONbored
JSONbored force-pushed the codex/fix-github-token-minting-vulnerability branch from ba4cf0b to a6b036b Compare June 28, 2026 11:10
@JSONbored
JSONbored merged commit ee62a4d into main Jun 28, 2026
19 checks passed
@JSONbored
JSONbored deleted the codex/fix-github-token-minting-vulnerability branch June 28, 2026 11:40
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

None yet

Development

Successfully merging this pull request may close these issues.

1 participant