Skip to content

validateRideLength accepts any Object.prototype key as a ride length #1089

Description

@gangster

validateRideLength uses the in operator to check a submitted ride length against the known values. in walks the prototype chain, so alongside the four real lengths it also accepts every property inherited from Object.prototypetoString, constructor, hasOwnProperty, valueOf, __proto__ — and returns them unchanged, with no validation error.

Those values reach the database and are rendered back on the event page.

Reproducing

From the app directory:

node -e "
const { makeValidator, ErrorCollector } = require('./models/calEventValidator');
for (const v of ['0-3', 'bogus', 'toString', 'constructor', '__proto__']) {
  const errors = new ErrorCollector();
  const got = makeValidator({ ridelength: v }, errors).validateRideLength('ridelength');
  console.log(JSON.stringify(v).padEnd(15), '->', JSON.stringify(got));
}"

On main this prints:

"0-3"           -> "0-3"          (correct)
"bogus"         -> null           (correctly rejected)
"toString"      -> "toString"     (should be null)
"constructor"   -> "constructor"  (should be null)
"__proto__"     -> "__proto__"    (should be null)

hasOwnProperty and valueOf behave the same way. None of them record a validation error.

It reaches the database and the page

Posting an otherwise valid event to manage_event with ridelength: "constructor" (payload shape per CALENDAR_API.md) returns 200, and reading the event back through retrieve_event confirms it stored:

ridelength: "constructor"

EventDetails.vue renders the field directly:

rideLength() {
  if (this.evt.ridelength) {
    return `${this.evt.ridelength} miles`;
  }

so the event details page then displays "constructor miles".

Cause

app/models/calEventValidator.js:

validateRideLength(rideLength) {
  value = getString(rideLength);
  return (value in RideLength) ? value : null;
}

RideLength is an ordinary frozen object, so it still inherits from Object.prototype, and in finds those inherited keys.

A second, smaller problem in the same function

value is assigned without const/let, so it becomes an implicit global on every call — confirmed with globalThis.value === "0-3" after one call. It is harmless today only because this file is non-strict CommonJS; it would throw a ReferenceError under ESM or "use strict", which is where the rest of the project is heading.

Suggested fix

Use an own-property check, and declare the local:

-    validateRideLength(rideLength) {
-      value = getString(rideLength);
-      return (value in RideLength) ? value : null;
-    },
+    validateRideLength(field) {
+      const value = getString(field);
+      return Object.hasOwn(RideLength, value) ? value : null;
+    },

Severity

Low. The column is a varchar, and Vue escapes its output, so this is bad data rather than injection — an organizer can write arbitrary prototype-key strings into a public field on their own ride. Worth fixing because it is small, and because the same in-instead-of-own-property pattern is easy to copy into somewhere it matters more.

Not changed

Unlike the other validators, this one silently returns null for an unrecognised value rather than recording an error via errors.addError(field). That may well be deliberate — rejecting a bad ride length outright would fail the whole save — so I have left the behaviour alone. Flagging it in case it should be revisited separately.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions