fix: count empty merge sources against maxTotalMergeKeys - #797
Merged
Merged
Conversation
The maxTotalMergeKeys guard (added for the quadratic merge-key DoS, GHSA-g796-fgmg-93mv) charges the budget once per *folded* key. A merge source that folds no keys — an empty mapping — therefore costs real work but never touches the counter. Aliasing one N-element sequence of empty mappings into K merge targets does O(K*N) work while the guard never fires: arr: &arr [{}, {}, ...] # N empty maps targets: - <<: *arr # x K With merge enabled (YAML11_SCHEMA), an ~500 KB document of this shape takes ~13s to load and scales quadratically, restoring the DoS the guard was meant to prevent; a real key-folding merge of the same size is correctly rejected. Charge one unit for a source that folds no keys, so a sequence of empty mappings is bounded by maxTotalMergeKeys like any other merge. Normal and single empty-map merges are unaffected.
Member
|
Note: next time, things will be simpler if you report such a problem via the standard way instead of a PR https://github.com/nodeca/js-yaml?tab=security-ov-file. The cost of writing code is much lower than that of PR processing. Especially when a PR needs to be reviewed urgently and there's no option to request improvements. |
Member
|
See dfd3a29 |
This was referenced Sep 2, 2026
This comment has been minimized.
This comment has been minimized.
This comment was marked as off-topic.
This comment was marked as off-topic.
opendroid
pushed a commit
to opendroid/the-infinity
that referenced
this pull request
Sep 9, 2026
Security fix: empty mappings in merge sequences now count toward maxTotalMergeKeys, and merge sequence size is hard-limited to 100, bounding CPU usage (nodeca/js-yaml#797).
MatiasFernandez
added a commit
to GemTalk/Jasper
that referenced
this pull request
Sep 9, 2026
The dependencyVulnFloors test asserted two things about checked-in JSON: that package.json still declares the `fast-uri` and `js-yaml` overrides, and that every copy the lockfile resolves sits at or above a hardcoded floor. Neither earns its keep. The first is a change detector — it only fails when someone deliberately edits the line, and then tells them they edited the line they just edited. The second freezes an advisory snapshot from the day it was written. A floor is a ratchet with a manual crank, and nothing turns it: when the world moves past the floor the test does not go red, it goes vacuously green, which is the worst failure mode a guard can have. That already happened — `fast-uri`'s override moved to ^3.1.7 in #544 while its floor stayed at 3.1.4, and #582 moves `js-yaml` to ^4.3.2 (the backport of the merge-key limits, nodeca/js-yaml#797) against a floor still reading 4.3.0, so the check would now accept a refresh back to a version with a known unpatched CPU-DoS. Keeping it honest means remembering to edit a constant on every Dependabot bump, and two bumps in, we already did not. It was also mis-housed: a repo-policy check over package.json and package-lock.json, sitting in the client vitest suite that wants a live stone, when its actual neighbours are `lint:lockfile` and `lint:supply-chain`. Nothing replaces it for now. Dependabot alerts already watch the same advisory database with data that stays current, and they arrive as PRs we can merge rather than as a constant someone has to maintain by hand.
This was referenced Sep 9, 2026
Knapp-Kevin
pushed a commit
to MythologIQ-Labs-LLC/FailSafe
that referenced
this pull request
Sep 10, 2026
…456) Security-motivated patch bump: hard-limits YAML merge-sequence size and counts empty mappings toward maxTotalMergeKeys, fixing a CPU-exhaustion DoS vector (nodeca/js-yaml#797). Lockfile-only diff, no protected surface touched. 8/9 required checks green (CodeQL neutral/informational), mergeable_state clean at base main c369cee.
thomas-hochbichler
added a commit
to thomas-hochbichler/obsidian-remarkable-tagged-sync
that referenced
this pull request
Sep 10, 2026
A dev-only transitive dependency, and the same one-line lockfile change Dependabot proposed in #140. 4.3.2 backports two limits on YAML merge keys from v5.4.1: a hard cap of 100 on a merge sequence, and empty mappings counted toward maxTotalMergeKeys so they cannot be used to burn CPU (nodeca/js-yaml#797). Reproduced by hand rather than merged from #140 because the Git identity gate checks every commit a branch adds to main against one address, and a Dependabot commit can never carry it. That makes every Dependabot PR structurally red here, which is worth deciding about separately -- this commit only takes the bump. `npm run matrix` reads the test matrix through js-yaml and still passes; the full suite is green. Claude-Session: https://claude.ai/code/session_0134WoHk57opQQAtELDmPorU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 13, 2026
Open
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
maxTotalMergeKeysguard added for the quadratic merge-key DoS (GHSA-g796-fgmg-93mv, fixed in 5.2.0) charges the budget once per folded key. A merge source that folds no keys — an empty mapping — costs real work to process but never increments the counter, so the guard can be bypassed:mergeSourceiterates every element of the<<:sequence, and aliasing one N-element array of empty maps into K targets doesO(K*N)work — none of it counted.Impact
With merge enabled (
YAML11_SCHEMA, orCORE_SCHEMA.withTags(mergeTag)— the standard YAML-1.1 / v4-compat configuration), the load time scales quadratically:A ~1 MB document projects to minutes of CPU on a single core. This restores the same availability DoS the
maxTotalMergeKeysguard was shipped to prevent. A real key-folding merge of comparable size is correctly rejected today (merge keys exceeded maxTotalMergeKeys), so the guard works for the intended shape but is defeated by zero-key sources.Attacker → control: attacker supplies an untrusted YAML document to an app that loads with merge enabled; the defeated control is
maxTotalMergeKeys(default 10000).Fix
Track whether a source folded any keys; if it folded none, charge one unit for it. A sequence of empty mappings is then bounded by
maxTotalMergeKeyslike any other merge.Tests
A case in
test/core/pathological.test.mjs(next to the existing merge-chain test) asserts the empty-source payload is rejected; it hangs for ~13 s and loads on the current code, and throws promptly with the fix. Normal and single empty-map merges still load correctly. Full suite green.