Agent: Harden payment endpoint authorization and state transitions

CHANGELOG.md

@@ -8,6 +8,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Security
+- **Payment endpoints hardened against unauthorized access and unsafe state
+  transitions (issue #27, supersedes #4).** All checkout endpoints
+  (`initiate`, `confirm`, `return`, `cancel`) now enforce one access policy
+  before touching records or providers: an authenticated submitter may act
+  only on their own submission, and an anonymous public-form submitter needs
+  a short-lived (`FORMS_WORKFLOWS_PAYMENT_CHECKOUT_TTL`, default 1 hour),
+  unguessable session-bound checkout credential issued when the form post
+  enters `pending_payment`. Guessing a numeric submission or payment-record
+  ID now yields a non-disclosing 404. Payment initiation is idempotent per
+  attempt — repeated GETs reuse the single open `PaymentRecord` and its
+  stable idempotency key (now forwarded to Stripe's `PaymentIntent.create`)
+  instead of creating uncontrolled attempts; a legitimate retry after
+  cancel/failure starts a fresh key. Cancellation is a CSRF-protected
+  `POST`-only action restricted to `pending_payment` submissions and can no
+  longer revert a completed/submitted/approved submission to `draft`.
+  Stripe webhooks now **fail closed**: events are rejected (HTTP 400) when
+  `STRIPE_WEBHOOK_SECRET` is unset or the signature is invalid, and the
+  provider interface requires the same authenticated-event guarantee from
+  all providers via `WebhookVerificationError`. Replayed or out-of-order
+  events no longer roll payment state backward (`completed`/`refunded` are
+  terminal; only `completed → refunded` is allowed) and duplicate deliveries
+  create no duplicate workflow tasks. `payment_collect.html` and
+  `payment_error.html` cancel actions are now POST forms.
+
 - **`social-auth-core` 4.8.3 → 5.1.1 in `poetry.lock`.** Closes five Dependabot
   advisories against the locked version: GHSA-vqg6-3fw6-j9jg (session fixation),
   GHSA-3c93-f73f-qc9h (VK App backend accepted unsigned callback data),

django_forms_workflows/payments/__init__.py

@@ -12,7 +12,13 @@
 without modifying the library itself.
 """
 
-from .base import PaymentFlow, PaymentProvider, PaymentResult, PaymentStatus
+from .base import (
+    PaymentFlow,
+    PaymentProvider,
+    PaymentResult,
+    PaymentStatus,
+    WebhookVerificationError,
+)
 from .registry import (
     clear,
     get_available_providers,
@@ -27,6 +33,7 @@
     "PaymentProvider",
     "PaymentResult",
     "PaymentStatus",
+    "WebhookVerificationError",
     "clear",
     "get_available_providers",
     "get_provider",

django_forms_workflows/payments/base.py

@@ -47,6 +47,17 @@ class PaymentStatus(Enum):
     REFUNDED = "refunded"
 
 
+class WebhookVerificationError(Exception):
+    """
+    Raised by a provider's ``handle_webhook()`` when the event payload
+    cannot be authenticated (e.g. a missing/invalid signature, or a
+    required verification secret that is not configured).
+
+    Providers must raise this instead of processing unverified payloads:
+    the webhook endpoint fails closed and discards the event.
+    """
+
+
 @dataclass
 class PaymentResult:
     """Returned by create_payment() and confirm_payment()."""
@@ -93,12 +104,19 @@ def create_payment(
         submission_id: int,
         description: str = "",
         metadata: dict[str, Any] | None = None,
+        idempotency_key: str | None = None,
     ) -> PaymentResult:
         """
         Initiate a payment.
 
         For INLINE providers: creates a payment intent, returns client_secret.
         For REDIRECT providers: creates a session, returns redirect_url.
+
+        ``idempotency_key`` is stable for a single payment attempt (one
+        ``PaymentRecord``).  Providers that support request idempotency
+        should forward it so a retried initiation cannot create duplicate
+        provider-side charges.  A new attempt (after a cancellation or
+        failure) is issued a fresh key.
         """
 
     @abstractmethod
@@ -121,6 +139,12 @@ def handle_webhook(self, request: Any) -> PaymentResult | None:
 
         Override if the provider sends server-to-server callbacks.
         Returns None if the event is not relevant.
+
+        Implementations MUST authenticate the event before returning a
+        result (e.g. verify a signature or shared secret) and raise
+        :class:`WebhookVerificationError` when verification fails or the
+        required verification secret is not configured.  Unverified
+        payloads must never produce a state-changing result.
         """
         return None
 

django_forms_workflows/payments/checkout.py

@@ -0,0 +1,84 @@
+"""
+Checkout authorization for the payment flow.
+
+Payment endpoints act on a :class:`~django_forms_workflows.models.FormSubmission`
+that is in ``pending_payment``.  Access is governed by exactly one policy:
+
+* **Owned submissions** — only the authenticated submitter may act on them.
+  A numeric submission or payment-record ID alone grants nothing.
+* **Anonymous (public-form) submissions** — the browser that just posted the
+  public form is issued a *checkout credential*: an unguessable token stored
+  in its session and bound to the submission ID, valid for a short time
+  (``FORMS_WORKFLOWS_PAYMENT_CHECKOUT_TTL`` seconds, default one hour).
+
+The credential is granted by the submit views when a submission enters
+``pending_payment`` and is revoked when the checkout is cancelled.  It is
+never exposed in a URL.  Callers that fail the policy receive a
+non-disclosing 404, identical to a nonexistent submission.
+"""
+
+from __future__ import annotations
+
+import secrets
+import time
+
+from django.conf import settings
+
+SESSION_KEY = "payment_checkouts"
+DEFAULT_CHECKOUT_TTL_SECONDS = 3600  # 1 hour
+
+
+def _checkout_ttl() -> int:
+    return int(
+        getattr(
+            settings,
+            "FORMS_WORKFLOWS_PAYMENT_CHECKOUT_TTL",
+            DEFAULT_CHECKOUT_TTL_SECONDS,
+        )
+    )
+
+
+def grant_checkout_access(request, submission) -> None:
+    """Bind a short-lived checkout credential for *submission* to the session.
+
+    Called by the submit views right after a submission enters
+    ``pending_payment`` so the same browser session can complete (confirm,
+    return, or cancel) the checkout that belongs to it.
+    """
+    checkouts = dict(request.session.get(SESSION_KEY, {}))
+    checkouts[str(submission.id)] = {
+        "token": secrets.token_urlsafe(32),
+        "exp": time.time() + _checkout_ttl(),
+    }
+    request.session[SESSION_KEY] = checkouts
+    request.session.modified = True
+
+
+def revoke_checkout_access(request, submission) -> None:
+    """Drop any checkout credential for *submission* from the session."""
+    checkouts = dict(request.session.get(SESSION_KEY, {}))
+    if checkouts.pop(str(submission.id), None) is not None:
+        request.session[SESSION_KEY] = checkouts
+        request.session.modified = True
+
+
+def has_checkout_access(request, submission) -> bool:
+    """The single payment access policy used by every payment endpoint.
+
+    * Authenticated submitter: may act only on their own submission.
+    * Anonymous submission (``submitter`` is null): the caller's session
+      must hold an unexpired checkout credential issued by
+      :func:`grant_checkout_access`.
+    """
+    if submission.submitter_id is not None:
+        return (
+            request.user.is_authenticated and request.user.pk == submission.submitter_id
+        )
+
+    entry = request.session.get(SESSION_KEY, {}).get(str(submission.id))
+    if not entry:
+        return False
+    try:
+        return float(entry.get("exp", 0)) > time.time()
+    except (TypeError, ValueError):
+        return False

django_forms_workflows/payments/stripe_provider.py

@@ -9,19 +9,27 @@
 
     STRIPE_SECRET_KEY = "sk_test_..."
     STRIPE_PUBLISHABLE_KEY = "pk_test_..."
-    STRIPE_WEBHOOK_SECRET = "whsec_..."   # optional, for webhook verification
+    STRIPE_WEBHOOK_SECRET = "whsec_..."   # REQUIRED to accept webhooks
+
+Webhooks fail closed: when ``STRIPE_WEBHOOK_SECRET`` is absent or the
+signature does not verify, the event is rejected and no state is applied.
 """
 
 from __future__ import annotations
 
-import json
 import logging
 from decimal import Decimal
 from typing import Any
 
 from django.conf import settings
 
-from .base import PaymentFlow, PaymentProvider, PaymentResult, PaymentStatus
+from .base import (
+    PaymentFlow,
+    PaymentProvider,
+    PaymentResult,
+    PaymentStatus,
+    WebhookVerificationError,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -59,19 +67,28 @@ def create_payment(
         submission_id: int,
         description: str = "",
         metadata: dict[str, Any] | None = None,
+        idempotency_key: str | None = None,
     ) -> PaymentResult:
         stripe = _get_stripe()
         stripe.api_key = settings.STRIPE_SECRET_KEY
 
         amount_cents = int(amount * 100)
 
+        # Forward the stable per-attempt idempotency key so a retried
+        # initiation of the same PaymentRecord cannot create duplicate
+        # PaymentIntents on Stripe's side.
+        request_options: dict[str, Any] = {}
+        if idempotency_key:
+            request_options["idempotency_key"] = idempotency_key
+
         try:
             intent = stripe.PaymentIntent.create(
                 amount=amount_cents,
                 currency=currency.lower(),
                 description=description or f"Form submission #{submission_id}",
                 metadata=metadata or {},
                 automatic_payment_methods={"enabled": True},
+                **request_options,
             )
             return PaymentResult(
                 success=True,
@@ -131,19 +148,27 @@ def handle_webhook(self, request: Any) -> PaymentResult | None:
         stripe = _get_stripe()
         webhook_secret = getattr(settings, "STRIPE_WEBHOOK_SECRET", "")
 
+        if not webhook_secret:
+            # Fail closed: without the webhook secret we cannot prove the
+            # event came from Stripe, so no state change may be applied.
+            logger.error(
+                "Stripe webhook received but STRIPE_WEBHOOK_SECRET is not "
+                "configured; refusing to process the event."
+            )
+            raise WebhookVerificationError(
+                "STRIPE_WEBHOOK_SECRET is not configured; cannot verify event."
+            )
+
         payload = request.body
         sig_header = request.META.get("HTTP_STRIPE_SIGNATURE", "")
 
-        if webhook_secret:
-            try:
-                event = stripe.Webhook.construct_event(
-                    payload, sig_header, webhook_secret
-                )
-            except (ValueError, stripe.error.SignatureVerificationError) as e:
-                logger.warning("Stripe webhook verification failed: %s", e)
-                return None
-        else:
-            event = stripe.Event.construct_from(json.loads(payload), stripe.api_key)
+        try:
+            event = stripe.Webhook.construct_event(payload, sig_header, webhook_secret)
+        except (ValueError, stripe.error.SignatureVerificationError) as e:
+            logger.warning("Stripe webhook signature verification failed: %s", e)
+            raise WebhookVerificationError(
+                "Stripe webhook signature verification failed."
+            ) from e
 
         if event.type == "payment_intent.succeeded":
             intent = event.data.object