security: webhook signature verification and input validation

[derekbarbosa]

Harden all HTTP ingestion endpoints against untrusted input. This PR addresses four security gaps identified during an audit of the webhook and submit API attack surface:

Webhook HMAC signature verification
The ForgeProvider trait's validate_event now accepts the request body and an optional secret, enabling cryptographic verification of incoming webhooks. Three authentication methods are supported:

• Standard Webhooks HMAC-SHA256 (GitLab 19.0+ signing tokens)
• GitHub X-Hub-Signature-256 HMAC-SHA256
• Legacy X-Gitlab-Token constant-time comparison

Secret type is auto-detected
whsec_ prefix triggers Standard Webhooks key decoding; plain strings are used directly. A warning is logged at startup if base64 decoding fails for a whsec_-prefixed token.

The access control in forge_webhook is revise
When webhook_secret is configured, non-localhost requests are permitted because the signature check IS the access control. --enable-unsafe-all-submit is only needed for unauthenticated setups.

Body and decompression size limits
An explicit DefaultBodyLimit::max(2 MiB) is added to the axum router. The fetch_and_inject_thread function gains a 10 MiB download cap and a 50 MiB decompression cap (via Read::take()) to mitigate gzip bombs from lore.kernel.org fetches.

SHA format validation -- is_valid_git_sha() validates that commit SHAs are 40 or 64 hex characters. Applied in both forge parse_payload methods. Not applied to the CLI submit path, which legitimately sends git refs and short SHAs.

URL validation and SSRF mitigation
is_safe_repo_url() rejects repository URLs targeting cloud metadata endpoints (169.254.*), loopback addresses, and other internal destinations. Applied in forge parse_payload methods. PR/MR numbers must be positive. Message-ID path separators are rejected in the Thread submit handler.

New dependencies: hmac 0.13, base64 0.22, subtle 2

[derekbarbosa]

finished running some additional tests, triggering reviews via HMAC tokens appear to be working well.

[rgushchin]

Gemini has some objections/findings:
🚨 Regression
* Silent MBox Truncation: The new limit in fetch_and_inject_thread (decoder.take(MAX_MBOX_DECOMPRESSED))
silently truncates streams exceeding 50MB because read_to_string will successfully return Ok upon hitting the limit.
This breaks the parsing of legitimate large threads. It needs an explicit io::Error when the limit is breached instead
of silently succeeding.

**🛡️ Security / Incomplete Fixes**
* **Path Traversal via msgid:** The blocklist (`\`, `/`) for `clean_msgid` misses URL-encoded payloads (e.g., `%2f`).

Since this gets passed to reqwest, an attacker can bypass the check and traverse upstream lore endpoints. Fix: Use
urlencoding::encode(clean_msgid) rather than a blocklist.
* SSRF String-Matching Bypass: is_safe_repo_url uses basic string containment to block loopback addresses (127. 0.0.1), which is trivially bypassed using decimal IPs (e.g., 0x7f000001 or 2130706433). We should either validate
via DNS/socket resolution or strictly enforce webhook secrets.

**🐛 Minor Bug snippet**
* **Case-sensitive Signature Validation:** `verify_github_signature` formats the hash with `{:02x}` (strict lowercase)

and uses ct_eq. If a forge happens to send an uppercase valid hex signature, it will fail. Validating raw decoded
bytes is safer.

[derekbarbosa]

Thank you for the review. I am curious why gemini did not pick this up with the review skill.

fixing now, will run a few more passes of review locally before pushing V2.

[rgushchin]

Idk, I always ask it to focus on potential regressions for the existing functionality and separately security concerns. Maybe we really need to spawn sashiko for sashiko?

[rgushchin]

That would be actually cool!

[derekbarbosa]

That would be actually cool!

/me looks over at custom-prompts RFC 😄

[derekbarbosa]

v1 -> v2:

• addressed review concerns from latest review and from review-pr skill

also, I had gemini do an analysis on some of the claims made

Silent MBox Truncation
Fixed. fetch_and_inject_thread now returns an explicit io::Error when the decompressed output hits the 50 MiB limit, rather than silently truncating. Commit a4b7f33.

SSRF Blocklist
Extended with decimal IP (2130706433) and hex IP (0x7f000001, 0x7f.0) representations of loopback addresses. Note that this remains a best-effort blocklist by design -- DNS rebinding can still bypass string-based checks. The primary access control is webhook signature verification (HMAC), which runs before parse_payload. An attacker who can forge a valid signature already holds the webhook secret, at which point SSRF is a secondary concern. The code and design doc both document this explicitly.

^ considering webhooks are still "best effort" and this is documented in multiple places (as well as HMAC and/or SSL/TLS being recommended), I am veering towards "YAGNI" for making the blocklist more robust than it needs to be.

Path Traversal via URL-encoded msgid
Not a vulnerability in practice. reqwest preserves percent-encoding in URL paths -- a literal %2F is sent to the server, not decoded to /. Added a code comment documenting this behavior at the check site.

[rgushchin]

My Gemini has few more findings, it looks like some of those are false positives based on your comments above, but not all of them. Please, take a look (1 and 3 in particular). Thanks!

1. Global 2MB Body Limit blocks large CLI submissions (Regression)

Issue: axum::extract::DefaultBodyLimit::max(2 * 1024 * 1024) is applied globally to the Axum router.
Impact: It imposes a strict 2MB limit on the /api/submit endpoint used by the CLI. For Linux kernel development, patch series frequently exceed 2MB. Valid CLI
submissions of large patchsets will now crash with HTTP 413 Payload Too Large , breaking an existing core functionality. Furthermore, GitHub allows webhook payloads
up to 25MB, meaning large legitimate PRs will silently fail validation.
Recommendation: Remove the global blanket limit. Apply the body limits exclusively to endpoints that require them, or increase the limit sufficiently to handle 25MB
external payloads and large patch injections.

2. Valid RFC-5322 Message-IDs are rejected (Regression)

Issue: In fetch_and_inject_thread , clean_msgid.contains('/') rejects threads containing a forward or backward slash.
Impact: Per RFC 5322 section 3.2.3, / is an explicitly valid atext character and is routinely found in legitimate Message-ID addresses in the wild. Hard-
rejecting it prevents the CLI from successfully resolving threads that previously worked.
Recommendation: Instead of rejecting the input, properly URL-encode the msgid (e.g. converting / to %2F ) before structuring the reqwest URL for
lore.kernel.org .

3. Reverse Proxy Authentication Bypass

Issue: The access control logic relies on checking addr.ip().is_loopback() . However, WEBHOOK_SECURITY.md explicitly recommends terminating TLS using a local
reverse proxy (e.g., nginx or Caddy).
Impact: If Sashiko is running behind a local reverse proxy, Axum evaluates the connection from the proxy, causing is_loopback() to evaluate to true for all
inbound internet traffic. Attackers can fire unauthenticated webhooks through the proxy, bypassing the !has_secret && !state.allow_all_submit restriction.
Recommendation: Utilize Axum extractors to read trustable X-Forwarded-For or X-Real-IP proxy headers for network evaluation, or specifically require the
webhook_secret regardless of the loopback origin.

4. Overeager SSRF Blocklist False Positives

Issue: is_safe_repo_url() attempts to mitigate SSRF via basic substring matching, such as .contains("127.0.0.1") and .contains("0x7f000001") , against the entire
repository URL.
Impact: Legitimate public repositories that happen to include these strings in their GitHub username or repository names (e.g., https://github.com/Bob-
127.0.0.1/sashiko_fork.git ) will suddenly trigger a false positive and fail ingestion.
Recommendation: Parse the provided string using the url crate, and constrain the SSRF and IP blacklisting to evaluate strictly against the parsed host component
rather than performing string matching across the entire path.

[derekbarbosa]

v2 -> v3:

• Raise DefaultBodyLimit from 2 MiB to 25 MiB to accommodate large
  CLI mbox submissions and GitHub webhook payloads (up to 25 MB).
• Replace message-ID slash/backslash blocklist with percent-encoding
  (percent-encoding crate). RFC 5322 message-IDs containing path-
  significant characters are now encoded rather than rejected.
  Also encode '%' to prevent ambiguous percent-sequences.
• Restructure forge_webhook access control: when webhook_secret is
  configured, signature verification is the sole access control for
  ALL requests, including loopback. Fixes authentication bypass when
  running behind a local reverse proxy where all traffic arrives
  from 127.0.0.1. The is_loopback shortcut now only applies when no
  secret is configured.
• Replace full-URL string matching in is_safe_repo_url() with parsed
  host-only checks (url crate). Eliminates false positives from
  blocklist patterns in usernames or paths.
  add octal 0177, decimal 2130706433, hex 0x7f representations.
• Return explicit io::Error when decompressed mbox hits the 50 MiB
  limit instead of silently truncating.
• Read decompressed mbox into Vec before UTF-8 conversion to
  avoid InvalidData errors when the byte limit splits a multi-byte
  character at the boundary.
• Clarify ForgeProvider::validate_event doc: when secret is None,
  only event-type validation is performed and the request is treated
  as unauthenticated.
  New dependencies: url 2.5, percent-encoding 2.3