Skip to content

fix: add cryptographic signature verification to validateAuth() - #5

Open
JackBinswitch-btc wants to merge 1 commit into
secret-mars:mainfrom
JackBinswitch-btc:fix/validate-auth-signature
Open

fix: add cryptographic signature verification to validateAuth()#5
JackBinswitch-btc wants to merge 1 commit into
secret-mars:mainfrom
JackBinswitch-btc:fix/validate-auth-signature

Conversation

@JackBinswitch-btc

Copy link
Copy Markdown

Summary

Fixes #4validateAuth() was checking signature format (base64 length) but never performing any cryptographic verification. Any caller could pass an arbitrary base64 string and successfully authenticate as any Bitcoin address.

What was wrong

// Before: only a length check — any ~88-char base64 string passes
if (typeof body.signature !== 'string' || body.signature.length < 80 || body.signature.length > 100) {
  return 'Invalid signature format ...';
}
// No actual verification performed — always returned null (valid)

What was fixed

  • Added src/crypto.ts with a pure-JS BIP-137 signature verifier (verifyBip137) using @noble/curves (secp256k1) and @noble/hashes (sha256, ripemd160). The library is lightweight, dependency-free, and Cloudflare Workers compatible.
  • The verifier handles all BIP-137 recovery flag ranges: uncompressed P2PKH, compressed P2PKH, P2SH-P2WPKH, and native SegWit P2WPKH (bc1q) addresses.
  • Updated validateAuth() in src/index.ts to call verifyBip137() with the canonical message string, recover the signer's Bitcoin address from the ECDSA signature, and reject any request where the recovered address does not match the claimed btc_address.
  • Added @noble/curves and @noble/hashes as runtime dependencies.

Verification

Build passes with no TypeScript errors (wrangler deploy --dry-run).

Test plan

  • Generate a valid BIP-137 signature for a known P2WPKH (bc1q) address signing x402-task | create_task | <addr> | <timestamp> and confirm the endpoint accepts it
  • Send the same request with a random base64 string as the signature and confirm it is now rejected with Signature verification failed
  • Send a valid signature from address A while claiming to be address B and confirm it is rejected
  • Confirm existing endpoints (GET /api/tasks, GET /api/stats) still work without auth

🤖 Generated with Claude Code

Previously, validateAuth() only checked that the signature field was a
base64 string of roughly the right length (80-100 chars). Any attacker
could pass an arbitrary base64 value and bypass all auth checks, since
the signature was never verified against the claimed Bitcoin address.

This commit:
- Adds src/crypto.ts with a pure-JS BIP-137 verifyBip137() function
  built on @noble/curves (secp256k1) and @noble/hashes (sha256, ripemd160).
  It handles all BIP-137 address types: P2PKH, compressed P2PKH,
  P2SH-P2WPKH, and native P2WPKH (bc1q) addresses.
- Updates validateAuth() in src/index.ts to call verifyBip137() with
  the canonical message format ("x402-task | {action} | {address} | {ts}"),
  recover the signer's Bitcoin address, and reject any request where the
  recovered address does not match the claimed address.
- Adds @noble/curves and @noble/hashes as runtime dependencies.

Fixes secret-mars#4

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@tfireubs-ui tfireubs-ui left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed the BIP-137 implementation in src/crypto.ts. Good use of @noble/curves and @noble/hashes — both run in Cloudflare Workers, message hash format is correct (double SHA256 with prefix), recovery ID handling is correct for all flag ranges.

One bug to note — P2SH-P2WPKH (flags 35-38):
For flags 35-38, pubkeyToP2wpkhAddress() derives a bc1q... address, but P2SH-P2WPKH wallets sign with a 3... address (their public-facing address). The recovered address comparison would always fail for those wallets. Fix: for p2sh-p2wpkh, derive the P2SH address by encoding hash160(OP_0 || hash160(pubkey)) with version byte 0x05.

This bug only affects P2SH-P2WPKH ("wrapped SegWit") wallets. Native SegWit (bc1q) and P2PKH wallets work correctly.

Replay protection: The timestamp window (300s) is the only replay guard. PR #6 (also open for this issue) adds a nonce in the signed message for stronger replay protection — worth considering here too.

vs PR #6: Both PRs solve the same issue. PR #6 adds a nonce (breaking API change) and uses pure-JS EC math; this PR uses @noble/curves (smaller, more maintainable). Maintainer should pick one — if choosing this PR, I'd suggest fixing the P2SH-P2WPKH case or narrowing the supported flag range to 31-34 (compressed P2PKH) and 39-42 (P2WPKH native SegWit), which covers the common cases.

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.

validateAuth() checks signature format but never verifies it cryptographically

2 participants