Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions app/components/Package/Dependencies.vue
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,17 @@ function getDeprecatedDepInfo(depName: string) {
return vulnTree.value.deprecatedPackages.find(p => p.name === depName && p.depth === 'direct')
}

// Cache URL dependency lookups with computed map
const urlDepMap = computed(() => {
if (!vulnTree.value) return new Map()
return new Map(vulnTree.value.urlDependencies.map(dep => [dep.name, dep]))
})

// Check if a dependency uses git: or https: URL
function getUrlDepInfo(depName: string) {
return urlDepMap.value.get(depName) ?? null
}

// Expanded state for each section
const depsExpanded = shallowRef(false)
const peerDepsExpanded = shallowRef(false)
Expand Down Expand Up @@ -73,6 +84,8 @@ const sortedOptionalDependencies = computed(() => {

// Get version tooltip
function getDepVersionTooltip(dep: string, version: string) {
const urlDep = getUrlDepInfo(dep)
if (urlDep) return urlDep.url
const outdated = outdatedDeps.value[dep]
if (outdated) return getOutdatedTooltip(outdated, t)
if (getVulnerableDepInfo(dep) || getDeprecatedDepInfo(dep)) return version
Expand All @@ -82,6 +95,7 @@ function getDepVersionTooltip(dep: string, version: string) {

// Get version class
function getDepVersionClass(dep: string) {
if (getUrlDepInfo(dep)) return 'text-orange-700 dark:text-orange-500'
const outdated = outdatedDeps.value[dep]
if (outdated) return getVersionClass(outdated)
if (getVulnerableDepInfo(dep) || getDeprecatedDepInfo(dep)) return getVersionClass(undefined)
Expand Down Expand Up @@ -164,6 +178,19 @@ const numberFormatter = useNumberFormatter()
>
<span class="sr-only">{{ $t('package.deprecated.label') }}</span>
</LinkBase>
<TooltipApp
v-if="getUrlDepInfo(dep)"
class="shrink-0 text-orange-700 dark:text-orange-500"
:text="getUrlDepInfo(dep)!.url"
>
<button
type="button"
class="p-2 -m-2"
:aria-label="`git/https dependency: ${getUrlDepInfo(dep)!.url}`"
Copy link
Copy Markdown
Member

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!

>
<span class="i-lucide:triangle-alert w-3 h-3" aria-hidden="true" />
</button>
</TooltipApp>
<LinkBase
:to="packageRoute(dep, version)"
class="block truncate"
Expand Down
70 changes: 69 additions & 1 deletion server/utils/dependency-analysis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type {
PackageVulnerabilityInfo,
VulnerabilityTreeResult,
DeprecatedPackageInfo,
UrlDependencyInfo,
OsvAffected,
OsvRange,
} from '#shared/types/dependency-analysis'
Expand Down Expand Up @@ -255,6 +256,64 @@ 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 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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('git+') ||
url.startsWith('http:') ||
url.startsWith('https:') ||
url.startsWith('file:')
Comment on lines +264 to +268
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per https://docs.npmjs.com/cli/v11/configuring-npm/package-json#git-urls-as-dependencies, these are also valid and insecure:

  • github:foo/bar
  • gist:foo
  • bitbucket:foo/bar
  • gitlab:foo/bar
  • foo/bar (this means GitHub)

🤔 We can't just assume any protocol is insecure, as npm: is fine.

Maybe something like this works?

Suggested change
url.startsWith('git:') ||
url.startsWith('git+') ||
url.startsWith('http:') ||
url.startsWith('https:') ||
url.startsWith('file:')
// starts with any `foo:` except `npm:`
url.match(/^(?!npm:)[^:]+:/) ||
// foo/bar
url.match(/^[^/]+/[^/]+$/)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oooh wait a sec, I think we can just do...

Suggested change
url.startsWith('git:') ||
url.startsWith('git+') ||
url.startsWith('http:') ||
url.startsWith('https:') ||
url.startsWith('file:')
!semver.valid(url) && !url.startsWith('npm:')

👀 although we'd have to think through "loose" mode and coercion to be sure we're using the same logic as package managers do.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I found this live example of a package with a dependency pointing at github:: https://npmx-r89mhfl1x-npmx.vercel.app/package/react-native-gauge

)
}

/**
* 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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 path that "leads" to a given package is returned, so it seems it should be possible to derive what we need directly, no? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 resolveDependencyTree directly.

const versionData = packument.versions[version]
if (!versionData) return []

const urlDeps: UrlDependencyInfo[] = []
// Include devDependencies only for the root package
const allDeps = depth === 'root'
? {
...versionData.dependencies,
...versionData.optionalDependencies,
...versionData.devDependencies,
}
: {
...versionData.dependencies,
...versionData.optionalDependencies,
}

for (const [depName, depUrl] of Object.entries(allDeps || {})) {
if (isUrlDependency(depUrl)) {
urlDeps.push({
name: depName,
url: depUrl,
depth,
path: [...path, `${depName}@${depUrl}`],
Comment thread
coderabbitai[bot] marked this conversation as resolved.
})
}
}

return urlDeps
} catch {
return []
Comment thread
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
Expand Down Expand Up @@ -289,6 +348,14 @@ export const analyzeDependencyTree = defineCachedFunction(
return depthOrder[a.depth] - depthOrder[b.depth]
})

// Scan for git: and https: URL dependencies in all packages
const urlDepResults = await mapWithConcurrency(
packages,
pkg => scanUrlDependencies(pkg.name, pkg.version, pkg.depth, pkg.path),
OSV_DETAIL_CONCURRENCY,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create a separate constant for this, as it's unrelated to fetching OSV details

)
const urlDependencies = urlDepResults.flat()

// 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)
Expand Down Expand Up @@ -347,6 +414,7 @@ export const analyzeDependencyTree = defineCachedFunction(
version,
vulnerablePackages,
deprecatedPackages,
urlDependencies,
totalPackages: packages.length,
failedQueries,
totalCounts,
Expand All @@ -356,6 +424,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}`,
},
)
15 changes: 15 additions & 0 deletions shared/types/dependency-analysis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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+) */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 how could root ever happen? this is only a possibility in the dependencies array of another package, isn't it?

depth: DependencyDepth
/** Dependency path from root package */
path: string[]
}

/**
* Result of dependency tree analysis
*/
Expand All @@ -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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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
/** All dependencies using git: or https: URLs */
urlDependencies: UrlDependencyInfo[]
/** All packages using unsafe `git:`, `https:` (etc.) URLs in the tree */
packagesWithUrlVersion: UrlDependencyInfo[]

/** Total packages analyzed */
totalPackages: number
/** Number of packages that could not be checked (OSV query failed) */
Expand Down
Loading