diff --git a/.changeset/some-dryers-stop.md b/.changeset/some-dryers-stop.md new file mode 100644 index 00000000..0de26266 --- /dev/null +++ b/.changeset/some-dryers-stop.md @@ -0,0 +1,7 @@ +--- +"@nodesecure/scanner": minor +"@nodesecure/tarball": minor +"@nodesecure/mama": minor +--- + +feat: add npx and bin confusion warning diff --git a/README.md b/README.md index 47dc6f83..02ad2516 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,7 @@ Scanner builds on [JS-X-Ray](https://github.com/NodeSecure/js-x-ray) (SAST) and - Detects: - [Manifest confusion](https://blog.vlt.sh/blog/the-massive-hole-in-the-npm-ecosystem) - [Dependency confusion](https://www.landh.tech/blog/20250610-netflix-vulnerability-dependency-confusion/) + - [Npx and Bin confusion](https://www.landh.tech/blog/20260521-npx-used-confusion-and-its-super-effective/) - Typosquatting of popular package names - Install scripts (e.g. `install`, `preinstall`, `postinstall`, `preuninstall`, `postuninstall`) - Highlights packages by name, version(s), or maintainer diff --git a/workspaces/mama/docs/extractNpxFromScripts.md b/workspaces/mama/docs/extractNpxFromScripts.md new file mode 100644 index 00000000..a8ccfdf9 --- /dev/null +++ b/workspaces/mama/docs/extractNpxFromScripts.md @@ -0,0 +1,104 @@ +# extractNpxFromScripts + +Parse the binary name, flags and script name from a npx command `npx --flags binaryName` in each script if present. + +## Function Signature + +```ts +export type NpxCommand = { + binaryName: string; + flags: string[]; + scriptName: string; +}; + +export function* extractNpxFromScripts( + scripts: Record | undefined +): IterableIterator +``` + + +## Example Usage + +```ts + +extractNpxFromScripts({ +test: "npx --yes=false jest", +exec: "npx my-internal-tool", +start: "npm run start" +}); + +/* +Will yield: + +{ binaryName: "jest", +flags: ["--yes=false"], +scriptName: "test" +} + +Then: + +{ +binaryName: "my-internal-tool", +flags: [], scriptName: +"exec" +} + +Nothing is extracted from the start script since there is no npx command in it. +*/ + +extractNpxFromScripts({ +release: "npx -y -p @changesets/cli@3.0.1 -c 'changeset version'" +}); + +/* +Will yield: + +{ +binaryName: "@changesets/cli", +flags: ["-y", "-p"], +scriptName: "release" +} + +Note: that the binary name is given without the version +*/ + +extractNpxFromScripts({ +release: "npx a && npx b" +}); + +/* +Will yield: + +{ +binaryName: "a", +flags: [], +scriptName: "release" +} + +Then: + +{ +binaryName: "b", +flags: [], +scriptName: "release" +} + +*/ +``` + + +## How It Works + +The function uses a regular expression: + +```ts +/\bnpx\s+((?:--?\w[\w-]*(?:[=\s]\S+)?\s+)*)(\S+)/g +``` + +to extract for each script: + +* **binaryName** → the binary name (without the version when there is one) in the npx command (e.g. `"npx jest"` → `"jest"`) +* **flags** → the flags in the npx command (e.g. `"npx -y --no jest"` → `["-y", "--no"]`) +* **scriptName** → the name of the script where there is an npx command (e.g. `{"test":"npx -y --no jest"}` → `"test"`) + +If there is no script containing a npx command the function does not yield anything. diff --git a/workspaces/mama/src/ManifestManager.class.ts b/workspaces/mama/src/ManifestManager.class.ts index c2a42005..37b64b1c 100644 --- a/workspaces/mama/src/ManifestManager.class.ts +++ b/workspaces/mama/src/ManifestManager.class.ts @@ -17,7 +17,9 @@ import { fromData } from "ssri"; // Import Internal Dependencies import { packageJSONIntegrityHash, - inspectModuleType + inspectModuleType, + extractNpxFromScripts, + type NpxCommand } from "./utils/index.ts"; type WithRequired = T & { [P in K]-?: T[P] }; @@ -250,6 +252,10 @@ export class ManifestManager< } } + public* extractNpxFromScripts(): IterableIterator { + yield* extractNpxFromScripts(this.document.scripts); + } + static async fromPackageJSON( locationOrManifest: string | ManifestManager ): Promise { diff --git a/workspaces/mama/src/index.ts b/workspaces/mama/src/index.ts index 750ff967..90b02cd7 100644 --- a/workspaces/mama/src/index.ts +++ b/workspaces/mama/src/index.ts @@ -2,8 +2,10 @@ export * from "./ManifestManager.class.ts"; export { packageJSONIntegrityHash, parseNpmSpec, + extractNpxFromScripts, inspectModuleType, scanLockFiles, LOCK_FILES, - type PackageModuleType + type PackageModuleType, + type NpxCommand } from "./utils/index.ts"; diff --git a/workspaces/mama/src/utils/extractNpxFromScripts.ts b/workspaces/mama/src/utils/extractNpxFromScripts.ts new file mode 100644 index 00000000..7f37eef5 --- /dev/null +++ b/workspaces/mama/src/utils/extractNpxFromScripts.ts @@ -0,0 +1,38 @@ +// Import Internal Dependencies +import { parseNpmSpec } from "./parseNpmSpec.ts"; + +export type NpxCommand = { + binaryName: string; + flags: string[]; + scriptName: string; +}; + +export function* extractNpxFromScripts( + scripts: Record | undefined +): IterableIterator { + if (!scripts) { + return; + } + + for (const [scriptName, scriptValue] of Object.entries(scripts)) { + for (const npx of extractNpx(scriptValue, scriptName)) { + yield npx; + } + } +} + +function* extractNpx(command: string, scriptName: string): IterableIterator { + const npxPattern = /\bnpx\s+((?:--?\w[\w-]*(?:[=\s]\S+)?\s+)*)(\S+)/g; + let match: RegExpExecArray | null; + while ((match = npxPattern.exec(command)) !== null) { + const flags = match[1].split(" ").filter(Boolean); + + const npmSpec = parseNpmSpec(match[2]); + + yield { + binaryName: npmSpec?.name!, + flags, + scriptName + }; + } +} diff --git a/workspaces/mama/src/utils/index.ts b/workspaces/mama/src/utils/index.ts index 9b176a77..56f605ac 100644 --- a/workspaces/mama/src/utils/index.ts +++ b/workspaces/mama/src/utils/index.ts @@ -1,4 +1,5 @@ export * from "./integrity-hash.ts"; +export * from "./extractNpxFromScripts.ts"; export * from "./inspectModuleType.ts"; export * from "./parseNpmSpec.ts"; export * from "./scanLockFiles.ts"; diff --git a/workspaces/mama/test/ManifestManager.spec.ts b/workspaces/mama/test/ManifestManager.spec.ts index 8cce3ab6..a5232ab3 100644 --- a/workspaces/mama/test/ManifestManager.spec.ts +++ b/workspaces/mama/test/ManifestManager.spec.ts @@ -951,4 +951,49 @@ describe("ManifestManager", () => { ); }); }); + + describe("npx command extraction from scripts", () => { + it("should extract nothing when there is no scripts in the manifest", () => { + const packageJSON: PackageJSON = { + ...kMinimalPackageJSON, + dependencies: { + kleur: "1.0.0" + }, + devDependencies: { + mocha: "1.0.0" + }, + gypfile: false + }; + + const mama = new ManifestManager(packageJSON); + + assert.deepEqual(Array.from(mama.extractNpxFromScripts()), []); + }); + + it("should extract the npx command from the scritps in the manifest", () => { + const packageJSON: PackageJSON = { + scripts: { + test: "npx --no jest", + exec: "npx my-internal-tool", + start: "npm run start" + }, + ...kMinimalPackageJSON, + dependencies: { + kleur: "1.0.0" + }, + devDependencies: { + mocha: "1.0.0" + }, + gypfile: false + }; + + const mama = new ManifestManager(packageJSON); + + assert.deepEqual(Array.from(mama.extractNpxFromScripts()), + [ + { binaryName: "jest", flags: ["--no"], scriptName: "test" }, + { binaryName: "my-internal-tool", flags: [], scriptName: "exec" } + ]); + }); + }); }); diff --git a/workspaces/mama/test/extractNpxFromScripts.spec.ts b/workspaces/mama/test/extractNpxFromScripts.spec.ts new file mode 100644 index 00000000..f9184586 --- /dev/null +++ b/workspaces/mama/test/extractNpxFromScripts.spec.ts @@ -0,0 +1,125 @@ +// Import Node.js Dependencies +import assert from "node:assert"; +import { describe, it } from "node:test"; + +// Import Internal Dependencies +import { extractNpxFromScripts } from "../src/utils/index.ts"; + +describe("extractNpxFromScripts", () => { + describe("npx binary name extraction", () => { + it("should not extract anynthing when this is not an npx command", () => { + assert.deepEqual(Array.from(extractNpxFromScripts({ start: "npm run start" })), []); + }); + + it("should extract binary name for the simplest npx command", () => { + assert.deepEqual(Array.from(extractNpxFromScripts({ exec: "npx my-internal-tool" })), [ + { + binaryName: "my-internal-tool", + flags: [], + scriptName: "exec" + } + ]); + }); + + it("should extract the binary name when the npx command is not trimmed", () => { + assert.deepEqual(Array.from(extractNpxFromScripts({ exec: " npx my-internal-tool " })), [ + { + binaryName: "my-internal-tool", + flags: [], + scriptName: "exec" + } + ]); + }); + + it("should extract the binary name when the npx command is not the only command", () => { + assert.deepEqual(Array.from(extractNpxFromScripts({ test: "tsc && npx jest --coverage" })), [ + { + binaryName: "jest", + flags: [], + scriptName: "test" + } + ]); + }); + + it("should extract the flags", () => { + assert.deepEqual(Array.from(extractNpxFromScripts({ test: "npx --no jest" })), [ + { + binaryName: "jest", + flags: ["--no"], + scriptName: "test" + } + ]); + }); + + it("should be able to extract multiple flags", () => { + assert.deepEqual(Array.from(extractNpxFromScripts({ test: "npx --no --quiet jest" })), [ + { + binaryName: "jest", + flags: ["--no", "--quiet"], + scriptName: "test" + } + ]); + + assert.deepEqual(Array.from(extractNpxFromScripts({ test: "npx --no --quiet jest" })), [ + { + binaryName: "jest", + flags: ["--no", "--quiet"], + scriptName: "test" + } + ]); + }); + + it("should not match unrelated command", () => { + assert.deepEqual(Array.from(extractNpxFromScripts({ solve: "rnpx solve" })), []); + }); + + it("should extract remove the version from the binary name when there is one", () => { + assert.deepEqual( + Array.from(extractNpxFromScripts({ release: "npx -y -p @changesets/cli@3.0.1 -c 'changeset version'" })), + [ + { + binaryName: "@changesets/cli", + flags: ["-y", "-p"], + scriptName: "release" + } + ] + ); + }); + + it("should be able to match multiple npx commands in one script", () => { + assert.deepEqual( + Array.from(extractNpxFromScripts({ release: "npx a && npx b" })), + [ + { + binaryName: "a", + flags: [], + scriptName: "release" + }, + { + binaryName: "b", + flags: [], + scriptName: "release" + } + ] + ); + }); + }); + + describe("npx command extraction from scripts", () => { + it("should extract nothing when there is no scripts", () => { + assert.deepEqual(Array.from(extractNpxFromScripts(undefined)), []); + assert.deepEqual(Array.from(extractNpxFromScripts({})), []); + }); + + it("should extract the npx command from the scripts when there is one", () => { + assert.deepEqual(Array.from(extractNpxFromScripts({ + test: "npx --yes=false jest", + exec: "npx my-internal-tool", + start: "npm run start" + })), [ + { binaryName: "jest", flags: ["--yes=false"], scriptName: "test" }, + { binaryName: "my-internal-tool", flags: [], scriptName: "exec" } + ]); + }); + }); +}); diff --git a/workspaces/scanner/src/class/TarballScanner.class.ts b/workspaces/scanner/src/class/TarballScanner.class.ts index 7b2a2360..02ccadef 100644 --- a/workspaces/scanner/src/class/TarballScanner.class.ts +++ b/workspaces/scanner/src/class/TarballScanner.class.ts @@ -200,8 +200,8 @@ export class TarballScanner { ref: any, result: ScanResultPayload ): void { - const { description, engines, repository, scripts, author, integrity } = result; - Object.assign(ref, { description, engines, repository, scripts, author, integrity }); + const { description, engines, repository, scripts, author, integrity, bin } = result; + Object.assign(ref, { description, engines, repository, scripts, author, integrity, bin }); ref.warnings.push(...result.warnings); ref.licenses = result.licenses; diff --git a/workspaces/scanner/src/depWalker.ts b/workspaces/scanner/src/depWalker.ts index 809ed9a1..4df958e2 100644 --- a/workspaces/scanner/src/depWalker.ts +++ b/workspaces/scanner/src/depWalker.ts @@ -13,15 +13,18 @@ import * as Vulnera from "@nodesecure/vulnera"; import { npm } from "@nodesecure/tree-walker"; import { ManifestManager, - parseNpmSpec + parseNpmSpec, + extractNpxFromScripts } from "@nodesecure/mama"; import { getNpmRegistryURL } from "@nodesecure/npm-registry-sdk"; import type Config from "@npmcli/config"; +import * as i18n from "@nodesecure/i18n"; // Import Internal Dependencies import { addMissingVersionFlags, getDependenciesWarnings, + getNpxAndBinConfusionWarnings, getUsedDeps, getManifestLinks, NPM_TOKEN @@ -45,7 +48,9 @@ import type { GlobalWarning, DependencyConfusionWarning, Options, - Payload + Payload, + NpxConfusion, + BinConfusion } from "./types.ts"; import { HighlightedPackages } from "./extractors/probes/HighlightedPackagesExtractor.class.ts"; @@ -88,6 +93,8 @@ const kRootDependencyId = 0; const kCollectableTypes = ["url", "hostname", "ip", "email"]; +const kSafeNpxFlags = new Set(["--no", "--no-install", "--no-yes", "--yes=false"]); + const { version: packageVersion } = JSON.parse( readFileSync( new URL(path.join("..", "package.json"), import.meta.url), @@ -364,12 +371,53 @@ export async function depWalker( // Because we are dealing with package only one time it may happen sometimes. const globalWarnings: GlobalWarning[] = []; const highlightedPackagesExtractor = new HighlightedPackages(options.highlight?.packages ?? {}); + const npxConfusions = new Map(); + const binConfusions = new Map(); + + if (mama.document.bin) { + Object.keys(mama.document.bin).forEach((binName) => { + addToConfusions(binConfusions, binName, { + name: mama.name, + version: mama.version + }); + }); + } + + for (const npxConfusion of mama.extractNpxFromScripts()) { + if (!hasNpxSafeFlag(npxConfusion.flags)) { + addToConfusions(npxConfusions, npxConfusion.binaryName, { + name: mama.name, + version: mama.version, + scriptName: npxConfusion.scriptName + }); + } + } + for (const [packageName, dependency] of dependencies) { const metadataIntegrities = dependency.metadata?.integrity ?? {}; for (const [version, integrity] of Object.entries(metadataIntegrities)) { const dependencyVer = dependency.versions[version] as DependencyVersion; + if (dependencyVer.bin) { + Object.keys(dependencyVer.bin).forEach((binName) => { + addToConfusions(binConfusions, binName, { + name: packageName, + version + }); + }); + } + + for (const npxConfusion of extractNpxFromScripts(dependencyVer.scripts)) { + if (!hasNpxSafeFlag(npxConfusion.flags)) { + addToConfusions(npxConfusions, npxConfusion.binaryName, { + name: packageName, + version, + scriptName: npxConfusion.scriptName + }); + } + } + const isEmptyPackage = dependencyVer.warnings.some((warning) => warning.kind === "empty-package"); if (isEmptyPackage) { globalWarnings.push({ @@ -424,12 +472,30 @@ export async function depWalker( } try { - const { warnings, illuminated } = await getDependenciesWarnings( - dependencies, - options.highlight?.contacts, - isRemoteScanning - ); - payload.warnings = globalWarnings.concat(dependencyConfusionWarnings as GlobalWarning[]).concat(warnings); + const [{ warnings, illuminated }, + npxBinConfusionWarnings + ] = await Promise.all([ + getDependenciesWarnings(dependencies, + options.highlight?.contacts, + isRemoteScanning), + getNpxAndBinConfusionWarnings({ + npxConfusions, + binConfusions, + getToken: i18n.getToken, + token: tokenStore.get(getNpmRegistryURL()), + packument: (name, opts) => statsCollector.track({ + name: "npmRegistrySDK.packument", + phase: "npx-bin-warnings-collection", + fn: () => npmRegistrySDK.packument(name, opts) + }) + }) + ]); + + payload.warnings = globalWarnings + .concat(dependencyConfusionWarnings) + .concat(npxBinConfusionWarnings) + .concat(warnings); + const { highlightedPackages } = highlightedPackagesExtractor.done(); payload.highlighted = { contacts: illuminated, @@ -446,6 +512,21 @@ export async function depWalker( } } +type AnyConfusion = BinConfusion | NpxConfusion; + +function addToConfusions(confusions: Map, key: string, payload: AnyConfusion) { + if (confusions.has(key)) { + confusions.get(key)?.push(payload); + } + else { + confusions.set(key, [payload]); + } +} + +function hasNpxSafeFlag(flags: string[]) { + return flags.some((flag) => kSafeNpxFlags.has(flag)); +} + function extractHighlightedIdentifiers( collectables: DefaultCollectableSet[], identifiersToHighlight: Set diff --git a/workspaces/scanner/src/i18n/english.js b/workspaces/scanner/src/i18n/english.js index cbf7f439..85c3e302 100644 --- a/workspaces/scanner/src/i18n/english.js +++ b/workspaces/scanner/src/i18n/english.js @@ -7,7 +7,11 @@ const scanner = { typo_squatting: tS`Dependency '${0}' is similar to the following popular packages: ${1}`, dependency_confusion: "This dependency was found on both a public and private registry but its signature does not match", dependency_confusion_missing: "This dependency was found on the private but not on the public registry, this dependency is vulnerable to dependency confusion attacks.", - dependency_confusion_missing_org: tS`The org '${0}' is not claimed on the public registry` + dependency_confusion_missing_org: tS`The org '${0}' is not claimed on the public registry`, + npx_confusion_unclaimed: tS`npx '${0}' found in package.json script ${1} of package ${2} and unclaimed on the public registry, an attacker can register that name and achieve RCE on every developer and CI pipeline that runs this script`, + npx_confusion_claimed: tS`npx '${0}' found in package.json script ${1} of package ${2} and claimed on the public registry, verify that it is a trusted package, an attacker could have registered that name and achieve RCE on every developer and CI pipeline that runs this script`, + bin_confusion_unclaimed: tS`Binary '${0}' found in package.json bin of ${1} and unclaimed on the public registry, an attacker can register that name and achieve RCE on every developer and CI pipeline that runs this binary`, + bin_confusion_claimed: tS`Binary '${0}' found in package.json bin of ${1} and claimed on the public registry, verify that it is a trusted package, an attacker could have registered that name and achieve RCE on every developer and CI pipeline that runs this binary` }; export default { scanner }; diff --git a/workspaces/scanner/src/i18n/french.js b/workspaces/scanner/src/i18n/french.js index d692c1fa..ff866eb3 100644 --- a/workspaces/scanner/src/i18n/french.js +++ b/workspaces/scanner/src/i18n/french.js @@ -7,8 +7,11 @@ const scanner = { typo_squatting: tS`La dépendance '${0}' est similaire aux packages populaires suivants : ${1}`, dependency_confusion: "Cette dépendance a été trouvée à la fois sur un registre public et privé, mais sa signature ne correspond pas.", dependency_confusion_missing: "Cette dépendance a été trouvée seulement sur le registre privé, cette dépendance est vulnérable à une attaque par confusion de dépendance.", - dependency_confusion_missing_org: tS`L'organisation '${0}' n'est pas revendiquée sur le registre public` + dependency_confusion_missing_org: tS`L'organisation '${0}' n'est pas revendiquée sur le registre public`, + npx_confusion_unclaimed: tS`npx '${0}' a été trouvé dans le script package.json ${1} du package ${2} et n'est pas revendiqué sur le registre public, un attaquant peut enregistrer ce nom et obtenir une RCE sur chaque poste de développeur et chaque pipeline CI qui exécute ce script`, + npx_confusion_claimed: tS`npx '${0}' a été trouvé dans le script package.json ${1} du package ${2} et est revendiqué sur le registre public, vérifiez qu'il s'agit d'un package de confiance, un attaquant pourrait avoir enregistré ce nom et obtenir une RCE sur chaque poste de développeur et chaque pipeline CI qui exécute ce script`, + bin_confusion_unclaimed: tS`Le binaire '${0}' a été trouvé dans le champ bin du package.json de ${1} et n'est pas revendiqué sur le registre public, un attaquant peut enregistrer ce nom et obtenir une RCE sur chaque poste de développeur et chaque pipeline CI qui qui exécute ce binaire`, + bin_confusion_claimed: tS`Le binaire '${0}' a été trouvé dans le champ bin du package.json de ${1} et est revendiqué sur le registre public, vérifiez qu'il s'agit d'un package de confiance, un attaquant pourrait avoir enregistré ce nom et obtenir une RCE sur chaque poste de développeur et chaque pipeline CI qui exécute ce binaire` }; export default { scanner }; - diff --git a/workspaces/scanner/src/registry/NpmRegistryProvider.ts b/workspaces/scanner/src/registry/NpmRegistryProvider.ts index 48848283..8c66421c 100644 --- a/workspaces/scanner/src/registry/NpmRegistryProvider.ts +++ b/workspaces/scanner/src/registry/NpmRegistryProvider.ts @@ -8,7 +8,6 @@ import { packageJSONIntegrityHash } from "@nodesecure/mama"; import type { Packument, PackumentVersion, Signature } from "@nodesecure/npm-types"; import { getNpmRegistryURL } from "@nodesecure/npm-registry-sdk"; import * as i18n from "@nodesecure/i18n"; -import { isHTTPError } from "@openally/httpie"; // Import Internal Dependencies import { PackumentExtractor } from "./PackumentExtractor.ts"; @@ -20,7 +19,7 @@ import type { TokenStore } from "../types.ts"; import { Logger } from "../class/logger.class.ts"; -import { getLinks } from "../utils/getLinks.ts"; +import { hasStatusCode, getLinks } from "../utils/index.ts"; // CONSTANTS const kNotFoundStatusCode = 404; @@ -184,7 +183,7 @@ export class NpmRegistryProvider { } catch (err) { const isScoped = Boolean(org); - if (isHTTPError(err) && err.statusCode === kNotFoundStatusCode && !isScoped) { + if (hasStatusCode(err) && err.statusCode === kNotFoundStatusCode && !isScoped) { this.#addDependencyConfusionWarning(warnings, await i18n.getToken("scanner.dependency_confusion_missing")); } } @@ -209,10 +208,10 @@ export class NpmRegistryProvider { async enrichScopedDependencyConfusionWarnings(warnings: DependencyConfusionWarning[], org: string) { try { - await this.#npmApiClient.org(this.name); + await this.#npmApiClient.org(org); } catch (err) { - if (isHTTPError(err) && err.statusCode === kNotFoundStatusCode) { + if (hasStatusCode(err) && err.statusCode === kNotFoundStatusCode) { await this.#addDependencyConfusionWarning(warnings, await i18n.getToken("scanner.dependency_confusion_missing_org", org)); } } diff --git a/workspaces/scanner/src/types.ts b/workspaces/scanner/src/types.ts index ca979b15..3009c052 100644 --- a/workspaces/scanner/src/types.ts +++ b/workspaces/scanner/src/types.ts @@ -73,6 +73,8 @@ export interface DependencyVersion { engines: Engines; repository?: Repository | string; scripts: Record; + /** Binaries exposed by the package, as declared in its package.json */ + bin?: Record; /** * JS-X-Ray warnings * @@ -170,6 +172,27 @@ export type DependencyConfusionWarning = { }; }; +export type NpxConfusionWarning = { + type: "npx-confusion"; + message: string; + metadata: { + name: string; + version: string; + npxBinaryName: string; + scriptName: string; + }; +}; + +export type BinConfusionWarning = { + type: "bin-confusion"; + message: string; + metadata: { + name: string; + version: string; + binaryName: string; + }; +}; + export type GlobalWarning = { message: string; } & ( { type: @@ -186,7 +209,9 @@ export type GlobalWarning = { message: string; } & ( }; } | - DependencyConfusionWarning); + DependencyConfusionWarning + | BinConfusionWarning | NpxConfusionWarning + ); export type ApiStats = { /** @@ -386,3 +411,14 @@ export interface TokenStore { */ get(registry: string): string | undefined; } + +type Confusion = { + name: string; + version: string; +}; + +export type NpxConfusion = Confusion & { + scriptName: string; +}; + +export type BinConfusion = Confusion; diff --git a/workspaces/scanner/src/utils/index.ts b/workspaces/scanner/src/utils/index.ts index 053cec90..4e5fa201 100644 --- a/workspaces/scanner/src/utils/index.ts +++ b/workspaces/scanner/src/utils/index.ts @@ -6,6 +6,20 @@ export * from "./getUsedDeps.ts"; export * from "./isNodesecurePayload.ts"; export * from "./npmrc.ts"; +export interface WithStatusCode { + statusCode: number; +} + +// TODO: replace with isHTTPError or isHttpieError when those ones will be fixed in @openally/httpie +export function hasStatusCode(err: unknown): err is WithStatusCode { + return ( + typeof err === "object" && + err !== null && + "statusCode" in err && + typeof (err as { statusCode?: unknown; }).statusCode === "number" + ); +} + export const NPM_TOKEN = typeof process.env.NODE_SECURE_TOKEN === "string" ? { token: process.env.NODE_SECURE_TOKEN } : {}; diff --git a/workspaces/scanner/src/utils/warnings.ts b/workspaces/scanner/src/utils/warnings.ts index ebadd3cf..640d0121 100644 --- a/workspaces/scanner/src/utils/warnings.ts +++ b/workspaces/scanner/src/utils/warnings.ts @@ -9,11 +9,16 @@ import { type IlluminatedContact, type ContactExtractorPackageMetadata } from "@nodesecure/contact"; -import type { Contact } from "@nodesecure/npm-types"; +import type { Contact, Packument } from "@nodesecure/npm-types"; +import { getNpmRegistryURL } from "@nodesecure/npm-registry-sdk"; // Import Internal Dependencies import { TopPackages } from "../class/TopPackages.class.ts"; -import type { Dependency, GlobalWarning } from "../types.ts"; +import type { + BinConfusion, BinConfusionWarning, Dependency, + GlobalWarning, NpxConfusion, NpxConfusionWarning +} from "../types.ts"; +import { hasStatusCode } from "./index.ts"; await i18n.extendFromSystemPath( path.join(import.meta.dirname, "..", "i18n") @@ -127,3 +132,127 @@ async function searchTypoSquattingByName( return null; } + +type GetNpxAndBinConfusionWarningParams = { + packument: (name: string, options?: { + registry: string; + token?: string; + }) => Promise; + getToken: (token: string, ...params: any[]) => Promise; + token: string | undefined; + npxConfusions: Map; + binConfusions: Map; +}; + +const kNotFoundStatusCode = 404; + +export async function getNpxAndBinConfusionWarnings({ + packument, + getToken, + token, + npxConfusions, + binConfusions }: GetNpxAndBinConfusionWarningParams) { + const operationQueue: Promise[] = []; + + const warnings: (BinConfusionWarning | NpxConfusionWarning)[] = []; + + const seenPromises = new Map>(); + + for (const [npxBinaryName, packages] of npxConfusions.entries()) { + for (const { version, name, scriptName } of packages) { + const spec = `${name}@${version}`; + + let packumentPromise: Promise; + + if (seenPromises.has(npxBinaryName)) { + packumentPromise = seenPromises.get(npxBinaryName)!; + } + else { + packumentPromise = packument(npxBinaryName, { + registry: getNpmRegistryURL(), + token + }); + seenPromises.set(npxBinaryName, packumentPromise); + } + + operationQueue.push( + packumentPromise.then(async() => { + warnings.push({ + type: "npx-confusion", + message: await getToken("scanner.npx_confusion_claimed", npxBinaryName, scriptName, spec), + metadata: { + version, + name, + npxBinaryName, + scriptName + } + }); + }).catch(async(err) => { + if (hasStatusCode(err) && err.statusCode === kNotFoundStatusCode) { + warnings.push({ + type: "npx-confusion", + message: await getToken("scanner.npx_confusion_unclaimed", npxBinaryName, scriptName, spec), + metadata: { + version, + name, + npxBinaryName, + scriptName + } + }); + } + }) + + ); + } + } + + for (const [binName, packages] of binConfusions.entries()) { + for (const { version, name } of packages) { + const spec = `${name}@${version}`; + + let packumentPromise: Promise; + + if (seenPromises.has(binName)) { + packumentPromise = seenPromises.get(binName)!; + } + else { + packumentPromise = packument(binName, { + registry: getNpmRegistryURL(), + token + }); + seenPromises.set(binName, packumentPromise); + } + + operationQueue.push( + packumentPromise.then(async() => { + warnings.push({ + type: "bin-confusion", + message: await getToken("scanner.bin_confusion_claimed", binName, spec), + metadata: { + version, + name, + binaryName: binName + } + }); + }).catch(async(err) => { + if (hasStatusCode(err) && err.statusCode === kNotFoundStatusCode) { + warnings.push({ + type: "bin-confusion", + message: await getToken("scanner.bin_confusion_unclaimed", binName, spec), + metadata: { + version, + name, + binaryName: binName + } + }); + } + }) + + ); + } + } + + await Promise.allSettled(operationQueue); + + return warnings; +} diff --git a/workspaces/scanner/test/NpmRegistryProvider.spec.ts b/workspaces/scanner/test/NpmRegistryProvider.spec.ts index 61f5c7fa..a0b55134 100644 --- a/workspaces/scanner/test/NpmRegistryProvider.spec.ts +++ b/workspaces/scanner/test/NpmRegistryProvider.spec.ts @@ -870,7 +870,7 @@ describe("NpmRegistryProvider", { concurrency: 2 }, () => { assert.deepEqual(warnings, []); assert.strictEqual(mockOrg.mock.callCount(), 1); - assert.deepEqual(mockOrg.mock.calls[0].arguments, ["@foo/utils"]); + assert.deepEqual(mockOrg.mock.calls[0].arguments, ["foo"]); }); }); }); diff --git a/workspaces/scanner/test/depWalker.spec.ts b/workspaces/scanner/test/depWalker.spec.ts index 9701f70c..8ffa4bbb 100644 --- a/workspaces/scanner/test/depWalker.spec.ts +++ b/workspaces/scanner/test/depWalker.spec.ts @@ -54,6 +54,11 @@ const pkgHighlightedPackages = JSON.parse(readFileSync( "utf8" )); +const pkgNpxBinConfusion = JSON.parse(readFileSync( + path.join(kFixturePath, "npx-bin-confusion.json"), + "utf8" +)); + function cleanupPayload(payload: Payload) { for (const pkg of Object.values(payload)) { const versions = Object.values( @@ -181,10 +186,10 @@ describe("depWalker", { concurrency: 2 }, () => { assert.strictEqual(typeof metadata.startedAt, "number"); assert.strictEqual(typeof metadata.executionTime, "number"); assert.strictEqual(Array.isArray(metadata.apiCalls), true); - assert.strictEqual(metadata.apiCallsCount, 42); + assert.strictEqual(metadata.apiCallsCount, 57); assert.strictEqual(metadata.errorCount, 2); assert.strictEqual(metadata.errors.length, 2); - assert.strictEqual(statsCount(), 40); + assert.strictEqual(statsCount(), 55); assert.deepEqual(metadata.apiCalls.flatMap(({ name, tarball }) => (name.startsWith("tarball.scanDirOrArchive") ? [tarball] : [])).sort(byFilesCount), [{ path: "All", filesCount: 37 }, @@ -279,6 +284,27 @@ describe("depWalker", { concurrency: 2 }, () => { }); }); + describe("npx & bin confusion", () => { + it("should emit a global warning for both the unclaimed npx binary and the unclaimed bin name", { skip }, async(t) => { + Vulnera.setStrategy(Vulnera.strategies.GITHUB_ADVISORY); + const { logger } = buildLogger(); + t.after(() => logger.removeAllListeners()); + + const result = await depWalker( + new ManifestManager(pkgNpxBinConfusion), + { + ...structuredClone(kDefaultWalkerOptions), + isVerbose: true + }, + logger + ); + + assert.strictEqual(result.warnings.length, 2); + assert.ok(result.warnings.some(({ type }) => type === "npx-confusion")); + assert.ok(result.warnings.some(({ type }) => type === "bin-confusion")); + }); + }); + describe("highlight", () => { it("should highlight packages matching a semver range map", { skip }, async(t) => { const { logger } = buildLogger(); diff --git a/workspaces/scanner/test/fixtures/depWalker/npx-bin-confusion.json b/workspaces/scanner/test/fixtures/depWalker/npx-bin-confusion.json new file mode 100644 index 00000000..7ca413a4 --- /dev/null +++ b/workspaces/scanner/test/fixtures/depWalker/npx-bin-confusion.json @@ -0,0 +1,15 @@ +{ + "name": "npx-bin-confusion", + "version": "1.0.0", + "description": "Fixture using an unclaimed npx binary in its scripts and an unclaimed bin name", + "main": "index.js", + "scripts": { + "build": "npx --yes dfkljqlfjm", + "test": "npx --no dfkljqlfjm" + }, + "bin": { + "zqmlkfjdsqf": "./bin/index.js" + }, + "devDependencies": {}, + "dependencies": {} +} diff --git a/workspaces/scanner/test/utils/warnings.spec.ts b/workspaces/scanner/test/utils/warnings.spec.ts index 7a1ffe81..8b24ae69 100644 --- a/workspaces/scanner/test/utils/warnings.spec.ts +++ b/workspaces/scanner/test/utils/warnings.spec.ts @@ -4,12 +4,22 @@ import assert from "node:assert/strict"; // Import Third-party Dependencies import * as i18n from "@nodesecure/i18n"; +import type { Packument } from "@nodesecure/npm-types"; +import { HttpieOnHttpError } from "@openally/httpie"; +import { getNpmRegistryURL } from "@nodesecure/npm-registry-sdk"; // Import Internal Dependencies import { - getDependenciesWarnings + getDependenciesWarnings, + getNpxAndBinConfusionWarnings } from "../../src/utils/index.ts"; -import type { Dependency } from "../../src/types.ts"; +import type { + Dependency, + NpxConfusion, + BinConfusion, + NpxConfusionWarning, + BinConfusionWarning +} from "../../src/types.ts"; function createDependency( maintainers = [], @@ -27,6 +37,17 @@ function createDependency( } as unknown as Dependency; } +// warnings are pushed from promise callbacks, so a claimed binary (resolved +// packument) always lands before an unclaimed one (rejected packument). +// that ordering is an artifact of promise scheduling, not of the input. +function sortByMessage( + warnings: (NpxConfusionWarning | BinConfusionWarning)[] +) { + return [...warnings].sort( + (left, right) => left.message.localeCompare(right.message) + ); +} + describe("utils.getDependenciesWarnings", () => { it("should warn for library '@scarf/scarf'", async() => { const deps = new Map([ @@ -62,3 +83,169 @@ describe("utils.getDependenciesWarnings", () => { ); }); }); + +describe("getNpxAndBinConfusionWarnings", () => { + it("should get bin and npx confusion warnings", async(t) => { + const npxConfusions = new Map([ + ["bar", [{ + name: "jest", + version: "14.0.5", + scriptName: "dev" + }]], + ["foo", [ + { + name: "react", + version: "19.0.0", + scriptName: "start" + }, + { + name: "axios", + version: "5.1.2", + scriptName: "test" + } + ]], + ["not-404", [{ + name: "lodash", + version: "1.0.0", + scriptName: "build" + }]] + ]); + + const binConfusions = new Map([ + ["foo", [{ + name: "jest", + version: "14.0.5" + }]], + ["something", [ + { + name: "react", + version: "19.0.0" + }, + { + name: "axios", + version: "5.1.2" + } + ]], + ["not-404", [{ + name: "lodash", + version: "1.0.0" + }]] + ]); + + const packumentMock = t.mock.fn<(name: string, options?: { + registry: string; + token?: string; + }) => Promise>(); + + const getTokenMock = t.mock.fn< + (token: string, ...params: any[]) => Promise + >(); + + getTokenMock.mock.mockImplementation((token, ...params) => Promise.resolve(`${token} ${params + .map((param) => String(param)).join(" ")}`)); + + packumentMock.mock.mockImplementation((name) => { + if (["something", "bar"].includes(name)) { + return Promise.resolve({} as unknown as Packument); + } + + if (name === "not-404") { + return Promise.reject(new Error()); + } + + return Promise.reject(new HttpieOnHttpError({ + data: null, + headers: {}, + statusMessage: "Not found", + statusCode: 404 + })); + }); + + const warnings = await getNpxAndBinConfusionWarnings({ + packument: packumentMock, + getToken: getTokenMock, + npxConfusions, + binConfusions, + token: "token" + }); + + assert.strictEqual(packumentMock.mock.callCount(), 4); + + const registryOptions = { + registry: getNpmRegistryURL(), + token: "token" + }; + assert.deepEqual( + packumentMock.mock.calls.map((call) => call.arguments), + [ + ["bar", registryOptions], + ["foo", registryOptions], + ["not-404", registryOptions], + ["something", registryOptions] + ] + ); + + // the two 'not-404' binaries fail with a non-404 error, so they are skipped + assert.strictEqual(getTokenMock.mock.callCount(), 6); + + assert.deepEqual(sortByMessage(warnings), sortByMessage([ + { + type: "npx-confusion", + message: "scanner.npx_confusion_claimed bar dev jest@14.0.5", + metadata: { + name: "jest", + version: "14.0.5", + npxBinaryName: "bar", + scriptName: "dev" + } + }, + { + type: "npx-confusion", + message: "scanner.npx_confusion_unclaimed foo start react@19.0.0", + metadata: { + name: "react", + version: "19.0.0", + npxBinaryName: "foo", + scriptName: "start" + } + }, + { + type: "npx-confusion", + message: "scanner.npx_confusion_unclaimed foo test axios@5.1.2", + metadata: { + name: "axios", + version: "5.1.2", + npxBinaryName: "foo", + scriptName: "test" + } + }, + { + type: "bin-confusion", + message: "scanner.bin_confusion_unclaimed foo jest@14.0.5", + metadata: { + name: "jest", + version: "14.0.5", + binaryName: "foo" + } + }, + { + type: "bin-confusion", + message: "scanner.bin_confusion_claimed something react@19.0.0", + metadata: { + name: "react", + version: "19.0.0", + binaryName: "something" + } + }, + { + type: "bin-confusion", + message: "scanner.bin_confusion_claimed something axios@5.1.2", + metadata: { + name: "axios", + version: "5.1.2", + binaryName: "something" + } + } + ])); + }); +}); diff --git a/workspaces/tarball/src/tarball.ts b/workspaces/tarball/src/tarball.ts index b837edbf..f929ec90 100644 --- a/workspaces/tarball/src/tarball.ts +++ b/workspaces/tarball/src/tarball.ts @@ -67,13 +67,14 @@ export async function scanPackageCore( warnings.push(...code.warnings); const { files, dependencies, flags } = dependencySet.extract(); - const { description, engines, repository, scripts } = mama.document; + const { description, engines, repository, scripts, bin } = mama.document; return { description, engines, repository, scripts, + bin, author: mama.author, integrity: mama.isWorkspace ? null : mama.integrity, type: mama.moduleType, @@ -115,8 +116,8 @@ export async function scanDirOrArchive( ): Promise { const result = await scanPackageCore(locationOrManifest, options.astAnalyserOptions); - const { description, engines, repository, scripts, author, integrity } = result; - Object.assign(ref, { description, engines, repository, scripts, author, integrity }); + const { description, engines, repository, scripts, author, integrity, bin } = result; + Object.assign(ref, { description, engines, repository, scripts, author, integrity, bin }); ref.warnings.push(...result.warnings); ref.licenses = result.licenses; diff --git a/workspaces/tarball/src/types.ts b/workspaces/tarball/src/types.ts index ee5a2a09..644ea3d1 100644 --- a/workspaces/tarball/src/types.ts +++ b/workspaces/tarball/src/types.ts @@ -29,6 +29,7 @@ export interface ScanResultPayload { engines?: Record; repository?: any; scripts?: Record; + bin?: Record; author?: any; integrity?: string | null; type: string; @@ -59,6 +60,7 @@ export interface DependencyRef { engines: Record; repository: any; scripts: Record; + bin?: Record; warnings: any; licenses: conformance.SpdxFileLicenseConformance[]; uniqueLicenseIds: string[];