Implement systematic parameter validation testing across all server routes #1528

omarraf · 2025-08-19T21:27:47Z

Description

This PR implements a comprehensive parameter validation testing framework across all routes in src/server/routes as part of a major security initiative. The goal is to address these limitations via systematic testing using OED's standard Chai/Mocha frameworks.

This work builds upon #1512, which established the foundational methodology for the units.js route.

Fixes #1497

Type of change

Note merging this changes the database configuration.
This change requires a documentation update

Checklist

I have followed the OED pull request ideas
I have removed text in ( ) from the issue request
You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

omarraf · 2025-08-19T21:38:20Z

Hi @huss, I created this draft PR to get some feedback on the approach before continuing further.

Temporary accommodations made:

Some tests are commented out due to failing
A few test expectations are broadened to handle auth-first middleware flows
Some security limits are conservative estimates vs. actual database constraints

Few questions I had:

Overall approach - does this testing methodology look good?
Security limits - are the hardcoded values reasonable? (1000 char strings, etc.)
Any other immediate concerns?

I just wanted to make sure I'm on the right track before continuing, thanks!

huss

Thanks to @omarraf for another contribution. First, I would like to verify you are the only one who worked on this. I only see commits from you so just double checking. Second, I think the work is good. I read through some of the files to see how it is generally set up. I've made some comments in these files to give my thoughts. Given this is a draft, I wanted to get my thoughts back sooner rather than later and that some overall changes may be made, I did not do a full review. I hope what I did is useful and please let me know if there are specific files you would like me to look at that I did not. If anything is unclear, you have thoughts or need help then please just let me know.

src/server/routes/authenticator.js

src/server/routes/baseline.js

src/server/test/routes/authenticatorParamsTest.js

huss · 2025-08-20T20:07:40Z

src/server/test/routes/ciksParamsTest.js

+
+    mocha.describe('Error Handling', () => {
+        mocha.it('should handle database connection errors gracefully', async () => {
+            const res = await chai.request(app)


Can you help me understand how a DB error would happen so this actually tests that?

The test as written doesn't actually simulate a real database error. It's just making a normal request and expecting it to work. To properly test database error handling, I'd need to either mock the database connection to throw an error or temporarily break the connection somehow. I'll remove this test since it's not actually testing error handling as intended.

Maybe change the description strings so they more accurately reflect the test?

huss · 2025-08-20T20:08:59Z

src/server/test/routes/compareReadingsParamsTest.js

+            });
+
+            mocha.it('should reject invalid meter ID patterns', async () => {
+                const invalidPatterns = [


Does this apply across several routes so should be centralized? Also, could the map pattern be the base for this pattern with the added ones?

src/server/test/routes/compareReadingsParamsTest.js

omarraf · 2025-08-20T20:51:04Z

Hi @huss, I really appreciate you getting back so quickly, I'm going to address all of the comments soon. Just to answer your question real quick:

I would like to verify you are the only one who worked on this

Yes, I ended up working on this alone, as the rest of my team was pulled onto other priorities.

omarraf · 2025-08-26T20:48:21Z

Thanks again for taking the time to give your feedback @huss. I'll work through improving the PR and let you know if I need any help or need a review.

…ts to include 429

…tests to use defined constants

…tring length and improve extra fields validation

… to include 429 for rate limiting

… and oversized parameters until proper handling is implemented

…edge cases

omarraf · 2025-10-27T23:59:10Z

Hi @huss, I've addressed the comments you provided, let me know if I'm missing anything or need me to clarify.

Changes:

Consolidated route schemas to use shared validation constants
Updated parameter tests to reuse the helper utilities, handle rate-limited responses
Temporarily skipped compareReadings DoS checks (TODO added)

Some web/integration tests are failing in CI, which I didn't address in this PR, didn't do enough digging to figure out why

huss · 2025-11-18T18:26:03Z

First, I want to apologize to @omarraf for the long delay in getting to this review. I made a mistake in my task tracking system and lost track of this PR. I'm very sorry this happened.

Second, there are 27 failing tests where most are for Obvius. I think many (all?) are due to changes in route checking. Would it be okay if I try to address them and push out changes?

omarraf · 2025-11-18T19:11:08Z

No problem at all @huss. Feel free to make any changes

- Make them alphabetical in declaration. - Rename string so it comes first so all go together. - Add more HTTP_CODE const responses and use in some places. - Some formatting.

- Use limits in test so always stay in sync. - formatting

Did not change comments. This does not fix other places such as routes. Also, possible some lines might be too long now.

Also formatting.

The new param checking was stricter so the historical use of email instead of the newer username was causing issues. This uses username no matter which is used in the request.

Note other values should be tested but another PR already deals with that.

huss · 2025-11-24T13:46:35Z

I have seen periodic failure of tests on GitHub and the latest tests have 13 failures that are slightly different. The last 7 are 429/too many requests and I suspect the first 6 are OED's rate limiting error (the message is truncated so not certain). The first 6 might be because the new tests runs so fast and there are so many that it is hitting the limit for the first time. The last 7 are less clear but I did see that src/server/routes/obvius.js has a new limit on multer but I have not checked that possibility out. Note these tests do not fail locally for me. This is going to have to be tracked down and I'm open to ideas.

huss

Thanks to @omarraf for the updated code. There are a lot of new tests and I appreciate the work in doing this. Overall, this is working well. I've tried to clear any resolved comments and added some more. I have not checked every test vs the route to verify all is being checked. I think that is a future task either after any changes are done or after this PR. One area with several comments is if tests could be generalized and the basic code reused across tests. For example, notes and maybe many strings are similar across params and routes. I don't know if you want to go into this now and it would be fine if it is done after this PR (not necessarily by you). Another point is that the code from the pending PR that started this process such as src/server/test/util/validationHelpers.js is not showing the git record of their having done the work. Did you change any of it or can it be included via that PR?

Please let me know your thoughts on the big picture items so we can decide how they will be addressed. There are other ones (such as using the HTTP codes throughout the coded as now done with route testing) that I plan to post as issues in the future and not in any comments on this PR.

Finally, I welcome your comments on the commits I made. I changed more than I might normally do because you have put in so much work and my review took so long. I hope this was okay.

huss · 2025-11-18T18:19:31Z

src/server/test/routes/compareReadingsParamsTest.js

+                expect(res.status).to.equal(400);
+            });
+
+            mocha.it.skip('should validate curr_start parameter', async () => { // TODO: re-enable once compareReadings rejects invalid ISO values without hitting DB


OED prefers comments on the line above the code. Applies in several places.

huss · 2025-11-18T21:29:36Z

src/server/test/routes/authenticatorParamsTest.js

+            // Response should not reveal whether user exists or not
+            expect([400, 401]).to.include(res1.status);
+        });
+    });


I'm unclear how this tests rate limiting. Could you help me understand? Also, it raised another questing in my mind if we should rate limit in app.js for login attempts with a lower limit. What do you think?

Yeah looking back at it I think the comment is a bit misleading, it doesn't actually test rate limiting but just checks that all 10 requests fail with 400/401. I can either update the comment or add assertions for 429. Also I think adding a rate limit for logins is a good idea. We could add a seperate rate limiter instance in app.js just for /api/login with a lower threshold. I'm happy to add it in this PR or in a seperate issue.

huss · 2025-11-18T21:30:58Z

src/server/test/routes/authenticatorParamsTest.js

+            // Response should not reveal whether user exists or not
+            expect([400, 401]).to.include(res1.status);
+        });
+    });


There seems a lot of similarities to the user test above. Do you think it would be possible to create a more general test to handle both?

There definitely is overlap, both test username/password validation, malicious inputs, and field length limits. I kept them separate mainly because of different context and test organization. Authenticator tests validate middleware behavior (login endpoint), while user tests validate CRUD operations (/create, /edit, /delete)

That said, we could extract the shared test data (malicious username patterns, SQL injection strings, etc.) into a shared fixture file in src/server/test/util/. I can refactor it that way if need be

huss · 2025-11-18T21:42:46Z

src/server/routes/compareReadings.js

+const ISO_DURATION_REGEX = /^P(?!$)(\d+Y)?(\d+M)?(\d+D)?(T(\d+H)?(\d+M)?(\d+(\.\d+)?S)?)?$/;
+
+function isValidIsoDateTime(value) {
+	return typeof value === 'string' && moment.parseZone(value, moment.ISO_8601, true).isValid();


I see the value of checking if it is a string. However, doesn't the param validation do this before this runs?

Yeah I think you're right, I just added it to be safe but I can remove

huss · 2025-11-23T15:58:18Z

src/server/test/routes/readingsParamsTest.js

+            expect([400, 500]).to.include(res.status);
+        });
+    });
+});


I think these endpoints should be used in the .get and .describe as possible.

huss · 2025-11-23T19:07:20Z

src/server/routes/compareReadings.js

+		const currEndRaw = req.query.curr_end;
+		const shiftRaw = req.query.shift;
+
+		if (!isValidIsoDateTime(currStartRaw) || !isValidIsoDateTime(currEndRaw) || !isValidIsoDuration(shiftRaw)) {


I don't have a problem with the new checks but I want to record that we don't do them in other open routes. This could be made into an issue for the future if you agree.

Yeah sounds good, I'd be open to working on the issue as well if need be

huss · 2025-11-23T21:51:08Z

src/server/test/routes/ciksParamsTest.js

+
+    mocha.describe('Error Handling', () => {
+        mocha.it('should handle database connection errors gracefully', async () => {
+            const res = await chai.request(app)


Maybe change the description strings so they more accurately reflect the test?

huss · 2025-11-24T14:24:59Z

src/server/test/routes/ciksParamsTest.js

+			const res = await chai.request(app)
+				.get('/api/ciks');
+
+			expect([HTTP_CODE.OK, HTTP_CODE.INTERNAL_SERVER_ERROR]).to.include(res.status);


I'm wondering why this and the next test would expect an error code.

I think I added this by mistake, it was probably because I was running the tests locally very frequently so some failed with server errors. I'll clean this up

src/server/test/routes/authenticatorParamsTest.js

omarraf · 2025-11-29T08:15:28Z

Thanks for the thorough review and commits you made @huss , they're very helpful and I appreciate your time to improve the code. The changes you made look good to me and I'll address the rest of your requested changes soon. Some of my thoughts on your questions:

One area with several comments is if tests could be generalized and the basic code reused across tests

Definitely think it makes sense to consolidate similar validation patterns that could share test logic, but I think it'd be a great follow-up task rather than doing it in this PR since it would be a significant refactor and a seperate PR may be easier to review.

Did you change any of it or can it be included via that PR?

Yeah, the validationHelpers.js I added directly from the previous PR on units.js when I first started this issue. I had left a comment at the top of the file conveying the original author and how the current file would be replaced by that PR. Since then I have built on their work and added several new helper functions (validateCommaSeparatedIdPatterns, expectValidCommaSeparatedIds, validateNoExtraFields, and modified testInvalidField to support multiple expected status codes). I want them to get credit for building the foundation so we can potentially take their original commit and add my modifications on top, or however you'd prefer to handle it.

For the bigger picture items I think we should focus on addressing the rate-limiting problem in CI and doing a thorough audit of all tests vs routes making sure we have complete coverage. For future work we can open up a new issue to do the test generalization and http codes throughout the codebase, but I'm happy to implement in this PR if need be.

…ting Merge remote-tracking branch 'origin/development' into systematic-testing

…r improved readability

omarraf · 2025-12-21T16:40:18Z

@huss I addressed some of the comments you left, let me know if there's anything else I can change or clarify.

Since this PR has been open for a while and covers a lot of ground, I'd like to wrap it up soon if there aren't any blocking issues. For the bigger refactoring items we discussed, I'm totally open to tackling those in follow-up issues. Same goes for the systematic http code audit and rate-limiting improvements in CI.

Let me know if there's anything else you'd like me to address in this PR. Happy to keep working on this effort either way.

omarraf · 2025-12-22T20:32:53Z

Regarding the CI test failures, since most appear to be related to rate limiting, would it be possible to configure higher rate limits specifically for the test environment, or is there a preferred approach for handling this in CI? I wanted to check whether that would be considered good practice here.

omarraf added 22 commits August 11, 2025 14:54

add testing for conversions.js

2605d8b

fix failing tests for conversion tests

8993069

authenticator.js param validation and tests

ff6e993

login param validation and tests

502242a

users.js param validation and tests

5c15e07

meters testing and validation

86f6e8a

groups param validation and testing

076f29d

compareReadings param validation and test file

752bc91

maps.js param validation and testing

8d91358

update conversions to use framework

203a881

preferences.js param validation and testing

65f2996

csv.js test file

0efd79e

ciks.js test file

ffea4c3

version.js test

7aa0a6d

improve paramter validation

eaa96dd

response.js test file

69bdb1e

create readings.js param test file

3f0e79d

verification param test file

7848dcb

obvius.js param test file

4d9883b

conversionArray.js param validation and test file

ada8ad3

baseline test file

ae0f440

temporary validation helper file

f3e92ec

huss requested changes Aug 20, 2025

View reviewed changes

omarraf and others added 4 commits August 31, 2025 11:09

use standardized auth function

f5e262c

fix comment placement

6717849

Merge branch 'development' into systematic-testing

eca9979

Merge branch 'development' into systematic-testing

2a00d48

omarraf added 6 commits October 26, 2025 00:56

fix: update status code expectations in authentication validation tes…

ebfc2ed

…ts to include 429

update string length validations in conversions and meters parameter …

30644e3

…tests to use defined constants

update group creation validation tests to use defined constants for s…

cc8624b

…tring length and improve extra fields validation

fix: update expected status codes in group and meter validation tests…

5312c64

… to include 429 for rate limiting

fix: skip parameter validation tests for curr_start, curr_end, shift,…

f9a79eb

… and oversized parameters until proper handling is implemented

fix: update username validation tests to handle undefined and length …

8bfa643

…edge cases

omarraf marked this pull request as ready for review October 27, 2025 23:47

omarraf requested a review from huss October 27, 2025 23:59

huss added 2 commits November 18, 2025 15:07

update string length consts & return code

ff68158

- Make them alphabetical in declaration. - Rename string so it comes first so all go together. - Add more HTTP_CODE const responses and use in some places. - Some formatting.

minor changes to commit

aa686d9

- Use limits in test so always stay in sync. - formatting

huss mentioned this pull request Nov 19, 2025

Unit params tets #1512

Open

5 tasks

huss added 7 commits November 23, 2025 09:38

tab indenting & formatting of tests

d28952c

use HTTP_CODE in tests

5cc8df1

Did not change comments. This does not fix other places such as routes. Also, possible some lines might be too long now.

remove unused imports

f23995d

convert more tests to use const on limits

68ee0f5

Also formatting.

fix obvius route/test

a6bf026

The new param checking was stricter so the historical use of email instead of the newer username was causing issues. This uses username no matter which is used in the request.

add defaultAreaUnit to allowed params for preferences route

83a4df8

fix password length on user test

0c3ad61

Note other values should be tested but another PR already deals with that.

huss requested changes Nov 24, 2025

View reviewed changes

omarraf added 4 commits December 21, 2025 08:20

Merge remote-tracking branch 'origin/development' into systematic-tes…

6b8dffe

…ting Merge remote-tracking branch 'origin/development' into systematic-testing

update comments

5128bcf

Refactor CIKs parameter validation tests to assert only HTTP 200 status

bfa9a1e

Refactor readings parameter validation tests to use base endpoints fo…

d726809

…r improved readability

Implement systematic parameter validation testing across all server routes #1528

Are you sure you want to change the base?

Implement systematic parameter validation testing across all server routes #1528

Uh oh!

Conversation

omarraf commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist

Limitations

Uh oh!

omarraf commented Aug 19, 2025

Uh oh!

huss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omarraf Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

omarraf commented Aug 20, 2025

Uh oh!

omarraf commented Aug 26, 2025

Uh oh!

omarraf commented Oct 27, 2025

Uh oh!

huss commented Nov 18, 2025

Uh oh!

omarraf commented Nov 18, 2025

Uh oh!

huss commented Nov 24, 2025

Uh oh!

huss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omarraf Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

omarraf commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

omarraf commented Aug 19, 2025 •

edited

Loading

omarraf Aug 26, 2025 •

edited

Loading

omarraf Dec 21, 2025 •

edited

Loading

omarraf commented Nov 29, 2025 •

edited

Loading