From fe0bd7d7539b3d303cc297817c0bca0c51a9315e Mon Sep 17 00:00:00 2001
From: Josh Deeden <git@deeden.org>
Date: Wed, 5 Aug 2026 11:38:16 -0700
Subject: [PATCH] Reject prototype keys in validateRideLength (#1089)

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.
---
 app/models/calEventValidator.js |  9 ++++++---
 app/test/validator_test.js      | 35 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/app/models/calEventValidator.js b/app/models/calEventValidator.js
index 23980ae0..2afd6d2b 100644
--- a/app/models/calEventValidator.js
+++ b/app/models/calEventValidator.js
@@ -187,9 +187,12 @@ function makeValidator(input, errors) {
       return validStatus;
     },
 
-    validateRideLength(rideLength) {
-      value = getString(rideLength);
-      return (value in RideLength) ? value : null;
+    // if not specified ( or not one of the known lengths ) returns null.
+    // note: uses hasOwn, not 'in': 'in' walks the prototype chain,
+    // which would accept "toString", "constructor", etc. as ride lengths.
+    validateRideLength(field) {
+      const value = getString(field);
+      return Object.hasOwn(RideLength, value) ? value : null;
     },
   };
 }
diff --git a/app/test/validator_test.js b/app/test/validator_test.js
index b9e2938d..4ce1ccea 100644
--- a/app/test/validator_test.js
+++ b/app/test/validator_test.js
@@ -1,4 +1,5 @@
 const { ErrorCollector, makeValidator } = require("../models/calEventValidator");
+const { RideLength } = require("../models/calConst");
 const { describe, it } = require("node:test");
 const assert = require("node:assert/strict");
 
@@ -48,4 +49,38 @@ describe('event field validation', () => {
     assert.ok(msg.key);
     assert.equal(msg.key, `Please enter a value for <span class="field-name">key</span>`);
   });
+  it('ride length validator should accept the known lengths', () => {
+    for (const want of Object.keys(RideLength)) {
+      const errors = new ErrorCollector();
+      const v = makeValidator({ ridelength: want }, errors);
+      assert.equal(v.validateRideLength('ridelength'), want);
+      assert.equal(errors.count, 0);
+    }
+  });
+  it('ride length validator should reject anything else', () => {
+    const list = [
+      "bogus",
+      "",
+      null,
+      // these are inherited from Object.prototype;
+      // an 'in' test would let them through. re: #1089
+      "toString",
+      "constructor",
+      "hasOwnProperty",
+      "valueOf",
+      "__proto__",
+    ];
+    for (const bad of list) {
+      const errors = new ErrorCollector();
+      const v = makeValidator({ ridelength: bad }, errors);
+      assert.equal(v.validateRideLength('ridelength'), null, `for ${JSON.stringify(bad)}`);
+    }
+  });
+  it('ride length validator should not leak a global', () => {
+    delete globalThis.value;
+    const errors = new ErrorCollector();
+    const v = makeValidator({ ridelength: '0-3' }, errors);
+    v.validateRideLength('ridelength');
+    assert.equal(globalThis.value, undefined, "expected no implicit global");
+  });
 });