Skip to content

fix(mcp): bound lint-pr-text body file reads#2945

Merged
loopover-orb[bot] merged 3 commits into
mainfrom
codex/fix-lint-pr-text-local-dos-issue
Jul 4, 2026
Merged

fix(mcp): bound lint-pr-text body file reads#2945
loopover-orb[bot] merged 3 commits into
mainfrom
codex/fix-lint-pr-text-local-dos-issue

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The lint-pr-text CLI accepted --body-file paths and used readFileSync after only existsSync, which allowed special files (FIFOs, symlinks, devices) or very large files to hang or exhaust memory.
  • This change hardens local CLI behavior to prevent local hangs and DoS-like memory exhaustion when a repository or automation provides attacker-controlled paths.

Description

  • Add a cliTextFileMaxBytes (1 MiB) limit and import lstatSync in packages/gittensory-mcp/bin/gittensory-mcp.js to enable robust file checks.
  • Introduce readCliTextFile(path, label) which validates existence, requires a regular file (stats.isFile()), enforces the size cap, and then reads the file.
  • Route lint-pr-text, slop-risk, and issue-slop CLI codepaths to use readCliTextFile instead of raw readFileSync, preserving normal functionality for regular files under the cap.
  • Add a unit test in test/unit/mcp-cli-lint-pr-text.test.ts that asserts --body-file rejects directories, symlinks, and oversized files.

Testing

  • Ran git diff --check to validate whitespace/metadata, which passed.
  • Built the MCP bundle with npm run build:mcp, which succeeded.
  • Ran unit tests with npx vitest run test/unit/mcp-cli-lint-pr-text.test.ts test/unit/mcp-cli-slop-risk.test.ts, and all tests passed.

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 4, 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 5421021 Commit Preview URL

Branch Preview URL
Jul 04 2026, 07:59 AM

const stats = lstatSync(path);
if (!stats.isFile()) throw new Error(`${label} file must be a regular file: ${path}`);
if (stats.size > cliTextFileMaxBytes) throw new Error(`${label} file is too large: ${path} (max ${cliTextFileMaxBytes} bytes)`);
return readFileSync(path, "utf8");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 TOCTOU race allows symlink and special-file bypass in readCliTextFile

readCliTextFile checks file type with lstatSync but reads with readFileSync, creating a race window.

Open the file with fs.openSync and O_NOFOLLOW, then fstatSync and read from the file descriptor.

AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="packages/gittensory-mcp/bin/gittensory-mcp.js">
<violation number="1" location="packages/gittensory-mcp/bin/gittensory-mcp.js:1493">
<priority>P2</priority>
<title>TOCTOU race allows symlink and special-file bypass in readCliTextFile</title>
<evidence>The readCliTextFile function validates that a path is a regular file and under the size limit using lstatSync, but then calls readFileSync(path, 'utf8') afterwards. An attacker can replace the regular file with a symlink or FIFO between the lstatSync check and the readFileSync call, bypassing the safety checks. This is a classic time-of-check to time-of-use (TOCTOU) race condition.</evidence>
<recommendation>Replace the separate lstatSync and readFileSync calls with a single atomic operation: open the path with fs.openSync using fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW, run fs.fstatSync on the returned file descriptor to verify it is a regular file and under the size limit, then pass the file descriptor to readFileSync (which accepts an fd in Node.js). This eliminates the race window.</recommendation>
</violation>
</file>

@superagent-security superagent-security Bot added the pr:flagged PR flagged for review by security analysis. label Jul 4, 2026
@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.02%. Comparing base (f7b5abc) to head (5421021).
⚠️ Report is 56 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2945   +/-   ##
=======================================
  Coverage   96.02%   96.02%           
=======================================
  Files         259      259           
  Lines       28406    28406           
  Branches    10339    10339           
=======================================
  Hits        27278    27278           
  Misses        491      491           
  Partials      637      637           
🚀 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 4, 2026
Read --body-file through a single file descriptor (openSync with
O_NOFOLLOW, then fstatSync/readFileSync/closeSync on that fd) instead
of a path-based stat-then-read pair, so a symlink or special file
swapped in between the two calls can no longer bypass the isFile()
and size checks.
@superagent-security superagent-security Bot removed the pr:flagged PR flagged for review by security analysis. label Jul 4, 2026
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 4, 2026
@loopover-orb

loopover-orb Bot commented Jul 4, 2026

Copy link
Copy Markdown

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-04 08:01:52 UTC

4 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The change replaces unbounded path-based CLI reads with a descriptor-based helper that rejects missing, non-regular, symlinked-final-component, and oversized files before posting. The implementation closes the TOCTOU gap for the opened file descriptor and caps the actual read, so the visible paths for lint-pr-text, slop-risk, and issue-slop are materially safer. The tests cover the three intended CLI entry points with directory, symlink, and oversized inputs.

Nits — 4 non-blocking
  • nit: packages/gittensory-mcp/bin/gittensory-mcp.js:1485 the comment says this rejects symlinks, but `O_NOFOLLOW` only rejects the final path component; parent-directory symlinks still resolve, so tighten the wording or add a stronger path policy if that matters.
  • nit: packages/gittensory-mcp/bin/gittensory-mcp.js:1494 handles `EMLINK` as a symlink-like failure, but `O_NOFOLLOW` symlink failures are normally `ELOOP`; keep `EMLINK` only with a short rationale or drop it to avoid misleading future readers.
  • packages/gittensory-mcp/bin/gittensory-mcp.js:1485 adjust the helper comment to say “final path component” for `O_NOFOLLOW`, or add explicit parent path validation if the intended contract is no symlinks anywhere in the supplied path.
  • test/unit/mcp-cli-lint-pr-text.test.ts:69 add the same helper-path coverage for a valid file exactly at `1024 * 1024` bytes so the boundary condition is locked in, not only the reject-at-plus-one case.
Signal Result Evidence
Code review ✅ No 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: 60 registered-repo PR(s), 51 merged, 421 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 60 PR(s), 421 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 60 PR(s), 421 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.
  • No action.
  • 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

stats.size only proves the file's size at fstatSync time; a regular
file can still grow before the subsequent read, so trusting the stat
alone left readFileSync unbounded. Read at most cliTextFileMaxBytes + 1
bytes directly from the descriptor and fail if that cap is exceeded.

Also cover the slop-risk and issue-slop CLI entry points, which share
readCliTextFile with lint-pr-text but had no regression test for the
non-regular/oversized rejection paths.

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit d3ea149 into main Jul 4, 2026
13 checks passed
@loopover-orb
loopover-orb Bot deleted the codex/fix-lint-pr-text-local-dos-issue branch July 4, 2026 08:04
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant