From 8e04e3ddd3d09676b44ea3ba0a2a8f16ee78b3d4 Mon Sep 17 00:00:00 2001 From: GENTILHOMME Thomas Date: Sat, 28 Mar 2026 19:05:14 +0100 Subject: [PATCH] refactor: enhance error resilience & add missing NPM userAgent --- .changeset/orange-points-reply.md | 7 + workspaces/scanner/src/depWalker.ts | 54 ++++--- workspaces/scanner/src/index.ts | 3 +- workspaces/tarball/src/tarball.ts | 3 +- workspaces/tree-walker/src/npm/walker.ts | 25 ++- .../tree-walker/test/npm/TreeWalker.spec.ts | 153 ++++++++++++++++++ 6 files changed, 212 insertions(+), 33 deletions(-) create mode 100644 .changeset/orange-points-reply.md diff --git a/.changeset/orange-points-reply.md b/.changeset/orange-points-reply.md new file mode 100644 index 00000000..c6b11836 --- /dev/null +++ b/.changeset/orange-points-reply.md @@ -0,0 +1,7 @@ +--- +"@nodesecure/tree-walker": major +"@nodesecure/scanner": patch +"@nodesecure/tarball": patch +--- + +Enhance error resilience and add missing pacote (npm) userAgent for all HTTP requests diff --git a/workspaces/scanner/src/depWalker.ts b/workspaces/scanner/src/depWalker.ts index 3a440b52..c2790b46 100644 --- a/workspaces/scanner/src/depWalker.ts +++ b/workspaces/scanner/src/depWalker.ts @@ -134,7 +134,11 @@ export async function depWalker( const tokenStore = new RegistryTokenStore(npmRcConfig, NPM_TOKEN.token, npmRcEntries); const npmProjectConfig = tokenStore.getConfig(registry); - const pacoteScopedConfig = { ...npmProjectConfig, ...npmRcEntries }; + const pacoteScopedConfig = { + ...npmProjectConfig, + ...npmRcEntries, + userAgent: `@nodesecure/scanner node/${process.version}` + }; const pacoteProvider: PacoteProvider = { async extract(spec, dest, opts): Promise { @@ -201,29 +205,30 @@ export async function depWalker( () => npmRegistrySDK.org(namespace) ) }; - { - logger - .start(ScannerLoggerEvents.analysis.tree) - .start(ScannerLoggerEvents.analysis.tarball) - .start(ScannerLoggerEvents.analysis.registry); - const fetchedMetadataPackages = new Set(); - const operationsQueue: Promise[] = []; - - await using tarballScanner = new TarballScanner({ - tempDir, - statsCollector, - pacoteProvider, - collectables, - maxConcurrency, - logger, - workers - }); - const rootDepsOptions: npm.WalkOptions = { - maxDepth, - includeDevDeps, - packageLock - }; + logger + .start(ScannerLoggerEvents.analysis.tree) + .start(ScannerLoggerEvents.analysis.tarball) + .start(ScannerLoggerEvents.analysis.registry); + const fetchedMetadataPackages = new Set(); + const operationsQueue: Promise[] = []; + + await using tarballScanner = new TarballScanner({ + tempDir, + statsCollector, + pacoteProvider, + collectables, + maxConcurrency, + logger, + workers + }); + + const rootDepsOptions: npm.WalkOptions = { + maxDepth, + includeDevDeps, + packageLock + }; + try { for await (const current of npmTreeWalker.walk(manifest, rootDepsOptions)) { const { name, version, integrity, ...currentVersion } = current; const dependency: Dependency = { @@ -316,7 +321,8 @@ export async function depWalker( }) ); } - + } + finally { logger.end(ScannerLoggerEvents.analysis.tree); await Promise.allSettled(operationsQueue); diff --git a/workspaces/scanner/src/index.ts b/workspaces/scanner/src/index.ts index 65af41ff..b3176944 100644 --- a/workspaces/scanner/src/index.ts +++ b/workspaces/scanner/src/index.ts @@ -86,7 +86,8 @@ export async function from( logger.start(ScannerLoggerEvents.manifest.fetch); const manifest = await pacote.manifest(packageName, { - ...NPM_TOKEN, registry, cache: `${os.homedir()}/.npm` + ...NPM_TOKEN, registry, cache: `${os.homedir()}/.npm`, + userAgent: `@nodesecure/scanner node/${process.version}` }); logger.end(ScannerLoggerEvents.manifest.fetch); diff --git a/workspaces/tarball/src/tarball.ts b/workspaces/tarball/src/tarball.ts index 2038a569..a205d5f3 100644 --- a/workspaces/tarball/src/tarball.ts +++ b/workspaces/tarball/src/tarball.ts @@ -226,7 +226,8 @@ export async function extractAndResolve( { ...NPM_TOKEN, registry, - cache: `${os.homedir()}/.npm` + cache: `${os.homedir()}/.npm`, + userAgent: `@nodesecure/tarball node/${process.version}` } ); diff --git a/workspaces/tree-walker/src/npm/walker.ts b/workspaces/tree-walker/src/npm/walker.ts index 6119b675..863eac02 100644 --- a/workspaces/tree-walker/src/npm/walker.ts +++ b/workspaces/tree-walker/src/npm/walker.ts @@ -127,7 +127,8 @@ export class TreeWalker { return { ...utils.NPM_TOKEN, registry: this.registry, - cache: `${os.homedir()}/.npm` + cache: `${os.homedir()}/.npm`, + userAgent: `@nodesecure/tree-walker node/${process.version}` }; } @@ -177,10 +178,20 @@ export class TreeWalker { ): AsyncIterableIterator { const { currDepth = 1, parent, maxDepth, gitURL } = options; - const { name, version, deprecated, ...pkg } = await this.providers.pacote.manifest( - gitURL ?? spec, - this.registryOptions - ); + let manifest: pacote.AbbreviatedManifest & pacote.ManifestResult; + try { + manifest = await this.providers.pacote.manifest( + gitURL ?? spec, + this.registryOptions + ); + } + catch { + // Unable to fetch manifest for this dependency (e.g. registry unreachable, 403, etc.) + // Skip this branch of the tree entirely rather than crashing the whole walk. + return; + } + + const { name, version, deprecated, ...pkg } = manifest; const { dependencies, customResolvers, alias } = utils.mergeDependencies(pkg); const current = new Dependency(name, version, { @@ -367,7 +378,7 @@ export class TreeWalker { ...packageLockOptions })); - for await (const dep of combineAsyncIterators({}, ...iterators)) { + for await (const dep of combineAsyncIterators({ throwError: false }, ...iterators)) { yield dep.exportAsPlainObject(); } @@ -392,7 +403,7 @@ export class TreeWalker { .map(dependencies.entries(), ([name, ver]) => this.walkRemoteDependency(`${name}@${ver}`, walkRemoteOptions)) ]; - for await (const dep of combineAsyncIterators({}, ...iterators)) { + for await (const dep of combineAsyncIterators({ throwError: false }, ...iterators)) { yield dep.exportAsPlainObject(); } diff --git a/workspaces/tree-walker/test/npm/TreeWalker.spec.ts b/workspaces/tree-walker/test/npm/TreeWalker.spec.ts index 73a9e160..7c6d6bda 100644 --- a/workspaces/tree-walker/test/npm/TreeWalker.spec.ts +++ b/workspaces/tree-walker/test/npm/TreeWalker.spec.ts @@ -239,6 +239,159 @@ describe("npm.TreeWalker", () => { ); }); }); + + describe("error resilience", () => { + test(`Given a manifest with transitive dependencies and a mocked pacote that fails on transitive fetches + it should gracefully skip the failing dependencies and still yield the root`, async(t) => { + const rootManifest = { + name: "test-pkg", + version: "1.0.0", + dependencies: { + "dep-a": "^1.0.0", + "dep-b": "^2.0.0" + } + }; + + let callCount = 0; + const mockedPacoteProvider = { + manifest: t.mock.fn(async(_: string) => { + callCount++; + // First call is for root integrity check, let it fail (existOnRemoteRegistry = false) + if (callCount === 1) { + throw new Error("root not found"); + } + // Transitive dependency fetches all fail (simulating registry 403) + throw new Error("403 Forbidden"); + }), + packument: t.mock.fn(async() => { + throw new Error("403 Forbidden"); + }) + }; + + const walker = new npm.TreeWalker({ + providers: { + pacote: mockedPacoteProvider + } + }); + + const dependencies: DependencyJSON[] = []; + for await (const dependency of walker.walk(rootManifest as any, { maxDepth: 10 })) { + dependencies.push(dependency); + } + + // Should still yield the root dependency despite all transitive fetches failing + assert.strictEqual(dependencies.length, 1); + assert.strictEqual(dependencies[0].name, "test-pkg"); + assert.strictEqual(dependencies[0].id, 0); + assert.strictEqual(dependencies[0].existOnRemoteRegistry, false); + }); + + test(`Given a manifest where some transitive dependencies fail and others succeed + it should yield the successful ones and skip the failures without leaking promises`, async(t) => { + const rootManifest = { + name: "test-pkg", + version: "1.0.0", + dependencies: { + "good-dep": "^1.0.0", + "bad-dep": "^1.0.0" + } + }; + + let callCount = 0; + const mockedPacoteProvider = { + manifest: t.mock.fn(async(spec: string) => { + callCount++; + // First call: root integrity check -> fail + if (callCount === 1) { + throw new Error("root not found"); + } + // good-dep resolves successfully + if (typeof spec === "string" && spec.includes("good-dep")) { + return { + name: "good-dep", + version: "1.0.0", + dependencies: {}, + _integrity: "sha512-good" + }; + } + + // bad-dep always fails + throw new Error("503 Service Unavailable"); + }), + packument: t.mock.fn(async(name: string) => { + if (name === "good-dep") { + return { + "dist-tags": { latest: "1.0.0" }, + versions: { + "1.0.0": { version: "1.0.0" } + } + }; + } + + throw new Error("503 Service Unavailable"); + }) + }; + + const walker = new npm.TreeWalker({ + providers: { + // @ts-expect-error + pacote: mockedPacoteProvider + } + }); + + const dependencies: DependencyJSON[] = []; + for await (const dependency of walker.walk(rootManifest, { maxDepth: 10 })) { + dependencies.push(dependency); + } + + const names = dependencies + .map((dependency) => dependency.name) + .sort(); + // Should yield good-dep and root, but not bad-dep + assert.ok(names.includes("test-pkg"), "root dependency should be present"); + assert.ok(names.includes("good-dep"), "successful dependency should be present"); + assert.ok(!names.includes("bad-dep"), "failed dependency should be skipped"); + }); + + test(`Given a manifest where all transitive dependency fetches throw + it should not produce unhandled promise rejections`, async(t) => { + const rootManifest = { + name: "test-pkg", + version: "1.0.0", + dependencies: { + "fail-a": "^1.0.0", + "fail-b": "^1.0.0", + "fail-c": "^1.0.0" + } + }; + + const mockedPacoteProvider = { + manifest: t.mock.fn(async() => { + throw new Error("registry unreachable"); + }), + packument: t.mock.fn(async() => { + throw new Error("registry unreachable"); + }) + }; + + const walker = new npm.TreeWalker({ + providers: { + pacote: mockedPacoteProvider + } + }); + + // This should complete without throwing and without leaking unhandled rejections + const dependencies: DependencyJSON[] = []; + for await (const dependency of walker.walk(rootManifest, { maxDepth: 10 })) { + dependencies.push(dependency); + } + + // Only root should be yielded + assert.strictEqual(dependencies.length, 1); + assert.strictEqual(dependencies[0].name, "test-pkg"); + assert.strictEqual(dependencies[0].existOnRemoteRegistry, false); + }); + }); }); function sortByName(left: string, right: string) {