Repository navigation
fix(package): require the oldest supported LTS (node >= 22) - #1479
sasjs-dev[bot] wants to merge 11 commits into
Conversation
The declared node floor permits end-of-life releases - node 14 and 16 reached EOL in 2023, 18 on 2025-04-30, and 20 on 2026-04-30. The oldest line still receiving security fixes is 22, which is also the version CI runs. Raise engines.node to >=22 so the declaration matches the runtimes this package is tested and supported on. Verification: the only package.json change is the engines.node value, and CI runs on node 22.
Coverage reportTotal coverage
Show files with reduced coverage 🔻Reduced coverage
Report generated by 🧪jest coverage report action from ef17b9a |
There was a problem hiding this comment.
Reviewed the full base...main diff (package.json only: engines.node raised from "^14.18.0 || >=16.10.0" to ">=22") plus hardening checks at the head: manifest pinning (all exact), .npmrc (ignore-scripts + save-exact), lockfile present, gitleaks pre-commit hook (pinned v8.30.1), CI audit gate, depcheck triage, secrets scan, and npm audit against the head lockfile (prod-scoped: clean). CI (test (22)) is green on the head. Findings:
-
package-lock.json:66-68 - the root lockfile entry still records "engines": {"node": "^14.18.0 || >=16.10.0"}. npm ci does not rewrite it on its own (verified: a manifest engines bump leaves the lockfile root untouched until npm install is re-run), so the lockfile now contradicts the manifest. Most consumers read the tarball's package.json (correct: >=22), but anyone installing from the repo or reading the lockfile sees the stale contract. Re-run npm install after merging to regenerate, or touch the lockfile root entry in this PR.
-
package.json:12-13 - the "checkNodeVersion" and "nodeVersionMessage" scripts still enforce and print "^14.18.0 || >=16.10.0 to install SASjs CLI" (the embedded node -e message says exactly that, and rejects node 17-21 since its logic is "m > 16"). With engines now >=22, those scripts are both stale and actively wrong: they accept node 18/20 (which engines now rejects) as fine and describe a floor of 14.18. Either update the embedded check to >=22 or drop the scripts entirely - engines (npm enforces it with engine-strict, and GitHub installers honour it) already does the job. Advisory consistency, but it will print a misleading version requirement to users on node 18/20.
-
.github/workflows/npmpublish.yml:45 - for awareness while touching engines: the release workflow's semantic-release step is "npx -p @semantic-release/exec -p semantic-release -- semantic-release" - unpinned, no --yes, resolved fresh on every release run. Not this PR's change, so out of scope for a blocking finding, but a pinned form (the sasjs/lint publish.yml:72-78 shape) is the org's current pattern and would keep a future audit clean here too.
-
Ghost import: src/utils/utils.ts:7 and src/commands/run/run.ts:25 import axios, but axios is declared nowhere in package.json - it rides on the hoist from @sasjs/adapter's dependency (verified in the lockfile). The adapter manifest also pins axios 1.20.0, so today it works, but the import is a contract violation waiting on the adapter changing its own tree. Declare axios as a direct dependency. Pre-existing, not introduced by this PR, but it surfaced in the depcheck pass so it is reported.
4 findings above for review.
package-lock.json still recorded the old "engines": {"node": "^14.18.0 ||
>=16.10.0"} at the root while the manifest now declares >=22. npm ci does not
rewrite it, so regenerate the lockfile and let the two agree.
…se tooling Three findings from review, plus one of the same staleness class: - axios is imported by src/utils/utils.ts and src/commands/run/run.ts but was declared nowhere - it rode on the hoist from @sasjs/adapter's dependency. Declared directly, pinned to 1.20.0 (the version already resolved, so the tree does not move). - the `checkNodeVersion` and `nodeVersionMessage` npm scripts are referenced by no workflow and no doc, and the check embedded "^14.18.0 || >=16.10.0" with `m > 16` logic, so it would accept node 18/20 that engines now rejects. Dropped both; engines does the job. - src/utils/utils.ts's checkNodeVersion() refused only below node 12, and said so to the user. Raised to 22, matching engines, with its spec updated. - .github/workflows/npmpublish.yml ran `npx -p ... semantic-release` unpinned, resolving latest on every release with publish credentials. Pinned to the versions the other sasjs release workflows use. Verification: - npm ci: 698 packages - npx jest src/utils/spec/utils.spec.ts: 46 tests pass - npm run lint: clean - package-lock.json root now records axios and engines >=22 Note: `npm run test:mocked` bails on src/commands/version/spec/version.spec.ts in a sandbox, because printVersion() shells out to `npm list -g @sasjs/cli` and nothing is installed globally. Reproduced identically on the base commit with these changes stashed, so it is environmental, not this diff.
|
All four addressed.
On the npx-integrity point: accepted, and now stated rather than implied. The top levels are pinned but the transitive tree is fetched at release time, so it is weaker than a lockfile. The alternative was carrying the tool as a devDependency with a locked tree but advisories no upgrade clears - both are imperfect, and this trades audit noise for no integrity lock. A dedicated toolchain lockfile (its own |
The release toolchain now lives in `release/` with its own package.json and lockfile, installed by the publish workflow with `npm ci --prefix release`, rather than being resolved from npx at release time. Locked, so the resolved versions and their integrity hashes are pinned - the gap raised in review, where npx pinned only the top levels and fetched the transitive tree fresh. The `files` allowlist already keeps `release/` out of the tarball. Verification: every plugin in the release config resolves from release/node_modules.
The toolchain in release/ carries twelve advisories that cannot be fixed from this repo - the ranges cover the newest published version - and that code runs with publish credentials. Moving it out of the package's dependencies kept it out of the package's `npm audit`, which is right; it also made it invisible, which is not. release/audit.mjs reports it separately and by name: the twelve are exempt with their reasons, anything else fails the build, and an exemption that stops appearing is reported so it can be deleted rather than left to rot. It uses --package-lock-only, so it needs no install and runs on every PR as well as in the release. Verification: node release/audit.mjs reports 12 exempt and exits 0.
sas-language runs `prettier --check .`, which covers release/ - it is the only one of the five repos that checks that directory - and it rejected the new script. Formatted with that config (printWidth 100). The file is kept byte-identical in all five repos, so the same content goes to the four that do not check it. Otherwise the copies drift and a later fix lands in some of them only. Verification: `prettier --check .` clean, `node release/audit.mjs` still reports 12 exempt and exits 0, in all five.
There was a problem hiding this comment.
Re-review at the new head 9bb2d8d; the delta since the prior review at c6e342e is the axios declaration (package.json:76, lockfile root updated to match), the dead version-script removal, the release-tooling lock tree, the checkNodeVersion alignment, and the scoped release audit script. All four prior findings are addressed: axios is now declared, the stale checkNodeVersion/nodeVersionMessage scripts are dropped (engines does the job), the release tooling is pinned in a locked tree (release/package.json + lockfile, npm ci --prefix release --ignore-scripts at npmpublish.yml:50, plugin set matches the release config), and the lockfile engines root is synced (package-lock.json:67-69 now ">=22"). CI (test (22)) is green on the head. The checkNodeVersion updates in src/utils/utils.ts:497-505 and the spec at src/utils/spec/utils.spec.ts:239-260 are consistent with engines. The delta since adds release/audit.mjs (a scoped audit of the release toolchain, sound) and its workflow hooks. Two findings on the current head:
-
src/utils/utils.ts:497 - checkNodeVersion() is now exported but has no caller anywhere in the repo: the only references are the definition itself and the spec (src/utils/spec/utils.spec.ts:233-260). It was already caller-less in base before this PR touched it (verified against origin/main), so this is not a regression the PR introduces - but the PR edits dead code (the threshold and message), which is the natural moment to either wire it into the CLI entrypoint (src/index.ts's require('./cli') path) or drop it. As it stands the runtime version check it implements never runs; engines + npm's own warning are the only enforcement.
-
.github/workflows/npmpublish.yml:53 - "release/node_modules/.bin/semantic-release" runs as the Semantic Release step but the workflow's checkout is a plain actions/checkout@v4 with no fetch-depth setting, so the clone is shallow (depth 1). semantic-release needs the full history and tags to determine the version: it will attempt "git fetch --unshallow" itself (verified in semantic-release 25.0.9's lib/git.js), which works today on a public repo but is an extra network round-trip on every release run and fails if the fetch is blocked. The sibling repos set "fetch-depth: 0" on the release checkout for exactly this reason (utils publish.yml:34-38, lint publish.yml, adapter npmpublish.yml:34-38). Add "with: fetch-depth: 0" to the checkout step.
2 findings above for review.
The publish workflow's checkout is shallow (depth 1), and semantic-release needs the full history and tags to compute the version. It fetches --unshallow itself, which works on a public repo but is a wasted round-trip on every release and fails outright if the fetch is blocked. The sibling release workflows (adapter, utils, sas-language) all set fetch-depth: 0 for this reason.
`checkNodeVersion` in src/utils/utils.ts has no caller: src/index.ts does not call it, it is not re-exported from cli.ts or index.ts, and nothing else in the repo references it. It was already unreachable before this PR; this PR edited it (the floor and the message), which is the moment to settle it either way. Dropped rather than wired into the entrypoint, because as written it would fail silently there: it reports through `process.logger?.error`, and the entrypoint has not set up a logger yet, so the optional chain short-circuits and the process exits 1 with no message at all. `engines: ">=22"` is the enforcement, and npm surfaces it on install. The spec block covering the function goes with it. Verification: tsc -p tsconfig.json clean, prettier clean, src/utils/spec 44 tests pass (was 46).
|
Both addressed.
I did not wire it into the entrypoint, because as written it would fail silently there: it reports through |
There was a problem hiding this comment.
Re-review at the new head d96585a. The delta since the prior review at 9bb2d8d is the fetch-depth: 0 on the release checkout (.github/workflows/npmpublish.yml:28) and the removal of the unreachable checkNodeVersion function and its spec (src/utils/utils.ts, src/utils/spec/utils.spec.ts); the rest of the diff is unchanged from the earlier heads. Hardening checks run at the head: manifest/lockfile sync including the release tree, .npmrc, gitleaks hook, CI audit gate, depcheck triage, secrets scan. CI (test (22)) is green on the head. One finding:
- package.json:81-82 - jwt-decode 3.1.2 and lodash.groupby 4.6.0 are declared as runtime dependencies but nothing in the tree references them: no import of jwtDecode or groupBy anywhere in src, docs, or scripts (lodash.uniqby beside them is used; these two are not), so every consumer of @sasjs/cli installs them for nothing. Also package.json:121 - tslib 2.8.1 is a devDependency with no reference anywhere in the tree and no importHelpers in tsconfig. Pre-existing rather than introduced by this PR, surfaced by the depcheck pass; advisory, but a manifest-cleanup PR like this one is the natural moment to drop all three.
1 finding above for review.
d96585a to
b82f4fd
Compare
There was a problem hiding this comment.
Re-review at the new head b82f4fd; the branch was force-pushed (history diverged from d96585a), so this covers the full current base...main diff. The delta adds the release-toolchain audit step (release/audit.mjs hooked into run-tests.yml and npmpublish.yml) and drops the unreachable checkNodeVersion along with its spec - the removal is verified complete, no references remain anywhere in src or the manifest. The audit script runs clean here (12 exempt, exit 0) and the release manifest/lock root agree. Hardening checks at the head: exact pins throughout, .npmrc ignore-scripts+save-exact, gitleaks hook pinned v8.30.1, prod-scoped strict audit gate in CI (run-tests.yml:33), npm ci used throughout, lockfile engines root in sync (">=22"), depcheck triage with zero ghosts, no secrets in the diff, CI (test (22)) green. Finding:
- package.json:81-82,121 (carried from the d96585a review, still present at this head) - jwt-decode 3.1.2 and lodash.groupby 4.6.0 are declared as runtime dependencies and tslib 2.8.1 as a devDependency, but nothing in the tree references any of the three: no import of jwtDecode, groupBy, or tslib anywhere in src, docs, or scripts (verified by full-tree grep at the head; lodash.uniqby beside them is used, these are not), and no importHelpers in tsconfig to justify tslib. Every @sasjs/cli consumer installs the first two for nothing. Pre-existing rather than introduced by this PR, surfaced by the depcheck pass; advisory, but a manifest-hygiene PR like this one is the natural moment to drop all three (plus @types/lodash.groupby at :103, which loses its reason if lodash.groupby goes).
1 finding above for review.
…udit The audit caught a new advisory, which is what it is for: GHSA-rj75-hqrm-r3gf, moderate, quadratic complexity in flat selector parsing. It cannot be fixed from here. The vulnerable copy is npm's own bundled dependency - release/node_modules/npm/node_modules/postcss-selector-parser - so no resolver change reaches it, and 11.21.0 is the last npm 11.x while @semantic-release/npm requires ^11. The eventual fix is that plugin moving to npm 12. It is also unreachable in this usage: the parser reads this repo's own release config, not the commits being matched. Named with that reasoning rather than silenced, so the audit still fails on anything new. 13 exempt, exit 0.
…udit The audit caught a new advisory, which is what it is for: GHSA-rj75-hqrm-r3gf, moderate, quadratic complexity in flat selector parsing. It cannot be fixed from here. The vulnerable copy is npm's own bundled dependency - release/node_modules/npm/node_modules/postcss-selector-parser - so no resolver change reaches it, and 11.21.0 is the last npm 11.x while @semantic-release/npm requires ^11. The eventual fix is that plugin moving to npm 12. It is also unreachable in this usage: the parser reads this repo's own release config, not the commits being matched. Named with that reasoning rather than silenced, so the audit still fails on anything new. 13 exempt, exit 0.
There was a problem hiding this comment.
Re-review at the new head 70c4ace; the delta since the review at b82f4fd is the postcss-selector-parser exemption in release/audit.mjs alone. Verified at the head: node release/audit.mjs reports 13 exempt, exit 0, the file is byte-identical to the other four repos' copies, and CI (test (22)) is green. The delta introduces no new findings. One carried finding remains open:
- package.json:81-82,103,121 (carried from the b82f4fd review, still present at this head) - jwt-decode 3.1.2 and lodash.groupby 4.6.0 are declared as runtime dependencies and tslib 2.8.1 as a devDependency, but nothing in the tree references any of the three: no import of jwtDecode, groupBy or tslib anywhere in src, docs or scripts (re-verified by full-tree grep at this head), and no importHelpers in tsconfig to justify tslib. Every @sasjs/cli consumer installs the first two for nothing. Pre-existing rather than introduced by this PR, surfaced by the depcheck pass; advisory, but a manifest-hygiene PR like this one is the natural moment to drop all three, plus @types/lodash.groupby at :103 which loses its reason if lodash.groupby goes.
1 finding above for review.
Surfaced by the depcheck pass and carried through several reviews. Verified by full-tree grep before removing: - jwt-decode 3.1.2 and lodash.groupby 4.6.0 were runtime dependencies, but nothing in src, docs or scripts imports jwtDecode or groupBy. lodash.uniqby beside them is used; these two were not, so every @sasjs/cli consumer installed them for nothing. - tslib 2.8.1 was a devDependency with no reference anywhere, and tsconfig sets no importHelpers to justify it. - @types/lodash.groupby 4.6.9 loses its reason with lodash.groupby gone. Verified after removal: `tsc --noEmit` clean, and the utils and config specs run 83 tests, all passing.
|
Carried finding addressed in jwt-decode, lodash.groupby, tslib, @types/lodash.groupby. All four removed. I re-verified your grep before acting rather than taking it on trust: zero references to Verified after removal: This finding was carried through three reviews and I had not acted on it - that is a miss on my part. |
There was a problem hiding this comment.
Re-review at the new head ef17b9a; the delta since the review at 70c4ace is the manifest cleanup alone: jwt-decode, lodash.groupby, @types/lodash.groupby and tslib dropped from package.json with the lockfile regenerated. Verified at the head: nothing in src, docs or scripts references any of the four, lodash.groupby is gone from the lockfile entirely, and jwt-decode and tslib remain only as transitive entries under @sasjs/utils and emnapi, which is correct rather than a leftover. depcheck triage at the head reports no ghosts and no confirmed-unused declarations. The carried finding from 70c4ace is addressed; the delta introduces no new findings.
No blocking issues.
What
Five changes, all in the manifest, one source file, and the workflows.
Supported node.
engines.nodewas^14.18.0 || >=16.10.0. Node 14, 16, 18 and 20 are all EOL; the oldest line still receiving security fixes is 22, which is what CI already runs. Raised to>=22. The old range was also already false: the production tree requires>=18.Declare
axios.src/importsaxiosbut the manifest never declared it - it resolved through another package's tree, so a change in that tree could take the CLI out. Now declared and pinned exactly, like the rest.Correct the node check.
checkNodeVersioninsrc/utils/utils.tsrejected anything below major 12, which no longer agreed with anything. Raised to 22 to match the manifest, and the two dead npm scripts around it (nodeVersionMessage,checkNodeVersion) are gone.Release tooling.
semantic-releaseand@semantic-release/execno longer resolve fromnpxat release time. They live inrelease/with their own package.json and lockfile, installed by the publish workflow withnpm ci --prefix release. That pins the resolved versions and their integrity hashes instead of fetching the transitive tree fresh on every release, which is what thenpx -pform did - a form that runs with publish credentials.The toolchain's own advisories, in their own scope. Keeping that tree out of the package's audit is correct - it is not shipped - but it must not become invisible either: it runs in CI with publish credentials.
release/audit.mjsaudits it separately and by name. Twelve advisories are exempt, each with the reason it cannot be fixed from this repo (the ranges cover the newest published version) and the reason it is unreachable here (thebracesdenial of service needs an attacker-controlled brace pattern, and the patterns come from this repo's own release config). Anything else fails the build, and an exemption that stops appearing is reported so it can be deleted rather than left to rot. It uses--package-lock-only, so it needs no install and gates every PR as well as the release.Verification
package-lock.jsonis unchanged at 743 entries - the CLI already kept the toolchain out of it;release/package-lock.jsonis new with 453npm audit --omit=dev --audit-level=low: 0 vulnerabilities (the CI gate)node release/audit.mjs: 12 exempt, exit 0npm run lint,npm test: cleanrelease/node_modules