fix(signals): subject over-long manifest entries to dedup and list cap#524
Conversation
normalizeStringList truncated an over-long entry then continue'd, bypassing both the !result.includes de-dup and the MAX_LIST_ITEMS cap -- so untrusted manifests could produce duplicated and over-cap (201+ entries fit under the 64KB gate) wantedPaths/blockedPaths/etc. Truncate in place and fall through to the same de-dup and cap logic.
|
Note Gittensory Gate skippedPR closed before full evaluation. No late first comment was created.
Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
JSONbored
left a comment
There was a problem hiding this comment.
Approving. This is a correct, well-scoped fix for a real input-validation bug.
On main, normalizeStringList (src/signals/focus-manifest.ts) truncates an over-long entry, pushes it unconditionally, then continues — so an over-long entry skips both the !result.includes de-dup and the MAX_LIST_ITEMS (200) cap break. Since manifests are parsed from untrusted arbitrary repos under the 64 KB MAX_FOCUS_MANIFEST_BYTES gate, a crafted .gittensory.yml/.json of all over-long entries bypasses both limits (e.g. 201 x 301-char entries fit under 64 KB and yield a 201-entry list past the 200 cap; two over-long entries sharing a 300-char prefix both land after truncation).
The fix truncates in place and falls through to the shared de-dup + cap path — minimal and correct. Tests cover both the de-dup-after-truncation case and the 250-entry -> 200 cap with the warning, and both fail against the old code. Same defect family as the merged #464, different site. CI green.
One optional follow-up (not blocking): the truncated value is the raw 300-char prefix slice, so two distinct entries that differ only after char 300 collapse to one — acceptable here since these are advisory path lists, but worth a one-line note if exact-path fidelity ever matters.
|
reviewbot · verdict: Reviewed 2 changed file(s) — two independent AI reviewers: Reviewer A · Worth double-checking
Reviewer B · Suggestions
Worth double-checking
|
Closes #522.
normalizeStringListtruncated an over-long entry (>MAX_ITEM_LENGTH300) thencontinued, skipping both the!result.includesde-duplication and theMAX_LIST_ITEMS(200) cap that the normal path enforces. Manifests are fetched from untrusted, arbitrary repos, so a crafted.gittensory.yml/.jsondefeated both limits:break; 201 × 301-char entries (~61.5 KB) fit under the 64 KBMAX_FOCUS_MANIFEST_BYTESgate, yielding a 201+ entry list past the 200 cap.Change
continue).exceeded 200 entrieswarning. Both fail on the old code.Verification
focus-manifest.test.ts65/65;tsc --noEmitclean; changed file branch coverage 98.36% (≥ 97% gate). (The one local-only failure ingittensory-focus-manifest.test.tsis a pre-existing Windows CRLF artifact comparing the on-disk.gittensory.yml; it passes in CI on LF and is unrelated to this change.)Same family as merged #464 (dedup-after-cap), different site/defect.