fix: bind Sigstore provenance to admitted workload - #2847
Conversation
🦋 Changeset detectedLatest commit: 8e85dda The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
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 |
Scope checkThis PR changes 624 lines across 14 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. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right change for the stated security gap. It binds Sigstore provenance to the admitted workload by threading verified repository, workflow, commit, and invocation metadata from the registry verifier through the release verifier into the release service, where it is checked against the canonicalized stored workload identity both during initial verification and again immediately before create (TOCTOU protection on policy changes). The architecture fits EmDash’s existing policy/evaluation split, and the regression coverage is broad.
I checked the changed files, the workload identity parser, the release-action/e2e fixtures, and the workload-policy semantics. The implementation is mostly solid, but one logic bug in the ref binding check conflates two distinct GitHub refs and would reject legitimate releases where the workflow file ref and the triggering run ref differ—a configuration the rest of the codebase explicitly supports (e.g. workflowRef on refs/heads/main and ref on refs/tags/v1.2.3). There are also two smaller robustness/formatter items.
Headline: merge blocked on fixing the workflow/run-ref comparison in evaluateWorkloadAttestation; the other two items are defensive cleanups.
| return { ok: false, reasonCode: "ATTESTED_WORKFLOW_MISMATCH" }; | ||
| } | ||
| const workflowRef = identity.workflow.ref.slice(identity.workflow.ref.lastIndexOf("@") + 1); | ||
| if (provenance.workflowRef !== workflowRef || provenance.workflowRef !== identity.run.ref) { |
There was a problem hiding this comment.
[needs fixing] The ref check conflates the workflow file ref with the run ref. provenance.workflowRef is the verified workflow file ref (externalParameters.workflow.ref in SLSA), while identity.run.ref is the ref that triggered the run (github.ref). The codebase supports them being different: connection/request fixtures use workflowRef on refs/heads/main and ref on refs/tags/v1.2.3, and evaluateWorkloadPolicy checks them independently. Since the attestation does not carry the triggering ref, the binding should only verify that the attested workflow file ref matches the ref portion of identity.workflow.ref.
| if (provenance.workflowRef !== workflowRef || provenance.workflowRef !== identity.run.ref) { | |
| if (provenance.workflowRef !== workflowRef) { | |
| return { ok: false, reasonCode: "ATTESTED_REF_MISMATCH" }; | |
| } |
Remove || provenance.workflowRef !== identity.run.ref.
| } | ||
| const workflowMarker = "/.github/workflows/"; | ||
| const markerIndex = identity.workflow.ref.toLowerCase().indexOf(workflowMarker); | ||
| const expectedBuilderId = `${provenance.sourceRepository}${identity.workflow.ref.slice(markerIndex)}`; |
There was a problem hiding this comment.
[suggestion] Use the normalized sourceRepository already computed on line 376 instead of raw provenance.sourceRepository. The repository check is case-insensitive, so if a verifier returned a non-canonical-cased source repository, expectedBuilderId would be built with that mixed casing while a canonicalized builderId would be lowercase, causing a false ATTESTED_WORKFLOW_MISMATCH even though the repository matches.
| const expectedBuilderId = `${provenance.sourceRepository}${identity.workflow.ref.slice(markerIndex)}`; | |
| const expectedBuilderId = `${sourceRepository}${identity.workflow.ref.slice(markerIndex)}`; |
| if (provenance.commitSha !== identity.run.commitSha) { | ||
| return { ok: false, reasonCode: "ATTESTED_COMMIT_MISMATCH" }; | ||
| } | ||
| const invocationId = `${provenance.sourceRepository}/actions/runs/${identity.run.id}/attempts/${identity.run.attempt}`; |
There was a problem hiding this comment.
[suggestion] Same canonicalization point as the builderId line: construct invocationId from the normalized sourceRepository so the comparison stays consistent with the repository binding established above.
| const invocationId = `${provenance.sourceRepository}/actions/runs/${identity.run.id}/attempts/${identity.run.attempt}`; | |
| const invocationId = `${sourceRepository}/actions/runs/${identity.run.id}/attempts/${identity.run.attempt}`; |
| predicateType: input.provenance.predicateType, | ||
| sourceRepository: input.provenance.sourceRepository, | ||
| builderId: input.provenance.builderId, | ||
| repositoryId: "123456789", | ||
| workflowRef: "refs/heads/main", | ||
| commitSha: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", | ||
| invocationId: "https://github.com/example/gallery/actions/runs/100/attempts/1", | ||
| }, |
There was a problem hiding this comment.
[suggestion] The embedded Worker script added lines 46–52 at a different indentation level than the surrounding object keys (8 tabs vs. 14 tabs). It is syntactically valid inside the template literal, but the misalignment makes the structure easy to misread and edit. Please re-indent the new provenance fields to match the surrounding keys before merging.
github-actions
Bot
added
cla: signed
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is the right change for the stated security gap. The prior blocking bug—the conflation of the workflow-file ref with the triggering run ref in evaluateWorkloadAttestation—is fixed: the attested ref is now extracted from identity.workflow.ref (the workflow file ref), while the policy's allowedRefs still governs identity.run.ref (the trigger ref). The rest of the implementation is sound: the registry provenance verifier binds repository, workflow, commit, and invocation identity; the release verifier guards against missing fields; the release service parses, re-digests, and canonicalizes the stored workload identity; and the publication workflow now re-checks the active workload policy immediately before entering the create phase, closing the TOCTOU window.
I checked the changed files, the new stored-identity parser, the attestation binding logic, the registry provenance contract test, the release-verifier field guard, and the integration test covering a policy change between final verification and create. The regression coverage matches the PR description.
The remaining items are tiny test-clarity cleanups, not blockers. The changeset is correct to list only @emdash-cms/registry-verification; the two affected apps are private and do not need entries.
| sourceRepository: PROVENANCE.sourceRepository, | ||
| builderId: PROVENANCE.builderId, | ||
| repositoryId: WORKLOAD_IDENTITY.repository.id, | ||
| workflowRef: WORKLOAD_IDENTITY.run.ref, |
There was a problem hiding this comment.
[suggestion] The verifierReport() helper uses WORKLOAD_IDENTITY.run.ref as the attested workflow ref. This happens to equal the workflow file ref (refs/heads/main) in the default fixture, but the field semantically represents the workflow file ref extracted from identity.workflow.ref. Derive it from there so the fixture stays correct if a future test changes the triggering run ref independently.
| workflowRef: WORKLOAD_IDENTITY.run.ref, | |
| workflowRef: WORKLOAD_IDENTITY.workflow.ref.slice( | |
| WORKLOAD_IDENTITY.workflow.ref.lastIndexOf("@") + 1, | |
| ), |
| ).resolves.toMatchObject({ success: true }); | ||
| }); | ||
|
|
||
| it("uses canonical repository casing for builder and invocation identity", async () => { |
There was a problem hiding this comment.
[suggestion] The test title claims it checks canonical casing for the builder and invocation identity, but the body only mutates provenance.sourceRepository and relies on the repository casing tolerance in evaluateWorkloadAttestation. Rename it to describe what it actually verifies.
| it("uses canonical repository casing for builder and invocation identity", async () => { | |
| it("uses canonical repository casing for the attested source repository", async () => { |
There was a problem hiding this comment.
This re-review confirms the fix remains sound. The prior blocking bug—the conflation of the attested workflow-file ref with the triggering run ref in evaluateWorkloadAttestation—is still correctly resolved: provenance.workflowRef is matched against the @ref suffix of identity.workflow.ref (the workflow file ref), while evaluateWorkloadPolicy independently governs identity.run.ref (the trigger ref) against allowedRefs.
I re-read the diff and the changed files in full, traced the new parseStoredWorkloadIdentity parser against the github-oidc normalizer that produces the stored JSON, and checked the re-check path in the publication workflow. Nothing new jumped out:
- The registry provenance verifier now binds and verifies
repositoryId,workflowRef,commitSha, andinvocationIdfrom the Sigstore certificate OIDs / predicate, returning the narrowerVerifiedGitHubProvenancetype compatibly. - The release verifier guards against missing new fields before propagating them.
parseStoredWorkloadIdentityre-parses, validates, canonicalizes, and re-digests the stored workload identity; the canonical check matches the JSON produced by the existing OIDC normalizer.evaluateWorkloadAttestationbinds the verified provenance identity back to the parsed stored identity and re-evaluates the active workload policy.publishVerifiedIntentrepeats that binding/policy check immediately before advancing to thecreatingphase, closing the TOCTOU window.- Test coverage matches the PR description: substituted IDs/refs/commits/identities, malformed/non-canonical stored identities, digest mismatches, incomplete verifier reports, and a policy change between final verification and create are all exercised.
No new logic bugs, regressions, or convention violations. Approving again.
github-actions
Bot
added
review/approved
There was a problem hiding this comment.
This is the right change for the reported security gap: binding the Sigstore attestation to the stored workload identity and re-evaluating the policy immediately before the create phase closes a real TOCTOU window. The structure—verifier returns a richer provenance identity, the service canonicalizes and re-digests the stored identity, and both initial evaluation and the pre-create check use the same binding function—is sound.
I re-read the diff and traced the flow through the github-oidc normalizer, both stored-identity parsers, the workload policy evaluator, and the publication workflow. The regression coverage matches the PR description, the changeset correctly covers the only published package (@emdash-cms/registry-verification), and no AGENTS.md conventions for this code area are violated.
New finding not caught in the prior review: evaluateWorkloadAttestation lowercases the repository prefix when reconstructing expectedBuilderId and invocationId (it takes the repository name from identity.repository.name, which github-oidc.ts lowercases). GitHub's Fulcio builder ID and invocation URI preserve the original repository casing from workflow_ref, so mixed-case repositories will pass admission-time policy matching but fail attestation with ATTESTED_WORKFLOW_MISMATCH or ATTESTED_INVOCATION_MISMATCH. The fix is to derive the repo prefix from identity.workflow.ref (which the parser already validates starts with the repository) and compare source repositories case-insensitively. A regression test with a mixed-case provenance builderId/invocationId should be added alongside the fix.
| if (!policyDecision.ok) return { ok: false, reasonCode: policyDecision.code }; | ||
| const sourceRepository = `https://github.com/${identity.repository.name}`; | ||
| if ( | ||
| provenance.repositoryId !== identity.repository.id || | ||
| provenance.sourceRepository.toLowerCase() !== sourceRepository | ||
| ) { | ||
| return { ok: false, reasonCode: "ATTESTED_REPOSITORY_MISMATCH" }; | ||
| } |
There was a problem hiding this comment.
[needs fixing] sourceRepository is built from identity.repository.name, which github-oidc.ts lowercases when normalizing the OIDC token. The Sigstore-attested builderId and OID 21 invocationId, however, preserve the original casing from the GitHub workflow_ref claim. Repositories with uppercase letters therefore pass admission-time policy matching (which normalizes case) but fail here with ATTESTED_WORKFLOW_MISMATCH or ATTESTED_INVOCATION_MISMATCH because expectedBuilderId and invocationId use a lowercased prefix.
Derive the repo prefix from identity.workflow.ref — the parser already validates that the ref starts with the repository — and compare source repositories case-insensitively. Add a regression test that upper-cases the provenance builderId/invocationId (or the stored workflow ref).
| if (!policyDecision.ok) return { ok: false, reasonCode: policyDecision.code }; | |
| const sourceRepository = `https://github.com/${identity.repository.name}`; | |
| if ( | |
| provenance.repositoryId !== identity.repository.id || | |
| provenance.sourceRepository.toLowerCase() !== sourceRepository | |
| ) { | |
| return { ok: false, reasonCode: "ATTESTED_REPOSITORY_MISMATCH" }; | |
| } | |
| const workflowMarker = "/.github/workflows/"; | |
| const markerIndex = identity.workflow.ref.toLowerCase().indexOf(workflowMarker); | |
| if (markerIndex < 1) { | |
| return { ok: false, reasonCode: "ATTESTED_WORKFLOW_MISMATCH" }; | |
| } | |
| const sourceRepository = `https://github.com/${identity.workflow.ref.slice(0, markerIndex)}`; | |
| if ( | |
| provenance.repositoryId !== identity.repository.id || | |
| provenance.sourceRepository.toLowerCase() !== sourceRepository.toLowerCase() | |
| ) { | |
| return { ok: false, reasonCode: "ATTESTED_REPOSITORY_MISMATCH" }; | |
| } | |
| const expectedBuilderId = `${sourceRepository}${identity.workflow.ref.slice(markerIndex)}`; | |
| if (provenance.builderId !== expectedBuilderId) { | |
| return { ok: false, reasonCode: "ATTESTED_WORKFLOW_MISMATCH" }; | |
| } |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | f7a9390 | Sep 02 2026, 11:24 AM |
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is the right change for the reported delegated-release TOCTOU gap. Binding the Sigstore attestation to the stored workload identity through repository ID, workflow ref, commit SHA, and invocation identity, and re-evaluating that binding immediately before the create phase, closes a real window where policy or identity could drift after final verification. The richer VerifiedGitHubProvenance contract in @emdash-cms/registry-verification, the canonical digest-aware parser in workload/stored-identity.ts, and the dual workload checks in evaluateVerifiedRelease and the pre-create evaluateWorkloadAttestation call form a coherent defense.
I re-read the full diff and traced the flow through GitHubProvenanceVerifier, the release-verifier guard, normalizeVerifierReport/parseNormalizedVerifierReport, evaluateWorkloadAttestation, both evaluateVerifiedRelease callers, and publication-operation.ts#advancePhase. The prior review's mixed-case repository finding is fixed: evaluateWorkloadAttestation now derives the repository prefix from identity.workflow.ref (which preserves GitHub casework) and compares sourceRepository case-insensitively, while the builderId and invocationId comparisons stay case-sensitive. The regression coverage matches the PR description, the changeset correctly covers the only published package (@emdash-cms/registry-verification), and no AGENTS.md conventions are violated.
The code is essentially ready. One consolidation suggestion remains: a publication-critical path in publisher-do.ts still uses the older, non-digest-aware parseStoredWorkloadIdentity from workload/types.js even though this PR introduces a stronger one in workload/stored-identity.ts. Downstream checks in advancePhase currently catch any mismatch, but centralizing on the new parser removes the duplicate name and contract.
Findings
-
[suggestion]
apps/release-service/src/publisher-do/publisher-do.ts:1111This call site still uses
parseStoredWorkloadIdentityfrom../workload/types.js, which only validates canonical JSON and does not verify the stored digest againstintent.workloadIdentityDigest. The PR introduces a digest-awareparseStoredWorkloadIdentityin../workload/stored-identity.jsand relies on it for all security-critical verification paths; leaving this publication-criticaladvancePublicationOperationPhaseauthorization construction on the weaker parser splits canonical parsing and digest verification across two functions with the same name. DownstreamadvancePhasecurrently comparesauthorization.identityDigest !== intent.workload_identity_digest, so the current flow is safe, but it would be cleaner and less error-prone to use the new parser here too.const identity = intent ? await parseStoredWorkloadIdentity( intent.workloadIdentityJson, intent.workloadIdentityDigest, ) : null;Update the import to
../workload/stored-identity.jsand drop the now-redundantparseStoredWorkloadIdentityexport fromtypes.tsonce this migration is complete.
github-actions
Bot
added
review/approved
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is the right change for the delegated-release TOCTOU gap and the follow-up to the prior review. The PR now consistently uses the digest-aware, canonical parseStoredWorkloadIdentity from workload/stored-identity.ts everywhere security-critical workload identity is parsed, including the previously-flagged publisher-do.ts#advancePublicationOperationPhase path.
I re-read the full set of changes:
stored-identity.tsreplaces the oldtypes.tsparser with canonical JSON + digest verification, preserving the existing lowercase normalization for repository owner/name while allowing the workflow ref to retain GitHub casing.evaluateWorkloadAttestationbinds the Sigstore attestation to the stored identity by checking repository ID (case-insensitive repository URL), workflow file (case-sensitive builder ID), ref, commit SHA, and invocation identity against both the active policy and the parsed stored identity.evaluateVerifiedReleaseand the pre-create path inpublishing/workflow.tsboth re-evaluate the workload against the current policy, closing the window where policy or attestation could drift between final verification and record creation.release-verifierpropagates the enrichedVerifiedGitHubProvenancefields and defensively rejects any non-GitHub verifier that omits them.- Tests cover the regression cases described in the PR, including mixed-casing handling, digest/canonical failures, substituted attestation fields, incomplete reports, and a policy change between final verification and create.
- Only the published package
@emdash-cms/registry-verificationis changed; the changeset accurately reflects that.
No AGENTS.md conventions are violated: all SQL uses parameterized queries, no UI strings are introduced, and the documentation/changeset claims match the implementation. The code is ready to merge.
github-actions
Bot
added
review/approved
1 participant
What does this PR do?
Binds each verified Sigstore attestation to the GitHub OpenID Connect identity that admitted the release intent. The provenance verifier now reports the verified repository ID, workflow ref, commit, and invocation identity. The release service canonically parses and rehashes its stored workload identity, checks it against the active workload policy and attestation during initial and final verification, then repeats that check immediately before entering the create phase.
Adds regression coverage for substituted repository IDs, workflow files, refs, commits, run and invocation identities, malformed or non-canonical stored identities, digest mismatches, incomplete verifier reports, and a policy change between final verification and create.
Closes: N/A — independently validated delegated-release security finding with no public issue.
Type of change
Checklist
pnpm typecheckpasses — not run repo-wide; all three affected package typechecks passpnpm lintpasses — full type-aware lint reports zero diagnosticspnpm testpasses (or targeted tests for my change) — complete affected package suites passpnpm formathas been run — all changed files were formatted and checked directly to avoid touching unrelated untracked worktree filesmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. (N/A: no admin UI strings)AI-generated code disclosure
Screenshots / test output
No visual changes.
pnpm lint:json | jq '.diagnostics | length'—0@emdash-cms/registry-verificationNode, packed-output, and Workerd suites — 152 tests pass@emdash-cms/release-verifiersuite — 14 tests pass@emdash-cms/release-servicecore suite, run serially to avoid Workerd contention — 438 tests passpublint— passattw --pack0.18.2 crashes locally on Node 24.17.0 withCannot read properties of undefined (reading 'filename');npm pack, the repository's packed-output test, package typecheck, andpublintall complete successfully.