Repository navigation
Conversation
|
@Riri-dev1 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! 🚀 |
211c07f to
cfcd5c4
Compare
Miracle656
left a comment
There was a problem hiding this comment.
Scanned clean (both filename and content vectors) and the design is the right shape — a registry keyed by hashed key, a distinguishable 4401 / key_revoked close, and Redis pub/sub so a revocation reaches every instance. The problem is that it does not currently pass its own tests.
I approved the fork CI run so it would actually execute. 4 tests fail in src/__tests__/keyRevocationConnections.test.ts — your own test file, in a clean CI environment, not just locally:
FAIL 2. Revocation drops live subscriptions …
proves a GraphQL subscription stops and closes with 4401 "key_revoked"
FAIL 2. … proves a WebSocket price stream (/ws) closes with 4401 when revoked
AssertionError: expected 1006 to be 4401
FAIL 3. Multi-key isolation: other keys are unaffected
FAIL 4. Cross-instance propagation via Redis pub/sub
Final tally: Test Files 1 failed | 155 passed, Tests 4 failed.
1006 is the detail worth chasing. It means the socket closed with no close frame at all, so conn.close(4401, ...) never ran on that connection — this is not a wrong code being sent, it is nothing being sent and the connection dying at teardown instead.
connectionRegistry itself reads correctly to me, so my suspicion is registration rather than closing: if extractTokenFromRequest does not find the key the test supplies, authenticatedKeyHash stays null, setupStream never calls connectionRegistry.register, and revocation then has nothing to close. That would explain all four failures at once, including the isolation and Redis cases, which is why I prefer it to four separate causes. Worth logging the registry size at revocation time to confirm before changing anything.
One other thing to look at while you are in there: making the /ws handler async moves the whole auth lookup in front of the socket being wired up. Please check no early frame can be missed in that window.
Happy to re-review as soon as CI is green — the structure is good and I would like to merge it.
fix: close live connections on key revocation with cross-instance propagation (#503)
Summary
Fixes API key revocation so that revoking a key immediately terminates any live, long-lived WebSocket or GraphQL subscriptions authenticated with that key, preventing revoked keys from continuing to stream market data indefinitely.
Key Changes
src/ws/connectionRegistry.ts):ConnectionRegistrytracking live connections keyed by their SHA-256 hashed API key.closeConnectionsForKey(keyHash, 4401, 'key_revoked')which closes matching connections with distinguishable WebSocket close code4401and reason"key_revoked", leaving other keys' connections unaffected.src/api/admin.ts):DELETE /admin/keys/:idto close all live connections matching the revoked key's hash immediately afterrevokedAtis set in Postgres.publishRevocation(existing.hash).src/ws/connectionRegistry.ts,src/index.ts,src/app.ts):key:revoked.key:revokedacross all instances and drops matching connections locally upon receipt.src/index.tsand clean subscriber disconnect insrc/app.ts(onClose).src/api/graphql.ts): Interceptsconnection_initpayload and HTTP upgrade headers via MercuriusonConnect, validates the API key, hashes it, and registers the underlying WebSocket. Cleans up ononDisconnect.src/api/websocket.ts,src/ws/fanout.ts): Authenticates keys on/wsand registers active sockets withconnectionRegistry.src/__tests__/keyRevocationConnections.test.ts):4401and reason"key_revoked"upon key revocation.Related issue
Closes #503
Type of change
Checklist
npx tsc --noEmitpassesnpm run buildpasses