Skip to content

fix(score): bound native PDF reads before allocation - #865

Draft
seonghobae wants to merge 13 commits into
developfrom
fix/bounded-score-pdf-read-864
Draft

seonghobae wants to merge 13 commits into
developfrom
fix/bounded-score-pdf-read-864

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 16, 2026 •

Copy link
Copy Markdown
Collaborator

Security gap

Closes #864 after protected merge. The native score-read command previously resolved the app-owned score path and then used an unbounded file read, so a stored PDF that grew after attachment could allocate beyond the 25 MiB product ceiling before the JavaScript bridge rejected it.

Exact current identity

  • Protected base: develop@314ddeae7b775a4957594b599358c8255617eb2e.
  • Exact current head: 1f4877413e2eed30b224eaf1b095af3b0b905cb0.
  • Branch: fix/bounded-score-pdf-read-864.
  • Open, Draft, mergeable, and unmerged.

The former head c2c86b8b4f82cbdceabdc52516a33d8d2bd8614a had diverged from protected develop because two repository workflow/backstop commits landed after its merge base 749511c3ad4000090048718f685c6bee6b3d2c25. This was repaired without force-push or destructive rebase: 1f487741… is an ordinary two-parent merge of c2c86b… and protected develop@314ddeae…, with the one overlapping CHANGELOG entry reconciled. Fresh compare reports behind_by=0; the unique score-read delta remains exactly six files.

Regression-first repair

RED head f83a1baebc793658c4d1805be00f11238f09ceac committed the GUI-independent regression before production changes. Current production behavior:

  • opens the already-authorized canonical stored-score path once;
  • reads descriptor metadata and rejects a non-file or a size above MAX_SCORE_PDF_BYTES before heap allocation;
  • allocates exactly the bounded metadata snapshot, reads exactly that amount, then probes one extra byte on the same descriptor so post-metadata growth fails closed without extending the heap buffer;
  • fails closed on truncation after the metadata snapshot;
  • revalidates %PDF- magic on the bytes actually returned to IPC;
  • keeps local path/content out of errors; and
  • routes Tauri read_score_pdf through read_validated_score_pdf instead of std::fs::read.

The regression set covers valid content, empty/short/wrong-magic content, a sparse MAX_SCORE_PDF_BYTES + 1 file, missing paths, a non-file descriptor where supported, growth after the metadata snapshot, and truncation after the metadata snapshot.

Scope / ownership

This PR owns only the native read-time allocation/content boundary. resolve_existing_score_pdf remains the path-authority owner; broader filesystem/workspace authority remains outside this slice. #1239 / PR #1241 is the separate Score Storage write/publication owner and non-force descends from this exact foundation. No dependency/root-lock, network, database, model, review credential, or new IPC-command authority is added by the score-read delta.

Exact-current-head RCA

Exact 1f4877413e2eed30b224eaf1b095af3b0b905cb0 has terminal repository evidence rather than the older queued state:

  • build-baseline 35452436519: SUCCESS;
  • SBOM 35452436616: SUCCESS;
  • Security Scan 35452436569: SUCCESS;
  • SAST Semgrep 35452436556: SUCCESS;
  • ci 35452436567: FAILURE only in ci / build-and-test at Run quickcheck; the separate Rust gate is SUCCESS;
  • CodeQL PR 35452436510: FAILURE in the compatibility-verdict jobs for Python, JavaScript/TypeScript and Actions, while language detection and the later dispatch job succeed.

The ci / build-and-test failure is inherited protected-base Ruff formatting debt in services/analysis-engine/tests/test_supply_chain_policy.py, not this six-file score-read delta. Canonical single-writer repair remains #1176 exact 8fe6b6d99c009527ef0bcba419e6f6debdb23c23, whose only live diff is that formatter repair. Do not duplicate it into #865.

The CodeQL failure is likewise not evidence for changing #865 score-read source: the compatibility jobs fail while enforcing the central current-head dispatch verdict. Central CodeQL/required-workflow settlement remains a .github control-plane responsibility; do not restore duplicate repository scanners, synthesize statuses or create a no-op descendant here.

Merge gate

Keep Draft. #1176 must normally integrate first, then #865 must ordinary/non-force reconcile to the new protected develop and reacquire exact-head evidence. A qualifying independent non-author current-head approval is also absent. No force-push, destructive rebase, copied formatter delta, gate weakening, synthetic status, source-neutral wake commit, blind rerun or predecessor-evidence transfer.

@coderabbitai

coderabbitai Bot commented Aug 16, 2026 •

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent EXECUTION-FIRST owner repair on the existing fix/bounded-score-pdf-read-864 branch only. Fresh identity immediately before this handoff: PR #865 is open/Draft/unmerged at exact head f86e266b2ab2dc5a95e6b4a484e777b29f0feeaf; live protected develop is now 7ad56cf0065d068ec6463d92726de4855a6e201d after #783 merged. The branches diverged from merge base acdbea6344fe1231c39535b575f4de35e4c607c9; #865 is 10 commits ahead of that merge base and 1 protected-base commit behind. Refetch head/base/changed blobs/reviews/checks before every write and adapt rather than overwrite if anything moves. Do not create another PR/branch, force-push, destructively rebase, bypass protection, or weaken any gate.

This is the canonical owner of accepted high-priority issue #864. Non-destructively integrate the live protected develop@7ad56cf... into this branch (ordinary merge/restack that preserves history; no force rewrite), resolving only real overlap. Fresh comparison shows #783 touched security/workflow/npm surfaces plus CHANGELOG.md; #865 owns only CHANGELOG.md, apps/desktop/core/Cargo.toml, apps/desktop/core/src/root.rs, apps/desktop/core/src/score_pdf.rs, apps/desktop/core/tests/score_pdf_read.rs, and apps/desktop/src-tauri/src/main.rs. Preserve both #783's shipped dependency-security/changelog truth and #865's bounded native PDF read entry. Do not import or duplicate #783's dependency authority into #865.

Before resolving the Draft, apply systematic-debugging, receiving-code-review, and verification-before-completion: re-run the realistic Rust regressions for read_validated_score_pdf (valid bytes, empty/short/wrong magic, sparse >25 MiB, same-descriptor growth probe, truncation-after-snapshot, non-file where supported), then full canonical ./scripts/harness/quickcheck.sh, owned coverage/rustdoc, cross-platform build/release, SAST/security-audit/Trivy/SBOM/supply-chain. Treat old nanoid/pdfjs-dist/undici failures as predecessor evidence only; with #783 now in the merge result, inspect any new exact-head failure from its actual checkout/job/log instead of carrying the old classification forward. Also revalidate current central required coverage/OpenCode/Noema/Strix gates against current .github/main; the PR body's old central SHAs/issues are stale.

If the merged exact head is clean, update the PR body to the actual head/base and current evidence, move out of Draft only when the branch itself is review-ready, request/obtain independent non-author review through the existing authorized review path, and resolve only findings proven addressed. Do not merge unless every applicable required exact-head check and qualifying independent approval pass on the unchanged head.

Integrate protected develop@749511c3ad4000090048718f685c6bee6b3d2c25 into the canonical #864 owner branch without rewriting history. Preserve the shipped npm/PDF security baseline and first-playable-range changelog truth while retaining the bounded native score-PDF read implementation and regressions.
@seonghobae
seonghobae marked this pull request as ready for review August 28, 2026 04:34

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 2 potential issues.

Devin Review

Comment thread apps/desktop/core/src/root.rs
Comment thread apps/desktop/core/src/score_pdf.rs

Copy link
Copy Markdown
Collaborator Author

@OpenCode EXECUTION-FIRST continuation on the existing canonical #864 owner branch only. Apply superpowers:using-superpowers, systematic-debugging, test-driven-development for any behavior repair, and verification-before-completion.

Fresh identity immediately before this handoff: PR #865 remains open/Draft/unmerged at exact faf565d92d464de8edb968ee123e6e953eb1b5d6; protected develop remains 749511c3ad4000090048718f685c6bee6b3d2c25. Refetch both and the six changed blobs before any mutation; abort/adapt if either moved. Do not create a competing branch/PR, force-push, destructively rebase, weaken gates, self-approve, or modify foreign repositories.

The current hosted repository evidence is stale/incomplete rather than GREEN: exact-head security-audit and Bandit succeeded, but ci run 32984389563 has remained queued with zero jobs materialized, and build-baseline run 32984469772 was cancelled with zero jobs materialized. A direct failed-jobs rerun of that cancelled run was attempted and GitHub rejected it (403: This workflow run cannot be retried). Treat those states as non-success and do not merely report them.

First verify the current source still implements the #864 native pre-allocation boundary and run the focused Rust regressions for valid bytes, empty/short/wrong magic, sparse >25 MiB, same-descriptor growth probe, truncation-after-snapshot, missing/non-file paths where supported. Then run the repository's canonical full quickcheck/rustfmt/clippy/rustdoc/owned coverage and any locally executable release/security checks. If a BandScope-owned defect appears, reproduce RED and repair it narrowly on this same branch, then rerun focused/full GREEN. If source is clean, use the repository-authorized PR-state path to move this exact unchanged head out of Draft so fresh required workflows can materialize; do not fabricate success from the old queued/cancelled runs. After Ready, refetch exact head/base and let the normal exact-head CI/build/security/OpenCode/Noema/Strix gates run. Do not request an independent review until deterministic exact-head gates have materialized cleanly.

Re-emit the unchanged #864 tree so cancelled/never-materialized current-head workflow evidence is replaced by fresh exact-head runs. No production, test, dependency, workflow, or documentation content changes.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent review

Review exact current head c2c86b8b4f82cbdceabdc52516a33d8d2bd8614a against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25. Repository CI/build/release/security/SAST/SBOM workflows are terminal-success on this unchanged head, coverage evidence is current-head success, and both existing Devin threads are resolved. The required OpenCode job 99473405631 failed only because no authenticated APPROVED/CHANGES_REQUESTED OpenCode review appeared on this SHA during its polling window.

Revalidate the native stored-score trust boundary: pre-allocation 25 MiB ceiling, same-descriptor exact read plus growth probe, truncation failure, PDF magic revalidation, path/payload-safe errors, preserved path-containment ownership, Rust regressions/rustdoc, and current security/supply-chain evidence. Bind any formal verdict only to c2c86b8…; do not inherit predecessor-head or fallback/model-unavailable evidence. Do not change source unless a current-head finding is independently reproduced.

Copy link
Copy Markdown
Collaborator Author

Fresh gate RCA for exact 1f4877413e2eed30b224eaf1b095af3b0b905cb0; no blind rerun or score-source edit is warranted.

  1. ci / build-and-test job 105970978378 is failing on inherited protected-base Ruff formatting, specifically services/analysis-engine/tests/test_supply_chain_policy.py (Would reformat ...). The canonical single-writer repair already exists as repair(ci): format consolidated supply-chain policy test #1176 exact 8fe6b6d99c009527ef0bcba419e6f6debdb23c23; that PR's only live diff is the formatter-equivalent change and its repository-owned ci/build/SBOM/Security/Semgrep are SUCCESS. repair(ci): format consolidated supply-chain policy test #1176 remains Ready but unmerged because CodeQL central settlement and qualifying independent approval are still missing. Do not copy that formatter delta into fix(score): bound native PDF reads before allocation #865.

  2. Strix job 105972205103 is also not a fix(score): bound native PDF reads before allocation #865 source finding. The required-workflow smoke passed and the job used protected central .github@e6334e229581a918e2f22de18733b76fa65d7e71 plus contextual-orchestrator/orchestrator/free. Provider preflight eventually reached chat/completions, then Strix itself hit LLM CONNECTION FAILED / Request timed out. More importantly, the terminal configuration failure is ERROR: Strix evidence binder is missing: .../trusted-workspace/scripts/ci/strix_evidence_binding.py, exit 2. The binder exists in the protected central .github owner, while the gate is resolving it from the BandScope target workspace. This is central required-workflow/evidence-binding ownership, not permission to add the central binder to BandScope or weaken Strix.

Keep #865 Draft and preserve the score-read delta. Its local product repair is not the causal source of either failure; wait for ordinary integration of the formatter prerequisite and central Strix/CodeQL owner settlement, then obtain fresh exact-head evidence rather than manufacturing a wake commit.

Copy link
Copy Markdown
Collaborator Author

Central owner path now exists for the deterministic Strix binder failure: ContextualWisdomLab/.github#2292. #865 should consume that repair only after it is integrated/released through the protected central required-workflow contract; do not vendor strix_evidence_binding.py into BandScope.

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

Labels

bug Something isn't working priority: medium Normal-priority or P2 work status: draft Draft pull request type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security(score): bound and revalidate native PDF reads before allocation

1 participant