THU-430: Missing canary proof-of-CK-possession on device revocation enables E2EE state reset#632
Conversation
…tion - extract canary hashing/verification into shared backend/src/lib/canary.ts - revoke endpoint now requires X-Device-ID header and canarySecret body when E2EE is active - verify caller is a trusted device before allowing revocation (defense-in-depth) - block PowerSync upload DELETE for devices table to force use of dedicated API - add re-bootstrap canary check in first-device-bootstrap flow - add frontend revokeDevice API helper and wire up use-revoke-device hook - expand test coverage: canary proof, pre-encryption path, missing header, untrusted device
raivieiraadriano92
changed the title
Semgrep Security ScanNo security issues found. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
This comment was marked as outdated.
This comment was marked as outdated.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Full attack chain test: stolen session cannot revoke devices to reset E2EE state - Defense-in-depth: re-bootstrap blocked with wrong canary when metadata exists - Defense-in-depth: re-bootstrap allowed with correct canary (recovery flow) - PowerSync upload DELETE on devices table blocked Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion - gracefully handle missing canary when E2EE is not set up - allow revokeDevice API to omit canarySecret for pre-E2EE users
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 08e9ee2. Configure here.
| canarySecret = await extractCanarySecret(httpClient) | ||
| } catch { | ||
| // E2EE not set up (fetchCanary 404) or CK unavailable — proceed without proof | ||
| } |
There was a problem hiding this comment.
Catch-all swallows real errors for E2EE users
Medium Severity
The bare catch in revokeDeviceWithProof swallows every error from extractCanarySecret, not just the expected fetchCanary 404 that indicates a pre-E2EE user. For E2EE users hitting transient failures (network timeouts, server 500s, crypto API errors, corrupted CK), the real error is silently discarded and the revoke proceeds without proof — the backend then returns a confusing 403 "Canary secret required" instead of surfacing the actual problem. Compare with denyDeviceWithProof, which correctly lets errors propagate. The catch here needs to distinguish the pre-E2EE case (e.g., HttpError with status 404) from unexpected failures and rethrow the latter.
Reviewed by Cursor Bugbot for commit 08e9ee2. Configure here.
ReviewSecurity
The bare Breaking Change
Previously optional; now returns 400 if absent. Existing deployed clients (mobile app versions in the field) that call Convention
|
| let canarySecret: string | undefined | ||
| try { | ||
| canarySecret = await extractCanarySecret(httpClient) | ||
| } catch { | ||
| // E2EE not set up (fetchCanary 404) or CK unavailable — proceed without proof | ||
| } | ||
| await revokeDeviceApi(httpClient, deviceId, canarySecret) |
There was a problem hiding this comment.
Two issues here: (1) the bare catch {} swallows everything — a CK cache miss or network error is indistinguishable from "E2EE not set up", so the backend ends up returning a confusing 403 instead of the real error; (2) let + assignment inside try violates CLAUDE.md's const-over-let rule.
Fix both at once — catch only 404, and eliminate the let:
| let canarySecret: string | undefined | |
| try { | |
| canarySecret = await extractCanarySecret(httpClient) | |
| } catch { | |
| // E2EE not set up (fetchCanary 404) or CK unavailable — proceed without proof | |
| } | |
| await revokeDeviceApi(httpClient, deviceId, canarySecret) | |
| const canarySecret = await extractCanarySecret(httpClient).catch((err: unknown) => { | |
| if (err instanceof Error && 'status' in err && (err as { status: number }).status === 404) return undefined | |
| throw err | |
| }) |
| const callerDeviceId = request.headers.get('x-device-id')?.trim() | ||
| if (!callerDeviceId) { | ||
| set.status = 400 | ||
| return { error: 'X-Device-ID header is required' } | ||
| } |
There was a problem hiding this comment.
This is a breaking change for existing clients that call this endpoint without X-Device-ID. Old app versions in the field will start receiving 400 instead of 204. Worth coordinating a client-version gate or making the header optional when metadata is null (pre-E2EE users don't need the caller device check anyway).
1 participant
Summary
POST /account/devices/:id/revoke) now requires canary proof-of-CK-possession when E2EE is active, closing an attack vector where a stolen session could reset all encryption stateDELETEoperations on thedevicestable via PowerSync upload — devices can only be revoked through the dedicated API endpointRoot cause
The revoke endpoint deleted envelopes without requiring canary proof, unlike approve/deny which both require it. An attacker with a stolen session could revoke all devices (deleting all envelopes), then re-bootstrap as first device with attacker-controlled keys — fully resetting E2EE state without ever possessing the Content Key.
Changes
Backend
hashCanarySecret,verifyCanaryProof,verifyCanaryProofWithMetadatainto sharedbackend/src/lib/canary.tsX-Device-IDheader +canarySecretbody (when encryption metadata exists)uploadDenyDeleteset in PowerSync upload handler blocking device hard-deletesFrontend
revokeDeviceAPI function (src/api/encryption.ts)revokeDeviceWithProofservice function (mirrorsdenyDeviceWithProofpattern)useRevokeDevicehook to extract canary proof before calling APITest plan
🤖 Generated with Claude Code
Note
High Risk
Changes enforcement on device revocation and first-device bootstrap in E2EE flows; mistakes could either reintroduce an account-takeover vector or block legitimate device recovery/revocation.
Overview
Closes an E2EE state-reset attack by tightening
POST /account/devices/:id/revoke: the endpoint now requiresX-Device-IDand, when encryption metadata exists, a validcanarySecretplus a trusted-caller device check before deleting envelopes/revoking devices.Adds defense-in-depth in the encryption bootstrap path to block “first device” re-bootstrap when encryption metadata already exists unless the submitted canary secret matches existing metadata, and centralizes canary hashing/verification into new shared
backend/src/lib/canary.ts.Hardens PowerSync uploads by denying
DELETEoperations on thedevicestable (forcing use of the revoke API), and updates frontend revoke flows to automatically include canary proof via newrevokeDeviceAPI +revokeDeviceWithProofservice and rewireduseRevokeDevicehook; tests are expanded to cover the new security behaviors and backwards-compat pre-E2EE revocation.Reviewed by Cursor Bugbot for commit 08e9ee2. Bugbot is set up for automated code reviews on this repo. Configure here.