fix: proof-verify delegated release authority - #2849
Conversation
Scope checkThis PR changes 630 lines across 12 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.
The approach is sound and directly addresses the audit finding. Moving approval-authority verification and baseline selection onto proof-verified repository CARs is the right design, and carrying the immutable approver set from the awaiting_approval transition closes the unsigned-envelope attack path. The code fits the release-service architecture and the test additions cover the advertised attack scenarios (CID substitution, attacker-DID passkey, injected baseline, omitted baseline).
I reviewed the changed source files, call sites in the release-intent/publishing workflows, and the new registry-client tests, and checked AGENTS.md conventions (changesets, comments, localization-invariant backend code, no logged-out hot-path changes). I did not run the test suite or type checker.
Headline conclusion: the PR is technically correct, but I have three suggestions before merge:
-
Full-repo export now shares a 512 KiB response cap with single JSON page fetches.
readPublisherVerificationSnapshotnow downloads the entire publisher repo viacom.atproto.sync.getRepo, but the existingMAX_PDS_RESPONSE_BYTESconstant is still512 * 1024. A publisher repo larger than that will fail verification withPUBLISHER_PDS_INVALID. The previous paginatedrepo.listRecordsapproach could handle much larger release histories. Either raise the cap for repo-export calls or document the hard limit. -
guardedFetchmasks upstream 404s fromDirectPdsClient. It rewrites upstream404responses to200so thatrepo.getRecorderror bodies survivefetchVerifiedResource. That's correct for the JSON endpoint but wrong forsync.getRepo/sync.getRecord: a missing repo or record now surfaces asPDS_RESPONSE_TYPE_INVALID/PUBLISHER_PDS_INVALIDinstead ofPUBLISHER_IDENTITY_INVALIDorRECORD_NOT_FOUND. The newgetPackageRepositorypath is affected. -
The changeset describes the new API but could be more useful. It names
DirectPdsClient.getPackageRepository()accurately, but a one-sentence entry doesn't explain why a consumer should care (proof-verified reads replace trust in unsigned PDS envelopes) or show basic usage. Expanding it slightly would make the CHANGELOG more useful.
Findings
-
[suggestion]
apps/release-service/src/verification/pds.ts:17The new
getPackageRepository()path fetches the entire publisher repository export withcom.atproto.sync.getRepo, but the response limit is still512 * 1024, the same value previously used for a single JSON page. A publisher repo larger than that will fail baseline selection withPUBLISHER_PDS_INVALIDand block legitimate releases. Give repo-export calls a larger, separate budget (or make it configurable) instead of reusing the page-sized cap.const MAX_PDS_RESPONSE_BYTES = 512 * 1024; const MAX_REPO_EXPORT_RESPONSE_BYTES = 5 * 1024 * 1024;Then use
MAX_REPO_EXPORT_RESPONSE_BYTESat thegetPackageRepository()client construction so single-record calls keep the tighter limit. -
[suggestion]
apps/release-service/src/verification/pds.ts:176guardedFetchrewrites every upstream404to a200sorepo.getRecorderror bodies can be read. That status rewrite is now inherited by theDirectPdsClientcalls tosync.getRepoandsync.getRecord. A missing repo or record therefore no longer reaches theDirectPdsClient404branch; instead it becomesPDS_RESPONSE_TYPE_INVALIDand in this file is mapped to the genericPUBLISHER_PDS_INVALIDcode instead of the more accuratePUBLISHER_IDENTITY_INVALID/RECORD_NOT_FOUND.Consider passing a separate fetch wrapper to the
sync.*endpoints that preserves404status, or haveDirectPdsClientread the true upstream status from thex-emdash-upstream-statusheader your wrapper already adds. -
[suggestion]
.changeset/clean-otters-prove.md:5The entry names the new API accurately, but it doesn't tell registry-client consumers why they should care or how to use it. Per
.changeset/README.md, a minor feature changeset should give readers enough context to decide whether the release affects them.Adds `DirectPdsClient.getPackageRepository()` for reading a package profile and every package release from one proof-verified AT Protocol repository export. Use this instead of combining unsigned `com.atproto.repo.getRecord`/`listRecords` responses when you need a tamper-evident view of a publisher's package data: ```ts const { profile, releases } = await directPdsClient.getPackageRepository("gallery");The profile and release records are verified against the repository commit signature, so unsigned PDS envelopes cannot substitute values or omit records.
02824fa to
7284b99
Compare
🦋 Changeset detectedLatest commit: 3f7ef8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
All three prior suggestions have been addressed, and a fresh pass over the changed source and tests found no new blocking issues.
- Repo-export byte budget:
apps/release-service/src/verification/pds.tsnow usesDEFAULT_DIRECT_PDS_MAX_RESPONSE_BYTES(5 MiB) forgetPackageRepository()instead of the 512 KiB single-record cap, and the test suite verifies it accepts a 600 KiB export. - 404 status preservation:
guardedFetch()now records the upstream status inx-emdash-upstream-statusand reconstructs the response with the real status, sosync.getRepo404s correctly surface asREPOSITORY_NOT_FOUND/PUBLISHER_IDENTITY_INVALID. - Changeset:
.changeset/clean-otters-prove.mdnow explains the newgetPackageRepository()API, when to use it, and how the verification works.
The approach remains sound: approval authority checks the immutable approver set from the awaiting_approval transition before checking the proof-verified current profile CID, and baseline/release-absence decisions use the signed repository CAR. Attack-focused tests for CID substitution, attacker-DID passkeys, injected higher-semver baselines, and omitted genuine baselines are present and look meaningful.
I did not run the test suite, linter, or type checker; conclusions are static.
github-actions
Bot
added
review/approved
7284b99 to
8df40f8
Compare
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is a sound security fix that addresses the audit finding by moving approval authority and baseline/absence decisions from unsigned PDS envelopes to proof-verified repository state. Carrying the immutable approver set out of the awaiting_approval transition closes the approver-substitution path, and using a signed repository CAR (sync.getRepo) for profile/release reads closes the CID/value baseline path. The three items from the previous review are still in place: getPackageRepository() uses the 5 MiB default byte budget, guardedFetch() preserves upstream 404 status through x-emdash-upstream-status, and the changeset explains the new API.
I re-read the changed source and tests and did not find any new blocking security or regression issues. However I do have two polish findings:
-
createGuardedIdentityFetch()inauthority.tsforwards requests tofetchVerifiedResource()but drops theinit.headersthatDirectPdsClient.getPackageProfile()sets (Accept: application/vnd.ipld.car). A spec-compliant PDS may reject async.getRecordrequest without that header, which would surface asPROFILE_FETCH_FAILED. The siblingguardedFetch()inverification/pds.tsalready merges the original headers;authority.tsshould do the same. -
The
MockPdsdocstring sayssync.getReporeturns JSON, but the handler returns a CAR (application/vnd.ipld.car). It should be corrected so future readers don't expect JSON there.
Neither affects the security guarantee, and I remain comfortable with the overall approach.
Findings
-
[suggestion]
apps/release-service/src/approvals/authority.ts:201-209createGuardedIdentityFetch()validates the method, then callsfetchVerifiedResource()with a callback that forwards onlyurlandrequestInit, droppinginit.headers. The caller (DirectPdsClient.getPackageProfile()) sendsAccept: application/vnd.ipld.car, which is therefore lost before the upstream PDS sees it. A strict PDS may return 406/400 for async.getRecordrequest without the expectedAcceptheader, causing an avoidablePROFILE_FETCH_FAILED.Sibling
guardedFetch()insrc/verification/pds.tsavoids this by merging the original headers into the callback fetch. Apply the same pattern here:function createGuardedIdentityFetch(fetchImplementation: typeof fetch): typeof fetch { return async (input, init) => { const requestedUrl = new URL(input instanceof Request ? input.url : input.toString()); const method = init?.method ?? (input instanceof Request ? input.method : "GET"); if (method !== "GET") { throw new ApprovalAuthorityError("PROFILE_FETCH_FAILED"); } const resource = await fetchVerifiedResource(requestedUrl, { fetch: (url, requestInit) => fetchImplementation(url, { ...requestInit, headers: init?.headers ?? (input instanceof Request ? input.headers : undefined), }), resolveHostname: (hostname) => resolvePublicHostname(hostname, fetchImplementation), headerTimeoutMs: 10_000, totalTimeoutMs: 30_000, maxBytes: MAX_PROFILE_RESPONSE_BYTES, maxRedirects: 1, }); -
[suggestion]
packages/atproto-test-utils/src/mock-pds.ts:13-15The docstring says
sync.getReporeturns JSON, butsyncGetRepo()actually returns CAR bytes withContent-Type: application/vnd.ipld.car. This is a false claim in a code-level comment that can mislead maintainers about the handler contract.* Response shapes mirror the cirrus PDS reference implementation: CAR bytes * for `sync.getRecord` with `Content-Type: application/vnd.ipld.car`, CAR bytes * for `sync.getRepo`, JSON `{ records, cursor? }` for `listRecords`, and JSON * `{ uri, cid, value }` for `repo.getRecord`.
github-actions
Bot
added
review/approved
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This remains the right fix for the audit finding. The PR moves approval-authority and access-baseline decisions from unsigned PDS envelopes to proof-verified repository state: verifyCurrentApprover now gates every approval on the immutable approver set captured at the awaiting_approval transition, and readPublisherVerificationSnapshot selects release baselines from a signed sync.getRepo CAR through DirectPdsClient.getPackageRepository(). That closes the unsigned-envelope approver-substitution, approver-suppression, and baseline-manipulation paths the audit identified.
Since the previous review:
createGuardedIdentityFetch()inauthority.tsnow forwards the caller'sinit.headers(e.g.Accept: application/vnd.ipld.car) into the guarded fetch, matching the siblingguardedFetch()inverification/pds.ts. ✅MockPds’s docstring now correctly describessync.getRepoas returning CAR bytes, not JSON. ✅
I re-read the updated source and tests and did not find any new blocking security, correctness, or convention issues:
DirectPdsClient.getPackageRepository()verifies the profile record against the publisher signing key before iterating the export, and release records are filtered to the package collection/rkey prefix and re-parsed against the package-release lexicon.loadCurrentApprovalPolicy()checks the proof-verified profile CID against the approval evidence and validates/sorts the current approver policy.- Attack-focused tests cover CID/value substitution, an attacker-DID passkey, an injected higher-semver baseline, and an omitted genuine baseline.
- The changeset for
@emdash-cms/registry-clientexplains the new public surface, default size limit, andREPOSITORY_NOT_FOUNDbehavior.
No new needs_fixing or suggestion findings.
github-actions
Bot
added
review/approved
1 participant
What does this PR do?
Fixes the delegated release service so unsigned PDS response envelopes cannot choose approvers or suppress access-escalation approval.
awaiting_approvaltransition into every approval authorization check.repo.listRecordsJSON.Addresses the security audit finding “Unsigned PDS envelopes drive approval and access-baseline decisions.”
Type of change
Checklist
pnpm typecheckpasses — affected package typechecks pass; the repository-wide command was not run.pnpm lintpasses — changed files have zero type-aware diagnostics; the repository-wide pre-edit baseline was not clean.pnpm testpasses (or targeted tests for my change)pnpm formathas been run — the repository formatter was run on every changed file, without formatting unrelated untracked files in the worktree.messages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
No visual changes.
--maxWorkers=1.