Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions client/src/components/Header.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import IconButton from '@mui/material/IconButton';
import Avatar from '@mui/material/Avatar';
import Button from '@mui/material/Button';
import Badge from '@mui/material/Badge';
import { apiOrigin } from '../lib/fetch';
import { lcFetch } from '../lib/fetch';
import { getNameFromId } from '../lib/events';
import { getWcaAuthorizationUrl } from '../lib/wcaAuth';
import NotificationsIcon from '@mui/icons-material/Notifications';
Expand Down Expand Up @@ -73,7 +73,8 @@ function Header({ user, room, notifications }) {
};

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

return (
Expand Down
44 changes: 40 additions & 4 deletions client/src/lib/fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,45 @@

export const apiOrigin = process.env.REACT_APP_API_ORIGIN;

export const lcFetch = (url, options) => (
fetch(`${apiOrigin}${url}`, {
const UNSAFE_METHODS = new Set(['DELETE', 'PATCH', 'POST', 'PUT']);
let csrfTokenRequest;

const requestCsrfToken = async () => {
if (!csrfTokenRequest) {
csrfTokenRequest = fetch(`${apiOrigin}/api/csrf-token`, {
credentials: 'include',
}).then(async (response) => {
if (!response.ok) {
throw new Error('Unable to prepare a secure request.');
}

const { csrfToken } = await response.json();
if (!csrfToken) {
throw new Error('Unable to prepare a secure request.');
}

return csrfToken;
}).catch((error) => {
csrfTokenRequest = null;
throw error;
});
}

return csrfTokenRequest;
};

const withCsrfToken = (headers, csrfToken) => ({
...(headers || {}),
'x-csrf-token': csrfToken,
});

export const lcFetch = async (url, options = {}) => {
const method = (options.method || 'GET').toUpperCase();
const csrfToken = UNSAFE_METHODS.has(method) ? await requestCsrfToken() : null;

return fetch(`${apiOrigin}${url}`, {
...options,
credentials: 'include',
})
);
...(csrfToken ? { headers: withCsrfToken(options.headers, csrfToken) } : {}),
});
};
48 changes: 48 additions & 0 deletions client/src/lib/fetch.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { lcFetch } from './fetch';

describe('lcFetch', () => {
beforeEach(() => {
window.fetch = jest.fn();
});

afterEach(() => {
jest.resetModules();
});

it('adds a server-issued CSRF token to unsafe requests', async () => {
window.fetch
.mockResolvedValueOnce({
json: () => Promise.resolve({ csrfToken: 'csrf-token' }),
ok: true,
})
.mockResolvedValueOnce({ ok: true });

await lcFetch('/api/updatePreference', {
headers: { 'Content-Type': 'application/json' },
method: 'PUT',
});

expect(window.fetch).toHaveBeenNthCalledWith(1, 'undefined/api/csrf-token', {
credentials: 'include',
});
expect(window.fetch).toHaveBeenNthCalledWith(2, 'undefined/api/updatePreference', {
credentials: 'include',
headers: {
'Content-Type': 'application/json',
'x-csrf-token': 'csrf-token',
},
method: 'PUT',
});
});

it('does not fetch a CSRF token for safe requests', async () => {
window.fetch.mockResolvedValue({ ok: true });

await lcFetch('/api/me');

expect(window.fetch).toHaveBeenCalledTimes(1);
expect(window.fetch).toHaveBeenCalledWith('undefined/api/me', {
credentials: 'include',
});
});
});
12 changes: 9 additions & 3 deletions client/src/store/middlewares/rooms.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import {
import {
clearRoomPassword,
persistRoomPassword,
purgeLegacyRoomPasswords,
readRoomPassword,
} from '../room/roomPasswordStorage';
import {
Expand Down Expand Up @@ -91,6 +92,11 @@ export const createRoomsNamespaceMiddleware = ({
ackTimeoutMs = DEFAULT_ACK_TIMEOUT_MS,
retryDelayMs = DEFAULT_RETRY_DELAY_MS,
} = {}) => (store) => {
try {
purgeLegacyRoomPasswords(storage);
} catch {
// Passwords are intentionally kept in memory only.
}
let roomsConnected = false;
let joinedRoomId = null;
let pendingJoinRequest = null;
Expand All @@ -113,15 +119,15 @@ export const createRoomsNamespaceMiddleware = ({

const readStoredRoomPassword = (roomId) => {
try {
return readRoomPassword(roomId, storage);
return readRoomPassword(roomId);
} catch {
return null;
}
};

const forgetRoomPassword = (roomId) => {
try {
clearRoomPassword(roomId, storage);
clearRoomPassword(roomId);
} catch {
// The next invalid join will try again.
}
Expand All @@ -145,7 +151,7 @@ export const createRoomsNamespaceMiddleware = ({
}

try {
persistRoomPassword(roomId, password, storage);
persistRoomPassword(roomId, password);
} catch {
warnPasswordStorage(roomId);
}
Expand Down
16 changes: 8 additions & 8 deletions client/src/store/middlewares/rooms.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
persistPendingResult,
} from '../room/resultOutbox';
import {
clearRoomPassword,
persistRoomPassword,
readRoomPassword,
} from '../room/roomPasswordStorage';
Expand Down Expand Up @@ -161,6 +162,7 @@ const result = {
describe('rooms middleware', () => {
beforeEach(() => {
window.localStorage.clear();
['private-room', 'new-private-room', 'room-one', 'room-two'].forEach(clearRoomPassword);
});

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

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

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

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

expect(store.getState().room.password).toBe('saved-password');
expect(readRoomPassword('private-room')).toBe('saved-password');
expect(store.getState().room.password).toBeNull();
expect(readRoomPassword('private-room')).toBeNull();
expect(window.localStorage.getItem('letscube.roomPassword.v1.private-room')).toBeNull();
});

it('remembers a successful private room password', () => {
Expand Down
33 changes: 22 additions & 11 deletions client/src/store/room/roomPasswordStorage.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
export const ROOM_PASSWORD_STORAGE_PREFIX = 'letscube.roomPassword.v1.';

const passwords = new Map();

const storageKey = (roomId) => {
if ((typeof roomId !== 'string' && typeof roomId !== 'number')
|| String(roomId).length === 0) {
Expand All @@ -9,24 +11,33 @@ const storageKey = (roomId) => {
return `${ROOM_PASSWORD_STORAGE_PREFIX}${encodeURIComponent(String(roomId))}`;
};

export const readRoomPassword = (roomId, storage = window.localStorage) => {
const password = storage.getItem(storageKey(roomId));
return password || null;
export const readRoomPassword = (roomId) => {
storageKey(roomId);
return passwords.get(String(roomId)) || null;
};

export const persistRoomPassword = (
roomId,
password,
storage = window.localStorage,
) => {
export const persistRoomPassword = (roomId, password) => {
if (typeof password !== 'string' || password.length === 0) {
throw new Error('Cannot save an empty room password.');
}

storage.setItem(storageKey(roomId), password);
storageKey(roomId);
passwords.set(String(roomId), password);
return password;
};

export const clearRoomPassword = (roomId, storage = window.localStorage) => {
storage.removeItem(storageKey(roomId));
export const clearRoomPassword = (roomId) => {
storageKey(roomId);
passwords.delete(String(roomId));
};

export const purgeLegacyRoomPasswords = (storage) => {
if (!storage) return;

for (let index = storage.length - 1; index >= 0; index -= 1) {
const key = storage.key(index);
if (key && key.startsWith(ROOM_PASSWORD_STORAGE_PREFIX)) {
storage.removeItem(key);
}
}
};
17 changes: 15 additions & 2 deletions client/src/store/room/roomPasswordStorage.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,26 @@ import {
ROOM_PASSWORD_STORAGE_PREFIX,
clearRoomPassword,
persistRoomPassword,
purgeLegacyRoomPasswords,
readRoomPassword,
} from './roomPasswordStorage';

describe('private room password storage', () => {
beforeEach(() => {
window.localStorage.clear();
clearRoomPassword('room-one');
clearRoomPassword('room-two');
});

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

expect(readRoomPassword('room-one')).toBe('first-password');
expect(readRoomPassword('room-two')).toBe('second-password');
expect(window.localStorage.getItem(
`${ROOM_PASSWORD_STORAGE_PREFIX}room-one`,
)).toBe('first-password');
)).toBeNull();
});

it('clears a password without affecting other rooms', () => {
Expand All @@ -35,4 +38,14 @@ describe('private room password storage', () => {
expect(() => persistRoomPassword(null, 'secret')).toThrow('room ID');
expect(() => persistRoomPassword('room-one', '')).toThrow('empty room password');
});

it('removes passwords saved by older versions', () => {
window.localStorage.setItem(`${ROOM_PASSWORD_STORAGE_PREFIX}room-one`, 'legacy-secret');
window.localStorage.setItem('unrelated', 'keep');

purgeLegacyRoomPasswords(window.localStorage);

expect(window.localStorage.getItem(`${ROOM_PASSWORD_STORAGE_PREFIX}room-one`)).toBeNull();
expect(window.localStorage.getItem('unrelated')).toBe('keep');
});
});
18 changes: 13 additions & 5 deletions cypress/e2e/local_stack.cy.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
describe('local app stack', () => {
const apiOrigin = Cypress.env('apiOrigin') || 'http://localhost:8080';

const post = (url, body) => cy.request(`${apiOrigin}/api/csrf-token`)
.then(({ body: { csrfToken } }) => cy.request({
body,
headers: { 'x-csrf-token': csrfToken },
method: 'POST',
url,
}));

const login = () => {
cy.request('POST', `${apiOrigin}/auth/code`, {
return post(`${apiOrigin}/auth/code`, {
code: 'cypress-test-code',
redirectUri: 'http://localhost:3000/wca-redirect',
});
};

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

loginAs(recipientId);
loginAs(requesterId);
cy.request('POST', `${apiOrigin}/api/friends/requests`, { userId: recipientId })
post(`${apiOrigin}/api/friends/requests`, { userId: recipientId })
.its('status').should('eq', 201);

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

loginAs(guestId);
loginAs(hostId);
cy.request('POST', `${apiOrigin}/api/friends/requests`, { userId: guestId })
post(`${apiOrigin}/api/friends/requests`, { userId: guestId })
.its('status').should('eq', 201);
loginAs(guestId);
cy.request('POST', `${apiOrigin}/api/friends/requests/${hostId}/accept`)
post(`${apiOrigin}/api/friends/requests/${hostId}/accept`)
.its('status').should('eq', 200);

loginAs(hostId);
Expand Down
3 changes: 3 additions & 0 deletions server/api.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const express = require('express');
const rateLimit = require('express-rate-limit');

const { User } = require('./models');
const auth = require('./middlewares/auth.js');
Expand All @@ -7,6 +8,7 @@ const createFriendsRouter = require('./api/friends');
const createNotificationsRouter = require('./api/notifications');
const createUsersRouter = require('./api/users');
const { isFeatureEnabled } = require('./features');
const { apiRateLimitOptions } = require('./middlewares/apiRateLimit');

const PREFERENCE_KEYS = new Set([
'showWCAID',
Expand All @@ -18,6 +20,7 @@ const PREFERENCE_KEYS = new Set([

module.exports = (app) => {
const router = express.Router();
router.use(rateLimit(apiRateLimitOptions()));
const sendError = (res, err) => {
const body = {
status: err.statusCode,
Expand Down
Loading
Loading