fix: address 14+ verified bugs from CLI audit

cli-releases/releases.json

@@ -330,4 +330,4 @@
       "hash": "fd8ec74bce3f91ec2b4b967d3cd7e3678dc17a16433d71c5fe9757e07de051d0"
     }
   }
-}
+}

cli/CHANGELOG.md

@@ -1,6 +1,19 @@
 # Mops CLI Changelog
 
 ## Next
+- Fix `mops test` and `mops watch` breaking when dependency paths contain spaces
+- Fix `mops sync` incorrectly reporting version-pinned dependencies as missing/unused
+- Fix `mops update --lock ignore` not respecting the lock option during intermediate installs
+- Fix `mops update` crashing with unhandled error when GitHub API is unavailable
+- Fix `mops add` writing dependency to config even when GitHub download fails
+- Fix GitHub dependency install crashing the entire process instead of reporting the error
+- Fix version comparison treating short version strings (e.g. `1.0`) as equal to longer ones (e.g. `1.0.5`)
+- Fix `mops remove` not cleaning up transitive dependencies of GitHub packages
+- Fix corrupted `mops.lock` file causing an unhandled crash instead of a helpful error message
+- Fix `mops sources` resolving package config from wrong directory in some contexts
+- Harden lock file integrity check against package ID prefix collisions
+- `mops build` now reports invalid canister names instead of silently ignoring them
+- Document `baseDir`, `readme`, and `dfx` fields in `[package]` config
 
 ## 2.5.0
 - Add support for `MOPS_REGISTRY_HOST` and `MOPS_REGISTRY_CANISTER_ID` environment variables for custom registry endpoints

cli/commands/add.ts

@@ -101,9 +101,12 @@ export async function add(
   }
 
   if (pkgDetails.repo) {
-    await installFromGithub(pkgDetails.name, pkgDetails.repo, {
+    let res = await installFromGithub(pkgDetails.name, pkgDetails.repo, {
       verbose: verbose,
     });
+    if (!res) {
+      process.exit(1);
+    }
   } else if (!pkgDetails.path) {
     let res = await installMopsDep(pkgDetails.name, pkgDetails.version, {
       verbose: verbose,

cli/commands/build.ts

@@ -24,7 +24,7 @@ export async function build(
   canisterNames: string[] | undefined,
   options: Partial<BuildOptions>,
 ): Promise<void> {
-  if (canisterNames?.length == 0) {
+  if (canisterNames?.length === 0) {
     cliError("No canisters specified to build");
   }
 
@@ -41,16 +41,11 @@ export async function build(
   }
 
   if (canisterNames) {
-    canisterNames = canisterNames.filter((name) => name in canisters);
-    if (canisterNames.length === 0) {
-      throw new Error("No valid canister names specified");
-    }
-    for (let name of canisterNames) {
-      if (!(name in canisters)) {
-        cliError(
-          `Motoko canister '${name}' not found in mops.toml configuration`,
-        );
-      }
+    let invalidNames = canisterNames.filter((name) => !(name in canisters));
+    if (invalidNames.length) {
+      cliError(
+        `Motoko canister(s) not found in mops.toml configuration: ${invalidNames.join(", ")}`,
+      );
     }
   }
 
@@ -184,7 +179,7 @@ export async function build(
 
   console.log(
     chalk.green(
-      `\n✓ Built ${Object.keys(filteredCanisters).length} canister${Object.keys(filteredCanisters).length == 1 ? "" : "s"} successfully`,
+      `\n✓ Built ${Object.keys(filteredCanisters).length} canister${Object.keys(filteredCanisters).length === 1 ? "" : "s"} successfully`,
     ),
   );
 }

cli/commands/install/install-dep.ts

@@ -20,12 +20,11 @@ export async function installDep(
   parentPkgPath?: string,
 ): Promise<boolean> {
   if (dep.repo) {
-    await installFromGithub(dep.name, dep.repo, {
+    return installFromGithub(dep.name, dep.repo, {
       silent,
       verbose,
       ignoreTransitive,
     });
-    return true;
   } else if (dep.path) {
     let depPath = dep.path;
     parentPkgPath = parentPkgPath || getRootDir();
@@ -46,5 +45,8 @@ export async function installDep(
     });
   }
 
-  return true;
+  console.warn(
+    `Warning: dependency "${dep.name}" has no version, repo, or path`,
+  );
+  return false;
 }

cli/commands/publish.ts

@@ -398,29 +398,29 @@ export async function publish(
     console.log(chalk.red("Error: ") + publishing.err);
     process.exit(1);
   }
-  let puiblishingId = publishing.ok;
+  let publishingId = publishing.ok;
 
   // upload test stats
   if (options.test) {
-    await actor.uploadTestStats(puiblishingId, {
+    await actor.uploadTestStats(publishingId, {
       passed: BigInt(reporter.passed),
       passedNames: reporter.passedNamesFlat,
     });
   }
 
   // upload benchmarks
   if (options.bench) {
-    await actor.uploadBenchmarks(puiblishingId, benchmarks);
+    await actor.uploadBenchmarks(publishingId, benchmarks);
   }
 
   // upload changelog
   if (changelog) {
-    await actor.uploadNotes(puiblishingId, changelog);
+    await actor.uploadNotes(publishingId, changelog);
   }
 
   // upload docs coverage
   if (options.docs) {
-    await actor.uploadDocsCoverage(puiblishingId, docsCov);
+    await actor.uploadDocsCoverage(publishingId, docsCov);
   }
 
   // upload files
@@ -438,7 +438,7 @@ export async function publish(
     }
 
     let res = await actor.startFileUpload(
-      puiblishingId,
+      publishingId,
       file,
       BigInt(chunkCount),
       firstChunk,
@@ -453,7 +453,7 @@ export async function publish(
       let start = i * chunkSize;
       let chunk = Array.from(content.slice(start, start + chunkSize));
       let res = await actor.uploadFileChunk(
-        puiblishingId,
+        publishingId,
         fileId,
         BigInt(i),
         chunk,
@@ -474,7 +474,7 @@ export async function publish(
   progress();
   logUpdate.done();
 
-  let res = await actor.finishPublish(puiblishingId);
+  let res = await actor.finishPublish(publishingId);
   if ("err" in res) {
     console.log(chalk.red("Error: ") + res.err);
     process.exit(1);

cli/commands/remove.ts

@@ -65,7 +65,10 @@ export async function remove(
     let config = readConfig(configFile);
     let deps: Dependency[] = Object.values(config.dependencies || {})
       .map((dep) => {
-        return [dep, ...getTransitiveDependenciesOf(dep.name, dep.version)];
+        return [
+          dep,
+          ...getTransitiveDependenciesOf(dep.name, dep.version, dep.repo),
+        ];
       })
       .flat();
     return deps;
@@ -97,7 +100,7 @@ export async function remove(
   // transitive deps of this package (including itself)
   let transitiveDepsOfPackage = [
     pkgDetails,
-    ...getTransitiveDependenciesOf(name, version),
+    ...getTransitiveDependenciesOf(name, version, pkgDetails.repo),
   ];
 
   // remove local cache

cli/commands/sources.ts

@@ -38,8 +38,9 @@ export async function sourcesArgs({
 
       // append baseDir
       let pkgBaseDir;
-      if (fs.existsSync(path.join(pkgDir, "mops.toml"))) {
-        let config = readConfig(path.join(pkgDir, "mops.toml"));
+      let resolvedMopsToml = path.resolve(cwd, pkgDir, "mops.toml");
+      if (fs.existsSync(resolvedMopsToml)) {
+        let config = readConfig(resolvedMopsToml);
         pkgBaseDir = path.join(pkgDir, config.package?.baseDir || "src");
       } else {
         pkgBaseDir = path.join(pkgDir, "src");

cli/commands/sync.ts

@@ -8,6 +8,7 @@ import { remove } from "./remove.js";
 import { checkIntegrity } from "../integrity.js";
 import { getMocPath } from "../helpers/get-moc-path.js";
 import { MOTOKO_IGNORE_PATTERNS } from "../constants.js";
+import { getDepName } from "../helpers/get-dep-name.js";
 
 type SyncOptions = {
   lock?: "update" | "ignore";
@@ -80,26 +81,22 @@ async function getUsedPackages(): Promise<string[]> {
 
 async function getMissingPackages(): Promise<string[]> {
   let config = readConfig();
-  let allDeps = [
-    ...Object.keys(config.dependencies || {}),
-    ...Object.keys(config["dev-dependencies"] || {}),
-  ];
-  let missing = new Set(await getUsedPackages());
-  for (let pkg of allDeps) {
-    missing.delete(pkg);
-  }
-  return [...missing];
+  let allDepNames = new Set(
+    [
+      ...Object.keys(config.dependencies || {}),
+      ...Object.keys(config["dev-dependencies"] || {}),
+    ].map((key) => getDepName(key)),
+  );
+  let used = await getUsedPackages();
+  return used.filter((pkg) => !allDepNames.has(pkg));
 }
 
 async function getUnusedPackages(): Promise<string[]> {
   let config = readConfig();
-  let allDeps = new Set([
+  let allDeps = [
     ...Object.keys(config.dependencies || {}),
     ...Object.keys(config["dev-dependencies"] || {}),
-  ]);
-  let used = await getUsedPackages();
-  for (let pkg of used) {
-    allDeps.delete(pkg);
-  }
-  return [...allDeps];
+  ];
+  let used = new Set(await getUsedPackages());
+  return allDeps.filter((key) => !used.has(getDepName(key)));
 }

cli/commands/test/test.ts

@@ -12,7 +12,7 @@ import debounce from "debounce";
 import { SemVer } from "semver";
 import { ActorMethod } from "@icp-sdk/core/agent";
 
-import { sources } from "../sources.js";
+import { sourcesArgs } from "../sources.js";
 import { getGlobalMocArgs, getRootDir, readConfig } from "../../mops.js";
 import { parallel } from "../../parallel.js";
 
@@ -231,7 +231,7 @@ export async function testWithReporter(
   reporter.addFiles(files);
 
   let config = readConfig();
-  let sourcesArr = await sources();
+  let sourcesArr = (await sourcesArgs()).flat();
   let globalMocArgs = getGlobalMocArgs(config);
 
   if (!mocPath) {
@@ -298,7 +298,7 @@ export async function testWithReporter(
       let mocArgs = [
         "--hide-warnings",
         "--error-detail=2",
-        ...sourcesArr.join(" ").split(" "),
+        ...sourcesArr,
         ...globalMocArgs,
         file,
       ].filter((x) => x);

cli/commands/update.ts

@@ -42,12 +42,18 @@ export async function update(pkg?: string, { lock }: UpdateOptions = {}) {
   for (let dep of githubDeps) {
     let { org, gitName, branch, commitHash } = parseGithubURL(dep.repo || "");
     let dev = !!config["dev-dependencies"]?.[dep.name];
-    let commit = await getGithubCommit(`${org}/${gitName}`, branch);
-    if (commit.sha !== commitHash) {
-      await add(
-        `https://github.com/${org}/${gitName}#${branch}@${commit.sha}`,
-        { dev },
-        dep.name,
+    try {
+      let commit = await getGithubCommit(`${org}/${gitName}`, branch);
+      if (commit.sha !== commitHash) {
+        await add(
+          `https://github.com/${org}/${gitName}#${branch}@${commit.sha}`,
+          { dev, lock },
+          dep.name,
+        );
+      }
+    } catch (err: any) {
+      console.log(
+        chalk.red("Error: ") + `Failed to update ${dep.name}: ${err.message}`,
       );
     }
   }
@@ -87,7 +93,7 @@ export async function update(pkg?: string, { lock }: UpdateOptions = {}) {
           );
         }) || dep[0];
 
-      await add(`${dep[0]}@${dep[2]}`, { dev }, asName);
+      await add(`${dep[0]}@${dep[2]}`, { dev, lock }, asName);
     }
   }
 

cli/commands/watch/error-checker.ts

@@ -5,7 +5,7 @@ import chalk from "chalk";
 
 import { getMocPath } from "../../helpers/get-moc-path.js";
 import { getGlobalMocArgs, getRootDir, readConfig } from "../../mops.js";
-import { sources } from "../sources.js";
+import { sourcesArgs } from "../sources.js";
 import { parallel } from "../../parallel.js";
 import { globMoFiles } from "./globMoFiles.js";
 
@@ -43,7 +43,7 @@ export class ErrorChecker {
 
     let rootDir = getRootDir();
     let mocPath = getMocPath();
-    let deps = await sources({ cwd: rootDir });
+    let deps = (await sourcesArgs({ cwd: rootDir })).flat();
     let globalMocArgs = getGlobalMocArgs(readConfig());
 
     let paths = globMoFiles(rootDir);
@@ -55,12 +55,7 @@ export class ErrorChecker {
       try {
         await promisify(execFile)(
           mocPath,
-          [
-            "--check",
-            ...deps.flatMap((x) => x.split(" ")),
-            ...globalMocArgs,
-            file,
-          ],
+          ["--check", ...deps, ...globalMocArgs, file],
           { cwd: rootDir },
         );
       } catch (error: any) {

cli/commands/watch/warning-checker.ts

@@ -5,7 +5,7 @@ import chalk from "chalk";
 
 import { getMocPath } from "../../helpers/get-moc-path.js";
 import { getGlobalMocArgs, getRootDir, readConfig } from "../../mops.js";
-import { sources } from "../sources.js";
+import { sourcesArgs } from "../sources.js";
 import { ErrorChecker } from "./error-checker.js";
 import { parallel } from "../../parallel.js";
 import { globMoFiles } from "./globMoFiles.js";
@@ -69,7 +69,7 @@ export class WarningChecker {
 
     let rootDir = getRootDir();
     let mocPath = getMocPath();
-    let deps = await sources({ cwd: rootDir });
+    let deps = (await sourcesArgs({ cwd: rootDir })).flat();
     let globalMocArgs = getGlobalMocArgs(readConfig());
     let paths = globMoFiles(rootDir);
 
@@ -83,12 +83,7 @@ export class WarningChecker {
 
       let { stderr } = await promisify(execFile)(
         mocPath,
-        [
-          "--check",
-          ...deps.flatMap((x) => x.split(" ")),
-          ...globalMocArgs,
-          file,
-        ],
+        ["--check", ...deps, ...globalMocArgs, file],
         { cwd: rootDir, signal },
       ).catch((error) => {
         if (error.code === "ABORT_ERR") {

cli/helpers/find-changelog-entry.ts

@@ -16,7 +16,9 @@ export function findChangelogEntry(changelog: string, version: string): string {
       }
     } else if (
       node.type === "heading" &&
-      toMarkdown(node).match(new RegExp(`\\b${version}\\b`))
+      toMarkdown(node).match(
+        new RegExp(`\\b${version.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b`),
+      )
     ) {
       depth = node.depth;
       found = true;