Skip to content

fix(cursor): reject overflowing uint32 varints - #749

Merged
testikun merged 17 commits into
openpi-dev:mainfrom
outlier27-cell:fix/issue-748-protobuf-uint32-overflow
Oct 23, 2026
Merged

testikun merged 17 commits into
openpi-dev:mainfrom
outlier27-cell:fix/issue-748-protobuf-uint32-overflow

Conversation

@outlier27-cell

@outlier27-cell outlier27-cell commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Closes #748. Cursor protobuf decoding silently wraps overflowing five-byte uint32 varints when the final byte contains payload beyond the 32-bit range.

Value

Malformed wire values fail explicitly while every valid uint32 value, including 0xffffffff, retains its value.

Approach

Reject a fifth uint32 byte above 0x0f before the JavaScript bitwise shift. Integrate current main f98d6a47 while retaining the existing field-zero guard, entry-local map reader, int64 bound and their tests. Resolve the test-file add/add conflict by keeping the current-main map regression and appending the contributor's Counter regression.

Validation

Final integrated head: 0eb4ce30120bd677d3bf6b93cacf913d9a57a321; tested and committed tree: a507b2481b3e7c3c62921927af8becc40b05c81e. Node 26.8.1, Bun 1.3.14.

  • bun install --frozen-lockfile: passed.
  • node --test --experimental-strip-types tests/extensions/ai-providers/cursor-protobuf*.test.ts: 4 passed, including uint32, int64, field-zero and map-boundary regressions.
  • Direct decoder smoke accepts zero and uint32 maximum, rejects overflowing/continued fifth bytes and truncated input. Independent boundary probe accepts all 8 selected uint32 boundaries and rejects all 240 malformed fifth-byte values.
  • bun run check: passed, exit 0.
  • bun run test: passed, exit 0; Node 2459 total / 2450 passed / 9 skipped / 0 failed; UI 88 files / 1338 passed / 0 failed.
  • Source integration only; no provider/model call, installed-Pi or UI acceptance claimed.

Two independent final source reviews passed on this exact head. Current-head CI run 38696103400 passed all required checks, including Windows. Merge remains subject to the repository's independent approval requirement.

Impact

  • User-visible: invalid Cursor protobuf wire data throws instead of silently wrapping.
  • Model-visible context/tools: no schema changes.
  • Runtime/lifecycle: shared uint32 parsing also covers tags, lengths and bools; only malformed out-of-range wire data becomes rejected.
  • Persisted configuration/data: no migration or settings change.
  • Compatibility/risk: two-file decoder/test change; valid uint32, current int32/int64 decoding and earlier malformed-wire protections remain intact.

@outlier27-cell

Copy link
Copy Markdown
Contributor Author

@tt-a1i 更新:解析器相关 CI 已通过。当前唯一失败是与本 PR 无关的 Web E2E 分片(session history / prompt admission),50 个同分片测试通过;贡献者权限不允许重新运行该任务。请协助重新运行该 CI 任务,确认后本 PR 即可审核。

@outlier27-cell

Copy link
Copy Markdown
Contributor Author

@tt-a1i 请审核此 PR。最新精确提交的 GitHub CI 已全部通过,uint32 五字节边界回归及完整校验均通过;现仅等待维护者审核。

@outlier27-cell

Copy link
Copy Markdown
Contributor Author

@testikun @tt-a1i 请审核此 PR。最新提交的 CI 已通过,当前仅等待维护者审核与合并。

tt-a1i
tt-a1i previously approved these changes Oct 13, 2026

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed head: f703bae373e50e899117c207c43d52c871156917 against main a18f457687b45c877bc8a15d689a7580c4f0659a.

Standards: no confirmed code-level blocker in the scoped change.

Spec: the inspected change matches the stated behavior in this PR. The parser changes share implementation/test files; keep the independent malformed-wire regressions when integrating.

Validation: node --test --experimental-strip-types tests/extensions/ai-providers/cursor-protobuf.test.ts: exit=0; ℹ tests 1; ℹ pass 1; ℹ fail 0; ℹ skipped 0

The unchanged-head focused receipts are reused after a live head/main refresh; #718 uses this round's latest-head tests. This code review does not claim installed-runtime, live-provider, full merged-queue or visual acceptance. No merge is authorized by this review.

@tt-a1i

tt-a1i commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator

Thanks! This was approved, but after the related fixes merged today (#745 / #739, #751, #753) it now conflicts with main. Could you rebase or merge main and resolve the conflict? I'll merge once CI is green on the updated head.

@testikun testikun left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

审查提交:ebb5b7c165a1290e8386289b5870dc6c6fc6b1ec

已审查本次提交,未发现阻塞问题,Approve。

uint32 在第五字节参与位运算前拒绝超出低四位的编码,合法最大值和补零编码仍可解析;int32/enum 的有符号路径使用独立 uint64 reader,未被本次检查误伤。错误传播和调用方清理路径符合现有契约。

本地验证:macOS / Node v24.20.0,独立 worktree;protobuf 及 Cursor 相关测试 44/44 通过;8 项独立探针覆盖 10,000 个确定性值、全部 16 个合法/240 个非法第五字节、截断、tag/length、packed、map、oneof、整数边界及真实 exec ID。新增回归在基线按预期失败,在当前提交通过。git diff --check 通过;当前提交远端 CI 18/18 成功。

本次未重跑全量 check/test,未进行 live Cursor、已安装 Pi 或 Windows 本地验收。PR 当前落后于 main;此次批准针对下述审查提交,更新后的提交仍需满足仓库同步和 CI 要求。

@testikun testikun left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed exact current head 8f55595f8c15ec1a15ca9dd02e28f95eccc73efa on main abc59e74e97cfe8e507131547ac4c91666a000ae. No outstanding scoped P1/P2 findings.

The contributor synced current main into the topic branch in this head (parents: prior reviewed ebb5b7c and current main abc59e7). Main is an ancestor. Its final diff against main is byte-for-byte identical to the previously approved two-file patch: three uint32 guard lines and nineteen regression-test lines. The scoped production/test files and Cursor callers are unchanged between the earlier reviewed base and current main. No further source change or history rewrite was needed. Original authorship/history is preserved.

Inspected the uint32 fifth-byte bound, field-zero guard, map-entry-local reader, signed/unsigned int64 boundary, provider stream failure/cleanup, discovery null fallback and tool JSON decoding. All existing protections and map/uint32 regressions are retained.

Validation in a new detached worktree with its own frozen Bun 1.3.14 dependencies, Node v24.20.0 and umask 0022: all 44 focused Cursor/protobuf/context tests passed; all eight independent boundary probes passed, including 10,000 deterministic uint32 values, all legal/invalid fifth bytes, scalar/container/exec-ID paths and an expected failure of the added regression against current main. git diff --check, bun run check and full bun run test passed: Node 2,421 passed / 0 failed / 8 skipped; UI 1,640 passed across all 107 files. Worktree is clean.

Exact-head remote CI is still running. Merge remains conditional on all current checks completing SUCCESS, valid reviews and repository gates. The eventual squash must retain the original PR author and explicit deduplicated Co-authored-by trailers for 杨嘉伟 / outlier27-cell and tt-a1i, whose earlier test conflict-resolution contribution is retained. No live provider/model call, installed-user-runtime smoke, real TUI/visual acceptance or manual Windows proof is claimed.

@testikun
testikun merged commit 31b2d04 into openpi-dev:main Oct 23, 2026
18 checks passed
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.

bug(cursor): reject overflowing terminating uint32 varints

3 participants