Skip to content

fix(api): block maintainer freeze override manifests - #4453

Merged
JSONbored merged 5 commits into
mainfrom
codex/propose-fix-for-api-security-vulnerability
Jul 10, 2026
Merged

fix(api): block maintainer freeze override manifests#4453
JSONbored merged 5 commits into
mainfrom
codex/propose-fix-for-api-security-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent a privilege-escalation where a maintainer-writable focus-manifest API could persist settings.agentGlobalFreezeOverride (source api_record) and thereby bypass the operator's DB-backed global agent freeze for that repo.

Description

  • Add stripMaintainerFocusManifestSettings(raw) in src/api/routes.ts to remove the operator-only settings.agentGlobalFreezeOverride field from incoming maintainer PUT payloads before parsing/persisting.
  • Wire the sanitizer into the focus-manifest PUT /v1/repos/:owner/:repo/focus-manifest handler so maintainer-supplied JSON is stripped of the operator-only override prior to upsertRepoFocusManifest(..., "api_record").
  • Add a regression route test in test/unit/routes-focus-manifest.test.ts that proves a repo owner with live GitHub write permission can set ordinary manifest settings such as agentDryRun while an attempted agentGlobalFreezeOverride: true is dropped and the DB-backed freeze remains effective.
  • Changes touch src/api/routes.ts and test/unit/routes-focus-manifest.test.ts only to minimally remediate the trust-boundary issue without changing settings resolution logic elsewhere.

Testing

  • Ran git diff --check with no problems.
  • Typecheck: npx tsc --noEmit succeeded.
  • Build: npm run build:miner succeeded.
  • Unit tests: npx vitest run test/unit/routes-focus-manifest.test.ts passed (15 tests in the file all green).
  • Coverage attempt: running the targeted Vitest coverage invocation completed the tests but coverage-report generation failed due to a local ast-v8-to-istanbul / jsTokens is not a function tooling error (tests still passed); full repo coverage (npm run test:coverage) was not executed in this environment.

Codex Task

@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 (ac648b5).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4453   +/-   ##
=======================================
  Coverage   94.11%   94.11%           
=======================================
  Files         432      432           
  Lines       38348    38354    +6     
  Branches    13979    13981    +2     
=======================================
+ Hits        36091    36097    +6     
  Misses       1600     1600           
  Partials      657      657           
Files with missing lines Coverage Δ
src/api/routes.ts 94.26% <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.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 9, 2026
@loopover-orb

loopover-orb Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

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

2 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 PUT /v1/repos/:owner/:repo/focus-manifest handler now runs incoming maintainer settings through a new stripMaintainerFocusManifestSettings() before persisting via upsertRepoFocusManifest(..., "api_record"), removing settings.agentGlobalFreezeOverride so a maintainer-writable manifest can no longer flip the operator-only DB-backed global freeze. The sanitizer correctly narrows to object/non-array/has-key before destructuring the field out immutably, and the added test exercises the real path end-to-end: it goes through the actual route with a live-permission session, sets the DB freeze via setGlobalAgentFrozen, asserts the response manifest has agentDryRun preserved and agentGlobalFreezeOverride stripped, then feeds the result through resolveEffectiveSettings and isDbFrozenForRepo to prove the freeze stays effective — this is a real regression test, not a fabricated one. The change is narrowly scoped to the described trust-boundary fix with no unrelated churn.

Nits — 4 non-blocking
  • codecov/patch is 85.71% vs the 99% target — the null/non-object `settings` early-return branch and the array-typed `settings` branch in stripMaintainerFocusManifestSettings (src/api/routes.ts) don't look covered by the added test, which only exercises the happy path where settings is a plain object containing the override key.
  • Worth confirming there's no other maintainer-reachable write path that persists a focus-manifest with source `api_record` (e.g. a bulk-import or PATCH-style endpoint) that bypasses this same sanitizer — the fix is applied at one call site only.
  • Add a couple of cheap unit tests directly against stripMaintainerFocusManifestSettings for `settings: null`, `settings` as an array, and payloads with no `settings` key at all, to close the branch-coverage gap without needing full route round-trips.
  • Consider naming the sanitizer's guard against non-object `raw` explicitly in a comment or type guard util if this pattern is used elsewhere for other operator-only fields, to keep future additions (e.g. other override flags) consistent.

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

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 9, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

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

@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 ac648b5 Commit Preview URL

Branch Preview URL
Jul 10 2026, 08:26 AM

…guards

codecov/patch was failing at 85.71% -- only the "settings has the override
key" happy path was tested. Adds cases for a non-object/array top-level body
and a null/non-object/array/no-override-key settings value, so every OR-chain
operand is independently exercised. Splits out the null-raw check (genuinely
unreachable via the route's own prior 400-on-null-body guard) with a targeted
v8-ignore rather than leaving it silently uncovered.
@JSONbored
JSONbored merged commit 0ab4d02 into main Jul 10, 2026
12 checks passed
@JSONbored
JSONbored deleted the codex/propose-fix-for-api-security-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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant