Skip to content

Deep-clone json and context in init hooks (match searchParams)#871

Closed
greymoth-jp wants to merge 2 commits into
sindresorhus:mainfrom
greymoth-jp:fix/init-hook-nested-json-context-leak
Closed

Deep-clone json and context in init hooks (match searchParams)#871
greymoth-jp wants to merge 2 commits into
sindresorhus:mainfrom
greymoth-jp:fix/init-hook-nested-json-context-leak

Conversation

@greymoth-jp

Copy link
Copy Markdown

cloneInitHookOptions deep-clones searchParams (#861) and retry so init-hook mutations don't leak across requests, but json and context are still only shallow-cloned (one level). A nested mutation in a beforeRequest hook (e.g. options.json.meta.requestId = ... on a reused json: {meta: {}}) leaks into subsequent requests. Existing tests only cover top-level mutation, so it's undetected.

#861 established the deep-clone-nested-init-hook-structures pattern for searchParams; json/context were left behind. The fix applies the same isolation (+2 tests). Verified both ways: old leaks the nested mutation, fix isolates it; top-level, Date, and class-instance cases preserved, 0 regression.

@sindresorhus

Copy link
Copy Markdown
Owner

This regresses cyclic json values when stringifyJson is used and any init hook is present. json is typed as unknown, and stringifyJson is the documented escape hatch for custom serializaton, so Ky should not recursively walk the value before handing it to the custom serializer.

I verified this manually: the same snippet succeeds on origin/main, but this branch throws RangeError: Maximum call stack size exceeded before stringifyJson runs.

I think the underlying leak is real, but deep-cloning arbitrary json is broader than needed. The lower-risk fix is probably to keep json shallow, or add cycle tracking if we really want recursive cloning there. Deep-cloning context makes more sense because it is explicitly a request metadata object, while json can be any serializer input.

Deep-cloning json overflowed the stack for cyclic or very deep serializer input before stringifyJson ran. Revert json to a shallow clone (restoring main's behavior) and keep context deep-cloned, since that is what the cross-request leak fix was actually about. Drop the nested-json test and add a regression test for cyclic json with a custom stringifyJson and an init hook present.
@greymoth-jp

Copy link
Copy Markdown
Author

You're right, deep-cloning json is overreach. It's unknown and stringifyJson is the documented escape hatch, so walking it recursively is the wrong default, and with no cycle tracking a cyclic or very deep value overflowed the stack before stringifyJson ran.

Reverted json to a shallow clone (restoring main's behavior); context keeps the deep clone since that's what the leak fix was about. Dropped the nested-json test, kept the nested-context one, and added a regression test for your exact case (cyclic json + custom stringifyJson + an init hook present). Full hooks suite is green.

@greymoth-jp greymoth-jp closed this Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants