Skip to content

fix(core): clamp sanitizeUnicodeText's code-point cap to zero - #5820

Open
ggbdpq wants to merge 1 commit into
apache:mainfrom
ggbdpq:fix/cap-code-points-boundary
Open

ggbdpq wants to merge 1 commit into
apache:mainfrom
ggbdpq:fix/cap-code-points-boundary

Conversation

@ggbdpq

@ggbdpq ggbdpq commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Summary

#5537 reported a boundary bypass in capCodePoints (packages/core/src/thread-search.ts): for maxCodePoints <= 0 the truncation's negative slice end kept nearly the whole string. That helper was removed with the TypeScript thread-search stack by 8d82007, but the same boundary bug survives in the sibling the issue cites as the correct one: sanitizeUnicodeText (packages/core/src/text-sanitize.ts).

With a negative maxCodePoints:

  • the loop's early break (opts.maxCodePoints >= 0 && …) never fires, so the whole input is expanded into the points array;
  • points.slice(0, negative) counts back from the end and keeps nearly the whole string - 'hello' @-2 returned 'hel…', 3 of 5 code points under a cap that allows none;
  • an empty input returned the '…' suffix instead of '', contradicting the function's own "empty input returns ''" contract.

This PR clamps the cap with Math.max(0, …) so negative budgets behave exactly like 0 - suffix-only for non-empty input, '' for empty - matching truncateUtf16Safe's existing maxUnits <= 0 guard (and its existing "treats a zero or negative budget as empty" test). The now-always-true >= 0 check inside the loop is dropped. No caller passes a negative cap (all call sites use positive constants), so runtime behavior on existing paths is unchanged.

The retarget from the now-deleted capCodePoints to its surviving sibling is explained on the issue.

Verification

check result
New boundary tests, red on the previous code sanitizeUnicodeText('hello', {maxCodePoints: -2}) → 'hel…' (expected '…'); sanitizeUnicodeText('', {maxCodePoints: -1}) → '…' (expected '') - both failed before the fix
Same tests after the fix green, 9/9 in text-sanitize.test.ts
sanitizeUnicodeText cap-boundary coverage previously none (the test file only covered truncateUtf16Safe); the new describe block pins the zero-cap, negative-cap, positive-cap, and surrogate-pair behavior
Full core suite (after a clean rebuild) 918 pass / 0 fail / 0 skipped
biome check on touched files clean
check:asf-headers 4152 covered files pass

Local note: the first full-suite run failed unrelated files (agent-run-hosted-root, deep-research, …) with stale exports from a cross-branch dist; npm run clean && npm run build in the core workspace resolved it (the stale-dist trap is issue #5641's subject).

Does this PR entail a change of behavior?

  • Yes
  • No

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: GLM-5.3-Flash (ZCode) located the surviving boundary and authored the fix and tests under direction.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Issue apache#5537 reported capCodePoints returning nearly the whole string
for maxCodePoints <= 0; that helper (packages/core/src/thread-search.ts)
was removed with the TypeScript thread-search stack by 8d82007 before
the fix could land. The same boundary bug survives in the sibling the
issue cites as correct: sanitizeUnicodeText treats a negative cap as
uncapped - the loop's early break never fires, the whole input is
expanded into the points array, and slice(0, negative) drops only a
couple of trailing code points, so 'hello' @-2 came back as 'hel...'.
An empty input even came back as the ellipsis suffix, contradicting
the "empty input returns ''" contract.

Clamp the cap with Math.max(0, ...) so negative budgets behave exactly
like 0 (suffix-only for non-empty input, '' for empty), matching
truncateUtf16Safe's `maxUnits <= 0` guard; drop the now-always-true
`>= 0` check in the loop. No caller passes a negative cap, so existing
paths are unchanged.

The sanitizeUnicodeText cap boundary had no test coverage; new tests
pin the zero-cap and negative-cap cases (both red on the previous
code), plus the positive-cap and surrogate-pair paths. Core suite
918/918 after a clean rebuild (a stale cross-branch dist initially
failed unrelated files); biome on both touched files and
check:asf-headers clean.

Fixes apache#5537

Generated-by: GLM-5.3-Flash (ZCode)
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 29, 2026

@hqhq1025 hqhq1025 left a comment

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.

Reviewed head e241f16d480a4fd771f63b5e20726c15ea05a9b5. sanitizeUnicodeText now clamps a negative code-point budget to zero before its one-extra-code-point truncation check (packages/core/src/text-sanitize.ts:101-122); this prevents a negative slice index from retaining most of an untrusted string. The focused tests cover positive, zero and negative budgets, empty input, and a supplementary-plane code point (packages/core/src/__tests__/text-sanitize.test.ts:60-80). I found no substantiated P0-P3 issue in these changes. The existing callers pass fixed positive caps, so their normal behavior is unchanged.

On Node 24, the core build and focused text-sanitize tests passed (9/9). I also exercised zero/negative caps with an empty suffix, zero-width removal and NFC normalization. Fresh main merge-tree and git diff --check are clean; this head's hosted test and label checks pass. I did not run the full repository suite or packaged applications.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

This branch has not been deployed

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

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants