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

[rgushchin]

🤖 AI-Generated Review 🤖

I have reviewed this pull request and uncovered a few regression risks and edge cases concerning input hardening:

1. SSRF logic bypass via git@ SSH URL injection (High Risk)

The manual host extraction in is_safe_repo_url contains a crucial logic flaw that permits attackers to circumvent string-based SSRF defenses (e.g., 127., localhost) for SSH URLs.

    // For git@ URLs, extract the host between @ and :
    let host = if let Some(rest) = url.strip_prefix("git@") {
        rest.split(':').next().unwrap_or("").to_ascii_lowercase()
    }

Exploit: Submitting an SSH clone URL of the form git@github.com@127.0.0.1:repo.git causes:

1. strip_prefix("git@") leaves github.com@127.0.0.1:repo.git.
2. split(':').next() extracts the 'host' as github.com@127.0.0.1.
3. The extracted string begins with "github.com", so the routine successfully bypasses the loopback blocks (!host.starts_with("127."), etc.).
   When git clone pipes this downstream string to the underlying ssh client, SSH parses [user@]hostname dynamically up to the last @. SSH correctly targets the host as 127.0.0.1 and establishes a connection, leading to an unauthenticated SSRF.

Recommendation: Consider extracting the SSH host securely by locating the last @ before the colon instead of .split(':'), or use string validation via the url crate.

2. URL blocklist evasion via Alternate IP Encodings

As acknowledged in the design document, the blocklist is exclusively string-based. Attackers can trivially bypass these matches for HTTP/HTTPS targets using standard browser and request parsing variants:

• Cloud Metadata (169.254.169.254) avoids 169.254. checks using decimal http://2852039166/latest/ or hex http://0xA9FEA9FE/latest/.
• The explicit loopback string constraint 2130706433 is easily sidestepped by calling other interfaces loopback equivalents like http://2130706434/ (127.0.0.2).

Recommendation: For a resilient blocklist approach, rely on DNS resolution explicitly mapping against std::net::IpAddr::is_loopback and is_private.

3. Off-by-one check in MAX_MBOX_DECOMPRESSED limits

In fetch_and_inject_thread(), when capping decompression sizes using a .take() adapter:

        let mut limited = decoder.take(MAX_MBOX_DECOMPRESSED);
        let mut raw_bytes = Vec::new();
        limited.read_to_end(&mut raw_bytes)?;
        if raw_bytes.len() as u64 >= MAX_MBOX_DECOMPRESSED {
            return Err(...)
        }

If an uploaded .mbox expands to explicitly and exactly 50 MiB, read_to_end() populates the buffer properly. However, because it asserts >= MAX_MBOX_DECOMPRESSED, it artificially trips the error for size bounding, rejecting perfectly valid content aligning with the size envelope. Evaluating against > would cover bounds efficiently without punishing exact matches.

---

Generated by Jetski assisted by Gemini 3.1 Pro

[derekbarbosa]

v3 -> v4:

• Fix SSRF bypass via git@ SSH URL injection. SSH resolves the
  LAST '@' as the user/host separator, but host extraction used
  the first segment. An attacker could submit
  "git@github.com@127.0.0.1:repo.git" and bypass all loopback
  checks. Fix: use rsplit('@') to match SSH parsing behavior.

• Extend cloud metadata SSRF coverage. Add decimal (2852039166)
  and hex (0xa9fe) representations of 169.254.169.254. Expand
  loopback decimal check from exact 2130706433 match to full
  127.0.0.0/8 range (2130706432..2130706687). Add octal 0177
  representation.

---

1. SSRF logic bypass via git@ SSH URL injection
   Fixed in v4: host extraction now uses rsplit('@').next() to match SSH's last-@ parsing behavior. git@github.com@127.0.0.1:repo.git correctly extracts 127.0.0.1 as the host and is blocked. Test added in test_is_safe_repo_url_blocks_ssh_injection.

2. URL blocklist evasion via alternate IP encodings
   Fixed in v4. Added decimal 2852039166 for 169.254.169.254, hex 0xa9fe prefix for the metadata range, and replaced the exact 2130706433 match with a range check (2130706432..=2130706687) covering the full 127.0.0.0/8 loopback block in decimal. Octal 0177 prefix was already covered in v3.

DNS resolution adds latency in the request path and introduces a DNS-as-oracle dependency. The primary access control is HMAC signature verification; the blocklist is defense-in-depth for the most common automated SSRF payloads. This trade-off is documented.

3. Off-by-one check in MAX_MBOX_DECOMPRESSED
   Read::take(N) guarantees read_to_end returns at most N bytes. len > N is always false when take(N) is the source.

The >= checks against truncation: if we read exactly N bytes, the stream had at least N bytes and may have had more (we can't tell because take stopped reading). The one ambiguous case — a file that is exactly 50 MiB — is rejected as a false positive rather than accepted as a possibly-truncated stream.

[derekbarbosa]

Some findings from the updated review-pr skill

  While the security model is robust, there are two minor issues related to false positives and precision:

   1. 🔵 False Positives in SSRF Blocklist (src/forge.rs): 
     The checks !host.contains("169.254.") and !host.contains("metadata.google") are overly broad. They will block legitimate subdomains that happen to contain those strings (e.g.,
  github-169.254.example.com or my-metadata.google.com). Since the URL crate strips auth/credentials and leaves only the hostname, it is much safer to use .starts_with and exact string
  equality.
     (Note: The blocklist remains "best effort" as documented; attackers can still bypass it using IPv4-mapped IPv6 like http://[::ffff:169.254.169.254] which url::Url normalizes to
  [::ffff:a9fe:a9fe]. This is acceptable given signature verification is your primary access control).

   2. 🔵 Decompression Limit Precision (src/api.rs):
     limited.read_to_end() on a Take adapter quietly yields EOF when the limit is reached. The current logic raw_bytes.len() as u64 >= MAX_MBOX_DECOMPRESSED will throw an error if the
  mbox is exactly 50 MiB, even if it hasn't actually exceeded the limit. To accurately determine if the stream was truncated, we must check if the underlying decoder has more data.

[derekbarbosa]

v4 -> v5:

• Narrow SSRF host checks: contains("169.254.") -> starts_with(),
  contains("metadata.google") -> exact "metadata.google.internal"
  match. Eliminates false positives on subdomains.

• Fix decompression limit false positive: probe one byte past the
  take() boundary to distinguish exactly-50-MiB files (accept) from
  truncated streams (reject).