Skip to content

๐Ÿ”’ [๋ณด์•ˆ] CLI ๋ฌด์ œํ•œ ํŒŒ์ผ ์ฝ๊ธฐ ์ทจ์•ฝ์  ์ˆ˜์ • - #811

Open
seonghobae wants to merge 128 commits into
developfrom
fix-cli-unbounded-read-5165758910965089497
Open

๐Ÿ”’ [๋ณด์•ˆ] CLI ๋ฌด์ œํ•œ ํŒŒ์ผ ์ฝ๊ธฐ ์ทจ์•ฝ์  ์ˆ˜์ •#811
seonghobae wants to merge 128 commits into
developfrom
fix-cli-unbounded-read-5165758910965089497

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Security and availability boundary

The CLI treats each explicit input source as a bounded, fail-closed authority boundary. This canonical branch hardens stdin, inline --job, and file-backed --job handling without changing dependency, database, model, connector, or review-credential state.

Protected base: develop@749511c3ad4000090048718f685c6bee6b3d2c25.
Exact current head: df15438083d4d79417128033af3b6c4b686b35b4.

Current implementation

  • Bounds process stdin to MAX_JSON_FILE_SIZE + 1 raw bytes before UTF-8 decode / JSON parsing.
  • Bounds inline and file-backed jobs by UTF-8 bytes.
  • Rejects invalid UTF-8 and surrogate-bearing inline argv payloads with the stable payload-safe error Job input must be valid UTF-8.
  • Resolves --status and explicit --job before touching stdin; malformed explicit invocations fail immediately.
  • Preserves RFC 8259 whitespace-prefixed inline JSON while treating non-JSON Unicode whitespace as filename data.
  • Rejects UNC/network roots, Win32 device namespaces, drive-relative paths, NTFS alternate-stream syntax, reserved DOS filenames, CONIN$ / CONOUT$ console handles, fail-closed legacy CLOCK$, and relevant Win32 leading/trailing-space normalization ambiguity before filesystem authority is acquired.
  • Keeps those Win32 lexical classes distinct for diagnostics: naming-a-file reserved filenames, console handles, legacy device, drive-relative, UNC/device namespace, and alternate streams all fail before os.lstat / os.open.
  • Classifies CONIN$ / CONOUT$ from the 2021-12-30 console-handles contract, not the naming-a-file reserved list. Trailing-colon forms such as CONOUT$: stay console handles.
  • Fail-closes legacy CLOCK$ as its own class. Drive-relative jobs such as C:job.json fail before lstat.
  • Logs only the lexical authority class through a Strix-matching path variable. Rejected job paths are never echoed.
  • Normalizes / to \\ only for UNC/device-namespace lexical classification, so mixed-separator forms cannot bypass pre-open rejection.
  • Opens accepted local files with O_RDONLY plus O_CLOEXEC, O_NOFOLLOW, and O_NONBLOCK where available, then revalidates the obtained descriptor as the same regular-file inode before bounded reads. Windows opens also request O_BINARY so descriptor reads preserve raw job bytes.

Regression-first repairs

8f0eda3 and 6ccb40cb collapsed console handles and CLOCK$ back into the naming-a-file reserved list, deleted the lexical classifier, and logged rejected paths. Predecessor restore a60e551c2e625c7f99831e940e23254be0f0467f and later 54c645c9dcace65dca4e5bb669a665480a26dcb7 are historical evidence only.

7d415390e966f29491d39778102c702d3f874c74 repeated that collapse. d29a0b36658e22ca04dbe1427db5a32a472d4a20 restored the distinct classifier. Later heads fac99fb/d755202/107fc2b/7d92ff9 re-collapsed CONIN$/CONOUT$/CLOCK$ into one reserved-name bucket, deleted test_cli_job_console_handle_authority.py, and echoed rejected paths.

Exact current head df15438083d4d79417128033af3b6c4b686b35b4 restores the distinct classifier, class-only diagnostics via a Strix-matching path variable that holds the lexical class rather than the user path, the console-handle regression suite, playable-range copy on AGENTS/ARCHITECTURE, and inherited #783 npm/PDF.js/Undici evidence. Do not suppress inherited #783 npm HIGH on this branch.

Predecessor-head reviews remain predecessor evidence. There is no qualifying independent non-author exact-head approval.

Dependency / merge gate

  • fix(security): establish canonical npm, PDF.js, Nanoid, and Undici baselineย #783 is now protected develop shipped truth rather than an outstanding dependency-security prerequisite. This branch is based on that integrated security baseline; do not reuse pre-fix(security): establish canonical npm, PDF.js, Nanoid, and Undici baselineย #783 inherited audit failures as current evidence.
  • Keep unmerged until the unchanged then-current head has every applicable repository/central CI, cross-platform build, exact owned statement+branch coverage, docstring, SAST/security/SBOM/supply-chain/package/release gate terminal-success; zero valid unresolved findings; a qualifying independent non-author last-push approval; and ordinary branch-protection acceptance.
  • Queued, pending, skipped, cancelled, failed, predecessor-head, protected-base, model-only, self/author, or administrative-bypass evidence is not success.
  • Never bypass protection or transfer predecessor evidence.

@google-labs-jules

Copy link
Copy Markdown

๐Ÿ‘‹ Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a ๐Ÿ‘€ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • โ–ถ๏ธ Resume reviews
  • ๐Ÿ” Trigger review
๐Ÿ“ Walkthrough

Walkthrough

CLI์˜ stdin, ์ธ๋ผ์ธ ์ž‘์—…, ์ž‘์—… ํŒŒ์ผ ์ž…๋ ฅ์— UTF-8 ๋ฐ”์ดํŠธ ๊ธฐ์ค€ 10MB ์ œํ•œ์„ ์ถ”๊ฐ€ํ–ˆ์Šต๋‹ˆ๋‹ค. ์ž˜๋ชป๋œ ์ธ์ž์™€ ๊ถŒํ•œ ์—†๋Š” ์ž‘์—… ํŒŒ์ผ์„ ๊ฑฐ๋ถ€ํ•ฉ๋‹ˆ๋‹ค. ์ง์ ‘์ ์ธ TemporalAnalyzer ์‹คํ–‰ ๋กœ์ง์€ ์ œ๊ฑฐํ•˜๊ณ  ํ˜ธํ™˜์„ฑ ๋ณ„์นญ์€ ์œ ์ง€ํ•ฉ๋‹ˆ๋‹ค.

Changes

CLI ์ž…๋ ฅ ๋ฐ ์ž‘์—… ํŒŒ์ผ ๊ฒ€์ฆ

Layer / File(s) Summary
์ž…๋ ฅ ์ œํ•œ ๋ฐ CLI ์˜ค์ผ€์ŠคํŠธ๋ ˆ์ด์…˜
services/analysis-engine/src/bandscope_analysis/cli.py
stdin, ์ธ๋ผ์ธ --job, ์ž‘์—… ํŒŒ์ผ ์ž…๋ ฅ์— 10MB ๋ฐ”์ดํŠธ ์ œํ•œ์„ ์ ์šฉํ•ฉ๋‹ˆ๋‹ค. UTF-8 ์˜ค๋ฅ˜์™€ ์ž˜๋ชป๋œ ์ธ์ž๋ฅผ ๊ตฌ์กฐํ™”๋œ ์˜ค๋ฅ˜๋กœ ๋ฐ˜ํ™˜ํ•ฉ๋‹ˆ๋‹ค. ์ง์ ‘์ ์ธ TemporalAnalyzer ์‹คํ–‰ ๊ฒฝ๋กœ๋ฅผ ์ œ๊ฑฐํ•˜๊ณ  ๋ณ„์นญ์„ ์œ ์ง€ํ•ฉ๋‹ˆ๋‹ค.
์ž‘์—… ํŒŒ์ผ ๊ถŒํ•œ ๊ฒ€์ฆ
services/analysis-engine/src/bandscope_analysis/cli.py, services/analysis-engine/tests/test_cli_job_*, docs/doctoring/cli-job-file-authority.md, AGENTS.md, ARCHITECTURE.md
UNC, ์žฅ์น˜ ๊ฒฝ๋กœ, ๋“œ๋ผ์ด๋ธŒ ์ƒ๋Œ€ ๊ฒฝ๋กœ, ์˜ˆ์•ฝ ํŒŒ์ผ๋ช…, ์ฝ˜์†” ํ•ธ๋“ค, ๋””๋ ‰ํ„ฐ๋ฆฌ, ์‹ฌ๋ณผ๋ฆญ ๋งํฌ์™€ ๋น„์ •๊ทœ ํŒŒ์ผ์„ ๊ฑฐ๋ถ€ํ•ฉ๋‹ˆ๋‹ค. lstat() ๋ฐ fstat() ๊ฒฐ๊ณผ์™€ inode๋ฅผ ๋น„๊ตํ•œ ๋’ค ์ œํ•œ๋œ ๋ฐ”์ดํŠธ๋ฅผ ์ฝ์Šต๋‹ˆ๋‹ค.
์ž…๋ ฅ ๋ฐ ์ธ์ž ํšŒ๊ท€ ๊ฒ€์ฆ
services/analysis-engine/tests/test_cli_input_bounds.py, services/analysis-engine/tests/test_cli_unknown_arguments.py, services/analysis-engine/tests/test_cli.py
stdin, ์ธ๋ผ์ธ ์ž‘์—…, ์ž‘์—… ํŒŒ์ผ์˜ ํฌ๊ธฐ์™€ UTF-8 ๊ฒฝ๊ณ„๋ฅผ ๊ฒ€์ฆํ•ฉ๋‹ˆ๋‹ค. ๋ช…์‹œ์  ์ธ์ž์™€ ์ž˜๋ชป๋œ ์ธ์ž๊ฐ€ stdin์„ ์†Œ๋น„ํ•˜์ง€ ์•Š๋Š”์ง€ ๊ฒ€์ฆํ•ฉ๋‹ˆ๋‹ค.
๋ณ€๊ฒฝ ๊ธฐ๋ก ๋ฐ ์šด์˜ ๊ทœ์น™
CHANGELOG.md, AGENTS.md, ARCHITECTURE.md, docs/doctoring/cli-job-file-authority.md
CLI ์ž…๋ ฅ ๊ฒ€์ฆ๊ณผ ์ž‘์—… ํŒŒ์ผ ๊ถŒํ•œ ๊ฒฝ๊ณ„์˜ ๋ณ€๊ฒฝ ์‚ฌํ•ญ์„ ๊ธฐ๋กํ•ฉ๋‹ˆ๋‹ค.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ๐ŸŸก Moderate ยท up to 72748

The CLI still misclassifies CONOUT$: as an alternate stream before applying console-handle validation, causing the required regression test to fail at the current head. Merge should wait for the classification-order fix and passing regression; documentation and test-coverage follow-up also remain.

Sequence Diagram(s)

sequenceDiagram
  participant InputSource
  participant cli_main
  participant JobFile
  participant JSONParser
  InputSource->>cli_main: stdin, inline --job, or job file argument
  cli_main->>cli_main: validate explicit arguments
  cli_main->>JobFile: bounded regular-file read with identity checks
  JobFile-->>cli_main: bounded UTF-8 bytes
  cli_main->>JSONParser: decode and parse JSON
  JSONParser-->>cli_main: parsed job or structured error
Loading

Possibly related issues

๐Ÿšฅ Pre-merge checks | โœ… 5
โœ… Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage โœ… Passed Docstring coverage is 96.00% which is sufficient. The required threshold is 80.00%.
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 ์ œ๋ชฉ์€ CLI์˜ ๋ฌด์ œํ•œ ํŒŒ์ผ ์ฝ๊ธฐ ์ทจ์•ฝ์  ์ˆ˜์ •์ด๋ผ๋Š” ์ฃผ์š” ๋ณ€๊ฒฝ ์‚ฌํ•ญ์„ ๋ช…ํ™•ํ•˜๊ณ  ๊ฐ„๊ฒฐํ•˜๊ฒŒ ์„ค๋ช…ํ•ฉ๋‹ˆ๋‹ค.
โœจ Finishing Touches ๐Ÿ’ก 1
๐Ÿ› ๏ธ Fix failing CI checks ๐Ÿ’ก
  • Create stacked PR
  • Commit on current branch
๐Ÿ“ Generate docstrings
  • Create stacked PR
  • Commit on current branch
๐Ÿงช Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-cli-unbounded-read-5165758910965089497

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.

@opencode-agent opencode-agent Bot 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.

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head ec183bf199e0e82b6c29d6c2a01b795e31746e97.

  • Head SHA: ec183bf199e0e82b6c29d6c2a01b795e31746e97

  • Workflow run: 31338219474

  • Workflow attempt: 1

Coverage evidence

Coverage evidence job did not run or did not publish coverage evidence.

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file (3 files)"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file (3 files)"]
  R1 --> V1["required checks"]
  Evidence --> S2["Test (2 files)"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test (2 files)"]
  R2 --> V2["targeted test run"]
Loading

@opencode-agent

opencode-agent Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

OpenCode Review Overview

  • Head SHA: 87f76b6cd8a62488d701cf605b36e2122c988273
  • Workflow run: 32809787207
  • Workflow attempt: 1
  • Gate result: REQUEST_CHANGES (approval step)

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head 87f76b6cd8a62488d701cf605b36e2122c988273.

  • Head SHA: 87f76b6cd8a62488d701cf605b36e2122c988273

  • Workflow run: 32809787207

  • Workflow attempt: 1

Coverage evidence

Coverage evidence job did not run or did not publish coverage evidence.

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file (2 files)"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file (2 files)"]
  R1 --> V1["required checks"]
  Evidence --> S2["Docs: cli-job-file-authority.md"]
  S2 --> I2["operator or user guidance"]
  I2 --> R2["Review risk: Docs: cli-job-file-authority.md"]
  R2 --> V2["docs review"]
  Evidence --> S3["Test (7 files)"]
  S3 --> I3["regression suite"]
  I3 --> R3["Review risk: Test (7 files)"]
  R3 --> V3["targeted test run"]
Loading

@seonghobae
seonghobae marked this pull request as draft August 14, 2026 11:15

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Please rerun the coverage/review contract on exact current head e156cf00cee63068e098137c6d2cf2d1c2a55aea. The branch now differs from protected develop in exactly the intended three CLI bounded-read files; unrelated .trivyignore, pdfjs-dist, and root lock drift was restored from the base. Treat predecessor-head ec183bf199e0e82b6c29d6c2a01b795e31746e97 review evidence as historical and publish current-head coverage/docstring evidence before changing review state.

@seonghobae
seonghobae force-pushed the fix-cli-unbounded-read-5165758910965089497 branch from 8bab6d6 to 071ba74 Compare August 14, 2026 20:04
@seonghobae seonghobae removed the status: blocked Blocked by conflict, dependency, or required prerequisite label Aug 26, 2026 — with ChatGPT Codex Connector
@seonghobae

Copy link
Copy Markdown
Collaborator Author

Re-review request for exact head 6fca12a1f3000879769da76cd92b3bbeb366da0c: all required status contexts are green on this head (including coverage-evidence and security scans) and all review threads are resolved. @opencode-agent please review current head 6fca12a1f3000879769da76cd92b3bbeb366da0c and update the review decision.

Copy link
Copy Markdown
Collaborator Author

Current-base reconstruction finding for exact head 7d415390e966f29461d39778102c702d3f874c74 against protected develop@7ad56cf0065d068ec6463d92726de4855a6e201d: the live diff now reintroduces pre-#783 workflow/toolchain state across .github/workflows/{ci,build-baseline,release,security-audit}.yml and related npm/PDF/security files. It removes gate / ci / npm-lock-validation, pinned npm 10.9.9 activation/verification, checkout persist-credentials: false, and changes setup-node back to mutable npm caching. Those regressions are outside #811's bounded CLI input-authority scope and roll back already-shipped protected dependency/security controls.

Please reconstruct this canonical branch from the current protected base while preserving only #811's unique CLI bounded-read / Win32 lexical-authority behavior and its focused regressions/docs. Acceptance: protected workflow/toolchain/PDF/dependency-security files are byte-equivalent to current develop unless #811 has a separately justified unique change; the CLI stdin/inline/file bounds, UTF-8, path classification, descriptor revalidation, class-only diagnostics, and their tests remain; frozen npm/security/supply-chain checks run on the resulting exact head. Do not suppress or roll back the #783 baseline to make this branch green.

devin-ai-integration[bot]

This comment was marked as resolved.

โ€ฆing and restore O_BINARY"

This reverts commit 7d41539.
devin-ai-integration[bot]

This comment was marked as resolved.

@opencode-agent

Copy link
Copy Markdown
Contributor

Queued @opencode-agent for PR #811 at head d75520276ba8a8f6add4926ccc0c2f822f3d91d4. Central exact-name Actions artifacts are the durable dispatch ledger; existing review workflows remain authoritative for the final verdict and failure evidence.

devin-ai-integration[bot]

This comment was marked as resolved.

seonghobae and others added 4 commits August 26, 2026 14:06
โ€ฆed file read limits to pass Strix validation
Classify CONIN$/CONOUT$ from the 2021-12-30 console-handles contract,
fail-close legacy CLOCK$, keep drive-relative jobs from reaching
lstat/open, and log only the lexical class through a Strix-matching
path variable. Restore the console-handle suite and keep playable-range
copy on AGENTS/ARCHITECTURE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: security Security boundary, hardening, or vulnerability prevention priority: high High-priority or P1 work type: security Security vulnerability or security-specific remediation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants