-
-
Notifications
You must be signed in to change notification settings - Fork 425
feat: flag git: and https: dependencies as security concern #2017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
761e38e
2840313
b10c55f
5d5cecc
8cb7b06
9e7c02a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import type { | |
| PackageVulnerabilityInfo, | ||
| VulnerabilityTreeResult, | ||
| DeprecatedPackageInfo, | ||
| UrlDependencyInfo, | ||
| OsvAffected, | ||
| OsvRange, | ||
| } from '#shared/types/dependency-analysis' | ||
|
|
@@ -255,6 +256,52 @@ function getSeverityLevel(vuln: OsvVulnerability): OsvSeverityLevel { | |
| return 'unknown' | ||
| } | ||
|
|
||
| /** | ||
| * Check if a dependency URL is a git: or https: URL that should be flagged. | ||
| */ | ||
| function isUrlDependency(url: string): boolean { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this function is a good candidate for great good unit test coverage. Let's do some table tests covering all the supported formats (see my comment below). |
||
| return url.startsWith('git:') || url.startsWith('https:') || url.startsWith('git+https:') | ||
| } | ||
|
|
||
| /** | ||
| * Scan a package's dependencies for git: and https: URLs. | ||
| * Returns a map of package names to their URL dependencies. | ||
| */ | ||
| async function scanUrlDependencies( | ||
| name: string, | ||
| version: string, | ||
| depth: DependencyDepth, | ||
| path: string[], | ||
| ): Promise<UrlDependencyInfo[]> { | ||
| try { | ||
| const packument = await fetchNpmPackage(name) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be very slow, since it's re-fetching the packument for every package in the tree. This data was already constructed to begin with by fetching all these packuments and the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it helps simplify things, I think it would even be fine to just attach a new field for this in |
||
| const versionData = packument.versions[version] | ||
| if (!versionData) return [] | ||
|
|
||
| const urlDeps: UrlDependencyInfo[] = [] | ||
| const allDeps = { | ||
| ...versionData.dependencies, | ||
| ...versionData.optionalDependencies, | ||
| ...versionData.devDependencies, | ||
| } | ||
|
|
||
| for (const [depName, depUrl] of Object.entries(allDeps || {})) { | ||
| if (isUrlDependency(depUrl)) { | ||
| urlDeps.push({ | ||
| name: depName, | ||
| url: depUrl, | ||
| depth, | ||
| path: [...path, `${depName}@${depUrl}`], | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }) | ||
| } | ||
| } | ||
|
|
||
| return urlDeps | ||
| } catch { | ||
| return [] | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Analyze entire dependency tree for vulnerabilities and deprecated packages. | ||
| * Uses OSV batch API for efficient vulnerability discovery, then fetches | ||
|
|
@@ -289,6 +336,9 @@ export const analyzeDependencyTree = defineCachedFunction( | |
| return depthOrder[a.depth] - depthOrder[b.depth] | ||
| }) | ||
|
|
||
| // Scan for git: and https: URL dependencies in the root package | ||
| const urlDependencies = await scanUrlDependencies(name, version, 'root', []) | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Step 1: Use batch API to find which packages have vulnerabilities | ||
| // This is much faster than individual queries - one request for all packages | ||
| const { vulnerableIndices, failed: batchFailed } = await queryOsvBatch(packages) | ||
|
|
@@ -347,6 +397,7 @@ export const analyzeDependencyTree = defineCachedFunction( | |
| version, | ||
| vulnerablePackages, | ||
| deprecatedPackages, | ||
| urlDependencies, | ||
| totalPackages: packages.length, | ||
| failedQueries, | ||
| totalCounts, | ||
|
|
@@ -356,6 +407,6 @@ export const analyzeDependencyTree = defineCachedFunction( | |
| maxAge: 60 * 60, | ||
| swr: true, | ||
| name: 'dependency-analysis', | ||
| getKey: (name: string, version: string) => `v2:${name}@${version}`, | ||
| getKey: (name: string, version: string) => `v3:${name}@${version}`, | ||
| }, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -180,6 +180,19 @@ export interface DeprecatedPackageInfo { | |||||||||
| message: string | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * URL-based dependency info (git:, https:) | ||||||||||
| */ | ||||||||||
| export interface UrlDependencyInfo { | ||||||||||
| name: string | ||||||||||
| /** The git: or https: URL */ | ||||||||||
| url: string | ||||||||||
| /** Depth in dependency tree: root (0), direct (1), transitive (2+) */ | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 how could root ever happen? this is only a possibility in the |
||||||||||
| depth: DependencyDepth | ||||||||||
| /** Dependency path from root package */ | ||||||||||
| path: string[] | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Result of dependency tree analysis | ||||||||||
| */ | ||||||||||
|
|
@@ -192,6 +205,8 @@ export interface VulnerabilityTreeResult { | |||||||||
| vulnerablePackages: PackageVulnerabilityInfo[] | ||||||||||
| /** All deprecated packages in the tree */ | ||||||||||
| deprecatedPackages: DeprecatedPackageInfo[] | ||||||||||
| /** All dependencies using git: or https: URLs */ | ||||||||||
| urlDependencies: UrlDependencyInfo[] | ||||||||||
|
Comment on lines
+208
to
+209
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The two fields above use the term "package", so it would be nice to have consistency here to avoid giving a false sense of difference:
Suggested change
|
||||||||||
| /** Total packages analyzed */ | ||||||||||
| totalPackages: number | ||||||||||
| /** Number of packages that could not be checked (OSV query failed) */ | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hardcoded English string needs to be internationalized. Check out guidance in CONTRIBUTING.md if you aren't familiar!