feat: add tempo subscriptions#331
Conversation
992c30a to
313f314
Compare
commit: |
a09cb0e to
d651f09
Compare
| ) | ||
| const active = matches.filter((record) => isActive(record)) | ||
|
|
||
| const subscription = (() => { |
There was a problem hiding this comment.
it feels odd we have to have the client pass a subscription id in order to disambiguate, shouldn't the server be able to filter subscriptions to check if one is valid for the given route / user?
| import type { SubscriptionRecord } from './Types.js' | ||
|
|
||
| const recordPrefix = 'tempo:subscription:record:' | ||
| const resourcePrefix = 'tempo:subscription:resource:' |
There was a problem hiding this comment.
allow these to be customized
| return records.filter((record: unknown): record is SubscriptionRecord => Boolean(record)) | ||
| }, | ||
| async put(record) { | ||
| await store.put(recordKey(record.subscriptionId), record) |
There was a problem hiding this comment.
add comments about what we are doing here
| return 'minute' | ||
| case 3_600: | ||
| return 'hour' | ||
| case 86_400: |
There was a problem hiding this comment.
make these constants
| } | ||
| } | ||
|
|
||
| function formatBillingInterval(periodSeconds: string) { |
There was a problem hiding this comment.
is there no way to manually bill users e.g. in a cron? we should add this
5d333a6 to
e776f4e
Compare
|
Great addition, @brendanjryan. The main thing I’d pin down early is how a later charge proves linkage to the original authorization window. A minimal stable tuple for that would already help a lot: authorization ref, cumulative spent, remaining window, and a verifier-facing record ref. |
5929e02 to
bb873c5
Compare
|
@jithinraj not sure I follow -- the auth lives on the subscription itself. what is the verifier in this context? |
|
Got it, thanks @brendanjryan. By “verifier-facing” I meant downstream audit / reconciliation, not a separate verifier path inside this PR. My earlier framing was broader than the current scope. The useful boundary seems simpler: subscription reuse is determined by the server’s lookup key plus Might be worth a short note around that boundary, mainly to distinguish when |
eb9dc93 to
75552f0
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
bb4deca to
7fea5da
Compare
| period: toSubscriptionPeriodSeconds(request), | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
We require consumers to also specify scopes post-T3 (only relevant here, not for local).
| activate: async ({ accessKey, request, resolved }) => { | ||
| const userId = resolved.key.split(':')[1] ?? 'anonymous' | ||
| const id = subscriptionId(userId) | ||
| const reference = chargeWithAccessKey({ | ||
| accessKey, | ||
| amount: request.amount, | ||
| periodIndex: 0, | ||
| subscriptionId: id, | ||
| }) | ||
| const record = { | ||
| amount: request.amount, | ||
| billingAnchor: new Date().toISOString(), | ||
| chainId: request.methodDetails?.chainId, | ||
| currency: request.currency, | ||
| lastChargedPeriod: 0, | ||
| lookupKey: resolved.key, | ||
| periodCount: request.periodCount, | ||
| periodUnit: request.periodUnit, | ||
| recipient: request.recipient, | ||
| reference, | ||
| subscriptionExpires: request.subscriptionExpires, | ||
| subscriptionId: id, | ||
| timestamp: new Date().toISOString(), | ||
| } satisfies Subscription.SubscriptionRecord | ||
|
|
||
| return { | ||
| receipt: Subscription.createSubscriptionReceipt(record), | ||
| subscription: record, | ||
| } | ||
| }, |
There was a problem hiding this comment.
I thought most of this could be handled internally
f451b0f to
ae2fd65
Compare
tempoxyz-cyclops-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
PR #331 adds Tempo subscription support (recurring TIP-20 payments via signed key authorizations), an authorize hook + stableBinding plumbing on Mppx, sequential compose() evaluation, an optional Receipt.subscriptionId, and a background renewSubscription helper. The implementation is well tested for single-tier happy paths, but workers verified several reachable issues in the new lifecycle. Most severe: the built-in automatic renewal path replays the activation KeyAuthorization on every renewal and is rejected by Tempo as KeyAlreadyExists, breaking recurring collection on a real node after period 0. Two distinct economic-binding gaps (cross-tier and overdue-period reuse) converge on the same root cause: authorize() and verify()/store.activate() reuse paths only check isActive(record) without comparing route binding fields or lastChargedPeriod. Recommend not merging until the inline findings tagged Critical/High are addressed.
Reviewer Callouts
- ⚡ Manual
activatehook contract (src/tempo/server/Subscription.ts:333-417): Customparameters.activateis passed the credential'saccessKeyandresolved.keywith no built-in consistency check. Automatic mode safely fails closed viasubmitSubscriptionPayment's stored-vs-supplied address comparison (:732-737); manual mode does not. Document that customactivateimplementations MUST verifyaccessKeymatches the application's expected access key forresolved.key. - ⚡
mppx.verifyCredential(credential)is side-effectful for subscription (src/server/Mppx.ts:368→subscription.verify→store.activate): The standalone "verify" call consumescredentialKey, may submit an on-chain transfer, and persists a record. Most users will assumeverifyCredentialis read-only. Document or split into check/settle phases. - ⚡
success()helper + SSE composition (src/server/Mppx.ts:502,src/tempo/server/internal/transport.ts:81-87):success()constructs a fake credential{ challenge, payload: {} }. Currently safe because the only method shippingauthorizeissubscription(HTTP). A futureauthorize+ SSE-stylerespondReceiptcomposition would throwNo SSE context available. Either hardensuccess()or document the constraint. - ⚡
paymentOfspreads function values into proxypaymentobject (src/proxy/Service.ts:209-220):_stableBinding,authorize,verify,request,respond,transportend up on the returnedpaymentobject. Dropped byJSON.stringifyin OpenAPI serialization, but a non-JSON serializer could leak closures oversecretKey/ store-private-keys. Explicitly destructure known function fields out ofrest. - ⚡
renewSubscriptionbackground helper ignoresactivationTimeoutMs(src/tempo/server/Subscription.ts:803): The background helper builds its own defaultSubscriptionStorewrapper, so cron always uses the default 15-minute timeout regardless of how the request-pathsubscription()was configured. Plumb timeouts into the background entrypoint. - ⚡
compose()ordering is now load-bearing (src/server/Mppx.ts:1287-1294): No-credential branch evaluatesauthorizehooks sequentially; first 200 wins. Document that side effects of earlier handlers may run even when a later handler would have served the request. - ⚡
Receipt.subscriptionIdis now part of the publicPayment-Receiptschema (src/Receipt.ts:23): Spec-additive, but.strict()downstream validators will reject. Worth calling out as forward-compatible. - ⚡
assertAuthorizationScopesonly requirestransferSelector(src/tempo/subscription/KeyAuthorization.ts:324-359): Clients must sign bothtransferandtransferWithMemo; the server submitstransferWithMemo, so a single-selector authorization passesverifybut reverts at simulation. Self-DoS, but worth documenting. - ⚡
request()hook can throwVerificationFailedErrorbefore HMAC is checked (src/tempo/server/Subscription.ts:163-166, called fromMppx.ts:465): A malformed credential whosechallenge.request.methodDetails.accessKeyis missing makes the request hook throw before Tier-1 HMAC runs. Express v4 hangs; v5/Hono/Next.js/Elysia return 500. WrapresolveRouteChallengein aPaymentError-aware try/catch increateMethodFnto normalize to a 402.
tempoxyz-cyclops-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review — wevm/mppx PR #331 (Tempo Subscriptions Hardening)
Summary
PR #331 introduces the Tempo subscription payment method (key-authorization–based recurring billing), the public authorize/stableBinding server-method hooks, dynamic SDK-managed access keys, activation replay protection, and renewal idempotency. The defensive design is broadly correct, but the activation/renewal state machines still contain multiple high-severity races and a broken verifyCredential path that re-introduce double-charge / orphaned-state outcomes after the previous "fix: address cyclops subscription findings" commit. There is also a DoS in the new compose dispatcher for any custom stableBinding. None of these break protocol-level signature/HMAC bindings, but they do break the documented billing semantics under realistic concurrency and externally reachable conditions.
Inline comments below mark each finding at the relevant changed line.
Reviewer Callouts
-
⚡
subscriptionBindingexcludeschainId→ compose dispatch always misses subscription candidates.src/tempo/server/Subscription.ts:83-94does not putchainIdindefaults, so_canonicalRequest.methodDetailsis undefined while issued challenges always carrymethodDetails.chainId. InMppx.compose()(src/server/Mppx.ts:1316-1322) the candidate set ends up empty and dispatch silently falls through to "first handler with matching name+intent". Either includemethodDetails.chainIdin subscriptiondefaultsor omitchainIdfromsubscriptionBinding. -
⚡ Unauthenticated state mutation in the no-credential path.
tempo.subscription.request()callsparameters.resolve(...)before any credential is presented and, in auto mode, unconditionally callsstore.getOrCreateAccessKey(resolved.key)(src/tempo/server/Subscription.ts:153-165,src/tempo/subscription/Store.ts:207-224), persisting a fresh secp256k1 private key per uniqueresolveoutput. Following the obvious test pattern (header → resolve → key) gives a developer an unauthenticated unbounded-storage primitive. Document thatresolvemust authenticate, or move key generation to the credential-bearing path. -
⚡
accessKeyis intentionally excluded fromsubscriptionBinding. Reviewers should confirm no future schema change introduces a route where the canonicalaccessKeylegitimately differs from the stored subscription's; silently bypassing the equality check would defeat per-credential authorization scoping. -
⚡ Fragile
withReceipt()error-string contract across 5 callsites.'withReceipt() requires a response argument'(src/server/Mppx.ts:547) is sentinel-matched bysrc/middlewares/{elysia,hono,nextjs,express}.tsandsrc/proxy/Proxy.ts. Export a typed sentinel error andinstanceof-check in adapters. -
⚡
Mppx.toNodeListener()does not probewithReceipt()for management responses. Unlike Hono/Elysia/Next/Express,src/server/Mppx.ts:1494-1501always callsresult.withReceipt(new Response())and only forwards thePayment-Receiptheader. -
⚡
credentialKeyactivation markers never expire (src/tempo/subscription/Store.ts:130-139). Every activation attempt — including failed ones — leaves a permanent entry. Consider a TTL keyed off the challengeexpires. -
⚡ Latest-period-only renewal is intentional (
src/tempo/server/Subscription.ts:841-864+ integration testrenews only the latest elapsed week period when multiple periods passed). After multiple missed periods,renew()charges once for the current period and advanceslastChargedPerioddirectly to it. A user who drains their wallet for several periods and tops up later effectively pauses billing. Make a deliberate product decision and document it. -
⚡ Background
renew()hooks can mutate stored amount/recipient/currency without detection.assertSubscriptionRequestMatchonly runs whenrequestis threaded intovalidateSubscriptionSettlement; background renewals do not. Worth a JSDoc note onrenew()that handlers must preserve those fields. -
⚡ Discovery (
x-payment-info) leaks every field of_canonicalRequestfor subscription routes includingsubscriptionExpires,periodCount,periodUnit,description,externalId(src/discovery/OpenApi.ts:204-208). Warn onsubscription.Parameters.externalIdthat the value is publicly advertised.
tempoxyz-cyclops-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
PR #331 introduces Tempo subscription support. Challenge HMAC binding, key-authorization scope/limit/expiry/source checks, per-tx period limits, credential-key replay protection, and the new stableBinding Tier 2 checks are sound. The serious issues are concentrated in the new SubscriptionStore lifecycle: stale-recovery for both activation and renewal does not carry an ownership token through the final commit phase. Three High-severity findings and several Medium/Low items are filed inline below.
Additional finding (file not in diff)
toNodeListener discards subscription authorize management responses (src/server/Mppx.ts:1492-1507).
tempo.subscription's authorize hook can return a 409 Retry-After management response when a renewal is concurrently in-flight (src/tempo/server/Subscription.ts:125-133). The Hono / Next / Elysia / Express adapters detect a management response via getManagementResponse() and forward it; toNodeListener does not, copying only the Payment-Receipt header from the wrapped response. Concurrent renewal callers on a Node listener therefore receive 200 OK + paid content instead of the documented backoff signal, defeating the in-flight protection.
Recommended Fix: Mirror the management-response detection used in src/middlewares/express.ts (call result.withReceipt() with no argument inside a try/catch sentinel) inside toNodeListener and forward the management response via NodeListener.sendResponse.
Reviewer Callouts
- ⚡ Activation/renewal lock semantics: please double-check the stale-recovery semantics are intentional. Depending on the timing branch, the current behaviour either silently double-commits or silently drops state — a human design pass on lock ownership tokens would help.
- ⚡
paymentOfstrip list (src/proxy/Service.ts:213-226) strips_stableBindingbut not the same-named functionstableBindingarriving via...method. Today the discovery doc is JSON-stringified so it is silently dropped; future non-JSON consumers would surface a callable. - ⚡
compose()candidate filter silently demotes a throwingstableBinding(src/server/Mppx.ts:1312-1327): thetry/catchfalls back to first-name+intent dispatch. Per-handler Tier 2 still catches mismatches today; if Tier 2 is ever weakened, this fails open. - ⚡
success()syntheticcredentialForReceipt = { challenge, payload: {} }: for any future MCP-on-authorize path, the receipt will carry a brand-newchallengeIdthe client never saw and cannot correlate. - ⚡
memoServerIdinconsistency: activation usesrealm(Subscription.ts:402), auto-renewal usessubscription.lookupKey(Subscription.ts:723). Downstream attribution tooling that filters byserverIdwill not find auto-renewal transfers. - ⚡
verifyCredentialhas settlement side effects for subscriptions: callingmppx.verifyCredential(token)from a webhook will trigger activation (on-chaintransferWithMemo+ KV writes). Surface in migration notes; currently documented in JSDoc only.
| const { subscription } = result | ||
| const committed = await store.update(activationKey(subscription.lookupKey), (current) => { | ||
| const marker = current as ActivationMarker | null | ||
| if (marker?.challengeId !== challengeId) return { op: 'noop', result: false } |
There was a problem hiding this comment.
🚨 [SECURITY] Stale activation can cancel/overwrite a newer activation after both billing hooks run
This commit-ownership check passes once and then writes { ...marker, committingAt: timestamp() } while leaving the original startedAt intact. Because isStaleActivation() reads only startedAt, a second activation can immediately treat the committing marker as stale and replace it. The first activation, holding committed = true, then runs lines 196–205: it cancels the current lookup owner (now the second, paid activation), writes its own subscription record, and resets the lookup pointer back to itself — without re-checking ownership. Activation billing happens inside create() before this commit, so both attempts may have already executed irreversible payment side effects.
Recommended Fix:
Make activation commit ownership durable. When entering commit, refresh the marker's staleness timestamp (or have isStaleActivation() treat committingAt as non-stale), AND add a final compare-and-set immediately before each store.put(recordKey…) and store.put(lookupRecordKey…) write that bails out if the marker no longer contains this challengeId.
| result: true, | ||
| } | ||
| }) | ||
| if (!committed) return { status: 'claimMismatch' } |
There was a problem hiding this comment.
🚨 [SECURITY] Successful activation payments are silently dropped when the activation marker is replaced
When create() (which submits the on-chain transferWithMemo and, with the default waitForConfirmation: true, awaits the receipt) takes longer than activationTimeoutMs (default 15 minutes), the client retries with a new challenge. The stale marker is replaced (lines 159–171), and when the original payment finally confirms, this branch returns { status: 'claimMismatch' } and the subscription record is never persisted. The on-chain KeyAuthorization per-period limit has been consumed, so the retry's transaction reverts on chain — the user is charged with no subscription record and cannot retry until the next period.
Recommended Fix:
Reconcile a confirmed-but-superseded activation rather than dropping it (e.g. recover by Tempo memo Attribution.encode({ challengeId, serverId }), or persist the late-confirming subscription under a quarantine record key for the integrator to resolve). A naive "always save" patch is unsafe — it must be combined with the ownership fix in the previous comment so the late writer cannot overwrite the live activation.
|
|
||
| const committed = await store.update(recordKey(subscriptionId), (current) => { | ||
| const existing = current as SubscriptionRecord | null | ||
| if (!existing || existing.inFlightPeriod !== periodIndex) { |
There was a problem hiding this comment.
🚨 [SECURITY] Stale renewal attempts can commit over (or clear) a newer renewal for the same period
The renewal commit checks only existing.inFlightPeriod === periodIndex. Two attempts for the same billing period necessarily share periodIndex, and clearRenewalState() at line 119 has the same flaw. Therefore a stale attempt A whose hook eventually returns can:
- Pass this check after a newer attempt B has acquired the in-flight marker, committing A's result over B and clearing B's in-flight state.
- Throw after B has acquired the marker, causing A's catch path (
clearRenewalState(subscriptionId, periodIndex)at line 300) to clear B's marker.
The default server path uses a deterministic inFlightReference = "renewal:<subscriptionId>:<periodIndex>" (src/tempo/server/Subscription.ts:486-489), so checking inFlightReference alone would not distinguish same-period retries either.
Recommended Fix:
Use a per-attempt ownership token on both clearRenewalState() and this commit. Compare the exact inFlightStartedAt written by this caller, or add a separate per-attempt nonce distinct from the deterministic inFlightReference. A stale attempt must not clear or commit if the stored attempt token has changed.
| const existing = resolved ? await store.getByKey(resolved.key) : null | ||
| const accessKey = | ||
| resolved && !credential | ||
| ? await resolveChallengeAccessKey({ |
There was a problem hiding this comment.
🛡️ [DEFENSE-IN-DEPTH] Eager access-key persistence on every authenticated GET enables KV exhaustion if resolve() is lax
Every authenticated request to a subscription-protected route calls store.getOrCreateAccessKey(resolved.key), which persists a server-owned secp256k1 keypair before any payment occurs. If parameters.resolve is lax (e.g. derives the key from a query parameter without authenticating the caller), an unauthenticated attacker can pollute KV with access-key records for arbitrary keys.
Recommended Fix:
Document loudly that resolve() must perform identity / authorization before returning a key. Additionally, add a store.get precheck inside getOrCreateAccessKey so the random secp256k1 keypair is only generated when the record is actually missing (currently Secp256k1.randomPrivateKey() runs on every request).
| }) | ||
| if (!renew) return null | ||
|
|
||
| const renewal = await settleRenewal({ |
There was a problem hiding this comment.
💡 [SUGGESTION] Background renew() skips request-match assertion
validateSubscriptionSettlement only runs assertSubscriptionRequestMatch when options.request is supplied. The cron-style background renew() does not pass request, so a misbehaving custom renew hook that mutates amount/currency/recipient in the returned subscription is committed without challenge — the persisted record drifts away from on-chain reality.
Recommended Fix:
Always assert the renewed record matches the prior record's economic-binding fields, even outside the request path.
20489a8 to
22d2558
Compare
777b93f to
b01c9be
Compare
b01c9be to
e0ec9b9
Compare
e0ec9b9 to
8272e50
Compare
Summary
Added Tempo subscription support with activation, renewal, reusable subscription authorization, adapter management-response handling, compose dispatch hardening, subscription lifecycle race protection, ergonomic Date/period inputs, attribution metadata cleanup, and typed receipt-response sentinel handling.