Skip to content

fix(security): bound focus manifest ingestion - #890

Merged
JSONbored merged 3 commits into
mainfrom
codex/propose-fix-for-focus-manifest-vulnerability
Jun 19, 2026
Merged

fix(security): bound focus manifest ingestion#890
JSONbored merged 3 commits into
mainfrom
codex/propose-fix-for-focus-manifest-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The repo-owned fetcher and caller-supplied focusManifest objects were accepted and parsed without an overall byte cap, allowing large or malicious manifests to be fully materialized and parsed which can exhaust Worker memory/CPU and crash the process.

Description

  • Import and reuse MAX_FOCUS_MANIFEST_BYTES and add a focusManifestInputSchema that refines caller-supplied manifests by their JSON-serialized byte length so focusManifest cannot exceed the configured cap in both the API and MCP schemas (src/api/routes.ts, src/mcp/server.ts).
  • Add isJsonByteLengthWithinLimit helper used by the schema refinement and wire the focusManifest fields to focusManifestInputSchema to enforce the serialized-size bound.
  • Add a pre-parse request-body limit (LOCAL_BRANCH_ANALYSIS_MAX_BODY_BYTES) and use readRequestBodyWithLimit + JSON.parse for the /v1/local/branch-analysis and /v1/local/remediation-plan routes so oversized incoming requests are rejected before full JSON materialization, while preserving the existing repo-owned manifest fallback behavior.

Testing

  • Ran unit tests with npm test -- --run test/unit/focus-manifest-loader.test.ts test/unit/focus-manifest.test.ts test/unit/openapi.test.ts, and all tests passed (Test Files 3 passed, Tests 115 passed).
  • Ran npm run typecheck (TypeScript tsc --noEmit) and it completed successfully.
  • Ran git diff --check and it reported no issues.

Codex Task

@ghost

ghost commented Jun 18, 2026

Copy link
Copy Markdown

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal Result Evidence Action
Gate result ⚠️ Skipped #890 is no longer open. No action.

💰 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.

@ghost ghost added the gittensory:reviewed label Jun 18, 2026
@dosubot dosubot Bot added the size:M label Jun 18, 2026
@ghost

ghost commented Jun 18, 2026

Copy link
Copy Markdown

gittensory · advisory review

Reviewed 3 changed file(s) — two independent AI reviewers.

Changed files: src/api/routes.ts, src/mcp/server.ts, test/unit/routes-remediation-plan.test.ts

Suggested action:Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR adds request‑size limits and a serialized‑size guard for focusManifest on the local branch analysis and remediation‑plan endpoints, introduces shared validation utilities, and adds comprehensive tests covering oversized payloads and manifests.

Suggestions

  • Consider extracting isJsonByteLengthWithinLimit and the focusManifest schema into a shared module to avoid duplication between routes.ts and server.ts.
  • Add a brief comment documenting why the 1 MiB limit mirrors the webhook limit for future maintainers.
  • If other endpoints accept a focusManifest, apply the same size validation to keep behavior consistent.

Worth double-checking

  • Ensure MAX_FOCUS_MANIFEST_BYTES is exported from focus-manifest and set to an appropriate value; an overly low limit could reject legitimate manifests.
  • Verify that any external clients expecting larger request bodies are updated, as the new 1 MiB cap may cause 413 responses.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR adds size limits to focusManifest ingestion in the /v1/local/branch-analysis and /v1/local/remediation-plan API routes, as well as in the MCP server, to prevent excessive resource consumption. It implements payload size checks via Content-Length and streamed body limits, plus a JSON byte-length validation for the focusManifest field. The changes follow existing patterns (similar to the webhook handler) and include comprehensive tests covering edge cases.

Suggestions

  • Extract the duplicated isJsonByteLengthWithinLimit helper function into a shared utility module to avoid code duplication between src/api/routes.ts and src/mcp/server.ts.
  • Consider importing LOCAL_BRANCH_ANALYSIS_MAX_BODY_BYTES in the test file instead of redefining MAX_BODY_BYTES to maintain a single source of truth (though acceptable as-is since the constant is only used in routes.ts).

Worth double-checking

  • The isJsonByteLengthWithinLimit function uses JSON.stringify which may throw for non-serializable values (e.g., functions, circular structures). While caught and returning false (treating as oversized), this could mask serialization issues. However, the prior z.record(z.string(), z.unknown()) schema ensures the value is a plain object, making this risk low in practice.
  • The change relies on MAX_FOCUS_MANIFEST_BYTES being correctly defined in ../signals/focus-manifest. If this constant is missing or set too high, the limit may be ineffective. Verify its value and definition in that file.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.07407% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.68%. Comparing base (25b77f1) to head (248e2c2).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/mcp/server.ts 20.00% 4 Missing ⚠️
src/api/routes.ts 86.36% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
- Coverage   96.72%   96.68%   -0.05%     
==========================================
  Files         109      109              
  Lines       14750    14775      +25     
  Branches     5340     5344       +4     
==========================================
+ Hits        14267    14285      +18     
- Misses         95      102       +7     
  Partials      388      388              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@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 Jun 19, 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 248e2c2 Commit Preview URL

Branch Preview URL
Jun 19 2026, 06:03 AM

@JSONbored
JSONbored merged commit f023e56 into main Jun 19, 2026
18 of 20 checks passed
@JSONbored
JSONbored deleted the codex/propose-fix-for-focus-manifest-vulnerability branch June 19, 2026 06:06
@github-actions github-actions Bot mentioned this pull request Jun 19, 2026
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant