Skip to content

fix(review): bound manifest path instruction globs#1396

Open
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-unbounded-review-path-globs
Open

fix(review): bound manifest path instruction globs#1396
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-unbounded-review-path-globs

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The review.path_instructions[].path field was accepted without per-item length/character limits and was repeatedly compiled into regexes and emitted verbatim into the AI prompt, enabling CPU/memory/AI-budget exhaustion from malicious manifests.
  • The change hardens manifest parsing and AI-review prompt construction so repo-controlled globs cannot trigger repeated expensive regex compilation or unbounded model input.

Description

  • Added MAX_REVIEW_PATH_GUIDANCE_LENGTH and CONTROL_CHARACTER_PATTERN and validate review.path_instructions[].path to reject control characters and truncate to MAX_ITEM_LENGTH during parsing in parseReviewPathInstructions in src/signals/focus-manifest.ts.
  • Changed resolveReviewPathInstructions to normalize changed paths once, compile each manifest glob once via compileManifestPathMatcher, and evaluate those matchers against normalized paths rather than recompiling per path.
  • Bounded the assembled AI-review guidance section to a safe character budget (4,000 chars) and include an omission note when entries are skipped to keep the final prompt size constrained.
  • Added unit tests in test/unit/focus-manifest.test.ts to cover control-character rejection, overlong-path truncation, prompt guidance bounding, and blank-normalized-path behavior.

Testing

  • Ran npx vitest run test/unit/focus-manifest.test.ts and all tests in that file passed (113 tests) covering the new branches and regressions.
  • Ran npm run typecheck and tsc --noEmit which succeeded with no type errors.
  • Attempted coverage: npx vitest --coverage for the unit file and npm run test:coverage for the full suite executed tests, but coverage generation failed due to an unrelated coverage provider/runtime error (TypeError: jsTokens is not a function) coming from the coverage toolchain; this prevented producing a full coverage report locally.
  • npm audit --audit-level=moderate could not complete due to the registry returning 403 Forbidden in this environment.

Codex Task

@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 25, 2026
@superagent-security

Copy link
Copy Markdown

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant