shift-org / shift-org/shift-docs

validateRideLength accepts any Object.prototype key as a ride length

Open Beginner friendly
#1,089 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
30
Forks
25
Avg merge
9m
Merged PRs (30d)
1

Description

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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in app/models/calEventValidator.js at validateRideLength and run the node reproduction from the issue in the app directory. Check the own-property validation and local variable behavior against the listed real and prototype-key values. Done means valid ride lengths still pass, prototype keys are rejected, and no global value is created.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
backend, database
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.