diff --git a/server/api/github/webhook.post.ts b/server/api/github/webhook.post.ts index 65031ef..2e7dbe4 100644 --- a/server/api/github/webhook.post.ts +++ b/server/api/github/webhook.post.ts @@ -10,6 +10,7 @@ import { verify } from '@octokit/webhooks-methods' import { sql } from 'drizzle-orm' import { installation, webhookEvent } from '~~/server/db/schema' import { enqueue } from '~~/server/utils/queue' +import { revokeKeysForInstallation } from '~~/server/utils/tangled-pubkey' const RECOGNISED_EVENTS = new Set([ 'push', @@ -86,9 +87,10 @@ export default defineEventHandler(async event => { }).onConflictDoNothing({ target: installation.id }) } else if (action === 'deleted') { + // Revoke the user's sh.tangled.publicKey PDS records first; the cascade + // below drops the local ssh_key rows that hold the rkeys we need. + await revokeKeysForInstallation(body.installation.id) // installation row deletion cascades to user_identity, ssh_key, repo_mapping. - // The corresponding sh.tangled.publicKey record on the user's PDS is revoked - // in commit 15. await db.delete(installation).where(sql`${installation.id} = ${body.installation.id}`) } else if (action === 'suspend') { diff --git a/server/utils/tangled-pubkey.ts b/server/utils/tangled-pubkey.ts index dad5bd6..776d6cf 100644 --- a/server/utils/tangled-pubkey.ts +++ b/server/utils/tangled-pubkey.ts @@ -2,6 +2,7 @@ import { Agent } from '@atproto/api' import type { OAuthSession } from '@atproto/oauth-client-node' import { sql } from 'drizzle-orm' import { sshKey } from '../db/schema' +import { useOAuthClient } from './atproto-oauth' import { useDb } from './db' import { encrypt } from './encryption' import { generateKeypair } from './ssh-keypair' @@ -115,3 +116,45 @@ export async function rotateKey(opts: { return generateAndPublishKey(opts) } + +/** + * Delete every `sh.tangled.publicKey` record we published for an installation + * from the owning user's PDS. + * + * Called on `installation.deleted` *before* the local cascade delete, so the + * user isn't left with a dead key to clean up by hand. Restoring the OAuth + * session can fail if the user revoked the app on their PDS before uninstalling + * the GitHub App; in that case there's nothing for us to delete, so we swallow + * the error and let the caller proceed with the local cascade. A 404 on the + * delete itself is likewise treated as already-gone. + */ +export async function revokeKeysForInstallation(installationId: number): Promise { + const db = useDb() + const rows = await db.select({ did: sshKey.did, rkey: sshKey.tangledKeyRkey }) + .from(sshKey) + .where(sql`${sshKey.installationId} = ${installationId}`) + + const client = await useOAuthClient() + + for (const row of rows) { + if (!row.rkey) continue + try { + // eslint-disable-next-line no-await-in-loop -- one PDS session per row + const session = await client.restore(row.did) + const agent = new Agent(session) + // eslint-disable-next-line no-await-in-loop -- sequential PDS deletes + await agent.com.atproto.repo.deleteRecord({ + repo: row.did, + collection: PUBKEY_LEXICON, + rkey: row.rkey, + }) + } + catch (err) { + const status = err && typeof err === 'object' && 'status' in err && typeof err.status === 'number' + ? err.status + : undefined + if (status === 404) continue + console.error(`failed to revoke publicKey record for did ${row.did} (installation ${installationId})`, err) + } + } +} diff --git a/test/unit/tangled-pubkey.spec.ts b/test/unit/tangled-pubkey.spec.ts index 64d02ce..8cfdf92 100644 --- a/test/unit/tangled-pubkey.spec.ts +++ b/test/unit/tangled-pubkey.spec.ts @@ -4,13 +4,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { installation, sshKey } from '../../server/db/schema' import { clearDb, setDb, useDb } from '../../server/utils/db' import { clearEncryptionKeyCache, decrypt } from '../../server/utils/encryption' -import { generateAndPublishKey, rotateKey } from '../../server/utils/tangled-pubkey' import { createTestDb } from '../utils/db' const ORIGINAL_ENC_KEY = process.env.NUXT_ENCRYPTION_KEY const createRecordMock = vi.fn<(input: { repo: string, collection: string, record: Record }) => Promise<{ data: { uri: string, cid: string } }>>() const deleteRecordMock = vi.fn<(input: { repo: string, collection: string, rkey: string }) => Promise>() +const restoreMock = vi.fn<(did: string) => Promise<{ did: string }>>() vi.mock('@atproto/api', () => ({ Agent: class { @@ -25,6 +25,12 @@ vi.mock('@atproto/api', () => ({ }, })) +vi.mock('../../server/utils/atproto-oauth', () => ({ + useOAuthClient: async () => ({ restore: restoreMock }), +})) + +const { generateAndPublishKey, revokeKeysForInstallation, rotateKey } = await import('../../server/utils/tangled-pubkey') + function fakeOauthSession(did: string) { // The Agent mock above ignores its constructor argument, so we only need // a `.did` field for the helper itself. @@ -232,3 +238,74 @@ describe('rotateKey', () => { expect(rows).toHaveLength(1) }) }) + +describe('revokeKeysForInstallation', () => { + beforeEach(async () => { + process.env.NUXT_ENCRYPTION_KEY = crypto.randomBytes(32).toString('base64') + clearEncryptionKeyCache() + + setDb(await createTestDb()) + await useDb().insert(installation).values({ + id: 1, accountLogin: 'alice', accountId: 100, accountType: 'User', + }) + + createRecordMock.mockReset() + deleteRecordMock.mockReset() + restoreMock.mockReset() + createRecordMock.mockResolvedValue({ + data: { uri: 'at://did:plc:abc/sh.tangled.publicKey/3kh2y4xq2lk2v', cid: 'bafy' }, + }) + deleteRecordMock.mockResolvedValue({}) + restoreMock.mockImplementation(async (did: string) => ({ did })) + }) + + afterEach(() => { + if (ORIGINAL_ENC_KEY === undefined) delete process.env.NUXT_ENCRYPTION_KEY + else process.env.NUXT_ENCRYPTION_KEY = ORIGINAL_ENC_KEY + clearEncryptionKeyCache() + clearDb() + }) + + it('deletes the publicKey PDS record for the installation', async () => { + await generateAndPublishKey({ + oauthSession: fakeOauthSession('did:plc:abc'), + installationId: 1, + }) + + await revokeKeysForInstallation(1) + + expect(restoreMock).toHaveBeenCalledWith('did:plc:abc') + expect(deleteRecordMock).toHaveBeenCalledTimes(1) + const del = deleteRecordMock.mock.calls[0][0] + expect(del.repo).toBe('did:plc:abc') + expect(del.collection).toBe('sh.tangled.publicKey') + expect(del.rkey).toBe('3kh2y4xq2lk2v') + }) + + it('no-ops when the installation has no keys', async () => { + await revokeKeysForInstallation(1) + expect(restoreMock).not.toHaveBeenCalled() + expect(deleteRecordMock).not.toHaveBeenCalled() + }) + + it('swallows a 404 from the PDS delete', async () => { + await generateAndPublishKey({ + oauthSession: fakeOauthSession('did:plc:abc'), + installationId: 1, + }) + deleteRecordMock.mockRejectedValueOnce(Object.assign(new Error('not found'), { status: 404 })) + + await expect(revokeKeysForInstallation(1)).resolves.toBeUndefined() + }) + + it('continues when OAuth session restoration fails', async () => { + await generateAndPublishKey({ + oauthSession: fakeOauthSession('did:plc:abc'), + installationId: 1, + }) + restoreMock.mockRejectedValueOnce(new Error('session gone')) + + await expect(revokeKeysForInstallation(1)).resolves.toBeUndefined() + expect(deleteRecordMock).not.toHaveBeenCalled() + }) +})