fix: reject requests for repos not in the tracked allowlist#142
Conversation
All five GitHub-PAT-proxying routes under /api/gt/repos/[owner]/[name]/ were passing arbitrary owner/name params directly to octokit without checking the tracked repo list, allowing any authenticated dashboard user to read files, READMEs and metadata from private repos accessible to the PAT. Add assertTrackedRepo() helper (src/lib/repos-server) that resolves the live repo list and returns 404 for any owner/name not present. The helper is called at the top of every PAT-proxying GET handler before the first octokit call. Closes MkDev11#141
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new exported helper ChangesRepo Allowlist Authorization
sequenceDiagram
participant Client
participant APIRoute
participant assertTrackedRepo
participant isTrackedRepoServer
participant DB
Client->>APIRoute: GET /api/.../{owner}/{name}
APIRoute->>assertTrackedRepo: assertTrackedRepo(owner, name)
assertTrackedRepo->>isTrackedRepoServer: isTrackedRepoServer(fullName)
isTrackedRepoServer->>DB: DB repo_weights lookup (fallback)
DB-->>isTrackedRepoServer: membership boolean
isTrackedRepoServer-->>assertTrackedRepo: allowed? (true/false)
assertTrackedRepo-->>APIRoute: NextResponse or null
APIRoute-->>Client: 404 short-circuit or continue to GitHub calls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/app/api/related-prs/[owner]/[name]/[number]/route.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/app/api/repos/[owner]/[name]/owner-comments/route.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/lib/assert-tracked-repo.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/assert-tracked-repo.ts (1)
5-5: Update:getLiveReposAsyncServeruses in-memory throttling for upstream fetch, but still hits SQLite every call.
refreshLiveIfStale()caches the latest upstream snapshot inliveByLc/liveConfigJsonByLc, throttles remote fetches withREFRESH_MS(5 min) +lastAttemptAtbackoff, and dedupes concurrent refreshes viainFlight—so no upstream network request per request.getLiveReposAsyncServer()still callsbuildList()→readAll(), which runsSELECT full_name, weight, config_json FROM repo_weightson every invocation (then sorts), so there’s per-request DB overhead remaining.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/assert-tracked-repo.ts` at line 5, The current call to getLiveReposAsyncServer triggers buildList/readAll (running SELECT on repo_weights) on every request; instead use the in-memory cache maintained by refreshLiveIfStale (liveByLc and liveConfigJsonByLc) or change getLiveReposAsyncServer to return the cached snapshot to avoid per-request SQLite hits. Update the code that currently does "const { repos } = await getLiveReposAsyncServer()" so it reads from the cached structures maintained by refreshLiveIfStale (or add a new accessor like getCachedLiveRepos that returns liveByLc/liveConfigJsonByLc) and keep the existing REFRESH_MS/lastAttemptAt/inFlight throttling logic intact to dedupe and backoff remote refreshes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/assert-tracked-repo.ts`:
- Line 5: The current call to getLiveReposAsyncServer triggers buildList/readAll
(running SELECT on repo_weights) on every request; instead use the in-memory
cache maintained by refreshLiveIfStale (liveByLc and liveConfigJsonByLc) or
change getLiveReposAsyncServer to return the cached snapshot to avoid
per-request SQLite hits. Update the code that currently does "const { repos } =
await getLiveReposAsyncServer()" so it reads from the cached structures
maintained by refreshLiveIfStale (or add a new accessor like getCachedLiveRepos
that returns liveByLc/liveConfigJsonByLc) and keep the existing
REFRESH_MS/lastAttemptAt/inFlight throttling logic intact to dedupe and backoff
remote refreshes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a3a5f085-dac9-4e69-a8b9-3ea3e8fd9fef
📒 Files selected for processing (6)
src/app/api/gt/repos/[owner]/[name]/contents/route.tssrc/app/api/gt/repos/[owner]/[name]/contributing/route.tssrc/app/api/gt/repos/[owner]/[name]/health/route.tssrc/app/api/gt/repos/[owner]/[name]/readme/route.tssrc/app/api/gt/repos/[owner]/[name]/route.tssrc/lib/assert-tracked-repo.ts
|
This is a good start, but it does not fully fix the privileged-PAT access-control issue. The allowlist is added to the five
Those routes can still fetch private issue/PR bodies, timeline events, comments, commit metadata, etc. from arbitrary repos accessible to the PAT. Please apply the same tracked-repo allowlist check to all caller-controlled GitHub proxy routes before any |
… routes Apply assertTrackedRepo() to the four remaining PAT-proxying routes missed in the initial fix: - src/app/api/pull/[owner]/[name]/[number]/route.ts - src/app/api/issue/[owner]/[name]/[number]/route.ts - src/app/api/related-issues/[owner]/[name]/[number]/route.ts - src/app/api/issue/[owner]/[name]/[number]/timeline/route.ts Requested in PR MkDev11#142 review by MkDev11.
|
Just updated. Please check again. |
|
Thanks for updating this. The previously flagged issue/pull detail and timeline routes are now covered, but the privileged-PAT allowlist fix is still incomplete. Two caller-controlled routes can still trigger GitHub fetches for arbitrary untracked repos:
Please add the tracked-repo allowlist check to these remaining caller-controlled routes before any helper that can reach GitHub. Until then, the root privileged-PAT access-control issue is not fully fixed, so |
…okup Two more caller-controlled routes reached the privileged PAT without an allowlist check, flagged in PR MkDev11#142 review: - related-prs/[owner]/[name]/[number]: refreshIssueLinkedPrsIfStale -> fetchIssueLinkedPrs -> withRotation - repos/[owner]/[name]/owner-comments: refreshCommentsIfStale -> fetchIssueCommentsFromGithub -> withRotation Audited all 22 [owner]/[name] API routes; these were the only two remaining that transitively hit withRotation. The rest read solely from the local DB / gittensor.io (refreshIssues/PullsIfStale are imported but void-ed, not invoked). Also address the CodeRabbit note about per-request SQLite hits: add isTrackedRepoServer(), an O(1) lookup against the in-memory live snapshot (liveByLc) with a DB fallback only on cold start, and have assertTrackedRepo() use it instead of building+sorting the full repo list on every gated request.
|
Thank you for your review, @MkDev11 , Could you please review this again? |
2 participants
Summary
All five GitHub-PAT-proxying routes under
/api/gt/repos/[owner]/[name]/were forwarding arbitraryowner/nameparams directly to Octokit with no allowlist check, letting any authenticated dashboard user read files, READMEs, and metadata from private repos accessible to the server's PAT.src/lib/assert-tracked-repo.ts— a single async helper that resolves the live repo list viagetLiveReposAsyncServer()and returns a404 Not Foundresponse for anyowner/namenot present in it.assertTrackedRepo()at the top of every PAT-proxying GET handler before the first Octokit call:contents,readme,contributing,health, and the baseroute.ts.Related Issues
Closes #141
Type of Change
Testing
pnpm buildpasses (TypeScript type-check clean)Verification: Requesting
/api/gt/repos/some-random-org/private-repo/contents?path=.envnow returns{"error":"Not found"}with status404. Tracked repos continue to work as before.Checklist
Summary by CodeRabbit