fix(fuzz): fuzz the codec that ships — core 0.4.0, all 14 targets, fail loudly (LAB-1136) - #251
Merged
Merged
Conversation
…l loudly (LAB-1136)
The fuzz workspace resolved deps independently of the parent crate
(own [workspace]), so the core 0.4.0 bump never reached it: every fuzz
iteration since has exercised the old 0.2.1 array-of-ints codec, and the
msgpack bin envelope decode path had zero fuzz coverage.
- rust/fuzz/Cargo.toml: cachekit-core =0.2.1 -> =0.4.0 (same features)
- Fix 0.3.0 StorageEnvelope::new(Vec<u8>) -> new(&[u8]) at four call
sites (checksum_collision, format_injection x2, layered_security)
- Update two stale error-message assertions to 0.4.0's reworded
variants ('integrity check failed', 'input exceeds maximum size');
the invariants themselves (corruption rejected, size limits enforced)
still hold
- fuzz-smoke.yml: build all targets once, run every target from
'cargo fuzz list' (no hand-maintained allow-list to rot), and let any
non-zero build/run exit fail the job - the old .fuzz_failures marker
was written but never read, so a broken target could not turn the job
red. Timeout 20 -> 45 min (14 targets x 60s = 14 min fuzzing plus
~20 min toolchain install + ASAN builds on an uncached runner).
- security-deep.yml: the extended-fuzz steps passed --features
compression,checksum to a crate with no [features] table, so cargo
errored before fuzzing and '|| true' swallowed it - the nominal
3x1h job was a no-op. Drop the bogus flags, replace 'timeout 3600
... || true' with '-- -max_total_time=3600' so a clean hour exits 0
and a build error or crash fails the job.
- Delete stale comments citing core 0.1.1 and closed #114
…des (LAB-1136) Panel (bug-hunter, security, craftsman, catchphrase) on the first commit: - fuzz-smoke.yml: resolve 'cargo fuzz list' in a standalone assignment and fail on an empty list — 'for t in $(cmd)' does not propagate a failing $(cmd) under set -e, so a broken list would loop zero times and go green having fuzzed nothing [CRIT] - Commit rust/fuzz/Cargo.lock (was gitignored): with CARGO_HOME=/tmp every CI run freshly resolved all fuzz-workspace transitives to newest release on a persistent self-hosted runner [MAJ, CWE-829] - Pin cargo-fuzz --version 0.13.2 in both workflows (floating install on self-hosted runner; same pattern as kani-verifier) [MAJ] - security-deep.yml: timeout 200 -> 240 min with budget arithmetic (-max_total_time is pure fuzz time; old 'timeout 3600' capped wall time and the job never ran honestly anyway) [MAJ] - security-deep.yml: add if:failure() crash-artifact upload — a crash now fails the fuzz step itself, and without upload the reproducer died with the ephemeral runner [MAJ] - security-deep.yml: delete dead 'Generate coverage report' step (output never uploaded or read; reintroduced the || true swallow) - security-deep.yml: state the real reason for the 3-of-14 subset (time budget) now the stale #114 justification is gone - checksum_collision.rs:85: dead 'Checksum' arm -> 'integrity check failed' (no 0.4.0 message contains capital-C Checksum) - integer_overflow.rs: delete tautological message assert (every 0.4.0 error matched it); the invariant is no-panic on extreme sizes - fuzz/Cargo.toml: document why the exact =0.4.0 pin exists and that it must move with every parent-crate core bump
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. 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: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughFuzzing workflows now use pinned tooling, dynamic or explicit target execution, stricter failure handling, and crash-artifact uploads. The fuzz crate pins ChangesFuzzing alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions as GitHub Actions
participant CargoFuzz as cargo fuzz
participant Targets as fuzz targets
participant Artifacts as crash artifacts
GitHubActions->>CargoFuzz: install pinned cargo-fuzz and build targets
GitHubActions->>CargoFuzz: list or select targets
CargoFuzz->>Targets: run with time budget
Targets-->>GitHubActions: return success or failure
GitHubActions->>Artifacts: upload artifacts on failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
27Bslash6
changed the title
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…LAB-1136)
Second expert-panel pass (crypto/protocol gate). Each item below is a way the
hardened gate could still assert less than it claims.
Workflows:
- Enforce the committed fuzz lockfile. `cargo fuzz build/run` have no --locked
passthrough, so the lockfile added here was advisory to the only commands that
consume it: on any manifest/lock drift cargo would silently re-resolve every
transitive to newest-on-crates.io, on a self-hosted runner with an empty
CARGO_HOME. A `cargo fetch --locked` step now fails loudly on drift. Without
it the CWE-829 fix was a lockfile nobody checked.
- Guard fuzz_targets/ vs [[bin]] parity. `cargo fuzz list` enumerates Cargo.toml
stanzas, NOT files, so a new fuzz_targets/*.rs without its stanza is never
built and never run and the job still goes green -- the same silent-dark mode
the old hardcoded array caused. Nothing in cargo enforces agreement; assert it.
The old comment claimed the derived list made new targets impossible to lose,
which overstated what deriving from Cargo.toml actually buys.
- Build before fuzzing in security-deep.yml, so a compile error surfaces in
~10 min as a build failure rather than three hours in as a fuzz-step error.
Its budget comment already itemised a build step that did not exist.
- Delete the "Check for crashes" step. With `|| true` gone a crash fails its own
fuzz step and kills the job, so this step could only run in the no-crash case
and print a reassuring green -- a named step structurally incapable of
failing, which is precisely the manufactured evidence this PR set out to
remove. It also ran `find artifacts` without creating the dir, swallowing the
error. The fuzz steps' exit codes are the signal.
- Cut PR crash-artifact retention 30d -> 3d. This repo is public and the job runs
on pull_request, so the artifact is a working reproducer for an unfixed defect
in the shipped compression/AES-GCM path, downloadable by anyone for a month.
- Correct the nightly-pin comments: they justified the pin with "cargo-fuzz
0.13.1" while both jobs install 0.13.2 -- stale on arrival, in the PR that
exists to kill stale comments.
byte_storage_checksum_collision.rs:
- Both Ok(_) arms asserted nothing, so the one bug this target exists to find --
a corrupted payload extracting successfully as *different* data -- passed
silently. The real invariant is reject-or-return-original; assert it, and for
the forged all-0xFF checksum assert the genuine checksum really was 0xFF.
- Match error variants instead of Display text. Asserting on err.to_string()
means a #[error(...)] reword in a core release fails this gate on a false
crash; the sibling integer_overflow target already dropped that pattern.
- Widen flip_byte_idx u8 -> u32: payloads run to 4096 bytes, so a u8 index
confined every bit flip this target could ever generate to the first 256.
- Comments said "Blake3"; core 0.4.0 uses xxHash3-64 ("19x faster than Blake3",
byte_storage.rs:6) and checksum is [u8; 8]. Naming a cryptographic hash where
a 64-bit non-cryptographic one lives invites an assumption of collision
resistance this code does not have.
rust/fuzz/Makefile:
- `make quick`/`make deep` still ran every target with `|| true`, so the local
counterpart of this gate swallowed build and run failures exactly as CI did;
only crash artifacts could fail it. Now `|| exit 1` (pipefail is already set
at the top of the file, so it propagates through the tee).
- FUZZ_TARGETS was a hand-maintained 14-target list -- the same drift hazard
removed from the workflow, three directories away. Derived from the [[bin]]
stanzas now, recursively expanded so no cargo/grep runs for `make help`.
Verified: cargo check --locked --all-targets clean on all 14 targets;
cargo fetch --locked succeeds against the committed lock (so the new
enforcement step passes rather than spuriously failing); both workflows parse
and pass actionlint/shellcheck; Makefile derives the same 14 target names.
27Bslash6
dismissed stale reviews from coderabbitai[bot] and kodus-27b[bot]
via
July 29, 2026 12:27
cdb146c
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
1 participant
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.
Closes LAB-1136.
What
The fuzz harness has been a false safety signal for three minor versions of cachekit-core: the fuzz workspace pinned
=0.2.1(own[workspace], so parent bumps never propagate), the smoke job hardcoded 4 of 14 targets, and no failure path existed for a target that fails to build or run. LAB-900 shipped the 0.4.0 msgpackbinenvelope citing a green fuzz job that had fuzzed the old array-of-ints codec.rust/fuzz/Cargo.toml:cachekit-core =0.2.1 → =0.4.0(same feature set, exact pin kept and now documented).Cargo.lockis now committed (was gitignored — withCARGO_HOME=/tmpevery CI run freshly resolved all transitives on the self-hosted runner).StorageEnvelope::new(Vec<u8>) → new(&[u8])at four call sites (the two the ticket named plus two inbyte_storage_format_injection.rs); two stale error-message assertions updated (0.4.0 reworded its error variants — the invariants themselves held); one tautological assertion deleted.fuzz-smoke.yml: builds all targets once, runs every target fromcargo fuzz list(no hand-maintained allow-list to rot), fails on any non-zero build/run exit, guards against a failing/emptyfuzz list(command substitution in afor-word does not propagate underset -e), pins cargo-fuzz 0.13.2, timeout 20 → 45 min.security-deep.ymlextended fuzz: finding — yes, the feature flags were erroring.--features compression,checksumagainst a crate with no[features]table makes cargo error before any fuzzing, and|| trueswallowed it: the nominal 3×1h job was a no-op. Flags dropped,|| trueremoved,timeout 3600→-- -max_total_time=3600(clean hour exits 0, crash/build error fails), crash check retained,if: failure()artifact upload added, dead coverage step deleted, timeout 200 → 240 min with the arithmetic in a comment.Budget arithmetic (fuzz-smoke)
~2 min rustup + ~10 min cargo-fuzz install (no cross-run cache) + ~10 min one-shot ASAN build + 14 × 60 s fuzzing ≈ 36 min worst case →
timeout-minutes: 45.Target list —
cargo fuzz buildresult (local, nightly-2026-04-27, cargo-fuzz 0.13.2)All 14 build and each ran clean under a smoke run (none excluded):
Negative proof (the job can fail)
With
compile_error!("negative proof: job must go red")appended toencryption_roundtrip.rs, both new steps were executed locally under GitHub Actions shell semantics (bash --noprofile --norc -e -o pipefail):Build all fuzz targetsstep → exit 1The old workflow turned the same condition into a
::warning::plus a marker file that nothing ever read.Expert panel
Panel ran (bug-hunter-supreme, security-specialist, code-craftsman, catchphrase-agent) on the first commit; surviving findings applied in the second commit — including a CRIT (the
for t in $(cargo fuzz list)empty-list silent-pass), the uncommitted fuzz lockfile (CWE-829), cargo-fuzz version pinning, the security-deep timeout under-budget, and the missing crash-artifact upload. One out-of-scope finding (Atheris job's|| trueswallow + crash artifacts landing outside the checked corpus dir) is filed separately rather than expanding this PR.Docs
No docs needed:
rust/fuzz/README.mdandrust/fuzz/Makefilealready document/enumerate all 14 targets;DEVELOPMENT.mdfuzzing instructions are generic. The only stale text (core 0.1.1 / closed #114 comments) lived in the workflows and is deleted here. Repo-wide sweep for=0.2.1/core 0.1.1/#114is clean.Summary by CodeRabbit
Tests
Chores