test: guard against ejs regressing below ^6.0.1 (DII-2600) - #711
Open
manisha1997 wants to merge 13 commits into
Open
manisha1997 wants to merge 13 commits into
manisha1997 wants to merge 13 commits into
Conversation
- Drop Node 20 from all CI test matrices; minimum is now Node 22 - Update hardcoded node-version '20' → '22' in release jobs - Switch test job runners to ubuntu-x64 - SHA-pin actions/checkout (v4) and actions/setup-node (v6) - Add Artifactory OIDC Auth step to all test jobs - Add ARTIFACTORY_URL env var and id-token: write permissions - Add lockfile-hygiene job (twilio/sdk-actions/npm-lockfile-hygiene) to cli-test.yml - Guard test jobs with if: github.repository_owner == 'twilio' - Fix dead SonarCloud condition (16.x → lts/*) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JFrog Curation blocks ejs 3.x tree-wide (CVE-2023-29827, CVSS 9.8, no fixed version on the 3.x line). @oclif/core, @oclif/plugin-autocomplete, @oclif/plugin-help/plugins/update/version/warn-if-update-available, and the oclif devDependency all pull in nested ejs 3.x at various versions. An override forces every consumer in the tree to a single ejs@6.0.1 without needing to bump any oclif package itself.
…itical-cve-override
npm install was consistently hitting npm's own "Exit handler never called!" bug on this Windows CI image right after printing ~30 deprecation warnings from the oclif v1 dependency tree, before any install progress. npm ci skips the resolution/mutation (arborist) code path most bug reports implicate, using the lockfile directly instead, and --loglevel=error cuts the volume of synchronous console writes suspected of racing with npm's exit handling on Windows.
npm ci was still hitting "Exit handler never called!" even without the resolution/mutation code path (npm ci vs npm install) or verbose log output as suspects. The npm-cache directory is keyed only on appveyor.yml, so it survives across builds -- including our own earlier crashed runs -- and a corrupted/partial cache entry from a killed run is a well-documented trigger for this exact npm bug. Re-key the cache on package-lock.json too and clean it defensively before every install.
…store public lockfile URLs CI was failing two independent ways: - `audit`: `make install` (which deletes and regenerates package-lock.json from scratch) pulled undici@5.29.0 transitively via our own @actions/core@^1.0.0 / @actions/github@^6.0.0 devDependencies, and Twilio's Artifactory curation policy now blocks that version (403). Bumped both to the same major versions @twilio/cli-core already uses (^2.0.0 / ^9.0.0), which resolve to undici@6.x instead. - `lockfile-hygiene` / AppVeyor's `npm ci`: the committed package-lock.json had 26 entries resolved to npmjs.artifacts.twilio.com instead of the public registry (from someone's local Artifactory-backed `npm install` never being normalized back). AppVeyor has no route to that internal host, so `npm ci` there was failing outright. Regenerated the lockfile via `make install` (only reachable through Artifactory from this network) and ported the restore-lockfile-registry.js script from twilio-cli-core to rewrite the resulting Artifactory URLs back to registry.npmjs.org. Also wired the script into `make install` itself (rather than per-workflow) since this repo's Makefile always deletes and rebuilds the lockfile from scratch, so every future install self-corrects before anyone commits.
The lockfile-hygiene CI check scans .npmrc (among other files) for registry/resolved host lines, and its script runs under `bash -e -o pipefail`. Our .npmrc only had `engine-strict=true`, which matches nothing, so the scan's `grep | grep | sed` pipeline for that file exited 1, pipefail propagated it, and set -e killed the whole check before it ever reached package-lock.json -- regardless of that file's actual content. Reproduced locally against the exact PR merge commit. Pinning the public registry explicitly is good practice on its own (it's the default for anyone running npm here without a corporate .npmrc override) and incidentally gives the scan a matching line so it completes instead of dying early.
The new audit gate (from the merged CI branch) flags file-type@16.5.4 for a moderate-severity infinite-loop DoS on malformed input. The GitHub Security Advisory's first patched version is 21.3.1, not the 22.0.2 that `npm audit fix --force` defaults to -- both are equally ESM-only (file-type dropped CJS support back at v17), so there's no version that avoids the code change; 21.3.1 is simply the minimal version that actually contains the fix. file-type's single call site (email:send's attachment content-type detection) is switched from a top-level require to a dynamic import, since the package can no longer be loaded via require() from this CommonJS file. Bumped .eslintrc's ecmaVersion 2018 -> 2020 so the parser recognizes import() syntax. Note: a larger, pre-existing set of vulnerable production dependencies (axios, basic-ftp, brace-expansion, follow-redirects, form-data, ip-address, js-yaml, lodash, minimatch, picomatch, qs, ws) also fails this same audit gate -- confirmed independently reproducible on main once the CI branch merges, unrelated to this fix. Left out of scope here; flagged separately for the CI branch owners.
ejs is not a direct dependency: it arrives through @oclif/core, whose 1.x and 2.x lines still declare `ejs: ^3.x`. The 6.0.1 floor is held only by the `overrides` block in package.json, which a dependency bump, lockfile regeneration or merge can drop silently — reintroducing CVE-2023-29827. Add .github/scripts/verify-ejs-override.js, which asserts the floor from three independent angles: the overrides declaration in package.json, every resolved ejs entry in package-lock.json, and every ejs copy actually installed under node_modules (which catches a nested 3.x copy that a lockfile check alone would miss). Wire it into unit tests, the audit check, and both release workflows. The package.json and lockfile checks need no install, so it runs as a fail-fast step in the lockfile-hygiene job before anything else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Base automatically changed from
ejs-critical-cve-override_internal
to
chore/oss-publishing-node22-ci-updates
September 10, 2026 07:50
Base automatically changed from
chore/oss-publishing-node22-ci-updates
to
main
September 16, 2026 10:51
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.
DII-2600
Regression guard for the
ejs@^6.0.1override added in #710. Split out of that PR so the CVEfix and the new CI check can be reviewed separately.
Why
ejsis not a direct dependency. It arrives through@oclif/core, whose 1.x and 2.x lines stilldeclare
ejs: ^3.x. The 6.0.1 floor is held only by theoverridesblock inpackage.json,which a dependency bump, a lockfile regeneration, or a merge can drop silently — quietly
reintroducing CVE-2023-29827.
Nothing upstream of the repo catches that:
npm pack ejs@3.1.10still succeeds through ourinternal registry, so curation would not block a regression either.
What
.github/scripts/verify-ejs-override.jsasserts the floor from three independent angles:package.json— theoverrides.ejsdeclaration exists and holds the floorpackage-lock.json— every resolvedejsentry satisfies the floornode_modules— everyejscopy actually installed satisfies the floorChecks 1 and 2 need no install, so the guard runs before
npm cias a fail-fast step in thelockfile-hygienejob. Check 3 catches a nested 3.x copy that a lockfile check alone would miss.Zero dependencies, so it cannot itself be broken by a dependency change.
Wired into unit tests (pre- and post-install), the audit check, and both release workflows.
Test report
Verified it passes on a healthy tree and actually fails on four seeded regressions:
overrides.ejsdeleted + lockfile back to 3.1.10^3.1.10@oclif/core/node_modules/ejs@3.1.10on diskFailure output names the advisory, gives remediation steps, and emits a GitHub Actions
::errorannotation.
Also confirmed on this branch:
npm ls ejs→ single dedupedejs@6.0.1, 209 tests passing,lint clean,
npm audit --audit-level=moderate --production→ 0 vulnerabilities.Checklist
🤖 Generated with Claude Code