sindresorhus / sindresorhus/query-string

`parse()` treats `types` keys inherited from `Object.prototype` as custom types

Open
#421 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
6.9k
Forks
455
PR merge metrics
No merged PRs in 30d

Description

Pull requests appear to be disabled on this repo, so filing as an issue. I have a tested one-line fix on a branch and am happy to hand it over in whatever form is useful: https://github.com/MFA-G/query-string/tree/fix/types-prototype-keys

Problem

parse() indexes the types map with keys taken straight from the query string:

returnValue[key] = parseValue(value, options, options.types[key]);

The default is Object.create(null), but that default is discarded as soon as a caller passes their own map — which is the only way the option is ever used. A plain {} inherits Object.prototype, so a query key that collides with a prototype member resolves to the inherited value instead of undefined, and parseValue treats it as a custom type function.

Reproduction

query-string 9.5.1, Node 22:

import queryString from "query-string";

queryString.parse("toString=1&a=2", {types: {}});
//=> {a: "2", toString: "[object Undefined]"}   expected toString: "1"

queryString.parse("toString[]=1&toString[]=2", {arrayFormat: "bracket", types: {}});
//=> {toString: ["[object Undefined]", "[object Undefined]"]}

queryString.parse("constructor=1", {types: {}});
//=> {constructor: ["1"]}                       expected constructor: "1"

valueOf and hasOwnProperty do not merely corrupt the value, they throw — so a query string can crash a parse that has nothing to do with those keys:

queryString.parse("valueOf=hello", {types: {}});
//=> TypeError: Cannot convert undefined or null to object

queryString.parse("hasOwnProperty=x", {types: {}});
//=> TypeError: Cannot convert undefined or null to object

Since query keys are usually attacker-controlled, any parse call with a caller-supplied types map has a small DoS / value-corruption surface. Passing Object.create(null) as types avoids all of it, but nothing in the docs suggests that is required.

Suggested fix

Normalize options.types to a null-prototype object right after the option merge, so only own keys are consulted:

options.types = {__proto__: null, ...options.types};

This preserves the documented default and every documented use of the option; the only behavior that changes is the accidental prototype lookup. Callers already passing Object.create(null) are unaffected.

I have a regression test for toString / valueOf / hasOwnProperty / constructor plus the array-format path on the branch above; npm test is green (191 passed, plus the pre-existing known failure).

Contributor guide

Open the contributing guide

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 at the parse() path where options.types[key] is passed to parseValue, and review the supplied reproduction cases for prototype keys and array formats. Run npm test and verify regression coverage for toString, valueOf, hasOwnProperty, and constructor while preserving normal custom types behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.