[WTF-2695]: Escape package name before using in spawn (#186)

## This PR contains

-   [x] Bug fix
-   [ ] Feature
-   [ ] Refactor
-   [ ] Documentation
-   [ ] Other (describe)

## What is the purpose of this PR?

Preventing shell expansion using single quotes did not work on Windows
as the cmd prompt version of `npm show` would use the quote marks in the
api call.

The closest specification of npm package names comes from the docs where
it says that it must be usable as part of a URL. We don't use
encodeURIComponent() as that would escape @ and / as well, so encodeURI
is enough.

https://docs.npmjs.com/cli/v10/configuring-npm/package-json#name

## Relevant changes

- Escape the input for spawn rather than just relying on preventing
shell expansion.
- During testing the audit command would get stuck in cycles present in
the npm audit report. This logic was updated to avoid such cycles.
- Catch errors thrown by npm when looking up available versions.

## What should be covered while testing?

Audit command works on windows, mac, and linux.

Author: weirdwater

packages/pluggable-widgets-tools/CHANGELOG.md

@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Fixed
 
 -   We fixed an issue on Windows where the generated `.mpk` was missing the widget's `.xml` files and icon/tile PNGs.
+-   We fixed an error thrown by the `audit` command on windows. It would fail when looking up available versions for vulnerable packages.
 
 ### Changed
 

packages/pluggable-widgets-tools/src/commands/audit.ts

@@ -20,7 +20,7 @@ export async function auditPluggableWidgetsTools(fix: boolean = false) {
     }
 
     // Collect updateable, vulnerable packages installed by pwt
-    const vulnerabilities = NpmAudit.collectVulnerabilities(report, report.vulnerabilities[pluggableWidgetsTools]);
+    const vulnerabilities = NpmAudit.collectVulnerabilities(report, pluggableWidgetsTools);
     const vulnerableDependencies = vulnerabilities
         .map(v => v.name)
         .reduce((unique, p) => (unique.includes(p) ? unique : [...unique, p]), [] as NpmAudit.PackageName[]);
@@ -49,7 +49,8 @@ export async function auditPluggableWidgetsTools(fix: boolean = false) {
         const update = p.safeRange
             ? green(`${symbols.pointerSmall} ${p.safeRange}`)
             : red(`${symbols.cross} No update available`);
-        console.log(`    ${whiteBright(bold(p.name))}   ${p.vulnerableRange}  ${update}`);
+        const status = p.error ? red(p.error.message) : update;
+        console.log(`    ${whiteBright(bold(p.name))}   ${p.vulnerableRange}  ${status}`);
     });
 
     // Add overrides for updateable dependencies
@@ -86,6 +87,7 @@ interface UpdateablePackage {
     name: NpmAudit.PackageName;
     vulnerableRange: string;
     safeRange?: string;
+    error?: Error;
 }
 
 /**
@@ -96,17 +98,23 @@ interface UpdateablePackage {
  * Using the ^ version range avoids this, as the version is specific enough for npm.
  */
 async function findSafeVersion({ name, range }: NpmAudit.Dependency): Promise<UpdateablePackage> {
-    const versions = await promisify(exec)(`npm show '${name}' versions --json`).then(
-        ({ stdout }) => JSON.parse(stdout) as string[]
-    );
+    const updateablePackage = { name, vulnerableRange: range };
+    const escapedName = encodeURI(name); // npm package names must be usable as part of a URL
+    const versions = await promisify(exec)(`npm show ${escapedName} versions --json`)
+        .then(({ stdout }) => JSON.parse(stdout) as string[])
+        .catch(_ => new Error("Unable to fetch available versions"));
+
+    if (versions instanceof Error) {
+        return { ...updateablePackage, error: versions };
+    }
 
     const maxVulnerable = maxSatisfying(versions, range);
     const gtMaxVulnerable = ">" + maxVulnerable;
     const minNonVulnerable = minSatisfying(versions, gtMaxVulnerable);
 
     if (!minNonVulnerable) {
-        return { name, vulnerableRange: range };
+        return updateablePackage;
     }
 
-    return { name, vulnerableRange: range, safeRange: "^" + minNonVulnerable };
+    return { ...updateablePackage, safeRange: "^" + minNonVulnerable };
 }

packages/pluggable-widgets-tools/src/common.ts

@@ -1,6 +1,13 @@
-export function ensure<T>(arg?: T): T {
-    if (arg == null) {
-        throw new Error("Did not expect an argument to be undefined");
+export function ensure<T>(arg?: T, label: string = "argument"): T {
+    if (arg === null || arg === undefined) {
+        throw new Error(`Did not expect ${label} to be ${arg}`);
     }
     return arg;
 }
+
+export function partition<T, A extends T, B extends Exclude<T, A>>(
+    input: Array<T>,
+    predicate: (x: T) => x is A
+): [A[], B[]] {
+    return [input.filter(predicate), input.filter((x): x is B => !predicate(x))] as const;
+}

packages/pluggable-widgets-tools/src/utils/npmAudit.ts

@@ -1,8 +1,9 @@
 import assert from "node:assert";
-import { exec } from "node:child_process";
 import { existsSync } from "node:fs";
 import { join } from "node:path";
 import { widgetRoot } from "../widget/paths";
+import { ensure, partition } from "../common";
+import { exec } from "node:child_process";
 
 export type Report = {
     auditReportVersion: 2;
@@ -59,15 +60,28 @@ export type Vulnerability = {
     range: string;
 };
 
-export function collectVulnerabilities(report: Report, dependency: Dependency): Vulnerability[] {
-    const vulnerabilities = dependency.via.filter(v => typeof v !== "string");
-    if (vulnerabilities.length > 0) {
-        return vulnerabilities;
+export function collectVulnerabilities(report: Report, rootDependency: PackageName): Vulnerability[] {
+    const dependencies: PackageName[] = [rootDependency];
+    const dependenciesSeen: PackageName[] = [];
+    const allVulnerabilities: Vulnerability[] = [];
+
+    while (dependencies.length > 0) {
+        const dependencyName = ensure(dependencies.shift());
+        dependenciesSeen.push(dependencyName);
+
+        const [transients, vulnerabilities] = partition(
+            report.vulnerabilities[dependencyName].via,
+            v => typeof v === "string"
+        );
+
+        if (vulnerabilities.length > 0) {
+            allVulnerabilities.push(...vulnerabilities);
+            continue;
+        }
+        dependencies.push(...transients.filter(d => !dependenciesSeen.includes(d)));
     }
 
-    return dependency.via
-        .filter(v => typeof v === "string")
-        .flatMap(v => collectVulnerabilities(report, report.vulnerabilities[v]));
+    return allVulnerabilities;
 }
 
 export async function run(): Promise<Report> {