Repository navigation
fix(auth): address the SASjs Server authorization screen as a path - #1475
Conversation
The SASjs Server interface used a hash router, so its authorization code screen lived at /#/SASjsLogon. It is a real path on the interface now, so the URL the CLI prints for the authorization code flow uses the path. A fragment is never sent to the server, so the path form is what makes the URL work as a deep link rather than depending on the client moving it.
![sasjs-dev[bot]](https://avatars.githubusercontent.com/u/67858352?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the diff (2 files: getAuthUrl path change and new unit tests) plus repo hardening at this head.
- .github/workflows/run-tests.yml:33 - the pipeline is red at this head: the Check npm audit step (npm audit --omit=dev --audit-level=low) fails on the high-severity braces advisory GHSA-vfj7-8cjw-p6xm, reachable via shelljs@0.10.0 -> fast-glob -> micromatch -> braces 3.0.3 (reproduced locally against the lockfile at this head). The advisory is upstream and unrelated to the one-line change, but it also stops the Linux job before the test suite runs, and a red pipeline means the PR is not merge-ready. Clear it before merge: npm's suggested fix is downgrading shelljs to 0.8.5 (flagged breaking; the same advisory was just cleared in Data Controller's sas workspace by pinning shelljs 0.8.5 in overrides), or hold until braces ships a patched release.
- src/utils/auth.ts:19 - the SASjs branch now emits serverUrl + "/SASjsLogon?client_id=..." as a real path, but that path only resolves once sasjs/server#463 lands: #463 is still open, the latest server release (v1.19.0, 2026-10-01) predates it, and before #463 only "/" served the app shell - which is exactly why the hash form was used. Every SASjs Server deployed today therefore fails to resolve the printed URL, so a CLI released with this change hands users a dead link in the authorization-code flow. Ship after a server release carrying #463, or keep the hash form until the server version can be assumed.
2 findings above for review.
|
The Why the advisory is present but not reachableCVE-2026-93687 / GHSA-vfj7-8cjw-p6xm is a stack overflow in It reaches the CLI through The reason it is not a real exposure is what feeds it. Enumerating every and The advisory's CVSS vector is Attack Vector: Network with no privileges and no user interaction, which describes a service globbing patterns supplied by a remote caller. The CLI is a local tool with no such surface. What the exception is
Verified both directions: it passes here ( The script records the condition that revokes it: if the CLI ever globs a pattern it did not write itself - from |
Coverage reportTotal coverage
Show new covered files 🌑Coverage of new files
Show files with reduced coverage 🔻Reduced coverage
Report generated by 🧪jest coverage report action from db1638f |
|
Both findings addressed - the second one is correct and changes when this can merge.
|
There was a problem hiding this comment.
Re-review at the new head 97faf58; scope is the delta since the prior review at 5c4837f (the audit-allowlist commit) plus carried state. The pipeline finding is settled at this head, and the earlier auth.ts concern is moot now that sasjs/server v1.21.0 (2026-10-03) ships the /SASjsLogon path.
- .github/workflows/npmpublish.yml:42 - the release workflow still runs
npm audit --omit=dev --audit-level=low, which fails on the braces advisory GHSA-vfj7-8cjw-p6xm (production chain shelljs@0.10.0 -> fast-glob -> micromatch -> braces 3.0.3, confirmed against the lockfile at this head; braces 3.0.3 is still the latest published release, so no upgrade clears it). This PR swaps the PR gate in run-tests.yml for scripts/audit-production.js but leaves the release gate as it was, so the job that runs on push to main fails at the audit step before Semantic Release runs - the patch this PR carries would never be published. Run scripts/audit-production.js there as well, or the allowance only greens PR builds.
1 finding above for review.
97faf58 to
e455ac5
Compare
|
Taking your suggestion - the script is gone and the advisory is cleared properly instead of allowed. Why there is no upstream fixI checked whether anything in the chain has moved: Every current version still requires the vulnerable What changed
Verified
The second finding stands: this should not merge until a server release carries |
shelljs was the only production dependency reaching braces - 0.10.0 brings fast-glob -> micromatch -> braces, and braces carries GHSA-vfj7-8cjw-p6xm with no fixed release to move to. Pinning shelljs down cleared the audit, but it left the CLI on a 2022 release of a runtime dependency, so the dependency is gone instead. src/utils/shell.ts provides the five operations the CLI actually used - run a command, copy a folder, remove files and folders, list the entries matching a pattern, and find an executable - on spawnSync, fs and the `copy` that @sasjs/utils already exports. The shapes match what the call sites expect, so behaviour is unchanged: a command still answers with stdout/stderr/code, and `cp -r <dir>/. <dest>` still copies a directory's contents rather than the directory itself. Only `*` is supported in the listing patterns, which is all the CLI's three patterns use - `*main`, `*master` and `*-main`, each matched against a single directory. That is why no glob engine is needed, and `rm` expands a pattern for the same reason shelljs did. The three specs that mocked shelljs now mock the new module, and the mocks keep intercepting the call sites because both compile to a property access on the module. The production tree no longer contains shelljs or braces, and `npm audit --omit=dev --audit-level=low` reports 0 vulnerabilities.
e455ac5 to
faadabb
Compare
|
Taken further - the dependency is gone rather than pinned. Why pinning was not enough
What replaces it
The shapes match what the call sites expect, so the behaviour they rely on is unchanged - a command still answers with No glob engine is needed: the only patterns are Verified
One thing worth recording separately: the mocked suite is flaky under parallel workers. The ordering finding still stands: this should not merge until a server release carries |
There was a problem hiding this comment.
Re-review at the new head faadabb (history was force-pushed since e455ac5); scope is the full current base...head diff. The prior braces finding is moot at this head: shelljs is gone from package.json, package-lock.json and src/, and npm audit --omit=dev --audit-level=low against this lockfile reports 0 vulnerabilities (braces 3.0.3 remains only as a dev-only transitive of chokidar). Both CI checks at this head are green. Hardening (.npmrc ignore-scripts/save-exact, gitleaks pre-commit at v8.30.1, npm ci + strict audit in both workflows) carries clean from prior reviews.
-
src/utils/shell.ts:38 - exec() loses shelljs's 20MB output buffer and its failure mapping. spawnSync's default maxBuffer is 1MB (verified: a command emitting 3MB of stdout comes back with status null, signal SIGTERM, error ENOBUFS, truncated 1.1MB stdout), and shell.ts:52 maps that to code 0 - a buffer overflow is reported as success. shelljs used a 20MB default and reported overflow as code 1 with 'maxBuffer exceeded' on stderr. Concrete case: src/utils/utils.ts:344 runs a user deploy script with silent:true and, on code 0, writes stdout to a log; a chatty script now gets killed mid-run and logged as succeeded. Pass an explicit maxBuffer (e.g. 20 * 1024 * 1024) to spawnSync and map result.error (ENOBUFS) to a non-zero code with a clear stderr message instead of
result.status ?? 0. -
src/utils/utils.ts:49,64,78 - cp() now rejects where shelljs.cp silently continued. The previous
shelljs.cp('-r',${...}[0]}/., folderPath)set shelljs's error state and returned an empty ShellString when the source was missing (e.g. ls() found no match, so [0] is undefined and the path isundefined/.), and createApp carried on to the errorCallback for the download check. fs-extra copy rejects with ENOENT on the same input (verified), and because these are async executors, the rejection leaves the outer promise unsettled-with-an-unhandled-rejection rather than reaching the reject at utils.ts:55/70/84 -sasjs createwould hang or crash with an unhandled ENOENT instead of reporting the friendly error. Guard the ls() result: ifls(pattern)[0]is undefined, call errorCallback (the existing 404 path at utils.ts:160 does exactly this) before invoking cp, and/or .catch() the cp() rejections onto reject.
2 findings above for review.
Two ways the replacement was less careful than shelljs, both raised in review. exec() dropped shelljs' output ceiling and its failure mapping. spawnSync defaults maxBuffer to 1MB where shelljs allowed 20MB, so a chatty command was killed mid-run; worse, a killed command comes back with status null and the reason on `error`, and `status ?? 0` reported that as a success - a deploy script that overflowed was logged as having run. The ceiling is 20MB again and an error result is reported as a non-zero code with the reason on stderr. cp() rejects where shelljs' cp quietly carried on. When ls() found no match the source became `undefined/.`, which shelljs accepted and fs-extra refuses with ENOENT - and because these bodies run inside async Promise executors, that rejection never reached the caller's reject, so `sasjs create` would hang or crash instead of reporting the friendly error. The match is now checked before the copy, and everything from the unzip onwards is caught and routed to the error callback that settles the promise. That also covers a download that is not a zip, which previously hung the same way.
|
Both findings fixed in
|
There was a problem hiding this comment.
Re-review at the new head 3ea8d82; scope is the delta since the prior review at faadabb plus carried state.
-
src/utils/utils.ts:180 - the
if (!extractedFolder)guard returns through errorCallback without stopping the spinner, while the sibling catch branch at src/utils/utils.ts:192 stops it first. On this path the unzip produced nothing, so the last spinner frame stays on the terminal above the error message. Call spinner.stop() before errorCallback to match the catch branch. -
src/utils/shell.ts - the new module (glob-to-regex ls, exec failure mapping, rm expansion, which) has no direct spec: every suite that touches it (downloadFile.spec.ts, create.spec.ts, generateDocs.spec.ts) replaces it with a jest mock. The overflow defect caught in the prior review is the class of failure a direct spec would have caught, and the new maxBuffer + result.error mapping that fixes it is likewise untested. Add src/utils/spec/shell.spec.ts covering exec success, non-zero exit, and an error result, and ls matching plus the './' prefix rule.
2 findings above for review.
createApp downloads the seed zip and unpacks it in the process working directory, and so does the docs submodule and the template probe. Two runs in one process share that directory, so two apps created at once overwrite each other's archive mid-extract and the loser fails on an invalid zip. That is what the mocked suite does whenever jest runs more than one spec at a time. Each run now unpacks into its own temporary directory, which is removed when it is done, and the docs unpack into a subdirectory of it: the seed app's folder is still in there and its name ends in `-main` too, so one shared directory would make the docs match ambiguous. The intermediate `rm` calls the shared directory needed are gone with it. The working directory is left untouched, so a create no longer depends on where the process happens to be and two concurrent creates cannot collide.
|
Added The change
Each run now unpacks into its own temporary directory, removed when it is done. The docs get a subdirectory of it, because the seed app's folder is still in there and its name ends in Correction: CI was never affectedI said earlier that this could red the build. That was wrong, and checking it properly is what showed me why: CI runs the mocked suite with Measured properly, under CI's own invocation: So this is not a CI fix. It is still worth having - a create no longer depends on where the process happens to be, leaves the working directory untouched, and two concurrent creates cannot collide - and it takes the parallel failure count from ~24 to ~1. What is still flaky, in parallel onlyRunning specs in parallel still collides on a second, different thing: the specs name their app folders with That reproduces deterministically - It is a test-harness issue (71 call sites across 27 spec files) and |
There was a problem hiding this comment.
Re-review at the new head 6d5d0de; scope is the delta since the prior review at 3ea8d82 (the workspace-isolation change in src/utils/utils.ts) plus carried state. Both CI checks are green at this head; hardening (.npmrc, gitleaks pre-commit, strict audit in both workflows, lockfile) carries clean from prior reviews.
-
src/utils/utils.ts:196 - carried from the prior review and still open: the
if (!extractedFolder)guard returns through errorCallback without stopping the spinner, while the sibling catch at src/utils/utils.ts:207 stops it first. On this path the unzip produced nothing, so the last spinner frame stays on the terminal above the error message. Call spinner.stop() before the errorCallback at line 197. -
src/utils/shell.ts - carried and still open: the shell wrapper (exec, ls, cp, rm, which) has no direct spec - every suite that touches it (downloadFile.spec.ts, create.spec.ts, generateDocs.spec.ts) replaces it with a jest mock. This head makes it more load-bearing, not less: the create flow now depends on ls() matching against a mkdtemp workspace path and on rm() cleaning that workspace in a finally block, none of it directly tested. Add src/utils/spec/shell.spec.ts covering exec success, non-zero exit and error results, ls matching plus the './' prefix rule, and rm glob expansion.
2 findings above for review.
The specs name their app and output folders with generateTimestamp(), which is second-resolution - year, month, day, hour, minute, second, and nothing finer. Two specs in one directory starting within the same second therefore build the same folder name and clobber each other's app, which is what the docs specs do whenever jest runs them in parallel. generateUniqueTimestamp() keeps that readable timestamp and appends the milliseconds and a short random suffix, and the specs use it instead. The pair that collided - generateDot and initDocs, which failed 7 tests on every run - passes consistently now, and the full suite run in parallel goes from 21-25 failures to the one environmental printVersion failure.
|
Fixed the second collision too, in The fix
The 27 specs that used the old one now use it. It is a drop-in, so every prefix pattern ( VerifiedThe pair that collided, run together in parallel, three times: And the full suite run in parallel - which is where the flakiness was visible at all: The one remaining failure is Worth recording: my first attempt broke a specThe first version of this edit damaged That made the expected string no longer match the compiled output. I reverted the commit and redid it line-by-line, touching only import lines and the call sites, and checked that every changed line in all 27 files is either an import or the rename. The spec passes again (17/17), and I confirmed it passes at the commit before the change and on It is a small illustration of why the parallel runs were worth doing: this would have been invisible under |
which() chose between `which` and `where` on isWindows(), but its only caller is
`isLinux() && which('wget')` - isLinux() short-circuits, so the Windows arm can
never run. The branch is removed rather than kept for a caller that does not
exist, and the comment records what would bring it back: something that needs to
ask on Windows.
The behaviour its one caller relies on is unchanged - the path of wget when it
is there, and null when it is not.
|
Verified the behaviour its one caller relies on is unchanged:
|
There was a problem hiding this comment.
Re-review at the new head 6b43982; scope is the delta since the prior review at 6d5d0de (the unique-suffix rename across test specs plus the generateUniqueTimestamp helper in src/utils/test.ts) plus carried state. Both CI checks are green at this head; hardening (.npmrc ignore-scripts/save-exact, gitleaks pre-commit at v8.30.1, strict npm audit in run-tests.yml, npm ci, lockfile) carries clean from prior reviews.
-
src/commands/testing/spec/testing.spec.ts:14, src/utils/spec/file.spec.ts:8, src/utils/spec/zip.spec.ts:11, src/commands/init/spec/init.spec.ts:8, src/commands/compile/spec/compareFolders.spec.ts:6 - the rename to generateUniqueTimestamp left the
generateTimestampimport in place with no remaining usage in each of these five files. The repo's lint is prettier-only, so CI will not flag it; drop the unused import from each. -
src/utils/utils.ts:196 - carried from the prior review and still open: the
if (!extractedFolder)guard returns through errorCallback without stopping the spinner, while the sibling catch at src/utils/utils.ts:207 stops it first. On this path the unzip produced nothing, so the last spinner frame stays on the terminal above the error message. Call spinner.stop() before the errorCallback at line 197. -
src/utils/shell.ts - carried and still open: the shell wrapper (exec, ls, cp, rm, which) has no direct spec - every suite that touches it replaces it with a jest mock. The create flow now depends on ls() matching against a mkdtemp workspace path and rm() cleaning that workspace in a finally block, none of it directly tested. Add src/utils/spec/shell.spec.ts covering exec success, non-zero exit and error results, ls matching, and rm glob expansion.
3 findings above for review.
The `!extractedFolder` guard returned through errorCallback without stopping the spinner, so the last frame stayed on the terminal above the error message. The sibling catch already stops it before reporting.
Every suite that touches src/utils/shell.ts replaced it with a jest mock, so nothing exercised it - which is how the maxBuffer mapping shipped reporting a command killed by the output ceiling as a success. The new spec covers exec (a success, a non-zero exit, a command killed by the ceiling, and one that is merely chatty), ls (suffix matching, the ./ prefix the caller passed, no match, no directory), rm (glob expansion, recursive removal, a single file, a path that is not there), cp (contents into the destination rather than a folder named after the source) and which. The which assertions are Linux-only, because that is the only place the one caller asks.
The move to generateUniqueTimestamp left generateTimestamp imported with no remaining usage in five specs. The lint is prettier-only, so nothing flagged it.
|
All three addressed in
|
There was a problem hiding this comment.
Re-review at the new head db1638f; scope is the delta since the prior review at 6b43982 (the which() Windows-branch drop, the spinner stop on an empty unzip, the direct shell-wrapper spec, and the unused-import cleanup) plus carried state. Both CI checks are green at this head. Reviewed the diff - no blocking issues found.
1 participant
The authorization screen is a real path now
sasjs/server#463gives the SASjs Server interface real paths for its own screens, so the authorization code screen is/SASjsLogonrather than/#/SASjsLogon. This updates the URL the CLI prints for the authorization code flow to match.A fragment is never sent to the server, so the path form is what makes the URL work as a deep link rather than depending on the client moving it. The server keeps a compatibility shim for the old shape, so an older CLI keeps working against a newer server - this change is about emitting the right URL, not about unblocking anyone.
Only the SASjs Server branch changes. The SAS Viya branch already uses a path (
/SASLogon/oauth/authorize), and@sasjs/adapterneeds nothing - it uses the server's/SASLogonlogin route, which was never a hash route.Verified
getAuthUrlhad no coverage; it now has three cases - the SASjs path form, the Viya form, and a guard that the route stays out of the fragment. Theauthsuite is 11/11.tscclean, prettier clean. The 20sasjs docfailures innpm run test:mockedlocally are "doxygen: not found" - CI installs doxygen, this box does not.