From 572f079ae86b7c72c472c16d1336785dce39e46d Mon Sep 17 00:00:00 2001 From: olaayoade91-byte Date: Tue, 21 Jul 2026 11:50:36 +0000 Subject: [PATCH] fix: document X-StellarGate-Event as informational-only, route on signed body (#160) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The X-StellarGate-Event header is not covered by the HMAC signature — it can be altered in transit without invalidating the sig. This was a latent security issue: receivers that routed on the header rather than the signed body could be misled about the event type. The body already contains an authenticated `event` field (produced by build_payload and covered by the HMAC over "{timestamp}.{body}"), so no change to the signing scheme is needed. The fix is documentation, clear inline warnings, and tests that lock the contract. Changes: - webhook.rs module doc: list X-StellarGate-Event as a third header, explicitly marked NOT signed, with a note to route on the body field - sign() doc: note that the body already contains the event field, so event type is authenticated through the signature - dispatch() doc + inline comment: call out that the header is a convenience mirror of the body, not part of signed material - payments.rs redeliver_webhook(): same inline comment at the header send site - README: updated Verifying webhooks header table, added step 6 (read event from verified body), expanded Node.js example with a handleWebhook() that routes on body.event not the header - Tests: build_payload_includes_event_in_signed_body (unit) and event_field_in_body_matches_header_and_is_covered_by_signature (integration) lock the contract Also fixes pre-existing compile errors on main (parse_env missing ?, sqlx::Error conversion), updates anyhow 1.0.104 (RUSTSEC-2026-0190), spin 0.9.9 (yanked), and removes stale deny.toml entries. Fixes #160 --- Cargo.lock | 8 ++-- README.md | 24 +++++++++- deny.toml | 9 ---- src/api/payments.rs | 6 ++- src/config.rs | 19 ++++---- src/webhook.rs | 68 +++++++++++++++++++++++++++++ tests/webhook_dispatch_tests.rs | 77 +++++++++++++++++++++++++++++++++ 7 files changed, 185 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bc45a81..9edbdff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -19,9 +19,9 @@ checksum = "683d7910e743518b0e34f1186f92494becacb047c7b6bf616c96772180fef923" [[package]] name = "anyhow" -version = "1.0.102" +version = "1.0.104" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" +checksum = "330a5ed07fa54e4702c9d6c4174f74427fc0ef6e214bbd677ae50a5099946470" [[package]] name = "assert-json-diff" @@ -1965,9 +1965,9 @@ dependencies = [ [[package]] name = "spin" -version = "0.9.8" +version = "0.9.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67" +checksum = "3763264f6b73151db08c50ff20d7d8a0b8796e021cdea7ceedad07b80155fa0e" dependencies = [ "lock_api", ] diff --git a/README.md b/README.md index 0f09e41..9f7a089 100644 --- a/README.md +++ b/README.md @@ -380,13 +380,15 @@ matching payment status. ### Verifying webhooks -Every webhook request carries two headers: +Every webhook request carries two headers that together authenticate the event: | Header | Value | |---|---| | `X-StellarGate-Timestamp` | Unix time (seconds) at which the event was signed | | `X-StellarGate-Signature` | Hex HMAC-SHA256 of `"{timestamp}.{raw_body}"`, keyed with your `WEBHOOK_SECRET` | +A third header, `X-StellarGate-Event`, is included as a routing convenience (e.g. to quickly filter events in a load balancer before parsing JSON). **This header is not part of the signed material** — it mirrors the `event` field in the body but can be altered in transit without invalidating the signature. Always verify the signature first, then read the event type from the signed body. + The signature covers the timestamp as well as the body (Stripe-style), so a captured request cannot be replayed indefinitely. To verify: @@ -399,6 +401,9 @@ captured request cannot be replayed indefinitely. To verify: 4. Compute `HMAC_SHA256(WEBHOOK_SECRET, "{t}.{raw_body}")` and hex-encode it. 5. Compare it to `sig` with a **constant-time** equality check. Reject on mismatch. +6. After the signature passes, read the `event` field from the **body** to + determine the event type. Do **not** route on `X-StellarGate-Event` for + security-sensitive logic. Example (Node.js): @@ -417,6 +422,23 @@ function verify(rawBody, headers, secret, toleranceSec = 300) { .digest("hex"); return crypto.timingSafeEqual(Buffer.from(sig), Buffer.from(expected)); } + +// Usage: always read the event type from the verified body, never from the header. +// X-StellarGate-Event is a convenience header only — it is NOT signed. +function handleWebhook(rawBody, headers, secret) { + if (!verify(rawBody, headers, secret)) { + throw new Error("invalid signature"); + } + const payload = JSON.parse(rawBody); + const event = payload.event; // ← authenticated; safe to route on + // const event = headers["x-stellargate-event"]; // ← NOT authenticated; do not use + switch (event) { + case "payment.completed": /* ... */ break; + case "payment.overpaid": /* ... */ break; + case "payment.underpaid": /* ... */ break; + case "payment.expired": /* ... */ break; + } +} ``` ## Project Structure diff --git a/deny.toml b/deny.toml index 0476842..cb9d63a 100644 --- a/deny.toml +++ b/deny.toml @@ -33,23 +33,14 @@ allow = [ "Apache-2.0 WITH LLVM-exception", # compiler-builtins and related LLVM crates "BSD-2-Clause", "BSD-3-Clause", - "ISC", # used by some cryptography crates (ring deps) "Unicode-DFS-2016", # unicode-ident, icu_* and related crates "Unicode-3.0", # newer unicode-ident / icu_* releases - "CC0-1.0", # public-domain data crates "Zlib", # miniz_oxide and compression crates "OpenSSL", # openssl-sys transitive dep via native-tls ] # Crates that either bundle their own license text or use a non-SPDX # expression that cargo-deny cannot auto-classify. Add with justification. -# -# ring uses a BoringSSL-derived license not expressible in SPDX; if it is -# pulled in transitively (e.g. via rustls), it is acceptable for a payments -# service. See https://github.com/briansmith/ring/blob/main/LICENSE -[[licenses.exceptions]] -allow = ["LicenseRef-ring"] -name = "ring" # ── Bans ────────────────────────────────────────────────────────────────────── [bans] diff --git a/src/api/payments.rs b/src/api/payments.rs index f1a45d3..369f7e7 100644 --- a/src/api/payments.rs +++ b/src/api/payments.rs @@ -187,7 +187,8 @@ pub async fn create( .bind(&merchant_id) .bind(key) .execute(&state.pool) - .await?; + .await + .map_err(anyhow::Error::from)?; } } @@ -498,6 +499,9 @@ pub async fn redeliver_webhook( .header("Content-Type", "application/json") .header("X-StellarGate-Signature", &signature) .header("X-StellarGate-Timestamp", timestamp.to_string()) + // Convenience header — mirrors the `event` field already present in + // the signed body. NOT covered by the HMAC; receivers must route on + // the authenticated body field, not this header. .header("X-StellarGate-Event", &event) .body(delivery.payload.clone()) .send() diff --git a/src/config.rs b/src/config.rs index e2dcd7c..056a0a5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -164,13 +164,13 @@ impl Config { } }, webhook_secret, - webhook_retry_attempts: parse_env("WEBHOOK_RETRY_ATTEMPTS", 3), - webhook_retry_delay_ms: parse_env("WEBHOOK_RETRY_DELAY_MS", 5000), - poll_interval_secs: parse_env("POLL_INTERVAL_SECS", 10), - payment_ttl_secs: parse_env("PAYMENT_TTL_SECS", 3600), - rate_limit_requests_per_sec: parse_env("RATE_LIMIT_REQUESTS_PER_SEC", 10), - db_pool_max_connections: parse_env("DB_POOL_MAX_CONNECTIONS", 10), - db_busy_timeout_ms: parse_env("DB_BUSY_TIMEOUT_MS", 5000), + webhook_retry_attempts: parse_env("WEBHOOK_RETRY_ATTEMPTS", 3)?, + webhook_retry_delay_ms: parse_env("WEBHOOK_RETRY_DELAY_MS", 5000)?, + poll_interval_secs: parse_env("POLL_INTERVAL_SECS", 10)?, + payment_ttl_secs: parse_env("PAYMENT_TTL_SECS", 3600)?, + rate_limit_requests_per_sec: parse_env("RATE_LIMIT_REQUESTS_PER_SEC", 10)?, + db_pool_max_connections: parse_env("DB_POOL_MAX_CONNECTIONS", 10)?, + db_busy_timeout_ms: parse_env("DB_BUSY_TIMEOUT_MS", 5000)?, cors_allowed_origins, listener_mode: ListenerMode::parse( &std::env::var("STELLAR_LISTENER_MODE").unwrap_or_default(), @@ -700,10 +700,7 @@ mod tests { cfg.webhook_retry_attempts = 3; cfg.webhook_retry_delay_ms = 0; let err = cfg.validate_timing().unwrap_err().to_string(); - assert!( - err.contains("WEBHOOK_RETRY_DELAY_MS"), - "got: {err}" - ); + assert!(err.contains("WEBHOOK_RETRY_DELAY_MS"), "got: {err}"); } #[test] diff --git a/src/webhook.rs b/src/webhook.rs index 0058f09..bc18afe 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -7,6 +7,12 @@ //! - `X-StellarGate-Timestamp`: the Unix time (seconds) the event was signed. //! - `X-StellarGate-Signature`: the hex HMAC-SHA256 of `"{timestamp}.{body}"` //! (Stripe-style), keyed with the shared `WEBHOOK_SECRET`. +//! - `X-StellarGate-Event`: a convenience copy of the `event` field from the +//! body, included so receivers can route cheaply before parsing JSON. +//! **This header is not covered by the HMAC signature.** It can be altered +//! in transit without invalidating the signature. Receivers that make +//! security-sensitive decisions MUST route on the `event` field inside the +//! verified JSON body, not on this header. //! //! Binding the signature to the timestamp stops a captured request from being //! replayed indefinitely: a receiver recomputes the signature over the same @@ -27,6 +33,12 @@ type HmacSha256 = Hmac; /// Compute the hex-encoded HMAC-SHA256 signature for a webhook, binding it to /// `timestamp` by signing the Stripe-style payload `"{timestamp}.{body}"`. /// +/// The `body` is the full JSON event payload produced by [`build_payload`], +/// which always includes an `"event"` field. Because the event type is part of +/// the signed body, receivers can rely on it being authentic after verifying +/// the signature — no separate signing of the `X-StellarGate-Event` header is +/// needed or performed. +/// /// Receivers must recompute the signature over the same `"{timestamp}.{body}"` /// string and reject the request if `timestamp` is too far from their own clock /// (see the README), which is what prevents replay of an old, valid signature. @@ -74,6 +86,12 @@ pub fn build_payload(payment: &db::Payment, event: &str, delta: Option<&str>) -> /// recorded in the `webhook_deliveries` table. Errors are logged, never /// propagated — a failed webhook must not roll back a confirmed payment. /// +/// The signed body (`"{timestamp}.{body}"`) already contains the `event` field, +/// so the event type is fully authenticated by the HMAC signature. The +/// `X-StellarGate-Event` header is also included as a routing convenience but +/// is **not** covered by the signature — receivers must not use it for +/// security-sensitive decisions. +/// /// `delta` is the absolute amount difference included in overpaid/underpaid /// events; pass `None` for exact-payment events. pub async fn dispatch(state: &AppState, payment: &db::Payment, event: &str, delta: Option<&str>) { @@ -124,6 +142,9 @@ pub async fn dispatch(state: &AppState, payment: &db::Payment, event: &str, delt .header("Content-Type", "application/json") .header("X-StellarGate-Signature", &signature) .header("X-StellarGate-Timestamp", timestamp.to_string()) + // Convenience header — mirrors the `event` field already present in + // the signed body. NOT covered by the HMAC; receivers must route on + // the authenticated body field, not this header. .header("X-StellarGate-Event", event) .body(body.clone()) .send() @@ -209,4 +230,51 @@ mod tests { let body = b"{\"event\":\"payment.success\"}"; assert_ne!(sign("secret-a", 1, body), sign("secret-b", 1, body)); } + + #[test] + fn build_payload_includes_event_in_signed_body() { + // The `event` field must be present in the JSON body so that after + // signature verification a receiver can read the authenticated event + // type from the body rather than from the unsigned X-StellarGate-Event + // header. This test locks that contract. + let payment = db::Payment { + id: "pay_1".into(), + merchant_id: "merchant_1".into(), + destination_address: "GDESTINATION".into(), + memo: "ABCD1234".into(), + amount: "10".into(), + asset: "XLM".into(), + status: "completed".into(), + tx_hash: Some("txhash".into()), + paid_amount: Some("10".into()), + webhook_url: None, + created_at: "2026-01-01T00:00:00".into(), + updated_at: "2026-01-01T00:00:01".into(), + expires_at: "2026-01-01T01:00:00".into(), + }; + + for event in &[ + "payment.completed", + "payment.overpaid", + "payment.underpaid", + "payment.expired", + ] { + let payload = build_payload(&payment, event, None); + assert_eq!( + payload["event"].as_str(), + Some(*event), + "build_payload must embed the event type in the JSON body \ + so it is covered by the HMAC signature (event={event})" + ); + + // Confirm the serialised bytes contain the event string, i.e. that + // it survives the round-trip through serde_json::to_vec used in dispatch(). + let body = serde_json::to_vec(&payload).unwrap(); + let body_str = String::from_utf8(body).unwrap(); + assert!( + body_str.contains(event), + "serialised body must contain the event string (event={event})" + ); + } + } } diff --git a/tests/webhook_dispatch_tests.rs b/tests/webhook_dispatch_tests.rs index 7a4fe68..53ecddb 100644 --- a/tests/webhook_dispatch_tests.rs +++ b/tests/webhook_dispatch_tests.rs @@ -185,3 +185,80 @@ async fn dispatch_marks_failed_after_exhausting_retries() { assert_eq!(deliveries[0].status, "failed"); assert_eq!(deliveries[0].attempts, 3); } + +#[tokio::test] +async fn event_field_in_body_matches_header_and_is_covered_by_signature() { + /* Security regression test for issue #160. + * + * The X-StellarGate-Event header is NOT covered by the HMAC signature. + * Receivers MUST route on the `event` field inside the verified JSON body, + * not on the header. This test asserts: + * + * 1. The signed body contains the `event` field. + * 2. The header value mirrors the body's event field (i.e. they agree when + * the request has not been tampered with). + * 3. The HMAC signature is computed over the body (which includes `event`), + * so altering the header would not invalidate the signature — confirming + * the header is informational only. + */ + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/hook")) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + + let cfg = make_config("test-secret", 1); + let state = setup_state(cfg).await; + let payment = create_test_payment(&state, &format!("{}/hook", server.uri())).await; + + webhook::dispatch(&state, &payment, "payment.completed", None).await; + + let received = server.received_requests().await.unwrap(); + assert_eq!(received.len(), 1); + let req = &received[0]; + + // 1. The header is present. + let header_event = req + .headers + .get("X-StellarGate-Event") + .expect("X-StellarGate-Event header must be present") + .to_str() + .unwrap(); + + // 2. The body contains the `event` field. + let body: serde_json::Value = + serde_json::from_slice(&req.body).expect("body must be valid JSON"); + let body_event = body["event"] + .as_str() + .expect("body must contain an `event` field"); + + // 3. Header and body agree (no tampering in this happy-path test). + assert_eq!( + header_event, body_event, + "X-StellarGate-Event header must mirror the body event field" + ); + assert_eq!(body_event, "payment.completed"); + + // 4. The signature is valid over the body (which contains `event`), + // confirming the event type is authenticated through the body, not the header. + let timestamp: i64 = req + .headers + .get("X-StellarGate-Timestamp") + .unwrap() + .to_str() + .unwrap() + .parse() + .unwrap(); + let expected_sig = webhook::sign(&state.config.webhook_secret, timestamp, &req.body); + assert_eq!( + req.headers + .get("X-StellarGate-Signature") + .unwrap() + .to_str() + .unwrap(), + expected_sig, + "signature must be valid over the body (which contains the event field)" + ); +}