Skip to content

refactor(registry): share installer verification#2009

Open
ascorbic wants to merge 3 commits into
feat/delegated-release-servicefrom
feat/delegated-release-service-21-installer-verification
Open

refactor(registry): share installer verification#2009
ascorbic wants to merge 3 commits into
feat/delegated-release-servicefrom
feat/delegated-release-service-21-installer-verification

Conversation

@ascorbic

Copy link
Copy Markdown
Collaborator

What does this PR do?

Moves installer checksum, archive, and manifest consistency checks onto the shared registry-verification primitives while preserving legacy checksum and error behavior. Adds a runtime-neutral checksum subpath so Worker installer paths do not load Node/Sigstore code.

Related: #1908

Type of change

  • Bug fix
  • Feature (requires maintainer-approved Discussion)
  • Refactor (no behavior change)
  • Translation
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm lint passes
  • pnpm test passes (or targeted tests for my change)
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • User-visible strings in the admin UI are wrapped for translation (if applicable). N/A, no admin UI strings.
  • I have added a changeset (if this PR changes a published package)
  • New features link to an approved Discussion: N/A, behavior-preserving refactor under Delegated release service #1908.

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: OpenCode with GPT-5.6 Sol and independent review agents

Screenshots / test output

Full monorepo build and package typecheck pass. Affected core tests: 65 passed. Registry verification: 93 Node and 36 workerd tests passed, including packed-output checks.

@changeset-bot

changeset-bot Bot commented Jul 13, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 19bcb4e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 19 packages
Name Type
emdash Patch
@emdash-cms/registry-verification Minor
@emdash-cms/cloudflare Patch
@emdash-cms/sandbox-workerd Patch
@emdash-cms/fixture-perf-site Patch
@emdash-cms/perf-demo-site Patch
@emdash-cms/cache-demo-site Patch
@emdash-cms/do-demo-site Patch
@emdash-cms/do-solo-demo-site Patch
@emdash-cms/plugin-cli Patch
@emdash-cms/release-verifier Patch
@emdash-cms/admin Patch
@emdash-cms/auth Patch
@emdash-cms/blocks Patch
@emdash-cms/gutenberg-to-portable-text Patch
@emdash-cms/x402 Patch
create-emdash Patch
@emdash-cms/auth-atproto Patch
@emdash-cms/plugin-embeds Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions Bot added the review/needs-review No maintainer or bot review yet label Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Scope check

This PR changes 530 lines across 9 files. Large PRs are harder to review and more likely to be closed without review.

If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs.

See CONTRIBUTING.md for contribution guidelines.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 13, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-playground 19bcb4e Jul 13 2026, 11:20 AM

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 13, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-demo-cache 19bcb4e Jul 13 2026, 11:20 AM

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 13, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-demo-do 19bcb4e Jul 13 2026, 11:20 AM

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good direction: moving installer checksum, archive, and manifest checks onto the shared @emdash-cms/registry-verification primitives is the right refactor, and adding a runtime-neutral ./checksum subpath keeps Worker installer bundles free of Node/Sigstore code. I read the full diff plus the changed source files, traced the install/update error paths in both marketplace and registry handlers, and checked against the existing checksum tests.

Two things need attention before merge:

  1. Changeset gap. The PR adds a new public ./checksum export subpath to @emdash-cms/registry-verification (changes to packages/registry-verification/package.json and tsdown.config.ts), but the included changeset only bumps emdash. That package is public (not in .changeset/config.json#ignore, access: "public"), so it needs its own changeset entry.

  2. Legacy error precedence is no longer preserved. The PR description says the refactor preserves "legacy checksum and error behavior." In practice the order of checks changed:

    • Marketplace install/update: MarketplaceClient.downloadBundle now validates manifest identity inside extractBundle before returning, so handleMarketplaceInstall/handleMarketplaceUpdate compare the marketplace checksum only after identity is known. Previously the handlers compared checksum first, then identity. When both mismatch, callers now get MANIFEST_* instead of CHECKSUM_MISMATCH.
    • Registry install/update: handleRegistryInstall/Update now run verifyChecksum before validateRegistryArtifactBundle. Previously the registry flow extracted the bundle and checked manifest.version / manifest.id before the checksum. When both mismatch, callers now get CHECKSUM_MISMATCH instead of MANIFEST_VERSION_MISMATCH / MANIFEST_ID_MISMATCH.
      If preserving the exact legacy precedence is intentional, the changeset/tests should document the new order. If not, the code should be reordered so the old precedence still holds.

Everything else looks solid: the shared checksum helper is covered by existing tests, the Worker packed-output check now guards checksum.js, and the marketplace handler tests were updated to match the new canonical checksum value.


Findings

  • [needs fixing] .changeset/installer-shared-verification.md:2

    This changeset only bumps emdash, but the PR also adds a new public ./checksum export subpath to @emdash-cms/registry-verification (see packages/registry-verification/package.json and tsdown.config.ts). That package is public and not in .changeset/config.json#ignore, so a published-package change needs a changeset entry too.

    ---
    "emdash": patch
    "@emdash-cms/registry-verification": minor
    ---
    
  • [needs fixing] packages/core/src/plugins/marketplace.ts:293-295

    downloadBundle now enforces manifest identity inside extractBundle before it returns. That means handleMarketplaceInstall / handleMarketplaceUpdate can only compare the marketplace checksum after identity has already been validated. The old flow compared checksum first, then identity, so when both mismatch callers used to receive CHECKSUM_MISMATCH and now receive MANIFEST_MISMATCH / MANIFEST_VERSION_MISMATCH. This contradicts the PR's stated goal of preserving legacy error behavior.

    To keep the old precedence, split download and validation: return raw bundle bytes (or a checksum computed from the bytes) from downloadBundle, verify the marketplace checksum in the handler first, and only then call extractBundle with expectedSlug / expectedVersion.

  • [needs fixing] packages/core/src/api/handlers/registry.ts:987-1000

    In handleRegistryInstall, validateRegistryArtifactBundle (which checks manifest.version and manifest.id) now runs after verifyChecksum. The original registry install flow extracted the bundle and checked version/identity before comparing the checksum, so when both mismatch callers previously got MANIFEST_VERSION_MISMATCH / MANIFEST_ID_MISMATCH and now get CHECKSUM_MISMATCH. That is the opposite of the marketplace change and also breaks the "preserving legacy error behavior" claim.

    If the legacy order is intentional, document it in the changeset/tests. Otherwise, move the bundle validation block above the checksum block so version/identity are checked first, as before.

    		// Step 5: extract and verify the bundle manifest against this release.
    		const bundleResult = await validateRegistryArtifactBundle(
    			artifactBytes,
    			slug,
    			version,
    			"install",
    		);
    		if (!bundleResult.success) return bundleResult;
    		const bundle = bundleResult.data;
    
    		// Step 6: verify the bytes against the signed record's checksum.
    		const checksumOk = await verifyChecksum(artifactBytes, declaredChecksum);
    		if (!checksumOk) {
    			return {
    				success: false,
    				error: {
    					code: "CHECKSUM_MISMATCH",
    					message:
    						"Artifact bytes do not match the release record's checksum, or the checksum encoding is unsupported.",
    				},
    			};
    		}
    

@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-review No maintainer or bot review yet labels Jul 13, 2026
@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/awaiting-author Reviewed; waiting on the author to respond labels Jul 13, 2026
@ascorbic

Copy link
Copy Markdown
Collaborator Author

Addressed the changeset finding in 19bcb4e by adding a minor release for @emdash-cms/registry-verification.

I checked both precedence findings against the PR parent, 475803a, and did not reorder either path:

  • Marketplace: parent MarketplaceClientImpl.downloadBundle() already called extractBundle(tarballBytes, { expectedSlug: id, expectedVersion: version }), so manifest identity/version validation occurred before the handler received bundle and compared its checksum. The now-removed handler validation was redundant and unreachable for mismatched identity/version.
  • Registry: parent handleRegistryInstall() and handleRegistryUpdate() both called verifyChecksum(artifactBytes, declaredChecksum) before extractBundle() and manifest identity/version checks. Current code preserves that checksum-first order.

Verification after the changeset fix: full build, package typecheck, lint 0, core affected tests 36/36, registry-verification Node 93/93 and workerd 36/36 including packed-output checks.

~ Sol 🤖

@ascorbic ascorbic added bot:review Trigger an emdashbot code review on this PR and removed bot:review Trigger an emdashbot code review on this PR labels Jul 13, 2026

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — all prior findings resolved or retracted.

This is a behavior-preserving refactor that moves installer checksum, archive, and manifest consistency checks onto shared @emdash-cms/registry-verification primitives, and adds a runtime-neutral ./checksum subpath so Worker installer paths don't pull in Node/Sigstore code. The approach is sound and fits EmDash's architecture.

Prior findings:

  1. Changeset gap — RESOLVED. The changeset (.changeset/installer-shared-verification.md) now correctly bumps both "emdash": patch and "@emdash-cms/registry-verification": minor. The registry-verification package is public and not in .changeset/config.json#ignore, so the new ./checksum export subpath requires its own bump.

  2. Marketplace error precedence — RETRACTED (was incorrect). I previously claimed downloadBundle "now" validates identity inside extractBundle before the handler's checksum comparison, reversing the old order. On re-inspection, downloadBundle's call extractBundle(tarballBytes, { expectedSlug: id, expectedVersion: version }) is not in the diff — it was unchanged by this PR. Identity was always validated inside downloadBundle before the handler's checksum comparison. The removed validateBundleIdentity was a redundant post-checksum identity check. Removing it has no observable effect.

  3. Registry error precedence — RETRACTED (was incorrect). I previously claimed verifyChecksum (Step 5) now runs before validateRegistryArtifactBundle (Step 6), reversing the old order. The verifyChecksum call is not in the diff — it was always Step 5, before the bundle extraction. The PR only refactored Step 6 (the extract + version/identity checks) into validateRegistryArtifactBundle; the checksum step was unchanged.

What I verified this round:

  • validateRegistryArtifactBundle correctly preserves the old version-before-identity precedence: when extractBundle throws MANIFEST_MISMATCH/MANIFEST_VERSION_MISMATCH, the catch block re-extracts without expectations and checks version first, then identity — matching the original handler logic for both install and update. The test expect(both.error?.code).toBe("MANIFEST_VERSION_MISMATCH") confirms this.
  • For the update operation, non-manifest errors are re-thrown (matching the old unguarded extractBundle call); for install, they're caught and returned as INVALID_BUNDLE (matching the old try-catch).
  • verifyChecksum's hex path (compute multihash → decode → parse expected from hex → compareDigestBytes) is equivalent to the old sha256Hex string comparison, including case-insensitive handling. The multihash path (verifyMultihash(bytes, checksum.toLowerCase())) preserves the old case-insensitive base32 comparison.
  • extractBundle's checksum computation (computeMultihashdecodeMultihash → digest → hex) produces the same value as the old crypto.subtle.digest("SHA-256", ...) path — confirmed by the updated test expect(bundle.checksum).toBe(createHash("sha256").update(gzipped).digest("hex")).
  • The ./checksum subpath only depends on ./errors.js (no Node/Sigstore imports). The packed-output check (check-packed-output.mjs) now also guards checksum.js for createRequire/@sigstore.
  • The tsdown.config.ts entry includes src/checksum.ts; package.json exports map ./checksum to dist/checksum.js + .d.ts.
  • No dangling references to the removed validateBundleIdentity or sha256Hex/sha256MultibaseMultihash helpers remain in source.
  • New test file artifact-verification.test.ts covers valid bundles, malformed/unsafe archives, size/entry limits, manifest rejection, install/update identity and version errors (including the both-mismatch precedence), legacy checksum compatibility, and enforced-access semantics.

The PR is clean. LGTM.

@github-actions github-actions Bot added review/approved Approved; no new commits since and removed review/needs-rereview Author pushed changes since the last review labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core bot:review Trigger an emdashbot code review on this PR cla: signed review/approved Approved; no new commits since size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant