Conversation
…ing from top to bottom the semversions
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a prototype “quick fix” workflow under cmd/devguard-cli/test/ to fetch package metadata from registries (npm/crates) and attempt to validate whether updating a dependency chain results in a vulnerable package version being at/above a fixed version.
Changes:
- Add npm registry response type definitions for JSON unmarshalling.
- Add registry helper functions (npm/crates) and a quickfix CLI that walks a dependency chain.
- Add semver parsing/recommendation logic to select newer versions within the same major band.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
cmd/devguard-cli/test/types.go |
Introduces structs for npm registry JSON responses. |
cmd/devguard-cli/test/quickfix.go |
Implements the dependency-chain “quick fix” logic and a main() driver. |
cmd/devguard-cli/test/package_manager_functions.go |
Adds registry request helpers for npm/crates and a version existence check. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (7)
cmd/devguard-cli/test/quickfix_test.go:350
- Typo in test failure message: "shouldnt" -> "shouldn't".
t.Errorf("using just Name %q shouldnt find scoped dep", purl.Name)
}
cmd/devguard-cli/test/debian_resolver.go:296
- ResolveBestVersion() assumes allVersionsMeta.Versions[0] is the newest version when constraint is empty, but FetchPackageMetadata/parseParagraphs currently return Versions in the iteration order of Packages.xz (not guaranteed sorted). This can return a non-latest version. Consider sorting Versions descending once when building DebianResponse (and/or updating DebianResponse.Versions comment to match behavior).
// If no constraint, return newest version
if constraint == "" {
if len(allVersionsMeta.Versions) > 0 {
return allVersionsMeta.Versions[0], nil
}
cmd/devguard-cli/test/npm_resolver.go:177
- In the "exact" case, ResolveBestVersion() returns baseVersion without checking that it actually exists in allVersionsMeta (e.g., a dependency could specify an exact version that is not published). This can lead to a later 404 when fetching metadata. Consider verifying the version exists in allVersionsMeta.Versions (or returning a clear error) before returning it.
// For exact version, simply return the requested version; equality with currentVersion is allowed
if rangeType == "exact" {
if baseVersion == currentVersion {
return "", fmt.Errorf("exact version %s is same as current version, no upgrade possible", baseVersion)
}
return baseVersion, nil
}
cmd/devguard-cli/test/quickfix.go:169
- The PR adds substantial new decision logic in checkVulnerabilityFixChain()/CheckVulnerabilityFixChainAuto (version selection, dependency constraint resolution, and the final fixed-version check), but the current tests only cover helpers and NPM constraint parsing. Consider adding unit tests for the chain-resolution behavior using a small fake Resolver implementation (so tests don’t depend on network registries) to cover: (1) selecting an upgraded root version, (2) resolving next dependency versions from constraints (including empty/no-constraint), and (3) fixed-version comparison outcomes.
func checkVulnerabilityFixChain[T any](resolver Resolver[T], purls []packageurl.PackageURL, fixedVersion string) (string, error) {
if len(purls) < 2 {
return "", fmt.Errorf("purl array must contain at least 2 elements")
}
cmd/devguard-cli/test/debian_resolver.go:123
- fetchVersionMetadata downloads and decompresses the full Packages.xz for each call. Since checkVulnerabilityFixChain calls FetchPackageMetadata multiple times per package in the chain, this can result in repeatedly downloading large index files and significantly slow down runs (and put load on deb.debian.org). Consider adding caching keyed by (suite, arch) for the parsed Packages.xz contents, or fetching once per suite/arch and reusing it across resolutions.
// Fetch from Packages.xz for the specified suite
url := "https://deb.debian.org/debian/dists/" + suite + "/main/binary-" + arch + "/Packages.xz"
resp, err := httpClient.Get(url)
if err != nil {
return DebianResponse{}, fmt.Errorf("failed to fetch Packages.xz: %w", err)
}
defer resp.Body.Close()
cmd/devguard-cli/test/debian_resolver.go:175
- This error message hard-codes the string "suite" instead of including the actual suite that was queried. This makes debugging much harder when the lookup fails. Consider passing the suite into parseParagraphs() (or formatting the error in fetchVersionMetadata) so the error includes the real suite name.
if targetDependencies == nil {
return DebianResponse{}, fmt.Errorf("package %s@%s not found in %s", pkgName, pkgVersion, "suite")
}
cmd/devguard-cli/test/npm_resolver.go:255
- The comment says "remove quotes if present" but the code trims "/" characters. Either update the comment to match the behavior (trimming slashes) or change the trimming to what is actually intended.
normalizedVersion := strings.Trim(pkg.Version, "/") // remove quotes if present
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+396
to
+404
| // Normalize both versions by removing epoch and any +X suffixes for comparison | ||
| normalizeVer := func(v string) string { | ||
| // Remove epoch prefix | ||
| if idx := strings.Index(v, ":"); idx != -1 { | ||
| v = v[idx+1:] | ||
| } | ||
| // Remove any +X suffix (binary rebuild +b1, +b2, source mods +dfsg, etc.) | ||
| if idx := strings.Index(v, "+"); idx != -1 { | ||
| v = v[:idx] |
There was a problem hiding this comment.
debianVersionsMatch() currently strips everything after the first '+'. This will cause distinct Debian versions like "2.36-9+deb12u9" and "2.36-9+deb12u10" to normalize to the same value and be treated as equal, which can select the wrong package stanza (and therefore wrong dependencies). Consider only stripping known rebuild/source suffixes (e.g. "+bN", "+dfsg", "+ds") and keep security-update suffixes like "+debXXuY", or switch to parsing with pault.ag/go/debian/version and compare equality while handling the epoch mismatch separately.
Suggested change
| // Normalize both versions by removing epoch and any +X suffixes for comparison | |
| normalizeVer := func(v string) string { | |
| // Remove epoch prefix | |
| if idx := strings.Index(v, ":"); idx != -1 { | |
| v = v[idx+1:] | |
| } | |
| // Remove any +X suffix (binary rebuild +b1, +b2, source mods +dfsg, etc.) | |
| if idx := strings.Index(v, "+"); idx != -1 { | |
| v = v[:idx] | |
| // Regex to strip known rebuild/source suffixes but keep security-update suffixes like "+deb12u9". | |
| // Matches: | |
| // +bN (binary rebuilds) | |
| // +dfsg, +dfsgN | |
| // +ds, +dsN | |
| rebuildSuffixRe := regexp.MustCompile(`^(.*?)(\+b[0-9]+|\+dfsg[0-9]*|\+ds[0-9]*)$`) | |
| // Normalize both versions by removing epoch and known rebuild/source suffixes for comparison. | |
| normalizeVer := func(v string) string { | |
| // Remove epoch prefix. | |
| if idx := strings.Index(v, ":"); idx != -1 { | |
| v = v[idx+1:] | |
| } | |
| // Strip known rebuild/source suffixes at the end, but keep security-update suffixes like "+deb12u9". | |
| if matches := rebuildSuffixRe.FindStringSubmatch(v); matches != nil { | |
| v = matches[1] |
Comment on lines
+222
to
+226
| return "", fmt.Errorf("package %s not found in %s@%s dependencies", nextPkgName, pkgName, latestVersion) | ||
| } | ||
|
|
||
| fmt.Printf(" %s@%s requires %s: %s\n", pkgName, latestVersion, nextPkgName, nextVersionConstraint) | ||
|
|
There was a problem hiding this comment.
checkVulnerabilityFixChain treats an empty VersionConstraint as "dependency not found" (nextVersionConstraint == ""), but Debian dependencies often omit a version constraint and parseDependencies stores those as an empty string. This will incorrectly error for valid dependencies without constraints. Consider changing the Resolver interface to return (VersionConstraint, bool) (or a dedicated constraint type with an 'Exists' flag), and treat an empty-but-present constraint as "no constraint" rather than "missing dependency".
Suggested change
| return "", fmt.Errorf("package %s not found in %s@%s dependencies", nextPkgName, pkgName, latestVersion) | |
| } | |
| fmt.Printf(" %s@%s requires %s: %s\n", pkgName, latestVersion, nextPkgName, nextVersionConstraint) | |
| // No explicit version constraint: dependency exists but is unconstrained. | |
| fmt.Printf(" %s@%s has unconstrained dependency %s\n", pkgName, latestVersion, nextPkgName) | |
| } else { | |
| fmt.Printf(" %s@%s requires %s: %s\n", pkgName, latestVersion, nextPkgName, nextVersionConstraint) | |
| } |
…vguard into Improve-Quick-Fix
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 32 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (2)
database/migrations/20260304170435_add_direct_dependency_fixed_version.down.sql:15
- Down migration is empty, so rolling back won’t remove the
direct_dependency_fixed_versioncolumn. Add the correspondingALTER TABLE ... DROP COLUMN ...statement (ideally also guarded withIF EXISTS).
-- Copyright 2026 larshermges
--
-- Licensed under the Apache License, Version 2.0 (the "License");
-- you may not use this file except in compliance with the License.
-- You may obtain a copy of the License at
--
-- https://www.apache.org/licenses/LICENSE-2.0
--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
database/migrations/20260206102932_add_csaf_package_scope.down.sql:15
- Down migration is empty, so rolling back won’t remove the
csaf_package_scopecolumn introduced in the corresponding up migration. Add the reverseALTER TABLE ... DROP COLUMN ...statement.
-- Copyright 2026 larshermges
--
-- Licensed under the Apache License, Version 2.0 (the "License");
-- you may not use this file except in compliance with the License.
-- You may obtain a copy of the License at
--
-- https://www.apache.org/licenses/LICENSE-2.0
--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+94
to
+95
| var currentMajor, currentMinor, currentPatch int | ||
| if _, err := fmt.Sscanf(currentVersion, "%d.%d.%d", ¤tMajor, ¤tMinor, ¤tPatch); err != nil { |
Comment on lines
+256
to
+267
| // Build full package name (handles scoped packages like @babel/core) | ||
| fullName := pkg.Name | ||
| if pkg.Namespace != "" { | ||
| fullName = pkg.Namespace + "/" + pkg.Name | ||
| } | ||
| encodedName := url.QueryEscape(fullName) | ||
|
|
||
| if pkg.Version != "" { | ||
| req, err = httpClient.Get("https://registry.npmjs.org/" + encodedName + "/" + normalizedVersion) | ||
| } else { | ||
| req, err = httpClient.Get("https://registry.npmjs.org/" + encodedName) | ||
| } |
Comment on lines
+105
to
+110
| // save as json | ||
| b, _ := json.Marshal(packages) // nolint | ||
| fmt.Printf("Fetched and parsed Packages.xz for suite %s and arch %s, total packages: %d\n", suite, arch, len(packages)) | ||
|
|
||
| os.WriteFile("packages_"+suite+"_"+arch+".json", b, 0644) // nolint | ||
|
|
Comment on lines
+275
to
+286
| func NewVulnerabilityPathAnalysisFixedVersionResolver() *VulnerabilityPathAnalysisFixedVersionResolver { | ||
| debianResolver := NewDebianResolver() | ||
| npmResolver := &NPMResolver{} | ||
|
|
||
| d := &VulnerabilityPathAnalysisFixedVersionResolver{ | ||
| debianResolver: debianResolver, | ||
| npmResolver: npmResolver, | ||
| } | ||
|
|
||
| d.debianResolver.getPackagesXZ("bookworm", "amd64") | ||
|
|
||
| return d |
Comment on lines
+248
to
+253
| if isFixed { | ||
| return purls[0].String(), nil | ||
| } | ||
|
|
||
| return "", nil | ||
| } |
| if componentPurl != "" { | ||
| countQuery = countQuery.Where("component_purl LIKE ?", "%"+componentPurl+"%") | ||
| } | ||
| countQuery.Model(&models.DependencyVuln{}).Count(&totalCount) |
Comment on lines
+76
to
+77
| runner.RunResolveFixedVersionsPipeline(false) | ||
| return nil |
| runner.RunResolveFixedVersionsPipeline(false) | ||
| return nil | ||
| }); err != nil { | ||
| slog.Error("could not resolve direct depend ency fixed versions", "err", err) |
Comment on lines
+115
to
+116
| DaemonRunner shared.DaemonRunner | ||
| FixVersionResolver shared.FixedVersionResolver |
| modernc.org/sqlite v1.46.1 // indirect | ||
| modernc.org/strutil v1.2.1 // indirect | ||
| modernc.org/zappy v1.0.0 // indirect | ||
| pault.ag/go/topsort v0.1.1 // indirect |
…d allocating a single big string
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.