Reject prototype keys in validateRideLength (#1089) - #1090
Open
gangster wants to merge 1 commit into
Open
Conversation
The ride length check used the 'in' operator, which walks the prototype chain, so "toString", "constructor", "hasOwnProperty", "valueOf" and "__proto__" were all accepted as valid ride lengths and returned unchanged. They persist to the calevent row and EventDetails renders the field verbatim, so an event could display, for example, "constructor miles". Switch to Object.hasOwn so only the four declared lengths pass. Also declare the local: 'value' was assigned without const/let and so became an implicit global on every call, which would throw under strict mode or ESM. Rename the parameter to 'field' to match the other validators, which all take a field name rather than a value. Adds three cases to validator_test.js. Two of them fail before this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1089.
validateRideLengthtested the submitted value within, which walks the prototype chain, sotoString,constructor,hasOwnProperty,valueOfand__proto__were all accepted as valid ride lengths and stored verbatim.EventDetails.vuerenders the field directly, so an event could display "constructor miles".Three changes in those three lines:
Object.hasOwninstead ofin, so only the four declared lengths pass.const value— it was previously assigned with no declaration and became an implicit global on every call (globalThis.value === "0-3"after one call). Harmless under non-strict CommonJS, aReferenceErrorunder ESM or"use strict".rideLengthtofield, to match every other validator in the file, which all take a field name rather than a value.Tests
Three cases added to the existing
validator_test.js. Two of them fail before this change — I stashed the source fix to confirm they aren't vacuous:End-to-end verification
Unit tests alone did not seem like enough for a validation change, so I restarted the dev server on the patched code and re-ran the same
manage_eventround trip used to find this, reading each result back throughretrieve_event:ridelengthconstructor"constructor"null__proto__nulltoStringnull0-3"0-3""0-3"bogusnullnullThe
0-3row is the one that matters: prototype keys are rejected without over-rejecting legitimate values. Loading the event page with0-3set renders "0-3 miles" as expected, and the prototype-key text is gone.Not changed
Unlike the other validators, this one silently returns
nullfor an unrecognised value rather than recording an error througherrors.addError(field). That may be deliberate — rejecting a bad ride length outright would fail the whole save — so the behaviour is unchanged here. Flagging it in case it is worth revisiting separately.Object.hasOwnneeds Node 16.9+;docker-compose.ymlpinsnode:24.15.0-slim. There is no existinghasOwn/hasOwnPropertyusage in the codebase, so happy to switch toObject.prototype.hasOwnProperty.call(...)if you would rather stay conservative.