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 publish-security downgrade detection and surfaces it in the UI, including two alert blocks on the package page. Introduces app/utils/publish-security.ts with detection logic and a PublishSecurityDowngrade interface. Extends package data flow (transformPackument and shared types) to carry per-version trust metadata (securityVersions, trustLevel, hasProvenance). Wires a computed trusted fallback (installVersionOverride) into Terminal/Install and useInstallCommand so generated install commands can prefer a trusted version. Adds translations and tests for downgrade detection and install-override behaviour. 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)
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.
| 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, | |
| } | |
| }) |
app/utils/publish-security.ts
Outdated
| function sortByRecency(a: VersionWithIndex, b: VersionWithIndex): number { | ||
| const aValid = Number.isFinite(a.timestamp) | ||
| const bValid = Number.isFinite(b.timestamp) | ||
|
|
||
| if (aValid && bValid && a.timestamp !== b.timestamp) { | ||
| return b.timestamp - a.timestamp | ||
| } | ||
|
|
||
| if (aValid !== bValid) { | ||
| return aValid ? -1 : 1 | ||
| } | ||
|
|
||
| const semverOrder = compare(b.version, a.version) | ||
| if (semverOrder !== 0) return semverOrder | ||
|
|
||
| return a.index - b.index | ||
| } |
There was a problem hiding this comment.
semver.compare can throw on invalid version strings.
compare(b.version, a.version) on line 45 throws a TypeError if either string is not valid semver. While npm versions are usually valid, edge cases exist (e.g. very old packages, yanked/corrupted metadata). A single bad version in the array would crash the entire sort and, by extension, the page render.
🛡️ Proposed defensive fix
+import { compare, valid } from 'semver'
function sortByRecency(a: VersionWithIndex, b: VersionWithIndex): number {
const aValid = Number.isFinite(a.timestamp)
const bValid = Number.isFinite(b.timestamp)
if (aValid && bValid && a.timestamp !== b.timestamp) {
return b.timestamp - a.timestamp
}
if (aValid !== bValid) {
return aValid ? -1 : 1
}
- const semverOrder = compare(b.version, a.version)
- if (semverOrder !== 0) return semverOrder
+ if (valid(a.version) && valid(b.version)) {
+ const semverOrder = compare(b.version, a.version)
+ if (semverOrder !== 0) return semverOrder
+ }
return a.index - b.index
}
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, |
2 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