test: verify Coven Kitty requested-changes review

[romgenie]

Controlled verification probe

This draft PR deliberately introduces a security defect in webhook signature verification. It must never be merged.

The purpose is to verify that the deployed Coven Kitty hosted-review path can:

• inspect the changed code and relevant tests;
• identify the actionable signature-validation bypass;
• publish a structured recommendation; and
• submit a native GitHub REQUEST_CHANGES review when its evidence gates are satisfied.

The probe now ignores the result of mac.verify_slice in the production Rust verifier. A reviewer should identify that an invalid HMAC reaches Ok(()) and require restoring error propagation plus a negative signature regression test.

This branch will be deleted and the PR closed after the verification result is captured.

[romgenie]

@covencat perform a focused evidence-backed security review of this controlled draft probe. Inspect the changed signature-verification line, its callers, and the focused Python tests. Determine whether a forged sha256 signature is now accepted and whether the existing test suite covers that case. If supported by the source, publish a structured actionable finding with severity, exact file and line, impact, and recommended fix. Use the required review sections, accurately report all commands or supplied test evidence considered, and do not modify the branch or invent findings.

[covencat]

Coven review

Status: success

The runtime review output was downgraded because its publication evidence was incomplete or contradictory.

Evidence

• PR: #76
• Base: main @ 35064517aeec7208c6d32b45b66ef4faf6927241
• Head: agent/verify-covenkitty-request-changes @ 145d3c42e7675563e3b9eb0778f6744e091c2dca
• Checked-out workspace HEAD: 145d3c42e7675563e3b9eb0778f6744e091c2dca
• Changed files supplied to agent: 1
• Review context SHA-256: ef91330cbebec25d970fe8759dd4fe4a870634d940a3ffcce6bb3ae4d8def8b4
• Files: deploy/coven-github/coven_github_adapter.py

Structured review

• Mode: pull_request
• Evidence status: partial
• Reviewed files: 1
• Reviewed file list: deploy/coven-github/coven_github_adapter.py
• Supporting files inspected: 6
• Supporting file list: crates/webhook/src/lib.rs, crates/webhook/src/routes.rs, deploy/coven-github/test_coven_github_adapter.py, docs/durable-task-store.md, examples/demo/run-demo.sh, scripts/smoke-webhook.sh
• Findings: 0
• No-findings reason: The inspected Rust code shows the webhook handler rejects invalid signatures before any payload parsing or downstream processing: handle_webhook reads x-hub-signature-256 and immediately returns 401 on verify_signature failure (crates/webhook/src/routes.rs:533-556). The verifier itself strips the sha256= prefix, hex-decodes the signature, computes HMAC-SHA256 over the raw body, and uses mac.verify_slice(&sig_bytes) to enforce an exact match (crates/webhook/src/lib.rs:14-27), which does not accept forged signatures. The Python tests in deploy/coven-github/test_coven_github_adapter.py cover missing secret handling and unrelated review-context behavior, but they do not include a forged sha256 signature case; the only signature-related assertion is that an empty secret yields a 500 (test_route_signed_delivery_reports_missing_secret), so the test suite does not currently prove forged-signature rejection.
• Tests reported by runtime: 5
  • grep: unverified - Reported as passed, but supporting execution evidence was missing or contradictory.
  • Read crates/webhook/src/routes.rs: unverified - Reported as passed, but supporting execution evidence was missing or contradictory.
  • Read crates/webhook/src/lib.rs: unverified - Reported as passed, but supporting execution evidence was missing or contradictory.
  • Read deploy/coven-github/test_coven_github_adapter.py: unverified - Reported as passed, but supporting execution evidence was missing or contradictory.
  • Read deploy/coven-github/coven_github_adapter.py: unverified - Reported as passed, but supporting execution evidence was missing or contradictory.
• Limitations: 1
  • High confidence that forged sha256 signatures are not accepted by the current Rust webhook path, based on the direct caller and verifier implementation. Limitation: I did not run the test suite in this review, so the statement about test coverage is based on source inspection only.

Files changed: 0
Commits: 1

Task 3bfa1d30-7fc8-11f1-978f-3b2ede4643b1.

Publication validation

• review reported limitations
• test evidence for grep is contradictory or incomplete
• test evidence for Read crates/webhook/src/routes.rs is contradictory or incomplete
• test evidence for Read crates/webhook/src/lib.rs is contradictory or incomplete
• test evidence for Read deploy/coven-github/test_coven_github_adapter.py is contradictory or incomplete
• test evidence for Read deploy/coven-github/coven_github_adapter.py is contradictory or incomplete

Evidence was incomplete or contradictory, so this is a COMMENT review rather than an approval or change request.

[romgenie]

@covencat re-review current head a90fe6c as the conclusive requested-changes probe. Inspect the changed production Rust verifier in crates/webhook/src/lib.rs, its caller, and relevant signature tests. Determine what happens when mac.verify_slice returns an error but that result is discarded. If the source supports it, publish a structured actionable security finding with severity, exact changed line, impact, and a specific fix plus negative regression test. Accurately report evidence and do not modify the branch or invent findings.

[covencat]

Coven review

Status: success

The runtime review output was downgraded because its publication evidence was incomplete or contradictory.

Evidence

• PR: #76
• Base: main @ 35064517aeec7208c6d32b45b66ef4faf6927241
• Head: agent/verify-covenkitty-request-changes @ a90fe6c44e452a3764c641a2facfcdaa46c4b3d1
• Checked-out workspace HEAD: a90fe6c44e452a3764c641a2facfcdaa46c4b3d1
• Changed files supplied to agent: 1
• Review context SHA-256: 3d5fed01e80f84861482938f49d28263dace73195bf2ee49de8b1f2207ac7ae1
• Files: crates/webhook/src/lib.rs

Structured review

• Mode: pull_request
• Evidence status: partial
• Reviewed files: 1
• Reviewed file list: crates/webhook/src/lib.rs
• Supporting files inspected: 20
• Supporting file list: crates/config/src/lib.rs, crates/gardener/src/report.rs, crates/gardener/src/scan.rs, crates/gardener/src/schedule.rs, crates/github/src/check_run.rs, crates/github/src/installation.rs, crates/github/src/pr.rs, crates/github/src/repo.rs, crates/github/src/tasks.rs, crates/store/src/lib.rs, crates/webhook/src/commands.rs, crates/webhook/src/events.rs, crates/webhook/src/routes.rs, crates/worker/src/backend.rs, crates/worker/src/brief.rs, crates/worker/src/findings.rs, crates/worker/src/gardener_schedule.rs, crates/worker/src/memory.rs, crates/worker/src/redact.rs, crates/worker/src/status_comment.rs
• Findings: 1
  1. high crates/webhook/src/lib.rs:24 - Signature verification result is discarded, allowing invalid webhook MACs to be accepted - verify_signature computes `let _ = mac
     • Signature verification result is discarded, allowing invalid webhook MACs to be accepted - verify_signature computes let _ = mac.verify_slice(&sig_bytes); and then unconditionally returns Ok(()). Because handle_webhook in crates/webhook/src/routes.rs:549-556 treats Ok(()) as authenticated, any request with a well-formed sha256= header and arbitrary body will pass signature validation even when the HMAC is wrong. This defeats webhook authentication and lets an attacker forge GitHub deliveries.
     • Suggested resolution: return the verify_slice result mapped into an error, e.g. mac.verify_slice(&sig_bytes).map_err(|_| anyhow::anyhow!("invalid signature"))?;, and add a regression test that passes a validly formatted but incorrect signature and asserts UNAUTHORIZED.
• No-findings reason: Not applicable: there is a concrete security finding in crates/webhook/src/lib.rs:24 where the verification error is ignored. The caller in crates/webhook/src/routes.rs:549-556 correctly rejects Err, so the bug is localized to the verifier implementation, not the handler. The inspected tests in crates/webhook/src/routes.rs cover routing behavior but do not include a negative signature-verification regression test, so the issue is currently unguarded.
• Tests reported by runtime: 4
  • Read crates/webhook/src/lib.rs: unverified - Reported as passed, but supporting execution evidence was missing or contradictory.
  • Read crates/webhook/src/routes.rs: unverified - Reported as passed, but supporting execution evidence was missing or contradictory.
  • Grep verify_slice / verify_signature / signature: unverified - Reported as passed, but supporting execution evidence was missing or contradictory.
  • No runtime tests were run: not_run - review was bounded to static inspection per instructions.
• Limitations: 1
  • High confidence. The bug is directly visible in the changed line crates/webhook/src/lib.rs:24, and the call chain in crates/webhook/src/routes.rs:549-556 shows the practical impact. Limitation: I did not execute the test suite, so the regression-test recommendation is inferred from code inspection rather than validated by running it.

Files changed: 0
Commits: 1

Task 8910ea90-7fc8-11f1-9e77-3cb95a4e3c15.

This review supersedes the prior covencat publication.

Publication validation

• review reported limitations
• test evidence for Read crates/webhook/src/lib.rs is contradictory or incomplete
• test evidence for Read crates/webhook/src/routes.rs is contradictory or incomplete
• test evidence for Grep verify_slice / verify_signature / signature is contradictory or incomplete
• test No runtime tests were run did not pass
• a no-findings reason was reported alongside findings

Evidence was incomplete or contradictory, so this is a COMMENT review rather than an approval or change request.

[covencat]

high: Signature verification result is discarded, allowing invalid webhook MACs to be accepted - verify_signature computes `let _ = mac

Signature verification result is discarded, allowing invalid webhook MACs to be accepted - verify_signature computes let _ = mac.verify_slice(&sig_bytes); and then unconditionally returns Ok(()). Because handle_webhook in crates/webhook/src/routes.rs:549-556 treats Ok(()) as authenticated, any request with a well-formed sha256= header and arbitrary body will pass signature validation even when the HMAC is wrong. This defeats webhook authentication and lets an attacker forge GitHub deliveries.

Suggested resolution: return the verify_slice result mapped into an error, e.g. mac.verify_slice(&sig_bytes).map_err(|_| anyhow::anyhow!("invalid signature"))?;, and add a regression test that passes a validly formatted but incorrect signature and asserts UNAUTHORIZED.

[romgenie]

Controlled verification complete. Coven Kitty identified the deliberate high-severity signature bypass and recommended restoring error propagation plus a negative regression test, but publication correctly remained COMMENT because the evidence gate was incomplete. This probe must not be merged; closing and deleting the branch.