Repository navigation
feat(ws): authenticate and meter price streams - #592
Solvecraft000 wants to merge 1 commit into
Conversation
|
@Solvecraft000 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
tsc --noEmit clean vitest 106 files, 1232 passing (up from 102 / 1157) dropped-tests none Four of the eight screened in this group. The other four were held, and the reason is worth stating because it keeps recurring: each one merges cleanly and fails on its own. #590 72 type errors alone, including syntax errors in src/api/rest.ts and src/api/schemas.ts -- "Declaration or statement expected", "Expression expected". Not type mismatches; the file does not parse. #584 3 type errors alone #592 conflicts #595 conflicts Merged together before screening, that group produced 68 failing test files and syntax errors I first mistook for a bad three-way merge. They were not: #590 carries them on its own branch. Screening each PR individually before batching costs one typecheck per PR and saves an hour of bisection, which is the lesson of the day. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Miracle656
left a comment
There was a problem hiding this comment.
This is careful work and several details are better than I expected — but it has one problem I cannot resolve on your behalf, because the answer is a decision about how the endpoint is billed.
What is right
handleProtocolsnever selects the credential-bearing subprotocol back to the client.protocols.has('lens') ? 'lens' : falsemeans thelens-key.<token>offer is never echoed in the response. That is the detail most implementations of this pattern get wrong.sec-websocket-protocoladded toREDACT_PATHS. If the key can travel in a subprotocol, it will end up in a log line eventually, and you closed that in the same PR rather than leaving it for someone to find.- The lease, and releasing it on a failed upgrade. A rejected handshake never reaches the socket handler, so a reserved slot would leak forever. The
onResponsehook with theupgradedflag, pluscloseanderroron the socket, covers all three exits, and thereleasedguard makes double-release harmless. That is the part I expected to be wrong and it is not. checkQuota(keyId, additionalCents = 0)preserves the existing comparison when the argument is absent (>=) and uses>only for the look-ahead case. Changing a shared quota predicate without disturbing its existing callers is exactly the right instinct.
Blocking: this makes the x402 payment path unreachable, and double-charges API-key clients
/ws on main is a paid endpoint, settled per connection through x402 — no API key involved:
const paymentAddress = paymentAddressFor(network)
const paymentHeader = (req.headers['x-payment'] as string) || (req.query as any).payment
...
} else if (!paymentHeader) {
socket.send(JSON.stringify({ type: 'error', status: 402, message: 'Payment required for real-time streaming', requirements }))
socket.close()
return
} else {
// verify, then setupStream
}ORACLE_PAYMENT_ADDRESS / ORACLE_PAYMENT_ADDRESS_{TESTNET,MAINNET} are documented settings, so this is a supported path, not dead code.
Your preValidation runs before any of that and rejects a request with no API key:
if (!supplied.token) {
api_key_auth_failures_total.inc({ reason: 'missing' })
return reply.status(401).send({ error: 'Unauthorized', code: 'missing' })
}So an x402 client that is willing to pay — and on main would be told its price and allowed to pay it — now gets a 401 at the handshake and never sees the 402. The paid path is gone.
And for a client that does present an API key, on a network with a payment address configured, the two mechanisms stack:
preValidationvalidates the key, reserves a slot, and checks the key's quota has room for 50 cents.- The socket handler still finds
paymentAddressset and noX-Paymentheader, so it sends402 Payment requiredand closes — a valid, in-quota key is refused. - If that client does also pay x402,
setupStreamthen callsrecordUsage(req.apiKey!.id, STREAM_PRICE_CENTS)and bills the key another 50 cents on top of the on-chain payment it just made.
Charged twice, or refused despite being authenticated and in quota, depending on whether it pays.
What I would merge
Treat a key and a payment as alternatives, not as layers. Concretely:
In preValidation:
- malformed credentials → 401
malformed(as you have it); - a token present → validate, quota-check, cap-check, reserve the lease (as you have it);
- no token → resolve the network and check
paymentAddressFor(network):- configured → let the request through with no
req.apiKey, so the existing x402 flow runs unchanged; - not configured → 401
missing. This is the case that actually closes #502: today, with x402 unconfigured,/wsstreams to anyone who connects, and that is the hole the issue is about.
- configured → let the request through with no
In the socket handler: if req.apiKey is set, skip the x402 requirement entirely and go straight to setupStream. The key is the authorization; the payment branch is for callers who have no key.
In setupStream: only recordUsage when req.apiKey is set — an x402 caller has already paid on chain and has no quota row to debit. Right now req.apiKey!.id would throw for that caller, which the ! is hiding.
That keeps both ways of paying, bills each exactly once, and makes the free-stream case impossible rather than conditional.
Two smaller things
maxConnectionsPerKey() throws, per request. A typo in WS_MAX_CONNECTIONS_PER_KEY turns every WebSocket handshake into a 500 rather than failing visibly once. Parse it once at module load and, if it is invalid, log an error and fall back to the default of 5 — a restrictive fallback is safe, and the operator gets a line that names the variable. (Please do not make it a boot-time throw: a startup validator crash-looped one of our services recently, and a per-connection limit is not worth taking the process down for.)
The 401 code is unknown for a revoked or expired key. lookupApiKey collapses all three failures into null, so a revoked key is reported as unrecognised, both to the client and in the api_key_auth_failures_total{reason} label. /admin and the HTTP auth hook distinguish these (revoked, expired), and the WebSocket path reading differently makes the metric misleading. lookupApiKeyResult already computes the reason; exporting it would let the handshake use the real one.
On the test merge
tests/integration/ws.test.ts has moved under you — main has since gained the ping/backpressure and fan-out-cleanup assertions (config.ws.pingIntervalMs, maxMissedPings, fanOutManager.getClientCount()), and your version replaces the block they live in. src/metrics.ts also conflicts, additively, against api_key_cache_lookups_total from #594. Please rebase on main and keep both sets of assertions — the cleanup checks are load-bearing for a different issue and should not be lost to this merge.
Happy to take another look as soon as it is updated. The handshake and lease mechanics are the hard part and they are already sound; it is the interaction with x402 that needs the decision.
Summary
lens-key.<key>subprotocol (never a URL parameter).WS_MAX_CONNECTIONS_PER_KEY.Fixes #502
Verification
git diff --checkpassed.