Use SHA-512 integrity in package locks - #14701
Use SHA-512 integrity in package locks#14701Sean McManus (sean-mcmanus) wants to merge 5 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Upgrades dependency integrity and enforces reproducible dependency installation.
Changes:
- Migrates lockfile integrity values to SHA-512.
- Adds lockfile validation, updater tooling, and tests.
- Replaces mutable npm installs with npm ci.
Reviewed changes
Copilot reviewed 29 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file| File | Description |
|---|---|
| Themes/package-lock.json | Migrates integrity hashes to SHA-512. |
| Extension/readme.developer.md | Documents verified Yarn bootstrap. |
| Extension/package.json | Adds lockfile scripts and bootstrap changes. |
| Extension/.yarn-bootstrap/package-lock.json | Upgrades Yarn integrity to SHA-512. |
| Extension/.scripts/verifyYarnLock.test.mjs | Tests lockfile integrity validation. |
| Extension/.scripts/verifyYarnLock.mjs | Validates Yarn and npm lockfiles. |
| Extension/.scripts/updatePackageLockIntegrity.test.mjs | Tests updater and atomic writes. |
| Extension/.scripts/updatePackageLockIntegrity.mjs | Adds byte-verifying lockfile updater. |
| Extension/.scripts/subresourceIntegrity.mjs | Parses and computes integrity values. |
| Extension/.scripts/packageLockFiles.mjs | Discovers repository package locks. |
| Build/package/jobs_package_vsix.yml | Uses frozen npm installation. |
| .github/workflows/question-closer.yml | Uses npm ci. |
| .github/workflows/question-closer-debugger.yml | Uses npm ci. |
| .github/workflows/more-info-needed-closer.yml | Uses npm ci. |
| .github/workflows/more-info-needed-closer-debugger.yml | Uses npm ci. |
| .github/workflows/locker.yml | Uses npm ci. |
| .github/workflows/job-compile-and-test.yml | Runs repository lock validation. |
| .github/workflows/investigate-costing-closer-debugger.yml | Uses npm ci. |
| .github/workflows/investigate-closer-debugger.yml | Uses npm ci. |
| .github/workflows/feature-request-reopener.yml | Uses npm ci. |
| .github/workflows/feature-request-debugger.yml | Uses npm ci. |
| .github/workflows/feature-request-closer-triage.yml | Uses npm ci. |
| .github/workflows/feature-request-closer-no-milestone.yml | Uses npm ci. |
| .github/workflows/external-closer-debugger.yml | Uses npm ci. |
| .github/workflows/enhancement-reopener.yml | Uses npm ci. |
| .github/workflows/enhancement-closer-triage.yml | Uses npm ci. |
| .github/workflows/enhancement-closer-no-milestone.yml | Uses npm ci. |
| .github/workflows/duplicate-closer.yml | Uses npm ci. |
| .github/workflows/by-design-closer.yml | Uses npm ci. |
| .github/workflows/by-design-closer-debugger.yml | Uses npm ci. |
| .github/workflows/bug-debugger.yml | Uses npm ci. |
- Extension/.yarn-bootstrap/package-lock.json: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 34 changed files in this pull request and generated 2 comments.
Files not reviewed (1)- Extension/.yarn-bootstrap/package-lock.json: Generated file
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 34 changed files in this pull request and generated 1 comment.
Files not reviewed (1)- Extension/.yarn-bootstrap/package-lock.json: Generated file
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 34 changed files in this pull request and generated no new comments.
Files not reviewed (1)- Extension/.yarn-bootstrap/package-lock.json: Generated file
Sorry, something went wrong.
Sean McManus (sean-mcmanus)
left a comment
There was a problem hiding this comment.
✨Copilot (agent102): Reviewed at 493b432e merged onto main 92768bc0. The merge base is the current target head, so the merged result is byte-identical to the PR head.
Independent validation performed locally:
- Every changed line in Extension/yarn.lock, Extension/.yarn-bootstrap/package-lock.json, ExtensionPack/package-lock.json, Themes/package-lock.json, and .github/actions/package-lock.json is an integrity value; no version, resolution, or dependency-graph drift.
- 35 sampled SHA-512 values (npm locks and yarn entries) match the dist.integrity published by the upstream registry exactly.
- yarn 1.22.22's bundled ssri selects the strongest listed algorithm, so the retained sha1-… sha512-… pairs are verified against SHA-512, not SHA-1.
- npm ci --omit=dev in .github/actions (85 packages), npm ci in Themes (283 packages), and the new npm ci --prefix .yarn-bootstrap + offline global install all succeed and yield yarn 1.22.22; ExtensionPack's non-root package set is identical to Themes. These paths are not covered by PR CI, so they were exercised directly.
- verify-lockfiles passes and all 31 tests pass.
Only two nits below; nothing blocking from my side.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 34 changed files in this pull request and generated no new comments.
Files not reviewed (1)- Extension/.yarn-bootstrap/package-lock.json: Generated file
Extension/.scripts/packageLockFiles.mjs:88
- This exemption accepts any non-link file:, link:, or workspace: value without validating its target. For example, a non-root entry with resolved: "file:../../outside" and no integrity passes both the verifier and updater, even though the analogous link: true traversal case is rejected above. That lets an npm lock consume bytes outside the reviewed repository without a digest. Resolve these targets relative to the lockfile and exempt only normalized, repository-contained local dependencies (or require integrity for file tarballs).
|| (packageEntry.link !== true && typeof packageEntry.resolved === 'string' && /^(?:file|link|workspace):/i.test(packageEntry.resolved))
Extension/.scripts/verifyYarnLock.mjs:94
- A normal registry selector is exempted whenever its mutable resolved field starts with file:, link:, or workspace:. Thus dependency@1.0.0 can be redirected to an unhashed local path and still pass validation; the dependency range itself does not authorize a local source. Exempt missing integrity only when hasExplicitLocalSelector(currentEntry.selector) confirms that every selector declares the local protocol.
if (currentEntry && currentEntry.integrity === undefined
&& !isExplicitLocalResolution(currentEntry.resolved)
&& !hasExplicitLocalSelector(currentEntry.selector)) {
Sorry, something went wrong.
Sean McManus (sean-mcmanus)
left a comment
There was a problem hiding this comment.
✨Copilot (agent102): Re-checked at 62cd4633. Both of my earlier nits are fixed and independently verified: the removed test-yarn-lock/verify-yarn-lock aliases have no remaining consumer, and validatePackageLocks now prefixes malformed-JSON, missing-packages, and ENOENT failures with the offending path while preserving error.cause. 32/32 tests pass and verify-lockfiles exits 0 at this head.
The automated review at this head reported no new threads but listed two suppressed low-confidence observations. I verified both against this revision and they reproduce, so I have consolidated them into the single comment below — they share one root cause across packageLockFiles.mjs and verifyYarnLock.mjs.
Sorry, something went wrong.
| const normalizedPackagePath = packagePath.replaceAll('\\', '/'); | ||
| return workspacePaths.has(getWorkspaceTargetPath(packageEntry)) | ||
| || workspacePaths.has(normalizedPackagePath) | ||
| || (packageEntry.link !== true && typeof packageEntry.resolved === 'string' && /^(?:file|link|workspace):/i.test(packageEntry.resolved)) |
There was a problem hiding this comment.
✨Copilot (agent102): [Minor] This exemption treats a lockfile-controlled resolved prefix as authorization to skip integrity, without validating the target — and the same pattern exists on the Yarn side at verifyYarnLock.mjs line 93 (!isExplicitLocalResolution(currentEntry.resolved)). Both were raised as suppressed low-confidence observations in the automated review at this head; I verified them against this revision rather than taking them as given.
Calling the exported validators directly at 62cd4633:
| Fixture (no integrity) | Result |
|---|---|
| npm node_modules/evil with resolved: "file:../../outside/evil.tgz" | accepted |
| npm node_modules/evil with resolved: "link:/etc/evil" | accepted |
| npm node_modules/evil with resolved: "workspace:../../outside" | accepted |
| npm node_modules/evil with link: true, resolved: "../../outside" | rejected |
| npm node_modules/evil with resolved: "https://example.test/evil.tgz" | rejected |
| yarn dependency@1.0.0 with resolved "file:../../outside/dep.tgz" | accepted |
| yarn dependency@1.0.0 with resolved "link:/etc/evil" | accepted |
| yarn dependency@1.0.0 with resolved "https://example.test/dep.tgz" | rejected |
So the link: true path that was hardened earlier in this PR rejects traversal, absolute, and protocol targets, while its non-link sibling accepts an arbitrary string after the protocol prefix. On the Yarn side an ordinary registry selector is exempted purely because of its resolved line, which does not match the earlier statement that "only selectors whose actual dependency range uses an explicit file:, link:, or workspace: protocol are exempt" — the condition is an OR of the selector check and the resolution check.
Impact is bounded: no entry in any current lockfile lacks integrity, so nothing relies on this exemption today, and it only matters for a future lockfile change. But since the stated goal is repository-wide integrity enforcement, it is worth closing so the validator cannot be opted out of by editing the lockfile it is validating.
Minimal resolution: apply the same target rules the link: true path already uses — resolve the local target relative to the lockfile and exempt only normalized, repository-contained paths — or, for Yarn, rely on hasExplicitLocalSelector alone. Because every current entry carries integrity, either change is inert against the lockfiles in this PR.
Sorry, something went wrong.
|
It looks like this is not actually necessary. |
Sorry, something went wrong.
Summary
Upgrade dependency integrity from SHA-1-only values to SHA-512 across the Yarn and npm lockfiles. Add repository-wide validation and a byte-verifying npm lock updater, and make npm consumers use frozen installs so builds consume the reviewed dependency graph.
The updater fetches exact tarballs from the configured approved registry, verifies each existing SHA-1 value against the downloaded bytes, computes SHA-512 independently, and protects lockfile replacement against partial or concurrent writes.
Validation