ljharb / qs

A querystring parser with nesting support
BSD 3-Clause "New" or "Revised" License
8.47k stars 731 forks source link

[Fix] `parse`: Fix parsing when the global Object prototype is frozen #473

Closed jportner closed 1 year ago

jportner commented 1 year ago

Background

Click to expand _Note: examples use the Node.js REPL with strict mode: `NODE_REPL_MODE=strict node`._ ### Inheritance and Shadowing Objects in JavaScript [inherit properties from their prototype chain](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_prototype_chain). For example, the "toString" property can be accessed on all objects, but it doesn't actually exist on each object, it exists on the global Object prototype: ```js let obj = {}; obj.toString(); // '[object Object]' Object.getOwnPropertyDescriptor(obj, 'toString'); // undefined Object.getOwnPropertyDescriptor(Object.prototype, 'toString'); // { value: [Function: toString], writable: true, enumerable: false, configurable: true } ``` Under normal circumstances, you can assign a property to an object using the `=` operator, and any property of the same name in the object's prototype chain will not be modified, but will be "[shadowed](https://en.wikipedia.org/wiki/Variable_shadowing)" by the new property: ```js obj.toString = () => 'foo'; obj.toString(); // 'foo' Object.getOwnPropertyDescriptor(obj, 'toString'); // { value: [Function: toString], writable: true, enumerable: true, configurable: true } ``` ### Prototype Pollution [From Snyk:](https://learn.snyk.io/lessons/prototype-pollution/javascript/) > Prototype pollution is an injection attack that targets JavaScript runtimes. With prototype pollution, an attacker might control the default values of an object's properties. This allows the attacker to tamper with the logic of the application and can also lead to denial of service or, in extreme cases, remote code execution. There are a few different ways to mitigate Prototype Pollution, and one way to do it across the board is to [freeze the global "root" objects and their prototypes (Object, Function, Array, etc.)](https://www.npmjs.com/package/nopp) [From MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze): > The `Object.freeze()` static method *freezes* an object. Freezing an object [prevents extensions](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/preventExtensions) and makes existing properties non-writable and non-configurable. A frozen object can no longer be changed: new properties cannot be added, existing properties cannot be removed, their enumerability, configurability, writability, or value cannot be changed, and the object's prototype cannot be re-assigned. This means that any attempt to change the Object prototype will fail. If using [strict mode](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode), it will throw an error; otherwise, it will be silently ignored. If the Object prototype becomes frozen, all of its properties are no longer writable or configurable: ```js Object.freeze(Object.prototype); Object.getOwnPropertyDescriptor(Object.prototype, 'toString'); // { value: [Function: toString], writable: false, enumerable: false, configurable: false } ``` This also prevents shadowing properties with assignment. If an object _doesn't_ already have a property defined (such as "toString"), **and** it inherits a _non-writable_ property of that name from its prototype chain, any attempt to assign the property on that object will fail: ```js let obj2 = {}; obj2.toString = () => 'bar'; // Uncaught TypeError: Cannot assign to read only property 'toString' of object '#' obj2.toString(); // '[object Object]' ``` This behavior is described in the [ECMAScript 2016 specification](https://262.ecma-international.org/7.0/#sec-strict-mode-of-ecmascript): > Assignment to an undeclared identifier or otherwise unresolvable reference does not create a property in the [global object](https://262.ecma-international.org/7.0/#global-object). When a simple assignment occurs within [strict mode code](https://262.ecma-international.org/7.0/#sec-strict-mode-code), its [LeftHandSideExpression](https://262.ecma-international.org/7.0/#prod-LeftHandSideExpression) must not evaluate to an unresolvable [Reference](https://262.ecma-international.org/7.0/#sec-reference-specification-type). If it does a **ReferenceError** exception is thrown ([6.2.3.2](https://262.ecma-international.org/7.0/#sec-putvalue)). The [LeftHandSideExpression](https://262.ecma-international.org/7.0/#prod-LeftHandSideExpression) also may not be a reference to a data property with the attribute value {[[Writable]]: **false**}, to an accessor property with the attribute value {[[Set]]: **undefined**}, nor to a non-existent property of an object whose [[Extensible]] internal slot has the value **false**. In these cases a `TypeError` exception is thrown ([12.15](https://262.ecma-international.org/7.0/#sec-assignment-operators)).

The Problem

The allowPrototypes option is designed to strip any query parameters if they would shadow/overwrite properties on the global Object prototype (for example, "toString"). This logic is in the parseKeys() function:

https://github.com/ljharb/qs/blob/9dca37f15de317fe9ad0ced907cdf250ba310880/lib/parse.js#L172-L177

However, before calling parseKeys(), the parse module first calls parseValues() to create a temporary object:

https://github.com/ljharb/qs/blob/9dca37f15de317fe9ad0ced907cdf250ba310880/lib/parse.js#L246-L256

Unfortunately, when you have frozen the global Object prototype, the parseValues() function will throw a TypeError when inherited properties are present. Example:

Object.freeze(Object.prototype);
const { parse } = require('./lib');
parse('foo=bar&toString=123');
// Uncaught TypeError: Cannot assign to read only property 'toString' of object '#<Object>'

This means that projects cannot use this package if they have frozen the global Object prototype.

The Solution

Since the parseValues() function returns a temporary object, the simple solution here is to create a "plain object" that does not have a prototype. This doesn't require any other logic changes, because the if the allowPrototypes option is set to false, the parseKeys() function will subsequently strip out any problematic properties.

With this change, this package will be compatible with the "frozen prototype" approach of mitigating Prototype Pollution 🎉

Now you can use the same example as above, and it works as expected:

Object.freeze(Object.prototype);
const { parse } = require('./lib');
parse('foo=bar&toString=123');
// { foo: 'bar' }
jportner commented 1 year ago

Unfortunately, when you have frozen the global Object prototype, the parseKeys() function will throw a TypeError when inherited properties are present. Example:

Whoops, I wrote parseKeys() here when I meant to write parseValues(), that is what was throwing the error. I edited the PR description accordingly.

ljharb commented 1 year ago

Note that node --use_strict is NOT "strict mode", is something v8-specific, and should never, ever be used. The only proper strict mode is with 'use strict'; or in a class body, or an ES Module.

jportner commented 1 year ago

Note that node --use_strict is NOT "strict mode", is something v8-specific, and should never, ever be used. The only proper strict mode is with 'use strict'; or in a class body, or an ES Module.

TIL, thanks for pointing this out!

Apparently the correct way to enable strict mode in the Node.js REPL is to use the NODE_REPL_MODE=strict env var.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.80 :warning:

Comparison is base (9dca37f) 99.79% compared to head (7895b94) 99.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #473 +/- ## ========================================== - Coverage 99.79% 99.00% -0.80% ========================================== Files 8 8 Lines 1487 1501 +14 Branches 176 177 +1 ========================================== + Hits 1484 1486 +2 - Misses 3 15 +12 ``` | [Impacted Files](https://app.codecov.io/gh/ljharb/qs/pull/473?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Harband) | Coverage Δ | | |---|---|---| | [lib/parse.js](https://app.codecov.io/gh/ljharb/qs/pull/473?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Harband#diff-bGliL3BhcnNlLmpz) | `100.00% <100.00%> (ø)` | | | [test/parse.js](https://app.codecov.io/gh/ljharb/qs/pull/473?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Harband#diff-dGVzdC9wYXJzZS5qcw==) | `99.80% <100.00%> (+<0.01%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/ljharb/qs/pull/473/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Harband)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

ljharb commented 1 year ago

node < 6 has a different error message; i fixed that locally with a regex, but node <= 0.8 actually doesn't throw when Object.prototype is frozen, so I'll make a package to detect that, and update this, and then land it.

joe-matsec commented 1 year ago

Thank you!