PayPal webhook signature verification always returns :ok — unsigned webhooks are accepted

[timujinne]

Summary

PayPal webhook signature verification always succeeds. Any POST to
/webhooks/billing/paypal is treated as authentic, regardless of signature.

Verified on phoenix_kit_billing 0.7.1.

Root cause

PhoenixKitBilling.Providers.PayPal.verify_webhook_signature/3 forwards the
signature to verify_webhook_via_api/3, which has two clauses:

# paypal.ex:343
defp verify_webhook_via_api(token, payload, headers) when is_map(headers) do
  # builds the verify-webhook-signature request from
  # paypal-auth-algo / paypal-cert-url / paypal-transmission-id /
  # paypal-transmission-sig / paypal-transmission-time
  ...
  {:ok, %{"verification_status" => "SUCCESS"}} -> :ok
  {:ok, _} -> {:error, :invalid_signature}
end

# paypal.ex:363
defp verify_webhook_via_api(_token, _payload, _signature) do
  # If signature is just a string, we can't verify properly
  # In production, headers should be passed
  Logger.warning("PayPal webhook verification requires full headers map")
  :ok            # <-- paypal.ex:367
end

The first clause requires a map of headers. The controller passes a
single header value:

# webhook_controller.ex:59
def paypal(conn, _params) do
  handle_webhook(conn, :paypal, "paypal-transmission-sig")
end

handle_webhook/3 reads that one header with get_req_header/2 and passes the
resulting string down. A string never matches is_map/1, so every PayPal
webhook falls into the second clause, logs a warning and returns :ok.

PayPal's verification API needs five headers, and the controller's own docstring
says so — "PayPal verification requires multiple headers for signature
verification" (webhook_controller.ex:56). The code states the requirement and
does not meet it.

Impact

The {:error, :invalid_signature} branch that returns 401
(webhook_controller.ex:143-147) is unreachable for PayPal. Signature
verification is not a check that can fail — it is a function that returns :ok.

Anyone who knows the endpoint URL can submit webhook events without possessing
the webhook secret. handle_webhook_event/1 acts on event_type /
resource, including CHECKOUT.ORDER.APPROVED.

The route is mounted unconditionally by PhoenixKitBilling.Web.Routes
(routes.ex:18) — there is no gate on "provider configured". A host that
installed the billing module but never configured PayPal still exposes the
endpoint.

How this was observed

External POST to a live host, no signature headers, deliberately malformed body:

POST /webhooks/billing/paypal  ->  400

400, not 404 — the route is mounted and the request is processed.
Not 401 — the unsigned request is not rejected at the verification stage; it
proceeds and fails later, while decoding the body.

Note the failure mode is silent by design: the fallback clause logs at
warning level and returns success, so a host whose PayPal integration "works"
in testing is indistinguishable from one where verification never runs.

Suggested fix

1. Pass the full header map from the controller to the provider for PayPal,
   rather than a single header value.
2. Make the fallback clause fail closed: if verification cannot be
   performed, return {:error, :invalid_signature} instead of :ok.
3. Cover both directions with tests — the negative case (missing headers /
   bad signature must yield 401) is the one that matters here; a test that only
   asserts the happy path would still pass against the current code.
4. Consider gating the route on a configured provider, so hosts that do not use
   PayPal do not expose a webhook endpoint at all.

Happy to open a PR if that helps.

[ddon]

@timujinne are you going to fix this?

[timujinne]

Yes — this is fixed. The fix is on branch b008/fail-closed-webhook (7318b3f) in our fork.

Verified by mutation: before the fix, a forged webhook didn't just return 200 — it reached payment processing (Payment succeeded without invoice_uuid in the log). The change makes signature verification fail-closed (missing/forged signature → reject).

One caveat worth deciding on rather than discovering in a release: the fix closes the hole at the cost of reject-all. After it, all PayPal webhooks are rejected, not only forged ones — the only clause that actually verifies the signature requires a map, but the controller always passes the signature as a string, so that clause is currently unreachable. Before the fix it "worked" only by accepting everything unchecked. So the trade-off is between "insecurely accepts" and "securely rejects all" until a separate change makes the verify path reachable and restores acceptance of valid webhooks.

Two ways forward, your call:

1. Open a PR now with just the fail-closed fix (secure, reject-all interim), or
2. Include the dispatch fix so valid webhooks are accepted end-to-end (fail-closed and functional) in one PR.

Which do you prefer?

[timujinne]

To be precise about the blast radius (I overstated "all webhooks" above):

The gate order in handle_webhook/3 is secret before verify:

with {:ok, raw_body}  <- get_raw_body(conn),
     {:ok, signature} <- get_signature(conn, signature_header),
     {:ok, secret}    <- get_webhook_secret(provider),   # :not_configured -> 400 here
     :ok              <- verify_signature(provider, raw_body, signature, secret),

So get_webhook_secret/1 returns {:error, :not_configured} → 400 before verify runs. That means:

• Installs without the PayPal webhook secret configured returned 400 before this change and still return 400 — no behavior change.
• Only installs with the secret configured change behavior: from 200 (accept-anything) to 401 (reject), because that's the only path that reaches the verify clause.

So the reject-all effect is scoped to installs that actually have PayPal webhooks configured — not universal. The trade-off decision (interim fail-closed vs. full fix restoring valid-webhook acceptance) stands; this just narrows who it affects.