Skip to content
Open
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
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
"check:typescript": "./src/scripts/checkTypescript.sh",
"check:types": "tsc -p . --noEmit",
"check:lint": "eslint --ignore-path .eslintignore --ext .ts,.tsx,.js,.jsx ./src/client/app",
"test": "NODE_ENV=test mocha --timeout 15000 \"src/server/test/**/*.js\"",
"test": "npm run noLimitTest && npm run rateTest",
"noLimitTest": "NODE_ENV=test mocha --timeout 15000 \"src/server/test/**/*.js\"",
"rateTest": "NODE_ENV=ratetest mocha --timeout 15000 \"src/server/rateTest/**/*.js\"",
"testsome": "NODE_ENV=test mocha --timeout 15000",
"createdb": "node ./src/server/services/createDB.js",
"developerdb": "node -e 'require(\"./src/server/util/developer.js\").createShiftReadingsFunction()'",
Expand Down
27 changes: 24 additions & 3 deletions src/server/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,12 @@ const ciks = require('./routes/ciks');
// the goal is to avoid hitting rate limiting in testing.
// Note that NODE_ENV of test should only be set for the testing environment and is done in
// package.json in the script section for the test ones.
const isTestEnvironment = process.env.NODE_ENV === 'test';
const testMultiplier = isTestEnvironment ? 100 : 1;
const env = process.env.NODE_ENV;

const isTestEnvironment = env === 'test';
const isRateTest = env === 'ratetest';

const testMultiplier = isTestEnvironment ? 1000 : 1;

// Limit the rate of overall requests to OED
// TODO Verify that user see the message returned, see https://express-rate-limit.mintlify.app/reference/configuration#message
Expand Down Expand Up @@ -116,6 +120,23 @@ const exportRawLimiter = rateLimit({
// Apply the raw export limit
app.use('/api/readings/line/raw/meters', exportRawLimiter);

// Limit the number of login attempts
const loginLimiter = rateLimit({
// Window of 1 hour
windowMs: 60 * 60 * 1000,
/* Rationale: The login route requires a more specifc and strict rate limit that must be tested seperately.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Misspelled: specifc, seperately & enviroment

This is to validate that the rate limit in place is functioning correctly.
If running in the rate test enviroment, limit to 1 request (1 per hour)
Otherwise, use the standard limit based on the configured multiplier. */
limit: isRateTest ? 1 : 900 * testMultiplier,
// Return rate limit info in the `RateLimit-*` headers
standardHeaders: true,
// Disable the `X-RateLimit-*` headers
legacyHeaders: false,
});
//Apply the login limit
app.use('/api/login', loginLimiter);


// If other logging is turned off, there's no reason to log HTTP requests either.
// TODO: Potentially modify the Morgan logger to use the log API, thus unifying all our logging.
Expand All @@ -131,7 +152,7 @@ app.use('/api/users', users);
app.use('/api/meters', meters);
app.use('/api/readings', readings);
app.use('/api/preferences', preferences);
app.use('/api/login', login);
app.use('api/login', login);
app.use('/api/groups', groups);
app.use('/api/verification', verification);
app.use('/api/version', version);
Expand Down
38 changes: 38 additions & 0 deletions src/server/rateTest/rateTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

/* This file currently only tests the login route rate limiting functionality.
It ensures that repeated login attempts are blocked after
exceeding the configured rate limit. */

// TODO: Create additonal rate limits tests for other OED Routes
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

spelling: additonal


const { HTTP_CODE } = require('../util/readingsUtils');
const { chai, mocha, expect, app } = require('../test/common');
const { todo } = require('node:test');

mocha.describe('Login Rate Limit', () => {
mocha.it('Should block repeated login attempts with 429', async () => {
const first = await chai.request(app)
.post('/api/login')
.send({
username: 'invalidUser',
password: 'invalidPassword'
});

//First request that makes it through to authentication
expect(first).to.have.status(HTTP_CODE.UNAUTHORIZED);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test is failing on my machine. It is giving a 404/NOT_FOUND instead of 401/UNAUTHORIZED. I have not looked into why. Do you see that when you do npm run rateTest?

I'm going to put off testing all the new package script choices until this is resolved.


const second = await chai.request(app)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Redoing comment since code moved. I had:

This implicitly assumes the rate is 1 request per a unit of time and that the test runs fast enough to not cross that amount of time between the first and second attempt. That should be okay but a comment should make it explicit. Note if the time frame of the limit is changed as suggested in another comment (to per hour) then the odds of this happening goes to zero but the rationale should still be commented.

I think discussing here that the limit should be 1 for an hour so this fails with a comment has value.

.post('/api/login')
.send({
username: 'invalidUser',
password: 'invalidPassword'
});

//Second request that triggers the rate limit of 1 request per hour
expect(second).to.have.status(HTTP_CODE.TOO_MANY_REQUESTS);
expect(second.text).to.include('Too many requests');
});
});