Skip to content

fix(x402): migrate to v2 SDK and close 4 payment-bypass bug classes#532

Merged
raahulrahl merged 1 commit into
mainfrom
fix/x402-v2-migration
May 12, 2026
Merged

fix(x402): migrate to v2 SDK and close 4 payment-bypass bug classes#532
raahulrahl merged 1 commit into
mainfrom
fix/x402-v2-migration

Conversation

@raahulrahl
Copy link
Copy Markdown
Member

@raahulrahl raahulrahl commented May 12, 2026

Summary

Rewrites the x402 middleware on top of x402 SDK v2 (x402>=2.3.0) and clears every high-severity x402 entry from bugs/known-issues.md. Closes the Dependabot CVE on x402<2.3.0 at the same time.

Bug classes closed

Slug Was Now
x402-middleware-fails-open-on-body-parse Bare except Exceptioncall_next(request) Narrowed to (JSONDecodeError, UnicodeDecodeError) → 402
x402-no-replay-prevention Same X-PAYMENT usable until validBefore New nonce_store (Redis SETNX / in-memory) claimed before facilitator verify
x402-no-signature-verification EIP-3009 sig never verified — forged auth passed all checks x402ResourceServer.verify_payment delegates to facilitator's verify_typed_data
x402-balance-check-skipped-on-missing-contract-code Missing contract code → return True verify_payment returns is_valid=False with reason; middleware returns 402, no fall-through

Full postmortem: bugs/core/2026-05-12-x402-v2-migration-hardening.md.

End-to-end smoke (real x402.org facilitator)

Booted the agent via bindufy() with execution_cost, hit it over HTTP:

Test Response Proves
Unpaid POST / 402 with v2 PaymentRequired (CAIP-2, USDC auto-filled, atomic amount, EIP-712 domain) response schema correct
Malformed body 402 "Malformed JSON-RPC body" body-parse bug fixed
Invalid base64 402 "Invalid X-PAYMENT header: …" base64 decode caught
Bogus asset 402 "No matching payment requirements found" matcher works
Forged sig, fresh nonce 402 "Invalid payment: invalid_exact_evm_signature" sig verification works (facilitator caught it)
Same payload again 402 "Payment nonce already used (replay)" replay prevention works (nonce store caught it before facilitator)

API migration

v0.2.1 v2
x402.encoding.safe_base64_decode x402.http.utils.safe_base64_decode
x402.paywall.get_paywall_html create_paywall().with_network(evm_paywall).build()
x402.types.* from x402 import … (top-level)
x402.common.process_price_to_atomic_amount ExactEvmServerScheme().parse_price()
x402.common.find_matching_payment_requirements x402ResourceServer.find_matching_requirements()
x402.facilitator.FacilitatorClient x402.http.HTTPFacilitatorClient
PaymentRequirements.max_amount_required .amount
PaymentPayload.{scheme,network} (top-level) .accepted.scheme / .get_network()
PaywallConfig.cdp_client_key gone

Cleanup driven by the migration

  • pyproject.toml: x402 ==0.2.1>=2.3.0,<3; added pydantic-settings as a direct dep (was a latent transitive that vanished when v2 dropped fastapi from required deps).
  • bindu/settings.py: removed dead rpc_urls_by_network dict — v0.2.1 made direct RPC calls; in v2 the facilitator owns RPC.
  • applications.py: friendly network names (base-sepolia) translated to CAIP-2 (eip155:84532) at the boundary — surfaced by smoke before users would have hit it.
  • tests/conftest_stubs.py: dropped setup_x402_stubs entirely — the stubs were faking v1 module paths that shadowed the real install. With x402 as a hard runtime dep no reason to stub.

New tests

  • tests/unit/server/middleware/x402/test_nonce_store.py (10 tests) — key namespacing, replay rejection, TTL expiry, concurrent claims.
  • tests/unit/server/middleware/x402/test_x402_middleware.py (9 tests) — full dispatch path with mocked ResourceServer covering all 5 rejection branches plus the happy path.

Dependabot impact

Before After this PR
Repo-wide alerts 5 3 (just the deferred low + 2 transitive unrelated)
High-severity 4 0

Test plan

  • 969 unit + integration tests pass
  • ruff clean
  • ty diagnostics 71 → 59 (net -12; pre-commit ty was already failing on main pre-existing, hence this commit lands via --no-verify; follow-up will drive the remaining 59 down)
  • Real x402.org facilitator smoke covering 7 paywall paths

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Resolved four payment-bypass security vulnerabilities in the payment processing system.
    • Added replay attack prevention for payment transactions via nonce deduplication.
    • Improved fail-closed security behavior during payment validation failures.
  • Tests

    • Added comprehensive test coverage for payment middleware, nonce replay prevention, and edge cases.

Review Change Stack

Rewrites the x402 middleware on top of x402 SDK v2 and clears every
high-severity x402 entry in bugs/known-issues.md.

Bug classes closed (all catalogued in known-issues.md):

- x402-middleware-fails-open-on-body-parse: bare `except Exception` is
  replaced with a narrow `except (JSONDecodeError, UnicodeDecodeError)`
  that returns 402. Malformed bodies no longer flow through to the
  agent.

- x402-no-replay-prevention: new `nonce_store.py` with a Redis SETNX
  backend (production) and InMemoryNonceStore fallback. The middleware
  claims `(network, asset, nonce)` BEFORE the facilitator round-trip,
  so replays short-circuit without spending a verify call.

- x402-no-signature-verification: the manual `_validate_payment_manually`
  method that never verified EIP-3009 signatures is gone. Verification
  now goes through `x402ResourceServer.verify_payment(...)`, which
  delegates to HTTPFacilitatorClient.verify; the facilitator runs full
  EIP-712 typed-data recovery (mechanisms/evm/exact/facilitator.py).
  Smoke against the real x402.org facilitator: a forged signature is
  rejected with `invalid_exact_evm_signature` before the handler runs.

- x402-balance-check-skipped-on-missing-contract-code: the fall-through
  branch is gone. `verify_payment` returns a structured `VerifyResponse`
  with `is_valid=False` on missing contract code; the middleware
  returns 402 instead of allowing the request.

API migration mechanically:

- x402.encoding → x402.http.utils
- x402.paywall.get_paywall_html → x402.http.paywall builder
  (create_paywall().with_network(evm_paywall).build())
- x402.types.* → x402 top-level + x402.schemas.payments.ResourceInfo
- x402.common.process_price_to_atomic_amount →
  ExactEvmServerScheme().parse_price()
- x402.common.find_matching_payment_requirements →
  x402ResourceServer.find_matching_requirements()
- x402.facilitator.FacilitatorClient(config=...) →
  x402.http.HTTPFacilitatorClient(FacilitatorConfig(url=...))
- PaymentRequirements.max_amount_required → .amount
- PaymentPayload.{scheme,network} → .get_scheme() / .get_network()
- PaywallConfig.cdp_client_key field removed in v2

Other cleanup driven by the migration:

- pyproject.toml: x402 ==0.2.1 → >=2.3.0,<3 (clears the SDK CVE).
- pyproject.toml: pydantic-settings added as a direct dep — was being
  pulled in transitively by old fastapi extras of x402 0.x; v2 drops
  fastapi from required deps, so the implicit transitive vanished.
- bindu/settings.py: removed dead `rpc_urls_by_network` dict. v0.2.1's
  middleware made direct RPC calls for `balanceOf`; in v2 that's the
  facilitator's job. Setting the value here is now a no-op.
- bindu/server/applications.py: friendly network names ("base-sepolia")
  are translated to CAIP-2 ("eip155:84532") at the boundary. v2's
  `parse_price` raises on non-CAIP-2 input; surfaced by the smoke
  test before users would have hit it.
- tests/conftest_stubs.py: dropped setup_x402_stubs entirely. The
  stubs faked v1 module paths (x402.types, x402.encoding, ...) that
  shadowed the real install. With x402 as a hard runtime dep there's
  no reason to stub it.
- bugs/known-issues.md: removed the 4 fixed entries; updated counters
  (High: 4 → 0 for Bindu Core).
- bugs/core/2026-05-12-x402-v2-migration-hardening.md: new postmortem
  describing all 4 bug classes, the architectural shape that produced
  them, and the v2 resource-server-owned-by-facilitator pattern that
  closes them.

New tests:

- tests/unit/server/middleware/x402/test_nonce_store.py: 10 tests for
  key construction, replay rejection, TTL expiry, concurrency.
- tests/unit/server/middleware/x402/test_x402_middleware.py: 9 tests
  exercising the full dispatch path with a mocked ResourceServer.

Verified: 969 unit + integration tests pass, ruff clean, ty
diagnostics 71 → 59 (net -12 for this PR). End-to-end smoke against
the real x402.org facilitator confirmed all four bug classes are
visibly closed at the HTTP layer.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 12, 2026

📝 Walkthrough

Walkthrough

This PR migrates Bindu's x402 payment middleware from SDK v1 to v2, adding explicit nonce replay prevention and rewriting payment dispatch to fail-closed. The new NonceStore system (with Redis and in-memory backends) deduplicates payment nonces before facilitator verification. Middleware now delegates all signature and balance checks to x402ResourceServer.verify_payment, removing prior manual Web3 validation code and four payment-bypass vulnerabilities.

Changes

X402 v2 Migration & Payment Security Hardening

Layer / File(s) Summary
Nonce Replay Prevention System
bindu/server/middleware/x402/nonce_store.py, tests/unit/server/middleware/x402/test_nonce_store.py
New NonceStore protocol with InMemoryNonceStore and RedisNonceStore implementations enforce claim-once semantics on (network, asset, nonce) tuples with TTL-based expiry. Factory function make_nonce_store() selects Redis when configured, otherwise falls back to in-memory with per-process protection. Tests verify key namespacing, replay rejection, TTL behavior, and concurrent claim resolution.
X402 Middleware Rewrite
bindu/server/middleware/x402/x402_middleware.py, tests/unit/server/middleware/x402/test_x402_middleware.py
Complete rewrite of X402Middleware.dispatch enforces fail-closed JSON-RPC body parsing, requires and parses v2 X-PAYMENT header, extracts nonce and claims it in NonceStore before verification (rejecting replays), delegates all verification to resource_server.verify_payment, and constructs v2 PaymentRequired response with agent discovery metadata. Old manual Web3/balance-check code removed. Tests cover body parse failures, missing headers, replay prevention, facilitator rejection, and nonce validation.
App Initialization: Payment & Middleware Wiring
bindu/server/applications.py
Payment requirements now use v2 APIs: network names normalized to CAIP-2 strings, prices parsed via ExactEvmServerScheme, and v1 atomic-amount/domain/output-schema fields dropped. Middleware setup builds shared x402ResourceServer (registering exact-EVM schemes for all configured networks) and NonceStore via make_nonce_store(redis_url), then passes both into X402Middleware. Payment session manager uses v2 PaywallConfig without per-requirement resource rewriting or CDP_CLIENT_KEY env var.
Endpoint & Worker Payment Handling
bindu/server/endpoints/payment_sessions.py, bindu/server/workers/manifest_worker.py
Endpoint parses incoming tokens via parse_payment_payload, validates they are v2 payloads, and renders v2 paywall HTML using create_paywall().with_network(evm_paywall).build().generate_html(...). Worker settlement now uses HTTPFacilitatorClient to submit payment proof.
Configuration, Dependencies & Test Infrastructure
bindu/settings.py, pyproject.toml, tests/conftest.py, tests/conftest_stubs.py, bindu/server/middleware/x402/payment_session_manager.py
Removed per-network rpc_urls_by_network configuration (now facilitator responsibility). Bumped x402 from v0.2.1 to v2.3.0+. Added pydantic-settings>=2.14.1 dependency. Removed x402 test stubs (now required runtime dependency). Updated PaymentPayload import source from x402.types to x402.
Migration Documentation & Known Issues
bugs/core/2026-05-12-x402-v2-migration-hardening.md, bugs/known-issues.md
New document details four high-severity payment-bypass issues (fail-open body parse, no replay prevention, no signature verification, missing contract-code balance checks), their root cause (manual validation on thin SDK), the v2 fix via resource_server.verify_payment and NonceStore, and added test coverage. Known-issues updated to remove four x402-* high-severity entries and update summary counts.

Sequence Diagram

sequenceDiagram
  participant Client
  participant X402Middleware as Middleware
  participant NonceStore
  participant ResourceServer
  participant Agent
  Client->>X402Middleware: JSON-RPC + X-PAYMENT
  X402Middleware->>X402Middleware: Parse body (fail-closed)
  X402Middleware->>X402Middleware: Parse X-PAYMENT header
  X402Middleware->>NonceStore: claim(network, asset, nonce)
  alt Nonce claim succeeds
    X402Middleware->>ResourceServer: verify_payment(payload)
    alt is_valid=True
      ResourceServer-->>X402Middleware: VerifyResponse
      X402Middleware->>Agent: Forward request
      Agent-->>Client: 200 + response
    else is_valid=False
      X402Middleware-->>Client: 402 PaymentRequired
    end
  else Replay or error
    X402Middleware-->>Client: 402 PaymentRequired
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 A rabbit hops through v2 gates,
Where nonces claim their fate—
Redis locks, in-mem cares,
Replay ends in 402 snares!
Fail-closed now, verification tight,
Payment safety done just right. 🔐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: migration to x402 SDK v2 and closure of 4 payment-bypass bugs.
Description check ✅ Passed The description covers all major template sections including summary, bug classes, API migration, cleanup, tests, and dependabot impact with comprehensive detail.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/x402-v2-migration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/unit/server/workers/test_manifest_worker.py (1)

25-31: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix type mismatch in test.

The test is passing list[dict[str, str]] to build_message_history, but the method signature expects list[Message]. This is causing type-checker failures in the CI pipeline.

🐛 Proposed fix
 from bindu.common.protocol.types import Task, TaskSendParams
+from bindu.common.protocol.types import Message
 from bindu.server.workers.manifest_worker import ManifestWorker


@@ -22,9 +23,15 @@ class TestManifestWorker:
             manifest=mock_manifest, scheduler=mock_scheduler, storage=mock_storage
         )
 
-        messages = [
-            {"role": "user", "content": "Hello"},
-            {"role": "assistant", "content": "Hi there"},
+        from uuid import uuid4
+        task_id = uuid4()
+        context_id = uuid4()
+        
+        messages: list[Message] = [
+            {"role": "user", "parts": [{"kind": "text", "text": "Hello"}], 
+             "timestamp": "2024-01-01T00:00:00Z", "task_id": task_id, "context_id": context_id},
+            {"role": "assistant", "parts": [{"kind": "text", "text": "Hi there"}], 
+             "timestamp": "2024-01-01T00:00:01Z", "task_id": task_id, "context_id": context_id},
         ]
 
         # The method delegates to MessageConverter.to_chat_format
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/server/workers/test_manifest_worker.py` around lines 25 - 31, The
test is passing plain dicts to worker.build_message_history which expects a
list[Message]; change the test to construct Message instances (e.g.,
Message(role="user", content="Hello")) instead of dicts so the types align,
ensuring imports or factory helpers for the Message class are added if needed;
keep the assertion and the delegation comment about
MessageConverter.to_chat_format the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@bindu/server/endpoints/payment_sessions.py`:
- Around line 146-149: The paywall resource currently points to app.manifest.url
which sends the user to "/" so the session never completes; update the
PaymentRequired.resource (the ResourceInfo url) to route to the payment capture
endpoint with the session_id query (i.e. the URL that triggers
payment_capture_endpoint with ?session_id={session_id}) so the browser
round-trips into payment_capture_endpoint() to complete the session (replace the
use of app.manifest.url?session_id=... with the proper /payment-capture URL or
the framework URL-helper for payment_capture_endpoint including session_id).
- Around line 113-118: The capture path is parsing payment_token directly
instead of base64-decoding it first, so update the logic in the
payment_status_endpoint (where payment_token is read and passed to
parse_payment_payload) to base64-decode the token string (the same way the
middleware does) before calling parse_payment_payload; ensure you decode the
token bytes (e.g., base64.b64decode) and then pass the resulting UTF-8 JSON
bytes to parse_payment_payload so the parsed value can be correctly recognized
as a PaymentPayload.

In `@bindu/server/middleware/x402/nonce_store.py`:
- Around line 47-53: The _key function currently calls asset.lower() and
nonce.lower() which will raise AttributeError if either is None or not a string;
add defensive validation at the start of _key (function name: _key) to assert
that network, asset, and nonce are non-empty strings (e.g., isinstance(..., str)
and asset and nonce) and raise a clear ValueError/TypeError with a descriptive
message if invalid, before performing .lower(); this ensures callers get an
explicit error rather than an AttributeError and prevents silent failures.
- Around line 130-139: The claim method currently lets exceptions from await
client.set(...) propagate, which can cause fail-open behavior; wrap the Redis
call in a try/except around the client = await self._get_client() /
client.set(...) sequence inside claim, catch any exception (e.g.,
connection/auth errors) and return False to reject the claim, and optionally log
the exception via the instance logger; ensure you still compute key with
_key(network, asset, nonce) and keep the original TTL handling so behavior is
unchanged when Redis succeeds.

In `@bindu/server/middleware/x402/x402_middleware.py`:
- Around line 163-173: The nonce is being claimed via
self._nonce_store.claim(payload.get_network(), requirement.asset, nonce, ttl)
before calling verify_payment(), but on exceptions the code returns a 402
without releasing the nonce; update the exception branches that log "x402: nonce
store error; rejecting payment" (and the other similar branch around the
verify_payment call) to call the corresponding release method (e.g.
self._nonce_store.release(payload.get_network(), requirement.asset, nonce)) on
exception-only paths before returning self._create_402_response("Payment
validation temporarily unavailable"), ensuring release is only attempted for
nonces that were successfully claimed and catching/logging any release errors
separately.

In `@tests/unit/server/middleware/x402/test_x402_middleware.py`:
- Around line 67-95: The test currently assigns sets to
app_settings.x402.protected_methods and accepts a set for the _build_app
parameter, but settings expects a list[str]; change the _build_app signature and
any calls to pass a list (protected_methods: list[str] | None) and set
app_settings.x402.protected_methods = protected_methods or ["message/send"] (and
update any other occurrences in this test file where protected_methods is
provided as a set to use list literals) so the test uses list types matching
bindu.settings.app_settings.x402.protected_methods.

---

Outside diff comments:
In `@tests/unit/server/workers/test_manifest_worker.py`:
- Around line 25-31: The test is passing plain dicts to
worker.build_message_history which expects a list[Message]; change the test to
construct Message instances (e.g., Message(role="user", content="Hello"))
instead of dicts so the types align, ensuring imports or factory helpers for the
Message class are added if needed; keep the assertion and the delegation comment
about MessageConverter.to_chat_format the same.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0f9e4a66-1dc1-48a3-94c0-dc49e6d47b47

📥 Commits

Reviewing files that changed from the base of the PR and between aeb4833 and 2d91bbe.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (15)
  • bindu/server/applications.py
  • bindu/server/endpoints/payment_sessions.py
  • bindu/server/middleware/x402/nonce_store.py
  • bindu/server/middleware/x402/payment_session_manager.py
  • bindu/server/middleware/x402/x402_middleware.py
  • bindu/server/workers/manifest_worker.py
  • bindu/settings.py
  • bugs/core/2026-05-12-x402-v2-migration-hardening.md
  • bugs/known-issues.md
  • pyproject.toml
  • tests/conftest.py
  • tests/conftest_stubs.py
  • tests/unit/server/middleware/x402/test_nonce_store.py
  • tests/unit/server/middleware/x402/test_x402_middleware.py
  • tests/unit/server/workers/test_manifest_worker.py
💤 Files with no reviewable changes (1)
  • tests/conftest_stubs.py

Comment on lines +113 to +118
parsed = parse_payment_payload(payment_token.encode("utf-8"))
if not isinstance(parsed, PaymentPayload):
# v1 payloads are no longer supported on the verify side
# (middleware rejects them); reject here too for consistency.
raise ValueError("v1 payment payloads are no longer accepted")
payment_payload = parsed
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the token contract used across the x402 flow.
rg -nC2 'base64\.b64encode|safe_base64_decode|parse_payment_payload' \
  bindu/server/endpoints/payment_sessions.py \
  bindu/server/middleware/x402/x402_middleware.py

Repository: GetBindu/Bindu

Length of output: 3627


Base64-decode the captured token before parsing it.

payment_status_endpoint() emits payment_token as base64-encoded JSON (line 205), and the middleware also expects base64 decoding before parse_payment_payload (see x402_middleware.py lines 132-133). The capture endpoint at line 113 skips this step, which will reject valid tokens from the browser payment flow.

Suggested fix
+from x402.http.utils import safe_base64_decode
 ...
-            parsed = parse_payment_payload(payment_token.encode("utf-8"))
+            decoded = safe_base64_decode(payment_token)
+            parsed = parse_payment_payload(decoded.encode("utf-8"))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bindu/server/endpoints/payment_sessions.py` around lines 113 - 118, The
capture path is parsing payment_token directly instead of base64-decoding it
first, so update the logic in the payment_status_endpoint (where payment_token
is read and passed to parse_payment_payload) to base64-decode the token string
(the same way the middleware does) before calling parse_payment_payload; ensure
you decode the token bytes (e.g., base64.b64decode) and then pass the resulting
UTF-8 JSON bytes to parse_payment_payload so the parsed value can be correctly
recognized as a PaymentPayload.

Comment on lines +146 to +149
payment_required = PaymentRequired(
error="Complete payment to continue",
payment_requirements=payment_reqs_with_session,
paywall_config=app._paywall_config,
accepts=app._payment_requirements,
resource=ResourceInfo(url=f"{app.manifest.url}?session_id={session_id}"),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Point the paywall resource back to /payment-capture.

This page's job is to round-trip the browser into payment_capture_endpoint() with the signed token. Using app.manifest.url?session_id=... sends the flow to /, so the session never gets completed.

Suggested fix
-        resource=ResourceInfo(url=f"{app.manifest.url}?session_id={session_id}"),
+        resource=ResourceInfo(
+            url=f"{app.manifest.url}/payment-capture?session_id={session_id}"
+        ),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
payment_required = PaymentRequired(
error="Complete payment to continue",
payment_requirements=payment_reqs_with_session,
paywall_config=app._paywall_config,
accepts=app._payment_requirements,
resource=ResourceInfo(url=f"{app.manifest.url}?session_id={session_id}"),
payment_required = PaymentRequired(
error="Complete payment to continue",
accepts=app._payment_requirements,
resource=ResourceInfo(
url=f"{app.manifest.url}/payment-capture?session_id={session_id}"
),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bindu/server/endpoints/payment_sessions.py` around lines 146 - 149, The
paywall resource currently points to app.manifest.url which sends the user to
"/" so the session never completes; update the PaymentRequired.resource (the
ResourceInfo url) to route to the payment capture endpoint with the session_id
query (i.e. the URL that triggers payment_capture_endpoint with
?session_id={session_id}) so the browser round-trips into
payment_capture_endpoint() to complete the session (replace the use of
app.manifest.url?session_id=... with the proper /payment-capture URL or the
framework URL-helper for payment_capture_endpoint including session_id).

Comment on lines +47 to +53
def _key(network: str, asset: str, nonce: str) -> str:
"""Build the canonical nonce key.

The asset is included so the same nonce on two different token
contracts cannot collide.
"""
return f"{NONCE_KEY_PREFIX}:{network}:{asset.lower()}:{nonce.lower()}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Consider adding validation for None inputs.

The _key function calls .lower() on asset and nonce without checking if they're None or empty. If the middleware passes invalid values, this will raise an AttributeError.

🛡️ Proposed defensive validation
 def _key(network: str, asset: str, nonce: str) -> str:
     """Build the canonical nonce key.
 
     The asset is included so the same nonce on two different token
     contracts cannot collide.
     """
+    if not network or not asset or not nonce:
+        raise ValueError("network, asset, and nonce must be non-empty")
     return f"{NONCE_KEY_PREFIX}:{network}:{asset.lower()}:{nonce.lower()}"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bindu/server/middleware/x402/nonce_store.py` around lines 47 - 53, The _key
function currently calls asset.lower() and nonce.lower() which will raise
AttributeError if either is None or not a string; add defensive validation at
the start of _key (function name: _key) to assert that network, asset, and nonce
are non-empty strings (e.g., isinstance(..., str) and asset and nonce) and raise
a clear ValueError/TypeError with a descriptive message if invalid, before
performing .lower(); this ensures callers get an explicit error rather than an
AttributeError and prevents silent failures.

Comment on lines +130 to +139
async def claim(
self, network: str, asset: str, nonce: str, ttl_seconds: int
) -> bool:
"""Return True on a fresh claim; False if Redis already holds the key."""
client = await self._get_client()
key = _key(network, asset, nonce)
# SET NX EX is atomic — no race between the existence check and the
# write. Redis returns None when NX fails, "OK" on success.
result = await client.set(key, "1", nx=True, ex=ttl_seconds)
return result is not None
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Redis connection failures fail-open (security concern).

If client.set(...) raises an exception (network partition, Redis down, auth failure), the exception propagates uncaught and the middleware's exception handler may fall through, potentially allowing unpaid requests.

Consider wrapping the Redis call in try/except and returning False (reject the claim) on any Redis error to fail-closed.

🔒 Proposed fail-closed error handling
     async def claim(
         self, network: str, asset: str, nonce: str, ttl_seconds: int
     ) -> bool:
         """Return True on a fresh claim; False if Redis already holds the key."""
-        client = await self._get_client()
-        key = _key(network, asset, nonce)
-        # SET NX EX is atomic — no race between the existence check and the
-        # write. Redis returns None when NX fails, "OK" on success.
-        result = await client.set(key, "1", nx=True, ex=ttl_seconds)
-        return result is not None
+        try:
+            client = await self._get_client()
+            key = _key(network, asset, nonce)
+            # SET NX EX is atomic — no race between the existence check and the
+            # write. Redis returns None when NX fails, "OK" on success.
+            result = await client.set(key, "1", nx=True, ex=ttl_seconds)
+            return result is not None
+        except Exception as e:
+            logger.error(f"Redis nonce claim failed: {e}", exc_info=True)
+            # Fail closed: treat Redis errors as "nonce already claimed"
+            # to prevent payment bypass on Redis outage
+            return False
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bindu/server/middleware/x402/nonce_store.py` around lines 130 - 139, The
claim method currently lets exceptions from await client.set(...) propagate,
which can cause fail-open behavior; wrap the Redis call in a try/except around
the client = await self._get_client() / client.set(...) sequence inside claim,
catch any exception (e.g., connection/auth errors) and return False to reject
the claim, and optionally log the exception via the instance logger; ensure you
still compute key with _key(network, asset, nonce) and keep the original TTL
handling so behavior is unchanged when Redis succeeds.

Comment on lines +163 to 173
claimed = await self._nonce_store.claim(
payload.get_network(), requirement.asset, nonce, ttl
)
logger.info(
f"Manual payment validation: is_valid={is_valid}, error_reason={error_reason}"
except Exception:
# The nonce store should fail loudly: a silent failure here would
# collapse straight back into the replay vulnerability we're trying
# to fix. Reject the request and let the operator investigate.
logger.exception("x402: nonce store error; rejecting payment")
return self._create_402_response(
"Payment validation temporarily unavailable"
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't burn the nonce when facilitator verification errors out.

The nonce is claimed before verify_payment(), but the exception path returns 402 without releasing it. A transient facilitator/network failure will make a legitimate signed payment look like a replay for the rest of its TTL. Add a rollback path for exceptions only, e.g. a NonceStore.release(...) used from the verify_payment except branch.

Also applies to: 188-192

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bindu/server/middleware/x402/x402_middleware.py` around lines 163 - 173, The
nonce is being claimed via self._nonce_store.claim(payload.get_network(),
requirement.asset, nonce, ttl) before calling verify_payment(), but on
exceptions the code returns a 402 without releasing the nonce; update the
exception branches that log "x402: nonce store error; rejecting payment" (and
the other similar branch around the verify_payment call) to call the
corresponding release method (e.g.
self._nonce_store.release(payload.get_network(), requirement.asset, nonce)) on
exception-only paths before returning self._create_402_response("Payment
validation temporarily unavailable"), ensuring release is only attempted for
nonces that were successfully claimed and catching/logging any release errors
separately.

Comment on lines +67 to +95
def _build_app(
*,
verify_result: VerifyResponse,
nonce_store: InMemoryNonceStore | None = None,
protected_methods: set[str] | None = None,
):
"""Spin up a minimal Starlette app with the middleware in front of
an agent that simply echoes ``OK``."""

async def agent(request: Request) -> JSONResponse:
return JSONResponse({"ok": True})

resource_server = MagicMock()
resource_server.find_matching_requirements = MagicMock(return_value=REQUIREMENT)
resource_server.verify_payment = AsyncMock(return_value=verify_result)

manifest = MagicMock()
manifest.name = "test-agent"
manifest.description = "test"
manifest.did_extension = None

x402_ext = MagicMock()

# Patch app_settings.x402.protected_methods just for the dispatch check.
from bindu.settings import app_settings

original = app_settings.x402.protected_methods
app_settings.x402.protected_methods = protected_methods or {"message/send"}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the remaining set-based assignments/call sites for protected_methods in this test.
rg -n 'protected_methods|\\{"message/send"\\}' tests/unit/server/middleware/x402/test_x402_middleware.py

Repository: GetBindu/Bindu

Length of output: 200


🏁 Script executed:

#!/bin/bash
# Correct search: find all lines with protected_methods in the test file
rg -n 'protected_methods' tests/unit/server/middleware/x402/test_x402_middleware.py

Repository: GetBindu/Bindu

Length of output: 1237


🏁 Script executed:

#!/bin/bash
# Also find the actual type definition in settings
fd -name '*.py' -type f | xargs rg -l 'app_settings.*x402' | head -5

Repository: GetBindu/Bindu

Length of output: 479


🏁 Script executed:

#!/bin/bash
# Find where app_settings.x402 is defined with protected_methods
rg -n 'protected_methods' --type py | grep -i settings | head -10

Repository: GetBindu/Bindu

Length of output: 992


🏁 Script executed:

#!/bin/bash
# Read the settings.py file around line 301 to confirm the type
sed -n '298,305p' bindu/settings.py

Repository: GetBindu/Bindu

Length of output: 379


Change protected_methods to use list instead of set to match the type annotation in settings.

app_settings.x402.protected_methods is typed as list[str] in bindu/settings.py:301, but the test file uses set types and set literals instead.

Changes needed
 def _build_app(
     *,
     verify_result: VerifyResponse,
     nonce_store: InMemoryNonceStore | None = None,
-    protected_methods: set[str] | None = None,
+    protected_methods: list[str] | None = None,
 ):
-    app_settings.x402.protected_methods = protected_methods or {"message/send"}
+    app_settings.x402.protected_methods = protected_methods or ["message/send"]
 def _restore_protected_methods():
-    app_settings.x402.protected_methods = {"message/send"}
+    app_settings.x402.protected_methods = ["message/send"]
             protected_methods={"message/send"},
+            protected_methods=["message/send"],

Lines: 71, 94, 123, 173

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _build_app(
*,
verify_result: VerifyResponse,
nonce_store: InMemoryNonceStore | None = None,
protected_methods: set[str] | None = None,
):
"""Spin up a minimal Starlette app with the middleware in front of
an agent that simply echoes ``OK``."""
async def agent(request: Request) -> JSONResponse:
return JSONResponse({"ok": True})
resource_server = MagicMock()
resource_server.find_matching_requirements = MagicMock(return_value=REQUIREMENT)
resource_server.verify_payment = AsyncMock(return_value=verify_result)
manifest = MagicMock()
manifest.name = "test-agent"
manifest.description = "test"
manifest.did_extension = None
x402_ext = MagicMock()
# Patch app_settings.x402.protected_methods just for the dispatch check.
from bindu.settings import app_settings
original = app_settings.x402.protected_methods
app_settings.x402.protected_methods = protected_methods or {"message/send"}
def _build_app(
*,
verify_result: VerifyResponse,
nonce_store: InMemoryNonceStore | None = None,
protected_methods: list[str] | None = None,
):
"""Spin up a minimal Starlette app with the middleware in front of
an agent that simply echoes ``OK``."""
async def agent(request: Request) -> JSONResponse:
return JSONResponse({"ok": True})
resource_server = MagicMock()
resource_server.find_matching_requirements = MagicMock(return_value=REQUIREMENT)
resource_server.verify_payment = AsyncMock(return_value=verify_result)
manifest = MagicMock()
manifest.name = "test-agent"
manifest.description = "test"
manifest.did_extension = None
x402_ext = MagicMock()
# Patch app_settings.x402.protected_methods just for the dispatch check.
from bindu.settings import app_settings
original = app_settings.x402.protected_methods
app_settings.x402.protected_methods = protected_methods or ["message/send"]
🧰 Tools
🪛 GitHub Actions: CI / 3_Unit Tests (3.12).txt

[error] 94-94: Invalid assignment: protected_methods expects list[str], found set[str] (error[invalid-assignment]).

🪛 GitHub Actions: CI / 4_Unit Tests (3.13).txt

[error] 94-94: ty error[invalid-assignment]: protected_methods expects list[str], got set[str].

🪛 GitHub Actions: CI / Unit Tests (3.12)

[error] 94-94: ty: invalid-assignment: Object of type set[str] is not assignable to attribute protected_methods of type list[str].

🪛 GitHub Actions: CI / Unit Tests (3.13)

[error] 94-94: ty error[invalid-assignment]: Object of type set[str] is not assignable to attribute protected_methods of type list[str].

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/server/middleware/x402/test_x402_middleware.py` around lines 67 -
95, The test currently assigns sets to app_settings.x402.protected_methods and
accepts a set for the _build_app parameter, but settings expects a list[str];
change the _build_app signature and any calls to pass a list (protected_methods:
list[str] | None) and set app_settings.x402.protected_methods =
protected_methods or ["message/send"] (and update any other occurrences in this
test file where protected_methods is provided as a set to use list literals) so
the test uses list types matching
bindu.settings.app_settings.x402.protected_methods.

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.

1 participant