Double check db queries for security vulnerabilities
Closed this issue · 1 comments
@gregallensworth you're good at spotting these types of things so figured it might be good for you to take a look. I looked briefly but don't recall seeing param checking for the API endpoints which are passed to the db.
parks.js parksNotIn()
Noted that photoCount is unpacked from options but is never used, as well as never supplied.
Action taken:
- Removed this line.
parks.js mostSharedParks_OLD()
This is obsolete and not used, having been replaced with a fixed table of top ten most-shared parks ever. Checking anyway should it be brought back into service.
Confirmed that datesForTime() would properly handle an unexpected value for options.interval
Noted that options.parkCount and options.photoCount are not forced to integers. Noted that they are never passed, and therefore always default values (10 and 10, both). No user-supplied input.
Options:
- Ignore. Does not accept user input, would hypothetically have server-side hardcoded limit if necessary.
- Alter line 198 and 199 to simply hardcode the values.
parks.js getSelectedParkPhotos()
Noted that options.photoCount is not forced to integer, though it is parameterized and in fact is never passed so is always the default (20).
Options:
- Ignore. Does not accept user input, would hypothetically have server-side hardcoded limit if necessary.
- Alter line 283 to simply hardcode the number of photos per "page".
activities.js getParksForActivity()
Noted that options.limit was not forced to integer, though is parameterized.
Noted that no calls to getParksForActivity() ever pass limit so this is always the default (500).
Options:
- Ignore. Does not accept user input, would hypothetically have server-side hardcoded limit if necessary.
- Alter line 42 to simply 'const limit = 500'
parks.js latestPhotoFromMostSharedPark()
Noted that options.photoCount is not forced to integer, though it is parameterized and in fact is never passed so is always the default (20).
This is used to select the X most-shared parks (20 most shared parks) then to find 1 photo from the most-shared of them. I imagine that this was done as a subquery to accommodate the possibility of candidate parks being only a subset of all parks, e.g. "20 most shared within a bounding box" which was not implemented?
Options:
- Ignore. Does not accept user input, would hypothetically have server-side hardcoded limit if necessary.
- Alter line 42 to simply
const photoCount = 20
parks.js parksNotIn()
Noted that options.notIn is not forced to integer. This is supplied by server-side code as the su_id attributes of mostSharedParks() and therefore is known to be numeric ID#s and not user input.
Action taken:
- Confirmed OK.
parks.js parksNotIn()
Noted that dateStr = datesForTime(options.interval) accepts arbitrary user-supplied strings for the interval. Confirmed that datesForTime() has a default which would effectively turn any unexpected interval into "the last week"
Action taken:
- Confirmed OK.
parks.js getSelectedParkPhotos()
Noted that options.offset is not forced to integer here, and does accept user input (server.js '/api/park/:id/photos' line 321) to "page through" the photos.
However, this is parameterized and therefore not problematic.
Action taken:
- Confirmed OK.