Skip to content

fix(mcp): validate and clamp 'days' argument in list_reports/search#409

Open
nankingjing wants to merge 1 commit into
gsscsd:mainfrom
nankingjing:fix-mcp-days-validation
Open

fix(mcp): validate and clamp 'days' argument in list_reports/search#409
nankingjing wants to merge 1 commit into
gsscsd:mainfrom
nankingjing:fix-mcp-days-validation

Conversation

@nankingjing

Copy link
Copy Markdown

Problem

toolListReports and toolSearch in mcp/src/index.ts compute the number of recent days to return with:

const days = Math.min(Number(args["days"] ?? 7), 30); // list_reports
const days = Math.min(Number(args["days"] ?? 7), 14); // search

This only clamps the upper bound. It has no lower bound and no NaN guard, so a malformed days argument from an MCP client produces incorrect results when fed to dates.slice(0, days):

days argument current days value dates.slice(0, days) result
"abc" (non-numeric) NaN slice(0, NaN)0 reports (silent empty)
0 0 0 reports
-5 (negative) -5 slice(0, -5)"all but the oldest 5"

The negative case is the worst: slice(0, -N) counts from the end, so it returns almost everything and the count is size-dependent — with 3 dates it returns 0, with 10 it returns 5, with 25 it returns 20. A "last -5 days" request should never return more data than "last 3 days".

Fix

Add a small clampDays() helper that parses the argument, rejects non-finite / sub-1 values (falling back to the default), and clamps to the max — then use it in both tools.

function clampDays(raw: unknown, fallback: number, max: number): number {
  const n = Math.floor(Number(raw));
  if (!Number.isFinite(n) || n < 1) return Math.min(fallback, max);
  return Math.min(n, max);
}

Behavior is unchanged for all valid inputs (undefined, null, 3, 40, "5" all behave exactly as before); only garbage input is corrected to the documented default.

Verification

Ran isolated Node tests replicating the slice logic against a fake manifest:

  • Reproduced the buggy behavior above (NaN → 0 reports, -5 → size-dependent result).
  • Confirmed the fix returns the documented default for invalid input and is identical to the old code for every valid input (10/10 cases pass).

Also ran, on the edited file:

  • prettier --check against the repo .prettierrc → clean.
  • tsc --noEmit against mcp/tsconfig.json (strict) → no errors.

No behavioral change to any other code path; no reformatting.

@nankingjing

Copy link
Copy Markdown
Author

Reviewed — clampDays correctly guards NaN/negative/zero with a fallback; much more robust than the bare Math.min. Ready for review.

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