Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rate limit getting device logs #1430

Closed
wants to merge 1 commit into from
Closed
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
13 changes: 13 additions & 0 deletions src/features/device-logs/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@ const deviceLogsRateLimiter = createRateLimitMiddleware(
},
);

// Rate limit device log get requests
const streamableDeviceLogsRateLimiter = createRateLimitMiddleware(
createRateLimiter('get-device-logs', {
points: 10, // allow 10 device log streams / get requests
blockDuration: 60, // seconds
duration: 60, // reset counter after 60 seconds (from the first batch of the window)
}),
{
ignoreIP: true,
},
);

export const setup = (
app: Application,
onLogWriteStreamInitialized: SetupOptions['onLogWriteStreamInitialized'],
Expand All @@ -30,6 +42,7 @@ export const setup = (
app.get(
'/device/v2/:uuid/logs',
middleware.fullyAuthenticatedUser,
streamableDeviceLogsRateLimiter(['params.uuid', 'query.stream']),
read(onLogReadStreamInitialized),
);
app.post(
Expand Down
20 changes: 14 additions & 6 deletions src/infra/rate-limiting/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export type RateLimitKeyFn = (
req: Request,
res: Response,
) => Resolvable<string>;
export type RateLimitKey = string | RateLimitKeyFn;
export type RateLimitKey = string | RateLimitKeyFn | string[];

export type RateLimitMiddleware = (
...args: Parameters<RequestHandler>
Expand Down Expand Up @@ -126,14 +126,22 @@ const $createRateLimitMiddleware = (
ignoreIP = false,
allowReset = true,
}: { ignoreIP?: boolean; allowReset?: boolean } = {},
field?: RateLimitKey,
fields?: RateLimitKey,
): RateLimitMiddleware => {
let fieldFn: RateLimitKeyFn;
if (field != null) {
if (typeof field === 'function') {
fieldFn = field;
if (fields != null) {
if (typeof fields === 'function') {
fieldFn = fields;
} else if (Array.isArray(fields)) {
fieldFn = (req) =>
fields
.map((field) => {
const path = _.toPath(field);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This _.toPath must occur only once in the setup phase as it is fairly costly when in very hot code (eg in rate limiting code that potentially runs for every request). If we didn't care about pre-calculating the path in order to do it only once then we would instead do _.get(req, field); and let lodash do it automatically each time

return _.get(req, path);
})
.join('$');
} else {
const path = _.toPath(field);
const path = _.toPath(fields);
fieldFn = (req) => _.get(req, path);
}
} else {
Expand Down
76 changes: 76 additions & 0 deletions test/06_device-log.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,4 +222,80 @@ describe('device log', () => {
'streamed log line 1',
]);
});

it('should rate limit device logs read streams', async () => {
const dummyLogs = [createLog({ message: 'not rate limited' })];
await supertest(ctx.device.apiKey)
.post(`/device/v2/${ctx.device.uuid}/logs`)
.send(dummyLogs)
.expect(201);

async function testRatelimitedDeviceLogsStream() {
let evalValue;
const req = supertest(ctx.user)
.get(`/device/v2/${ctx.device.uuid}/logs`)
.query({
stream: 1,
count: 1,
})
.parse(function (res, callback) {
res.on('data', async function (chunk) {
const parsedChunk = JSON.parse(Buffer.from(chunk).toString());
evalValue = parsedChunk;
// if data stream provides proper data terminate the stream (abort)
if (
typeof parsedChunk === 'object' &&
parsedChunk?.message === 'not rate limited'
) {
req.abort();
}
});
res.on('close', () => callback(null, null));
});

try {
await req;
} catch (error) {
if (error.code !== 'ABORTED') {
throw error;
}
}
return evalValue;
}

const notLimited = await testRatelimitedDeviceLogsStream();
expect(notLimited?.['message']).to.deep.equal(dummyLogs[0].message);

while ((await testRatelimitedDeviceLogsStream()) !== 'Too Many Requests') {
// no empty block
}
const rateLimited = await testRatelimitedDeviceLogsStream();
expect(rateLimited).to.be.string('Too Many Requests');
});

it('should rate limit device logs get requests', async () => {
const dummyLogs = [createLog({ message: 'not rate limited' })];
await supertest(ctx.device.apiKey)
.post(`/device/v2/${ctx.device.uuid}/logs`)
.send(dummyLogs)
.expect(201);

async function testRatelimitedDeviceLogs() {
return supertest(ctx.user)
.get(`/device/v2/${ctx.device.uuid}/logs`)
.query({
stream: 0,
count: 1,
});
}

const notLimited = await testRatelimitedDeviceLogs();
expect(notLimited.status).to.be.equal(200);

while ((await testRatelimitedDeviceLogs()).status !== 429) {
// no empty block
}
const rateLimited = await testRatelimitedDeviceLogs();
expect(rateLimited.status).to.be.equal(429);
});
});
Loading