jsonlint (@mapbox/jsonlint-lines-primitives ) overwrites parsed objects' prototypes, confusing getType()

[cxcorp]

From #1025

Describe the bug
The @mapbox/jsonlint-lines-primitives package's parser performs an assignment when populating object's values, instead of calling Object.defineProperty as per the ECMAScript standard's JSON.parse() spec. As a result, an object can change its own prototype (note: it can NOT pollute the global object prototype!), after which e.g. instanceof String can return true for object values.

The most visible effect is that validation returns a validation error for objects that would be valid, but are miscategorized as e.g. numbers. If used in a field which is only validated to be a specific type. e.g. urls and other strings, this allows objects of the wrong type to pass to validation, for example this source:

{
  "type": "raster",
  "mazoom": 10,
  "tileSize": 256,
  "tiles": [{"length": 123, "__proto__": ""}]
}

An example of an object failing validation due to type confusion:

{
  "sources": {
    "foo": { "__proto__": 123 }
  },
  // ...required fields here
}

causes

{ "message": "sources.foo: object expected, number found" }

To Reproduce
Steps to reproduce the behavior:

Try validating the source JSON above, or launch Node's REPL in the repo and try:

const jsonlint = require("@mapbox/jsonlint-lines-primitives")

jsonlint.parse('{"url": ""}') // Actual string
// { url: [String: ''] { __line__: 1 } }

jsonlint.parse('{"url": {"__proto__": ""}}') // Not actual string
// { url: String {} }

jsonlint.parse('{"url": {"__proto__": ""}}').url instanceof String
// true

// Other types:

jsonlint.parse('{"url": {"__proto__": 123}}')
// { url: Number {} }
jsonlint.parse('{"url": {"__proto__": []}}')
// { url: Array {} }

// Fake expected instance properties:

const fakeArray = jsonlint.parse('{"length": 10, "__proto__": []}')
fakeArray
// Array { length: [Number: 10] { __line__: 1 } }
fakeArray instanceof Array
// true
fakeArray.length
// [Number: 10] { __line__: 1 }

Expected behavior

Parsing the style JSON should not allow objects to alter their own prototypes.

Additional context

I opened this issue here, because the package appears abandoned.

We could:

1. make our own fork of the mentioned library to fix this issue there, or
2. fork a more up-to-date fork to add back the things we need
3. consider other libraries or ways to add a line number to parsed JSON
4. not add line numbers to parsed JSON
5. ignore the issue

The issue stems from these additional lines in the parser's grammar:

https://github.com/mapbox/jsonlint/blob/e31b7289baedf3e1000d7ae7edd42268212c9954/src/jsonlint.y#L84-L89

As you can see, it assigns directly to the produced object instead of using Object.defineProperty, and thus __proto__ can be modified (as could constructor). This could be fixed with the following patch:

diff --git a/src/jsonlint.y b/src/jsonlint.y
index 09a171f..3666866 100644
--- a/src/jsonlint.y
+++ b/src/jsonlint.y
@@ -83,9 +83,19 @@ JSONMember
 
 JSONMemberList
     : JSONMember
-        {{$$ = {}; $$[$1[0]] = $1[1];}}
+        {$$ = {}; Object.defineProperty($$, $1[0], {
+            value: $1[1],
+            configurable: true,
+            enumerable: true,
+            writable: true
+        });}
     | JSONMemberList ',' JSONMember
-        {$$ = $1; $1[$3[0]] = $3[1];}
+        {$$ = $1; Object.defineProperty($1, $3[0], {
+            value: $3[1],
+            configurable: true,
+            enumerable: true,
+            writable: true
+        });}
     ;
 
 JSONArray

[HarelM]

@jfirebaugh @anand @mourner any thought on the the json lint package in regards to maintenance?

There are also some hacks that are being done as part of the rollup config in this repo that are probably worth solving while we are at it...

[cxcorp]

Adding what I found after further testing: the getType() function (src/util/get_type.ts) correctly returns "object" for "fake arrays", as it uses Array.isArray() instead of instanceof Array.

Some validation functions already "successfully fail" for "fake strings" thanks to them using unbundle() which attempts to call value.valueOf() which throws an error because String.prototype.valueOf only works on actual strings, but some don't (e.g. light.color throws, source.url does not).