⏺ All CI checks pass. Phase 1 crypto (#244 + #245) is complete.

[navicore]

Summary of #244 + #245 implementation:

#244
#245

New builtins added:

• crypto.sha256 - SHA-256 hash (returns 64-char hex)
• crypto.hmac-sha256 - HMAC-SHA256 signature for webhooks/JWTs
• crypto.constant-time-eq - Timing-safe comparison (prevents timing attacks)
• crypto.random-bytes - Cryptographically secure random bytes
• crypto.uuid4 - UUID v4 generation

Files created/modified:

• crates/runtime/src/crypto.rs - Core implementation with 11 unit tests
• crates/runtime/src/lib.rs - Module registration
• crates/runtime/Cargo.toml - Added sha2, hmac, rand, uuid deps
• Cargo.toml - Workspace dependencies
• crates/compiler/src/codegen/runtime.rs - FFI declarations
• crates/compiler/src/builtins.rs - LSP signatures and docs
• crates/compiler/src/ast.rs - Builtin validation
• tests/integration/src/test-crypto.seq - 17 integration tests
• examples/crypto.seq - Working demo
• docs/BATTERIES_INCLUDED.md - Updated status

Test counts:

• 231 integration tests (17 new crypto tests)
• 11 unit tests in crypto.rs

Practical use cases now possible:

• Webhook signature verification
• JWT token generation/validation
• Secure session tokens
• API authentication (HMAC)
• Content addressing (SHA-256)

[claude]

Code Review: Phase 1 Crypto Implementation (#254)

Summary

This PR adds cryptographic primitives to Seq, implementing 5 new builtin functions for SHA-256, HMAC, secure random generation, UUID creation, and constant-time comparison. The implementation is well-architected and follows the project's established patterns. Overall, this is high-quality work with excellent test coverage and documentation.

✅ Strengths

1. Code Quality

• Clean, idiomatic Rust with proper error handling
• Follows established FFI patterns from encoding.rs and other runtime modules
• Excellent inline documentation and examples
• Consistent naming conventions (crypto.sha256, crypto.hmac-sha256, etc.)

2. Security Practices

• Uses well-vetted crates (sha2, hmac, rand, uuid) instead of custom crypto
• Implements constant-time comparison correctly to prevent timing attacks
• Uses rand::thread_rng() which leverages OS-level CSPRNG
• Proper 1024-byte limit on random generation to prevent memory exhaustion

3. Test Coverage

• 11 unit tests in crypto.rs covering edge cases (empty strings, zero bytes, uniqueness)
• 17 integration tests in test-crypto.seq including practical patterns (webhook verification)
• Tests verify both correctness (known SHA-256 hashes) and security properties (constant-time comparison, uniqueness)

4. Documentation

• Clear API documentation with stack effects
• Working examples in examples/crypto.seq demonstrating real-world use cases
• Updated BATTERIES_INCLUDED.md tracking implementation status

🔍 Issues & Recommendations

CRITICAL: Constant-Time Comparison Vulnerability

Location: crates/runtime/src/crypto.rs:116-119

The current implementation has a timing leak when strings have different lengths:

let mut result = a_bytes.len() ^ b_bytes.len();
for (x, y) in a_bytes.iter().zip(b_bytes.iter()) {
    result |= (*x as usize) ^ (*y as usize);
}

Problem: When lengths differ, the loop terminates early (zip stops at the shorter length), leaking timing information about the length difference.

Fix: Use the subtle crate (already in dependency tree via hmac) for truly constant-time comparison:

use subtle::ConstantTimeEq;

let a_bytes = a.as_str().as_bytes();
let b_bytes = b.as_str().as_bytes();
let eq = a_bytes.ct_eq(b_bytes);
unsafe { push(stack, Value::Bool(bool::from(eq))) }

Why this matters: Timing attacks can extract secrets byte-by-byte. The subtle crate is specifically designed for this and is already available (it's a transitive dependency of hmac).

Minor: Random Bytes Limit

Location: crates/runtime/src/crypto.rs:152-154

The 1024-byte limit is reasonable for preventing abuse, but:

• Documentation: Should be documented in the function's doc comment
• Consideration: 1024 bytes is quite generous. Common use cases (session tokens, nonces) rarely need more than 32-64 bytes. Consider lowering to 256-512 bytes, or add a comment explaining the rationale.

Minor: HMAC Key Size

Location: crates/runtime/src/crypto.rs:79-80

HmacSha256::new_from_slice(key.as_str().as_bytes()).expect("HMAC can take any key")

While technically true that HMAC accepts any key size, security best practices recommend:

• Minimum: 32 bytes (256 bits) for HMAC-SHA256
• Consideration: Add a warning in the doc comment about key size recommendations

Minor: Missing Input Validation

SHA-256 and HMAC functions don't validate input sizes. While unlikely to cause issues, extremely large strings could:

• Cause memory pressure
• Enable DoS attacks in web-facing applications

Recommendation: Consider adding a reasonable limit (e.g., 10MB) with a clear error message, or document that callers should validate input sizes in security-critical contexts.

Nitpick: Error Messages

Error messages use {:?} debug formatting which exposes full Value contents:

panic!("sha256: expected String on stack, got {:?}", value)

For Strings, this could leak sensitive data in logs. Consider using type-only error messages:

panic!("sha256: expected String on stack, got {}", value.type_name())

📋 Architecture & Integration

Positive

• ✅ Proper integration with compiler (ast.rs, builtins.rs, codegen/runtime.rs)
• ✅ Workspace dependencies in root Cargo.toml for version consistency
• ✅ FFI declarations follow established patterns
• ✅ Stack effects properly declared and validated by type system

Observations

• The crypto module uses global_string() for heap allocation, which is correct for returning strings
• Functions properly use unsafe blocks only where necessary (FFI boundary)
• No unnecessary cloning or allocations

🧪 Test Quality

Unit Tests

• ✅ Test known vectors (SHA-256 of "hello")
• ✅ Test edge cases (empty strings, zero bytes)
• ✅ Test uniqueness properties (UUIDs, random bytes)
• ✅ Test format validation (UUID structure)

Integration Tests

• ✅ Cover practical patterns (webhook verification)
• ✅ Test all 5 crypto functions
• ✅ Test security properties (constant-time comparison)

Missing Tests

• ⚠️ No test for random-bytes with maximum limit (1024 bytes)
• ⚠️ No test for negative input to random-bytes (panics are tested manually but not in CI)
• ⚠️ No test verifying constant-time property (though this is hard to test reliably)

🔐 Security Assessment

Strengths

1. ✅ Uses industry-standard crates with security audits
2. ✅ Recognizes need for constant-time comparison
3. ✅ Uses cryptographically secure RNG
4. ✅ Returns hex-encoded output (no binary string issues)

Weaknesses

1. ❌ Constant-time comparison has timing leak (see CRITICAL above)
2. ⚠️ No documentation of key size recommendations for HMAC
3. ⚠️ No input size limits could enable DoS

Recommendations

• Immediate: Fix constant-time comparison using subtle crate
• Short-term: Add key size recommendations to HMAC documentation
• Consider: Add input size limits with clear error messages

📊 Performance

• SHA-256 and HMAC performance is dominated by the underlying crate implementations
• global_string() allocation is appropriate for returning results
• No unnecessary allocations in hot paths
• thread_rng() uses thread-local storage, avoiding lock contention

🎯 Best Practices Compliance

Rust

• ✅ Proper use of #[unsafe(no_mangle)] for FFI
• ✅ Correct extern "C" ABI
• ✅ No clippy warnings (assumed, should verify)
• ✅ Follows project's stack-based value passing conventions

Crypto

• ✅ Don't roll your own crypto (uses established crates)
• ❌ Constant-time comparison needs fixing (see above)
• ✅ Uses hex encoding (avoids binary string pitfalls)
• ⚠️ Missing some security guidance in docs

📝 Documentation Review

Inline Docs

• ✅ Clear module-level documentation with examples
• ✅ Every function has doc comments with stack effects
• ✅ Safety requirements documented

Examples

• ✅ examples/crypto.seq demonstrates all 5 functions
• ✅ Shows practical patterns (webhook verification)
• ✅ Clear comments explaining each demo

Missing

• ⚠️ No mention of when to use constant-time-eq vs regular string.=
• ⚠️ No guidance on HMAC key management
• ⚠️ No warning about reusing UUIDs as secrets

✅ Checklist

• Code compiles without warnings
• Tests pass (231 integration, 11 unit)
• Documentation updated
• Examples provided
• Follows project conventions
• Security review (1 critical issue)
• Performance acceptable
• Integration with compiler/LSP

🚀 Recommendation

Approve with requested changes. This is solid work that enables important use cases (JWT, webhooks, secure tokens). The only blocking issue is the constant-time comparison timing leak.

Required Before Merge

1. Fix constant_time_eq to use subtle::ConstantTimeEq

Recommended Enhancements

1. Document 1024-byte limit for random-bytes
2. Add HMAC key size recommendations to docs
3. Consider input size limits to prevent DoS
4. Add test for random-bytes at maximum limit

Nice to Have

1. Example showing when to use constant-time-eq vs string.=
2. Warning about UUID security properties in docs

---

Great work implementing Phase 1 crypto! This brings Seq much closer to real-world application development. The test coverage is excellent and the API design is clean and intuitive. Once the constant-time comparison is fixed, this will be production-ready.

[claude]

Code Review: PR #254 - Crypto Phase 1 Implementation

Summary

This PR adds 5 cryptographic builtins to Seq: sha256, hmac-sha256, constant-time-eq, random-bytes, and uuid4. The implementation is excellent.

Code Quality - EXCELLENT

• Consistent with existing FFI patterns (encoding.rs)
• Comprehensive documentation with usage examples
• 11 unit tests + 17 integration tests
• Good error messages

Security - EXCELLENT

• Proper constant-time comparison using subtle crate
• Uses well-audited crypto crates (sha2, hmac, rand, uuid)
• 1024-byte limit prevents DoS attacks
• Cryptographically secure RNG from OS

Performance - GOOD

• Minimal allocations
• Thread-local RNG for performance
• Appropriate use of hex encoding

Test Coverage - EXCELLENT

All critical paths tested including edge cases: empty strings, zero-length, different lengths, max limits, uniqueness.

Verdict: STRONGLY RECOMMEND MERGING

This is production-ready code that follows security best practices and enables real-world use cases like JWT verification and webhook handling.

Minor suggestions (not blockers):

1. Consider making 1024-byte limit a constant
2. Add comment about OS CSPRNG source
3. Document HMAC key length best practices

Great work! Phase 1 crypto complete.

[claude]

Detailed Technical Review

Code Quality Details

Strengths:

• Module follows existing patterns from encoding.rs
• All functions properly document safety requirements
• Stack effects clearly documented
• Error messages are informative and actionable

Minor improvements:

• Line 81 in crypto.rs: Consider more specific error message for HMAC initialization
• Lines 152-158: The 1024-byte limit could be a named constant for maintainability

Security Analysis

Constant-time comparison (Lines 108-130):

• Correctly uses subtle::ConstantTimeEq
• Handles different-length strings without timing leaks
• Critical for preventing timing attacks on signature verification

HMAC-SHA256 (Lines 72-92):

• Uses standard hmac crate (widely audited)
• Proper key handling
• Appropriate for webhook verification and JWT signing

Random generation (Lines 146-167):

• Uses rand::thread_rng() backed by OS CSPRNG
• Input validation prevents negative values
• DoS protection via 1024-byte limit

UUID v4 (Lines 178-183):

• Standard uuid crate implementation
• Cryptographically secure randomness

Test Coverage Analysis

What's tested:

• All happy paths
• Empty/zero inputs
• Boundary conditions (max limit)
• Known test vectors (SHA-256, HMAC)
• Uniqueness properties (random, UUID)
• Practical patterns (webhook verification)

What could be added (not critical):

• Unicode/multibyte character handling
• Boundary values (1, 1023, 1024 bytes)
• HMAC with empty key

Performance Notes

• global_string() for string interning is consistent with codebase
• hex::encode() allocations are appropriate for this use case
• thread_rng() provides good balance of security and performance
• No unnecessary copies or allocations

Documentation Quality

• API docs at top of file are clear
• examples/crypto.seq provides working patterns
• Integration with LSP for IDE support
• BATTERIES_INCLUDED.md properly updated