feat: flag git: and https: dependencies as security concern#2017
feat: flag git: and https: dependencies as security concern#2017ndpvt-web wants to merge 6 commits intonpmx-dev:mainfrom
Conversation
Adds detection and flagging for dependencies that use git: or https: URLs, which can be manipulated as noted in the issue discussion. These URL-based dependencies bypass npm registry integrity checks. Closes npmx-dev#1084 Co-Authored-By: AI Assistant (Claude) <ai-assistant@contributor-bot.dev> Signed-off-by: ndpvt-web <ndpvt-web@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds detection, aggregation and UI reporting for URL-based dependencies. A new UrlDependencyInfo type (name, url, depth, path) is introduced and analyzeDependencyTree now scans packages for git:/git+:/http(s):/file: dependencies, returning urlDependencies: UrlDependencyInfo[] and using a 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/utils/dependency-resolver.ts (1)
109-110: Field declared but not populated in this file.The
urlfield is added to the interface, but theresolveDependencyTreefunction doesn't populate it. Looking atresolveVersion(lines 82-92), URL-based dependencies returnnulland are excluded from resolution. The actual URL dependency detection happens independency-analysis.tsviascanUrlDependencies.Consider whether this field should be:
- Removed if URL dependencies are handled entirely separately (current approach via
scanUrlDependencies)- Populated here if you want the resolver to track URLs for dependencies it encounters
The current implementation works because
scanUrlDependenciesreads URLs directly from the packument. However, this orphaned field could cause confusion for future maintainers.server/utils/dependency-analysis.ts (1)
259-264: Consider aligning URL detection with resolveVersion patterns.The
isUrlDependencyfunction checks forgit:,https:, andgit+https:prefixes. However,resolveVersionindependency-resolver.ts(lines 82-92) handles additional patterns that bypass registry resolution:
http://git+http:git+ssh:file:Depending on the security concerns you want to surface, you may want to align these checks:
♻️ Suggested expansion
function isUrlDependency(url: string): boolean { - return url.startsWith('git:') || url.startsWith('https:') || url.startsWith('git+https:') + return ( + url.startsWith('git:') || + url.startsWith('git+') || + url.startsWith('http:') || + url.startsWith('https:') || + url.startsWith('file:') + ) }app/components/Package/Dependencies.vue (1)
176-188: Consider cachinggetUrlDepInforesult to avoid repeated lookups.
getUrlDepInfo(dep)is called 3 times for the same dependency in this template block (lines 177, 179, 184). Each call iterates through theurlDependenciesarray.While the impact is minimal for small dependency lists, consider storing the result in a local variable pattern similar to how
getVulnerableDepInfoandgetDeprecatedDepInfoare used:♻️ Suggested improvement using a computed map
// Create a lookup map for URL dependencies const urlDepMap = computed(() => { if (!vulnTree.value) return new Map<string, UrlDependencyInfo>() return new Map(vulnTree.value.urlDependencies.map(p => [p.name, p])) }) function getUrlDepInfo(depName: string) { return urlDepMap.value.get(depName) ?? null }Alternatively, Vue's template compiler may optimise repeated calls with the same arguments, so this is a minor suggestion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6a59a39-1d5a-4a50-ab07-7a986c298ad8
📒 Files selected for processing (4)
app/components/Package/Dependencies.vueserver/utils/dependency-analysis.tsserver/utils/dependency-resolver.tsshared/types/dependency-analysis.ts
- Scan all packages for URL deps, not just root (fixes transitive detection) - Align isUrlDependency with resolveVersion URL patterns - Use computed map for URL dep lookups in Dependencies.vue - Remove orphaned url field from ResolvedPackage interface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the thorough review! I've addressed all feedback:
Updated in the latest commit. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/utils/dependency-analysis.ts (2)
287-293: Consider excludingdevDependenciesfrom transitive package scans.Including
devDependencieswhen scanning transitive packages may flag URL dependencies that are never actually installed for end users (since devDependencies of dependencies are not resolved into the tree). This could cause misleading warnings.For the root package, devDependencies are relevant, but for transitive packages they typically aren't installed.
💡 Suggested approach
async function scanUrlDependencies( name: string, version: string, depth: DependencyDepth, path: string[], ): Promise<UrlDependencyInfo[]> { try { const packument = await fetchNpmPackage(name) const versionData = packument.versions[version] if (!versionData) return [] const urlDeps: UrlDependencyInfo[] = [] - const allDeps = { - ...versionData.dependencies, - ...versionData.optionalDependencies, - ...versionData.devDependencies, - } + // Include devDependencies only for the root package + const allDeps = depth === 'root' + ? { + ...versionData.dependencies, + ...versionData.optionalDependencies, + ...versionData.devDependencies, + } + : { + ...versionData.dependencies, + ...versionData.optionalDependencies, + }
345-350: Consider parallelising URL dependency scanning for consistency.The scan currently processes packages sequentially, whereas OSV vulnerability queries use
mapWithConcurrency. SincefetchNpmPackagelikely caches packument data from the earlierresolveDependencyTreecall, the impact is minimal, but parallel processing would be more consistent with the rest of the codebase.♻️ Suggested refactor using mapWithConcurrency
// Scan for git: and https: URL dependencies in all packages - const urlDependencies: UrlDependencyInfo[] = [] - for (const pkg of packages) { - const pkgUrlDeps = await scanUrlDependencies(pkg.name, pkg.version, pkg.depth, pkg.path) - urlDependencies.push(...pkgUrlDeps) - } + const urlDepResults = await mapWithConcurrency( + packages, + pkg => scanUrlDependencies(pkg.name, pkg.version, pkg.depth, pkg.path), + OSV_DETAIL_CONCURRENCY, + ) + const urlDependencies = urlDepResults.flat()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96a2e5db-6be6-484f-8c91-d08d43f1c012
📒 Files selected for processing (2)
app/components/Package/Dependencies.vueserver/utils/dependency-analysis.ts
- Only include devDependencies when scanning the root package - Use mapWithConcurrency for consistent parallel processing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the additional feedback:
Updated in the latest commit. |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09702971-64e5-4e98-a9ea-41e853e5d7da
📒 Files selected for processing (1)
server/utils/dependency-analysis.ts
- Record child dependency depth instead of parent depth - Surface scan failures instead of silently returning empty array Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed both issues:
Updated in the latest commit. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/utils/dependency-analysis.ts (1)
316-320:⚠️ Potential issue | 🟠 MajorDon’t convert URL-scan failures into clean “no findings”.
Returning
[]here masks scan failures as absence of risky URL dependencies, creating false negatives in a security signal. Please propagate the failure (or surface an explicit scan-failed status in the result) instead of silently returning an empty list.Suggested fix
} catch (error) { // oxlint-disable-next-line no-console -- log URL dependency scan failures for debugging console.warn(`[dep-analysis] URL dependency scan failed for ${name}@${version}:`, error) - return [] + throw error } }As per coding guidelines, "Use error handling patterns consistently".
🧹 Nitpick comments (1)
server/utils/dependency-analysis.ts (1)
259-275: Update JSDoc to match actual behaviour and return type.The comments still describe only
git:/https:checks and say “returns a map”, but the implementation also matchesgit+,http:,file:and returnsUrlDependencyInfo[].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f05a63de-efe3-41d2-ba11-fb42c7f42ffb
📒 Files selected for processing (1)
server/utils/dependency-analysis.ts
| /** All dependencies using git: or https: URLs */ | ||
| urlDependencies: UrlDependencyInfo[] |
There was a problem hiding this comment.
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:
| /** All dependencies using git: or https: URLs */ | |
| urlDependencies: UrlDependencyInfo[] | |
| /** All packages using unsafe `git:`, `https:` (etc.) URLs in the tree */ | |
| packagesWithUrlVersion: UrlDependencyInfo[] |
| name: string | ||
| /** The git: or https: URL */ | ||
| url: string | ||
| /** Depth in dependency tree: root (0), direct (1), transitive (2+) */ |
There was a problem hiding this comment.
🤔 how could root ever happen? this is only a possibility in the dependencies array of another package, isn't it?
| <button | ||
| type="button" | ||
| class="p-2 -m-2" | ||
| :aria-label="`git/https dependency: ${getUrlDepInfo(dep)!.url}`" |
There was a problem hiding this comment.
This hardcoded English string needs to be internationalized. Check out guidance in CONTRIBUTING.md if you aren't familiar!
| url.startsWith('git:') || | ||
| url.startsWith('git+') || | ||
| url.startsWith('http:') || | ||
| url.startsWith('https:') || | ||
| url.startsWith('file:') |
There was a problem hiding this comment.
Per https://docs.npmjs.com/cli/v11/configuring-npm/package-json#git-urls-as-dependencies, these are also valid and insecure:
github:foo/bargist:foobitbucket:foo/bargitlab:foo/barfoo/bar(this means GitHub)
🤔 We can't just assume any protocol is insecure, as npm: is fine.
Maybe something like this works?
| 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(/^[^/]+/[^/]+$/) |
There was a problem hiding this comment.
oooh wait a sec, I think we can just do...
| 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.
There was a problem hiding this comment.
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
| const urlDepResults = await mapWithConcurrency( | ||
| packages, | ||
| pkg => scanUrlDependencies(pkg.name, pkg.version, pkg.depth, pkg.path), | ||
| OSV_DETAIL_CONCURRENCY, |
There was a problem hiding this comment.
We should create a separate constant for this, as it's unrelated to fetching OSV details
| path: string[], | ||
| ): Promise<UrlDependencyInfo[]> { | ||
| try { | ||
| const packument = await fetchNpmPackage(name) |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
If it helps simplify things, I think it would even be fine to just attach a new field for this in resolveDependencyTree directly.
| /** | ||
| * Check if a dependency URL is a git: or https: URL that should be flagged. | ||
| */ | ||
| function isUrlDependency(url: string): boolean { |
There was a problem hiding this comment.
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).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
serhalp
added
stale
|
@ndpvt-web hey there! are you interested in continuing to work on this PR? |
github-actions
bot
removed
the
stale
2 participants
Summary
Adds detection and visual flagging for dependencies that use
git:,https:, orgit+https:URLs instead of npm registry versions.These URL-based dependencies bypass npm registry integrity checks and can be manipulated, as noted in the linked discussion. This implementation helps users identify potential security concerns in their dependency trees.
Changes
isUrlDependency()utility function inserver/utils/dependency-analysis.tsto detect URL-based dependenciesscanUrlDependencies()function to scan dependency tree for URL-based depsshared/types/dependency-analysis.tswith TypeScript types for URL dependency trackingserver/utils/dependency-resolver.tsto include URL dependency scanning in analysisapp/components/Package/Dependencies.vueto display warning icons next to URL-based dependencies with tooltips explaining the security concernTesting
The implementation follows existing patterns in the codebase and integrates seamlessly with the current dependency analysis system.
Closes #1084
AI Disclosure
This PR was authored with the assistance of an AI coding assistant (Claude by Anthropic). All changes have been reviewed for correctness and follow the project's existing patterns and conventions.