feat: flag git: and https: dependencies as security concern#2017
feat: flag git: and https: dependencies as security concern#2017ndpvt-web wants to merge 4 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.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds URL-based dependency detection and reporting. A new UrlDependencyInfo type (name, url, depth, path) is introduced. analyzeDependencyTree now scans for git:/https: dependencies, aggregates urlDependencies: UrlDependencyInfo[], and uses a v3 cache key. The Vue component exposes a computed urlDepMap and getUrlDepInfo, prefers URL data for tooltips, applies an orange class for URL dependencies, and renders a TooltipApp block with an ARIA-labelled alert button for URL entries. Existing vulnerability and deprecation indicators are unchanged. 🚥 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)
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. |
1 participant
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.