Skip to content

Commit aa24029

Browse files
committed
Modular: rate limiter allows twice as many OPTIONS requests, since they can't be authenticated
Also tweaks rate limit parameters
1 parent 04853be commit aa24029

File tree

4 files changed

+24
-12
lines changed

4 files changed

+24
-12
lines changed

lib/appFactory.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,10 @@ module.exports = async function ({ hostIdentity, jwtSecret, accountMgr, storeRou
144144

145145
app.use([`${basePath}/`, `${basePath}/oauth`, `${basePath}/account`, `${basePath}/admin`], memorySession);
146146

147-
app.use((req, _res, next) => {
147+
app.use(async (req, _res, next) => {
148148
if (req.session?.privileges?.STORE) {
149149
// refunds the points consumed by rate-limiting middleware
150-
rateLimiterReward(req.ip, POINTS_UNAUTH_REQUEST - POINTS_AUTH_REQUEST);
150+
await rateLimiterReward(req.ip, POINTS_UNAUTH_REQUEST - POINTS_AUTH_REQUEST);
151151
}
152152
next();
153153
});

lib/middleware/rateLimiterMiddleware.js

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,18 @@
99
// per-instance.)
1010

1111
const { debuglog } = require('node:util');
12+
const errToMessages = require('../util/errToMessages');
1213
const { loggingMiddleware } = require('../logger');
1314
const { RateLimiterMemory, BurstyRateLimiter } = require('rate-limiter-flexible');
1415

1516
const POINTS_AUTH_REQUEST = 1;
16-
const POINTS_UNAUTH_REQUEST = 50;
17+
const POINTS_UNAUTH_REQUEST = 35;
1718
// OAuth page requires 9 GETs + 1 POST
1819
// root page + login = 17 requests
1920
const MAX_BURST_POINTS = 19 * POINTS_UNAUTH_REQUEST;
2021
// requesting an invitation is 7 requests
21-
const SUSTAINED_POINTS_PER_SEC = 8 * POINTS_UNAUTH_REQUEST;
22+
// allows (10 - 1) * 35 = 315 authorized req/sec
23+
const SUSTAINED_POINTS_PER_SEC = 10 * POINTS_UNAUTH_REQUEST;
2224

2325
// remotestorage.js appears to keep requesting until 10 failures
2426

@@ -80,12 +82,17 @@ const rateLimiterMiddleware = async (req, res, next) => {
8082
await rateLimiter.consume(req.ip, POINTS_UNAUTH_REQUEST);
8183
next();
8284
} catch (err) {
83-
if (debugEnabled) {
84-
await new Promise(resolve => loggingMiddleware(req, res, resolve));
85-
res.logNotes.add(`consumedPoints=${err.consumedPoints} msBeforeNext=${err.msBeforeNext}`);
85+
if (err instanceof Error) {
86+
errToMessages(err, res.logNotes);
87+
res.status(500).end();
88+
} else {
89+
if (debugEnabled) {
90+
await new Promise(resolve => loggingMiddleware(req, res, resolve));
91+
res.logNotes.add(`consumedPoints=${err.consumedPoints} msBeforeNext=${err.msBeforeNext}`);
92+
}
93+
res.set({ 'Retry-After': Math.ceil(err.msBeforeNext / 1000) });
94+
res.status(429).end();
8695
}
87-
res.set({ 'Retry-After': Math.ceil(err.msBeforeNext / 1000) });
88-
res.status(429).end();
8996
}
9097
};
9198

lib/routes/S3_store_router.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1100,7 +1100,7 @@ module.exports = function ({ endPoint = 'play.min.io', accessKey = 'Q3AM3UQ867SP
11001100
// Upload allows you to pass ContentLength, but then fails for streams longer than about 5.1MB, with a message about XML.
11011101

11021102
parallelUpload.on('httpUploadProgress', (progress) => { // TODO: does this add value?
1103-
console.debug(bucketName, s3Path, `part ${progress.part} ${progress.loaded?.toLocaleString()} / ${progress.total?.toLocaleString()} bytes`);
1103+
console.debug(bucketName, s3Path, `: part ${progress.part}: ${progress.loaded?.toLocaleString()} / ${progress.total?.toLocaleString()} bytes`);
11041104
});
11051105

11061106
const uploadResponse = await parallelUpload.done();

lib/routes/storage_common.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,12 @@ module.exports = function (hostIdentity, jwtSecret) {
2121
});
2222

2323
router.options('/:username/*',
24-
(_req, res, next) => { res.logLevel = 'debug'; next(); },
24+
async (req, res, next) => {
25+
// not authenticated, but no disk nor network access required, so we can allow more
26+
await rateLimiterReward(req.ip, Math.round(POINTS_UNAUTH_REQUEST / 2));
27+
res.logLevel = 'debug';
28+
next();
29+
},
2530
corsAllowPrivate,
2631
corsRS
2732
);
@@ -149,7 +154,7 @@ module.exports = function (hostIdentity, jwtSecret) {
149154
return [requiredScopeName, '*'].includes(grantedParts[0]) && grantedParts[1].startsWith(requiredPermission);
150155
})) {
151156
// refunds the points consumed by rate-limiter middleware
152-
rateLimiterReward(req.ip, POINTS_UNAUTH_REQUEST - POINTS_AUTH_REQUEST);
157+
await rateLimiterReward(req.ip, POINTS_UNAUTH_REQUEST - POINTS_AUTH_REQUEST);
153158
next();
154159
} else {
155160
return unauthorized(req, res, 403, 'insufficient_scope', requiredScope, `user has permissions '${grantedScopes}' but lacks '${requiredScope}'`);

0 commit comments

Comments
 (0)