Repository navigation
fix(utils): drop the Node crypto import, and tighten what the package ships - #274
Conversation
encodeToBase64 only needs 100 random bytes as obfuscation padding, so the
Node `crypto` import buys nothing the Web Crypto global does not provide. The
prefix is not a secret - decodeFromBase64 strips it by fixed offset - and
every supported runtime exposes crypto.getRandomValues (node 22 is now the
floor across the estate, browsers and the VS Code extension host all have it).
Dropping the import removes the crypto-browserify -> asn1.js -> vm-browserify
chain (555 KB of AES/RSA/ECDSA polyfills plus the `eval()` in vm-browserify's
shim) from any browser bundle that pulls in @sasjs/utils. That dual-use shape
is what makes npm's publish scanner slow to clear a package, and it is dead
weight for consumers that never call encodeToBase64.
Verification:
- base64.spec.ts pins the 200-char hex prefix (the offset decodeFromBase64
strips) and asserts each call is unique
- npx tsc --noEmit: clean
- npx jest --coverage: 39 suites, 440 tests pass; base64.ts 100%
- npm run lint: clean
- emitted build/utils/base64.js contains no require('crypto')
0da3ad8 to
896589b
Compare
![sasjs-dev[bot]](https://avatars.githubusercontent.com/u/67858352?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full base...head diff (2 files, +67/-3) plus hardening checks: dependency pinning (all exact), .npmrc (ignore-scripts + save-exact), lockfile, pre-commit gitleaks hook (pinned v8.30.1, native hook verified not neutered), CI audit step (npm audit --production --audit-level=low, no continue-on-error), depcheck triage, and npm audit against the lockfile - all clean.
-
src/utils/base64.ts:33-33 - encodeToBase64 still calls Buffer.from(randomHexPrefix() + str).toString('base64'), so the browser bundle keeps a Buffer dependency unless the bundler polyfills it. The stated goal (no crypto shim in the browser bundle) is met - the crypto-browserify chain came from require('crypto') alone, and src/fs/hash.ts:3 still imports createHash from 'crypto' but nothing the adapter's browser bundle reaches imports the utils barrel's fs module (adapter imports are all deep: @sasjs/utils/types, /error, /file, /time, /auth, /input, /logger; only fs/internal/helper.ts pulls the utils barrel from fs, and the adapter never imports fs). However, if a consumer imports encodeToBase64 from the @sasjs/utils barrel, the barrel's export * from './fs' (src/index.ts:11) drags in fs/hash.ts and its crypto import right back - the crypto-free claim holds only for deep imports (which is what the adapter does). For the barrel path, Buffer also remains: browsers lack Buffer natively, so a bundler will inject a polyfill for base64.ts's Buffer usage. If the goal is a fully browser-clean encode path, replacing Buffer with btoa/TextEncoder (with a Buffer fallback for Node < 19) would remove the last browser-hostile dependency in this file. Advisory: the change is a strict improvement over crypto.randomBytes; the residual Buffer use is the remaining gap.
-
src/utils/base64.ts:19-26 - Math.random fallback fires only when the global crypto object is absent (Node < 19 without the Web Crypto global). The comment says this is sufficient for padding, which matches the format: decodeFromBase64 (src/utils/base64.ts:37-44) strips a fixed 200-char offset regardless of content, so predictability of the prefix does not break decoding. The one nuance worth noting: the prefix's stated job includes making the encoded value unique and stopping base64-of-a-password from being obvious; Math.random output is predictable, so on such runtimes two rapid successive calls could produce colliding prefixes. Not a security boundary (the prefix was never secret - Node's crypto.randomBytes was used for the same padding), but if uniqueness matters, Date.now() concatenated into the Math.random bytes would harden it. Advisory only.
-
src/utils/spec/base64.spec.ts:33-36 - the fallback test mutates globalThis.crypto via Object.defineProperty with a plain {} - this passes the typeof crypto !== 'undefined' check and forces the Math.random branch as intended, but note the restore in the finally block re-defines the property; if the surrounding environment defines crypto as non-configurable (some runtimes), the test's defineProperty would throw before the try block starts, failing the suite rather than skipping. Minor robustness note; the repo's CI (test (22)) passed on the head, so this is not currently biting.
-
The 200-char prefix length and 100-byte size (PREFIX_HEX_LENGTH = 200 hex chars from 100 bytes) preserve decode compatibility with existing stored credentials: decodeFromBase64's substring(200) offset is unchanged, so values encoded by the old crypto.randomBytes(100).toString('hex') prefix decode identically under the new implementation. No stored-credential break.
-
Commit subject "fix(utils): encode credentials without the Node crypto module" is a valid Conventional Commit with a release-driving type (fix) for a user-visible change; the repo releases via semantic-release (publish.yml), so this ships a patch release as intended.
2 findings above for review (residual Buffer use and the barrel-path crypto re-entry worth documenting or fixing, plus the minor test-robustness note). The format-compatibility and hardening checks are clean; CI is green on the head.
semantic-release and its two plugins were devDependencies. That tree is where the unfixable advisories live - micromatch -> braces, and the npm package @semantic-release/npm bundles - and none of them has a patched release. Run it the way sasjs/cli does instead: npx fetches the pinned trio for the release run only, so none of it enters the lockfile. The tree drops from 747 lock entries to 315, and the dead `semantic-release` script goes with it. Verification: - npm ci: 305 packages - npm audit --production --audit-level=low: 0 vulnerabilities (the CI gate) - npm run lint: clean - npx jest: 39 suites, 440 tests pass The full-tree audit still reports advisories. They come from jest's own tree, which carries micromatch -> braces as well, not from the release tooling.
|
Thanks - splitting these into addressed, stale, and deliberately deferred. Residual Barrel-path crypto re-entry - correct, and worth stating plainly rather than leaving implied: the crypto-free claim holds for deep imports, which is what the adapter uses, not for the barrel. Math.random fallback (base64.ts:19-26) and the Format compatibility and the commit subject - agreed on both; no stored-credential break, and |
There was a problem hiding this comment.
Reviewed the full base...main diff at the new head (5 files; the delta since the last review drops the Math.random fallback and the release tooling) plus hardening checks: pinning, .npmrc, lockfile, gitleaks hook, CI audit gate, depcheck triage, secrets scan, and npm audit against the head lockfile. CI (test (22)) is green on the head. The prior review's test-robustness finding is resolved by the fallback's removal. Findings on the current head:
-
src/utils/base64.ts:9-12 - the Math.random fallback is gone and randomHexPrefix calls crypto.getRandomValues directly, so encodeToBase64 now throws ReferenceError on any runtime without the Web Crypto global: Node < 19 (18 and earlier lack globalThis.crypto), and older browsers (pre-2021 Safari, pre-37 Chrome without the flag). utils ships no "engines" field, so npm cannot warn consumers either. decodeFromBase64 is unaffected (pure Buffer), but every encode call hard-fails on such runtimes. Either declare the floor ("engines": {"node": ">=19"} - matching the fix's own claim of the oldest supported LTS elsewhere in the org) or keep a guarded fallback. If intentional, the breaking change deserves a note in the release.
-
src/utils/base64.ts:15 - encodeToBase64 still uses Buffer.from(...).toString('base64'), so the barrel path keeps a browser polyfill dependency; only the crypto shim was removed. The adapter is unaffected (its imports are deep), but "export * from './fs'" at src/index.ts:11 drags fs/hash.ts:3 "import { createHash } from 'crypto'" back into any barrel consumer's bundle, re-introducing the very crypto-browserify alias chain this PR removes for deep imports. If the barrel is meant to be browser-clean too, re-exporting base64 directly and keeping fs off the public barrel (or splitting fs into a subpath export) is the structural fix. Advisory, unchanged in substance from the prior review.
-
.github/workflows/publish.yml:72-78 - the release toolchain is now "npx -p @semantic-release/exec@6.0.3 -p @semantic-release/git@10.0.1 -p semantic-release@25.0.9 -- semantic-release". Pinning correct; npx auto-installs in non-interactive CI without prompting (verified under npm 11). Same un-locked-transitive-tree caveat as the org's other release workflows: the top levels are pinned but the resolved tree is fetched ad hoc at release time, so a compromised transitive dependency executes with publish credentials. Consider a lockfile (or npm-shrinkwrap) for the toolchain. Advisory.
-
The 200-hex prefix format is unchanged (PREFIX_HEX_LENGTH = 200, decode strips substring(200)), so existing stored credentials decode identically. Verified against the new spec tests at src/utils/spec/base64.spec.ts:18-31. No break.
3 findings above for review.
Review finding: with the Math.random fallback gone, encodeToBase64 calls crypto.getRandomValues directly, so it throws on any runtime without the Web Crypto global - node 18 and earlier, and pre-2021 browsers. utils declared no "engines" field, so npm could not warn a consumer either. Declare ">=22": the estate's floor, and the oldest supported LTS. The lockfile root is regenerated so the two agree. Verification: - npx jest: 39 suites, 440 tests pass - npm run lint: clean - package-lock.json root now records engines >=22
|
All three addressed or answered.
|
|
The two items I deferred now have issues, so they are tracked rather than dropped:
Neither blocks this PR. |
…ling from a locked tree Two changes to what leaves this repo. `files` allowlist: the package had none, so npm fell back to `.gitignore` and the tarball carried `.git-hooks`, `.github`, `.vscode`, `scripts/`, `updateCli`, `updateServer`, `CONTRIBUTING.md`, `PULL_REQUEST_TEMPLATE.md` and a dead `checkNodeVersion.js` - 364 files. It now carries the build output and the README: 347 files, nothing that is not part of the library. This also keeps the new `release/` tree out of the package. Release tooling: it now lives in `release/` with its own package.json and lockfile, installed by the publish workflow with `npm ci --prefix release`. Locked, so the resolved versions and their integrity hashes are pinned rather than fetched fresh at release time. Verification: - staged publishInit + npm pack --dry-run: 347 files, build output only - npx jest: 39 suites, 440 tests pass; npm run lint: clean - 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.
sasjs-dev
Bot
changed the title
…p release] 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 6cf8bfa; the delta since the prior review at 02fa4b9 is the engines declaration (package.json:7-9 ">=22", lockfile root synced), a files allowlist, the release-tooling lock tree, and the scoped release audit script. The prior node-floor finding is addressed (the encode path's crypto.getRandomValues requirement is now declared), and the toolchain-lock posture note is resolved by the locked tree (release/package.json + release/package-lock.json, npm ci --prefix release at publish.yml:72-79, plugin set exec 6.0.3 / git 10.0.1 / semantic-release 25.0.9 matches the release config, lockfile root in sync). CI (test (22)) is green on the head. The delta since adds release/audit.mjs (a scoped audit of the release toolchain, sound) and its workflow hooks. Findings on the current head:
-
package.json:36-55 (files allowlist) - the new allowlist drops the spec fixtures the published package has been shipping to real consumers: 3.6.5 (the current release, verified on unpkg) ships /file/spec/expectedOutputs.js, /file/spec/expectedOutputs.js.map and /file/spec/expectedOutputs.d.ts. Nothing in the allowlist matches "file/spec/" (the entries are "file/" - which does match - but ".d.ts" at root and the directory globs do not exclude it either way, so verify against a real npm pack: on the publish-time layout, publishInit at package.json:60 copies build/ to the root, and build/file/spec/expectedOutputs.* will be included by the "file/" entry). Wait - "file/" includes the spec directory, so the fixtures keep shipping; the real gap is the reverse: "index.js.map" is allowed at root but the allowlist's "*.d.ts" is root-only while the compiled tree also produces nested .d.ts files under each directory glob, which the directory entries already cover. Net: no exclusion regression found after tracing the globs against the publish layout - but the change is unverified by any pack test in CI, and 3.7.0 will be its first exercise. Run "npm pack --dry-run" on the publishInit layout once before merging (or add it to publishInit) to confirm the tarball contents - 363 files shipped in 3.6.5, the allowlist should cut that materially.
-
package.json:7-9 - "engines": { "node": ">=22" } declares the floor for the crypto-free encode path (crypto.getRandomValues needs the Web Crypto global, Node >= 19), so the declaration is sound; note only that this is a user-visible tightening (Node 18 consumers of encodeToBase64 now get an EBADENGINE warning rather than silent behaviour), which the fix type covers.
1 finding above for review (the pack-verification gap; the engines tightening is noted, not a defect).
|
Verified on the publishInit layout as you asked, and the numbers are now in the PR body: Your trace of the globs matches what the pack shows: On adding a pack gate to CI: I did not, because for this repo it would pass vacuously. The allowlist resolves against the publishInit layout, so a pack against the working tree reports 2 files and can never see a leak; a meaningful gate has to stage the build first. The adapter got one ( The engines note I have taken as you framed it - a deliberate, user-visible tightening. |
…lease-suppressing markers (#277) Two fixes, both about something being added that should not be. Target inserted a default `deployConfig` of `{deployServicePack: true, deployScripts: []}` whenever a target declared none, so every saved sasjsconfig carried a deployConfig the user never wrote - the example given in #221. That insertion is gone. `toJson` likewise inserted defaults for buildConfig, jobConfig, serviceConfig, streamConfig, testConfig, the sasjs folders and syncDirectories unless it was called with `withDefaults: true`. That argument now defaults to false, so `toJson()` returns the four mandatory attributes and whatever else was explicitly defined. Closes #221. The commit-msg hook now rejects `[skip release]` and `[skip ci]`, and checks the subject line only. GitHub builds the squashed commit's body from the list of subjects, and both markers act on the whole squashed commit: semantic-release drops any commit whose message matches `[skip release]` (lib/definitions/plugins.js), and `[skip ci]` makes GitHub skip the push-triggered workflow. Either one silently suppresses the release for the entire PR - which is what happened to this repo's #274, where two `chore(ci): ... [skip release]` subjects went into the squash body, the crypto fix merged, and no version was published. The body never reaches the squash commit, so a message that merely mentions the markers is accepted. Verification: - npx tsc --noEmit: clean - npx jest: 39 suites, 440 tests pass - npm run lint: clean - the hook against 8 subject shapes, including unscoped and breaking types Co-authored-by: sasjs-dev[bot] <sasjs-dev[bot]@users.noreply.github.com>
## [3.7.1](v3.7.0...v3.7.1) (2026-10-05) ### Bug Fixes * **ci:** make the release dispatchable ([#278](#278)) ([bf2f677](bf2f677)), closes [#277](#277) * **utils:** drop the Node crypto import, and tighten what the package ships ([#274](#274)) ([0f79733](0f79733)) * **utils:** insert no config a target does not declare, and reject release-suppressing markers ([#277](#277)) ([58baebb](58baebb)), closes [#274](#274)
|
🎉 This PR is included in version 3.7.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
The subject-only check caught the release marker and the ci marker, but missed their upper-case forms, the reversed word order, and the four GitHub forms - ci-skip, no-ci, skip-actions and actions-skip - which suppress a run just as completely. Both upstream filters are case-insensitive and accept any run of whitespace, so the pattern now mirrors them. This matters because both act on the WHOLE squashed commit rather than on the one commit carrying the marker: the release tooling drops any commit whose message matches, and GitHub skips the push-triggered workflow outright. Either one silently suppresses the release for the entire PR. Not hypothetical - it happened here. #274 carried two chore(ci) subjects with the release marker, so the squashed commit on main was filtered out whole, the crypto fix merged, and no version was published. The pattern uses a POSIX character class rather than backslash-s. Backslash-s is a GNU extension that BSD grep does not honour, so on macOS - where contributors run this hook - the pattern would stop matching every real marker and the guard would fail open, which is the failure this commit exists to close. Verified against 20 subject shapes: every conventional type with and without a scope passes, the release tooling's own commit is exempted, both release forms and all four GitHub forms are rejected in upper and lower case and with multiple spaces, a bare non-conventional subject is rejected, and a marker appearing only in the body still passes because the body never reaches the squash.
* fix(ci): widen the marker check to the filters it guards The subject-only check caught the release marker and the ci marker, but missed their upper-case forms, the reversed word order, and the four GitHub forms - ci-skip, no-ci, skip-actions and actions-skip - which suppress a run just as completely. Both upstream filters are case-insensitive and accept any run of whitespace, so the pattern now mirrors them. This matters because both act on the WHOLE squashed commit rather than on the one commit carrying the marker: the release tooling drops any commit whose message matches, and GitHub skips the push-triggered workflow outright. Either one silently suppresses the release for the entire PR. Not hypothetical - it happened here. #274 carried two chore(ci) subjects with the release marker, so the squashed commit on main was filtered out whole, the crypto fix merged, and no version was published. The pattern uses a POSIX character class rather than backslash-s. Backslash-s is a GNU extension that BSD grep does not honour, so on macOS - where contributors run this hook - the pattern would stop matching every real marker and the guard would fail open, which is the failure this commit exists to close. Verified against 20 subject shapes: every conventional type with and without a scope passes, the release tooling's own commit is exempted, both release forms and all four GitHub forms are rejected in upper and lower case and with multiple spaces, a bare non-conventional subject is rejected, and a marker appearing only in the body still passes because the body never reaches the squash. * fix(ci): name the postcss-selector-parser advisory in the toolchain audit 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. * chore(ci): format the release audit script for the prettier gate sas-language is the only one of the five repos that lints release/, and its prettier config sets printWidth 100, so the exemption block needed reformatting to pass that gate. The file stays byte-identical across all five. --------- Co-authored-by: sasjs-dev[bot] <sasjs-dev[bot]@users.noreply.github.com>
1 participant
What
Four changes to the package, all about what consumers download and what this repo runs.
Drop the Node
cryptoimport.encodeToBase64prepends 100 random bytes (200 hex characters) to the value and base64s the result;decodeFromBase64strips the prefix by fixed offset. The prefix is obfuscation padding, not a secret, so it does not need Node'scryptomodule - the Web Crypto global does the job on every runtime we support (node 22 is the floor across the estate; browsers and the VS Code extension host exposecrypto.getRandomValues).Ship only the built output. The package had no
filesallowlist, so npm fell back to.gitignoreand the tarball carried.git-hooks,.github,.vscode,scripts/,updateCli,updateServer,CONTRIBUTING.md,PULL_REQUEST_TEMPLATE.mdand a deadcheckNodeVersion.js. It now carries the build output and README only: 364 files -> 347.Supported node.
engines.nodewas>=14; 14, 16, 18 and 20 are all EOL. Raised to>=22, the oldest supported LTS.Release tooling.
semantic-releaseand its two plugins are no longer devDependencies. They live inrelease/with their own package.json and lockfile, installed by the publish workflow withnpm ci --prefix release, so their tree -micromatch->braces, and the npm package@semantic-release/npmbundles - stays out of the package's dependencies and out ofnpm audit, while the resolved versions and their integrity hashes are pinned.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.Why it matters
The adapter's browser bundle currently drags in
crypto-browserify->browserify-sign->parse-asn1->asn1.js->vm-browserify: 555 KB of AES/RSA/ECDSA polyfills plus aneval()from the vm shim, pulled in solely by this one import. That dual-use shape (dynamic code execution plus a full crypto suite in one bundle) is what makes npm's publish scanner slow to clear a package, and it is dead weight for consumers that never callencodeToBase64.Scope, stated precisely: this holds for deep imports, which is what the adapter uses (
@sasjs/utils/types,/error,/file,/time,/auth,/input,/logger). The barrel still re-enterscrypto, becausesrc/index.tsre-exports./fsandfs/hash.tsimportscreateHash. That is #275.Verification
base64.spec.tspins the 200-character hex prefix (the offsetdecodeFromBase64strips) and asserts each call is uniquenpx tsc --noEmit: cleannpx jest --coverage: 39 suites / 440 tests pass,base64.ts100%npm run lint: cleanbuild/utils/base64.jscontains norequire('crypto')publishInit+npm pack --dry-run: 347 files (was 364), build output and README onlypackage-lock.json: 747 -> 315 entries;release/package-lock.json: 479 entriesnpm audit --production --audit-level=low: 0 vulnerabilities (the CI gate)node release/audit.mjs: 12 exempt, exit 0Related
Companion change to the estate-wide engines bump to
>=22(the oldest supported LTS): sasjs/cli#1479, sasjs/lint#251, sasjs/sas-language#8, sasjs/test-framework#280, sasjs/adapter#919.Follow-ups filed rather than folded in: #275 (the barrel re-exports
fs, so a bare@sasjs/utilsimport still pulls the crypto chain) and #276 (encodeToBase64/decodeFromBase64depend onBuffer).