fix(score): bound native PDF reads before allocation - #865
Draft
seonghobae wants to merge 10 commits into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
This was referenced Aug 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security gap
Closes #864 after protected merge. Protected
developresolves the app-owned score path and then uses unboundedstd::fs::read, so a stored PDF that grows after attachment can allocate beyond the 25 MiB product ceiling before the JavaScript bridge can reject it.Protected base:
develop@acdbea6344fe1231c39535b575f4de35e4c607c9.Exact current head:
f86e266b2ab2dc5a95e6b4a484e777b29f0feeaf.Regression-first repair
RED head
f83a1baebc793658c4d1805be00f11238f09ceaccommitted GUI-independent integration tests before production changes. They require the missingread_validated_score_pdfboundary to return valid bytes unchanged, reject invalid/truncated/oversized inputs with payload-safe errors, and enforce the 25 MiB ceiling. Historical RED workflow state is not used as current success.The production repair now:
MAX_SCORE_PDF_BYTESbefore heap allocation;%PDF-magic on the bytes actually returned to IPC;read_score_pdfthrough the bounded core helper instead ofstd::fs::read.A deterministic unit regression supplies a stream whose bytes exceed the metadata-length snapshot, proving the one-byte growth probe rejects the race; a truncation-after-snapshot regression proves a short read fails closed. The integration suite also covers valid content, empty/short/wrong-magic content, a sparse
MAX_SCORE_PDF_BYTES + 1file, missing paths, and a non-file descriptor on Unix.Scope and ownership
This PR owns only the native read-time allocation/content boundary.
resolve_existing_score_pdfremains the path-authority owner; #858 owns broader filesystem path-authority hardening. #750 remains the separate JavaScript bridge-response validation slice. #783 remains the canonical JavaScript dependency-security baseline. No JavaScript dependency/root-lock, network, database, model, review credential, or new IPC command authority is added.The desktop-core crate keeps its existing package/dependency/lint contract; only its library entry path changes to a thin root module so the new bounded reader can be isolated without rewriting the historical core implementation. The historical public crate-root API is re-exported unchanged.
CHANGELOG.mdrecords the buyer-visible resource-boundary fix. No temporary/self-modifying workflow is used.Exact-current-head verification
Repository-owned exact-head workflow state for
f86e266b2ab2dc5a95e6b4a484e777b29f0feeaf:ci,release,build-baseline,sbom,SAST Semgrep,bandit, andsecret-scan-gate;security-auditand aggregateSecurity Scan.The failures were inspected at exact job/log level before classification:
security-auditrun31945502301, job95160728823, fails at the npm audit stage on the protected-base JavaScript set:nanoid,pdfjs-dist, andundici; Python/Rust audit stages are skipped only because npm exits first and skipped evidence is not success.Security Scanrun31945502360has successful Dependency Review, OSV base-vs-head comparison, and Scorecard; its only failed job istrivy-fsjob95160730223, which reports the protected-basepdfjs-distCVE-2026-16633finding and directs remediation to the shared base.This branch changes no JavaScript dependency/root lock or vulnerability-suppression policy. Those failures remain inherited #783 evidence and are not copied or suppressed here.
Current inline review-thread inspection returns zero threads, but there is also no qualifying independent non-author formal approval for this exact head.
Merge gate
Keep Draft until the unchanged exact head has all applicable repository and central gates terminal-success, including realistic Rust tests, cross-platform build, 100% owned statement+branch coverage where exposed, rustdoc/docstrings, SAST/security/SBOM/supply-chain/release evidence; #783 has reached protected
developand the security gates are regenerated cleanly; #861/central branch-coverage evidence is inherited and revalidated; current actionable threads remain zero; a qualifying independent non-author last-push approval exists; and branch protection permits merge without bypass.Protected central
.github/mainis nowb71a02a310e77f70c1e59f4719f6857cb33ca886and includes merged.github#1124, so the former relative-include materialization prerequisite is no longer pending. The remaining read-only central trusted-uv version-output failure is owned by open.github#1117at428d6eb13e1355d4304b207b3721c376fe768c66; no BandScope-local coverage workaround is valid for that pre-test control-plane failure.Queued, skipped, failed, predecessor-head, inherited protected-base, self/author, or administrative-bypass evidence is not success.