Skip to content

feat(ws): authenticate and meter price streams - #592

Open
Solvecraft000 wants to merge 1 commit into
Divinity-Labs:mainfrom
Solvecraft000:fix/issue-502
Open

Solvecraft000 wants to merge 1 commit into
Divinity-Labs:mainfrom
Solvecraft000:fix/issue-502

Conversation

@Solvecraft000

Copy link
Copy Markdown

Summary

  • Require a valid API key during the WebSocket handshake, using Authorization bearer auth or the browser lens-key.<key> subprotocol (never a URL parameter).
  • Reject revoked/unknown keys before upgrade, attach validated key context, and cap concurrent connections per key with WS_MAX_CONNECTIONS_PER_KEY.
  • Charge each opened stream at the existing $0.50 stream price against daily/monthly API-key usage quotas.
  • Redact the credential-bearing WebSocket subprotocol header from logs and document client usage.
  • Add integration coverage for missing/revoked credentials, browser subprotocols and connection limits.

Fixes #502

Verification

  • git diff --check passed.
  • Tests were added but not run in this pass.
  • Checked tracked source/config files for known PolinRider indicators and suspicious artifacts; none were found.

@drips-wave

drips-wave Bot commented Oct 21, 2026

Copy link
Copy Markdown

@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! 🚀

Learn more about application limits

Miracle656 added a commit that referenced this pull request Oct 22, 2026
  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 Miracle656 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

  • handleProtocols never selects the credential-bearing subprotocol back to the client. protocols.has('lens') ? 'lens' : false means the lens-key.<token> offer is never echoed in the response. That is the detail most implementations of this pattern get wrong.
  • sec-websocket-protocol added to REDACT_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 onResponse hook with the upgraded flag, plus close and error on the socket, covers all three exits, and the released guard 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:

  1. preValidation validates the key, reserves a slot, and checks the key's quota has room for 50 cents.
  2. The socket handler still finds paymentAddress set and no X-Payment header, so it sends 402 Payment required and closes — a valid, in-quota key is refused.
  3. If that client does also pay x402, setupStream then calls recordUsage(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, /ws streams to anyone who connects, and that is the hole the issue is about.

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.

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.

The websocket asks for nothing

2 participants