Skip to content

Commit

Permalink
Adds rate-limiting to defend against buggy and malicious clients
Browse files Browse the repository at this point in the history
  • Loading branch information
DougReeder committed Jul 18, 2024
1 parent 18b43d8 commit aabd26a
Show file tree
Hide file tree
Showing 10 changed files with 102 additions and 15 deletions.
29 changes: 22 additions & 7 deletions lib/appFactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const accountRouterFactory = require('./routes/account');
const adminFactory = require('./routes/admin');
const session = require('express-session');
const MemoryStore = require('memorystore')(session);
const { rateLimiterPenalty, rateLimiterBlock, rateLimiterMiddleware } = require('./middleware/rateLimiterMiddleware');

module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRouter, basePath = '' }) {
if (basePath && !basePath.startsWith('/')) { basePath = '/' + basePath; }
Expand All @@ -34,14 +35,18 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou

app.set('accountMgr', accountMgr);

// web browsers ask for this way too often
app.get('/favicon.ico', (req, res, _next) => {
// web browsers ask for this way too often, so doesn't log this
app.get(['/favicon.ico', '/apple-touch-icon*'], (req, res, _next) => {
res.set('Cache-Control', 'public, max-age=31536000');
res.status(404).end();
});

app.use(loggingMiddleware);

if (process.env.NODE_ENV === 'production') {
app.use(rateLimiterMiddleware);
}

app.use(robots(path.join(__dirname, 'robots.txt')));

const helmetStorage = helmet({
Expand Down Expand Up @@ -100,9 +105,10 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou
app.use(`${basePath}/assets`, express.static(path.join(__dirname, 'assets'), {
fallthrough: true, index: false, maxAge: '25m'
}));
app.use(`${basePath}/assets`, (req, res, next) => {
app.use(`${basePath}/assets`, async (req, res, _next) => {
res.set('Cache-Control', 'public, max-age=1500');
res.status(404).end();
await rateLimiterPenalty(req.ip);
});

app.use(`${basePath}/signup`, requestInviteRouter(storeRouter));
Expand Down Expand Up @@ -148,27 +154,36 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou
app.use(`${basePath}/admin`, admin);

// catches paths not handled and returns Not Found
app.use(basePath, function (req, res) {
app.use(basePath, async function (req, res) {
if (!res.get('Cache-Control')) {
res.set('Cache-Control', 'max-age=1500');
}
const subpath = req.path.slice(basePath.length).split('/')?.[1];
const name = req.path.slice(1);
errorPage(req, res, 404, { title: 'Not Found', message: `“${name}” doesn't exist` });
if (['storage', 'oauth', 'account', 'admin'].includes(subpath)) { // reasonable
errorPage(req, res, 404, { title: 'Not Found', message: `“${name}” doesn't exist` });
} else { // probably hostile
res.logNotes.add(`“${name}” shouldn't and doesn't exist`);
res.status(404).end();
await rateLimiterBlock(req.ip, 61);
}
});

// redirect for paths outside the app
app.use(function (req, res) {
app.use(async function (req, res) {
res.status(308).set('Location', basePath).end();
await rateLimiterPenalty(req.ip);
});

// error handler
app.use(function (err, req, res, _next) {
app.use(async function (err, req, res, _next) {
const message = err?.message || err?.errors?.find(e => e.message).message || err?.cause?.message || 'indescribable error';
errorPage(req, res, err.status || 500, {
title: shorten(message, 30),
message,
error: req.app.get('env') === 'development' ? err : {}
});
await rateLimiterPenalty(req.ip);
});

setTimeout(() => {
Expand Down
42 changes: 42 additions & 0 deletions lib/middleware/rateLimiterMiddleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
const { RateLimiterMemory, BurstyRateLimiter } = require('rate-limiter-flexible');

const SUSTAINED_REQ_PER_SEC = 8;
const MAX_BURST = 50; // remotestorage.js appears to keep requesting until 10 failures

const rateLimiterSustained = new RateLimiterMemory({
points: SUSTAINED_REQ_PER_SEC,
duration: 1
});

const rateLimiterBurst = new RateLimiterMemory({
keyPrefix: 'burst',
points: MAX_BURST - SUSTAINED_REQ_PER_SEC,
duration: 10
});

async function rateLimiterPenalty (key, points = 1) {
await rateLimiterSustained.penalty(key, points);
await rateLimiterBurst.penalty(key, points);
}

async function rateLimiterBlock (key, secDuration) {
await rateLimiterSustained.block(key, secDuration);
await rateLimiterBurst.block(key, secDuration);
}

const rateLimiter = new BurstyRateLimiter(
rateLimiterSustained,
rateLimiterBurst
);

const rateLimiterMiddleware = async (req, res, next) => {
try {
await rateLimiter.consume(req.ip);
next();
} catch (err) {
res.set({ 'Retry-After': Math.ceil(err.msBeforeNext / 1000) });
res.status(429).end();
}
};

module.exports = { rateLimiterPenalty, rateLimiterBlock, rateLimiterMiddleware };
4 changes: 3 additions & 1 deletion lib/middleware/sanityCheckUsername.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
/** sanity check of username, to defend against ".." and whatnot */
module.exports = function sanityCheckUsername (req, res, next) {
const { rateLimiterBlock } = require('./rateLimiterMiddleware');
module.exports = async function sanityCheckUsername (req, res, next) {
const username = req.params.username || req.data.username || '';
if (username.length > 0 && !/\/|^\.+$/.test(username) && /[\p{L}\p{Nd}]{1,63}/u.test(username)) {
return next();
}

res.logNotes.add('invalid user');
res.status(400).end();
await rateLimiterBlock(req.ip, 61);
};
6 changes: 5 additions & 1 deletion lib/routes/admin.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const { initProtocols, assembleContactURL, calcContactURL, protocolOptions } = r
const nameFromUseragent = require('../util/nameFromUseragent');
const removeUserDataFromSession = require('../util/removeUserDataFromSession');
const ParameterError = require('../util/ParameterError');
const { rateLimiterBlock, rateLimiterPenalty } = require('../middleware/rateLimiterMiddleware');

/* eslint no-unused-vars: ["warn", { "varsIgnorePattern": "^_", "argsIgnorePattern": "^_" }] */
/* eslint-disable no-case-declarations */
Expand Down Expand Up @@ -140,6 +141,7 @@ module.exports = async function (hostIdentity, jwtSecret, accountMgr, storeRoute
});

if (!verified) {
await rateLimiterPenalty(req.ip);
throw new Error('Verification failed.');
}

Expand Down Expand Up @@ -398,6 +400,7 @@ module.exports = async function (hostIdentity, jwtSecret, accountMgr, storeRoute
if (!(req.session.privileges?.ADMIN || (req.session.user && req.body.contacturl === req.session.user?.contactURL))) {
res.logNotes.add('session does not have ADMIN privilege');
res.status(401).type('text/plain').send('Ask an admin to send the invite');
await rateLimiterBlock(req.ip, 61);
return;
}

Expand Down Expand Up @@ -441,9 +444,10 @@ module.exports = async function (hostIdentity, jwtSecret, accountMgr, storeRoute
res.logNotes.add('session does not have ADMIN privilege');
if (['GET', 'HEAD'].includes(req.method)) {
res.redirect(307, './login');
} else {
} else { // TODO consider deleting this unused code
res.logNotes.add('session lacks ADMIN privilege');
res.status(401).end();
// await rateLimiterBlock(req.ip, 61);
}
}
}
Expand Down
6 changes: 4 additions & 2 deletions lib/routes/login.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const loginOptsWCreds = require('../util/loginOptsWCreds');
const removeUserDataFromSession = require('../util/removeUserDataFromSession');
const verifyCredential = require('../util/verifyCredential');
const updateSessionPrivileges = require('../util/updateSessionPrivileges');
const { rateLimiterPenalty } = require('../middleware/rateLimiterMiddleware');

module.exports = async function (hostIdentity, jwtSecret, account, isAdminLogin) {
const rpID = hostIdentity;
Expand Down Expand Up @@ -78,10 +79,11 @@ module.exports = async function (hostIdentity, jwtSecret, account, isAdminLogin)

errToMessages(err, res.logNotes);
if (['Error', 'NoSuchUserError'].includes(err.name)) {
return res.status(401).json({ msg: 'Your passkey could not be validated' });
res.status(401).json({ msg: 'Your passkey could not be validated' });
} else {
return res.status(500).json({ msg: 'If this problem persists, contact an administrator.' });
res.status(500).json({ msg: 'If this problem persists, contact an administrator.' });
}
await rateLimiterPenalty(req.ip);
}
});

Expand Down
3 changes: 3 additions & 0 deletions lib/routes/oauth.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const verifyCredential = require('../util/verifyCredential');
const removeUserDataFromSession = require('../util/removeUserDataFromSession');
const updateSessionPrivileges = require('../util/updateSessionPrivileges');
const errorPage = require('../util/errorPage');
const { rateLimiterPenalty, rateLimiterBlock } = require('../middleware/rateLimiterMiddleware');

const accessStrings = { r: 'Read', rw: 'Read/write' };

Expand Down Expand Up @@ -83,13 +84,15 @@ module.exports = function (hostIdentity, jwtSecret, account) {

await updateSessionPrivileges(req, user, false);
if (!req.session.privileges.STORE) {
await rateLimiterPenalty(req.ip);
throw new Error('STORE privilege required to grant access');
}

let redirectOrigin;
try {
redirectOrigin = new URL(req.session.oauthParams.redirect_uri).origin;
} catch (err) {
await rateLimiterBlock(req.ip, 61);
throw new Error('Application origin is bad', { cause: err });
}

Expand Down
11 changes: 11 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"mkdirp": "^1.0.4",
"node-mocks-http": "^1.14.1",
"proquint": "^0.0.1",
"rate-limiter-flexible": "^5.0.3",
"robots.txt": "^1.1.0",
"winston": "^3.11.0",
"yaml": "^2.4.0"
Expand Down
11 changes: 9 additions & 2 deletions spec/modular/m_not_found.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,21 @@ describe('Nonexistant resource (modular)', function () {

/** This extends the test in shouldHandleNonexistingResource */
it('should say cacheable for 25 minutes', async function () {
const res = await chai.request(this.app).get('/fizbin');
const res = await chai.request(this.app).get('/account/wildebeest/');
expect(res).to.have.status(404);
expect(res).to.have.header('Cache-Control', /max-age=\d{4}/);
expect(res).to.have.header('Strict-Transport-Security', /^max-age=/);
expect(res).to.have.header('ETag');

expect(res.text).to.contain('<title>Not Found — Armadietto</title>');
expect(res.text).to.contain('>“fizbin” doesn&#39;t exist<');
expect(res.text).to.contain('>“account/wildebeest/” doesn&#39;t exist<');
});

it('should curtly & cache-ably refuse to serve unlikely paths', async function () {
const res = await chai.request(this.app).get('/_profiler/phpinfo');
expect(res).to.have.status(404);
expect(res).to.have.header('Cache-Control', /max-age=\d{4}/);
expect(res.text).to.equal('');
});

/** This tests that 404 for nonexistent assets is cache-able */
Expand Down
4 changes: 2 additions & 2 deletions spec/not_found.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ chai.use(chaiHttp);

exports.shouldHandleNonexistingResource = function () {
it('should return 404 Not Found', async function () {
const res = await chai.request(this.app).get('/zorp/gnu/');
const res = await chai.request(this.app).get('/account/wildebeest/');
expect(res).to.have.status(404);
expect(res).to.have.header('Content-Security-Policy', /sandbox.*default-src 'self'/);
expect(res).to.have.header('Referrer-Policy', 'no-referrer');
Expand All @@ -21,7 +21,7 @@ exports.shouldHandleNonexistingResource = function () {
// expect(res.text).to.contain('<title>Not Found — Armadietto</title>');
expect(res.text).to.match(/<h\d>Something went wrong<\/h\d>/);
expect(res.text).to.contain('>404<');
expect(res.text).to.contain('>“zorp/gnu/” doesn&#39;t exist<');
expect(res.text).to.contain('>“account/wildebeest/” doesn&#39;t exist<');

// navigation
expect(res.text).to.match(/<a [^>]*href="\/"[^>]*>Home<\/a>/);
Expand Down

0 comments on commit aabd26a

Please sign in to comment.