test(fuzz): add nesting-depth stack-safety target#78
Conversation
|
Warning Review limit reached
More reviews will be available in 44 minutes and 57 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR introduces ChangesNesting Depth Fuzz Target
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.github/workflows/ci.yml:
- Around line 81-84: Add an explicit permissions block to the fuzz job to scope
GITHUB_TOKEN to least privilege: in the job that contains the "Run fuzz targets"
step (the fuzz job), add a permissions: block with contents: read and actions:
write at the job level (not the step level) so actions/cache@v4 can restore/save
caches without granting broader default scopes.
In `@fuzz/fuzz_targets/fuzz_depth.rs`:
- Around line 25-35: run_boundary_assertions currently asserts only at N and N+1
for each depth constant; add explicit N-1 checks so the just-below cases are
deterministically covered. For both places that call assert_boundary (the block
using QJSON_DEFAULT_MAX_DEPTH/QJSON_MAX_MAX_DEPTH and the later similar block
around lines 49-54), add assertions like assert_boundary(mode,
QJSON_DEFAULT_MAX_DEPTH - 1, QJSON_DEFAULT_MAX_DEPTH, ShapeMode::Array) and
assert_boundary(mode, QJSON_MAX_MAX_DEPTH - 1, QJSON_MAX_MAX_DEPTH,
ShapeMode::Object) (and the Mixed variant for the u32::MAX case where
applicable), using the same mode iteration and preserving ShapeMode variants so
the N-1 boundary is explicitly tested for each mode.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99219920-4da7-4114-8bde-f96b02f38d56
📒 Files selected for processing (4)
.github/workflows/ci.ymlCONTRIBUTING.mdfuzz/Cargo.tomlfuzz/fuzz_targets/fuzz_depth.rs
| - name: Run fuzz targets | ||
| run: | | ||
| cargo +nightly fuzz run fuzz_parse_eager -- -max_total_time=60 | ||
| cargo +nightly fuzz run fuzz_depth -- -max_total_time=60 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the minimum GitHub Actions permissionsblock needed for a workflow that usesactions/checkout@v4, actions/cache@v4, and only runs cargo commands without writing to the repository?
💡 Result:
To run a GitHub Actions workflow that utilizes actions/checkout@v4 and actions/cache@v4 for cargo commands without writing to the repository, you should define the following minimal permissions block: permissions: actions: read contents: read Explanation: - contents: read: Required by actions/checkout@v4 to fetch your repository's code [1][2][3]. - actions: read: Required by actions/cache@v4 to interact with the GitHub Actions cache service [4]. Note that while some historical contexts or specific implementations might suggest write access for caching, the base requirement for simply using the cache service is read access; however, if your workflow is intended to save (write) caches, you must grant actions: write [4]. Since you specified that the workflow only runs cargo commands and does not write to the repository, you should use the block above to follow the principle of least privilege. If your workflow needs to save caches (which is standard practice for performance), use actions: write instead [4]. permissions: actions: write contents: read Setting permissions explicitly is a best practice, as it overrides the default read-write permissions, preventing the GITHUB_TOKEN from having unnecessary access to your repository [5][3].
Citations:
- 1: https://github.com/actions/checkout
- 2: https://github.com/actions/checkout/tree/v4
- 3: https://mickeygousset.com/posts/github-actions-checkout-fails-with-two-possible-error-messages/
- 4: Document that
actions: read/writepermissions are required to use gh-actions-cache in gh actions CI actions/gh-actions-cache#85 - 5: https://secure-pipelines.com/ci-cd-security/github-actions-security-cheat-sheet/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around the fuzz steps
echo "=== ci.yml lines 60-120 ==="
nl -ba .github/workflows/ci.yml | sed -n '60,120p'
# Check for any top-level permissions block
echo
echo "=== permissions blocks in ci.yml (all) ==="
rg -n "^\s*permissions:\s*$|^\s*permissions:\s*$|permissions:" .github/workflows/ci.yml || true
# Also check for other workflow files defining permissions (quick scan)
echo
echo "=== permissions blocks across all workflows ==="
ls -1 .github/workflows || true
rg -n "^\s*permissions:" .github/workflows || trueRepository: api7/lua-qjson
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ci.yml lines 70-95 (context) ==="
sed -n '70,95p' .github/workflows/ci.yml || true
echo
echo "=== permissions blocks in .github/workflows/ci.yml ==="
rg -n "^\s*permissions:" .github/workflows/ci.yml || true
echo
echo "=== permissions blocks across all workflows ==="
rg -n "^\s*permissions:" .github/workflows/*.yml .github/workflows/*.yaml 2>/dev/null || trueRepository: api7/lua-qjson
Length of output: 1021
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show ci.yml header (first 60 lines) ==="
sed -n '1,60p' .github/workflows/ci.yml || true
echo
echo "=== Find actions/cache usage in ci.yml ==="
rg -n "actions/cache@" .github/workflows/ci.yml || true
echo
echo "=== Show cache step sections around matches ==="
rg -n "actions/cache@" .github/workflows/ci.yml | cut -d: -f1 | while read -r line; do
start=$((line-10))
end=$((line+20))
echo "--- around line $line ($start-$end) ---"
sed -n "${start},${end}p" .github/workflows/ci.yml
echo
done
echo
echo "=== Check if ci.yml sets top-level workflow permissions (any other yml keys) ==="
# just in case: look for common patterns
rg -n "permissions:|GITHUB_TOKEN|token:" .github/workflows/ci.yml || trueRepository: api7/lua-qjson
Length of output: 4832
🌐 Web query:
actions/cache@v4 required permissions GITHUB_TOKEN actions: read write contents read
💡 Result:
For actions/cache@v4, the GITHUB_TOKEN needs these job-level permissions (or equivalents): permissions: actions: write contents: read Why: actions/cache (v4) saves/restores caches using GitHub’s cache service APIs; saving requires the actions scope to be write-capable, while restoring only requires reading repository contents. A documented real-world fix for the common “Resource not accessible by integration” error explicitly identifies permissions: { actions: write } as the missing permission for a cache-related action failure, with contents: read also needed to access repository code inputs in the workflow. [1][2] Note: GitHub also supports using workflow/job permissions to control least-privilege access for GITHUB_TOKEN. [2]
Citations:
- 1: Document that
actions: read/writepermissions are required to use gh-actions-cache in gh actions CI actions/gh-actions-cache#85 - 2: https://docs.github.com/actions/concepts/security/github_token
Scope the workflow token explicitly.
.github/workflows/ci.yml has no permissions: block, so the GITHUB_TOKEN for the fuzz job inherits default (broader) scopes. Since this job uses actions/cache@v4 (restores + saves caches), set least-privilege permissions explicitly:
permissions:
contents: read
actions: write🧰 Tools
🪛 zizmor (1.25.2)
[warning] 51-84: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for 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.
In @.github/workflows/ci.yml around lines 81 - 84, Add an explicit permissions
block to the fuzz job to scope GITHUB_TOKEN to least privilege: in the job that
contains the "Run fuzz targets" step (the fuzz job), add a permissions: block
with contents: read and actions: write at the job level (not the step level) so
actions/cache@v4 can restore/save caches without granting broader default
scopes.
| fn run_boundary_assertions() { | ||
| for mode in [QJSON_MODE_EAGER, QJSON_MODE_LAZY] { | ||
| assert_boundary(mode, 0, QJSON_DEFAULT_MAX_DEPTH, ShapeMode::Array); | ||
| assert_boundary( | ||
| mode, | ||
| QJSON_MAX_MAX_DEPTH, | ||
| QJSON_MAX_MAX_DEPTH, | ||
| ShapeMode::Object, | ||
| ); | ||
| assert_boundary(mode, u32::MAX, QJSON_MAX_MAX_DEPTH, ShapeMode::Mixed); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Assert the N-1 boundary cases explicitly.
The deterministic boundary check currently pins only N and N+1. That leaves the acceptance criteria's just-below cases for 1024 and 4096 to chance through fuzz input selection instead of asserting them directly.
Proposed change
fn assert_boundary(
mode: u32,
requested_max_depth: u32,
effective_max_depth: u32,
shape: ShapeMode,
) {
let opts = Options {
mode,
max_depth: requested_max_depth,
};
+ let below_limit_depth = effective_max_depth.saturating_sub(1);
+ let below_limit = nested_json(below_limit_depth, shape, &[0]);
+ assert_parse_ok(&below_limit, &opts);
+ walk_accepted_doc(&below_limit, &opts, shape.path_steps(below_limit_depth));
+
let at_limit = nested_json(effective_max_depth, shape, &[0]);
assert_parse_ok(&at_limit, &opts);
walk_accepted_doc(&at_limit, &opts, shape.path_steps(effective_max_depth));
let over_limit = nested_json(effective_max_depth + 1, shape, &[0]);
assert_nesting_error(&over_limit, &opts);
}Also applies to: 49-54
🤖 Prompt for 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.
In `@fuzz/fuzz_targets/fuzz_depth.rs` around lines 25 - 35,
run_boundary_assertions currently asserts only at N and N+1 for each depth
constant; add explicit N-1 checks so the just-below cases are deterministically
covered. For both places that call assert_boundary (the block using
QJSON_DEFAULT_MAX_DEPTH/QJSON_MAX_MAX_DEPTH and the later similar block around
lines 49-54), add assertions like assert_boundary(mode, QJSON_DEFAULT_MAX_DEPTH
- 1, QJSON_DEFAULT_MAX_DEPTH, ShapeMode::Array) and assert_boundary(mode,
QJSON_MAX_MAX_DEPTH - 1, QJSON_MAX_MAX_DEPTH, ShapeMode::Object) (and the Mixed
variant for the u32::MAX case where applicable), using the same mode iteration
and preserving ShapeMode variants so the N-1 boundary is explicitly tested for
each mode.
…fuzz # Conflicts: # fuzz/Cargo.toml
1 participant
Summary
fuzz_depthtarget for deeply nested arrays/objectsTest Plan
cargo +nightly fuzz run fuzz_depth -- -max_total_time=60cargo +nightly fuzz run fuzz_parse_eager -- -runs=0cargo test --releasecargo clippy --release --all-targets -- -D warningscargo clippy --manifest-path fuzz/Cargo.toml --bins -- -D warningsgit diff --checkCloses #74.
Summary by CodeRabbit
Tests
Chores