Skip to content

Commit d8e868b

Browse files
authored
Merge pull request #727 from devforth/fix/ws-bugs
fix: cancel stale websocket operations AdminForth/1980 AdminForth/198…
2 parents e0c39c9 + d8b8486 commit d8e868b

5 files changed

Lines changed: 433 additions & 28 deletions

File tree

‎adminforth/modules/socketBroker.ts‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,11 @@ export default class SocketBroker implements IWebSocketBroker {
8888
this.startChecker();
8989

9090
this.clients.set(client.id, client);
91+
const pendingSubscriptions = new Map<string, object>();
9192
client.onMessage(async (message) => {
93+
if (this.clients.get(client.id) !== client) {
94+
return;
95+
}
9296
const messageText = message.toString();
9397

9498
if (!messageText.trim()) {
@@ -130,20 +134,28 @@ export default class SocketBroker implements IWebSocketBroker {
130134
const topic = payload.topic;
131135

132136
if (payload.type === 'subscribe') {
137+
// Only the latest subscribe intent may commit after asynchronous authorization.
138+
const subscription = {};
139+
pendingSubscriptions.set(topic, subscription);
140+
let authResult = true;
133141
if (!topic.startsWith('/opentopic/')) {
134142
if (this.adminforth.config.auth.websocketTopicAuth) {
135-
let authResult = false;
143+
authResult = false;
136144
try {
137145
authResult = await this.adminforth.config.auth.websocketTopicAuth(topic, client.adminUser);
138146
} catch (e) {
139147
afLogger.error(`Error in websocketTopicAuth, assuming connection not allowed ${e}`);
140148
}
141-
if (!authResult) {
142-
client.send(JSON.stringify({ type: 'error', message: 'Unauthorized' }));
143-
return;
144-
}
145149
}
146150
}
151+
if (this.clients.get(client.id) !== client || pendingSubscriptions.get(topic) !== subscription) {
152+
return;
153+
}
154+
pendingSubscriptions.delete(topic);
155+
if (!authResult) {
156+
client.send(JSON.stringify({ type: 'error', message: 'Unauthorized' }));
157+
return;
158+
}
147159
if (!this.topics[topic]) {
148160
this.topics[topic] = [];
149161
}
@@ -163,16 +175,19 @@ export default class SocketBroker implements IWebSocketBroker {
163175
return;
164176
}
165177

178+
pendingSubscriptions.delete(topic);
166179
this.deleteClientFromTopic(client, topic);
167180
this.cleanupTopicIfEmpty(topic);
168181
client.topics.delete(topic);
169182
});
170183

171184
client.onClose(() => {
185+
pendingSubscriptions.clear();
172186
for (const topic of client.topics) {
173187
this.deleteClientFromTopic(client, topic);
174188
this.cleanupTopicIfEmpty(topic);
175189
}
190+
client.topics.clear();
176191
this.clients.delete(client.id);
177192
});
178193

‎adminforth/servers/express.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {
1515
HttpExtra,
1616
IAdminForthHttpResponse,
1717
} from '../types/Back.js';
18-
import { WebSocketServer } from 'ws';
18+
import { WebSocket, WebSocketServer } from 'ws';
1919
import { WebSocketClient } from './common.js';
2020
import http from 'http';
2121
import type { AddressInfo } from 'net';
@@ -310,6 +310,9 @@ class ExpressServer implements IExpressHttpServer {
310310
if (result.status === 'verifyFailed') {
311311
throw result.error;
312312
}
313+
if (ws.readyState !== WebSocket.OPEN) {
314+
return;
315+
}
313316

314317
this.adminforth.websocket.registerWsClient(
315318
new WebSocketClient({

‎adminforth/spa/src/websocket.ts‎

Lines changed: 33 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,16 @@ type Subscription = {
1010
const subscriptions: { [topic: string]: Subscription[] } = {};
1111
let nextSubscriptionId = 1;
1212

13-
interface ExtendedWebSocket extends WebSocket {
14-
connected?: boolean;
15-
}
16-
1713
const state: {
1814
status: 'connecting' | 'connected' | 'disconnected';
19-
ws: ExtendedWebSocket | null;
15+
ws: WebSocket | null;
16+
ready: boolean;
2017
} = {
2118
status: 'connecting',
22-
ws: null
19+
ws: null,
20+
ready: false,
2321
};
22+
let reconnectTimer: ReturnType<typeof setTimeout> | undefined;
2423

2524
function doPhysicalSubscribe(topic: string) {
2625
state.ws!.send(JSON.stringify({ type: 'subscribe', topic }));
@@ -37,7 +36,7 @@ async function connect () {
3736
console.error('Websocket not supported by this browser');
3837
return;
3938
}
40-
if (state.ws?.connected) {
39+
if (state.ws && state.ws.readyState !== WebSocket.CLOSED) {
4140
console.error('🔌 AFWS already connected');
4241
return;
4342
}
@@ -48,16 +47,20 @@ async function connect () {
4847
}
4948

5049
const clientId = encodeURIComponent(getAdminForthClientId());
51-
state.ws = new WebSocket(`${
50+
const ws = new WebSocket(`${
5251
window.location.protocol === 'http:' ? 'ws' : 'wss'
5352
}://${window.location.host}${base}/afws?clientId=${clientId}`);
53+
state.ws = ws;
5454
state.status = 'connecting';
55-
state.ws.addEventListener('open', () => {
55+
state.ready = false;
56+
ws.addEventListener('open', () => {
57+
if (state.ws !== ws) return;
5658
console.log('🔌 AFWS connected');
5759
state.status = 'connected';
5860

5961
});
60-
state.ws.addEventListener('message', (event) => {
62+
ws.addEventListener('message', (event) => {
63+
if (state.ws !== ws) return;
6164
const data = event.data.toString();
6265
if (data === 'pong') {
6366
return;
@@ -66,6 +69,7 @@ async function connect () {
6669

6770
const message = JSON.parse(data);
6871
if (message.type === 'ready') {
72+
state.ready = true;
6973
Object.keys(subscriptions).forEach((topic) => {
7074
doPhysicalSubscribe(topic);
7175
});
@@ -81,14 +85,18 @@ async function connect () {
8185
}
8286
}
8387
});
84-
state.ws.addEventListener('close', () => {
88+
ws.addEventListener('close', () => {
89+
if (state.ws !== ws) return;
8590
console.log('🔌 AFWS disconnected');
86-
setTimeout(() => {
91+
reconnectTimer = setTimeout(() => {
92+
if (state.ws !== ws) return;
93+
reconnectTimer = undefined;
8794
console.log('🔌 AFWS reconnecting after close');
8895
connect();
8996
// if it is first time, reconnect instantly
9097
}, state.status === 'connected' ? 0 : 2_000);
9198
state.status = 'disconnected';
99+
state.ready = false;
92100
});
93101
}
94102

@@ -100,11 +108,14 @@ try {
100108

101109
export function reconnect() {
102110
console.log('🔌 AFWS reconnect initiated');
103-
// disconnect if already connected
104-
if (state.status === 'connected') {
105-
state.ws!.close();
106-
}
107-
111+
clearTimeout(reconnectTimer);
112+
reconnectTimer = undefined;
113+
const ws = state.ws;
114+
state.ws = null;
115+
state.status = 'disconnected';
116+
state.ready = false;
117+
ws?.close();
118+
connect();
108119
}
109120

110121
setInterval(() => {
@@ -125,7 +136,7 @@ function unsubscribeSubscription(topic: string, subscriptionId: number): void {
125136
}
126137

127138
delete subscriptions[topic];
128-
if (state.status === 'connected') {
139+
if (state.ready) {
129140
doPhysicalUnsubscribe(topic);
130141
}
131142
}
@@ -141,7 +152,7 @@ export default {
141152
id: subscriptionId,
142153
callback,
143154
});
144-
if (isFirstSubscription && state.status === 'connected') {
155+
if (isFirstSubscription && state.ready) {
145156
doPhysicalSubscribe(topic);
146157
}
147158

@@ -161,7 +172,7 @@ export default {
161172
return;
162173
}
163174
delete subscriptions[topic];
164-
if (state.status === 'connected') {
175+
if (state.ready) {
165176
doPhysicalUnsubscribe(topic);
166177
}
167178
},
@@ -171,7 +182,7 @@ export default {
171182
.filter((topic) => topic.startsWith(prefix))
172183
.forEach((topic) => {
173184
delete subscriptions[topic];
174-
if (state.status === 'connected') {
185+
if (state.ready) {
175186
doPhysicalUnsubscribe(topic);
176187
}
177188
});
@@ -180,7 +191,7 @@ export default {
180191
unsubscribeAll(): void {
181192
Object.keys(subscriptions).forEach((topic) => {
182193
delete subscriptions[topic];
183-
if (state.status === 'connected') {
194+
if (state.ready) {
184195
doPhysicalUnsubscribe(topic);
185196
}
186197
});
Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
import { jest } from '@jest/globals';
2+
import { once } from 'node:events';
3+
import { createRequire } from 'node:module';
4+
import SocketBroker from '../../adminforth/modules/socketBroker';
5+
import ExpressServer from '../../adminforth/servers/express';
6+
import { WebSocketClient } from '../../adminforth/servers/common';
7+
8+
const require = createRequire(new URL('../../adminforth/package.json', import.meta.url));
9+
const express = require('express');
10+
const { WebSocket } = require('ws');
11+
const TOPIC = '/test/pending';
12+
const USER = { pk: 'alice', dbUser: { id: 'alice' } };
13+
14+
function deferred<T>() {
15+
let resolve!: (value: T) => void;
16+
const promise = new Promise<T>((done) => { resolve = done; });
17+
return { promise, resolve };
18+
}
19+
20+
function fixture(topicAuth: (topic: string, user: any) => Promise<boolean>) {
21+
const subscribed = jest.fn();
22+
const admin = { config: { auth: { websocketTopicAuth: topicAuth, websocketSubscribed: subscribed } } };
23+
const broker = new SocketBroker(admin as any);
24+
jest.spyOn(broker, 'startChecker').mockImplementation(async () => {});
25+
let onMessage!: (message: string) => Promise<void>;
26+
let onClose!: () => void;
27+
const sent: any[] = [];
28+
const client = new WebSocketClient({
29+
id: 'test-client', adminUser: USER,
30+
authorize: async () => ({ status: 'ok', adminUser: USER }),
31+
send: (message: string) => sent.push(JSON.parse(message)),
32+
close: () => onClose(),
33+
onMessage: (handler: typeof onMessage) => { onMessage = handler; },
34+
onClose: (handler: typeof onClose) => { onClose = handler; },
35+
});
36+
broker.registerWsClient(client);
37+
return {
38+
broker, client, sent, subscribed,
39+
message: (type: string, topic = TOPIC) => onMessage(JSON.stringify({ type, topic })),
40+
close: () => onClose(),
41+
};
42+
}
43+
44+
describe('pending websocket subscriptions', () => {
45+
it('unsubscribe cancels an earlier authorization before a later publish', async () => {
46+
const auth = deferred<boolean>();
47+
const f = fixture(() => auth.promise);
48+
const pending = f.message('subscribe');
49+
await f.message('unsubscribe');
50+
auth.resolve(true);
51+
await pending;
52+
await f.broker.publish(TOPIC, { after: 'unsubscribe' });
53+
54+
expect(f.broker.topics[TOPIC]).toBeUndefined();
55+
expect(f.client.topics.size).toBe(0);
56+
expect(f.subscribed).not.toHaveBeenCalled();
57+
expect(f.sent).toEqual([{ type: 'ready' }]);
58+
});
59+
60+
it('close cancels authorization and avoids filters for disconnected users', async () => {
61+
const auth = deferred<boolean>();
62+
const f = fixture(() => auth.promise);
63+
const pending = f.message('subscribe');
64+
f.close();
65+
auth.resolve(true);
66+
await pending;
67+
await f.broker.checkDeadClients();
68+
await f.broker.reauthorizeClients();
69+
const filter = jest.fn(async () => true);
70+
await f.broker.publish(TOPIC, {}, filter);
71+
72+
expect(f.broker.clients.size).toBe(0);
73+
expect(f.broker.topics[TOPIC]).toBeUndefined();
74+
expect(f.client.topics.size).toBe(0);
75+
expect(f.subscribed).not.toHaveBeenCalled();
76+
expect(filter).not.toHaveBeenCalled();
77+
});
78+
79+
it('an older allowed subscribe cannot override a newer denied resubscribe', async () => {
80+
const older = deferred<boolean>();
81+
const newer = deferred<boolean>();
82+
let calls = 0;
83+
const f = fixture(() => (++calls === 1 ? older.promise : newer.promise));
84+
const first = f.message('subscribe');
85+
await f.message('unsubscribe');
86+
const second = f.message('subscribe');
87+
newer.resolve(false);
88+
await second;
89+
older.resolve(true);
90+
await first;
91+
92+
expect(f.broker.topics[TOPIC]).toBeUndefined();
93+
expect(f.subscribed).not.toHaveBeenCalled();
94+
expect(f.sent).toEqual([{ type: 'ready' }, { type: 'error', message: 'Unauthorized' }]);
95+
});
96+
97+
it('an older denial cannot cancel a still-pending resubscribe', async () => {
98+
const older = deferred<boolean>();
99+
const newer = deferred<boolean>();
100+
let calls = 0;
101+
const f = fixture(() => (++calls === 1 ? older.promise : newer.promise));
102+
const first = f.message('subscribe');
103+
await f.message('unsubscribe');
104+
const second = f.message('subscribe');
105+
older.resolve(false);
106+
await first;
107+
expect(f.broker.topics[TOPIC]).toBeUndefined();
108+
expect(f.sent).toEqual([{ type: 'ready' }]);
109+
newer.resolve(true);
110+
await second;
111+
await f.broker.publish(TOPIC, { value: 1 });
112+
113+
expect(f.broker.topics[TOPIC]).toEqual([f.client]);
114+
expect(f.subscribed).toHaveBeenCalledTimes(1);
115+
expect(f.sent).toEqual([{ type: 'ready' }, { type: 'message', topic: TOPIC, data: { value: 1 } }]);
116+
});
117+
});
118+
119+
it('does not register a transport closed while handshake authorization was pending', async () => {
120+
const entered = deferred<void>();
121+
const authorization = deferred<any>();
122+
const authorize = jest.fn(async () => {
123+
entered.resolve();
124+
return authorization.promise;
125+
});
126+
const admin = {
127+
config: { baseUrl: '', auth: {} },
128+
auth: { authorizeByCookies: authorize },
129+
formatAdminForth: () => 'WebSocket regression fixture',
130+
websocket: undefined as any,
131+
};
132+
const broker = new SocketBroker(admin as any);
133+
admin.websocket = broker;
134+
jest.spyOn(broker, 'startChecker').mockImplementation(async () => {});
135+
const registration = jest.spyOn(broker, 'registerWsClient');
136+
const server = new ExpressServer(admin as any) as any;
137+
server.expressApp = express();
138+
server.setupWsServer();
139+
server.server.listen(0, '127.0.0.1');
140+
await once(server.server, 'listening');
141+
const socket = new WebSocket(`ws://127.0.0.1:${server.server.address().port}/afws`);
142+
try {
143+
await once(socket, 'open');
144+
await entered.promise;
145+
const closed = once(socket, 'close');
146+
socket.close();
147+
await closed;
148+
authorization.resolve({ status: 'ok', adminUser: USER });
149+
// Drain the handshake continuation before checking its observable effects.
150+
await new Promise<void>((resolve) => setImmediate(resolve));
151+
await broker.reauthorizeClients();
152+
153+
expect(registration).not.toHaveBeenCalled();
154+
expect(broker.clients.size).toBe(0);
155+
expect(authorize).toHaveBeenCalledTimes(1);
156+
} finally {
157+
authorization.resolve({ status: 'noToken' });
158+
socket.terminate();
159+
await new Promise<void>((resolve) => server.server.close(resolve));
160+
}
161+
});

0 commit comments

Comments
 (0)