Skip to content

fix(cursor): bound local installer manifests - #6319

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:transfer/690-cursor-manifest-bounds
Sep 30, 2026
Merged

lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:transfer/690-cursor-manifest-bounds

Conversation

@luvs01

@luvs01 luvs01 commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Bound the Cursor local-installer update manifest to 64 KiB before JSON decoding, and reject overlong version and URL fields before caching the hint.
  • Discard oversized streams on a best-effort basis without awaiting cancellation. A stream whose cancel() never settles must still reject immediately.
  • Carry my original changes, source commits 05fafe903bc25182a2fc3cde054c36d769e6c403 and 3d27193a1b6641089747dff308eed12ceb6fca8e. Inspected source head: 085547f0a1097507af40ee258a8a0714707103fb; its CI-only retrigger commit is omitted. Add cumulative-byte, stalled-cancel and exact-boundary regressions.
  • Preserve the existing unavailable-hint reason codes, platform mapping, cache/coalescing and installer-host policy. This lookup does not download or run installers.

Verification

  • Base: cda8ff4e4eb5c48d7af1f5b108e6551fbdc1e5d3; validated head: 09fcf7bc5c35c66b7def4c9bc64b325f7f0f6221. Inspected upstream dev 3008bae77043701fe0f6200c9117ab7e1f9da2ee is 4 commits ahead; its diff has no overlap with these three paths or their instructions/dependency locks. The inspected package change only bumps the release version from 2.74 to 2.75; dependency and test commands are unchanged.
  • Bun 1.4.0: bun test tests/providers/cursor/cursor-local-installer.test.ts: 21 pass, 0 fail. The same tests against the base implementation produced 18 pass, 3 expected failures for oversized fields/body and non-settling-cancel rejection. Valid multichunk JSON, including exactly 64 KiB, remains accepted.
  • bun run typecheck, bun run structure:check, bun run privacy:scan, bun run scripts/file-size-ratchet.ts, and git diff --check: passed.
  • Documented focused-validation resource exception: this independent three-file carry uses focused regressions for both the production fetch dependency and hint/cache behavior, plus the static/structural gates. Repeating the repository-wide import-connected suite for each transfer in this series is disproportionate to the available local validation budget. The full local suite and test:changed have not been run on this head; broader integration and platform coverage remains for exact-head CI. No timeout or baseline was changed.
  • Synthetic responses only; no live installer request was made. Exact-head upstream CI and the CodeRabbit full review have now completed, as detailed below.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Privacy validation passed. Independent source inspection and CodeRabbit's exact-head security architecture review found no introduced or worsened security concern. This does not constitute human maintainer approval or sponsorship; applicable maintainer requirements remain in force before merge.

Review readiness

  • Required local validation passed with its scope documented.
  • Branch meets the documented recent-dev criterion (base and inspected upstream SHA are recorded above).
  • All correct Codex and CodeRabbit findings are fixed.
  • Ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • Oversized installer manifests are now rejected before they can be parsed, and excessively long download URLs or version values are treated as unusable.
    • Manifest downloads within the 64 KiB limit continue to be accepted, including responses delivered in smaller chunks.
  • Documentation
    • Clarified the size limits for manifests, download URLs, and version values.

Exact-head readiness evidence

  • Cross-platform CI completed successfully at 09fcf7bc5c35c66b7def4c9bc64b325f7f0f6221; the aggregate ci, all four test shards and applicable gates/smokes passed. Skipped optional jobs are not counted as passes.
  • CodeRabbit's full review covered all three changed files at this exact head, with no actionable comments or unresolved inline review threads. Its docstring-coverage warning remains a warning. The GitHub Codex request has no confirmed response and is not counted as a completed review.
  • Retained limits: a fetched chunk may itself temporarily exceed 64 KiB before rejection; the implementation bounds accumulated retained body/JSON parsing, not all transport allocation. Exact 256-character version and 4096-character URL acceptance cases are optional additional coverage, not claimed tests. No live installer behavior was tested.
  • Latest readiness inspection found dev at 592c5cfc043cd5b69e8aea0f12b9a0644cc50612, five commits ahead of this base, within the documented ten-commit limit. No overlap with these three changed paths was found. Human maintainer review/sponsorship and merge are separate steps; no merge or deployment is requested.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 50876883-3586-4d24-86bc-a01a75450d8f

📥 Commits

Reviewing files that changed from the base of the PR and between 3008bae and 09fcf7b.

📒 Files selected for processing (3)
  • src/integrations/cursor-local-installer.ts
  • structure/clients/integrations.md
  • tests/providers/cursor/cursor-local-installer.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The Cursor local installer now reads manifest responses as bounded streams. It rejects oversized response bodies, URLs, and version candidates. Tests cover stream handling and field limits, and the integration documentation describes the bounds.

Changes

Cursor manifest bounds

Layer / File(s) Summary
Bounded manifest reading
src/integrations/cursor-local-installer.ts, tests/providers/cursor/cursor-local-installer.test.ts
The installer reads response bodies as streams and rejects bodies over 64 KiB. It attempts to cancel an oversized stream without waiting for cancellation. Tests cover cancellation, valid chunked manifests, and the unreachable hint for an oversized fetched manifest.
Manifest field limits
src/integrations/cursor-local-installer.ts, tests/providers/cursor/cursor-local-installer.test.ts, structure/clients/integrations.md
The installer rejects URLs longer than 4 KiB and version candidates longer than 256 characters. Tests check oversized fields, and the documentation describes the body and field limits.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: halysondev

Merge Risk: ⚪ Minimal · up to 09fcf

The change bounds Cursor installer manifests and rejects oversized fields while preserving valid manifests at the size limit. No actionable merge-blocking issue is established; normal exact-head CI should complete before merging.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 09fcf

The change reduces exposure to oversized manifest inputs without expanding download or installation authority. Existing URL restrictions, failure responses, and cache behavior remain intact. No introduced or worsened security concern was established in the reviewed path.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A malicious manifest response can affect hint availability and the data returned for its host-and-platform cache key. The change narrows accepted input sizes without adding tenant, credential, datastore, or installer-execution authority in the inspected path.

Trust Boundaries and Controls

  • observed — External manifest data crosses into a usable hint only after bounded decoding and field validation. The existing HTTPS downloads.cursor.com/local-mode URL restriction remains in force, and the management caller does not expose the helper's optional updateHost as request input.

Resilience and Maintainability Implications

  • observed — Overflow initiates best-effort cancellation without awaiting settlement and handles cancellation-promise rejection. The lookup failure becomes a settled unavailable hint rather than leaving coalesced callers waiting on cancellation. Existing cache behavior shares in-flight work, applies success or failure expiry after settlement, and permits recovery after expiry. Cancellation completion and total transport memory are not guaranteed by the byte check.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding bounds to Cursor local installer manifests, including manifest size and field limits.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 30, 2026

luvs01 commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Please review the current head 09fcf7bc5c35c66b7def4c9bc64b325f7f0f6221, including the bounded regression where cumulative chunks exceed 64 KiB and stream cancellation never settles. Local focused tests pass 21/21; broader exact-head CI and upstream review remain pending. The PR stays Draft.

@coderabbitai full review
@codex review

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 30 / 80

이 PR은 Cursor 로컬(Private Inference) 설치 안내를 만들 때 쓰는 업데이트 매니페스트 응답에 크기 한도를 겁니다. 베이스는 dev입니다.

예전에는 response.json()으로 본문을 통째로 읽었습니다. 응답이 아주 크거나, version/URL 문자열이 비정상적으로 길면 파싱·캐시 쪽 부담이 커질 수 있습니다. 이번 변경은 (1) 본문을 스트리밍으로 읽되 합이 64 KiB를 넘는 순간 읽고 멈추고, (2) URL은 4 KiB·version(또는 productVersion/name)은 256자를 넘으면 설치 링크로 쓰지 않으며, (3) 한도 초과 때 cancel()이 끝나지 않아도 기다리지 않고 바로 거절합니다. 설치 파일을 받거나 실행하지는 않고, 대시보드에 보여줄 URL만 고릅니다. 문서(structure/clients/integrations.md)와 회귀 테스트(필드 과대·본문 과대·cancel 멈춤·정확히 64 KiB 멀티청크)도 같이 들어왔습니다. types/config 쪼개기로 이 PR 때문에 닫을 무효·중복 PR은 없습니다.

라인 - src/integrations/cursor-local-installer.ts fetchJson · 한도 검사는 read()로 받은 청크 길이를 더한 뒤에 합니다. 한 번에 64 KiB보다 큰 청크가 오면, 거절하기 직전에 그 큰 덩어리가 잠깐 메모리에 올라갈 수 있습니다. 평소 fetch 청크는 작지만, “절대 64 KiB 초과 할당 없음”까지는 보장하지 않습니다.
라인 - tests/providers/cursor/cursor-local-installer.test.ts · 본문은 정확히 64 KiB 허용을 검사하지만, version 256자·URL 4096자 “딱 한계” 통과 케이스는 없습니다. 지금 있는 건 257자 version / 한도 넘는 URL 거절 쪽입니다.
라인 - PR이 Draft이고 본문 checklist도 Ready가 아닙니다. 로컬은 포커스 테스트·일부 정적 게이트까지 적혀 있고, 전체 스위트는 exact-head CI에 맡긴다고 했습니다.

메인테이너의 판단이 필요한 지점

한 청크가 한도를 크게 넘을 때 메모리 스파이크를 더 줄일지(예: Content-Length가 있으면 미리 거절), 아니면 지금 스트리밍+누적 상한으로 충분한지. Draft를 Ready로 올리기 전에 exact-head CI만 보면 되는지.

너의 추천

방향은 맞고 범위도 좁습니다. 이름 있는 상수, cancel 비대기, 64 KiB 경계 테스트는 유지하는 쪽을 추천합니다. 머지 전에는 Draft 해제와 CI 초록 확인이면 충분하고, 필드 “딱 한계” 테스트와 초대형 단일 청크 메모리는 원하면 작은 후속으로 두면 됩니다.

이 댓글은 grok-bot이 작성했습니다

@luvs01
luvs01 marked this pull request as ready for review September 30, 2026 11:52
@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer integration into dev (MAINTAINERS.md, dev-only exception) by @lidge-jun.

  • Head: 09fcf7bc5c35c66b7def4c9bc64b325f7f0f6221. Cross-platform CI 36707600199 (pull_request, attempt 1) passed on this head, including all four Linux test shards; no failed or cancelled checks.
  • Independent review: correctness and security PASS. Manifest data is bounded before JSON parsing and the existing installer-host restriction is preserved; no credential, config-write, logging or installer-execution change.
  • Rebased locally onto dev 592c5cf without conflicts: focused Cursor installer/status tests, file-size ratchet and test-layout guards 62 pass / 0 fail; bun run typecheck, privacy:scan, structure:check pass.
  • scripts/ci/assert-mergeable-review.sh --maintainer-integration OK at this head.

Thanks @luvs01!

@lidge-jun
lidge-jun merged commit b933aa2 into lidge-jun:dev Sep 30, 2026
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants