Skip to content

fix(deps): resolve serialize-javascript RCE, uuid + fast-uri advisories (+Claude) - #230

Closed
dhruv-parekh-cs wants to merge 5 commits into
mainfrom
CL-snyk-fixes/03-Sep-2026
Closed

fix(deps): resolve serialize-javascript RCE, uuid + fast-uri advisories (+Claude)#230
dhruv-parekh-cs wants to merge 5 commits into
mainfrom
CL-snyk-fixes/03-Sep-2026

Conversation

@dhruv-parekh-cs

Copy link
Copy Markdown

Summary

Automated dependency-vulnerability remediation (snyk-fix run of 03-Sep-2026). npm advisories 11 → 4, including a high-severity RCE. 1 high remains and needs a decision — see below.

Issue Severity Package Before → After Bucket Rationale
Serialize JavaScript RCE via RegExp.flags/Date + CPU-exhaustion DoS high serialize-javascript (transitive) 6.0.27.1.1 non-fixable → override No declared dependency upgrade reaches it.
SNYK-JS-FASTURI-19502739 · -19502854 high ×2 fast-uri (transitive) ^3.1.5^3.1.7 non-fixable → override Existing override had gone stale — still applying, but below the newly-patched version.
uuid — missing buffer bounds check in v3/v5/v6 when buf is provided moderate uuid (transitive) stale 8.3.2 copy → 11.1.1 non-fixable → override A second, vulnerable copy survived alongside 14.0.2.
mocha moderate → low transitive audit-fixed fixable npm audit fix, non-breaking.
body-parser, express, qs moderate ×3 transitive audit-fixed fixable npm audit fix, non-breaking.
SNYK-JS-ADMZIP-19276676 — crafted ZIP triggers 4GB allocation high adm-zip (direct) unchanged ⚠️ not fixed Breaking change; see below.
nyc, istanbul-lib-processinfo, sinon, diff moderate/low dev tooling unchanged declined Only remedies are isSemVerMajor jumps of dev tooling.

All three overrides use caret ranges at the loosest safe granularity, so future patches float in without another PR. Confirmed in the resolved tree: serialize-javascript@7.1.1 overridden, uuid@11.1.1 overridden, fast-uri@3.1.7.

The two fast-uri advisories were published by Snyk during this run — the pre-fix scan reported only adm-zip. Not caused by anything in this PR, but worth knowing if you compare scans.

⚠️ Not fixed — adm-zip needs a behavioural decision

The advisory requires adm-zip@0.6.0. Under 0.x semver a minor bump is the breaking boundary, and this one really is breaking: writeZipPromise changed from Promise<boolean> to Promise<void>.

The bump was attempted and the TypeScript build caught it:

src/adapters/file-upload.ts(344,10): error TS1345:
  An expression of type 'void' cannot be tested for truthiness.

The code at src/adapters/file-upload.ts:339 uses that return value as a success flag:

const status = await zip.writeZipPromise(zipPath).catch(() => {  this.exit(1); });
if (!status) { this.log('Zipping project process failed! …'); this.exit(1); }

On 0.6.0, status is always undefined, so if (!status) would fire on every successful zip — turning a working upload into an unconditional exit(1). Deciding how success should now be detected (rely solely on the .catch? stat the output file?) changes runtime behaviour, which this skill does not do unattended. Reverted to ^0.5.18.

Mitigating context for prioritisation: the advisory is about reading a crafted ZIP, and this code path only writes archives (addLocalFolder, addLocalFile, writeZipPromise). That is not a reason to leave it — just useful for triage.

Ecosystem-native audit pass (npm)

  • Before: 11 (0 critical / 2 high / 7 moderate / 2 low)
  • npm audit fix (non-breaking, --force never used) resolved 3 → 8 remaining
  • Three overrides then resolved 4 more (including the serialize-javascript high) → 4 remaining
  • Explicitly declined as breaking: adm-zip→0.6.0, nyc→18.0.0, sinon→22.1.0 (all isSemVerMajor)

Self code review

One review round. No High or Medium findings on the diff.

  • Stale-override detectionfast-uri ^3.1.5 is exactly the rot step 2.5 exists to catch: still applying, but below the patched version. Raised rather than removed, since it's still doing real work.
  • Adaptation correctness — the adm-zip revert was verified by rebuilding (green) rather than assumed; the API surface actually used was checked method-by-method against 0.6.0 before the attempt.
  • No masking — the remaining high is reported prominently, not buried among the fixed rows.
  • Version specificity — three loosest safe carets; no advisory says any patched line must not float.
  • Over-reach — diff is package.json + package-lock.json, plus .talismanrc (below).
  • Secrets — none sourced, printed, or committed.

Second commit — .talismanrc checksum refresh

The lockfile change made this repo's pinned .talismanrc checksum for package-lock.json stale, so the secrets-scan pre-commit hook fails. Talisman flags npm's standard sha512- integrity fields as base64 "secrets" — a false positive, and this repo's existing convention (see its .talismanrc history) is a checksum-pinned allowlist entry rather than reworking the lockfile. The value was computed from the committed blob (talisman --checksum) and committed separately; the hook passes on that commit, which is what confirms the refresh is correct.

Validation

  • npm install — clean
  • npm run build (tsc -b + oclif manifest) — green
  • npm test129 tests, 8 suites, all passing
  • snyk test --all-projects re-scan — the serialize-javascript/uuid/fast-uri findings are gone; 1 remaining (the unfixable adm-zip high)

dhruv-parekh-cs and others added 2 commits September 3, 2026 11:53
npm advisories 11 -> 4.

Overrides added (transitive, no direct upgrade reaches them):
  serialize-javascript ^7.1.1  RCE via RegExp.flags/Date + CPU-exhaustion DoS (was 6.0.2, high)
  uuid                 ^11.1.1 missing buffer bounds check in v3/v5/v6 (stale 8.3.2 copy)

Raised stale override:
  fast-uri ^3.1.5 -> ^3.1.7  SNYK-JS-FASTURI-19502739 / -19502854 (both high,
                             newly published by Snyk during this run)

NOT fixed - see PR body: adm-zip (high) needs 0.6.0, which changes
writeZipPromise from Promise<boolean> to Promise<void>; src/adapters/file-upload.ts
uses that return value as a success flag, so the bump breaks the build and
adapting it is a behavior decision. Reverted to ^0.5.18.

Validation: build green; 129 tests + 8 suites passing.

#claude_code#
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
The dependency fix in the previous commit changed package-lock.json, so its
pinned .talismanrc checksum went stale and the secrets-scan check fails.
Talisman flags npm's standard sha512- integrity fields as base64 'secrets';
this repo's existing convention is a checksum-pinned allowlist entry, so this
refreshes that value rather than reworking the lockfile.

#claude_code#
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@dhruv-parekh-cs
dhruv-parekh-cs requested a review from a team as a code owner September 3, 2026 06:37
Copilot AI lite review requested due to automatic review settings September 3, 2026 06:37
@snyk-io

snyk-io Bot commented Sep 3, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type Count (with fixes) Without fixes Threshold Result
🔴 Critical Severity 0 0 10 ✅ Passed
🟠 High Severity 0 1 25 ✅ Passed
🟡 Medium Severity 0 0 500 ✅ Passed
🔵 Low Severity 0 0 1000 ✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity Breaches (with fixes) Breaches (no fixes) SLA Threshold (with/no fixes) Status
🔴 Critical 0 0 15 / 30 days ✅ Passed
🟠 High 0 0 30 / 120 days ✅ Passed
🟡 Medium 0 0 90 / 365 days ✅ Passed
🔵 Low 0 0 180 / 365 days ✅ Passed

ℹ️ Vulnerabilities Without Available Fixes (Informational Only)

The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:

  • Critical without fixes: 0
  • High without fixes: 1
  • Medium without fixes: 0
  • Low without fixes: 0

✅ BUILD PASSED - All security checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Broad dependency overrides currently force versions outside some dependents’ declared semver ranges and need manual validation of compatibility/regenerated lockfile impacts.

Pull request overview

This PR updates dependency resolutions to remediate known npm advisories (including a high-severity serialize-javascript issue) primarily via npm audit fix and targeted overrides, and refreshes the repo’s Talisman allowlist checksum due to the lockfile change.

Changes:

  • Bump fast-uri override to ^3.1.7 and add overrides for serialize-javascript@^7.1.1 and uuid@^11.1.1.
  • Update package-lock.json to reflect new resolved versions (notably fast-uri, serialize-javascript, uuid, and related transitive reshaping).
  • Refresh .talismanrc checksum for package-lock.json to keep secrets-scan hooks passing.
File summaries
File Description
package.json Updates npm overrides to address security advisories (fast-uri/serialize-javascript/uuid).
package-lock.json Regenerates the resolved dependency tree to match updated overrides and audit fixes.
.talismanrc Updates the pinned checksum entry for package-lock.json after lockfile changes.
Review details
  • Files reviewed: 2/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread package.json Outdated
dhruv-parekh-cs and others added 2 commits September 3, 2026 14:40
Addresses Copilot review on PR #230.

The global "uuid": "^11.1.1" override was a real regression: the root copy was
already uuid@14.0.2 (SAFE - the advisory is uuid <11.1.1), and only
istanbul-lib-processinfo's nested 8.3.2 was vulnerable. The global override
therefore DOWNGRADED the safe root copy three majors, against
@contentstack/cli-utilities' declared uuid ^14.0.0.

Scoped both to the actual parents, matching this file's existing convention
(eslint > ajv, minimatch@N > brace-expansion):
  "mocha": { "serialize-javascript": "^7.1.1" }
  "istanbul-lib-processinfo": { "uuid": "^11.1.1" }

Resolved tree now: root uuid@14.0.2 (restored), istanbul's nested uuid@11.1.1
(patched), serialize-javascript@7.1.1 (patched). Same security posture as the
global form - npm audit 4, snyk 1 high (adm-zip) - with no downgrade.

Validation: build green; 129 tests + 8 suites passing.

#claude_code#
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Scoping the overrides changed package-lock.json again, so the pinned checksum
from the earlier commit is stale. Same false-positive/allowlist convention as
before (Talisman reads npm's sha512- integrity fields as base64 secrets).

#claude_code#
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 3, 2026 09:13
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type Count (with fixes) Without fixes Threshold Result
🔴 Critical Severity 0 0 10 ✅ Passed
🟠 High Severity 0 1 25 ✅ Passed
🟡 Medium Severity 0 0 500 ✅ Passed
🔵 Low Severity 0 0 1000 ✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity Breaches (with fixes) Breaches (no fixes) SLA Threshold (with/no fixes) Status
🔴 Critical 0 0 15 / 30 days ✅ Passed
🟠 High 0 0 30 / 120 days ✅ Passed
🟡 Medium 0 0 90 / 365 days ✅ Passed
🔵 Low 0 0 180 / 365 days ✅ Passed

ℹ️ Vulnerabilities Without Available Fixes (Informational Only)

The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:

  • Critical without fixes: 0
  • High without fixes: 1
  • Medium without fixes: 0
  • Low without fixes: 0

✅ BUILD PASSED - All security checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The changes are confined to dependency overrides/lockfile updates for security remediation and appear consistent with the stated validation (install/build/tests).

Review details
  • Files reviewed: 2/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 3, 2026 09:15
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type Count (with fixes) Without fixes Threshold Result
🔴 Critical Severity 0 0 10 ✅ Passed
🟠 High Severity 0 1 25 ✅ Passed
🟡 Medium Severity 0 0 500 ✅ Passed
🔵 Low Severity 0 0 1000 ✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity Breaches (with fixes) Breaches (no fixes) SLA Threshold (with/no fixes) Status
🔴 Critical 0 0 15 / 30 days ✅ Passed
🟠 High 0 0 30 / 120 days ✅ Passed
🟡 Medium 0 0 90 / 365 days ✅ Passed
🔵 Low 0 0 180 / 365 days ✅ Passed

ℹ️ Vulnerabilities Without Available Fixes (Informational Only)

The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:

  • Critical without fixes: 0
  • High without fixes: 1
  • Medium without fixes: 0
  • Low without fixes: 0

✅ BUILD PASSED - All security checks passed

@dhruv-parekh-cs

Copy link
Copy Markdown
Author

Review comments addressed

1 comment, verified as correct, fixed.

Verdict Comment Action
✅ valid Global serialize-javascript/uuid overrides force versions outside dependents' declared ranges Scoped both to their actual parents (a558af5)

Copilot was right, and the uuid half was a genuine regression rather than a theoretical risk. On origin/main the root copy was already uuid@14.0.2not vulnerable (the advisory is uuid <11.1.1) — and only istanbul-lib-processinfo's nested 8.3.2 was affected. The global override therefore downgraded a safe root copy three majors to fix an unrelated nested one, against @contentstack/cli-utilities' declared uuid: ^14.0.0.

Now scoped, matching this file's existing convention (eslint > ajv, minimatch@N > brace-expansion):

"mocha": { "serialize-javascript": "^7.1.1" },
"istanbul-lib-processinfo": { "uuid": "^11.1.1" }

Resolved tree: root uuid@14.0.2 (restored), istanbul-lib-processinfo/node_modules/uuid@11.1.1 (patched), serialize-javascript@7.1.1 (patched). Identical security posture to the global form — npm audit 4, snyk test 1 high — with the downgrade gone.

Also: branch updated from main (was 1 commit behind, no conflicts), and the .talismanrc lockfile checksum refreshed again since the lockfile changed.

Validation: build green; 129 tests + 8 suites passing.

Still needs a human: adm-zip (high) remains unfixed — 0.6.0 changes writeZipPromise from Promise<boolean> to Promise<void>, and file-upload.ts:339 uses that value as a success flag, so if (!status) would fire on every successful zip. That's a behavioural decision, detailed in the PR description. This PR also needs an approving review before it can merge.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The changes are limited to dependency metadata/lockfile updates, the overrides are correctly scoped to avoid regressions, and the repository’s Node engine constraint (>=22) is compatible with the updated transitive packages’ engine requirements.

Review details
  • Files reviewed: 2/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@Harshi-Shah-CS Harshi-Shah-CS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

base branch needs to changed to development

@dhruv-parekh-cs

Copy link
Copy Markdown
Author

Closing in favour of #231, which targets development — this PR was opened against main by mistake.

Not a retarget, because the bases have diverged: development is 13 commits ahead of main and main is 6 ahead of development. Switching this PR's base would have pulled those 6 unrelated main commits (Update release.yml ×3, Update CODEOWNERS, PR #226, a lockfile chore) into development, touching .github/CODEOWNERS, .github/workflows/release.yml and .talismanrc. #231 is a fresh branch off development with only the dependency change.

Most of what this PR did is already on development:

  • "istanbul-lib-processinfo": { "uuid": "^11.1.1" } — the exact scoped form this PR arrived at after @Copilot's review
  • serialize-javascript ^7.1.0 resolving 7.1.1
  • adm-zip ^0.6.0, with file-upload.ts already adapted to writeZipPromise's void return — the behavioural decision this PR deliberately left to a human

The only real gap left was fast-uri: development resolves 3.1.6, which Snyk still reports high (fixedIn: 3.1.7). #231 raises the override floor to ^3.1.7, verified against a pristine development checkout — 137 tests + 8 suites green.

Thanks @Copilot — the uuid downgrade you caught here was real, and development had independently landed the same scoped fix.

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.

3 participants