fix(security): harden bounded JSON input handling - #528
Conversation
|
👋 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 New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA shared descriptor-safe JSON reader now enforces 32 MiB input and 128-level nesting limits. Governed scripts use the reader. Tests cover filesystem safety, parsing failures, limits, and delegation. Documentation records the security contract. ChangesBounded JSON hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build_figma_evidence_sync.py`:
- Around line 50-56: Update all seven _read_json implementations in
scripts/build_figma_evidence_sync.py (anchor lines 50-56),
scripts/build_benchmark_report.py (17-23), scripts/build_buyer_packet.py
(51-57), scripts/build_commercial_release.py (44-50),
scripts/build_procurement_due_diligence.py (42-48),
scripts/build_release_evidence_index.py (30-36), and scripts/sales_readiness.py
(244-250) to enforce a maximum JSON nesting depth before or during json.loads.
Use a shared depth-aware parser or validator that correctly ignores brackets
inside strings and escaped characters, while preserving the existing 32 MiB read
limits and normal JSON parsing for valid inputs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fc1fbd7c-e3fc-45e1-8ffd-122154a56bcb
📒 Files selected for processing (7)
scripts/build_benchmark_report.pyscripts/build_buyer_packet.pyscripts/build_commercial_release.pyscripts/build_figma_evidence_sync.pyscripts/build_procurement_due_diligence.pyscripts/build_release_evidence_index.pyscripts/sales_readiness.py
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
@jules Rebuild this PR on the current main before merge and address the exact-head security review. The present head is conflicted and repeats a previously rejected pattern from #388.
Required changes:
- Replace the seven duplicated
_read_jsonimplementations with one documented shared loader underscripts/(use a descriptive two-token module/function name). Preserve a 32 MiB byte cap and a conservative nesting cap (the package already uses 128) while correctly ignoring delimiters inside JSON strings and escaped characters. - Harden file opening, not only parsing: open the leaf with
O_CLOEXEC,O_NONBLOCK, andO_NOFOLLOWwhere supported; validate the opened descriptor withfstatas a regular file; read at most the limit plus one byte through that same descriptor; reject symlinks/FIFOs/directories and oversized or invalid UTF-8 input beforejson.loads. Avoid a pathstat()/open()TOCTOU gap. Preserve deterministic, content-free errors. - Use the shared loader in every affected script. Keep each script's existing object-shape contract, including explicitly validating
sales_readiness.pyinput as a JSON object rather than returning arbitrary JSON. - Add complete public docstrings and deterministic tests covering: exact-limit acceptance, limit+1 rejection, depth-boundary acceptance, over-depth rejection, brackets/escaped quotes inside strings, invalid UTF-8, malformed JSON, non-object JSON at each wrapper contract, symlink rejection (when supported), FIFO/non-regular rejection without blocking, and descriptor/path replacement behavior. Do not weaken the repository's 100% statement/branch coverage gate.
- Add an authoritative changelog fragment and regenerate
CHANGELOG.md. Update the PR description to avoid the unsupportedCRITICALseverity claim: these are local/CI artifact readers, so describe the bounded-input hardening and threat assumptions conservatively. - Rebase or otherwise rebuild the branch on the exact current
main; do not carry temporary workflows or unrelated changes. Rerun exact-head CI, Security Scan, SAST, and CodeRabbit review.
Resolve the CodeRabbit depth thread only after the shared implementation and tests cover it.
|
@jules Please address the unresolved CodeRabbit CWE-674 finding on the exact current head. Keep the existing 32 MiB bounded read, but consolidate all eight repository automation |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
@jules The current head is not acceptable and must not be merged. Remove .github/workflows/one-time-bounded-json-hardening.yml and commit the actual implementation directly. The generated helper in that workflow still uses path.open("rb"), so it retains the exact symlink/FIFO/path-replacement problems previously rejected in #388 and explicitly called out in my review. It also lacks the required symlink, FIFO/non-regular, and descriptor/path-replacement tests.
Implement the descriptor-safe shared loader itself on this branch: os.open with O_CLOEXEC | O_NONBLOCK | O_NOFOLLOW where available, fstat regular-file validation, lstat/descriptor identity validation, bounded reads from the same descriptor, then UTF-8/depth/JSON/object validation. Commit the real helper, delegation edits, tests, changelog/docs, and updated conservative PR metadata directly. Do not add or retain any temporary workflow. Resolve the existing CodeRabbit thread only after the real exact-head code and tests address it.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ipts-17708564860587448239
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build_figma_evidence_sync.py`:
- Around line 411-413: Update main() to compute out_dir using the same
repository-root base resolution as build_figma_evidence_sync(), then use that
shared directory for both the reported out path and manifest path. Add a
regression test covering a relative --out with --repo-root different from the
current working directory, asserting the returned manifest path exists.
In `@tests/test_bounded_json_scripts.py`:
- Around line 36-50: Add an escaped-quote case to
test_exact_size_depth_strings_escapes_and_unicode, including structural
delimiters after the escaped quote within a valid JSON string. Keep the existing
exact-size and depth assertions, and ensure the expected object reflects the
added string content so read_json_object verifies delimiters are ignored while
inside the escaped-quote string.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49878f93-4901-48e5-b62d-b2b95b2aa789
📒 Files selected for processing (13)
CHANGELOG.mddocs/bounded_json_input_security.mddocs/changelog.d/bounded-json-input-hardening.mdscripts/_bounded_json.pyscripts/build_benchmark_report.pyscripts/build_buyer_packet.pyscripts/build_commercial_release.pyscripts/build_figma_evidence_sync.pyscripts/build_pr_queue_governance.pyscripts/build_procurement_due_diligence.pyscripts/build_release_evidence_index.pyscripts/sales_readiness.pytests/test_bounded_json_scripts.py
|
@coderabbitai review Please review the exact current head. The previously reported repository-root output-path bug and escaped-quote depth-test gap are addressed, temporary one-time workflows have been removed, and the new regression test is documented. Report only findings still applicable to the current head. |
|
✅ Action performedReview finished.
|
요청하신 대로 기존 |
2 participants
Summary
Consolidates repository automation JSON reads behind a shared descriptor-safe loader. The loader bounds accepted input to 32 MiB, rejects structural nesting beyond 128 levels before recursive decoding, requires strict UTF-8 and an object root, and uses descriptor identity checks to reject non-regular inputs and detectable leaf-path replacement.
All eight affected automation scripts delegate to the shared contract. Deterministic tests cover byte and depth boundaries, delimiters and escapes inside strings, malformed and invalid-UTF-8 input, decoder recursion, non-object roots, symlink/FIFO/directory rejection where supported, descriptor/path replacement, and delegation. Public docstrings, authoritative documentation, APA-style references, an authoritative changelog fragment, and the generated
CHANGELOG.mdentry are included.Threat boundary
This is bounded-input hardening for local and CI artifact readers. It reduces uncontrolled resource-consumption and recursive-decoder availability risk. It does not make arbitrary JSON semantically trustworthy, replace operating-system permissions, or authorize processing from a hostile writable parent directory.
Validation
References
MITRE. (2026, April 30). CWE-400: Uncontrolled resource consumption (CWE Version 4.20).
MITRE. (2026, April 30). CWE-674: Uncontrolled recursion (CWE Version 4.20).
Python Software Foundation. (2026). json—JSON encoder and decoder (Python 3.12.13 documentation).
Python Software Foundation. (2026). os—Miscellaneous operating system interfaces (Python 3.12.13 documentation).
Summary by CodeRabbit