Close the remaining RNG guard bypasses and self-test the guard - #108
Conversation
|
Warning Review limit reached
Next review available in: 46 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe RNG hygiene guard now scans more script types, detects constructed device paths, and rejects dynamic ChangesRNG hygiene validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI workflow
participant test-rng-hygiene.sh
participant Git temporary index
participant check-rng-hygiene.sh
CI workflow->>test-rng-hygiene.sh: run self-test
test-rng-hygiene.sh->>Git temporary index: stage probe file
test-rng-hygiene.sh->>check-rng-hygiene.sh: scan staged probe
check-rng-hygiene.sh-->>test-rng-hygiene.sh: report finding or success
test-rng-hygiene.sh-->>CI workflow: return test status
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
CI failed the self-test, and the cause was not what I first assumed. Both fixes are pushed. What actually broke. That is the exact trap already documented in the guard, five lines above the code I was editing:
The harness flaw that hid it. My first diagnosis was that the temp index was empty. Wrong, but chasing it exposed something worse. The test only checked the guard's exit code, so any failure counted as a successful detection. If the guard had aborted for an unrelated reason, all eleven reject cases would have reported Two changes:
State now: guard clean on the tree, self-test 13/13, Worth recording that the self-test earned its place on its first run: it caught a defect in itself before it could certify a weakened guard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/check-rng-hygiene.sh (1)
181-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
normalise()function has drifted fromscan()'s inline normalization.
normalise()(Line 186-188) strips single-quote concatenation ('/dev/' + 'urandom') via\x27, but shellcheck flags it as never invoked (SC2329).scan()reimplements the double-quote case inline (Line 195-196) but never applies the single-quote pattern, so the two implementations are now out of sync.This does not currently open a bypass: any
/dev/'or/dev/"boundary is already caught directly by thedyndev_badrule (Line 220) matching the raw, unnormalized line. But the duplication is a maintenance hazard — a future contributor could reasonably assumenormalise()is wired in and rely on it, or extend only one of the two implementations, silently reintroducing drift between what is documented and what actually executes.Either wire the single-quote handling into
scan()and drop the standalone function, or removenormalise()if it is genuinely obsolete.♻️ Proposed fix: fold single-quote handling into `scan()` and drop the dead function
-normalise() { - sed -E 's/"[ \t]*\+\+?[ \t]*"//g; s/"[ \t]*"//g; s/\x27[ \t]*\+\+?[ \t]*\x27//g' -} - scan() { # $1 = ERE - printf '%s\n' "$CODE" | awk -v pat="$1" '{ + printf '%s\n' "$CODE" | awk -v pat="$1" -v sq="'" '{ raw = $0 line = $0; sub(/^[^:]*:[0-9]+:/, "", line) norm = line gsub(/"[ \t]*\+\+?[ \t]*"/, "", norm) gsub(/"[ \t]*"/, "", norm) + gsub(sq "[ \t]*\\+\\+?[ \t]*" sq, "", norm) + gsub(sq "[ \t]*" sq, "", norm) if (line ~ pat || norm ~ pat) print raw }' }🤖 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 `@scripts/check-rng-hygiene.sh` around lines 181 - 199, Remove the unused normalise() function and consolidate its single-quote concatenation handling into scan() alongside the existing double-quote normalization. Ensure scan() checks both normalized forms against the pattern while preserving raw-line reporting.Source: Linters/SAST tools
🤖 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/test-rng-hygiene.sh`:
- Line 15: Guard the directory change in the test script by making the cd
command fail immediately when it cannot enter the repository root. Ensure
subsequent $GUARD and probe-file checks never run from an unintended working
directory.
- Around line 108-114: Update the scripts passed to probe_hash_string.sh and
probe_marker_string.sh so each combines its string/marker statement with the
/dev/urandom assignment on one physical line, matching the split_line() one-line
bypass scenario. Keep the existing probe expectations and descriptions, while
ensuring the tests exercise quote-aware comment and marker handling rather than
independently flagging a second-line hazard.
- Around line 27-53: Update run_probe to check whether the target path in name
already exists before printf writes the probe content; if it exists, report the
harness failure, increment fails, and return without modifying or deleting that
file. Preserve the existing temporary index setup and cleanup behavior for newly
created probe files.
---
Nitpick comments:
In `@scripts/check-rng-hygiene.sh`:
- Around line 181-199: Remove the unused normalise() function and consolidate
its single-quote concatenation handling into scan() alongside the existing
double-quote normalization. Ensure scan() checks both normalized forms against
the pattern while preserving raw-line reporting.
🪄 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 Plus
Run ID: e73cc362-f1b8-4556-a52f-024702676646
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/check-rng-hygiene.shscripts/test-rng-hygiene.sh
|
All three fixed. The third was the one that mattered. The probes did not test what they claimed. Proved it rather than reasoning about it. Temporarily reintroduced the naive splitter the guard used to have:
So the corrected probes fail against the defect and pass against the fix, which is the only thing that makes them regression tests. Both are now single-line, with a comment saying why the line break matters. This is the second instance in this file of a test that would have credited the right answer for the wrong reason, after the exit-code-only check fixed earlier in this PR. Worth noting the pattern rather than just fixing the instance. Guarded the Corrected the working-tree claim, which was mine to get wrong. The PR description said the self-test runs "without modifying the working tree". Only the index is untouched, via Self-test 13/13 on the fixed guard, and it still reports failures against a deliberately broken one. |
1 participant
Summary
Four of the seven bypasses recorded in keep-node-2sul still worked against the current script. This closes them and adds
scripts/test-rng-hygiene.sh, which asserts every known bypass still fails.What was still bypassable
Verified by reproduction, each against a positive control that does fail the guard:
let src = "/dev/" + "urandom"d=urandom; head -c 32 /dev/$dscripts/x.bashThe other three from that issue were already fixed by #104: quote-aware comment splitting, a blank line ending the marker block, and rule 3 pinning the draw structurally rather than grepping for "refusing to format". The issue was written before that landed, so this PR only addresses what actually reproduced.
Fixes
File selection. The extension gate was the cheapest bypass: rename the file and the rules never see it.
list_sourcesnow also takes*.bash, and sweeps every tracked file whose first line is ash,bashorpythonshebang, so an extensionless hook is scanned too.Concatenation.
"/dev/" + "urandom"and the++spelling read the same file at runtime while matching no line regex. Rules now match against a normalised copy with quote-adjacent concatenation collapsed. Findings are still reported against the raw line, so output shows what the author wrote.Runtime device paths. Indirection through a variable defeats any literal match, so there is a new rule: a
/dev/path assembled from a variable is a finding unless marked. A dynamic device path is rare here, and the marker documents it when it is deliberate.The self-test
The durable half. This guard has now had seven bypasses across two rounds, and a scanner that quietly stops scanning reports a clean tree exactly like a clean tree does.
scripts/test-rng-hygiene.shstages probe files into a throwawayGIT_INDEX_FILE, so it never touches the working tree or staged changes, and asserts 11 rejections plus 2 acceptances.It leads with a positive control: if a plain
/dev/urandomever stops failing, the whole file is meaningless and it says so.Test plan
bash -nclean on both scripts,ci.ymlparses,nix fmtreports 0 changedNot claimed: this makes the guard harder to defeat, it does not make it undefeatable. A line-oriented scanner can always be outrun by enough indirection. What changed is that the cheap routes are closed and the known ones are now asserted on every run.
Summary by CodeRabbit
Tests
Chores