-
Notifications
You must be signed in to change notification settings - Fork 468
proper validation of passed ids #1592
Copy link
Copy link
Open
Labels
p-medium-priorityt-bugThe issue is related to an aspect of the software which is incorrectThe issue is related to an aspect of the software which is incorrect
Description
Describe the bug
The following routes send values after the route name using the colon (:) syntax (found by searching for /:):
- src/server/routes/compareReadings.js
- src/server/routes/groups.js
- src/server/routes/maps.js
- src/server/routes/meters.js
- src/server/routes/readings.js
- src/server/routes/unitReadings.js
- src/server/routes/users.js
When the values are validated, it uses a string match that does not exclude all bad values. There are two possible (maybe more) fixes:
- Enhance the validation on these strings which can include comma separated values.
- Send the values as parameters inside the request as done for all other values. This would allow for direct validation as an array of integer values and (I think) min of 1.
Personally, I think the second option might be better but here are some considerations:
- Was there a good reason it was done this way or is it a historical artifact from early on in the project?
- Does this impact any current scripts or endpoints that are directly used outside the OED web app? This needs to be determined and a plan if it is the case.
- Testing should be done to verify the route is fairly easy and the validation is too. This could be done for a sample route with comma separated values.
- All the uses in OED need to be updated to reflect this change. This should be easier now that OED is mostly using the Redux Toolkit. Note maps have a long-standing PR (PR Maps Update: Cards and Modals for Maps Components #1314) that would need to be rolled in if it is not yet merged.
- All the new tests need to check/validate this. PR Refactor test parameter validation into shared helpers #1583 has a TODO in src/server/test/routes/groupsParamsTest.js that indicates the group tests also return 200 for bad values (it should not). This is one place but all tests related to the routes updated would need updating. This includes route tests but also general tests that send these values.
Expected behavior
OED should properly validate all values sent. OED is now mostly doing that and this would be a remaining item.
Additional context
None
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
p-medium-priorityt-bugThe issue is related to an aspect of the software which is incorrectThe issue is related to an aspect of the software which is incorrect