Skip to content

[Fix] stringify: skip null/undefined filter-array entries instead of crashing in encoder - #551

Merged
ljharb merged 1 commit into
ljharb:mainfrom
gregkh:stringify-skip-null
Apr 27, 2026
Merged

ljharb merged 1 commit into
ljharb:mainfrom
gregkh:stringify-skip-null

Conversation

@gregkh

@gregkh gregkh commented Apr 13, 2026 •

Copy link
Copy Markdown

When filter is an array, its elements become objKeys directly and the top-level loop passes each entry as the recursive prefix argument without coercion. A null or undefined entry reached utils.encode as the str argument and str.length threw TypeError.

The crash requires two conditions to align: the filter array must contain null/undefined, and the object being stringified must have a literal 'null'/'undefined' property (otherwise obj[key] is undefined and the typeof-undefined early-return at stringify.js:137 fires before the encoder is called). Both are reachable from an attacker controlled input in applications that stringify user-submitted JSON with user-selected fields.

Fix this up by skipping null/undefined filter entries at the top-level loop. This matches JSON.stringify replacer-array semantics, which silently ignore non-string/non-number entries. Object.keys never yields null/undefined, so the guard only affects user-supplied filter arrays. The inner recursive loop already coerces key via String() when building keyPrefix, so it never passes a raw null to the encoder.

@ljharb ljharb left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need regression tests.

@ljharb
ljharb marked this pull request as draft April 15, 2026 23:10
@gregkh

gregkh commented Apr 19, 2026

Copy link
Copy Markdown
Author

Regression test added, thanks for reminding me!

@gregkh
gregkh marked this pull request as ready for review April 19, 2026 09:18
@ljharb ljharb changed the title [Fix] stringify: skip null/undefined filter-array entries instead o… [Fix] stringify: skip null/undefined filter-array entries instead of crashing in encoder Apr 22, 2026
…f crashing in `encoder`

When `filter` is an array, its elements become `objKeys` directly and the top-level loop passes each entry as the recursive `prefix` argument without coercion.
A `null` or `undefined` entry reached `utils.encode` as the `str` argument and `str.length` threw `TypeError`.

The crash requires two conditions to align: the filter array must contain `null`/`undefined`, and the object being stringified must have a literal `'null'`/`'undefined'` property
(otherwise `obj[key]` is `undefined` and the typeof-undefined early-return at stringify.js:137 fires before the encoder is called)

Both are reachable from an attacker controlled input in applications that stringify user-submitted JSON with user-selected fields.

Fix this up by skipping `null`/`undefined` filter entries at the top-level loop.
This somewhat matches `JSON.stringify` replacer-array semantics, which silently ignore non-string/non-number entries.
`Object.keys` never yields `null`/`undefined`, so the guard only affects user-supplied filter arrays.
The inner recursive loop already coerces `key` via `String()` when building `keyPrefix`, so it never passes a raw `null` to the encoder.

@ljharb ljharb left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I'd prefer you didn't use LLMs for open source contributions, this seems fine, thanks.

@ljharb
ljharb force-pushed the stringify-skip-null branch from 89913af to 0c180a4 Compare April 27, 2026 05:59
@ljharb
ljharb merged commit 0c180a4 into ljharb:main Apr 27, 2026
415 checks passed
@gregkh

gregkh commented Apr 27, 2026

Copy link
Copy Markdown
Author

Although I'd prefer you didn't use LLMs for open source contributions, this seems fine, thanks.

Sorry about that, a LLM found the problem, and helped with a portion of this. If there is a better way to mark this as such, I'll be glad to do so.

Thanks for taking the change, much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants