Skip to content

Fix signed-webhook breakage: X-Forwarded-* headers + plain-HTTP proxy mode - #94

Merged
joaoh82 merged 2 commits into
mainfrom
fix/webhook-proxy-fidelity
Jul 22, 2026
Merged

Fix signed-webhook breakage: X-Forwarded-* headers + plain-HTTP proxy mode#94
joaoh82 merged 2 commits into
mainfrom
fix/webhook-proxy-fidelity

Conversation

@joaoh82

@joaoh82 joaoh82 commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Why

Twilio webhooks pointed at a rustunnel tunnel failed signature validation (403) while ngrok worked immediately. Investigation (with a local byte-fidelity echo test) showed the request body is already byte-faithful through the tunnel — the breakage is at the edge:

  1. Port 80 answered every request with a 301. A provider configured with an http:// webhook URL either has its POST degraded to a GET or re-signs against the redirect target — signature validation can never succeed across that hop.
  2. No X-Forwarded-For / X-Forwarded-Proto / X-Forwarded-Host were added, so backends couldn't reconstruct the public URL that providers sign (this is why a manual base-URL override was needed at all).

What

  • forward_http sets X-Forwarded-For (appended to inbound chains), X-Forwarded-Proto, and X-Forwarded-Host; spoofed inbound Proto/Host are overwritten at the trust boundary.
  • New [server] plain_http_mode = "proxy" | "redirect" (default redirect, so no behavior change until enabled). proxy forwards plain-HTTP requests whose subdomain resolves to a tunnel — ngrok parity, making http:// webhook URLs work — and 308-redirects the rest. Enabled in deploy/server.toml and deploy/local/server.toml.
  • Redirects are now 308 (method/body-preserving) instead of 301, and Location targets the HTTPS listener port instead of echoing the plain-HTTP port.
  • Docs: webhook guidance in the client guide.

Tests

  • New tests/integration/webhook_fidelity.rs: Twilio-style HMAC-SHA1-signed form POST through the full proxy chain (both edges), captured as raw bytes by the local service and re-validated backend-side using a URL reconstructed from X-Forwarded-* — encoding the "proxy must not change bytes" invariant. Plus 308 fallback coverage.
  • Unit tests for header injection/append/overwrite and redirect status/port behavior.
  • cargo fmt, clippy -D warnings, and the full workspace suite (22 suites) pass locally.

Rollout

Edges need plain_http_mode = "proxy" added to their server.toml when this ships (config default stays redirect).

🤖 Generated with Claude Code

joaoh82 and others added 2 commits July 21, 2026 23:41
… mode

Twilio webhooks through a tunnel returned 403 signature failures. Root
causes at the edge (the body itself was already byte-faithful):

- Port 80 301-redirected every request. Providers configured with an
  http:// webhook URL either drop the POST to a GET or re-sign against
  the redirect target, so signature validation can never pass.
- No X-Forwarded-For/Proto/Host headers were added, so backends could
  not reconstruct the public URL without a manual base-URL override.

Changes:

- forward_http now sets X-Forwarded-For (appending to inbound chains),
  X-Forwarded-Proto and X-Forwarded-Host at the trust boundary.
- New [server] plain_http_mode = "proxy" | "redirect" (default
  "redirect"): proxy mode forwards plain-HTTP requests whose subdomain
  resolves to a tunnel (ngrok parity) and 308-redirects the rest.
- Redirects are now 308 (method/body-preserving) instead of 301, and
  the Location points at the HTTPS listener port instead of echoing
  the original port.
- Integration suite: Twilio-style HMAC-SHA1 signed POST through the
  full proxy chain, validated backend-side from raw received bytes,
  over both edges; 308 fallback coverage; unit tests for the new
  header and redirect logic.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dening

- plain-HTTP gate now uses a new side-effect-free TunnelCore::has_http_route
  instead of resolve_http, which dispatched a member and double-counted
  request metrics for every port-80 request.
- IP rate limiting hoisted to both listener entry points so the gate probe
  is throttled and requests are counted exactly once.
- Redirect Location is clamped to the configured domain — an arbitrary Host
  was an open redirect that 308 would forward method+body to.
- X-Forwarded-For docs now state only the rightmost entry is edge-verified.
- TestServerOpts gains plain_http_mode; new integration test proves
  redirect mode 308s even for registered tunnel hosts.
- Drop stale ACME HTTP-01 claim from deploy/server.toml (DNS-01 only).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@marvin-agent-rockflow marvin-agent-rockflow left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Head: 6111130 · Files: 14 · +749 / -20
Verdict: Approve

Solid fix for the Twilio/signed-webhook breakage. The root causes (301 method downgrade + missing X-Forwarded-*) are correctly identified, and the second commit hardens the first-pass issues well (open redirect under 308, gate probe side effects via has_http_route, IP rate-limit hoisted so the existence check is throttled and not double-counted).

Critical

  • None

Warnings

  • None blocking

Suggestions

  • Domain clamp is case-sensitive (redirect_to_https / extract_subdomain): Host headers are case-insensitive per RFC 9110. A mixed-case Host like MyApp.Edge.Rustunnel.com fails ends_with(".{domain}") and gets clamped to the bare domain, breaking an otherwise-valid 308. Same pre-existing pattern in extract_subdomain. Consider ASCII-lowercase before compare (low practical risk — most clients send lowercase — but cheap to fix).
  • WebSocket path still skips set_forwarded_headers (handle_ws_upgrade): pre-existing; only matters if backends behind WS tunnels rely on X-Forwarded-*. Not needed for the webhook goal of this PR.
  • Stale ACME HTTP-01 wording may still linger in docs/docker-deployment.md (this PR correctly dropped it from deploy/server.toml). Optional doc sweep.

Looks good

  • plain_http_mode default stays redirect (no surprise prod behavior); deploy configs opt into proxy explicitly with clear comments.
  • X-Forwarded-Proto/Host overwritten at the trust boundary; X-Forwarded-For append + rightmost-trusted docs match real reverse-proxy practice.
  • Body is never re-serialized — correct invariant for HMAC providers.
  • 301 → 308 is the right status for method/body-preserving redirects; Location now targets the HTTPS listener port.
  • Open-redirect clamp is important once 308 is in play (method+body would follow to attacker Host).
  • Integration suite (webhook_fidelity.rs) is excellent: raw-byte capture, Twilio-style HMAC-SHA1, both edges, plus redirect-mode never-proxies and 308 fallback coverage.
  • Unit tests cover header append/overwrite, redirect status/port, and domain clamp.

Automated hourly review by marvin-agent-rockflow (Hermes). Will re-review only if new commits land.

// Only redirect within our own domain — an arbitrary Host here would
// make this an open redirect (and, with 308, forward method + body to
// an attacker-chosen destination).
if name != domain && !name.ends_with(&format!(".{domain}")) {

@marvin-agent-rockflow marvin-agent-rockflow Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Suggestion (nit): This domain membership check is case-sensitive. Host values are case-insensitive in practice/RFC 9110 — e.g. MyApp.Edge.Example.com would fail ends_with(".{domain}") and get clamped to the bare domain, producing a surprising Location. Same pattern exists in extract_subdomain. Consider comparing lowercased forms (and/or lowercasing once at Host parse). Low practical risk; not blocking.

@joaoh82
joaoh82 merged commit fd0bfcc into main Jul 22, 2026
1 check passed
@joaoh82
joaoh82 deleted the fix/webhook-proxy-fidelity branch July 22, 2026 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants