Add Dependency Review and Submission #21

aegilops · 2025-12-04T17:54:39Z

Adds branch scanning, dependency submission, and more robustness.

fetching SBOM diffs for non-default branches
automatic dependency snapshot submission using Component Detection
better CLI options and documentation
improved advisory sync reliability
support for org listing with Enterprise Server

…ew and submission

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 15 out of 18 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/sbomCollector.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 15 out of 18 changed files in this pull request and generated 15 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/sbomCollector.ts

src/componentSubmission.ts

src/sbomCollector.ts

src/componentSubmission.ts

src/componentDetection.ts

src/cli.ts

src/sbomCollector.ts

src/componentSubmission.ts

src/cli.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ity/github-sbom-toolkit into dependency-review

Copilot

Pull request overview

Copilot reviewed 15 out of 18 changed files in this pull request and generated 12 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/componentDetection.ts

src/sbomCollector.ts

src/componentSubmission.ts

src/types.ts

src/componentSubmission.ts

src/sbomCollector.ts

Copilot · 2025-12-08T18:15:55Z

src/sbomCollector.ts

+                  console.debug(chalk.blue(`Force-submission enabled: submitting component snapshot for ${fullName} branch ${b.name}...`));
+                  if (await submitSnapshotIfPossible({ octokit: this.octokit, owner: org, repo: repo.name, branch: b.name, languages: this.opts.submitLanguages, quiet: this.opts.quiet, componentDetectionBinPath: this.opts.componentDetectionBinPath })) {
+                    // brief delay to allow snapshot ingestion
+                    await new Promise(r => setTimeout(r, 1500));


The magic number 1500 (milliseconds) for the delay after snapshot submission is hardcoded. This delay allows time for snapshot ingestion before retrying the dependency review diff. Consider:

Extracting this as a named constant (e.g., SNAPSHOT_INGESTION_DELAY_MS) to make its purpose clear

Making it configurable through options if different GitHub instances have different ingestion times

Adding a comment explaining why this specific duration was chosen

The same applies to the 3000ms delay on line 539.

@copilot open a new pull request to apply changes based on this feedback

src/sbomCollector.ts

src/componentDetection.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot · 2025-12-08T18:21:14Z

@aegilops I've opened a new pull request, #28, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot

Pull request overview

Copilot reviewed 15 out of 18 changed files in this pull request and generated 12 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/componentDetection.ts

src/serialization.ts

src/malwareAdvisories.ts

src/types.ts

src/sbomCollector.ts

src/componentSubmission.ts

src/sbomCollector.ts

src/componentSubmission.ts

src/componentDetection.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 15 out of 18 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-08T18:38:21Z

src/sbomCollector.ts

+                console.debug(chalk.green(`Fetching branch diff for ${fullName} branch ${b.name}...`));
+              } else {
+                console.debug(chalk.yellow(`Skipping branch diff for ${fullName} branch ${b.name} (no new commits).`));


[nitpick] The debug logging at lines 303 and 305 uses different colors (green for fetching, yellow for skipping). While this is helpful for human readers, it creates inconsistency with line 286 which uses blue. Consider standardizing debug message colors: e.g., blue for informational, yellow for warnings/skips, green for success, red for errors.

Suggested change

console.debug(chalk.green(`Fetching branch diff for ${fullName} branch ${b.name}...`));

} else {

console.debug(chalk.yellow(`Skipping branch diff for ${fullName} branch ${b.name} (no new commits).`));

console.debug(chalk.blue(`Fetching branch diff for ${fullName} branch ${b.name}...`));

} else {

console.debug(chalk.blue(`Skipping branch diff for ${fullName} branch ${b.name} (no new commits).`));

Copilot · 2025-12-08T18:38:22Z

CHANGELOG.md

@@ -0,0 +1,32 @@
+# Changelog
+
+## [2025-12-04] – 0.2.0 - Branch scanning and dependency submission


[nitpick] The CHANGELOG.md file shows a date of [2025-12-04] on line 3, but the current date is December 8, 2025. This appears to be a date in the past (4 days ago). While this could be intentional if the work was completed then, typically changelogs reflect the actual release date. Verify this is the intended date.

Suggested change

## [2025-12-04] – 0.2.0 - Branch scanning and dependency submission

## [2025-12-08] – 0.2.0 - Branch scanning and dependency submission

Copilot · 2025-12-08T18:38:22Z

src/sbomCollector.ts

+    }
+    // Validate repo format if provided
+    if (options.repo) {
+      if (typeof options.repo !== "string" || !options.repo.includes("/")) {


The error message at line 54 says 'If specifying "repo", it must be in the format "org/repo".' However, this is checking if the repo option is not a string OR doesn't include a slash. If options.repo is not a string, the error message is misleading because it assumes the value was provided but in wrong format. Consider separate error messages for "repo must be a string" vs "repo must be in format org/repo".

Suggested change

if (typeof options.repo !== "string" || !options.repo.includes("/")) {

if (typeof options.repo !== "string") {

throw new Error('If specifying "repo", it must be a string.');

}

if (!options.repo.includes("/")) {

Copilot · 2025-12-08T18:38:23Z

src/componentDetection.ts

+      let downloadURL: string = "";
+      // TODO: do we need to handle different architectures here?
+      // can we allow x64 on MacOS? We could allow an input parameter to override?
+      const assetName = process.platform === "win32" ? "component-detection-win-x64.exe" : process.platform === "linux" ? "component-detection-linux-x64" : "component-detection-osx-arm64";


In componentDetection.ts at line 318, the platform check uses a ternary for Windows vs Linux vs macOS, but the logic process.platform === "linux" ? "component-detection-linux-x64" : "component-detection-osx-arm64" assumes anything that's not Windows or Linux is macOS. This could fail on other platforms. Consider adding an explicit check for darwin platform or throwing an error for unsupported platforms.

Suggested change

const assetName = process.platform === "win32" ? "component-detection-win-x64.exe" : process.platform === "linux" ? "component-detection-linux-x64" : "component-detection-osx-arm64";

let assetName: string;

if (process.platform === "win32") {

assetName = "component-detection-win-x64.exe";

} else if (process.platform === "linux") {

assetName = "component-detection-linux-x64";

} else if (process.platform === "darwin") {

assetName = "component-detection-osx-arm64";

} else {

throw new Error(`Unsupported platform: ${process.platform}`);

}

Copilot · 2025-12-08T18:38:23Z

src/cli.ts

        if (args.enterprise && args.org) throw new Error("Specify only one of --enterprise or --org");
+        if (args.repo && (args.enterprise || args.org)) throw new Error("Specify only one of --enterprise, --org, or --repo");
+        if (args.repo && !(args.repo as string).includes("/")) throw new Error("--repo must be in the format owner/repo");
+        if (syncing && !args.sbomCache) throw new Error("--sync-sboms requires --sbom-cache to write updated SBOMs to disk");


[nitpick] In the CLI argument checking at lines 55-66, there's logic validating that --sync-sboms requires either --enterprise, --org, or --repo. However, at line 62, there's an additional check if (syncing && !args.sbomCache) that requires --sbom-cache to be provided when syncing. This check should logically be part of the main syncing validation block starting at line 56, not as a separate check, to keep related validation logic together and improve readability.

Suggested change

if (syncing && !args.sbomCache) throw new Error("--sync-sboms requires --sbom-cache to write updated SBOMs to disk");

if (!args.sbomCache) throw new Error("--sync-sboms requires --sbom-cache to write updated SBOMs to disk");

Copilot · 2025-12-08T18:38:23Z

src/malwareMatcher.ts

+          // Some ecosystems require a namespace (e.g., Maven, NuGet)
+          const ecosystemsWithNamespace = new Set(['maven', 'nuget', 'composer', 'golang']);
+          if (ecosystemsWithNamespace.has(change.ecosystem) && change.namespace) {
+            p = `pkg:${change.ecosystem}/${change.namespace}/${change.name}${change.version ? '@' + change.version : ''}`;
+          } else {
+            p = `pkg:${change.ecosystem}/${change.name}${change.version ? '@' + change.version : ''}`;
+          }
+


In the malwareMatcher.ts file, lines 162-169 appear to be orphaned code that doesn't belong to any function. These lines seem to be constructing a purl for Maven/NuGet ecosystems with namespaces, but they are placed outside of any function context. This code should either be removed or properly integrated into the enumerateDiffPackages function where it logically belongs.

Copilot · 2025-12-08T18:38:24Z

src/malwareMatcher.ts

+          // Some ecosystems require a namespace (e.g., Maven, NuGet)
+          const ecosystemsWithNamespace = new Set(['maven', 'nuget', 'composer', 'golang']);
+          if (ecosystemsWithNamespace.has(change.ecosystem) && change.namespace) {
+            p = `pkg:${change.ecosystem}/${change.namespace}/${change.name}${change.version ? '@' + change.version : ''}`;
+          } else {
+            p = `pkg:${change.ecosystem}/${change.name}${change.version ? '@' + change.version : ''}`;
+          }


The enumerateBasePackages function is annotated to return packages with __branch property (line 150), but the code at line 161 doesn't actually set this property on any returned packages. The comment at line 161 mentions "Annotate with default branch for reporting (if known)" but no annotation code follows. Either the code to set __branch is missing, or the return type annotation should not include __branch for base packages.

Suggested change

// Some ecosystems require a namespace (e.g., Maven, NuGet)

const ecosystemsWithNamespace = new Set(['maven', 'nuget', 'composer', 'golang']);

if (ecosystemsWithNamespace.has(change.ecosystem) && change.namespace) {

p = `pkg:${change.ecosystem}/${change.namespace}/${change.name}${change.version ? '@' + change.version : ''}`;

} else {

p = `pkg:${change.ecosystem}/${change.name}${change.version ? '@' + change.version : ''}`;

}

const branch = (repo as any).defaultBranch || undefined;

return list.map(pkg => ({

...pkg,

__branch: branch

}));

Copilot · 2025-12-08T18:38:24Z

src/sbomCollector.ts

+// Move applyQueries to module scope
+function applyQueries(
+  candidatePurls: string[],
+  queries: ParsedQuery[],
+  found: Map<string, string>,
+  branchTag?: string,
+  fallbackVersion?: string
+) {
+  const unique = Array.from(new Set(candidatePurls));
+  for (const p of unique) {
+    const pLower = p.toLowerCase();
+    const outKey = branchTag ? `${p}@${branchTag}` : p;
+    for (const q of queries) {
+      if (q.isPrefixWildcard) {
+        const prefix = q.lower.slice(0, -1);
+        if (pLower.startsWith(prefix)) { if (!found.has(outKey)) found.set(outKey, q.raw); }
+        continue;
+      }
+      if (q.versionConstraint && q.type && q.name) {
+        if (!pLower.startsWith("pkg:")) continue;
+        const body = p.slice(4);
+        const atIdx = body.indexOf("@");
+        const main = atIdx >= 0 ? body.slice(0, atIdx) : body;
+        const ver = atIdx >= 0 ? body.slice(atIdx + 1) : fallbackVersion;
+        const slashIdx = main.indexOf("/");
+        if (slashIdx < 0) continue;
+        const pType = main.slice(0, slashIdx).toLowerCase();
+        const pName = main.slice(slashIdx + 1);
+        if (pType === q.type && pName.toLowerCase() === q.name.toLowerCase() && ver) {
+          try {
+            const coerced = semver.coerce(ver)?.version || ver;
+            if (semver.valid(coerced) && semver.satisfies(coerced, q.versionConstraint, { includePrerelease: true })) {
+              if (!found.has(outKey)) found.set(outKey, q.raw);
+            }
+          } catch { /* ignore */ }
+        }
+      } else if (q.exact) {
+        if (pLower === q.exact) { if (!found.has(outKey)) found.set(outKey, q.raw); }
+      }
+    }
+  }
+}


The applyQueries function at line 595-635 in sbomCollector.ts is declared at module scope (outside the class), which breaks the expected encapsulation. This function appears to be extracted from inside the searchByPurlsWithReasons method but is placed incorrectly. The comment at line 594 says "// Move applyQueries to module scope" which suggests this was intentional, but it's placed in the middle of the class definition rather than before or after it, making the code structure confusing and potentially breaking the class context.

Copilot · 2025-12-08T18:38:24Z

src/sbomCollector.ts

+                continue;
+              }
+
+              const base = this.opts.branchDiffBase || sbom?.defaultBranch;


At line 290, there's a condition checking sbom.defaultBranch, but at this point in the flow, if skipped is true, sbom is the baseline (line 280) which may not have the defaultBranch property set if it came from an older cache. Similarly at line 313 for sbom?.defaultBranch. It would be safer to use repo.default_branch from the repo metadata which is known to be available.

Suggested change

const base = this.opts.branchDiffBase || sbom?.defaultBranch;

const base = this.opts.branchDiffBase || repo.default_branch;

Copilot · 2025-12-08T18:38:25Z

src/componentSubmission.ts

+export async function getLanguageIntersection(octokit: Octokit, owner: string, repo: string, languages: string[] | undefined, quiet: boolean = false): Promise<string[]> {
+    const langResp = await octokit.request('GET /repos/{owner}/{repo}/languages', { owner, repo });
+    const repoLangs = Object.keys(langResp.data || {});
+    const wanted = languages;
+    const intersect = wanted ? repoLangs.filter(l => wanted.some(w => w.toLowerCase() === l.toLowerCase())) : repoLangs;
+    if (!intersect.length) {
+        if (!quiet) console.error(chalk.yellow(`Skipping submission: none of selected languages present in repo (${repoLangs.join(', ')})`));
+        return [];
+    }
+    return intersect;


[nitpick] The function getLanguageIntersection is declared as async but doesn't actually await anything - the only await-able operation is octokit.request which is correctly awaited. However, the function is marked as returning Promise<string[]> which is correct for async functions. This is not a bug, but the async keyword could be removed for clarity since the function doesn't use any await internally. Minor optimization - not critical.

Suggested change

export async function getLanguageIntersection(octokit: Octokit, owner: string, repo: string, languages: string[] | undefined, quiet: boolean = false): Promise<string[]> {

const langResp = await octokit.request('GET /repos/{owner}/{repo}/languages', { owner, repo });

const repoLangs = Object.keys(langResp.data || {});

const wanted = languages;

const intersect = wanted ? repoLangs.filter(l => wanted.some(w => w.toLowerCase() === l.toLowerCase())) : repoLangs;

if (!intersect.length) {

if (!quiet) console.error(chalk.yellow(`Skipping submission: none of selected languages present in repo (${repoLangs.join(', ')})`));

return [];

}

return intersect;

export function getLanguageIntersection(octokit: Octokit, owner: string, repo: string, languages: string[] | undefined, quiet: boolean = false): Promise<string[]> {

return octokit.request('GET /repos/{owner}/{repo}/languages', { owner, repo }).then(langResp => {

const repoLangs = Object.keys(langResp.data || {});

const wanted = languages;

const intersect = wanted ? repoLangs.filter(l => wanted.some(w => w.toLowerCase() === l.toLowerCase())) : repoLangs;

if (!intersect.length) {

if (!quiet) console.error(chalk.yellow(`Skipping submission: none of selected languages present in repo (${repoLangs.join(', ')})`));

return [];

}

return intersect;

});

aegilops added 28 commits November 26, 2025 15:49

Added using Dependency Review on branches

c7899b5

Added Dependency Submission with Component Detection

d2a6b9f

Added language choice to submission

ec32784

Debug of submission, partial progress, but not working yet

5043ccb

Single repo scanning added. More scaffolding added to enable Dep Revi…

a729fb3

…ew and submission

Runs component-detection now, but is killed on MacOS

506b9a2

Allow using component-detection at a provided path

68f5d22

Refactor, make branch submission independent of freshly fetching SBOM

51a111e

Removed unused function

6f43ed0

Works for branch changes

c002406

Formatting

4787c2e

Updated malware matching to do diffs

290c070

Updated README

e55933e

Removed default branch limit

3e94895

Update to better handling rate limiting when querying malware database

5364da0

Allows just malware sync/caching

00116c5

Fix single repo behaviour on first pass

5c92626

Fixed missing diff from skipped branches

9f284c2

Allow forcing submission with Component Detection

f6d37c2

Fixed not waiting for process

33415a9

Now runs Component Detection correctly, on correct path

3e65ebc

Fixed mistake reading wrong path in Component Detection

6314b14

Fixed missing graphs by normalising appropriately

99b20d9

CHANGELOG

3a89bfb

Fixed submission by fixing calling of git to capture STDOUT properly

1887425

Update docs

9ea2fa0

Update GHES URL for listing orgs

7f13c04

CHANGELOG

dab9c61

Copilot AI review requested due to automatic review settings December 4, 2025 17:54

Copilot started reviewing on behalf of aegilops December 4, 2025 17:55 View session

Update src/componentDetection.ts

e6b11d9

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings December 8, 2025 17:17

Copilot started reviewing on behalf of aegilops December 8, 2025 17:17 View session

aegilops and others added 2 commits December 8, 2025 17:18

Use exact comparison

f963d36

Update src/sbomCollector.ts

1d04ee8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI reviewed Dec 8, 2025

View reviewed changes

src/sbomCollector.ts Outdated Show resolved Hide resolved

src/sbomCollector.ts Show resolved Hide resolved

src/sbomCollector.ts Show resolved Hide resolved

src/sbomCollector.ts Outdated Show resolved Hide resolved

aegilops and others added 3 commits December 8, 2025 17:40

Clarified comment

46f8f8c

Removed type ignore

fb52fb9

Update src/componentDetection.ts

4997078

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings December 8, 2025 17:52

Copilot started reviewing on behalf of aegilops December 8, 2025 17:52 View session

Formatted

e3aaf6f

Copilot AI reviewed Dec 8, 2025

View reviewed changes

Update src/sbomCollector.ts

4dcf2b3

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings December 8, 2025 18:07

Copilot started reviewing on behalf of aegilops December 8, 2025 18:08 View session

Merge branch 'dependency-review' of https://github.com/advanced-secur…

5ac59f7

…ity/github-sbom-toolkit into dependency-review

Copilot AI reviewed Dec 8, 2025

View reviewed changes

Update src/componentDetection.ts

ba8ff05

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings December 8, 2025 18:16

Copilot started reviewing on behalf of aegilops December 8, 2025 18:17 View session

Update src/sbomCollector.ts

5431556

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI mentioned this pull request Dec 8, 2025

Make snapshot ingestion delays configurable #28

Draft

Merge branch 'main' into dependency-review

a1e931f

Copilot AI reviewed Dec 8, 2025

View reviewed changes

Update src/malwareMatcher.ts

4b2e75c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings December 8, 2025 18:30

Copilot started reviewing on behalf of aegilops December 8, 2025 18:30 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

		@@ -0,0 +1,32 @@
		# Changelog

		## [2025-12-04] – 0.2.0 - Branch scanning and dependency submission

	## [2025-12-04] – 0.2.0 - Branch scanning and dependency submission
	## [2025-12-08] – 0.2.0 - Branch scanning and dependency submission

-      if (typeof options.repo !== "string" || !options.repo.includes("/")) {
+      if (typeof options.repo !== "string") {
+        throw new Error('If specifying "repo", it must be a string.');
+      }
+      if (!options.repo.includes("/")) {

-      const assetName = process.platform === "win32" ? "component-detection-win-x64.exe" : process.platform === "linux" ? "component-detection-linux-x64" : "component-detection-osx-arm64";
+      let assetName: string;
+      if (process.platform === "win32") {
+        assetName = "component-detection-win-x64.exe";
+      } else if (process.platform === "linux") {
+        assetName = "component-detection-linux-x64";
+      } else if (process.platform === "darwin") {
+        assetName = "component-detection-osx-arm64";
+      } else {
+        throw new Error(`Unsupported platform: ${process.platform}`);
+      }

	if (syncing && !args.sbomCache) throw new Error("--sync-sboms requires --sbom-cache to write updated SBOMs to disk");
	if (!args.sbomCache) throw new Error("--sync-sboms requires --sbom-cache to write updated SBOMs to disk");

	const base = this.opts.branchDiffBase \|\| sbom?.defaultBranch;
	const base = this.opts.branchDiffBase \|\| repo.default_branch;

Add Dependency Review and Submission #21

Are you sure you want to change the base?

Add Dependency Review and Submission #21

Uh oh!

Conversation

aegilops commented Dec 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

aegilops Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Dec 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!