Conversation
There was a problem hiding this comment.
Pull request overview
Adds a threat-oriented “adversarial code review” document capturing several trust-boundary/security findings across services, and updates adversary-detector digest-store tests to avoid temp-path collisions under parallel Tokio tests.
Changes:
- Add a new security review document with prioritized findings and recommended fixes.
- Update digest-store test temp-path generation to reduce parallel test collisions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/adversarial-code-review-2026-04-25.md | New threat-oriented security review notes and recommended remediation ordering. |
| crates/adversary-detector/src/digest.rs | Adjust test temp path naming to avoid parallel test overlap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### 1. WhatsApp webhook HMAC currently accepts any non-empty signature | ||
|
|
||
| **Severity:** Critical | ||
| **Evidence:** `crates/zeroclawed/src/channels/whatsapp.rs:824`, | ||
| `crates/zeroclawed/src/channels/whatsapp.rs:918` | ||
|
|
||
| When `webhook_secret` is configured, inbound WhatsApp webhook requests call | ||
| `verify_hmac_sha256`. The helper strips `sha256=`, ignores the secret and body, | ||
| and returns `true` for any non-empty signature header. | ||
|
|
||
| This creates a dangerous failure mode: the config and setup docs imply HMAC | ||
| protection, but an attacker who can reach the webhook only needs to include a | ||
| syntactically plausible `X-Hub-Signature-256` header. |
There was a problem hiding this comment.
The description of the WhatsApp HMAC issue is slightly inaccurate: verify_hmac_sha256 returns false unless the header starts with sha256=. The current placeholder accepts any signature with the sha256= prefix regardless of secret/body (even sha256= with empty hex), but a non-empty header without the prefix is rejected. Consider rewording this section to match the actual behavior so the finding isn’t overstated/ambiguous.
| ### 1. WhatsApp webhook HMAC currently accepts any non-empty signature | |
| **Severity:** Critical | |
| **Evidence:** `crates/zeroclawed/src/channels/whatsapp.rs:824`, | |
| `crates/zeroclawed/src/channels/whatsapp.rs:918` | |
| When `webhook_secret` is configured, inbound WhatsApp webhook requests call | |
| `verify_hmac_sha256`. The helper strips `sha256=`, ignores the secret and body, | |
| and returns `true` for any non-empty signature header. | |
| This creates a dangerous failure mode: the config and setup docs imply HMAC | |
| protection, but an attacker who can reach the webhook only needs to include a | |
| syntactically plausible `X-Hub-Signature-256` header. | |
| ### 1. WhatsApp webhook HMAC currently accepts any `sha256=`-prefixed signature | |
| **Severity:** Critical | |
| **Evidence:** `crates/zeroclawed/src/channels/whatsapp.rs:824`, | |
| `crates/zeroclawed/src/channels/whatsapp.rs:918` | |
| When `webhook_secret` is configured, inbound WhatsApp webhook requests call | |
| `verify_hmac_sha256`. The helper requires the header to start with `sha256=`, | |
| then strips that prefix, ignores the secret and body, and accepts any remaining | |
| value regardless of whether it matches the payload HMAC. | |
| This creates a dangerous failure mode: the config and setup docs imply HMAC | |
| protection, but an attacker who can reach the webhook only needs to include a | |
| syntactically plausible `X-Hub-Signature-256` header with the required | |
| `sha256=` prefix. |
There was a problem hiding this comment.
Codex integration sweep: addressed in #38 without editing this PR branch. WhatsApp webhook verification now uses real HMAC-SHA256, requires the sha256= prefix, and has valid/tampered/missing-prefix tests.
| fn tmp_path() -> PathBuf { | ||
| // Use a unique path in the system temp dir | ||
| // Tokio runs these tests concurrently; use a monotonic suffix so parallel | ||
| // tests cannot accidentally share a digest store. | ||
| let dir = std::env::temp_dir(); | ||
| let name = format!( | ||
| "digest-test-{}-{}", | ||
| std::process::id(), | ||
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_nanos() | ||
| NEXT_TMP_PATH_ID.fetch_add(1, Ordering::Relaxed) | ||
| ); |
There was a problem hiding this comment.
Using a process-local monotonic counter makes temp paths unique only within a single test run. If previous runs leave digest-test-<pid>-<n> files behind and the OS later reuses the same PID, these tests can read stale data and become flaky. Prefer using tempfile (already a dev-dep here) to create an auto-cleaned unique temp file/dir per test, or explicitly remove any pre-existing path before opening the store.
There was a problem hiding this comment.
Codex integration sweep: addressed in #38 without editing this PR branch. Digest tests now allocate paths under tempfile-created directories rather than pid/counter-derived names.
|
Closing — all 4 findings from this review were fixed in your PR #38 follow-up commits and cherry-picked into #44 (now in main as |
Summary
Why
The review identifies concrete trust-boundary risks and gives a prioritized fix order. While publishing it, the pre-push hook exposed a real test isolation issue: digest temp paths used process ID plus timestamp, which could collide under parallel Tokio tests.
Validation
cargo test -p adversary-detectorDraft until the security review findings and follow-up ordering are triaged.