Skip to content

Commit 7bbbacb

Browse files
coder13coder13
authored
Merge pull request #228 from coder13/agent/codeql-high-security
Harden browser password and API request handling
2 parents 3bc92d7 + a09e16e commit 7bbbacb

19 files changed

Lines changed: 343 additions & 139 deletions

‎client/src/components/Header.jsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import IconButton from '@mui/material/IconButton';
1111
import Avatar from '@mui/material/Avatar';
1212
import Button from '@mui/material/Button';
1313
import Badge from '@mui/material/Badge';
14-
import { apiOrigin } from '../lib/fetch';
14+
import { lcFetch } from '../lib/fetch';
1515
import { getNameFromId } from '../lib/events';
1616
import { getWcaAuthorizationUrl } from '../lib/wcaAuth';
1717
import NotificationsIcon from '@mui/icons-material/Notifications';
@@ -73,7 +73,8 @@ function Header({ user, room, notifications }) {
7373
};
7474

7575
const logout = () => {
76-
window.location = `${apiOrigin || ''}/auth/logout?redirect=${document.location.origin}/`;
76+
lcFetch('/auth/logout', { method: 'POST' })
77+
.finally(() => { window.location = `${document.location.origin}/`; });
7778
};
7879

7980
return (

‎client/src/lib/fetch.js‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,45 @@
22

33
export const apiOrigin = process.env.REACT_APP_API_ORIGIN;
44

5-
export const lcFetch = (url, options) => (
6-
fetch(`${apiOrigin}${url}`, {
5+
const UNSAFE_METHODS = new Set(['DELETE', 'PATCH', 'POST', 'PUT']);
6+
let csrfTokenRequest;
7+
8+
const requestCsrfToken = async () => {
9+
if (!csrfTokenRequest) {
10+
csrfTokenRequest = fetch(`${apiOrigin}/api/csrf-token`, {
11+
credentials: 'include',
12+
}).then(async (response) => {
13+
if (!response.ok) {
14+
throw new Error('Unable to prepare a secure request.');
15+
}
16+
17+
const { csrfToken } = await response.json();
18+
if (!csrfToken) {
19+
throw new Error('Unable to prepare a secure request.');
20+
}
21+
22+
return csrfToken;
23+
}).catch((error) => {
24+
csrfTokenRequest = null;
25+
throw error;
26+
});
27+
}
28+
29+
return csrfTokenRequest;
30+
};
31+
32+
const withCsrfToken = (headers, csrfToken) => ({
33+
...(headers || {}),
34+
'x-csrf-token': csrfToken,
35+
});
36+
37+
export const lcFetch = async (url, options = {}) => {
38+
const method = (options.method || 'GET').toUpperCase();
39+
const csrfToken = UNSAFE_METHODS.has(method) ? await requestCsrfToken() : null;
40+
41+
return fetch(`${apiOrigin}${url}`, {
742
...options,
843
credentials: 'include',
9-
})
10-
);
44+
...(csrfToken ? { headers: withCsrfToken(options.headers, csrfToken) } : {}),
45+
});
46+
};

‎client/src/lib/fetch.test.js‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import { lcFetch } from './fetch';
2+
3+
describe('lcFetch', () => {
4+
beforeEach(() => {
5+
window.fetch = jest.fn();
6+
});
7+
8+
afterEach(() => {
9+
jest.resetModules();
10+
});
11+
12+
it('adds a server-issued CSRF token to unsafe requests', async () => {
13+
window.fetch
14+
.mockResolvedValueOnce({
15+
json: () => Promise.resolve({ csrfToken: 'csrf-token' }),
16+
ok: true,
17+
})
18+
.mockResolvedValueOnce({ ok: true });
19+
20+
await lcFetch('/api/updatePreference', {
21+
headers: { 'Content-Type': 'application/json' },
22+
method: 'PUT',
23+
});
24+
25+
expect(window.fetch).toHaveBeenNthCalledWith(1, 'undefined/api/csrf-token', {
26+
credentials: 'include',
27+
});
28+
expect(window.fetch).toHaveBeenNthCalledWith(2, 'undefined/api/updatePreference', {
29+
credentials: 'include',
30+
headers: {
31+
'Content-Type': 'application/json',
32+
'x-csrf-token': 'csrf-token',
33+
},
34+
method: 'PUT',
35+
});
36+
});
37+
38+
it('does not fetch a CSRF token for safe requests', async () => {
39+
window.fetch.mockResolvedValue({ ok: true });
40+
41+
await lcFetch('/api/me');
42+
43+
expect(window.fetch).toHaveBeenCalledTimes(1);
44+
expect(window.fetch).toHaveBeenCalledWith('undefined/api/me', {
45+
credentials: 'include',
46+
});
47+
});
48+
});

‎client/src/store/middlewares/rooms.js‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ import {
5353
import {
5454
clearRoomPassword,
5555
persistRoomPassword,
56+
purgeLegacyRoomPasswords,
5657
readRoomPassword,
5758
} from '../room/roomPasswordStorage';
5859
import {
@@ -91,6 +92,11 @@ export const createRoomsNamespaceMiddleware = ({
9192
ackTimeoutMs = DEFAULT_ACK_TIMEOUT_MS,
9293
retryDelayMs = DEFAULT_RETRY_DELAY_MS,
9394
} = {}) => (store) => {
95+
try {
96+
purgeLegacyRoomPasswords(storage);
97+
} catch {
98+
// Passwords are intentionally kept in memory only.
99+
}
94100
let roomsConnected = false;
95101
let joinedRoomId = null;
96102
let pendingJoinRequest = null;
@@ -113,15 +119,15 @@ export const createRoomsNamespaceMiddleware = ({
113119

114120
const readStoredRoomPassword = (roomId) => {
115121
try {
116-
return readRoomPassword(roomId, storage);
122+
return readRoomPassword(roomId);
117123
} catch {
118124
return null;
119125
}
120126
};
121127

122128
const forgetRoomPassword = (roomId) => {
123129
try {
124-
clearRoomPassword(roomId, storage);
130+
clearRoomPassword(roomId);
125131
} catch {
126132
// The next invalid join will try again.
127133
}
@@ -145,7 +151,7 @@ export const createRoomsNamespaceMiddleware = ({
145151
}
146152

147153
try {
148-
persistRoomPassword(roomId, password, storage);
154+
persistRoomPassword(roomId, password);
149155
} catch {
150156
warnPasswordStorage(roomId);
151157
}

‎client/src/store/middlewares/rooms.test.js‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
persistPendingResult,
2121
} from '../room/resultOutbox';
2222
import {
23+
clearRoomPassword,
2324
persistRoomPassword,
2425
readRoomPassword,
2526
} from '../room/roomPasswordStorage';
@@ -161,6 +162,7 @@ const result = {
161162
describe('rooms middleware', () => {
162163
beforeEach(() => {
163164
window.localStorage.clear();
165+
['private-room', 'new-private-room', 'room-one', 'room-two'].forEach(clearRoomPassword);
164166
});
165167

166168
it('keeps the loaded room mounted while reconnecting its socket', () => {
@@ -195,8 +197,8 @@ describe('rooms middleware', () => {
195197
expect(joins[0].args[0]).toEqual({ id: 'room-two', password: 'secret' });
196198
});
197199

198-
it('uses a saved private room password after a refresh', () => {
199-
persistRoomPassword('private-room', 'saved-password');
200+
it('does not reuse a private room password after a refresh', () => {
201+
window.localStorage.setItem('letscube.roomPassword.v1.private-room', 'saved-password');
200202
const { namespace, store } = buildStore({
201203
roomState: {
202204
...initialRoom(),
@@ -209,10 +211,7 @@ describe('rooms middleware', () => {
209211
store.dispatch(joinRoom({ id: 'private-room' }));
210212

211213
const join = emissionsFor(namespace, Protocol.JOIN_ROOM)[0];
212-
expect(join.args[0]).toEqual({
213-
id: 'private-room',
214-
password: 'saved-password',
215-
});
214+
expect(join.args[0]).toEqual({ id: 'private-room', password: null });
216215

217216
const joinedRoom = {
218217
...initialRoom(),
@@ -223,8 +222,9 @@ describe('rooms middleware', () => {
223222
delete joinedRoom.resultSubmission;
224223
join.args[1](null, joinedRoom);
225224

226-
expect(store.getState().room.password).toBe('saved-password');
227-
expect(readRoomPassword('private-room')).toBe('saved-password');
225+
expect(store.getState().room.password).toBeNull();
226+
expect(readRoomPassword('private-room')).toBeNull();
227+
expect(window.localStorage.getItem('letscube.roomPassword.v1.private-room')).toBeNull();
228228
});
229229

230230
it('remembers a successful private room password', () => {

‎client/src/store/room/roomPasswordStorage.js‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
export const ROOM_PASSWORD_STORAGE_PREFIX = 'letscube.roomPassword.v1.';
22

3+
const passwords = new Map();
4+
35
const storageKey = (roomId) => {
46
if ((typeof roomId !== 'string' && typeof roomId !== 'number')
57
|| String(roomId).length === 0) {
@@ -9,24 +11,33 @@ const storageKey = (roomId) => {
911
return `${ROOM_PASSWORD_STORAGE_PREFIX}${encodeURIComponent(String(roomId))}`;
1012
};
1113

12-
export const readRoomPassword = (roomId, storage = window.localStorage) => {
13-
const password = storage.getItem(storageKey(roomId));
14-
return password || null;
14+
export const readRoomPassword = (roomId) => {
15+
storageKey(roomId);
16+
return passwords.get(String(roomId)) || null;
1517
};
1618

17-
export const persistRoomPassword = (
18-
roomId,
19-
password,
20-
storage = window.localStorage,
21-
) => {
19+
export const persistRoomPassword = (roomId, password) => {
2220
if (typeof password !== 'string' || password.length === 0) {
2321
throw new Error('Cannot save an empty room password.');
2422
}
2523

26-
storage.setItem(storageKey(roomId), password);
24+
storageKey(roomId);
25+
passwords.set(String(roomId), password);
2726
return password;
2827
};
2928

30-
export const clearRoomPassword = (roomId, storage = window.localStorage) => {
31-
storage.removeItem(storageKey(roomId));
29+
export const clearRoomPassword = (roomId) => {
30+
storageKey(roomId);
31+
passwords.delete(String(roomId));
32+
};
33+
34+
export const purgeLegacyRoomPasswords = (storage) => {
35+
if (!storage) return;
36+
37+
for (let index = storage.length - 1; index >= 0; index -= 1) {
38+
const key = storage.key(index);
39+
if (key && key.startsWith(ROOM_PASSWORD_STORAGE_PREFIX)) {
40+
storage.removeItem(key);
41+
}
42+
}
3243
};

‎client/src/store/room/roomPasswordStorage.test.js‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,23 +2,26 @@ import {
22
ROOM_PASSWORD_STORAGE_PREFIX,
33
clearRoomPassword,
44
persistRoomPassword,
5+
purgeLegacyRoomPasswords,
56
readRoomPassword,
67
} from './roomPasswordStorage';
78

89
describe('private room password storage', () => {
910
beforeEach(() => {
1011
window.localStorage.clear();
12+
clearRoomPassword('room-one');
13+
clearRoomPassword('room-two');
1114
});
1215

13-
it('stores passwords separately for each room', () => {
16+
it('keeps passwords in memory for the current tab only', () => {
1417
persistRoomPassword('room-one', 'first-password');
1518
persistRoomPassword('room-two', 'second-password');
1619

1720
expect(readRoomPassword('room-one')).toBe('first-password');
1821
expect(readRoomPassword('room-two')).toBe('second-password');
1922
expect(window.localStorage.getItem(
2023
`${ROOM_PASSWORD_STORAGE_PREFIX}room-one`,
21-
)).toBe('first-password');
24+
)).toBeNull();
2225
});
2326

2427
it('clears a password without affecting other rooms', () => {
@@ -35,4 +38,14 @@ describe('private room password storage', () => {
3538
expect(() => persistRoomPassword(null, 'secret')).toThrow('room ID');
3639
expect(() => persistRoomPassword('room-one', '')).toThrow('empty room password');
3740
});
41+
42+
it('removes passwords saved by older versions', () => {
43+
window.localStorage.setItem(`${ROOM_PASSWORD_STORAGE_PREFIX}room-one`, 'legacy-secret');
44+
window.localStorage.setItem('unrelated', 'keep');
45+
46+
purgeLegacyRoomPasswords(window.localStorage);
47+
48+
expect(window.localStorage.getItem(`${ROOM_PASSWORD_STORAGE_PREFIX}room-one`)).toBeNull();
49+
expect(window.localStorage.getItem('unrelated')).toBe('keep');
50+
});
3851
});

‎cypress/e2e/local_stack.cy.js‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,23 @@
11
describe('local app stack', () => {
22
const apiOrigin = Cypress.env('apiOrigin') || 'http://localhost:8080';
33

4+
const post = (url, body) => cy.request(`${apiOrigin}/api/csrf-token`)
5+
.then(({ body: { csrfToken } }) => cy.request({
6+
body,
7+
headers: { 'x-csrf-token': csrfToken },
8+
method: 'POST',
9+
url,
10+
}));
11+
412
const login = () => {
5-
cy.request('POST', `${apiOrigin}/auth/code`, {
13+
return post(`${apiOrigin}/auth/code`, {
614
code: 'cypress-test-code',
715
redirectUri: 'http://localhost:3000/wca-redirect',
816
});
917
};
1018

1119
const loginAs = (userId) => {
12-
cy.request('POST', `${apiOrigin}/auth/code`, {
20+
return post(`${apiOrigin}/auth/code`, {
1321
code: `cypress-test-user-${userId}`,
1422
redirectUri: 'http://localhost:3000/wca-redirect',
1523
});
@@ -150,7 +158,7 @@ describe('local app stack', () => {
150158

151159
loginAs(recipientId);
152160
loginAs(requesterId);
153-
cy.request('POST', `${apiOrigin}/api/friends/requests`, { userId: recipientId })
161+
post(`${apiOrigin}/api/friends/requests`, { userId: recipientId })
154162
.its('status').should('eq', 201);
155163

156164
loginAs(recipientId);
@@ -180,10 +188,10 @@ describe('local app stack', () => {
180188

181189
loginAs(guestId);
182190
loginAs(hostId);
183-
cy.request('POST', `${apiOrigin}/api/friends/requests`, { userId: guestId })
191+
post(`${apiOrigin}/api/friends/requests`, { userId: guestId })
184192
.its('status').should('eq', 201);
185193
loginAs(guestId);
186-
cy.request('POST', `${apiOrigin}/api/friends/requests/${hostId}/accept`)
194+
post(`${apiOrigin}/api/friends/requests/${hostId}/accept`)
187195
.its('status').should('eq', 200);
188196

189197
loginAs(hostId);

‎server/api.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
const express = require('express');
2+
const rateLimit = require('express-rate-limit');
23

34
const { User } = require('./models');
45
const auth = require('./middlewares/auth.js');
@@ -7,6 +8,7 @@ const createFriendsRouter = require('./api/friends');
78
const createNotificationsRouter = require('./api/notifications');
89
const createUsersRouter = require('./api/users');
910
const { isFeatureEnabled } = require('./features');
11+
const { apiRateLimitOptions } = require('./middlewares/apiRateLimit');
1012

1113
const PREFERENCE_KEYS = new Set([
1214
'showWCAID',
@@ -18,6 +20,7 @@ const PREFERENCE_KEYS = new Set([
1820

1921
module.exports = (app) => {
2022
const router = express.Router();
23+
router.use(rateLimit(apiRateLimitOptions()));
2124
const sendError = (res, err) => {
2225
const body = {
2326
status: err.statusCode,

0 commit comments

Comments
 (0)