feat: add CVE checking in java upgrade promotion

[yezhu6]

Add critical and high severity CVE issues checking.

[wangmingliang-ms]

LGTM

[Copilot]

Pull request overview

This pull request adds CVE (Common Vulnerabilities and Exposures) checking functionality to the Java upgrade promotion feature. The implementation fetches critical and high-severity CVE data from GitHub's Security Advisories API and notifies users about vulnerabilities in their Java dependencies.

Key Changes:

• Integrated GitHub Octokit library to query security advisories for Maven dependencies
• Added CVE detection with severity filtering (critical and high only)
• Implemented batch processing to handle API URL length limits
• Updated notification system to display CVE-specific messages and actions

Reviewed changes

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

Show a summary per file

File	Description
src/upgrade/cve.ts	New file implementing CVE fetching, filtering, and mapping logic using GitHub Security Advisories API
src/upgrade/type.ts	Added DependencyCheckItemCve type and CVE enum value to support CVE-related upgrade issues
src/upgrade/utility.ts	Added buildCVENotificationMessage function for CVE-specific notifications and updated buildFixPrompt to handle CVE reason
src/upgrade/display/notificationManager.ts	Updated notification rendering to handle CVE issues with dedicated button text and message formatting
src/upgrade/assessmentManager.ts	Refactored to extract dependency collection into getAllDependencies and integrated CVE checking via getCVEIssues
package.json	Added @octokit/rest dependency for GitHub API integration
CHANGELOG.md	Added version 0.27.0 entry documenting the new CVE checking feature

---

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

[Copilot]

Missing error handling in batchGetCVEIssues. While fetchCves has a try-catch block, errors in getCveUpgradeIssues or mapCvesToUpgradeIssues will propagate and could cause the entire assessment process to fail. Consider wrapping the loop body in a try-catch:

for (let i = 0; i < coordinates.length; i += BATCH_SIZE) {
  try {
    const batchCoordinates = coordinates.slice(i, i + BATCH_SIZE);
    const cveUpgradeIssues = await getCveUpgradeIssues(batchCoordinates);
    allCVEUpgradeIssues.push(...cveUpgradeIssues);
  } catch (error) {
    // Log error but continue processing other batches
    console.error('Error fetching CVE issues for batch:', error);
  }
}

Suggested change

	const batchCoordinates = coordinates.slice(i, i + BATCH_SIZE);
	const cveUpgradeIssues = await getCveUpgradeIssues(batchCoordinates);
	allCVEUpgradeIssues.push(...cveUpgradeIssues);
	try {
	const batchCoordinates = coordinates.slice(i, i + BATCH_SIZE);
	const cveUpgradeIssues = await getCveUpgradeIssues(batchCoordinates);
	allCVEUpgradeIssues.push(...cveUpgradeIssues);
	} catch (error) {
	// Log error but continue processing other batches
	console.error('Error fetching CVE issues for batch:', error);
	}

[Copilot]

Typo: "nodify" should be "notify".

Suggested change

	- feat - Add CVE checking to nodify users to fix the critical/high-severity CVE issues in https://github.com/microsoft/vscode-java-dependency/pull/948
	- feat - Add CVE checking to notify users to fix the critical/high-severity CVE issues in https://github.com/microsoft/vscode-java-dependency/pull/948

[Copilot]

Potential runtime error if the filtered array is empty. When accessing [0] on line 152, if the filter results in an empty array, mostCriticalCve will be undefined, causing errors when accessing properties like mostCriticalCve.id on line 157.

While fetchCves ensures each depCve.cves has at least one element, the redundant filtering here could theoretically produce an empty array in edge cases. Consider adding a check:

const filteredCves = [...depCve.cves]
  .filter((cve) => isCveAffectingDep(cve, depCve.dep, depCve.version))
  .sort((a, b) => Severity[b.severity] - Severity[a.severity]);

if (filteredCves.length === 0) {
  return null; // or handle appropriately
}
const mostCriticalCve = filteredCves[0];

Alternatively, remove the redundant filter since fetchCves already filters by isCveAffectingDep.

[Copilot]

Missing pagination handling for GitHub Security Advisories API. The per_page: 100 parameter limits results to 100 CVEs, but if there are more than 100 matching advisories, they will be silently ignored. This could miss critical vulnerabilities.

Consider implementing pagination to fetch all results:

let allData = [];
let page = 1;
let hasMore = true;

while (hasMore) {
  const response = await octokit.securityAdvisories.listGlobalAdvisories({
    ecosystem: "maven",
    affects: deps.map((p) => `${p.name}@${p.version}`),
    direction: "asc",
    sort: "published",
    per_page: 100,
    page
  });
  
  allData.push(...response.data);
  hasMore = response.data.length === 100;
  page++;
}

Suggested change

	const response = await octokit.securityAdvisories.listGlobalAdvisories({
	ecosystem: "maven",
	affects: deps.map((p) => `${p.name}@${p.version}`),
	direction: "asc",
	sort: "published",
	per_page: 100,
	});
	const allCves: CVE[] = response.data
	let allAdvisories: typeof response.data = [];
	let page = 1;
	let hasMore = true;
	while (hasMore) {
	const response = await octokit.securityAdvisories.listGlobalAdvisories({
	ecosystem: "maven",
	affects: deps.map((p) => `${p.name}@${p.version}`),
	direction: "asc",
	sort: "published",
	per_page: 100,
	page,
	});
	allAdvisories.push(...response.data);
	hasMore = response.data.length === 100;
	page++;
	}
	const allCves: CVE[] = allAdvisories

[wangmingliang-ms]

LGTM

[chagong]

I won't suggest send telemetry here in map. In case of big project that contains hundreds/thousands of packages, you may send big amount of telemetry event in a very short time, and causes telemetry data loss.

I would suggest set telemetry properties here and send only one event in the end.

[yezhu6]

In practice, allPackages is unlikely to have thousands of items because:

1. It's not counting individual dependencies - allPackages contains one entry per package container (like "Maven Dependencies", "Referenced Libraries", etc.), not per individual dependency.

2. Typical Java projects have very few containers - Usually just 2-5 containers per project (e.g., Maven Dependencies, JRE System Library, Project and External Dependencies).

3. The thousands of dependencies are nested inside - Each container result (x.value) contains an array of PackageDescription objects. Those inner arrays can potentially have hundreds of dependencies, but the outer allPackages array itself will be small.

[chagong]

what about the listed advisories is larger than 100? You don't handle pagination?

[yezhu6]

Yes, it's the potential issue.
Currently we use this same logic in our multiple scenarios and the pagination is the low priority TODO.
For our scenario, we just check if the CVE issue exist. I've crated an issue to track to add the pagination for all scenario https://github.com/devdiv-microsoft/vscode-java-upgrade/issues/1117