Skip to content

fix(mcp): gate onboarding pack previews#4379

Merged
JSONbored merged 5 commits into
mainfrom
codex/fix-mcp-onboarding-pack-access-vulnerability
Jul 10, 2026
Merged

fix(mcp): gate onboarding pack previews#4379
JSONbored merged 5 commits into
mainfrom
codex/fix-mcp-onboarding-pack-access-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The MCP onboarding-pack tool used a generic repo-read guard which allowed the shared static mcp credential (when a repo is in MCP_READ_REPO_ALLOWLIST) to retrieve maintainer/API-backed preview data that the HTTP preview route intentionally denies to that credential.
  • Prevent accidental disclosure of private maintainer policy–derived guidance by aligning the MCP tool's authorization with the HTTP maintainer/operator-scoped preview semantics.

Description

  • Add requireRepoOnboardingPackAccess to src/mcp/server.ts to reject the shared static mcp identity and otherwise defer to the usual repo-access checks.
  • Update getRepoOnboardingPack in src/mcp/server.ts to call requireRepoOnboardingPackAccess instead of the generic requireRepoAccess.
  • Add a regression unit test in test/unit/mcp-repo-onboarding-pack.test.ts that asserts the shared static mcp token is denied even when the repo is listed in MCP_READ_REPO_ALLOWLIST, and adjust the unregistered-repo case to exercise the trusted static api identity to preserve existing coverage.

Testing

  • Built the engine with npm --workspace @jsonbored/gittensory-engine run build and ran the focused unit suite with npx vitest run test/unit/mcp-repo-onboarding-pack.test.ts --reporter=verbose, and the test file passed (4 tests, 0 failures).
  • Ran the repository typecheck with npm run typecheck which completed successfully.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4379   +/-   ##
=======================================
  Coverage   94.11%   94.11%           
=======================================
  Files         432      432           
  Lines       38348    38351    +3     
  Branches    13979    13980    +1     
=======================================
+ Hits        36091    36094    +3     
  Misses       1600     1600           
  Partials      657      657           
Files with missing lines Coverage Δ
src/mcp/server.ts 95.59% <100.00%> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored self-assigned this Jul 9, 2026
@JSONbored JSONbored added the gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. label Jul 9, 2026
@JSONbored JSONbored force-pushed the codex/fix-mcp-onboarding-pack-access-vulnerability branch from 3538deb to bf5d9bd Compare July 9, 2026 08:38
@gittensory-orb

gittensory-orb Bot commented Jul 9, 2026

Copy link
Copy Markdown

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-10 08:38:09 UTC

3 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · unstable

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This PR closes a real authorization gap: the MCP onboarding-pack tool previously used the generic requireRepoAccess gate, letting the shared static `mcp` credential retrieve maintainer-scoped preview data via the read allowlist, contrary to the HTTP preview route's intent. The fix adds a narrow requireRepoOnboardingPackAccess wrapper that explicitly rejects the static `mcp` identity before falling back to the normal repo-access check, and the new test verifies denial even when the repo is allowlisted. The change is small, targeted, and the existing test tweak (switching the unregistered-repo case to the trusted static `api` identity) is a sensible adjustment to keep that test isolated from the new gate.

Blockers

Nits — 5 non-blocking
  • src/mcp/server.ts: requireRepoOnboardingPackAccess is only ever called from getRepoOnboardingPack — consider whether a private helper is warranted or if the two-line check could be inlined at the call site for a single-use gate.
  • test/unit/mcp-repo-onboarding-pack.test.ts: only the exact static `mcp` actor is tested against the allowlist; consider also asserting that a legitimate maintainer/owner session with the repo allowlisted still succeeds, to lock in the 'otherwise defer to requireRepoAccess' branch explicitly rather than relying on other pre-existing tests.
  • src/mcp/server.ts: the forbidden error message string is duplicated inline rather than reusing a shared constant/helper if one exists elsewhere in the file for similar maintainer-scoped denials — worth checking for DRY.
  • Add the missing issue reference in the PR description per repo contribution conventions.
  • Consider a positive-path test asserting a maintainer/operator session still succeeds through requireRepoOnboardingPackAccess for symmetry with the new denial test.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 334 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 334 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 48 PR(s), 334 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.
  • 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

@gittensory-orb gittensory-orb Bot added the manual-review Gittensor contributor context label Jul 9, 2026
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 10, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 61cf0a1 Commit Preview URL

Branch Preview URL
Jul 10 2026, 08:26 AM

This PR's own new requireRepoOnboardingPackAccess guard unconditionally
rejects the shared static "mcp" identity -- connectTestClient's default --
so its own added test broke an existing one that relied on that default.
Mirrors the { kind: "static", actor: "api" } pattern already used elsewhere
in this file for tools with the same access requirement.
@JSONbored JSONbored merged commit 9887536 into main Jul 10, 2026
12 checks passed
@JSONbored JSONbored deleted the codex/fix-mcp-onboarding-pack-access-vulnerability branch July 10, 2026 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. manual-review Gittensor contributor context

Projects

Development

Successfully merging this pull request may close these issues.

1 participant