feat(onramp): gate /onramp/token behind a signed proof of address ownership - #316
feat(onramp): gate /onramp/token behind a signed proof of address ownership#316piyalbasu wants to merge 22 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rmissive/strict, {result,reason})
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ors backend-v2 internal/auth)
…reason Code-review L1: tighten recordOnrampAuth's reason param to OnrampAuthReason (was string) and replace the magic "ok" literal with ONRAMP_AUTH_REASON.OK, keeping the metric reason vocabulary aligned with backend-v2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fold a fixed ONRAMP_AUTH_DOMAIN tag into the signed bytes so an onramp proof cannot be forged via the public signMessage dApp API (cross-protocol signature confusion). Rejects any signature that does not cover the tag; regression test included. The client-side signMessage refusal (the load-bearing mitigation) lands with the client PRs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the cast-based request augmentation (review L2) with module augmentation + server.decorateRequest, so request.onrampPrincipal is a first-class typed field — no casts at the set/read sites. Behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per-principal limiting (onramp:rl:<sub>) is sybil-bypassable (fresh keypair → fresh sub) so it adds no real security over the pre-existing per-IP limit, at the cost of a Redis round-trip per request. Removed pending a decision on aggregate quota protection. The endpoint remains rate-limited per-IP via the existing @fastify/rate-limit config. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cc81eaa to
1e65c59
Compare
Sibling to freighter-backend-v2#112's design doc. Documents the SEP-53 proof wire contract and a dedicated cross-language porting section (canonicalization landmines, file mapping, and the caveat that v2's onramp verifier is a second SEP-53 scheme, not its existing JWT verifier). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-protocol phrasing
…h-gate # Conflicts: # src/helper/metrics.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3555b96acd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds a stateless, per-request proof-of-address-ownership gate to POST /api/v1/onramp/token, so Coinbase Onramp session tokens can only be minted for a Stellar address the caller can prove they control. The change is staged behind a rollout mode (permissive → strict) and includes verifier/middleware infrastructure plus metrics and tests.
Changes:
- Introduces SEP-53–style signed proof verification (
Authorization: Stellar <payload>.<sig>) and a Fastify preHandler that enforces permissive/strict rollout behavior. - Updates
/onramp/tokento derive the Coinbase destination from the provensubwhen signed, falling back to legacy bodyaddressonly for permissive+unsigned requests. - Adds config parsing, metrics instrumentation, and unit/integration tests plus a design doc describing the wire contract.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/route/index.ts | Adds onramp auth preHandler, request decoration, and uses proven principal as destination. |
| src/route/index.test.ts | Adds route-level tests for signed/permissive/strict behaviors and rejection cases. |
| src/index.ts | Plumbs onrampAuthMode into server initialization. |
| src/helper/test-helper.ts | Extends dev server helper to accept auth mode and adds a test proof minter. |
| src/helper/metrics.ts | Adds onramp auth result/reason counter and helper to record outcomes. |
| src/config.ts | Adds ONRAMP_AUTH_MODE parsing and returns onrampAuthMode in config. |
| src/config.test.ts | Adds tests for ONRAMP_AUTH_MODE parsing/defaulting/validation. |
| src/auth/verifier.ts | Implements canonical JSON, body hashing, SEP-53 digesting, and proof verification. |
| src/auth/verifier.test.ts | Adds verifier test matrix (missing/malformed/expired/mismatch/bad sig/etc). |
| src/auth/mode.ts | Defines AuthMode and parseMode (permissive default, strict supported). |
| src/auth/mode.test.ts | Tests parseMode behavior. |
| src/auth/middleware.ts | Adds Fastify preHandler implementing permissive/strict policy + metrics + principal stash. |
| src/auth/errors.ts | Defines reason taxonomy and verifier result type. |
| src/auth/context.ts | Fastify module augmentation for request.onrampPrincipal. |
| docs/design/2026-06-24-onramp-auth-proof.md | Documents the protocol, verification order, rollout semantics, and porting notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…d 401, honest principal log - config: ONRAMP_AUTH_MODE is no longer a required env var; an unset value defaults to "permissive" via parseMode(undefined). Previously listing it in ENV_KEYS threw INVALID_ENV at startup for any deployment that hadn't added the var, contradicting the documented default. (Codex P1 / Copilot) - verifier: guard non-object JSON payloads (null, array, bare scalar) before reading claim fields, so a header like `Stellar bnVsbA.<sig>` returns 401 MALFORMED instead of throwing -> 500 and skipping the rejected metric. (Codex P2) + regression test over null/[]/123. - route: success log now records `principal: principal ?? null` (proven only) instead of the caller-supplied address, which is unproven for unsigned permissive requests; `destination` keeps the effective address. (Copilot) - config test: omit ONRAMP_AUTH_MODE entirely to simulate a genuinely absent env var and assert startup does not throw + defaults to permissive. (Copilot) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JakeUrban
left a comment
There was a problem hiding this comment.
Just reviewed the design doc so far and found one design decision I think we should reconsider. Once we decide on how to proceed I'll give this PR another review!
| 4. Expiry window: reject if `now > exp + 2` (expired) or `exp > now + 15 + 2` (too far future) → | ||
| `401 expired`. (≤15s lifetime, 2s skew.) |
There was a problem hiding this comment.
Why 2s skew when the wallet-backend had a 5s skew?
There was a problem hiding this comment.
TL;DR: Good catch — no real reason for the divergence, I'll align it to 5s to match the sibling and update the doc. A tighter window only risks bouncing legitimate clients whose clocks drift, with no security upside.
Detailed explanation (for agents)
This PR sets a 2s skew in src/auth/verifier.ts (ONRAMP_PROOF_SKEW_S = 2), while the sibling #112 uses a 5s leeway in freighter-backend-v2/internal/auth/claims.go (MaxTokenLifetime = 15s, ClockSkewLeeway = 5s). The lifetime already matches at 15s; the skew was meant to mirror #112 and just drifted.
Fix: set ONRAMP_PROOF_SKEW_S = 5, and update the "≤15s lifetime, 2s skew" note in the design doc's verification-order section to match.
| 2. **Envelope** is a SEP-53 signed message, not a Bearer JWT. _Forced:_ the onramp clients include | ||
| **Ledger hardware wallets**, whose on-device signer produces SEP-53 message signatures, not | ||
| raw JWT-signing-input signatures — a pure EdDSA JWT can't be produced on a Ledger. So the format is | ||
| dictated by the shared clients, not the backend. |
There was a problem hiding this comment.
Yep — the Ledger Stellar app added SEP-53 message signing in v6.0.0. Release notes: https://github.com/LedgerHQ/app-stellar/blob/develop/release-notes.md
| ## Porting to freighter-backend-v2 (Go) | ||
|
|
||
| When v2 grows an onramp endpoint, this gate ports to Go. **The clients are shared and sign SEP-53, so | ||
| v2's onramp verifier MUST accept this exact SEP-53 proof — it cannot reuse #112's Bearer-JWT verifier. | ||
| v2 will have two auth schemes: JWT for user-id auth, and this SEP-53 onramp proof.** Reuse v2's | ||
| `Mode`, `context`, metric, and reason vocabulary; add a _new_ SEP-53 onramp verifier alongside the JWT one. |
There was a problem hiding this comment.
Can we avoid having v2 support two types of auth credentials? We could include a signed message in the POST body rather than an authorization header. That way, when this functionality is ported to v2, clients can use the same JWT auth they usually do with v2's API.
There was a problem hiding this comment.
TL;DR: Agreed — I'll move the signed proof into the POST body so Authorization stays free for v2's normal JWT, and v2 keeps a single auth scheme. There's actually a hard reason this is the right call, not just a preference (below). I'll rework the doc + PR and re-request review.
Detailed explanation (for agents)
Why the body (not just a nice-to-have): in v2 an authenticated onramp request needs both the JWT (identity) and the address-ownership proof simultaneously — and a single Authorization header can't carry both a Bearer JWT and a Stellar proof. So the proof has to leave Authorization regardless of preference. These are two distinct concerns — who you are (JWT) vs. which address you control (the proof) — and the doc's "v2 will have two auth schemes" framing conflated them into competing credentials.
Plan:
- Move the proof out of the
Authorization: Stellar …header and into a POST body field. - Rework
body_hashto cover the business payload excluding the proof field (standard sign-everything-but-the-signature pattern), and spell that carve-out out in the wire contract so it stays reproducible byte-for-byte. - Rewrite the porting section: v2 keeps JWT-in-
Authorizationfor authN and validates the in-body proof for address ownership — no secondAuthorizationscheme.
…oof in body; 5s skew Addresses Jake's design-doc review on #316: - Generalize the src/auth primitive from onramp-specific to a reusable "address proof": verifyAddressProof, ADDRESS_PROOF_DOMAIN ("freighter:address-proof:v1"), reasons, and result type. The domain tag is named generically on purpose — it lives in the signed bytes, so a later rename would be a wire-breaking v2. Onramp is now the first consumer; the rollout policy (onrampAuthPreHandler, ONRAMP_AUTH_MODE, the onramp metric) stays onramp-scoped. - Move the proof out of the Authorization header into the `address_proof` POST-body field, so it composes with v2's JWT (which occupies Authorization) and v2 needs no second auth scheme. body_hash now covers the business body with the proof field removed (sign-everything-but-the-signature). - Bump clock skew 2s -> 5s to match freighter-backend-v2 #112 (ClockSkewLeeway). tsc clean; jest green (auth + route + config suites). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
TL;DR: @JakeUrban pushed What changed, per comment (for reviewers)1. Proof → request body (your main design point). The signed proof moved out of the 2. Skew 2s → 5s. Aligned to #112's 3. Ledger/SEP-53. Confirmed in the thread above — the Ledger Stellar app supports SEP-53 message signing natively (app ≥ v6.0.0, Also (not asked, but fell out of #1): the Verification: Follow-up: the client PR ( |
| additionalProperties: false, | ||
| }, | ||
| }, | ||
| preHandler: onrampAuthPreHandler({ mode: onrampAuthMode }), |
There was a problem hiding this comment.
False positive, same as the two earlier CodeQL missing-rate-limiting hits on this route (re-fired on the new commit). /onramp/token is rate-limited on two dimensions: per-IP via @fastify/rate-limit's per-route override — config.rateLimit: { max: 100, timeWindow: "1 minute" } — layered on the global limiter registered at server init. CodeQL doesn't model Fastify's route-level config.rateLimit. Dismissing as won't-fix.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bc1194d87
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| : {}; | ||
| const { [ADDRESS_PROOF_BODY_FIELD]: proofField, ...businessBody } = bodyObj; | ||
|
|
||
| if (!proofField || typeof proofField !== "string") { |
There was a problem hiding this comment.
Treat empty address proofs as malformed
In permissive mode, a request that includes address_proof: "" alongside a legacy address hits this branch as NO_TOKEN, so the middleware records it as anonymous and mints a token instead of rejecting it as a present-but-invalid proof. That breaks the rollout rule documented in the preHandler comments/tests that invalid supplied proofs must fail in both modes, and can mask broken signed clients or a cleared proof field during adoption; only an absent field should be classified as NO_TOKEN.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b5cf0c8 — the verifier now distinguishes an absent proof field (NO_TOKEN, anonymous-eligible in permissive) from a field that is present but unusable (empty string or non-string → MALFORMED, 401 in both modes), via an explicit in presence check. So address_proof: "" alongside a legacy address is now rejected rather than minting a token, restoring the present-but-invalid → 401 invariant. Added regression tests for the empty and non-string cases.
freighter-backend/src/auth/verifier.ts
Lines 74 to 98 in b5cf0c8
Codex P2 on #316: an `address_proof: ""` (present but empty) fell into the NO_TOKEN branch, so permissive mode treated it as anonymous and minted a token instead of rejecting a present-but-invalid proof — violating the "present but invalid -> 401 in both modes" rollout invariant. Distinguish absent (NO_TOKEN, anonymous-eligible) from present-but-unusable (empty string / non-string -> MALFORMED, 401 in both modes) via an explicit `in` presence check. Adds regression tests for empty and non-string proof fields. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@piyalbasu what is the status of this PR? |
TL;DR
Adds per-request authentication to
POST /api/v1/onramp/token. The caller must now present a short-lived signed proof that they control the Stellar address the Coinbase Onramp session is being minted for, so a session token can only be minted for an address the caller proves they control. Verification is stateless — no accounts, sessions, database lookups, or server-side secrets. This is an authentication building block.This is the JavaScript sibling of the Go auth work in stellar/freighter-backend-v2#112 — the same stateless Ed25519 proof-of-ownership model, one written in Go, one in TypeScript.
Rollout is staged behind a config switch,
ONRAMP_AUTH_MODE(defaultpermissive), which accepts both signed proofs and existing unsigned requests — so the current "Add funds" flow is unaffected. We flip tostrictlater, once the signing clients have shipped. This PR is backend-only; the extension and mobile signing changes land in separate PRs, so nothing is enforced on real clients yet.Closes: the Add Coinbase Onramp token auth gate work item — stellar/projects/58.
Implementation details (for agents)
Design doc:
docs/design/2026-06-24-onramp-auth-proof.md— sibling to #112's design doc; includes the wire contract and a cross-language porting reference for the eventual freighter-backend-v2 (Go) port.Identity model (the deliberate differences from #112). Same stateless Ed25519 proof-of-ownership as the Go PR, with two intended divergences:
G…strkey address the token will fund — it proves address ownership. In Bump the minor-and-patch group with 16 updates #112 thesubis a hex Ed25519 user ID — it proves user-id ownership.Authorization: Stellar <base64url(payload)>.<base64url(sig)>, rather than an RFC-6750 Bearer JWT. That's forced: the onramp clients include Ledger hardware wallets, whose on-device signer produces SEP-53 message signatures, not raw JWT-signing-input signatures — so a pure EdDSA JWT can't be produced on a Ledger.Everything else (rollout modes, metric shape, reason taxonomy, package layout) mirrors #112 so the two read as siblings.
What changed, by area. The new
src/auth/package mirrors v2'sinternal/auth/:src/auth/verifier.ts— canonical-JSON + SEP-53 encode +verifyOnrampProof: parses theStellar-scheme header, validatesexp(≤15s lifetime, 2s skew), bindsmethod+path+body_hash, validates the StrKey (→400before any Coinbase call), and verifies the Ed25519 signature against the provensub. The signed bytes are prefixed with a fixedONRAMP_AUTH_DOMAINtag (freighter:onramp-auth:v1) — see the cross-protocol note in Follow-ups.src/auth/mode.ts—AuthMode+parseMode(permissive/strict; empty→permissive; unknown→throws at startup). Mirrorsmode.go.src/auth/errors.ts—ONRAMP_AUTH_REASONtaxonomy + result type. Mirrorserrors.go.src/auth/middleware.ts—onrampAuthPreHandler({ mode })factory (mirrorsmiddleware/auth.goAuth(...)). Truth table: permissive (no proof → anonymous/legacy path; present-but-invalid → 401) vs strict (missing/invalid → 401). On success the proven principal is stashed on the request.src/auth/context.ts— Fastify module augmentation +decorateRequestforrequest.onrampPrincipal(mirrorscontext.go).src/route/index.ts— the/onramp/tokenroute: the Coinbase destination is now derived from the verifiedsubinstead of a caller-supplied body field, and the route uses the preHandler factory. The existing per-IP rate limit,clientIpforwarding +FREIGHTER_TRUST_PROXY_RANGEinternal-IP guard are retained.src/config.ts—ONRAMP_AUTH_MODE(defaultpermissive), resolved viaparseModeand validated at startup.src/helper/metrics.ts—freighter_backend_onramp_auth_requests_total{result,reason}(result∈authenticated|anonymous|rejected) for rollout-adoption signal; sibling to v2'sfreighter_auth_requests_total{result,reason}.Differences from #112 worth a reviewer's eye:
sub= Stellar strkey (address ownership) vs hex Ed25519 (user-id ownership).Rollout semantics.
permissiveaccepts both signed and legacy-unsigned requests, so un-updated clients keep working; a present-but-invalid proof is rejected (401) in both modes. Flip tostrictonce the signing clients have rolled out and the{result="anonymous"}series falls to ~0. A runbook update for the auth-gate reason taxonomy +ONRAMP_AUTH_MODEis staged inwallet-eng-runbooks(separate review).Verification.
npx tsc --noEmitclean; fulljestsuite green (167 passed / 3 pre-existing skips), including the verifier failure matrix (missing / malformed / expired / wrong-key / tampered-body / invalid-StrKey) and permissive & strict route behavior. A code review (8 finder angles + verification) and a crypto/backend security review were run and their findings addressed.Cross-protocol hardening (security). The onramp proof and the clients' public
signMessagedApp API use the same SEP-53 signing primitive, so the signed content is domain-tagged withONRAMP_AUTH_DOMAIN(and this verifier rejects any signature that doesn't cover the tag) to keep the two signing domains separate. The clientsignMessagepath also refuses to sign tagged messages, so onramp proofs originate only from the internal onramp signer — that part ships with the client PRs.Follow-ups / out of scope:
subis sybil-bypassable (an attacker uses a fresh keypair per request → freshsub), so it adds no real security over the pre-existing per-IP limit while costing a Redis round-trip per request. If Coinbase quota abuse needs mitigating, the effective control is an aggregate/global limit — a separate decision. The endpoint remains rate-limited per-IP via the existing@fastify/rate-limitconfig.signMessagepath, plus thesignMessagerefusal above) and mobile — ships in separate PRs. Until they roll out,permissivekeeps current clients working.methodAndPath,iat) intentionally not done.