Add auth/session crypto primitives and RNG seeding lint - #595
Conversation
Adds the three launch-blocking native crypto builtins from section 13.1 plus a static-analysis rule, giving WFL auth/session code correct primitives instead of hand-rolled interpreted-WFL versions. New builtins (src/stdlib/crypto.rs): - pbkdf2_hmac_sha256 of password and salt and iterations and length: raw PBKDF2-HMAC-SHA256 key derivation with the iteration loop in native Rust, so a login handler cannot turn a KDF into a whole-site DoS. Returns hex. Bounds iterations and output length; validated against standard RFC/NIST vectors. - constant_time_equals of a and b: timing-safe comparison (subtle crate) for MACs, tokens, session IDs, and reset codes. Fixes the No-Unlearning gap where only short-circuiting `is` comparison was available. - secure_random_bytes of n: CSPRNG bytes as hex for salts, session IDs, and CSRF/reset tokens, avoiding modulo bias from composing tokens via random_int. Analyzer rule (ANALYZE-SECURITY): random_seed is now an error in any file that also performs cryptographic/auth/session work, since seeding makes the CSPRNG predictable. `wfl --analyze` exits non-zero so CI can block it; seeding remains allowed in ordinary non-security code. Wired the builtins through the registry, analyzer typechecker, and type inference. Updated the crypto docs (including the webhook example, now using constant_time_equals), the builtin reference, and added a Dev Diary entry. Tests: tests/crypto_kdf_test.rs (RFC/NIST vectors, timing-safe compare, CSPRNG bounds), analyzer unit tests for the lint, and TestPrograms/crypto_auth_primitives_test.wfl end-to-end. Full suite and all 106 integration programs pass; fmt and clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Com1byUspKRzYbAEDs1B1v
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds three native crypto builtins (pbkdf2_hmac_sha256, constant_time_equals, secure_random_bytes), registers them in the builtins/typechecker, adds a static-analysis rule flagging random_seed near security-sensitive code, and includes documentation plus Rust/WFL test coverage. ChangesAuth & Session Crypto Primitives
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Analyzer
participant CallSiteCollector
participant check_insecure_rng_seeding
Analyzer->>CallSiteCollector: collect_calls_in_statements(program)
CallSiteCollector-->>check_insecure_rng_seeding: CallSite list
check_insecure_rng_seeding->>check_insecure_rng_seeding: detect security-sensitive builtin present
check_insecure_rng_seeding->>Analyzer: emit ANALYZE-SECURITY diagnostics for random_seed calls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const SECURITY_SENSITIVE_BUILTINS: &[&str] = &[ | ||
| "hash_password", | ||
| "verify_password", | ||
| "argon2_hash", | ||
| "argon2_verify", | ||
| "bcrypt_hash", | ||
| "bcrypt_verify", | ||
| "scrypt_hash", | ||
| "scrypt_verify", | ||
| "pbkdf2_hash", | ||
| "pbkdf2_verify", | ||
| "pbkdf2_hmac_sha256", | ||
| "constant_time_equals", | ||
| "secure_random_bytes", | ||
| "generate_csrf_token", | ||
| "sha256", | ||
| "hmac_sha256", | ||
| "wflhash256", | ||
| "wflhash512", | ||
| "wflhash256_with_salt", | ||
| "wflmac256", | ||
| ]; |
There was a problem hiding this comment.
🔍 Lint sensitivity list includes non-security hash functions, risking false positives
The SECURITY_SENSITIVE_BUILTINS list at src/analyzer/static_analyzer.rs:110-131 includes wflhash256, wflhash512, wflhash256_with_salt, and wflmac256. The crypto module docs explicitly state that WFLHASH is 'NOT externally audited' and is suitable for 'non-critical data integrity verification' and 'checksums'. A file that uses wflhash256 for a checksum alongside random_seed for a reproducible simulation would trigger a false-positive ANALYZE-SECURITY error. Consider whether these general-purpose hashes should be in the sensitivity list, or whether only the explicitly security-oriented builtins (password hashing, HMAC-SHA256, CSRF tokens, the new auth primitives) should trigger the lint.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — fixed in 2bc5911. Removed sha256, wflhash256, wflhash512, and wflhash256_with_salt from SECURITY_SENSITIVE_BUILTINS, since their documented use is checksums/integrity/deduplication rather than authentication. A file that hashes data for a checksum and separately seeds the RNG for a reproducible simulation no longer triggers a false-positive ANALYZE-SECURITY error. The MACs (hmac_sha256, wflmac256) stay in the list because they authenticate, not just hash. Added tests covering sha256/wflhash-with-seed producing no diagnostic.
Generated by Claude Code
![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
🧹 Nitpick comments (1)
tests/crypto_kdf_test.rs (1)
118-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding boundary tests for upper-limit rejection.
The suite covers zero-value rejection for iterations and length but not the upper-bound rejection paths (
iterations > 100_000_000,length > 1024). Adding a test for each would lock in the DoS-guard behavior and match the upstream bounds insrc/stdlib/crypto.rs.✏️ Suggested additions
+#[tokio::test] +async fn test_pbkdf2_hmac_sha256_rejects_excessive_iterations() { + let code = r#" + store result as pbkdf2_hmac_sha256 of "pw" and "salt" and 100000001 and 32 + "#; + assert!(run_wfl_code(code).await.is_err()); +} + +#[tokio::test] +async fn test_pbkdf2_hmac_sha256_rejects_excessive_length() { + let code = r#" + store result as pbkdf2_hmac_sha256 of "pw" and "salt" and 1000 and 1025 + "#; + assert!(run_wfl_code(code).await.is_err()); +}🤖 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 `@tests/crypto_kdf_test.rs` around lines 118 - 132, The PBKDF2 test coverage in `test_pbkdf2_hmac_sha256_rejects_zero_iterations` and `test_pbkdf2_hmac_sha256_rejects_zero_length` is missing the upper-bound rejection paths. Add boundary tests that exercise `pbkdf2_hmac_sha256` with `iterations > 100_000_000` and `length > 1024`, and assert `run_wfl_code(...).await.is_err()` so the DoS guard behavior is locked in alongside the existing zero-value checks.
🤖 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 `@src/analyzer/static_analyzer.rs`:
- Around line 147-280: The call-site walker in collect_calls_in_statement
currently falls through on Statement::RespondStatement, so any calls inside its
request, content, status, content_type, and headers fields are ignored. Extend
this match arm to traverse all expression fields on RespondStatement the same
way other statement variants are handled, using collect_calls_in_expression for
each optional/present expression and preserving recursion into nested
structures. This will ensure sensitive builtins and random_seed are discovered
when they only appear in a RespondStatement.
---
Nitpick comments:
In `@tests/crypto_kdf_test.rs`:
- Around line 118-132: The PBKDF2 test coverage in
`test_pbkdf2_hmac_sha256_rejects_zero_iterations` and
`test_pbkdf2_hmac_sha256_rejects_zero_length` is missing the upper-bound
rejection paths. Add boundary tests that exercise `pbkdf2_hmac_sha256` with
`iterations > 100_000_000` and `length > 1024`, and assert
`run_wfl_code(...).await.is_err()` so the DoS guard behavior is locked in
alongside the existing zero-value checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c45c97d9-3d52-4deb-85e3-0b69185195a5
📒 Files selected for processing (11)
Dev diary/2026-07-09-auth-session-crypto-primitives.mdDocs/05-standard-library/crypto-module.mdDocs/05-standard-library/overview.mdDocs/reference/builtin-functions-reference.mdTestPrograms/crypto_auth_primitives_test.wflsrc/analyzer/static_analyzer.rssrc/builtins.rssrc/stdlib/crypto.rssrc/stdlib/typechecker.rssrc/typechecker/mod.rstests/crypto_kdf_test.rs
Two review-comment fixes for the ANALYZE-SECURITY lint:
- Narrow SECURITY_SENSITIVE_BUILTINS to auth/secret/MAC builtins only. Remove
the general-purpose hashes (sha256, wflhash256/512/with_salt) whose documented
use is checksums and data integrity, so hashing a file while seeding the RNG
for a reproducible simulation no longer produces a false-positive error. MACs
(hmac_sha256, wflmac256) stay, since they authenticate rather than just hash.
- Handle Statement::RespondStatement in the call-site walker so builtins used in
a web handler's respond (request/content/status/content_type/headers) are seen;
previously they fell through the `_ => {}` arm and could hide a sensitive call.
Adds tests: sha256/wflhash checksums do not trigger the lint, and random_seed is
flagged when the only sensitive call is inside a respond statement.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Com1byUspKRzYbAEDs1B1v
Summary
This change closes three launch-blocking gaps in section 13.1 by adding three new native cryptographic builtins and a static-analysis rule to prevent insecure RNG seeding in security-sensitive code.
Key Changes
New Builtins (src/stdlib/crypto.rs):
pbkdf2_hmac_sha256— Raw PBKDF2-HMAC-SHA256 key derivation with caller-supplied salt, iteration count, and output length. Runs the iteration loop in native Rust to bound per-call cost and prevent DoS on login endpoints. Validated against RFC/NIST standard test vectors.constant_time_equals— Timing-safe string comparison for MACs, CSRF tokens, session IDs, and password-reset codes. Uses thesubtlecrate to prevent timing-based side-channel attacks.secure_random_bytes— CSPRNG byte generation from the OS, returned as lowercase hex. Provides uniform entropy for salts, session identifiers, and tokens without modulo bias.Static Analysis Rule (src/analyzer/static_analyzer.rs):
check_insecure_rng_seedingmethod flagsrandom_seedcalls in files that also perform cryptographic, authentication, or session work. The heuristic: if a file calls any security-sensitive builtin (hash_password, verify_password, sha256, hmac_sha256, etc.) and callsrandom_seed, everyrandom_seedcall site is reported as an error with codeANALYZE-SECURITY. This prevents seeding the general-purpose RNG in security code where predictable output undermines salts, tokens, and session IDs.collect_calls_in_statements,collect_calls_in_expression) traverses all statement and expression types, including nested scopes (actions, loops, if/try blocks, test blocks, websocket/event handlers, container methods).Type Registration (src/stdlib/typechecker.rs, src/typechecker/mod.rs):
Text;pbkdf2_hmac_sha256takes 4 arguments (password, salt, iterations, length);constant_time_equalstakes 2 (a, b);secure_random_bytestakes 1 (n).Builtin Registry (src/builtins.rs):
BUILTIN_FUNCTIONS.Documentation (Docs/05-standard-library/crypto-module.md, Docs/reference/builtin-functions-reference.md):
constant_time_equalsinstead ofisfor signature comparison.pbkdf2_hmac_sha256(raw KDF) vs.pbkdf2_hash(self-describing hash).Tests:
tests/crypto_kdf_test.rs— Comprehensive test suite validating PBKDF2 against standard vectors (c=1/2/4096, dkLen=32/40), constant-time equality (equal/unequal/length-mismatch, HMAC verification), and secure random bytes (length, unpredictability, bounds).TestPrograms/crypto_auth_primitives_test.wfl— End-to-end WFL program demonstrating all three primitives and a complete password-storage round-trip.src/analyzer/static_analyzer.rsfor the RNG seeding lint: flagged in crypto context, flagged inside action bodies, allowed without crypto, clean with crypto but no seeding.Implementation Details
secure_random_bytesn ≤ 4096. Zero is rejected for all three.https://claude.ai/code/session_01Com1byUspKRzYbAEDs1B1v
Summary by CodeRabbit
New Features
Bug Fixes
Tests