TEE signer: enclave repo signing, 2-of-3 embedded wallets, and pregenerated wallets#202
TEE signer: enclave repo signing, 2-of-3 embedded wallets, and pregenerated wallets#202daviddao wants to merge 7 commits into
Conversation
Adds an optional TEE-backed signing extension (all off by default): - packages/signer: standalone signer service that holds a root seed and derives per-(did, purpose) keys via HKDF-SHA256. Repo route signs commit digests (secp256k1, low-S, compact 64); wallet routes sign EVM digests / Solana messages ONLY for user-signed P-256 envelopes (TOFU-enrolled request key, monotonic nonce, freshness window). dstack attestation hook; dev mode for local work. - shared: SignerClient (internal-secret transport, typed errors). - pds-core/src/tee/: TeeKeypair (@atproto/crypto Keypair impl), actor-store keypair patch gated on a per-actor tee.json marker, PLC-based account adoption (rotate #atproto to the enclave did:key, marker written only after PLC accepts), /wallet routes, and env-flag wiring (EPDS_SIGNER_URL, EPDS_SIGNER_SECRET, EPDS_SIGNER_REQUIRE_ATTESTATION, EPDS_TEE_REPO_SIGNING, EPDS_TEE_ADOPT_ON_SIGNUP, EPDS_WALLET_ENABLED). The repo flow and the wallet flow stay strictly separate: disjoint key purposes, disjoint signer routes, disjoint authorization (PDS trust vs user-signed envelope). Normal ATProto reads/writes are unchanged; non-adopted accounts keep the stock local-key path. Docs: docs/design/tee-signer.md (architecture, threat model, honest limits incl. the OTP enrollment bootstrap), configuration.md, env examples, AGENTS.md. Coverage thresholds ratcheted 57/56/70/56 -> 61/60/72/60.
Moves wallet keys off the enclave root seed and onto the design doc's
recommended Model B ($13.3): each wallet is an independent secret with
user-held share majority, so durability and credible exit no longer
depend on a single enclave root.
Signer:
- wallet.ts (new): per-wallet 128-bit CSPRNG entropy -> BIP-39 -> HD
keys (EVM m/44'/60'/0'/0/0 via @scure/bip32, Solana m/44'/501'/0'/0'
via SLIP-10), split 2-of-3 with Privy's audited shamir-secret-sharing.
Server share encrypted AES-256-GCM under a root-seed-derived KEK (the
measurement-bound key); device + recovery shares leave only as JWEs
(ECDH-ES/A256GCM) encrypted to the user's enrolled P-256 request key.
Shares sent TO the enclave (sign/export/recover) are JWE-encrypted to
a published enclave encryption key, so the relaying PDS never sees a
second share next to the operator's server share.
- derive.ts/keys.ts: root-seed derivation now repo-signing + identity
keys ONLY - wallet keys are not derivable by construction.
- envelope.ts: payloads gain op ('sign'|'export') and a mandatory
deviceShareJwe, both covered by the user's signature.
- service.ts: new /v1/wallet/create, /v1/wallet/info/:did,
/v1/wallet/export (credible exit; response encrypted to the user) and
/v1/wallet/recover (authorization = possession of a share that
reconstructs this wallet; fresh SSS coefficients invalidate all old
shares; optional request-key rotation). Sign/export reconstruct the
entropy transiently, verify it against the wallet's registered public
keys, sign, and wipe. /v1/keys/derive rejects wallet purposes.
- store.ts: wallet table (encrypted server share + cached public
material, version bumped per re-shard) + rotateEnrollment.
Shared/pds-core:
- SignerClient: walletCreate/walletInfo/walletExport/walletRecover;
deriveKey narrowed to atproto/signing.
- wallet-router: /wallet/create, /wallet/export (envelope-authorized,
no PDS token), /wallet/recover; /wallet/info now serves stored wallet
material + the enclave encryption JWK.
Docs: tee-signer.md rewritten around the share model and its custody
invariant (no single party holds >=2 shares; user independently holds
2), configuration.md, env examples, changeset. Coverage ratcheted
61/60/72/60 -> 62/61/73/62.
…docs
- wallet-router: handlers extracted and exposed on two equivalent
surfaces - REST /wallet/* (unchanged) and the XRPC Lexicon namespace
/xrpc/app.gainforest.wallet.{enroll,create,getWallet,sign,export,
recover}. The XRPC router attaches body parsing per-route so
unmatched /xrpc/com.atproto.* traffic passes through to the stock
PDS untouched (covered by a pass-through test).
- setup.ts mounts both routers when EPDS_WALLET_ENABLED=1.
- AGENTS.md: new 'TEE Signer & Embedded Wallets' section with the
non-negotiable invariants (flow separation, wallet keys never
root-derived, never persist user shares, OAuth is never wallet
authorization) and the app.gainforest.* naming rule for any future
wallet lexicon work; signer package description updated.
- README.md: 'TEE Signing & Embedded Wallets' feature section +
further-reading link.
- tee-signer.md gains the NSID<->REST method table; configuration.md
and the changeset mention the XRPC surface.
- Coverage ratcheted 62/61/73/62 -> 63/61/74/62.
@atproto/pds installs its /xrpc catch-all while constructing pds.app, so appending app.gainforest.wallet.* routes during TEE setup allowed the stock auth/method handler to intercept them first. Prepare the custom router during setup, then finalize immediately before pds.start(): a thin Express gateway handles only wallet NSIDs and falls through to the fully configured stock/ePDS app for everything else. Regression coverage simulates a pre-existing stock /xrpc catch-all and proves the custom query/procedure win while finalizeApp stays idempotent. The design doc records the required ordering.
Provision receive-only wallets before a DID's first login (Privy-style wallet pregeneration), keyed by DID alone — including DIDs whose accounts still live on another PDS and migrate in later. - signer: POST /v1/wallet/pregenerate persists WHOLE entropy KEK-encrypted in a distinct AAD domain (epds-pregen\0v1\0<did>); idempotent; refuses once a wallet exists - signer: /v1/wallet/create claims a pregen record when present — decrypts, verifies the advertised addresses, splits 2-of-3, and deletes the whole-entropy blob in the same transaction (status 'claimed'); /v1/wallet/info exposes unclaimed records as 'pregen' - unclaimed wallets are receive-only: sign/export/recover all require the wallet row that only claiming creates - pds-core: POST /_internal/wallet/pregenerate (internal-secret gated, EPDS_WALLET_ENABLED=1 only) forwarding to the signer - shared: SignerClient.walletPregenerate + WalletPregenInfo/ WalletPregenerateResult types; create result status widened to 'created' | 'claimed' - docs: tee-signer.md 'Pregenerated wallets (defer-split)' section with the custody-honesty note; AGENTS.md invariant bullet; changeset
🦋 Changeset detectedLatest commit: 6f1d700 The changes in this PR will be included in the next version bump. 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds an optional TEE signer service with deterministic repo signing, encrypted 2-of-3 embedded wallets, pregenerated wallet claiming, REST/XRPC routes, pds-core integration, persistence, tests, configuration, and design documentation. ChangesTEE signer and embedded wallet
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 29482917529Coverage increased (+6.2%) to 62.63%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
docs/design/tee-signer.md (1)
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
To comply with Markdown best practices (and markdownlint rule MD040), specify a language identifier such as
textfor this code block.📝 Proposed fix
-``` +```text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/tee-signer.md` at line 50, Specify a language identifier on the fenced code block in the TEE signer documentation, using text or another appropriate language so the block complies with markdownlint rule MD040.Source: Linters/SAST tools
packages/signer/package.json (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
"type": "module"topackages/signer/package.jsonThis package is the odd one out in the workspace; the other Node packages already declare ESM, so this likely needs the same setting to keep
dist/index.jsloading consistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/signer/package.json` around lines 4 - 5, Add the "type": "module" field to packages/signer/package.json alongside the existing package metadata, ensuring the package treats dist/index.js and its Node modules as ESM consistently with the other workspace packages.packages/shared/src/signer-client.ts (1)
140-201: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo retry on transient signer failures for the repo-signing hot path.
request()has a timeout but no retry/backoff.signRepoDigest/deriveKey/attestation/healthare safe to retry (deterministic, no side effects), unlikewalletSign/walletRecoverwhich consume a server-side nonce and must not be blindly retried. Since repo commits go through this client on every write, a single transient network blip currently fails the write outright.Consider wrapping only the repo-path/no-side-effect calls with a small retry-with-backoff (e.g.
p-retry), per the guideline preferring established libraries over hand-rolled resiliency code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/signer-client.ts` around lines 140 - 201, Update the no-side-effect signer calls—health, attestation, deriveKey, and signRepoDigest—to use a small retry-with-backoff policy, preferably via the repository’s established retry library such as p-retry. Keep walletSign and walletRecover single-attempt, and preserve the existing request timeout and error behavior while retrying only transient failures.packages/pds-core/src/tee/actor-store-tee.ts (1)
108-128: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEvery repo write now does an extra filesystem read.
keypair()is on the hot write path; the patched version callsgetLocation+readTeeMarkeron every invocation, even for accounts that were never adopted. Consider caching the marker lookup result per-DID (populated on first read, invalidated whenadoptAccountIntoTeewrites a new marker) to avoid repeated disk I/O on every commit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pds-core/src/tee/actor-store-tee.ts` around lines 108 - 128, Cache the marker lookup result by DID within installTeeRepoSigning so actorStore.keypair does not repeat getLocation and readTeeMarker for each write. Populate the cache on the first lookup, reuse the cached marker or absence thereafter, and invalidate the corresponding entry when adoptAccountIntoTee writes a new marker so later keypair calls observe the TeeKeypair.packages/pds-core/src/tee/wallet-router.ts (1)
175-207: 🔒 Security & Privacy | 🔵 TrivialUnauthenticated
sign/exportendpoints are, by design, open to any caller who can reach the PDS.This matches the documented self-custodial security model (envelope is the authorization), so it's not a defect. Worth ensuring rate limiting/abuse protection for these two routes exists at the ingress/gateway layer, since each request still costs the signer a P-256 verification before rejection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pds-core/src/tee/wallet-router.ts` around lines 175 - 207, Preserve the intentionally unauthenticated sign and exportWallet handlers, since the signed envelope is their authorization mechanism. Verify that ingress or gateway-level rate limiting and abuse protection covers both routes, adding or configuring it there if missing; do not add PDS-side authentication.packages/signer/src/__tests__/service.test.ts (1)
130-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse isolated in-memory SQLite test fixtures.
Both suites use file-backed databases contrary to the test-fixture requirement.
packages/signer/src/__tests__/service.test.ts#L130-L158: create and close an isolated in-memory store/server per test, or make the stateful lifecycle one explicit scenario test.packages/signer/src/__tests__/store.test.ts#L10-L18: replace the temporary directory/database file with an in-memory store while retainingafterEachclosure.As per coding guidelines, database tests must use temporary in-memory SQLite databases and clean them up in
afterEach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/signer/src/__tests__/service.test.ts` around lines 130 - 158, Update the service-test fixture around beforeAll/afterAll in packages/signer/src/__tests__/service.test.ts lines 130-158 to use an isolated in-memory SignerStore per test, with server/store cleanup in afterEach, or isolate the lifecycle into one explicit scenario test. In packages/signer/src/__tests__/store.test.ts lines 10-18, replace the temporary directory and file-backed database with an in-memory SignerStore while retaining closure in afterEach.Source: Coding guidelines
packages/signer/src/index.ts (1)
25-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse an async
main()and await server startup.
listen()failures such as an occupied port occur after the currenttry/catchhas returned. Await the listening/error events so startup failures reach structured fatal logging.As per coding guidelines, service entry points should use an async
main()pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/signer/src/index.ts` around lines 25 - 74, Convert main to an async function and await the server startup through the app.listen flow, ensuring listen errors reject and propagate to the existing top-level catch for structured fatal logging. Update the entry-point invocation to await main using the project’s established async startup pattern, while preserving shutdown handling and server initialization.Source: Coding guidelines
packages/signer/src/envelope.ts (1)
28-35: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDocument
messageBase64as base64url. The signer path parses this field asbase64url, and the shared client already sends it that way. Rename it tomessageBase64Urlor call out the encoding in the wire-format doc to avoid client mismatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/signer/src/envelope.ts` around lines 28 - 35, Update the envelope wire-format documentation for messageBase64 to explicitly identify it as base64url, matching the signer parsing path and existing client payloads; alternatively rename the documented field to messageBase64Url consistently wherever this contract is defined.packages/signer/src/keys.ts (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
Number.parseIntover globalparseInt.For consistency and to avoid reliance on globals, use the ES2015+
Number.parseInt.♻️ Proposed refactor
- out += parseInt(hash[i], 16) >= 8 ? lower[i].toUpperCase() : lower[i] + out += Number.parseInt(hash[i], 16) >= 8 ? lower[i].toUpperCase() : lower[i]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/signer/src/keys.ts` at line 39, Replace the global parseInt call in the hash formatting expression with Number.parseInt, preserving the existing radix argument and uppercase/lowercase output behavior.Source: Linters/SAST tools
packages/signer/src/__tests__/derive.test.ts (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
.toHaveLength()for array length assertions.Using
.toHaveLength()instead of asserting the.lengthproperty with.toBe()provides better assertion failure messages, as Vitest will output the array contents if the length mismatches.
packages/signer/src/__tests__/derive.test.ts#L47-L47: Change toexpect(pub).toHaveLength(33).packages/signer/src/__tests__/derive.test.ts#L56-L56: Change toexpect(signature).toHaveLength(64).packages/signer/src/__tests__/derive.test.ts#L81-L81: Change toexpect(a).toHaveLength(33).packages/signer/src/__tests__/root-seed.test.ts#L20-L20: Change toexpect(seed).toHaveLength(32).packages/signer/src/__tests__/root-seed.test.ts#L64-L64: Change toexpect(seed).toHaveLength(32).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/signer/src/__tests__/derive.test.ts` at line 47, Replace direct .length equality assertions with toHaveLength assertions at packages/signer/src/__tests__/derive.test.ts lines 47-47, 56-56, and 81-81 for pub, signature, and a, respectively; also update packages/signer/src/__tests__/root-seed.test.ts lines 20-20 and 64-64 to use toHaveLength for seed.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pds-core/src/tee/actor-store-tee.ts`:
- Around line 73-91: Update readTeeMarker to return null only when fs.readFile
fails with an ENOENT error; propagate all other filesystem errors and malformed
JSON errors instead of swallowing them. Preserve the existing marker validation
and null result for missing marker files so installTeeRepoSigning and
adoptAccountIntoTee fail rather than falling back or re-adopting after transient
or corrupt reads.
- Around line 134-182: The adoptAccountIntoTee flow must serialize concurrent
adoption attempts for the same DID. Add a per-DID in-flight promise or lock
covering the existing marker check through marker creation, reuse the
established result for concurrent callers, and clean up the lock after
completion; add a concurrent test verifying only one updateAtprotoKey PLC
rotation is submitted.
In `@packages/pds-core/src/tee/setup.ts`:
- Around line 135-146: Strengthen the attestation validation in the signer setup
flow around attestation() before allowing repo or wallet signing: do not accept
mode='dstack' and a non-empty quote alone. Validate the quote chain, expected
measurement, freshness, and cryptographic binding between the quote’s report
data and identityPublicKeyHex, rejecting any failure; only log “attestation
verified” after all checks pass.
- Around line 257-261: Update the app setup around pds.app, gateway, and
walletXrpc so wallet XRPC remains within the existing PDS application middleware
and settings stack. Prefer mounting walletXrpc before the stock /xrpc catch-all
on pds.app; if retaining the gateway, mirror all required PDS middleware and
settings rather than using a fresh unconfigured Express instance.
In `@packages/signer/src/__tests__/derive.test.ts`:
- Around line 1-4: Reorder imports in
packages/signer/src/__tests__/derive.test.ts (lines 1-4) so node:crypto precedes
vitest, and in packages/signer/src/__tests__/root-seed.test.ts (lines 1-4) so
node:fs, node:os, and node:path precede vitest; preserve the ordering of
external and workspace imports afterward.
In `@packages/signer/src/envelope.ts`:
- Around line 74-118: Refactor parsePayload by extracting the shared field
validation and the sign-specific validation into small named helper functions,
then have parsePayload call them while preserving all existing rejection
conditions and defaults. Keep the op, nonce, iat, DID, deviceShareJwe, and
sign-purpose/message validation behavior identical, including the export path.
In `@packages/signer/src/index.ts`:
- Around line 48-50: Update the freshnessSec configuration in the signer
initialization to accept only positive safe integers from
SIGNER_WALLET_FRESHNESS_SEC; treat missing, non-numeric, zero, negative,
fractional, or unsafe parsed values as undefined so invalid values cannot reach
verifyEnvelope.
In `@packages/signer/src/service.ts`:
- Around line 211-230: Update the /v1/wallet/enroll handler and the
wallet-creation route around the referenced lines so requireSecret alone is
insufficient: require and verify a user-controlled authorization proof before
allowing initial enrollment or creation. Bind the verified proof to the DID and
request public key (and the creation inputs as applicable), reject missing or
invalid proofs, and preserve existing conflict and success responses after
verification.
- Around line 388-408: Update the wallet creation and recovery issuance flows
around store.claimPregen, store.createWallet, and the replacement logic to use
an idempotent two-phase issuance/acknowledgment protocol: encrypt and deliver
both shares successfully before activating the wallet or deleting/invalidating
prior entropy. Support safe retries with the same pending issuance, and commit
activation only after explicit acknowledgment, while ensuring device and
recovery shares are never persisted and are transmitted only as encrypted JWEs.
- Around line 594-657: Update the /v1/wallet/recover flow and its recovery-share
encryption/decryption helpers so recoveryShareJwe authenticates a structured
payload containing the DID, resolved target request key, nonce, and freshness
value alongside the share. Resolve and validate the target key before
decrypting, then reject payloads whose bound values do not match the request or
freshness requirements; ensure replacement-share generation encrypts this bound
payload for future recoveries.
---
Nitpick comments:
In `@docs/design/tee-signer.md`:
- Line 50: Specify a language identifier on the fenced code block in the TEE
signer documentation, using text or another appropriate language so the block
complies with markdownlint rule MD040.
In `@packages/pds-core/src/tee/actor-store-tee.ts`:
- Around line 108-128: Cache the marker lookup result by DID within
installTeeRepoSigning so actorStore.keypair does not repeat getLocation and
readTeeMarker for each write. Populate the cache on the first lookup, reuse the
cached marker or absence thereafter, and invalidate the corresponding entry when
adoptAccountIntoTee writes a new marker so later keypair calls observe the
TeeKeypair.
In `@packages/pds-core/src/tee/wallet-router.ts`:
- Around line 175-207: Preserve the intentionally unauthenticated sign and
exportWallet handlers, since the signed envelope is their authorization
mechanism. Verify that ingress or gateway-level rate limiting and abuse
protection covers both routes, adding or configuring it there if missing; do not
add PDS-side authentication.
In `@packages/shared/src/signer-client.ts`:
- Around line 140-201: Update the no-side-effect signer calls—health,
attestation, deriveKey, and signRepoDigest—to use a small retry-with-backoff
policy, preferably via the repository’s established retry library such as
p-retry. Keep walletSign and walletRecover single-attempt, and preserve the
existing request timeout and error behavior while retrying only transient
failures.
In `@packages/signer/package.json`:
- Around line 4-5: Add the "type": "module" field to
packages/signer/package.json alongside the existing package metadata, ensuring
the package treats dist/index.js and its Node modules as ESM consistently with
the other workspace packages.
In `@packages/signer/src/__tests__/derive.test.ts`:
- Line 47: Replace direct .length equality assertions with toHaveLength
assertions at packages/signer/src/__tests__/derive.test.ts lines 47-47, 56-56,
and 81-81 for pub, signature, and a, respectively; also update
packages/signer/src/__tests__/root-seed.test.ts lines 20-20 and 64-64 to use
toHaveLength for seed.
In `@packages/signer/src/__tests__/service.test.ts`:
- Around line 130-158: Update the service-test fixture around beforeAll/afterAll
in packages/signer/src/__tests__/service.test.ts lines 130-158 to use an
isolated in-memory SignerStore per test, with server/store cleanup in afterEach,
or isolate the lifecycle into one explicit scenario test. In
packages/signer/src/__tests__/store.test.ts lines 10-18, replace the temporary
directory and file-backed database with an in-memory SignerStore while retaining
closure in afterEach.
In `@packages/signer/src/envelope.ts`:
- Around line 28-35: Update the envelope wire-format documentation for
messageBase64 to explicitly identify it as base64url, matching the signer
parsing path and existing client payloads; alternatively rename the documented
field to messageBase64Url consistently wherever this contract is defined.
In `@packages/signer/src/index.ts`:
- Around line 25-74: Convert main to an async function and await the server
startup through the app.listen flow, ensuring listen errors reject and propagate
to the existing top-level catch for structured fatal logging. Update the
entry-point invocation to await main using the project’s established async
startup pattern, while preserving shutdown handling and server initialization.
In `@packages/signer/src/keys.ts`:
- Line 39: Replace the global parseInt call in the hash formatting expression
with Number.parseInt, preserving the existing radix argument and
uppercase/lowercase output behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c86e53c2-74bf-4fda-b8ff-b0fb2b46b743
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
.changeset/pregenerated-wallets.md.changeset/tee-signer-embedded-wallet.mdAGENTS.mdREADME.mddocs/configuration.mddocs/design/tee-signer.mddocs/design/testing-gaps.mdpackage.jsonpackages/pds-core/.env.examplepackages/pds-core/src/__tests__/actor-store-tee.test.tspackages/pds-core/src/__tests__/tee-keypair.test.tspackages/pds-core/src/__tests__/tee-setup.test.tspackages/pds-core/src/__tests__/wallet-router.test.tspackages/pds-core/src/index.tspackages/pds-core/src/tee/actor-store-tee.tspackages/pds-core/src/tee/setup.tspackages/pds-core/src/tee/tee-keypair.tspackages/pds-core/src/tee/user-auth.tspackages/pds-core/src/tee/wallet-router.tspackages/shared/src/__tests__/signer-client.test.tspackages/shared/src/index.tspackages/shared/src/signer-client.tspackages/signer/.env.examplepackages/signer/package.jsonpackages/signer/src/__tests__/derive.test.tspackages/signer/src/__tests__/envelope.test.tspackages/signer/src/__tests__/keys.test.tspackages/signer/src/__tests__/root-seed.test.tspackages/signer/src/__tests__/service.test.tspackages/signer/src/__tests__/store.test.tspackages/signer/src/__tests__/wallet.test.tspackages/signer/src/attestation.tspackages/signer/src/base58.tspackages/signer/src/derive.tspackages/signer/src/envelope.tspackages/signer/src/index.tspackages/signer/src/keys.tspackages/signer/src/purposes.tspackages/signer/src/root-seed.tspackages/signer/src/service.tspackages/signer/src/store.tspackages/signer/src/wallet.tspackages/signer/tsconfig.jsontsconfig.jsonvitest.config.ts
| export async function readTeeMarker( | ||
| directory: string, | ||
| ): Promise<TeeMarker | null> { | ||
| try { | ||
| const raw = await fs.readFile( | ||
| path.join(directory, TEE_MARKER_FILENAME), | ||
| 'utf8', | ||
| ) | ||
| const parsed: unknown = JSON.parse(raw) | ||
| if (typeof parsed !== 'object' || parsed === null) return null | ||
| const candidate = parsed as Record<string, unknown> | ||
| if (candidate.v === 1 && typeof candidate.keyDid === 'string') { | ||
| return parsed as TeeMarker | ||
| } | ||
| return null | ||
| } catch { | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
readTeeMarker treats every error as "not adopted" — including transient I/O failures.
The blanket catch { return null } can't distinguish "marker file doesn't exist" (safe, expected) from a transient read failure or malformed-JSON corruption (unsafe). This matters because of where the result is consumed:
- In
installTeeRepoSigning's patchedkeypair()(Lines 116-123), a transient failure for an already-adopted account silently falls back to the stale local key file — which no longer matches the DID document's#atprotokey after adoption rotated it. Any commit signed in that fallback path becomes unverifiable. - In
adoptAccountIntoTee(Lines 142-146), the same failure mode on an already-adopted account triggers a spurious re-adoption attempt.
Prefer failing loudly (propagate the error) for anything other than ENOENT, so a transient I/O hiccup surfaces as a write failure rather than a silently-invalid signature.
🛠️ Proposed fix
- } catch {
- return null
+ } catch (err) {
+ if ((err as NodeJS.ErrnoException)?.code === 'ENOENT') {
+ return null
+ }
+ throw err
}
}📝 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.
| export async function readTeeMarker( | |
| directory: string, | |
| ): Promise<TeeMarker | null> { | |
| try { | |
| const raw = await fs.readFile( | |
| path.join(directory, TEE_MARKER_FILENAME), | |
| 'utf8', | |
| ) | |
| const parsed: unknown = JSON.parse(raw) | |
| if (typeof parsed !== 'object' || parsed === null) return null | |
| const candidate = parsed as Record<string, unknown> | |
| if (candidate.v === 1 && typeof candidate.keyDid === 'string') { | |
| return parsed as TeeMarker | |
| } | |
| return null | |
| } catch { | |
| return null | |
| } | |
| } | |
| export async function readTeeMarker( | |
| directory: string, | |
| ): Promise<TeeMarker | null> { | |
| try { | |
| const raw = await fs.readFile( | |
| path.join(directory, TEE_MARKER_FILENAME), | |
| 'utf8', | |
| ) | |
| const parsed: unknown = JSON.parse(raw) | |
| if (typeof parsed !== 'object' || parsed === null) return null | |
| const candidate = parsed as Record<string, unknown> | |
| if (candidate.v === 1 && typeof candidate.keyDid === 'string') { | |
| return parsed as TeeMarker | |
| } | |
| return null | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException)?.code === 'ENOENT') { | |
| return null | |
| } | |
| throw err | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 76-79: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(
path.join(directory, TEE_MARKER_FILENAME),
'utf8',
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pds-core/src/tee/actor-store-tee.ts` around lines 73 - 91, Update
readTeeMarker to return null only when fs.readFile fails with an ENOENT error;
propagate all other filesystem errors and malformed JSON errors instead of
swallowing them. Preserve the existing marker validation and null result for
missing marker files so installTeeRepoSigning and adoptAccountIntoTee fail
rather than falling back or re-adopting after transient or corrupt reads.
| export async function adoptAccountIntoTee(opts: { | ||
| ctx: AdoptionCtx | ||
| signer: SignerClient | ||
| did: string | ||
| logger: LoggerLike | ||
| }): Promise<TeeMarker> { | ||
| const { ctx, signer, did, logger } = opts | ||
|
|
||
| const { directory } = await ctx.actorStore.getLocation(did) | ||
| const existing = await readTeeMarker(directory) | ||
| if (existing) { | ||
| return existing | ||
| } | ||
|
|
||
| const keyInfo = await signer.deriveKey(did, 'atproto/signing') | ||
| if (!keyInfo.didKey) { | ||
| throw new Error(`signer returned no didKey for ${did}`) | ||
| } | ||
|
|
||
| // Rotate the DID document's signing key. Signed by the PLC rotation | ||
| // key — the recoverable authority the signer never holds. | ||
| await ctx.plcClient.updateAtprotoKey(did, ctx.plcRotationKey, keyInfo.didKey) | ||
|
|
||
| const marker: TeeMarker = { | ||
| v: 1, | ||
| keyDid: keyInfo.didKey, | ||
| adoptedAt: new Date().toISOString(), | ||
| } | ||
| // Marker is written only after PLC accepted the rotation — a failed | ||
| // rotation must never leave the actor signing with a key that isn't | ||
| // in its DID document. | ||
| await writeTeeMarker(directory, marker) | ||
|
|
||
| try { | ||
| await ctx.sequencer.sequenceIdentityEvt(did) | ||
| } catch (err) { | ||
| // Non-fatal: the PLC op is authoritative; relays will converge. | ||
| logger.warn( | ||
| { err, did }, | ||
| 'failed to sequence identity event after TEE adoption', | ||
| ) | ||
| } | ||
|
|
||
| logger.info( | ||
| { did, keyDid: keyInfo.didKey }, | ||
| 'account adopted into TEE signing', | ||
| ) | ||
| return marker | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== actor-store-tee outline ==\n'
ast-grep outline packages/pds-core/src/tee/actor-store-tee.ts --view expanded || true
printf '\n== adoptAccountIntoTee references ==\n'
rg -n "adoptAccountIntoTee|readTeeMarker|writeTeeMarker|sequenceIdentityEvt|updateAtprotoKey" packages/pds-core/src -S
printf '\n== nearby code ==\n'
sed -n '1,260p' packages/pds-core/src/tee/actor-store-tee.ts
printf '\n== tests mentioning adoption / tee ==\n'
rg -n "adoptOnSignup|/_internal/tee/adopt|adoptAccountIntoTee|TEE adoption|tee marker|updateAtprotoKey" packages/pds-core -g '*.{test,spec}.ts' -SRepository: hypercerts-org/ePDS
Length of output: 14274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== actor-store-tee tests ==\n'
sed -n '1,280p' packages/pds-core/src/__tests__/actor-store-tee.test.ts
printf '\n== tee setup adoption path ==\n'
sed -n '1,260p' packages/pds-core/src/tee/setup.tsRepository: hypercerts-org/ePDS
Length of output: 16211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== updateAtprotoKey usages ==\n'
rg -n "updateAtprotoKey\(" -S .
printf '\n== related PLC client types / docs ==\n'
rg -n "plcClient|prev-commit|rotation key|updateAtprotoKey" packages docs -S
printf '\n== internal adopt call sites and concurrency clues ==\n'
rg -n "adoptAccountIntoTee\(|adoptOnSignup\(" packages/pds-core/src -SRepository: hypercerts-org/ePDS
Length of output: 3790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tee signer design notes on adoption / rotation ==\n'
sed -n '60,120p' docs/design/tee-signer.md
printf '\n'
sed -n '240,310p' docs/design/tee-signer.md
printf '\n== plc rotation client implementation / interface ==\n'
rg -n "updateAtprotoKey|prev-commit|commit" packages -SRepository: hypercerts-org/ePDS
Length of output: 9999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== signer-client repo rotation API ==\n'
sed -n '160,240p' packages/shared/src/signer-client.ts
printf '\n== signer service repo rotation endpoint ==\n'
rg -n "sign/repo|updateAtprotoKey|rotation" packages/signer/src -S
printf '\n== any PLC operation docs / comments ==\n'
rg -n "prev-commit|prevCommit|PLC op|rotation key|updateAtprotoKey" docs packages -SRepository: hypercerts-org/ePDS
Length of output: 6637
🌐 Web query:
did:plc rotation updateAtprotoKey prev-commit concurrent rotation semantics
💡 Result:
In the did:plc protocol, key rotations and identity updates are managed via a chain of operations, where each operation includes a prev field (referencing the CID of the previous operation) to establish a linear, verifiable history [1][2]. Concurrent rotation semantics in did:plc are governed by the directory server's validation logic, which maintains a 72-hour window for "authority-based" overrides [1]. The protocol supports hierarchy among rotation keys, where keys are listed in an ordered array within the identity's state [1]. A key with a higher authority (a lower index in the rotationKeys array) has the ability to invalidate—or "clobber"—a chain of operations signed by a lower-authority rotation key, provided the new, overriding operation is submitted within 72 hours of the operation being invalidated [1]. Key aspects of these semantics include: 1. History Rewriting: If an unauthorized or lower-authority update occurs, a higher-authority key can issue a new operation that points back to the CID of the last known "valid" state (the fork point), effectively discarding the branch created by the lower-authority key [1]. 2. Validation: The PLC directory server collects, validates, and stores these operations [1]. It ensures that updates are signed by authorized keys and conform to the expected sequence by checking the prev-commit link [1][3]. 3. Operation Chain: Every update (such as rotate_signing_key or update_atp_pds) must correctly reference the previous operation's CID to maintain the integrity of the log [1][4]. Failing to correctly reference the current head of the log typically results in rejection by the directory server, except in the case of authorized forks within the 72-hour window [1]. When updating atproto-specific keys (e.g., via updateAtprotoKey-like operations), the system relies on this underlying rotation mechanism to prove the validity of the change [1][5]. If an account is custodied by a PDS, the PDS often manages these operations, but users who maintain their own rotation keys can submit operations directly to the directory to bypass or override PDS actions, provided their signature is valid and their key authority is sufficient [6][7].
Citations:
- 1: https://github.com/did-method-plc/did-method-plc/blob/main/website/spec/v0.1/did-plc.md
- 2: https://github.com/did-method-plc/did-method-plc?tab=readme-ov-file
- 3: https://github.com/did-method-plc/did-method-plc
- 4: https://ipfs.copylaradio.com/ipfs/Qma9z8RajcS685p1VDyWxzck3na4FzVB2LjP23kfAgQoF8/specs/did-plc.html
- 5: How to edit `did:plc` rotation keys bluesky-social/atproto#3366
- 6: activateAccount for non-server-custodied keys fails with "Server rotation key not included in PLC DID data" bluesky-social/atproto#4819
- 7: https://github.com/MetaflameDragon/did-plc-tools/blob/main/guides/add_rot_key.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== updateAtprotoKey implementation ==\n'
rg -n "updateAtprotoKey" packages -S
printf '\n== PLC rotation / prev handling in signer or shared code ==\n'
rg -n "prev|rotate|atprotoKey|plc" packages/shared packages/signer packages/pds-core/src -S
printf '\n== around signer repo rotation endpoint ==\n'
sed -n '180,260p' packages/signer/src/service.tsRepository: hypercerts-org/ePDS
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate updateAtprotoKey implementation ==\n'
rg -n --context 4 "updateAtprotoKey\s*\(" packages/pds-core packages/shared packages/signer -S
printf '\n== adoptAccountIntoTee tests around races ==\n'
sed -n '150,230p' packages/pds-core/src/__tests__/actor-store-tee.test.tsRepository: hypercerts-org/ePDS
Length of output: 3690
Serialize TEE adoption per DID
Two concurrent calls can both pass the marker check and submit updateAtprotoKey before either writes tee.json. Keep a per-DID in-flight promise (or lock) around adoption, and add a concurrent test that only one PLC rotation is submitted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pds-core/src/tee/actor-store-tee.ts` around lines 134 - 182, The
adoptAccountIntoTee flow must serialize concurrent adoption attempts for the
same DID. Add a per-DID in-flight promise or lock covering the existing marker
check through marker creation, reuse the established result for concurrent
callers, and clean up the lock after completion; add a concurrent test verifying
only one updateAtprotoKey PLC rotation is submitted.
| const attestation = await signer.attestation() | ||
| if (env.EPDS_SIGNER_REQUIRE_ATTESTATION === '1') { | ||
| if (attestation.mode !== 'dstack' || !attestation.quote) { | ||
| throw new Error( | ||
| 'EPDS_SIGNER_REQUIRE_ATTESTATION=1 but the signer presented no hardware quote ' + | ||
| `(mode: ${attestation.mode}${attestation.note ? `, note: ${attestation.note}` : ''})`, | ||
| ) | ||
| } | ||
| logger.info( | ||
| { reportData: attestation.reportData }, | ||
| 'signer attestation verified (dstack quote present)', | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline packages/shared/src/signer-client.ts \
--items all --match 'SignerClient|attestation'
rg -n -C4 --glob '*.ts' \
'\b(verifyAttestation|verifyQuote|attestation|identityPublicKeyHex|reportData)\b' \
packages/shared packages/pds-core packages/signerRepository: hypercerts-org/ePDS
Length of output: 16150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/pds-core/src/tee/setup.ts
printf '\n--- signer attestation contract ---\n'
sed -n '1,220p' packages/signer/src/attestation.ts
printf '\n--- tee signer design doc references ---\n'
rg -n -C3 'attestation|quote|reportData|identityPublicKeyHex|verify' docs/design/tee-signer.md packages/pds-core/src/tee/setup.ts packages/shared/src/signer-client.ts packages/signer/src/service.tsRepository: hypercerts-org/ePDS
Length of output: 27202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow reads if the files are large
wc -l packages/pds-core/src/tee/setup.ts packages/signer/src/attestation.ts docs/design/tee-signer.md
printf '\n--- setup.ts around attestation flow ---\n'
sed -n '100,180p' packages/pds-core/src/tee/setup.ts
printf '\n--- signer attestation.ts ---\n'
sed -n '1,140p' packages/signer/src/attestation.ts
printf '\n--- tee signer design doc excerpts ---\n'
rg -n -C2 'attestation|quote|reportData|identityPublicKeyHex|verify|dstack' docs/design/tee-signer.mdRepository: hypercerts-org/ePDS
Length of output: 9927
Require a real attestation check here.
This accepts any signer that returns mode: 'dstack' with a non-empty quote; reject it until the quote chain, expected measurement, freshness, and binding to reportData/identityPublicKeyHex are checked before enabling repo or wallet signing.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 140-140: Refactor this code to not use nested template literals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pds-core/src/tee/setup.ts` around lines 135 - 146, Strengthen the
attestation validation in the signer setup flow around attestation() before
allowing repo or wallet signing: do not accept mode='dstack' and a non-empty
quote alone. Validate the quote chain, expected measurement, freshness, and
cryptographic binding between the quote’s report data and identityPublicKeyHex,
rejecting any failure; only log “attestation verified” after all checks pass.
Source: Coding guidelines
| const upstream = pds.app | ||
| const gateway = express() | ||
| gateway.use('/xrpc', walletXrpc) | ||
| gateway.use(upstream) | ||
| pds.app = gateway |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline packages/pds-core/src/tee/wallet-router.ts \
--items all --match 'createWalletXrpcRouter'
rg -n -C4 \
'\b(cors|compression|logger|trust proxy|rateLimit|helmet)\b' \
packages/pds-core/src/tee packages/pds-core/src/index.tsRepository: hypercerts-org/ePDS
Length of output: 39705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '212,272p' packages/pds-core/src/tee/setup.ts
printf '\n---\n'
sed -n '1158,1172p' packages/pds-core/src/index.ts
printf '\n---\n'
sed -n '1,120p' packages/pds-core/src/tee/wallet-router.tsRepository: hypercerts-org/ePDS
Length of output: 8051
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '272,460p' packages/pds-core/src/tee/wallet-router.ts
printf '\n---\n'
rg -n -C3 'compression|cors|helmet|trust proxy|logger|morgan|xrpc' packages/pds-core/src/tee/wallet-router.ts packages/pds-core/src/tee/setup.ts packages/pds-core/src/index.tsRepository: hypercerts-org/ePDS
Length of output: 33162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 "trust proxy|compression\(|cors\(|helmet\(|morgan\(|logger" packages/pds-core/src packages/pds-core
printf '\n---\n'
rg -n -C2 "app\.set\('trust proxy'|set\(\"trust proxy\"|compression\(|cors\(" packages/pds-core/srcRepository: hypercerts-org/ePDS
Length of output: 50376
🌐 Web query:
@atproto/pds 0.4.209 packages/pds/src/index.ts trust proxy compression cors logger express app.use xrpc
💡 Result:
In the @atproto/pds package, the packages/pds/src/index.ts file serves as the main entry point for the Personal Data Server's Express application [1][2]. It is responsible for configuring and mounting the server's middleware and routing [2]. The Express application in this file typically manages middleware in a specific order to ensure proper request handling and security [2]. Standard middleware such as logger, compression, authentication, CORS, and well-known routes are mounted onto the Express app instance using app.use [2]. A critical design pattern in the PDS architecture is that the XRPC router (handled by server.xrpc.router) is mounted after most other global middleware to ensure it only handles requests that have passed through necessary preprocessing (such as authentication or input sanitization) [2]. Specifically regarding your requested components: - logger: Used to record request/response cycles [2]. - compression: Applied as middleware to compress HTTP responses [2]. - cors: Configured via CORS middleware to manage cross-origin requests, typically with a set maxAge [2]. - trust proxy: Often required when the PDS runs behind a load balancer or reverse proxy (e.g., Nginx) to ensure the server correctly identifies the original client's IP address and protocol [2]. - express app.use: The primary method used in index.ts to register the middleware chain and the final XRPC router [2]. - XRPC: The central routing mechanism for AT Protocol API calls, which is mounted globally to handle all AT Protocol-specific service requests [2]. It is important to note that developers must be cautious with middleware order, particularly with body-parsing middleware. Because Express body parsers (like express.json) consume the request stream, they must be carefully positioned relative to the XRPC router to avoid issues with reading the request body multiple times [2].
Citations:
- 1: https://github.com/bluesky-social/atproto/blob/main/packages/pds/src/index.ts
- 2: https://github.com/w-social-eu/w-social-atproto/blob/main/XRPC_ARCHITECTURE.md
🌐 Web query:
site:github.com/bluesky-social/atproto packages/pds/src/index.ts "compression(" "cors(" "trust proxy" "@atproto/pds" 0.4.209
💡 Result:
In the @atproto/pds package (version 0.4.209), configurations for compression, CORS, and proxying are managed via environment variables and server configuration objects within the PDS architecture rather than being hardcoded in src/index.ts [1][2]. Proxy Configuration and Trust Proxy: The PDS handles proxying through specific configuration settings, often exposed in the environment or PDS configuration files [2]. The system includes a trustProxy option for its OAuth provider, which allows for specific validation logic—such as a function that evaluates IP addresses—rather than blindly trusting headers [3]. Other proxy settings configurable via environment variables include PDS_PROXY_ALLOW_HTTP2, PDS_PROXY_HEADERS_TIMEOUT, PDS_PROXY_BODY_TIMEOUT, PDS_PROXY_MAX_RESPONSE_SIZE, and PDS_PROXY_PREFER_COMPRESSED [1][2]. Compression: Compression behavior, particularly for repository data synchronization (getRepo), has been optimized to improve performance by coalescing data into larger chunks (e.g., ~64 KiB) before compression, which prevents the zlib transform from being overwhelmed by small writes [4][5]. The PDS also supports a preference for compressed responses when proxying upstream requests, which can be toggled via the PDS_PROXY_PREFER_COMPRESSED environment variable [6]. CORS: CORS policies are typically managed within the XRPC server framework utilized by the PDS. The PDS instantiates its server using configurations defined in its Lexicon and server-level setups, which handle standard web server middleware responsibilities like CORS [7][8]. For developers working with the PDS codebase, the main entry point is typically found in packages/pds/src/index.ts, which initializes the server based on these loaded configurations [7][9].
Citations:
- 1: https://github.com/bluesky-social/atproto/blob/main/packages/pds/src/config/env.ts
- 2: https://github.com/bluesky-social/atproto/blob/87a1f24262e0e644b6cf31cc7a0446d9127ffa94/packages/pds/src/config/config.ts
- 3: bluesky-social/atproto@82d5a2d
- 4: bluesky-social/atproto@cc329bf
- 5: Improve PDS getRepo Compression bluesky-social/atproto#5078
- 6: Default to unencoded responses bluesky-social/atproto#2834
- 7: https://github.com/bluesky-social/atproto/blob/main/packages/pds/src/index.ts
- 8: https://github.com/bluesky-social/atproto/blob/57ca2109/packages/pds/src/lexicon/index.ts
- 9: https://github.com/bluesky-social/atproto/tree/main/packages/pds
Keep wallet XRPC inside the existing PDS app stack. Mounting walletXrpc on a fresh gateway makes these methods bypass the PDS app’s middleware/settings contract. Move the wallet router ahead of the stock /xrpc catch-all on pds.app, or mirror the same middleware on the gateway.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 257-257: Express application should use Helmet
Context: express()
Note: [CWE-693] Protection Mechanism Failure (Express app without Helmet security headers).
(missing-helmet-typescript)
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 258-258: This framework implicitly discloses version information by default. Make sure it is safe here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pds-core/src/tee/setup.ts` around lines 257 - 261, Update the app
setup around pds.app, gateway, and walletXrpc so wallet XRPC remains within the
existing PDS application middleware and settings stack. Prefer mounting
walletXrpc before the stock /xrpc catch-all on pds.app; if retaining the
gateway, mirror all required PDS middleware and settings rather than using a
fresh unconfigured Express instance.
| import { describe, expect, it } from 'vitest' | ||
| import * as nodeCrypto from 'node:crypto' | ||
| import { verifySignature, parseDidKey } from '@atproto/crypto' | ||
| import { secp256k1 } from '@noble/curves/secp256k1' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reorder imports to place built-ins first.
As per coding guidelines, imports must be ordered as built-ins, external packages, workspace packages, and then local relative imports.
packages/signer/src/__tests__/derive.test.ts#L1-L4: Move thenode:cryptoimport above thevitestimport.packages/signer/src/__tests__/root-seed.test.ts#L1-L4: Move thenode:fs,node:os, andnode:pathimports above thevitestimport.
📍 Affects 2 files
packages/signer/src/__tests__/derive.test.ts#L1-L4(this comment)packages/signer/src/__tests__/root-seed.test.ts#L1-L4
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/signer/src/__tests__/derive.test.ts` around lines 1 - 4, Reorder
imports in packages/signer/src/__tests__/derive.test.ts (lines 1-4) so
node:crypto precedes vitest, and in
packages/signer/src/__tests__/root-seed.test.ts (lines 1-4) so node:fs, node:os,
and node:path precede vitest; preserve the ordering of external and workspace
imports afterward.
Source: Coding guidelines
| function parsePayload(bytes: Buffer): WalletEnvelopePayload | null { | ||
| let parsed: unknown | ||
| try { | ||
| parsed = JSON.parse(bytes.toString('utf8')) | ||
| } catch { | ||
| return null | ||
| } | ||
| if (typeof parsed !== 'object' || parsed === null) return null | ||
| const p = parsed as Record<string, unknown> | ||
| if (!isPlausibleDid(p.did)) return null | ||
| const op: unknown = p.op ?? 'sign' | ||
| if (op !== 'sign' && op !== 'export') return null | ||
| if (!Number.isSafeInteger(p.nonce) || (p.nonce as number) <= 0) return null | ||
| if (!Number.isSafeInteger(p.iat)) return null | ||
| if (!isCompactJwe(p.deviceShareJwe)) return null | ||
|
|
||
| if (op === 'sign') { | ||
| if (!isWalletPurpose(p.purpose)) return null | ||
| if (p.purpose === 'wallet/evm') { | ||
| if ( | ||
| typeof p.digestHex !== 'string' || | ||
| !/^[0-9a-fA-F]{64}$/.test(p.digestHex) | ||
| ) { | ||
| return null | ||
| } | ||
| } else { | ||
| const msg = | ||
| typeof p.messageBase64 === 'string' | ||
| ? decodeBase64Url(p.messageBase64, MAX_SOL_MESSAGE_BYTES) | ||
| : null | ||
| if (!msg) return null | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| did: p.did, | ||
| op, | ||
| purpose: p.purpose as WalletPurpose | undefined, | ||
| digestHex: p.digestHex as string | undefined, | ||
| messageBase64: p.messageBase64 as string | undefined, | ||
| deviceShareJwe: p.deviceShareJwe, | ||
| nonce: p.nonce as number, | ||
| iat: p.iat as number, | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
parsePayload cognitive complexity flagged by SonarCloud as a gate failure.
The linear chain of field checks pushes this function's cognitive complexity to 23 (limit 15), reported as a CI failure rather than a warning. Splitting the core-field checks and the sign-only checks into small named helpers keeps behavior identical while resolving the gate failure and improving reviewability of this security-critical validator.
♻️ Proposed refactor to reduce cognitive complexity
-function parsePayload(bytes: Buffer): WalletEnvelopePayload | null {
- let parsed: unknown
- try {
- parsed = JSON.parse(bytes.toString('utf8'))
- } catch {
- return null
- }
- if (typeof parsed !== 'object' || parsed === null) return null
- const p = parsed as Record<string, unknown>
- if (!isPlausibleDid(p.did)) return null
- const op: unknown = p.op ?? 'sign'
- if (op !== 'sign' && op !== 'export') return null
- if (!Number.isSafeInteger(p.nonce) || (p.nonce as number) <= 0) return null
- if (!Number.isSafeInteger(p.iat)) return null
- if (!isCompactJwe(p.deviceShareJwe)) return null
-
- if (op === 'sign') {
- if (!isWalletPurpose(p.purpose)) return null
- if (p.purpose === 'wallet/evm') {
- if (
- typeof p.digestHex !== 'string' ||
- !/^[0-9a-fA-F]{64}$/.test(p.digestHex)
- ) {
- return null
- }
- } else {
- const msg =
- typeof p.messageBase64 === 'string'
- ? decodeBase64Url(p.messageBase64, MAX_SOL_MESSAGE_BYTES)
- : null
- if (!msg) return null
- }
- }
-
- return {
- did: p.did,
- op,
- purpose: p.purpose as WalletPurpose | undefined,
- digestHex: p.digestHex as string | undefined,
- messageBase64: p.messageBase64 as string | undefined,
- deviceShareJwe: p.deviceShareJwe,
- nonce: p.nonce as number,
- iat: p.iat as number,
- }
-}
+function hasValidCoreFields(p: Record<string, unknown>): boolean {
+ return (
+ isPlausibleDid(p.did) &&
+ Number.isSafeInteger(p.nonce) &&
+ (p.nonce as number) > 0 &&
+ Number.isSafeInteger(p.iat) &&
+ isCompactJwe(p.deviceShareJwe)
+ )
+}
+
+function hasValidSignFields(p: Record<string, unknown>): boolean {
+ if (!isWalletPurpose(p.purpose)) return false
+ if (p.purpose === 'wallet/evm') {
+ return (
+ typeof p.digestHex === 'string' &&
+ /^[0-9a-fA-F]{64}$/.test(p.digestHex)
+ )
+ }
+ return (
+ typeof p.messageBase64 === 'string' &&
+ decodeBase64Url(p.messageBase64, MAX_SOL_MESSAGE_BYTES) !== null
+ )
+}
+
+function parsePayload(bytes: Buffer): WalletEnvelopePayload | null {
+ let parsed: unknown
+ try {
+ parsed = JSON.parse(bytes.toString('utf8'))
+ } catch {
+ return null
+ }
+ if (typeof parsed !== 'object' || parsed === null) return null
+ const p = parsed as Record<string, unknown>
+ const op: unknown = p.op ?? 'sign'
+ if (op !== 'sign' && op !== 'export') return null
+ if (!hasValidCoreFields(p)) return null
+ if (op === 'sign' && !hasValidSignFields(p)) return null
+
+ return {
+ did: p.did as string,
+ op,
+ purpose: p.purpose as WalletPurpose | undefined,
+ digestHex: p.digestHex as string | undefined,
+ messageBase64: p.messageBase64 as string | undefined,
+ deviceShareJwe: p.deviceShareJwe as string,
+ nonce: p.nonce as number,
+ iat: p.iat as number,
+ }
+}📝 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.
| function parsePayload(bytes: Buffer): WalletEnvelopePayload | null { | |
| let parsed: unknown | |
| try { | |
| parsed = JSON.parse(bytes.toString('utf8')) | |
| } catch { | |
| return null | |
| } | |
| if (typeof parsed !== 'object' || parsed === null) return null | |
| const p = parsed as Record<string, unknown> | |
| if (!isPlausibleDid(p.did)) return null | |
| const op: unknown = p.op ?? 'sign' | |
| if (op !== 'sign' && op !== 'export') return null | |
| if (!Number.isSafeInteger(p.nonce) || (p.nonce as number) <= 0) return null | |
| if (!Number.isSafeInteger(p.iat)) return null | |
| if (!isCompactJwe(p.deviceShareJwe)) return null | |
| if (op === 'sign') { | |
| if (!isWalletPurpose(p.purpose)) return null | |
| if (p.purpose === 'wallet/evm') { | |
| if ( | |
| typeof p.digestHex !== 'string' || | |
| !/^[0-9a-fA-F]{64}$/.test(p.digestHex) | |
| ) { | |
| return null | |
| } | |
| } else { | |
| const msg = | |
| typeof p.messageBase64 === 'string' | |
| ? decodeBase64Url(p.messageBase64, MAX_SOL_MESSAGE_BYTES) | |
| : null | |
| if (!msg) return null | |
| } | |
| } | |
| return { | |
| did: p.did, | |
| op, | |
| purpose: p.purpose as WalletPurpose | undefined, | |
| digestHex: p.digestHex as string | undefined, | |
| messageBase64: p.messageBase64 as string | undefined, | |
| deviceShareJwe: p.deviceShareJwe, | |
| nonce: p.nonce as number, | |
| iat: p.iat as number, | |
| } | |
| } | |
| function hasValidCoreFields(p: Record<string, unknown>): boolean { | |
| return ( | |
| isPlausibleDid(p.did) && | |
| Number.isSafeInteger(p.nonce) && | |
| (p.nonce as number) > 0 && | |
| Number.isSafeInteger(p.iat) && | |
| isCompactJwe(p.deviceShareJwe) | |
| ) | |
| } | |
| function hasValidSignFields(p: Record<string, unknown>): boolean { | |
| if (!isWalletPurpose(p.purpose)) return false | |
| if (p.purpose === 'wallet/evm') { | |
| return ( | |
| typeof p.digestHex === 'string' && | |
| /^[0-9a-fA-F]{64}$/.test(p.digestHex) | |
| ) | |
| } | |
| return ( | |
| typeof p.messageBase64 === 'string' && | |
| decodeBase64Url(p.messageBase64, MAX_SOL_MESSAGE_BYTES) !== null | |
| ) | |
| } | |
| function parsePayload(bytes: Buffer): WalletEnvelopePayload | null { | |
| let parsed: unknown | |
| try { | |
| parsed = JSON.parse(bytes.toString('utf8')) | |
| } catch { | |
| return null | |
| } | |
| if (typeof parsed !== 'object' || parsed === null) return null | |
| const p = parsed as Record<string, unknown> | |
| const op: unknown = p.op ?? 'sign' | |
| if (op !== 'sign' && op !== 'export') return null | |
| if (!hasValidCoreFields(p)) return null | |
| if (op === 'sign' && !hasValidSignFields(p)) return null | |
| return { | |
| did: p.did as string, | |
| op, | |
| purpose: p.purpose as WalletPurpose | undefined, | |
| digestHex: p.digestHex as string | undefined, | |
| messageBase64: p.messageBase64 as string | undefined, | |
| deviceShareJwe: p.deviceShareJwe as string, | |
| nonce: p.nonce as number, | |
| iat: p.iat as number, | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 74-74: Refactor this function to reduce its Cognitive Complexity from 23 to the 15 allowed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/signer/src/envelope.ts` around lines 74 - 118, Refactor parsePayload
by extracting the shared field validation and the sign-specific validation into
small named helper functions, then have parsePayload call them while preserving
all existing rejection conditions and defaults. Keep the op, nonce, iat, DID,
deviceShareJwe, and sign-purpose/message validation behavior identical,
including the export path.
Source: Linters/SAST tools
| freshnessSec: process.env.SIGNER_WALLET_FRESHNESS_SEC | ||
| ? parseInt(process.env.SIGNER_WALLET_FRESHNESS_SEC, 10) | ||
| : undefined, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and the freshness consumer.
ast-grep outline packages/signer/src/index.ts --view expanded || true
printf '\n---\n'
rg -n "freshnessSec|verifyEnvelope|SIGNER_WALLET_FRESHNESS_SEC" packages/signer/src packages/signer -g '!**/dist/**' -g '!**/build/**'
printf '\n---\n'
cat -n packages/signer/src/index.ts | sed -n '1,140p'Repository: hypercerts-org/ePDS
Length of output: 6030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the freshness validation path in the signer app.
TARGET="packages/signer/src/index.ts"
if [ -f "$TARGET" ]; then
cat -n "$TARGET" | sed -n '1,220p'
fi
printf '\n--- consumer ---\n'
rg -n "age > freshnessSec|freshnessSec" packages/signer/src -g '!**/dist/**' -g '!**/build/**'Repository: hypercerts-org/ePDS
Length of output: 3655
Reject invalid freshness configuration.
parseInt() can yield NaN; because NaN is not nullish, it flows into verifyEnvelope, and age > NaN is always false, so stale envelopes are never rejected. Accept only positive safe integers before passing this through.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 49-49: Prefer Number.parseInt over parseInt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/signer/src/index.ts` around lines 48 - 50, Update the freshnessSec
configuration in the signer initialization to accept only positive safe integers
from SIGNER_WALLET_FRESHNESS_SEC; treat missing, non-numeric, zero, negative,
fractional, or unsafe parsed values as undefined so invalid values cannot reach
verifyEnvelope.
| app.post('/v1/wallet/enroll', requireSecret, (req, res) => { | ||
| const { did, requestPublicKeyHex } = req.body ?? {} | ||
| if ( | ||
| !isPlausibleDid(did) || | ||
| !isCompressedP256Hex(requestPublicKeyHex) || | ||
| !isValidP256PublicKeyHex(requestPublicKeyHex) | ||
| ) { | ||
| res.status(400).json({ error: 'invalid did or requestPublicKeyHex' }) | ||
| return | ||
| } | ||
| const outcome = store.enroll(did, requestPublicKeyHex.toLowerCase()) | ||
| if (outcome === 'conflict') { | ||
| res.status(409).json({ | ||
| error: | ||
| 'a different request key is already enrolled for this DID; key rotation requires wallet recovery', | ||
| }) | ||
| return | ||
| } | ||
| logger.info({ did, outcome }, 'wallet enrollment') | ||
| res.json({ status: outcome }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Bind initial enrollment and creation to user-controlled proof.
Both routes require only the PDS-held internal secret. A compromised PDS can enroll its own key, create the wallet, and decrypt both returned user shares—giving it complete wallet control. Bootstrap authorization must include proof the PDS cannot synthesize.
Also applies to: 342-408
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/signer/src/service.ts` around lines 211 - 230, Update the
/v1/wallet/enroll handler and the wallet-creation route around the referenced
lines so requireSecret alone is insufficient: require and verify a
user-controlled authorization proof before allowing initial enrollment or
creation. Bind the verified proof to the DID and request public key (and the
creation inputs as applicable), reject missing or invalid proofs, and preserve
existing conflict and success responses after verification.
| const created = pregen | ||
| ? store.claimPregen(did, walletRow) | ||
| : store.createWallet(walletRow) | ||
| if (!created) { | ||
| res.status(409).json({ error: 'wallet already exists for this DID' }) | ||
| return | ||
| } | ||
| const [deviceShareJwe, recoveryShareJwe] = await Promise.all([ | ||
| encryptToRequestKey(enrollment.requestPubkeyHex, shares[1]), | ||
| encryptToRequestKey(enrollment.requestPubkeyHex, shares[2]), | ||
| ]) | ||
| logger.info( | ||
| { did, claimedPregen: pregen !== null }, | ||
| 'wallet created (2-of-3 shares issued)', | ||
| ) | ||
| res.json({ | ||
| status: pregen ? 'claimed' : 'created', | ||
| wallet: walletPublicInfo(store.getWallet(did) as WalletRow), | ||
| deviceShareJwe, | ||
| recoveryShareJwe, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Do not activate wallet state before the user safely receives the shares.
Creation commits the wallet—and deletes pregenerated entropy—before share encryption/delivery. Recovery invalidates old shares before replacements are delivered. Encryption failure or a lost response can therefore make funded wallets permanently inaccessible. Use an idempotent two-phase issuance/acknowledgment protocol.
As per path instructions, device and recovery shares must never be persisted and may only be transmitted as encrypted JWEs.
Also applies to: 640-664
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/signer/src/service.ts` around lines 388 - 408, Update the wallet
creation and recovery issuance flows around store.claimPregen,
store.createWallet, and the replacement logic to use an idempotent two-phase
issuance/acknowledgment protocol: encrypt and deliver both shares successfully
before activating the wallet or deleting/invalidating prior entropy. Support
safe retries with the same pending issuance, and commit activation only after
explicit acknowledgment, while ensuring device and recovery shares are never
persisted and are transmitted only as encrypted JWEs.
Source: Path instructions
| app.post('/v1/wallet/recover', requireSecret, async (req, res) => { | ||
| const { did, recoveryShareJwe, requestPublicKeyHex } = req.body ?? {} | ||
| if (!isPlausibleDid(did) || !isCompactJwe(recoveryShareJwe)) { | ||
| res.status(400).json({ error: 'invalid did or recoveryShareJwe' }) | ||
| return | ||
| } | ||
| if ( | ||
| requestPublicKeyHex !== undefined && | ||
| (!isCompressedP256Hex(requestPublicKeyHex) || | ||
| !isValidP256PublicKeyHex(requestPublicKeyHex)) | ||
| ) { | ||
| res.status(400).json({ error: 'invalid requestPublicKeyHex' }) | ||
| return | ||
| } | ||
| const wallet = store.getWallet(did) | ||
| const enrollment = store.getEnrollment(did) | ||
| if (!wallet || !enrollment) { | ||
| res.status(403).json({ error: 'no wallet exists for this DID' }) | ||
| return | ||
| } | ||
|
|
||
| let recoveryShare: Uint8Array | undefined | ||
| let serverShare: Uint8Array | undefined | ||
| let entropy: Uint8Array | undefined | ||
| let keys: WalletChainKeys | undefined | ||
| let newShares: [Uint8Array, Uint8Array, Uint8Array] | undefined | ||
| try { | ||
| try { | ||
| recoveryShare = await decryptJweToEnclave(rootSeed, recoveryShareJwe) | ||
| serverShare = decryptServerShare( | ||
| shareKek, | ||
| did, | ||
| wallet.serverShareCipherHex, | ||
| ) | ||
| entropy = await combineWalletShares(serverShare, recoveryShare) | ||
| keys = deriveChainKeys(entropy) | ||
| } catch (err) { | ||
| logger.warn({ err, did }, 'wallet recovery reconstruction failed') | ||
| res.status(403).json({ error: 'share reconstruction failed' }) | ||
| return | ||
| } | ||
| if (bytesToHex(keys.evmPublicKey) !== wallet.evmPubkeyHex) { | ||
| res.status(403).json({ error: 'recovery share does not match wallet' }) | ||
| return | ||
| } | ||
|
|
||
| // Fresh coefficients: every share changes, forward secrecy holds. | ||
| newShares = await splitWalletEntropy(entropy) | ||
| const version = store.replaceServerShare( | ||
| did, | ||
| encryptServerShare(shareKek, did, newShares[0]), | ||
| ) | ||
| const targetKeyHex = | ||
| typeof requestPublicKeyHex === 'string' | ||
| ? requestPublicKeyHex.toLowerCase() | ||
| : enrollment.requestPubkeyHex | ||
| if (targetKeyHex !== enrollment.requestPubkeyHex) { | ||
| store.rotateEnrollment(did, targetKeyHex) | ||
| logger.info({ did }, 'request key rotated during recovery') | ||
| } | ||
| const [deviceShareJwe, newRecoveryShareJwe] = await Promise.all([ | ||
| encryptToRequestKey(targetKeyHex, newShares[1]), | ||
| encryptToRequestKey(targetKeyHex, newShares[2]), | ||
| ]) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Cryptographically bind recovery to the replacement request key.
recoveryShareJwe authenticates only the share; requestPublicKeyHex remains mutable. The relaying PDS can replace that field in a valid recovery request, rotate enrollment to its own key, and receive both fresh shares. Include the DID, target key, nonce, and freshness inside the authenticated encrypted recovery payload.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/signer/src/service.ts` around lines 594 - 657, Update the
/v1/wallet/recover flow and its recovery-share encryption/decryption helpers so
recoveryShareJwe authenticates a structured payload containing the DID, resolved
target request key, nonce, and freshness value alongside the share. Resolve and
validate the target key before decrypting, then reject payloads whose bound
values do not match the request or freshness requirements; ensure
replacement-share generation encrypts this bound payload for future recoveries.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pds-core/src/tee/wallet-router.ts (1)
319-333: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExport a single route factory per file.
As per coding guidelines, "Route files must export a single
create*Router(ctx)factory function". This file currently exports two distinct router factories (createWalletRouteron line 296 andcreateWalletXrpcRouterhere).To adhere to the architectural convention, please split these into separate modules (for example, by moving the XRPC router to
wallet-xrpc-router.tsand exporting the shared handler factorybuildWalletHandlersfrom a common internal file).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pds-core/src/tee/wallet-router.ts` around lines 319 - 333, Split the two router factories into separate modules so each route file exports only one create*Router factory. Move createWalletXrpcRouter and its XRPC route registrations into a dedicated module, and place shared buildWalletHandlers logic in a common internal module for reuse by both routers; update imports and exports accordingly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pds-core/src/tee/wallet-router.ts`:
- Around line 207-210: Update the catch block in the wallet-router flow to call
logger.error instead of logger.warn while preserving the existing structured {
err, did } context, message, and sendSignerError(res, err) behavior.
---
Outside diff comments:
In `@packages/pds-core/src/tee/wallet-router.ts`:
- Around line 319-333: Split the two router factories into separate modules so
each route file exports only one create*Router factory. Move
createWalletXrpcRouter and its XRPC route registrations into a dedicated module,
and place shared buildWalletHandlers logic in a common internal module for reuse
by both routers; update imports and exports accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a073506a-04fc-4285-b932-0a74265a521e
📒 Files selected for processing (5)
.changeset/public-wallet-lookup.mdREADME.mddocs/design/tee-signer.mdpackages/pds-core/src/__tests__/wallet-router.test.tspackages/pds-core/src/tee/wallet-router.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- docs/design/tee-signer.md
- packages/pds-core/src/tests/wallet-router.test.ts
| } catch (err) { | ||
| logger.warn({ err, did }, 'public wallet info failed') | ||
| sendSignerError(res, err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use logger.error for structured error logging.
As per coding guidelines, "Use structured Pino error logging as logger.error({ err }, 'description')". Please change logger.warn to logger.error to comply with the project's error logging standard.
♻️ Proposed fix
- } catch (err) {
- logger.warn({ err, did }, 'public wallet info failed')
- sendSignerError(res, err)
- }
+ } catch (err) {
+ logger.error({ err, did }, 'public wallet info failed')
+ sendSignerError(res, err)
+ }📝 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.
| } catch (err) { | |
| logger.warn({ err, did }, 'public wallet info failed') | |
| sendSignerError(res, err) | |
| } | |
| } catch (err) { | |
| logger.error({ err, did }, 'public wallet info failed') | |
| sendSignerError(res, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pds-core/src/tee/wallet-router.ts` around lines 207 - 210, Update
the catch block in the wallet-router flow to call logger.error instead of
logger.warn while preserving the existing structured { err, did } context,
message, and sendSignerError(res, err) behavior.
Source: Coding guidelines
1 participant
What this adds
An optional TEE signer service plus PDS integration. From a single ATProto login, each user gets their normal repo identity and, additively, a self-custodial crypto wallet (EVM + Solana). Every private key is created and used inside a confidential VM. Everything is off by default; without
EPDS_SIGNER_URLthe PDS behaves exactly like stock ePDS.The two flows (kept strictly separate)
Repo signing (
EPDS_TEE_REPO_SIGNING=1):actorStore.keypair(did)is patched so adopted accounts sign repo commits in the enclave viaPOST /v1/sign/repo. Keys are derived from the enclave root seed per(did, purpose), so a replacement enclave that passes attestation re-derives identical keys. Accounts are adopted after creation by rotating the DID's#atprotomethod to the enclave key (automatic on signup withEPDS_TEE_ADOPT_ON_SIGNUP=1, or per account viaPOST /_internal/tee/adopt). Rollback is deleting the marker and rotating PLC back.Wallets (
EPDS_WALLET_ENABLED=1): each wallet is independent 128-bit entropy under a 2-of-3 Shamir split, using Privy's auditedshamir-secret-sharinglibrary. The signer persists only the server share, encrypted under a KEK derived from the root seed. Device and recovery shares go to the user as JWEs encrypted to their enrolled P-256 request key. The PDS relays ciphertext in both directions and can never read a share.Wallet keys are deliberately never derived from the root seed. The repo key is disposable (lost key means mint a new one and rotate PLC); a wallet address is the account itself, so its entropy must not be reproducible by whoever holds the seed.
Authorization model
A PDS-forwarded OAuth token is never sufficient for signing or export. Every sign/export request carries an envelope signed by the user's enrolled request key, with a monotonic nonce and freshness window, verified inside the enclave. The enclave reconstructs the entropy transiently from server share + device share, checks it reproduces the wallet's registered public keys, signs, and wipes. Recovery is authorized by possession of the recovery share and re-splits with fresh coefficients, so stolen old shares become useless.
Custody invariant: no single party ever holds two shares, and the user independently controls two (device + recovery). The operator can freeze but cannot trap;
POST /wallet/exportgives a credible exit.Honest limitation, documented in
docs/design/tee-signer.md: enrollment itself is trust-on-first-use over an operator-issued OAuth token. Passkey-bound enrollment is future work.Surfaces
Wallet routes are mounted twice, backed by the same handlers: REST under
/wallet/*and XRPC under/xrpc/app.gainforest.wallet.*(enroll, create, getWallet, sign, export, recover). The XRPC gateway is wrapped around the finished app so the stock/xrpccatch-all cannot intercept custom NSIDs.Pregenerated wallets (defer-split)
POST /_internal/wallet/pregenerate {"did": ...}provisions a wallet for a DID before its first login, so assets can be sent ahead of onboarding. Keyed by DID alone; the DID may still live on another PDS and migrate in later. The enclave stores the whole entropy KEK-encrypted in a separate AAD domain, and:/wallet/createafter enrollment claims it: decrypt, verify the advertised addresses, split 2-of-3, delete the whole-entropy blob in the same transaction, respond withstatus: 'claimed';Deployment and threat model
The signer runs on confidential-compute hardware in production (dstack in a CVM) and is intentionally not part of the Docker stack or Railway.
EPDS_SIGNER_REQUIRE_ATTESTATION=1makes pds-core refuse to start against an unattested signer. The design doc covers what a hostile host can and cannot do, including the nonce-rollback replay window and the requirement that KMS/code-approval authority sits with a separate party.Testing
Unit and integration tests across signer (share lifecycle walked end to end: enroll, create, sign, export, recover, re-sign; pregenerate, claim, post-claim signing), pds-core TEE wiring, wallet router, and signer client. 1239 tests pass; coverage stays above the ratchet thresholds.
format:check,lint, andtypecheckare clean.Docs
docs/design/tee-signer.md(architecture, custody model, threat model, code map), AGENTS.md invariants for contributors, configuration docs, and a changeset for client app developers and operators.Summary by CodeRabbit
New Features
Documentation
Tests