feat: rewrite TEE republish as a verify-in-enclave lease renewer#585
Merged
Conversation
Entire-Checkpoint: fc036595e056
Entire-Checkpoint: 38d6a126d501
Entire-Checkpoint: 3c00825431b3
Entire-Checkpoint: 0f7367351e38
Entire-Checkpoint: ac1ea4365a4f
…umns - DROP COLUMN IF EXISTS for encrypted_ipns_key, key_epoch, latest_cid, sequence_number - CREATE INDEX IDX_ipns_republish_schedule_ipns_name for getDueEntries JOIN - down() throws per D-01 greenfield waiver (matches Phase-66 pattern) - Timestamp 1751000000000 orders strictly after 1750000000000-ApiSchemaCutover
Entire-Checkpoint: 81315731a1b4
…vation - 3 RED tests: unset anchor returns MIN_EPOCH, 5-week anchor returns 2, future anchor clamps to MIN_EPOCH (1) - Import getInternalCurrentEpoch which does not exist yet (RED gate) - Also import afterEach and add EPOCH_ZERO_TIMESTAMP_MS cleanup to beforeEach
…vation - Export EPOCH_DURATION_MS constant (4-week interval) - Export getInternalCurrentEpoch() reading EPOCH_ZERO_TIMESTAMP_MS at call time (not module load) for testability - Returns MIN_EPOCH (1) as safe fallback when anchor unset or in the future - Clamps via Math.max(MIN_EPOCH, ...) — never returns below 1 - Satisfies §6.7-1: never reads a relay-supplied epoch scalar
…eEnrollRequiredError - Replace 3-arg decryptWithFallback tests with 2-arg API (keyEpoch hint) - Add stale guard tests: ReEnrollRequiredError thrown before any decryptIpnsKey call when keyEpoch < internalCurrentEpoch - 1 - Assert requiresReEnroll, keyEpoch, currentEpoch on error; message security check - Add vi.spyOn on getKeypair to assert no-unwrap on stale path - Add mid-rotation fallback test: key decrypts via internalCurrentEpoch - Update epoch migration round-trip to use 2-arg signature + setInternalEpoch helper - Import ReEnrollRequiredError which does not exist yet (RED gate)
…uiredError stale guard - Export ReEnrollRequiredError: requiresReEnroll=true, keyEpoch, currentEpoch; message names epoch integers only (no key material, satisfies T-67-02-I) - decryptWithFallback(encryptedIpnsKey, keyEpoch): derive internalCurrentEpoch from getInternalCurrentEpoch() — never reads relay-supplied epoch scalar (D-03) - Hard stale floor: keyEpoch < internalCurrentEpoch-1 throws ReEnrollRequiredError BEFORE any unwrap attempt, satisfying T-67-02-E2 and §6.7-3 - Fallback trial order: keyEpoch first, then internalCurrentEpoch (mid-rotation) - Remove 3-arg signature (currentEpoch/previousEpoch relay params gone, T-67-02-E) - Also imports getInternalCurrentEpoch from tee-keys (added in previous commit)
Entire-Checkpoint: a1ce4af2d8a8
RED gate: 5 tests assert equal-CID, equal-sequence, different-bytes, valid-signature, and explicit-lifetime behaviors. All fail because renewIpnsRecord is not yet exported from ipns-signer.ts. Security invariants under test: TEE-01 / TEE-02
Adds renewIpnsRecord(ed25519PrivateKey, marshaledExistingRecord, lifetimeMs?) alongside the existing signIpnsRecord (unchanged for back-compat). Implementation: parse the marshaled record with parseIpnsRecord from @cipherbox/crypto, then re-sign using createIpnsRecord with the parsed value and sequence only — structurally preventing CID repoint or sequence increment (TEE-01 / TEE-02). createIpnsRecord emits ValidityType 0 (EOL) automatically via the ipns package. All 5 ipns-signer tests pass: equal-CID, equal-sequence, different-bytes, valid-signature, explicit-lifetime.
Entire-Checkpoint: 75725eb8cbd0
- tests forwarding of encryptedIpnsPrivateKey + keyEpoch to createAndPublishIpnsRecord - test return value includes TEE fields when teeKeys provided - test omits TEE fields when teeKeys absent (unchanged behavior) - test fail-closed throw when teeKeys.currentPublicKey is empty
- import wrapKey, bytesToHex, hexToBytes from @cipherbox/crypto - validate teeKeys fail-closed: throws when currentPublicKey is empty or currentEpoch is not finite (does not publish un-enrolled subfolder) - ECIES-wrap the generated ipnsPrivateKey under teeKeys.currentPublicKey via wrapKey before publish (zero-knowledge: server only receives the hex-encoded wrapped ciphertext) - forward encryptedIpnsPrivateKey + keyEpoch to createAndPublishIpnsRecord so the subfolder's first ipns_records row enrolls in TEE renewal - return encryptedIpnsPrivateKey + keyEpoch from createSubfolder - do not zero ipnsPrivateKey/readKey/writeKey (D-09 terminal-owner)
Entire-Checkpoint: 532cb3df4d85
- build from repo root context (Dockerfile COPYs monorepo pnpm-lock.yaml/packages/*) - host port 3002 to avoid conflict with mock-ipns-routing on 3001 - TEE_MODE=simulator, CIPHERBOX_ENVIRONMENT=development - document TEE_WORKER_URL=http://localhost:3002 in .env.example
- bullmq@^5.67.3 and pg@^8.14.1 match apps/api versions (no new lockfile entry) - @types/pg for type-safe make-due DB poke in round-trip suite (67-08) - update pnpm-lock.yaml for tests/sdk-e2e importer
Entire-Checkpoint: fff60cd54d3e
- Add makeEntry helper creating real IPNS records + real ECIES encryption - Assert newSequenceNumber == parsed seq (no increment, §7.3 test 12) - Assert verify-fail path never calls decryptWithFallback (spy uncalled) - Assert binding-mismatch rejects without emitting signedRecord (§7.3 test 18) - Assert ReEnrollRequiredError surfaces as requiresReEnroll: true (§7.3 test 19) - Assert epoch upgrade fields present when usedEpoch != internalCurrentEpoch - Remove old relay-scalar fields (latestCid/sequenceNumber/currentEpoch/previousEpoch) - Replace signIpnsRecord mock with renewIpnsRecord mock
- Reshape RepublishEntry: add signedRecord, remove latestCid/sequenceNumber/currentEpoch/previousEpoch - Add requiresReEnroll to RepublishResult for structured re-enroll signal - Parse signedRecord bytes via parseIpnsRecord before any decryption - Verify signature via verifyIpnsRecordSignature BEFORE decryptWithFallback - Assert name-key binding using deriveEd25519PublicKey vs publicKeyFromIpnsName - Re-sign same CID + same sequence via renewIpnsRecord (no +1, TEE-02) - Epoch upgrade uses getInternalCurrentEpoch() not relay scalars (D-03) - Re-encrypt key before zeroing; zero key on all paths including binding-fail - ReEnrollRequiredError surfaces as requiresReEnroll: true with safe error string - Fix binding-mismatch test assertion to match exact safe error message
Entire-Checkpoint: f3abde09b188
…uild
- rewrite createMockEntry to remove dropped schedule fields (encryptedIpnsKey,
keyEpoch, latestCid, sequenceNumber — removed in 67-01)
- add createMockRecord helper for IpnsRecord pairs
- set up QB mocks: scheduleQBMock (innerJoin chain) + recordSelectQBMock +
recordUpdateQBMock (renewIpnsRecordEol)
- add getDueEntries tests: innerJoin condition, tombstone-filter exclusion,
paired {schedule, record} result shape
- add teeEntries tests: signedRecord from record, no latestCid/sequenceNumber/
currentEpoch/previousEpoch in the relay body
- add renewIpnsRecordEol CAS hit/miss/tombstone tests (equality CAS, no seq bump)
- add requiresReEnroll routing test (non-fatal failure)
- add epoch upgrade → ipnsRecordRepository test (not schedule)
- rewrite enrollFolder tests for 2-arg scheduling-only signature
…urce + equality CAS
Task 1: getDueEntries JOIN + teeEntries rebuild (TEE-03)
- RepublishEntry: remove latestCid/sequenceNumber/currentEpoch/previousEpoch;
add signedRecord (base64 of ipns_records.signed_record)
- RepublishResult: add requiresReEnroll?: true
- getDueEntries returns Array<{schedule, record}> via two-step QB:
scheduleQB inner-joins ipns_records (tombstoned_at IS NULL + key IS NOT NULL),
then records fetched with same filter to guard against race window
- processRepublishBatch builds teeEntries entirely from paired record:
encryptedIpnsKey, keyEpoch, signedRecord — no schedule snapshot, no epoch scalars
Task 2: renewIpnsRecordEol equality CAS + epoch upgrade → ipns_records (TEE-04)
- replace syncIpnsRecordSequence (LessThanOrEqual write-back) with
renewIpnsRecordEol: QB UPDATE signed_record WHERE sequence_number = :expected
AND tombstoned_at IS NULL (equality CAS, EOL-only, does NOT change seq)
- affected === 0 → log debug + return (harmless discard, no throw)
- epoch upgrade writes encryptedIpnsPrivateKey + keyEpoch to ipns_records,
not the schedule
- requiresReEnroll → log + handleEntryFailure (non-fatal, Phase 68/69 deferred)
- schedule save contains ONLY scheduling fields (no crypto columns)
- remove teeKeyStateService from batch path (epoch now self-derived by TEE)
…ch validation getInternalCurrentEpoch: a non-numeric or non-positive EPOCH_ZERO_TIMESTAMP_MS made parseInt return NaN, which flowed through Math.max into a NaN epoch and the stale-key guard. Treat unset, NaN, and non-positive anchors identically as MIN_EPOCH. Adds malformed/non-positive anchor tests. createSubfolder: currentEpoch was only checked for finiteness, so a negative or fractional epoch could still be published. Require a positive integer (>= 1), consistent with the fail-closed enrollment intent. Addresses CodeRabbit findings on tee-keys.ts and registration.ts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 13ea2052c2cf
…dings ROADMAP: Phase 67 plan count 7/8 -> 8/8 (all plans complete). STATE: align Current focus and progress bar with the frontmatter (Phase 68, 7/9, 78%). Capture 9 deferred CodeRabbit findings as todos: republish write-path error handling + epoch-upgrade CAS, canonical encryptedIpnsPrivateKey rename, and the renewIpnsRecord later-EOL invariant + test-quality cluster. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 81dd50501911
|
Warning Review limit reached
Next review available in: 39 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughPhase 67 marks the TEE lease-renewer flow complete: the API now sources signing inputs from ChangesTEE Lease-Renewer Contract Rewrite
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
github-actions
Bot
added
release:api:feat
Contributor
Release Preview
Cascade Details
|
Greptile SummaryThis PR rewrites the TEE republish worker from a record originator into a pure lease-renewer: the TEE receives the marshaled
Confidence Score: 5/5The rewrite is structurally sound: verify-before-decrypt order is enforced, the relay cannot inject a CID or sequence, tombstone immutability is preserved at two layers, and all key-zeroing paths are covered. The only finding is a missing array-length equality check in the name↔key binding that is vacuously safe in practice (Ed25519 public keys are always 32 bytes) but is worth tightening. The core invariants — same CID, same sequence, later EOL, relay cannot inject signing inputs, stale keys trigger re-enrollment — are all correctly implemented and backed by 76 TEE-worker unit tests, 71 API tests, and a live round-trip e2e suite. The flagged hardening gap in the binding comparison cannot be exploited given normal Ed25519 key sizes and does not represent a current defect. apps/tee-worker/src/routes/republish.ts — binding check length guard is missing; all other changed files look correct. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Relay as Relay (republish.service.ts)
participant TEE as TEE Worker (republish.ts)
participant DB as ipns_records (PostgreSQL)
Relay->>DB: getDueEntries() — schedules JOIN ipns_records (tombstonedAt IS NULL, signedRecord/keyEpoch NOT NULL)
DB-->>Relay: "DueEntryPair[] {schedule, record}"
Relay->>TEE: "POST /republish [{encryptedIpnsKey, keyEpoch, ipnsName, signedRecord}]"
Note over TEE: 1. parseIpnsRecord(signedRecordBytes)
Note over TEE: 2. verifyIpnsRecordSignature — BEFORE decrypt
Note over TEE: 3. decryptWithFallback(encryptedKey, keyEpoch) — throws ReEnrollRequiredError if stale
Note over TEE: 4. binding: deriveEd25519PublicKey(key) == publicKeyFromIpnsName(name)
Note over TEE: 5. renewIpnsRecord — same value+seq, later EOL
Note over TEE: 6. reEncryptForEpoch if usedEpoch != internalCurrentEpoch
Note over TEE: 7. key.fill(0)
TEE-->>Relay: "RepublishResult {signedRecord, newSequenceNumber, upgradedEncryptedKey?}"
Relay->>Relay: "assert newSequenceNumber === record.sequenceNumber (TEE-02)"
Relay->>Relay: publishSignedRecord(ipnsName, signedRecord)
Relay->>DB: scheduleRepository.save(schedule) — scheduling fields only
Relay->>DB: "renewIpnsRecordEol WHERE sequence_number=:loaded AND tombstoned_at IS NULL"
alt epoch upgrade
Relay->>DB: "ipnsRecordRepository.update {encryptedIpnsPrivateKey, keyEpoch}"
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Relay as Relay (republish.service.ts)
participant TEE as TEE Worker (republish.ts)
participant DB as ipns_records (PostgreSQL)
Relay->>DB: getDueEntries() — schedules JOIN ipns_records (tombstonedAt IS NULL, signedRecord/keyEpoch NOT NULL)
DB-->>Relay: "DueEntryPair[] {schedule, record}"
Relay->>TEE: "POST /republish [{encryptedIpnsKey, keyEpoch, ipnsName, signedRecord}]"
Note over TEE: 1. parseIpnsRecord(signedRecordBytes)
Note over TEE: 2. verifyIpnsRecordSignature — BEFORE decrypt
Note over TEE: 3. decryptWithFallback(encryptedKey, keyEpoch) — throws ReEnrollRequiredError if stale
Note over TEE: 4. binding: deriveEd25519PublicKey(key) == publicKeyFromIpnsName(name)
Note over TEE: 5. renewIpnsRecord — same value+seq, later EOL
Note over TEE: 6. reEncryptForEpoch if usedEpoch != internalCurrentEpoch
Note over TEE: 7. key.fill(0)
TEE-->>Relay: "RepublishResult {signedRecord, newSequenceNumber, upgradedEncryptedKey?}"
Relay->>Relay: "assert newSequenceNumber === record.sequenceNumber (TEE-02)"
Relay->>Relay: publishSignedRecord(ipnsName, signedRecord)
Relay->>DB: scheduleRepository.save(schedule) — scheduling fields only
Relay->>DB: "renewIpnsRecordEol WHERE sequence_number=:loaded AND tombstoned_at IS NULL"
alt epoch upgrade
Relay->>DB: "ipnsRecordRepository.update {encryptedIpnsPrivateKey, keyEpoch}"
end
Reviews (6): Last reviewed commit: "fix(api): scope TEE republish pairing an..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #585 +/- ##
==========================================
+ Coverage 65.57% 70.83% +5.25%
==========================================
Files 151 180 +29
Lines 12318 22521 +10203
Branches 1380 1390 +10
==========================================
+ Hits 8078 15952 +7874
- Misses 4005 6335 +2330
+ Partials 235 234 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The tee-republish round-trip needs the TEE worker (:3002), the cipherbox DB, redis and BullMQ. CI's sdk-e2e job provisions none of these (it uses cipherbox_test and runs no TEE worker), so the module-level beforeAll DB query failed with 'database cipherbox does not exist'. Gate the suite and its beforeAll to local runs via describe.skipIf(!!process.env.CI); it remains the documented local publish gate (verified 2/2 locally). Todo filed to optionally provision a CI TEE worker. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: dbcd0ba66601
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
apps/api/src/tee/tee.service.ts (1)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse the canonical encrypted-key field name in this contract.
RepublishEntryis where this payload shape gets cemented, andencryptedIpnsKeydiverges from the repo standardencryptedIpnsPrivateKey. Keeping the shorter alias here will keep spreading the non-canonical name across the API/TEE boundary and tests. As per coding guidelines, "UseencryptedIpnsPrivateKeyfor encrypted IPNS private keys".🤖 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 `@apps/api/src/tee/tee.service.ts` around lines 13 - 21, Rename the RepublishEntry field in tee.service.ts from the non-canonical encryptedIpnsKey to encryptedIpnsPrivateKey, and update any related references in the TEE/API contract so this payload consistently uses the repo-standard name. Keep the rest of the RepublishEntry shape unchanged, and make sure any callers, tests, or serializers/deserializers that use this interface are aligned with the new field name.Source: Coding guidelines
apps/tee-worker/src/__tests__/republish.test.ts (1)
215-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
renewIpnsRecordreceives the original marshaled record.Because the mock always returns fixed bytes, this test still passes if the route starts rebuilding or mutating the record before renewal. A call-argument assertion here would make the suite actually guard the “renew the exact existing record” contract. As per path instructions, "
**/*.test.ts: Focus on test coverage, edge cases, and test quality. Ensure tests are meaningful and not just for coverage metrics."♻️ Suggested assertion
it('processes a single entry successfully and returns valid base64 signedRecord', async () => { const { encryptedIpnsKey, keyEpoch, ipnsName, signedRecord } = await makeEntry({}); + const { renewIpnsRecord } = await import('../services/ipns-signer.js'); const app = await createTestApp(); const res = await postJson(app, '/republish', { entries: [{ encryptedIpnsKey, keyEpoch, ipnsName, signedRecord }], }); expect(res.status).toBe(200); const results = res.body.results as Array<Record<string, unknown>>; expect(results[0].success).toBe(true); + expect(vi.mocked(renewIpnsRecord)).toHaveBeenCalledWith( + expect.any(Uint8Array), + Buffer.from(signedRecord, 'base64') + ); // signedRecord is the base64 of the mock's [5, 6, 7, 8] const decoded = Buffer.from(results[0].signedRecord as string, 'base64'); expect(decoded.length).toBeGreaterThan(0); });🤖 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 `@apps/tee-worker/src/__tests__/republish.test.ts` around lines 215 - 229, The single-entry republish test only checks the returned base64 output, so it can miss regressions where the route mutates the record before renewal. Update the test around the republish flow to assert that renewIpnsRecord is called with the original marshaled signedRecord produced by makeEntry, using the existing createTestApp and postJson setup to verify the exact argument passed into the renewal path.Source: Path instructions
🤖 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 `@apps/api/src/republish/republish.service.ts`:
- Around line 197-211: The epoch-upgrade path in republish.service.ts is
overwriting ipns_records without the same concurrency guard used elsewhere, so
an older batch can clobber newer key material. Update the write in the
epoch-upgrade branch near the logger.log and ipnsRecordRepository.update call to
include the current row version from record (sequence_number) and only update
when tombstoned_at is null, matching the protection pattern used by
renewIpnsRecordEol(). If the guarded update affects no rows, treat it as a stale
write and skip the upgrade.
- Around line 268-284: Refreshing an existing enrollment in republish.service.ts
should reactivate the schedule row, not just update nextRepublishAt. In the
existing branch of the enrollment logic, also reset the status from stale to
active and clear failure state fields like consecutiveFailures and lastError
before saving the existing schedule so getDueEntries() can pick it up again. Use
the existing enrollment flow around the existing check and
scheduleRepository.save(existing) to apply the reset consistently.
- Around line 173-195: The republish flow in RepublishService currently only
checks that result.newSequenceNumber exists before calling publishSignedRecord
and renewIpnsRecordEol, so a forward/regressed TEE response can be published
even when it does not match the loaded record sequence. Add an explicit “same
sequence” guard in the success path of the republish handler to compare the
loaded record.sequenceNumber against the TEE-returned sequence before
publishing, and abort the publish/update path if they differ. Use the existing
symbols publishSignedRecord, renewIpnsRecordEol, and result.newSequenceNumber in
RepublishService to locate and enforce this invariant.
In `@apps/tee-worker/src/__tests__/ipns-signer.test.ts`:
- Around line 61-69: The renewal test only checks that the serialized IPNS
record bytes differ, which doesn’t verify the lease-renew behavior. Update the
`renewIpnsRecord` test in `ipns-signer.test.ts` to decode both the `original`
and `renewed` records and assert that the expiry/EOL (validity) value is
actually moved forward. Use the existing `makeOriginalRecord` and
`renewIpnsRecord` helpers to locate the assertion, and keep the byte-equality
check only as a secondary guard if needed.
In `@apps/tee-worker/src/routes/republish.ts`:
- Around line 57-61: Rename the RepublishEntry contract field from
encryptedIpnsKey to encryptedIpnsPrivateKey to match the canonical IPNS private
key naming. Update the interface in republish.ts and any related relay↔TEE
payload handling or serialization/deserialization that references RepublishEntry
so the new field name is used consistently across the boundary.
In `@packages/sdk-core/src/folder/registration.test.ts`:
- Around line 156-168: Add a fail-closed test in registration.test.ts for
invalid teeKeys.currentEpoch alongside the existing createSubfolder empty
currentPublicKey case, using the createSubfolder and
mockFns.createAndPublishIpnsRecord symbols to verify it rejects when
currentEpoch is 0, NaN, or non-integer. Keep the assertion that publishing is
not reached, and mirror the current test style so the new case clearly covers
the currentEpoch validation branch without changing production code.
In `@packages/sdk-core/src/folder/registration.ts`:
- Around line 92-114: The TEE enrollment checks in createSubfolder are happening
too late, after the node has already been sealed and uploaded, which allows side
effects before a fail-closed error. Move the teeKeys validation and wrapKey flow
earlier in registration.ts so createSubfolder verifies currentPublicKey and
currentEpoch before any IPFS upload work begins, then only proceed with the
upload path once the TEE fields are confirmed valid.
In `@tests/sdk-e2e/src/suites/tee-republish.test.ts`:
- Around line 137-141: The `enqueueRepublishBatch` helper leaks the Redis
connection if `queue.add` fails because `queue.close()` is only reached on
success. Update `enqueueRepublishBatch` to ensure the `Queue` instance is always
closed by wrapping the `queue.add` call in a `try/finally` (or equivalent
cleanup path) so the `republish` queue is closed even when `queue.add` throws.
---
Nitpick comments:
In `@apps/api/src/tee/tee.service.ts`:
- Around line 13-21: Rename the RepublishEntry field in tee.service.ts from the
non-canonical encryptedIpnsKey to encryptedIpnsPrivateKey, and update any
related references in the TEE/API contract so this payload consistently uses the
repo-standard name. Keep the rest of the RepublishEntry shape unchanged, and
make sure any callers, tests, or serializers/deserializers that use this
interface are aligned with the new field name.
In `@apps/tee-worker/src/__tests__/republish.test.ts`:
- Around line 215-229: The single-entry republish test only checks the returned
base64 output, so it can miss regressions where the route mutates the record
before renewal. Update the test around the republish flow to assert that
renewIpnsRecord is called with the original marshaled signedRecord produced by
makeEntry, using the existing createTestApp and postJson setup to verify the
exact argument passed into the renewal path.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5e9df46-3381-44a8-b84e-d1181a7ed58c
⛔ Files ignored due to path filters (2)
.claude/commands/ship-phase.mdis excluded by!.claude/commands/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (53)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/66-api-schema-cutover-publish-gate-and-tombstone/66-LEARNINGS.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-01-PLAN.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-01-SUMMARY.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-02-PLAN.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-02-SUMMARY.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-03-PLAN.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-03-SUMMARY.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-04-PLAN.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-04-SUMMARY.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-05-PLAN.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-05-SUMMARY.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-06-PLAN.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-06-SUMMARY.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-07-PLAN.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-07-SUMMARY.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-08-PLAN.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-08-SUMMARY.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-CONTEXT.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-DISCUSSION-LOG.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-PATTERNS.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-RESEARCH.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-SECURITY.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-VALIDATION.md.planning/phases/67-tee-lease-renewer-contract-rewrite/67-VERIFICATION.md.planning/todos/completed/2026-06-29-createsubfolder-tee-republish-wiring.md.planning/todos/pending/2026-07-01-rename-encrypted-ipns-key-canonical-field.md.planning/todos/pending/2026-07-01-renew-ipns-record-eol-invariant-and-tests.md.planning/todos/pending/2026-07-01-tee-republish-writepath-error-handling-hardening.mdapps/api/.env.exampleapps/api/src/ipns/ipns.service.tsapps/api/src/migrations/1751000000000-ScheduleCollapse.tsapps/api/src/republish/republish-schedule.entity.tsapps/api/src/republish/republish.service.spec.tsapps/api/src/republish/republish.service.tsapps/api/src/tee/tee.service.spec.tsapps/api/src/tee/tee.service.tsapps/tee-worker/src/__tests__/ipns-signer.test.tsapps/tee-worker/src/__tests__/key-manager.test.tsapps/tee-worker/src/__tests__/republish.test.tsapps/tee-worker/src/__tests__/tee-keys.test.tsapps/tee-worker/src/routes/republish.tsapps/tee-worker/src/services/ipns-signer.tsapps/tee-worker/src/services/key-manager.tsapps/tee-worker/src/services/tee-keys.tsdocker/docker-compose.ymlpackages/sdk-core/src/folder/registration.test.tspackages/sdk-core/src/folder/registration.tsrelease-please-config.jsontests/sdk-e2e/package.jsontests/sdk-e2e/src/suites/tee-republish.test.ts
Addresses two greptile P1 findings on republish.service.ts:
- Epoch-upgrade ipnsRecordRepository.update was scoped only by ipnsName, so it
could re-encrypt a tombstoned row (violating tombstone immutability) or touch
another user's row sharing the name. Scope it to
{ ipnsName, userId, tombstonedAt: IsNull(), keyEpoch: <loaded> } — tombstone
guard + owner scope + epoch CAS (also closes the concurrent-rotation clobber).
- getDueEntries filtered only encryptedIpnsPrivateKey non-null, so a record with
a null signedRecord/keyEpoch made the teeEntries map deref null with '!' OUTSIDE
the inner try, aborting the whole batch. Filter signedRecord + keyEpoch non-null
so an incomplete row drops out instead of crashing the batch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Entire-Checkpoint: 8d0200ea5de5
…ivation, fail-fast - republish.service: reject before publishing when the TEE returns a sequence != the loaded one (defense-in-depth for TEE-02; prevents delegated routing drifting ahead of the CAS-guarded ipns_records row). - enrollFolder: reactivate an existing schedule row on re-enroll (reset status, consecutiveFailures, lastError) so a previously-failed folder rejoins the batch. - createSubfolder: move TEE enrollment validation + wrap BEFORE the seal/IPFS upload so a malformed teeKeys fails closed without leaving an orphaned blob. - registration.test: add invalid-currentEpoch fail-closed case (0/NaN/1.5/-1). - tee-republish e2e: try/finally around queue.close to avoid a Redis leak. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: de6a6732976e
31 learnings mined from the phase artifacts: 10 decisions, 8 lessons, 8 patterns, 5 surprises (incl. the grep-AC-forced query-builder TypeORM metadata bug that only the sdk-e2e round-trip caught). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 01adc5658e8b
Provision the full TEE stack in the sdk-e2e job so the tee-republish lease-renewer round-trip is exercised in CI, not just locally. - Build the tee-worker and start it headless in simulator mode on :3002 before the API, so TeeModule.onModuleInit can populate tee_key_state (the bootstrap is a one-shot that swallows errors, so ordering matters). - Wire TEE_WORKER_URL + a shared TEE_WORKER_SECRET onto the API. - Export DB_DATABASE=cipherbox_test and REDIS_PORT=6379 onto the test step so the suite's DB/redis config hits the CI stack instead of local defaults. - Drop the SKIP_TEE_LIVE=!!CI gate in tee-republish.test.ts; the schedule-collapse migration is applied by the API's migrationsRun on boot. Resolves the deferred CI todo from the Phase 67 ship. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 30b7a3c115a3
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/republish/republish.service.ts (1)
74-88: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope republish record pairing and renewal by
userId.
getDueEntries()maps records byipnsNameonly, andrenewIpnsRecordEol()updates byipns_nameonly. Since Line 226 saysipnsNameuniqueness is app-level rather than database-enforced, duplicate names can pair or renew another user’s row. CarryuserIdthrough the lookup and CAS update. As per path instructions,apps/api/**should focus on security vulnerabilities and API contract consistency.Suggested fix
- const ipnsNames = schedules.map((s) => s.ipnsName); + const pairKey = (userId: string, ipnsName: string) => `${userId}:${ipnsName}`; + const ipnsNames = [...new Set(schedules.map((s) => s.ipnsName))]; + const userIds = [...new Set(schedules.map((s) => s.userId))]; const records = await this.ipnsRecordRepository.find({ where: { + userId: In(userIds), ipnsName: In(ipnsNames), tombstonedAt: IsNull(), encryptedIpnsPrivateKey: Not(IsNull()), @@ - const recordMap = new Map(records.map((r) => [r.ipnsName, r])); + const recordMap = new Map(records.map((r) => [pairKey(r.userId, r.ipnsName), r])); @@ - const record = recordMap.get(schedule.ipnsName); + const record = recordMap.get(pairKey(schedule.userId, schedule.ipnsName));await this.renewIpnsRecordEol( schedule.ipnsName, + schedule.userId, record.sequenceNumber, // loaded seq from the batch Buffer.from(result.signedRecord, 'base64') ); @@ private async renewIpnsRecordEol( ipnsName: string, + userId: string, loadedSequenceNumber: string, renewedSignedRecord: Buffer ): Promise<void> { @@ - .where('ipns_name = :ipnsName AND sequence_number = :expected AND tombstoned_at IS NULL', { + .where('ipns_name = :ipnsName AND user_id = :userId AND sequence_number = :expected AND tombstoned_at IS NULL', { ipnsName, + userId, expected: loadedSequenceNumber, })Also applies to: 209-213, 449-462
🤖 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 `@apps/api/src/republish/republish.service.ts` around lines 74 - 88, getDueEntries() and renewIpnsRecordEol() currently key only on ipnsName, which can pair or update another user’s record when names collide. Carry userId through the record lookup and renewal flow in RepublishService, and include it in the Map key and the optimistic update/CAS criteria. Update the repository query and any downstream accessors so the selected record is always matched by both ipnsName and userId, preserving API/security isolation.Source: Path instructions
🧹 Nitpick comments (1)
apps/api/src/republish/republish.service.ts (1)
134-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse the canonical
encryptedIpnsPrivateKeyfield name.Line 135 still sends an encrypted IPNS private key as
encryptedIpnsKey; the repo convention requiresencryptedIpnsPrivateKey. Rename the TEE republish DTO/worker consumer together to keep the contract consistent. As per coding guidelines, “UseencryptedIpnsPrivateKeyfor encrypted IPNS private keys.”🤖 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 `@apps/api/src/republish/republish.service.ts` around lines 134 - 138, The republish DTO is using the non-canonical encrypted IPNS private key field name, so update the mapping in republish.service.ts where teeEntries is built to use encryptedIpnsPrivateKey instead of encryptedIpnsKey. Make sure the TEE republish contract is kept consistent by renaming the corresponding DTO/worker consumer field to match, and verify any references to RepublishEntry align with the canonical encryptedIpnsPrivateKey name.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.
Outside diff comments:
In `@apps/api/src/republish/republish.service.ts`:
- Around line 74-88: getDueEntries() and renewIpnsRecordEol() currently key only
on ipnsName, which can pair or update another user’s record when names collide.
Carry userId through the record lookup and renewal flow in RepublishService, and
include it in the Map key and the optimistic update/CAS criteria. Update the
repository query and any downstream accessors so the selected record is always
matched by both ipnsName and userId, preserving API/security isolation.
---
Nitpick comments:
In `@apps/api/src/republish/republish.service.ts`:
- Around line 134-138: The republish DTO is using the non-canonical encrypted
IPNS private key field name, so update the mapping in republish.service.ts where
teeEntries is built to use encryptedIpnsPrivateKey instead of encryptedIpnsKey.
Make sure the TEE republish contract is kept consistent by renaming the
corresponding DTO/worker consumer field to match, and verify any references to
RepublishEntry align with the canonical encryptedIpnsPrivateKey name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08bd2462-c565-4327-821a-0d59f1988c5f
📒 Files selected for processing (9)
.github/workflows/ci.yml.planning/phases/67-tee-lease-renewer-contract-rewrite/67-LEARNINGS.md.planning/todos/completed/2026-07-01-ci-tee-worker-for-tee-republish-e2e.md.planning/todos/pending/2026-07-01-tee-republish-writepath-error-handling-hardening.mdapps/api/src/republish/republish.service.spec.tsapps/api/src/republish/republish.service.tspackages/sdk-core/src/folder/registration.test.tspackages/sdk-core/src/folder/registration.tstests/sdk-e2e/src/suites/tee-republish.test.ts
✅ Files skipped from review due to trivial changes (2)
- .planning/todos/pending/2026-07-01-tee-republish-writepath-error-handling-hardening.md
- .planning/phases/67-tee-lease-renewer-contract-rewrite/67-LEARNINGS.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/sdk-core/src/folder/registration.test.ts
- packages/sdk-core/src/folder/registration.ts
- tests/sdk-e2e/src/suites/tee-republish.test.ts
- apps/api/src/republish/republish.service.spec.ts
Address CodeRabbit review on PR #585. getDueEntries paired schedules to ipns_records by ipnsName only, and renewIpnsRecordEol CASed on ipns_name + sequence_number only. Since ipnsName uniqueness is app-level (not a DB constraint), a name shared across owners could pair or renew another user's row. Carry userId through both, matching the epoch-upgrade write that was already userId-scoped during the ship. - getDueEntries: key the record map by userId+ipnsName and filter the ipns_records query by userId. - renewIpnsRecordEol: add user_id to the equality CAS. - Tests: cross-user records no longer pair; the renewal CAS asserts userId. The CodeRabbit nitpick to rename the encryptedIpnsKey wire field is deferred to the write-path hardening todo (cross-package rename, own scoped change). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: de91c92602bf
Owner
Author
CodeRabbit review addressed (latest review, commit
|
FSM1
added a commit
that referenced
this pull request
Jul 1, 2026
chore: remove duplicate encryptedIpnsKey rename todo entry The encryptedIpnsKey -> encryptedIpnsPrivateKey wire-contract rename is already tracked by the dedicated todo 2026-07-01-rename-encrypted-ipns-key-canonical-field.md (which also covers upgradedEncryptedKey). Finding #4 was added to the write-path hardening todo during PR #585 review resolution before that dedicated todo was noticed, duplicating the item. Remove the redundant entry. Entire-Checkpoint: ea2585e9cec3 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 67 — TEE Lease-Renewer Contract Rewrite
Reshapes the TEE worker from a record originator into a pure record lease-renewer. The TEE receives a marshaled
signedRecord, verifies its signature in-enclave, and re-emits the same CID and same sequence with only a later EOL — it can no longer originate or repoint a CID, nor increment the sequence.Requirements: TEE-01, TEE-02, TEE-03, TEE-06
What changed
apps/tee-worker/src/routes/republish.ts): parse → verify Ed25519 signature before decryption → decrypt via internal-epoch fallback → assert name↔key binding → re-sign same value+sequence with later EOL → zero key on every path. The+1nincrement is gone;newSequenceNumber == parsed.sequence.renewIpnsRecord(ipns-signer.ts): takes only(ed25519PrivateKey, marshaledExistingRecord)— nocid/sequenceargs, so the relay cannot inject either. Value and sequence come exclusively from the parsed existing record.tee-keys.tsgetInternalCurrentEpoch): epoch derived from the TEE's own clock, never a relay-supplied scalar.decryptWithFallbackreshaped to 2 args; stale epoch-N-2 keys throwReEnrollRequiredErrorbefore any unwrap.deriveEd25519PublicKey(decryptedKey)byte-compared topublicKeyFromIpnsName(ipnsName);parsed.pubKeyis never trusted (undefined for Ed25519 identity records).ScheduleCollapsemigration + entity): drops the 4 duplicated signing-input columns (encrypted_ipns_key,key_epoch,latest_cid,sequence_number). The canonicalipns_recordsrow is now the sole source of signing inputs.republish.service.tsrenewIpnsRecordEol):WHERE sequence_number = :loaded AND tombstoned_at IS NULL— cannot regress the sequence or resurrect a tombstoned name. Two-layer tombstone filter (pre-batch + write).createSubfolderTEE wiring (sdk-core/registration.ts): ECIES-wraps the IPNS key under the TEE public key and enrolls new subfolders; fail-closed on incompleteteeKeys.docker-compose.yml): tee-worker service on:3002; sdk-e2ebullmq/pgdevDeps.Ship-pass changes (this PR, on top of the executed phase)
9fce9c8f— removed deadsignIpnsRecord(the old CID-originating primitive the rewrite superseded; a footgun that could mint an arbitrary CID/sequence) and embedded theinformation_schemacolumn-drop assertion in the tee-republish suitebeforeAll(T-67-08-T).526c900d—getInternalCurrentEpochtreats a malformed/non-positiveEPOCH_ZERO_TIMESTAMP_MSasMIN_EPOCH(a non-numeric anchor previously produced aNaNepoch);createSubfolderrequires a positive-integercurrentEpoch.da9f298/1624f17— gate the tee-republish sdk-e2e suite to the local live stack (skipIf(CI)); CI provisions no TEE worker /cipherboxDB, so the round-trip is the documented local publish gate (verified 2/2 locally).53dd036— greptile P1s: epoch-upgrade write now scoped to{ ipnsName, userId, tombstonedAt: IsNull(), keyEpoch }(tombstone immutability + owner scope + epoch CAS);getDueEntriesfilterssignedRecord/keyEpochnon-null to prevent a null deref aborting the whole batch.76b1517f— CodeRabbit PR review: reject publish when the TEE returns a different sequence (TEE-02 defense-in-depth); reactivate an existing schedule row on re-enroll; move TEE validation before the IPFS upload (fail-fast, no orphaned blob); + test hardening.eaa37faf,b4c04c3,3f80a8e— security + validation + learnings docs; ROADMAP/STATE drift fixes; deferred-findings todos.Gates
threats_open: 0; all 3 CRITICAL mitigations enforced in codeDeferred (todos filed under
.planning/todos/pending/)encryptedIpnsPrivateKeyrename — cross-layer wire-contract rename of the pre-existingencryptedIpnsKey/upgradedEncryptedKeyfields (CodeRabbit-labeled Heavy lift).renewIpnsRecordlater-EOL invariant + test hardening —ParsedIpnsRecorddoesn't expose validity, so a direct EOL assertion needs a crypto-package change; the invariant holds by construction today.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes