fix(security): GT-657 fix the half of a js-yaml advisory that has a fix, and name the half that has none - #441
Merged
Conversation
…ix, and name the half that has none Closes GT-657. GHSA-5p4m-2wfm-xmqj / CVE-2026-59870 (js-yaml, quadratic CPU in `!!omap`) was published between 2026-08-05 and 2026-08-08 and turned `Security Audit` red on every branch. The dating is the attribution evidence: the dependabot PRs of the 5th were green, PR #440 of the 8th was red, and #440 changed 8 documentation and JSON files with ZERO dependency files. Pre-existing branch debt, not a regression. The half that had a fix, and why it was missed ---------------------------------------------- The root overrides already carried `js-yaml: 4.3.0`, added for an EARLIER advisory. The new one is vulnerable through 4.0.0-4.3.0 INCLUSIVE, so the existing pin was exactly one patch short of it. Bumped to 4.3.1; re-resolving also collapsed three nested copies (@commitlint/load, @istanbuljs/load-nyc-config, cosmiconfig) into the single hoisted one. The half that has none, proven rather than assumed -------------------------------------------------- The remaining advisory arrives through @nestjs/swagger, which pins js-yaml EXACTLY, and every published release pins a vulnerable one: 11.4.4 -> 4.1.1 and 11.4.5 -> 4.3.0 (CVE-2026-59870), 11.4.6 -> 5.2.1 (GHSA-pm4m-ph32-ghv5). 11.4.6 is the latest stable; 12.0.0 is alpha only. npm overrides do not reach it, measured four ways with the same tree every time: a top-level override, a scoped @nestjs/swagger override, both repeated with the unrelated nested override objects removed, and --package-lock-only versus a real npm install. That last experiment REFUTES the generalisation GT-636 recorded — that a nested override object anywhere stops the top-level rule cascading. Removing them changed nothing. What actually blocks the override is the consumer's exact pin. Recorded rather than left standing: a wrong lesson inside a closed row is worse than no lesson. Why an acceptance was not enough -------------------------------- Leaving the job red is what GT-622 was just closed to remove — a permanently red check trains reviewers to discount red, and the next genuinely fixable advisory would land in a job nobody reads. `63-validate-npm-audit-gate` keeps the same HIGH threshold and adds one requirement: an advisory with no upstream fix must be NAMED, with the path it arrives by and what was checked upstream. It fails on an undeclared advisory, on a declaration for a different id or path, and — the rule that stops a graveyard — on a declaration whose advisory has DISAPPEARED, so the good news arrives as a red check asking for the entry's removal. The gate's own fixtures run in the job, because a gate whose exception list can swallow anything is a green button. Verified -------- node --test 63-validate-npm-audit-gate.test.mjs 19/19 (including the undeclared advisory red, the wrong-path declaration covering nothing, the stale declaration failing, and an unparseable registry stopping the run). `npm audit` now reports 0 critical / 2 high, both declared, 3 moderate untouched below the unchanged threshold. core-api 160/160 in 30 suites and core-domain 1704/1704 in 146 suites against the bumped tree; eslint 9.39.4 and jest 30.4.1 both boot. Guards 04, 08, 09 --check, 34, 39, 40, 41 (--execute --strict, 114 executed / 113 exit 0, the one non-zero being GT-78's root-cleanliness objecting to an untracked local .DS_Store absent on the runner), 42, 43, 46, 47, 49 and bilingual-terminology-lint all exit 0. Board: 642 / 655 done, 3 in progress, 3 pending, 7 deferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📊 Bilingual Coverage ImpactPR Changes
Repository Coverage
✅ Good: All EN changes have ES counterparts. Generated by GitHub Actions |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+2692
to
+2713
| "node_modules/@nestjs/swagger/node_modules/js-yaml": { | ||
| "version": "5.2.1", | ||
| "resolved": "https://registry.npmjs.org/js-yaml/-/js-yaml-5.2.1.tgz", | ||
| "integrity": "sha512-zfLtNfQqxVqq3uaTqSkh4x4hZw3KHobGUA0fJUj4wawW8bsQLTVqpHdXSIzidh7o+4lEW36tANuAGdaFx6Zgnw==", | ||
| "funding": [ | ||
| { | ||
| "type": "github", | ||
| "url": "https://github.com/sponsors/puzrin" | ||
| }, | ||
| { | ||
| "type": "github", | ||
| "url": "https://github.com/sponsors/nodeca" | ||
| } | ||
| ], | ||
| "license": "MIT", | ||
| "dependencies": { | ||
| "argparse": "^2.0.1" | ||
| }, | ||
| "bin": { | ||
| "js-yaml": "bin/js-yaml.mjs" | ||
| } | ||
| }, |
…wered by reverting `Trivy` reported "1 new alert including 1 high severity security vulnerability" on PR #441 — js-yaml 5.2.1 at package-lock.json. It reads as though this change introduced a vulnerability. It did not, and the revert was attempted before that was checked, so the check is recorded rather than the conclusion alone. Measured at BOTH versions: @nestjs/swagger 11.4.4 -> js-yaml 4.1.1 -> THREE advisories GHSA-52cp-r559-cp3m HIGH GHSA-5p4m-2wfm-xmqj HIGH GHSA-h67p-54hq-rp68 moderate @nestjs/swagger 11.4.6 -> js-yaml 5.2.1 -> ONE advisory GHSA-pm4m-ph32-ghv5 HIGH The bump removes two high advisories and leaves one. Trivy compares alerts on the changed lines, so an advisory id changing at the same lockfile position is "new" to it — the id changed, no hole opened. Two instruments disagree with each other here and both are being read correctly: npm reports 2 high ROWS for 11.4.6 (js-yaml and its parent) against 1 for 11.4.4, while 11.4.6 carries a third of the advisories. Counting rows says revert; counting holes says do not. No code or dependency change: the tree is byte-identical to the previous commit. What is added is the reasoning, in the exceptions entry as `doNotRevertTheSwaggerBump` and in GT-657's catalog section in both languages, so the next reader who sees the red Trivy check does not undo the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 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.
Closes GT-657.
GHSA-5p4m-2wfm-xmqj/ CVE-2026-59870 (js-yaml, quadratic CPU in!!omap) turnedSecurity Auditred on every branch. Attribution first, because "not my PR" is a claim that needs evidence: the dependabot PRs of 2026-08-05 were green, PR #440 of the 8th was red, and #440 changed 8 documentation and JSON files with zero dependency files. Pre-existing branch debt onmainanddevelop.The half that had a fix, and why it was missed
The root
overridesalready carriedjs-yaml: 4.3.0, added for an earlier advisory. The new one is vulnerable through4.0.0 - 4.3.0inclusive — the existing pin was exactly one patch short of it. Bumped to4.3.1; re-resolving also collapsed three nested copies into the single hoisted one.The half that has none, proven rather than assumed
@nestjs/swagger4.1.1exact4.3.0exact5.2.1exactThere is no version to bump to, and npm
overridesdo not reach a nested exact pin — measured four ways, same tree every time: top-level override, scoped@nestjs/swaggeroverride, both repeated with the unrelated nested override objects removed, and--package-lock-onlyversus a realnpm install.That last experiment refutes a generalisation this board already recorded: GT-636 concluded that a nested override object anywhere stops the top-level rule cascading. Removing them changed nothing — what blocks it is the consumer's exact pin. Corrected in the catalog rather than left standing.
Why an acceptance was not enough
Leaving the job red is precisely what #440 just closed GT-622 to remove: a permanently red check trains reviewers to discount red, and the next genuinely fixable advisory would land in a job nobody reads.
63-validate-npm-audit-gatekeeps the same HIGH threshold and adds one requirement — an advisory with no upstream fix must be named, with the path it arrives by and what was checked upstream.The gate's own fixtures run in the job, because a gate whose exception list can swallow anything is a green button.
Verification
node --test19/19.npm auditnow reports 0 critical / 2 high, both declared, 3 moderate untouched below the unchanged threshold. core-api 160/160 in 30 suites and core-domain 1704/1704 in 146 suites against the bumped tree; eslint 9.39.4 and jest 30.4.1 both boot. Guards 04, 08, 09--check, 34, 39, 40, 41--execute --strict, 42, 43, 46, 47, 49 andbilingual-terminology-lintall exit 0.Board: 642 / 655 done, 3 in progress, 3 pending, 7 deferred.
🤖 Generated with Claude Code