feat: detect npm publish security downgrade#1053
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds detection and UI for npm publish security downgrades and a way to prefer a previously trusted version when installing. Key changes: new publish-security utility with downgrade-detection API; package transform and shared types extended to include per-version trust metadata and optional securityVersions; Install component and useInstallCommand updated to accept an installVersionOverride which takes precedence over requestedVersion; package page shows downgrade alerts and supplies the override to install UI; i18n/schema entries and tests added for the new behaviours. Possibly related PRs
Suggested reviewers
🚥 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)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements a security downgrade detection feature that warns users when an npm package version was published without provenance after previous versions had provenance. This addresses supply-chain security concerns by alerting users to potential compromises.
Changes:
- Added security downgrade detection utilities that compare provenance status across package versions
- Implemented a prominent warning banner in the package installation UI when a downgrade is detected
- Modified install commands to default to the last trusted version when viewing a downgraded version
- Added comprehensive test coverage for the detection logic
- Added i18n strings in English locales for the security warning messages
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/utils/publish-security.ts |
Core detection logic for identifying security downgrades by comparing provenance across versions |
test/unit/app/utils/publish-security.spec.ts |
Comprehensive unit tests covering main scenarios and edge cases |
app/pages/package/[...package].vue |
Integration of downgrade detection with warning banner and version override logic |
app/composables/useInstallCommand.ts |
Added optional installVersionOverride parameter to support pinning to trusted versions |
app/components/Terminal/Install.vue |
Passes through installVersionOverride prop to support version pinning |
test/nuxt/composables/use-install-command.spec.ts |
Test coverage for the version override functionality |
i18n/locales/en.json, lunaria/files/en-US.json, lunaria/files/en-GB.json |
Warning message strings for the security banner |
![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 (2)
test/unit/app/utils/publish-security.spec.ts (1)
7-63: LGTM! Core scenarios are well covered.The tests verify the three primary outcomes: downgrade detected, latest is trusted, and no trusted history.
Consider adding edge case tests for robustness:
- Empty array input (should return
null)- Single version input (should return
null)- Versions with missing/invalid
timefields (to verify timestamp fallback logic)📝 Example edge case tests
it('returns null for empty versions array', () => { const result = detectPublishSecurityDowngrade([]) expect(result).toBeNull() }) it('returns null for single version', () => { const result = detectPublishSecurityDowngrade([ { version: '1.0.0', time: '2026-01-01T00:00:00.000Z', hasProvenance: false }, ]) expect(result).toBeNull() }) it('handles versions with missing timestamps', () => { const result = detectPublishSecurityDowngrade([ { version: '1.0.0', hasProvenance: true }, { version: '1.0.1', hasProvenance: false }, ]) // Should still detect downgrade using semver ordering expect(result).toEqual({ downgradedVersion: '1.0.1', downgradedPublishedAt: undefined, trustedVersion: '1.0.0', trustedPublishedAt: undefined, }) })app/composables/useInstallCommand.ts (1)
34-44: Minor duplication of version resolution logic.The version resolution pattern (
toValue(installVersionOverride) ?? toValue(requestedVersion)) is repeated in bothinstallCommandPartsandinstallCommand. This is acceptable for clarity, but if this pattern grows in usage, consider extracting it to a computed ref.♻️ Optional refactor to reduce duplication
+ const resolvedVersion = computed(() => + toValue(installVersionOverride) ?? toValue(requestedVersion) + ) + const installCommandParts = computed(() => { const name = toValue(packageName) if (!name) return [] - const version = toValue(installVersionOverride) ?? toValue(requestedVersion) return getInstallCommandParts({ packageName: name, packageManager: selectedPM.value, - version, + version: resolvedVersion.value, jsrInfo: toValue(jsrInfo), }) }) const installCommand = computed(() => { const name = toValue(packageName) if (!name) return '' - const version = toValue(installVersionOverride) ?? toValue(requestedVersion) return getInstallCommand({ packageName: name, packageManager: selectedPM.value, - version, + version: resolvedVersion.value, jsrInfo: toValue(jsrInfo), }) })
ghostdevv
left a comment
ghostdevv
left a comment
There was a problem hiding this comment.
Perhaps it should also include trusted publisher, both e18e and pnpm have logic like that
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/composables/npm/usePackage.ts (1)
87-96:⚠️ Potential issue | 🟠 MajorSame
hasProvenancesemantic issue in filtered versions.Line 87 uses
hasPublishTrustEvidence(version)which includes trusted-publisher evidence, again conflating it with provenance for thehasProvenancefield onSlimVersion.🐛 Proposed fix — use attestation-only check for hasProvenance
- const hasProvenance = hasPublishTrustEvidence(version) + const provenance = hasAttestations(version) const trustLevel = getTrustLevel(version) filteredVersions[v] = { - hasProvenance, + hasProvenance: provenance, trustLevel,
🧹 Nitpick comments (3)
app/composables/npm/usePackage.ts (1)
59-68:securityVersionsincludes every version — potential payload bloat for large packages.Unlike
filteredVersionswhich is pruned to ~5 recent + dist-tags,securityVersionsmaps over allpkg.versions. Popular packages can have hundreds or thousands of versions, and this array is shipped to every client in the initial SSR payload.Consider whether the downgrade detection logic could run server-side and only ship the result (or a trimmed window of versions) to the client.
app/utils/publish-security.ts (1)
55-87:detectPublishSecurityDowngradedoes not skip deprecated versions.A deprecated version that happens to be the newest by timestamp will be flagged as the "downgraded" version. Users seeing a deprecation notice and a security downgrade banner simultaneously may be confusing. Consider whether deprecated versions should be excluded from the "latest" candidate or documented as intentional.
app/pages/package/[...package].vue (1)
148-159: Fallback path derives security metadata from filtered versions only — downgrade detection may miss evidence.When
securityVersionsis not populated (e.g. client-side rehydration edge cases), the fallback on line 152 iteratespkg.value.versions, which only contains ~5 recent versions + dist-tag versions. If the trusted older version falls outside that window,detectPublishSecurityDowngradeForVersionwill returnnulland the warning won't appear.This is a graceful degradation (no false positive), but it's worth documenting or logging when the fallback is hit so it's clear why the banner might not show in certain scenarios.
app/composables/npm/usePackage.ts
Outdated
| const securityVersions = Object.entries(pkg.versions).map(([version, metadata]) => { | ||
| const trustLevel = getTrustLevel(metadata) | ||
| return { | ||
| version, | ||
| time: pkg.time[version], | ||
| hasProvenance: trustLevel !== 'none', | ||
| trustLevel, | ||
| deprecated: metadata.deprecated, | ||
| } | ||
| }) |
There was a problem hiding this comment.
hasProvenance conflates trusted-publisher evidence with attestation provenance.
On line 64, hasProvenance: trustLevel !== 'none' means a version published via a trusted publisher (OIDC) but without SLSA attestations is marked as hasProvenance: true. This is semantically incorrect — "provenance" specifically refers to verifiable build attestations, not publisher identity trust. The downstream getTrustRank fallback in publish-security.ts (line 25: return version.hasProvenance ? TRUST_RANK.provenance : TRUST_RANK.none) would then incorrectly elevate a trustedPublisher version to provenance rank when trustLevel is absent.
Consider using the attestation-only check for hasProvenance and relying on trustLevel for the broader trust signal:
🐛 Proposed fix
return {
version,
time: pkg.time[version],
- hasProvenance: trustLevel !== 'none',
+ hasProvenance: hasAttestations(metadata),
trustLevel,
deprecated: metadata.deprecated,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const securityVersions = Object.entries(pkg.versions).map(([version, metadata]) => { | |
| const trustLevel = getTrustLevel(metadata) | |
| return { | |
| version, | |
| time: pkg.time[version], | |
| hasProvenance: trustLevel !== 'none', | |
| trustLevel, | |
| deprecated: metadata.deprecated, | |
| } | |
| }) | |
| const securityVersions = Object.entries(pkg.versions).map(([version, metadata]) => { | |
| const trustLevel = getTrustLevel(metadata) | |
| return { | |
| version, | |
| time: pkg.time[version], | |
| hasProvenance: hasAttestations(metadata), | |
| trustLevel, | |
| deprecated: metadata.deprecated, | |
| } | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/pages/package/[...package].vue (1)
148-159: Fallback derivation looks sound, but the!!coercion onhasProvenanceis redundant.On
SlimVersion,hasProvenanceis already aboolean(set inusePackage.tsline 91/94). The double-negation!!metadata.hasProvenanceon line 155 is harmless but unnecessary.Otherwise, the fallback logic for older cached payloads that lack
securityVersionsis a nice defensive pattern.Nitpick: remove redundant coercion
return Object.entries(pkg.value.versions).map(([version, metadata]) => ({ version, time: pkg.value?.time?.[version], - hasProvenance: !!metadata.hasProvenance, + hasProvenance: metadata.hasProvenance, trustLevel: metadata.trustLevel, deprecated: metadata.deprecated, }))
| const trustLevel = getTrustLevel(version) | ||
| const hasProvenance = trustLevel !== 'none' | ||
|
|
||
| filteredVersions[v] = { | ||
| ...((version?.dist as { attestations?: unknown }) ? { hasProvenance: true } : {}), | ||
| hasProvenance, | ||
| trustLevel, |
There was a problem hiding this comment.
Same hasProvenance conflation applies here in the filtered versions.
Lines 90–91 replicate the same trustLevel !== 'none' → hasProvenance pattern flagged above. This should use hasAttestations(version) for consistency and correctness.
Proposed fix
- const trustLevel = getTrustLevel(version)
- const hasProvenance = trustLevel !== 'none'
+ const trustLevel = getTrustLevel(version)
+ const hasProvenance = hasAttestations(version)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const trustLevel = getTrustLevel(version) | |
| const hasProvenance = trustLevel !== 'none' | |
| filteredVersions[v] = { | |
| ...((version?.dist as { attestations?: unknown }) ? { hasProvenance: true } : {}), | |
| hasProvenance, | |
| trustLevel, | |
| const trustLevel = getTrustLevel(version) | |
| const hasProvenance = hasAttestations(version) | |
| filteredVersions[v] = { | |
| hasProvenance, | |
| trustLevel, |
# Conflicts: # app/composables/npm/usePackage.ts # app/pages/package/[[org]]/[name].vue
|
updated wording 
|
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
3 participants
Closes #534
Example:
This is a perfect example - an older version was indeed "downgraded":
https://npmxdev-git-fork-wojtekmaj-downgrade-detection-poetry.vercel.app/package/ts-api-utils/v/2.1.1
But the latest got an "upgrade" again:
https://npmxdev-git-fork-wojtekmaj-downgrade-detection-poetry.vercel.app/package/ts-api-utils/v/2.4.0