From cbd65b1ea77a8837cc5866a5cbf854b72d64f39e Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 07:13:38 -0700 Subject: [PATCH 01/13] feat(users): add re-authenticated account deletion Delete the signed-in human and their uploaded bytes and metadata, revoke owned credentials and pause owned runtime projections without deleting agent identity or memory. Keep shared chat history with an anonymized PostgreSQL author row. Require a password or explicit fresh OAuth sign-in; keep failed cleanup retryable. Keep minimal deletion intents and retry late upload cleanup, access revocation and chat-author redaction after the human row is gone. Ownership namespaces find bytes even when a concurrent upload loses its metadata. Service tests cover real Mongo/PostgreSQL deletion, token revocation, isolated ownership, agent continuity, OAuth freshness and storage/billing/PG failures. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../__tests__/service/accountDeletion.test.js | 228 ++++++++++++++++++ .../unit/routes/uploads.post.test.js | 4 + .../services/objectStore/mongoDriver.test.ts | 12 + .../schedulerService.assignedTasks.test.js | 5 + backend/__tests__/utils/testUtils.js | 3 +- backend/controllers/oauthController.ts | 2 +- backend/controllers/userController.ts | 2 +- backend/models/AccountDeletion.ts | 16 ++ backend/models/File.ts | 1 + backend/models/User.ts | 2 + backend/routes/uploads.ts | 20 +- backend/routes/users.ts | 31 +++ backend/services/accountDeletionService.ts | 127 ++++++++++ backend/services/objectStore/ObjectStore.ts | 3 + .../objectStore/drivers/mongoDriver.ts | 7 + backend/services/schedulerService.ts | 19 ++ backend/services/sessionTokenService.ts | 2 + docs/api/account-deletion.md | 84 +++++++ 18 files changed, 564 insertions(+), 4 deletions(-) create mode 100644 backend/__tests__/service/accountDeletion.test.js create mode 100644 backend/models/AccountDeletion.ts create mode 100644 backend/services/accountDeletionService.ts create mode 100644 docs/api/account-deletion.md diff --git a/backend/__tests__/service/accountDeletion.test.js b/backend/__tests__/service/accountDeletion.test.js new file mode 100644 index 0000000000..f7d78fe443 --- /dev/null +++ b/backend/__tests__/service/accountDeletion.test.js @@ -0,0 +1,228 @@ +const express = require('express'); +const request = require('supertest'); +const jwt = require('jsonwebtoken'); +const { createHash } = require('crypto'); +const { setupMongoDb, closeMongoDb, clearMongoDb, setupPgDb, closePgDb, clearPgDb } = require('../utils/testUtils'); +let mockPg; +jest.mock('../../config/db-pg', () => ({ pool: { query: (...args) => mockPg.query(...args) } })); +const mockStripe = { subscriptions: { cancel: jest.fn().mockResolvedValue({ status: 'canceled' }) } }; +jest.mock('../../services/billingService', () => ({ stripe: () => mockStripe })); +const User = require('../../models/User'); +const File = require('../../models/File'); +const MediaObject = require('../../models/MediaObject'); +const DeviceAuthorization = require('../../models/DeviceAuthorization'); +const OAuthLoginState = require('../../models/OAuthLoginState'); +const AgentCredential = require('../../models/AgentCredential'); +const AgentMemory = require('../../models/AgentMemory'); +const { AgentInstallation } = require('../../models/AgentRegistry'); +const { sweepAccountDeletions } = require('../../services/accountDeletionService'); +const AccountDeletion = require('../../models/AccountDeletion'); +const { issueSessionToken } = require('../../services/sessionTokenService'); +const { getObjectStore } = require('../../services/objectStore'); +const oauthController = require('../../controllers/oauthController'); +const routes = require('../../routes/users'); +const uploads = require('../../routes/uploads'); +const password = 'Delete-this-account-123!'; +const makeUser = (fields = {}) => User.create({ username: `person-${Math.random().toString(16).slice(2)}`, + email: `${Math.random()}@example.test`, password, ...fields }); +let ip = 0; +const deletion = (app, token, body = { confirmation: 'DELETE', password }) => request(app) + .delete('/api/users/me').set('cf-connecting-ip', `192.0.2.${++ip}`) + .set('Authorization', `Bearer ${token}`).send(body); +const addFile = async (owner, name, legacy = false) => { + if (!legacy) await getObjectStore().put(name, Buffer.from('private'), 'image/png'); + return File.create({ fileName: name, originalName: name, contentType: 'image/png', size: 7, + uploadedBy: owner._id, ...(legacy ? { data: Buffer.from('legacy') } : {}) }); +}; + +describe('Self-serve account deletion', () => { + let app; + beforeAll(async () => { + process.env.JWT_SECRET = 'account-deletion-service-test'; + await setupMongoDb(); + mockPg = await setupPgDb(); + await mockPg.query(`CREATE TABLE IF NOT EXISTS users (_id VARCHAR(24) PRIMARY KEY, + username VARCHAR(100), profile_picture TEXT, is_bot BOOLEAN DEFAULT false, + created_at TIMESTAMPTZ DEFAULT NOW(), updated_at TIMESTAMPTZ DEFAULT NOW())`); + app = express(); app.use(express.json()); + app.use('/api/users', routes); app.use('/api/uploads', uploads); + }); + afterEach(async () => { await clearMongoDb(); await clearPgDb(); await mockPg.query('DELETE FROM users'); }); + afterAll(async () => { await closeMongoDb(); await closePgDb(); }); + + it('deletes own bytes and legacy uploads, revokes credentials, preserves agents and other users', async () => { + const deviceToken = 'cm_delete-device'; + const human = await makeUser({ apiToken: 'cm_delete-api', deviceTokens: [{ + tokenHash: createHash('sha256').update(deviceToken).digest('hex'), label: 'Laptop' }] }); + const other = await makeUser(); + const agent = await makeUser({ isBot: true }); + const memory = await AgentMemory.create({ agentName: 'surviving-agent', instanceId: 'default', + sections: { long_term: { content: 'Remember this', visibility: 'private', byteSize: 13 } } }); + const ownInstallation = await AgentInstallation.create({ agentName: 'surviving-agent', + podId: other._id, version: '1.0.0', installedBy: human._id }); + const otherInstallation = await AgentInstallation.create({ agentName: 'surviving-agent', + podId: agent._id, version: '1.0.0', installedBy: other._id }); + const credential = await AgentCredential.create({ tokenHash: 'delete-runtime-hash', kind: 'runtime', + ownerUserId: human._id, agentUserId: agent._id, scopes: ['messages:write'] }); + const otherCredential = await AgentCredential.create({ tokenHash: 'keep-runtime-hash', kind: 'runtime', + ownerUserId: other._id, agentUserId: agent._id, scopes: ['messages:write'] }); + await DeviceAuthorization.create({ deviceCodeHash: 'delete-device', userCodeHash: 'delete-code', + hostname: 'laptop', clientName: 'cli', status: 'authorized', userId: human._id, + expiresAt: new Date(Date.now() + 60000) }); + await addFile(human, 'own.png'); await addFile(human, 'legacy.png', true); + await addFile(other, 'other.png'); await addFile(agent, 'agent.png'); + await mockPg.query('INSERT INTO users (_id, username, profile_picture) VALUES ($1,$2,$3)', + [String(human._id), 'Private display name', '/api/uploads/own.png']); + await mockPg.query('INSERT INTO pods (id,name,type,created_by) VALUES ($1,$2,$3,$4)', + ['123456789012345678901234', 'Shared', 'chat', String(other._id)]); + await mockPg.query('INSERT INTO messages (id,pod_id,user_id,content) VALUES ($1,$2,$3,$4)', + ['1', '123456789012345678901234', String(human._id), 'Shared history']); + const token = issueSessionToken(human); + await deletion(app, token).expect(204); + expect(await User.findById(human._id)).toBeNull(); + expect(await File.countDocuments({ uploadedBy: human._id })).toBe(0); + expect(await MediaObject.findOne({ key: 'own.png' })).toBeNull(); + expect(await MediaObject.countDocuments()).toBe(2); + expect(await User.findById(agent._id)).not.toBeNull(); + expect((await AgentMemory.findById(memory._id)).sections.long_term.content).toBe('Remember this'); + expect((await AgentInstallation.findById(ownInstallation._id)).status).toBe('paused'); + expect((await AgentInstallation.findById(otherInstallation._id)).status).toBe('active'); + expect(await User.findById(other._id)).not.toBeNull(); + expect((await AgentCredential.findById(credential._id)).status).toBe('revoked'); + expect((await AgentCredential.findById(otherCredential._id)).status).toBe('active'); + expect(await DeviceAuthorization.countDocuments({ userId: human._id })).toBe(0); + const author = (await mockPg.query('SELECT * FROM users WHERE _id = $1', [String(human._id)])).rows[0]; + expect(author.username).toBe('Deleted user'); expect(author.profile_picture).toBeNull(); + expect((await mockPg.query('SELECT content FROM messages')).rows[0].content).toBe('Shared history'); + // A profile mirror or credential mint that was authenticated before delete + // may finish late. The durable receipt reapplies redaction and revocation. + await mockPg.query('UPDATE users SET username = $2 WHERE _id = $1', [String(human._id), 'Late private name']); + await AgentCredential.updateOne({ _id: credential._id }, { $set: { status: 'active' } }); + await sweepAccountDeletions(); + expect((await mockPg.query('SELECT username FROM users WHERE _id = $1', [String(human._id)])).rows[0].username).toBe('Deleted user'); + expect((await AgentCredential.findById(credential._id)).status).toBe('revoked'); + for (const bearer of [token, 'cm_delete-api', deviceToken]) { + await request(app).get('/api/users/profile').set('Authorization', `Bearer ${bearer}`).expect(401); + } + await request(app).get('/api/uploads/own.png').expect(404); + await request(app).get('/api/uploads/legacy.png').expect(404); + await request(app).get('/api/uploads/other.png').expect(200); + }); + + it('requires explicit confirmation, the right password and a browser session', async () => { + const user = await makeUser({ apiToken: 'cm_no-deletion' }); const token = issueSessionToken(user); + await deletion(app, token, { password }).expect(400); + await deletion(app, token, { confirmation: 'DELETE' }).expect(401); + await deletion(app, token, { confirmation: 'DELETE', password: 'wrong' }).expect(401); + await deletion(app, 'cm_no-deletion').expect(403); + const after = await User.findById(user._id).select('+apiToken'); + expect(after).not.toBeNull(); + expect(after.apiToken).toBe('cm_no-deletion'); + expect(after.accountDeletionRequestedAt).toBeUndefined(); + }); + + it('requires fresh OAuth sign-in, not JWT issue time or refresh', async () => { + const user = await makeUser({ password: undefined, + authProviders: [{ provider: 'github', providerId: 'oauth-delete', linkedAt: new Date() }] }); + const now = Math.floor(Date.now() / 1000); + for (const marker of [undefined, now - 301, now + 5, 'now']) { + const token = jwt.sign({ id: user.id, ...(marker === undefined ? {} : { oauthAuthenticatedAt: marker }) }, + process.env.JWT_SECRET, { expiresIn: 1000 }); + await deletion(app, token, { confirmation: 'DELETE' }).expect(401); + } + await deletion(app, issueSessionToken(user, now - 1, 0), { confirmation: 'DELETE' }).expect(401); + // Real one-time OAuth exchange is the positive witness, not a fabricated claim. + await OAuthLoginState.create({ provider: 'github', state: 'deletion-state', userId: user._id, + exchangeCode: 'deletion-exchange', exchangeExpiresAt: new Date(Date.now() + 60000), + expiresAt: new Date(Date.now() + 60000) }); + let session; + await oauthController.exchangeOAuthCode({ body: { code: 'deletion-exchange' } }, { + json: (body) => { session = body; }, status: () => { throw new Error('Exchange failed'); } }); + expect(jwt.verify(session.token, process.env.JWT_SECRET).oauthAuthenticatedAt).toBeGreaterThanOrEqual(now); + await deletion(app, session.token, { confirmation: 'DELETE' }).expect(204); + }); + + it('keeps failed cleanup retryable and blocks new uploads', async () => { + const user = await makeUser(); const token = issueSessionToken(user); + await addFile(user, 'retry.png'); + jest.spyOn(getObjectStore(), 'delete').mockRejectedValueOnce(new Error('Storage unavailable')); + await deletion(app, token).expect(503); + expect(await File.countDocuments({ uploadedBy: user._id })).toBe(1); + expect((await User.findById(user._id)).accountDeletionRequestedAt).toBeTruthy(); + await request(app).post('/api/uploads').set('Authorization', `Bearer ${token}`) + .attach('image', Buffer.from('late'), 'late.png').expect(403); + await deletion(app, token).expect(204); + expect(await MediaObject.findOne({ key: 'retry.png' })).toBeNull(); + }); + + it('discards an upload that was already in flight', async () => { + const user = await makeUser(); const token = issueSessionToken(user); + const store = getObjectStore(); const originalPut = store.put.bind(store); + jest.spyOn(store, 'put').mockImplementationOnce(async (...args) => { + await deletion(app, token).expect(204); return originalPut(...args); + }); + await request(app).post('/api/uploads').set('Authorization', `Bearer ${token}`) + .attach('image', Buffer.from('late bytes'), 'late.png').expect(403); + expect(await MediaObject.countDocuments()).toBe(0); + expect(await File.countDocuments()).toBe(0); + }); + + it('leaves the account and uploads intact if subscription cancellation fails', async () => { + const user = await makeUser({ apiToken: 'cm_keep-on-billing-failure', + billing: { subscriptionId: 'sub_fail', subscriptionStatus: 'active' } }); + await addFile(user, 'billing.png'); + mockStripe.subscriptions.cancel.mockRejectedValueOnce(new Error('Provider unavailable')); + await deletion(app, issueSessionToken(user)).expect(503); + const after = await User.findById(user._id).select('+apiToken'); + expect(after.apiToken).toBe('cm_keep-on-billing-failure'); + expect(after.accountDeletionRequestedAt).toBeUndefined(); + expect(await MediaObject.findOne({ key: 'billing.png' })).not.toBeNull(); + expect(await File.countDocuments({ uploadedBy: user._id })).toBe(1); + }); + + it('keeps a PG failure retryable and only removes the human after anonymization succeeds', async () => { + const user = await makeUser(); const token = issueSessionToken(user); + jest.spyOn(mockPg, 'query').mockRejectedValueOnce(new Error('PG unavailable')); + await deletion(app, token).expect(503); + expect(await User.findById(user._id)).not.toBeNull(); + await deletion(app, token).expect(204); + expect(await User.findById(user._id)).toBeNull(); + }); + + it.each([false, true])('retries late-upload cleanup after User deletion even if File save fails (%s)', async (metadataFails) => { + const user = await makeUser(); const token = issueSessionToken(user); + const other = await makeUser(); await addFile(other, 'keep.png'); + const store = getObjectStore(); const originalPut = store.put.bind(store); + jest.spyOn(store, 'put').mockImplementationOnce(async (...args) => { + await deletion(app, token).expect(204); + await originalPut(...args); + if (metadataFails) jest.spyOn(File.prototype, 'save').mockRejectedValueOnce(new Error('Metadata unavailable')); + else jest.spyOn(store, 'delete').mockRejectedValueOnce(new Error('Late storage delete unavailable')); + }); + await request(app).post('/api/uploads').set('Authorization', `Bearer ${token}`) + .attach('image', Buffer.from('late bytes'), 'late.png').expect(500); + expect(await User.findById(user._id)).toBeNull(); + expect(await AccountDeletion.findById(user._id)).not.toBeNull(); + expect(await MediaObject.countDocuments()).toBe(2); + if (metadataFails) expect(await File.countDocuments({ uploadedBy: user._id })).toBe(0); + await sweepAccountDeletions(); + expect(await MediaObject.countDocuments()).toBe(1); + expect(await MediaObject.findOne({ key: 'keep.png' })).not.toBeNull(); + expect(await File.countDocuments({ uploadedBy: user._id })).toBe(0); + // Repeated and overlapping sweeps are safe after their data is gone. + await Promise.all([sweepAccountDeletions(), sweepAccountDeletions()]); + expect(await MediaObject.countDocuments()).toBe(1); + }); + + it('protects the last administrator', async () => { + const user = await makeUser({ role: 'admin' }); + await deletion(app, issueSessionToken(user)).expect(409); + expect(await User.findById(user._id)).not.toBeNull(); + }); + + it('cancels a subscription before deleting its billing linkage', async () => { + const user = await makeUser({ billing: { subscriptionId: 'sub_delete', subscriptionStatus: 'active' } }); + await deletion(app, issueSessionToken(user)).expect(204); + expect(mockStripe.subscriptions.cancel).toHaveBeenCalledWith('sub_delete', {}, { idempotencyKey: `account-deletion-${user.id}` }); + }); +}); diff --git a/backend/__tests__/unit/routes/uploads.post.test.js b/backend/__tests__/unit/routes/uploads.post.test.js index 84aaa5aadf..31962ba3c2 100644 --- a/backend/__tests__/unit/routes/uploads.post.test.js +++ b/backend/__tests__/unit/routes/uploads.post.test.js @@ -25,6 +25,10 @@ jest.mock('../../../models/File', () => { }); const File = require('../../../models/File'); +jest.mock('../../../models/User', () => ({ + findById: jest.fn(() => ({ select: () => ({ lean: async () => ({ _id: 'user1' }) }) })), +})); + jest.mock('../../../middleware/auth', () => (req, res, next) => { req.userId = 'user1'; next(); diff --git a/backend/__tests__/unit/services/objectStore/mongoDriver.test.ts b/backend/__tests__/unit/services/objectStore/mongoDriver.test.ts index 851d4ff09e..97cad717d4 100644 --- a/backend/__tests__/unit/services/objectStore/mongoDriver.test.ts +++ b/backend/__tests__/unit/services/objectStore/mongoDriver.test.ts @@ -66,6 +66,18 @@ describe('MongoObjectStore (ADR-002 Phase 1)', () => { expect(got).toBeNull(); }); + it('deletes only an exact ownership namespace and refuses an empty prefix', async () => { + await store.put('user-abc-one.png', Buffer.from('a'), 'image/png'); + await store.put('user-abc-two.png', Buffer.from('b'), 'image/png'); + await store.put('user-abcd-keep.png', Buffer.from('c'), 'image/png'); + await expect(store.deletePrefix('')).rejects.toThrow('prefix'); + expect(await store.get('user-abc-one.png')).not.toBeNull(); + await store.deletePrefix('user-abc-'); + expect(await store.get('user-abc-one.png')).toBeNull(); + expect(await store.get('user-abc-two.png')).toBeNull(); + expect(await store.get('user-abcd-keep.png')).not.toBeNull(); + }); + it('delete is a no-op for a missing key', async () => { await expect(store.delete('never-existed')).resolves.toBeUndefined(); }); diff --git a/backend/__tests__/unit/services/schedulerService.assignedTasks.test.js b/backend/__tests__/unit/services/schedulerService.assignedTasks.test.js index 2411711339..d90646d1a1 100644 --- a/backend/__tests__/unit/services/schedulerService.assignedTasks.test.js +++ b/backend/__tests__/unit/services/schedulerService.assignedTasks.test.js @@ -4,6 +4,10 @@ jest.mock('node-cron', () => ({ jest.mock('../../../services/assignedTaskWakeService', () => ({ dispatchAssignedTaskWakes: jest.fn().mockResolvedValue({ eligible: 0, woken: 0 }), })); +jest.mock('../../../services/accountDeletionService', () => ({ + sweepAccountDeletions: jest.fn().mockResolvedValue(undefined), +})); +const { sweepAccountDeletions } = require('../../../services/accountDeletionService'); const cron = require('node-cron'); const scheduler = require('../../../services/schedulerService'); const { dispatchAssignedTaskWakes } = require('../../../services/assignedTaskWakeService'); @@ -20,6 +24,7 @@ it('starts the assigned-row dispatcher on the one-minute clock and stops it with await job.tick(); } expect(dispatchAssignedTaskWakes).toHaveBeenCalledTimes(1); + expect(sweepAccountDeletions).toHaveBeenCalledTimes(1); scheduler.stop(); expect(minuteJobs.every((job) => job.stop.mock.calls.length === 1)).toBe(true); } finally { diff --git a/backend/__tests__/utils/testUtils.js b/backend/__tests__/utils/testUtils.js index f3a6e5abec..4a4addc067 100644 --- a/backend/__tests__/utils/testUtils.js +++ b/backend/__tests__/utils/testUtils.js @@ -155,7 +155,8 @@ const setupPgDb = async () => { implementation: () => require('crypto').randomUUID(), }); - pgPool = pgDb.adapters.createPg().pool(); + const { Pool } = pgDb.adapters.createPg(); + pgPool = new Pool(); await pgPool.query(` CREATE TABLE IF NOT EXISTS pods ( diff --git a/backend/controllers/oauthController.ts b/backend/controllers/oauthController.ts index c2672469a3..fdadcdd0a1 100644 --- a/backend/controllers/oauthController.ts +++ b/backend/controllers/oauthController.ts @@ -197,7 +197,7 @@ const generateUsername = async (hint: string, email: string) => { }; const issueSession = (user: any) => { - const token = issueSessionToken(user); + const token = issueSessionToken(user, undefined, undefined, Math.floor(Date.now() / 1000)); return { token, verified: user.verified, diff --git a/backend/controllers/userController.ts b/backend/controllers/userController.ts index f67f224806..d91744b775 100644 --- a/backend/controllers/userController.ts +++ b/backend/controllers/userController.ts @@ -43,7 +43,7 @@ const PRIVATE_USER_FIELDS = [ // Reservation metadata is used only by username availability checks. It is // not part of a profile response: historical message text remains the only // public record of the previous handle. -const INTERNAL_USER_FIELDS = ['previousUsernames']; +const INTERNAL_USER_FIELDS = ['previousUsernames', 'accountDeletionRequestedAt']; const isValidAccountLabel = (value: unknown) => typeof value === 'string' && value.trim().length > 0 diff --git a/backend/models/AccountDeletion.ts b/backend/models/AccountDeletion.ts new file mode 100644 index 0000000000..deb753d322 --- /dev/null +++ b/backend/models/AccountDeletion.ts @@ -0,0 +1,16 @@ +import mongoose, { Document, Schema, Types } from 'mongoose'; + +// Minimal durable deletion intent. Retain the opaque identity after User is +// gone so an upload already in flight cannot strand bytes on a cleanup error. +// No email, profile, password or token material belongs on this record. +export interface IAccountDeletion extends Document { + _id: Types.ObjectId; + createdAt: Date; +} +const schema = new Schema({ + _id: { type: Schema.Types.ObjectId, required: true }, + createdAt: { type: Date, default: Date.now }, +}); +export default mongoose.model('AccountDeletion', schema); +// eslint-disable-next-line @typescript-eslint/no-require-imports +module.exports = exports.default; Object.assign(module.exports, exports); diff --git a/backend/models/File.ts b/backend/models/File.ts index 6ed2378a04..eefcafe268 100644 --- a/backend/models/File.ts +++ b/backend/models/File.ts @@ -39,6 +39,7 @@ const fileSchema = new Schema({ }); fileSchema.index({ fileName: 1 }); +fileSchema.index({ uploadedBy: 1 }); fileSchema.index({ podId: 1, createdAt: -1 }); fileSchema.statics.findByFileName = function (fileName: string) { diff --git a/backend/models/User.ts b/backend/models/User.ts index 3ed0e0c778..7f7fe1ce1b 100644 --- a/backend/models/User.ts +++ b/backend/models/User.ts @@ -87,6 +87,7 @@ export interface IUser extends Document { banned: boolean; // Incrementing this invalidates every browser session without rotating JWT_SECRET. tokenVersion: number; + accountDeletionRequestedAt?: Date; bannedAt?: Date; banReason?: string; profilePicture: string; @@ -256,6 +257,7 @@ const userSchema = new Schema({ verified: { type: Boolean, default: false }, banned: { type: Boolean, default: false }, tokenVersion: { type: Number, default: 0 }, + accountDeletionRequestedAt: { type: Date }, bannedAt: { type: Date }, banReason: { type: String }, profilePicture: { type: String, default: 'default' }, diff --git a/backend/routes/uploads.ts b/backend/routes/uploads.ts index 58ddd8f85f..bc03b832a6 100644 --- a/backend/routes/uploads.ts +++ b/backend/routes/uploads.ts @@ -38,6 +38,8 @@ const multer = require('multer'); // eslint-disable-next-line global-require const File = require('../models/File'); // eslint-disable-next-line global-require +const User = require('../models/User'); +// eslint-disable-next-line global-require const auth = require('../middleware/auth'); // eslint-disable-next-line global-require const agentRuntimeAuth = require('../middleware/agentRuntimeAuth'); @@ -211,11 +213,16 @@ const handleUpload = async ( return; } + const uploader = await User.findById(uploaderId).select('accountDeletionRequestedAt').lean(); + if (!uploader || uploader.accountDeletionRequestedAt) { + res.status(403).json({ msg: 'Account deletion is in progress. Uploads are disabled.' }); + return; + } const store = getObjectStore(); const uniqueSuffix = `${Date.now()}-${Math.round(Math.random() * 1e9)}`; const ext = path.extname(req.file.originalname); - const fileName = `${uniqueSuffix}${ext}`; + const fileName = `user-${uploaderId}-${uniqueSuffix}${ext}`; const kind = kindFromName(req.file.originalname); const podId = req.body?.podId?.toString().trim() || null; @@ -234,6 +241,17 @@ const handleUpload = async ( }); await newFile.save(); + // A request authenticated before deletion may finish its upload after the + // deletion scan. Recheck AFTER metadata exists: either the scan owns it, + // or this request removes both the bytes and its metadata. + const remainingUploader = await User.findById(uploaderId).select('accountDeletionRequestedAt').lean(); + if (!remainingUploader || remainingUploader.accountDeletionRequestedAt) { + await store.delete(fileName); + await File.deleteOne({ _id: newFile._id }); + res.status(403).json({ msg: 'Account deletion is in progress. Upload discarded.' }); + return; + } + // Stored upload references must survive hostname changes. Callers can // persist this response directly without baking the current API origin // into User.profilePicture or package icon metadata. diff --git a/backend/routes/users.ts b/backend/routes/users.ts index 01b2467f8b..81dab76553 100644 --- a/backend/routes/users.ts +++ b/backend/routes/users.ts @@ -2,6 +2,7 @@ // DB-backed polling route below (same established pattern as messages.ts). import rateLimit from 'express-rate-limit'; import { createHash } from 'crypto'; +import { AccountDeletionError, deleteOwnAccount } from '../services/accountDeletionService'; import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; // eslint-disable-next-line global-require const express = require('express'); @@ -88,6 +89,36 @@ const profileWriteUserLimit = rateLimit({ }), }); +const accountDeletionRateLimit = rateLimit({ + windowMs: 15 * 60_000, + max: 10, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: cloudflareIpRateLimitKeyGenerator, +}); + +router.delete('/me', accountDeletionRateLimit, auth, async (req: any, res: any) => { + if (req.authType !== 'jwt') { + return res.status(403).json({ + code: 'BROWSER_SESSION_REQUIRED', error: 'Sign in through the browser to delete your account.', + }); + } + if (req.body?.confirmation !== 'DELETE') { + return res.status(400).json({ code: 'CONFIRMATION_REQUIRED', error: 'Confirm account deletion by typing DELETE.' }); + } + try { + const bearer = req.get('Authorization')?.replace(/^Bearer\s+/i, '') || req.get('x-auth-token') || ''; + await deleteOwnAccount(String(req.userId), bearer, req.body?.password); + return res.status(204).send(); + } catch (error) { + if (error instanceof AccountDeletionError) { + return res.status(error.status).json({ code: error.code, error: error.message }); + } + console.error('Account deletion failed:', error instanceof Error ? error.name : 'Unknown error'); + return res.status(503).json({ error: 'Account deletion could not finish. Please retry to complete cleanup.' }); + } +}); + router.get('/me/agent-connection', agentConnectionReadLimit, auth, async (req: any, res: any) => { try { const userId = req.userId || req.user?.id || req.user?._id; diff --git a/backend/services/accountDeletionService.ts b/backend/services/accountDeletionService.ts new file mode 100644 index 0000000000..87bf419d78 --- /dev/null +++ b/backend/services/accountDeletionService.ts @@ -0,0 +1,127 @@ +import jwt from 'jsonwebtoken'; +import User from '../models/User'; +import AccountDeletion from '../models/AccountDeletion'; +import File from '../models/File'; +import DeviceAuthorization from '../models/DeviceAuthorization'; +import OAuthLoginState from '../models/OAuthLoginState'; +import AgentCredential from '../models/AgentCredential'; +import Machine from '../models/Machine'; +import { AgentInstallation } from '../models/AgentRegistry'; +import { getObjectStore } from './objectStore'; +import { disconnectUserSessionSockets } from './sessionSocketService'; +import { stripe } from './billingService'; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +const { pool } = require('../config/db-pg'); + +export class AccountDeletionError extends Error { + constructor(public status: number, public code: string, message: string) { + super(message); + } +} + +const revokeOwnedAccess = async (userId: string): Promise => { + const credentials = await AgentCredential.find({ ownerUserId: userId }).select('_id').lean(); + await AgentCredential.revokeCascade(credentials.map((credential) => credential._id)); + await Machine.deleteMany({ ownerUserId: userId }); + await AgentInstallation.updateMany( + { installedBy: userId, status: 'active' }, + { $set: { status: 'paused' } }, + ); + await DeviceAuthorization.deleteMany({ userId }); + await OAuthLoginState.deleteMany({ userId }); + +}; + +const anonymizeChatAuthor = async (userId: string): Promise => { + // Shared messages remain. Their PG author join keeps an anonymous tombstone; + // it must not preserve the deleted person's display name or avatar. + if (pool) { + await pool.query( + 'UPDATE users SET username = $2, profile_picture = NULL, updated_at = $3 WHERE _id = $1', + [userId, 'Deleted user', new Date()], + ); + } +}; + +const deleteOwnedUploads = async (userId: string): Promise => { + // New keys carry an ownership namespace. This also finds bytes whose File + // save failed after put, or whose metadata was removed during a late put. + await getObjectStore().deletePrefix(`user-${userId}-`); + const files = await File.find({ uploadedBy: userId }).select('_id fileName').lean(); + for (const file of files) { + // Legacy keys have no namespace; retain metadata until deletion succeeds. + // eslint-disable-next-line no-await-in-loop + await getObjectStore().delete(file.fileName); + // eslint-disable-next-line no-await-in-loop + await File.deleteOne({ _id: file._id, uploadedBy: userId }); + } +}; + +/** Retried by the scheduler even after the human row has been removed. */ +export const sweepAccountDeletions = async (): Promise => { + const receipts = AccountDeletion.find().select('_id').lean().cursor(); + for await (const receipt of receipts) { + const userId = String(receipt._id); + // Reapply revocation/redaction too: a request authenticated before deletion + // can finish a credential mint or profile mirror after the first scan. + await revokeOwnedAccess(userId); + await anonymizeChatAuthor(userId); + await deleteOwnedUploads(userId); + } +}; + +/** Human deletion pauses owned runtimes, preserving independent agent User/memory rows. */ +export const deleteOwnAccount = async ( + userId: string, + bearer: string, + password: unknown, +): Promise => { + const user = await User.findById(userId); + if (!user || user.isBot) { + throw new AccountDeletionError(403, 'HUMAN_ACCOUNT_REQUIRED', 'Only human accounts can use account deletion.'); + } + if (user.password) { + if (typeof password !== 'string' || !password || password.length > 1024) { + throw new AccountDeletionError(401, 'PASSWORD_REQUIRED', 'Enter your password to delete your account.'); + } + if (!await user.comparePassword(password)) { + throw new AccountDeletionError(401, 'PASSWORD_REQUIRED', 'Incorrect password.'); + } + } else { + const claims = jwt.verify(bearer, process.env.JWT_SECRET!) as jwt.JwtPayload; + const age = Math.floor(Date.now() / 1000) - claims.oauthAuthenticatedAt; + if (String(claims.id) !== userId || !Number.isSafeInteger(claims.oauthAuthenticatedAt) + || age < 0 || age > 300) { + throw new AccountDeletionError(401, 'REAUTH_REQUIRED', + 'Sign in again with your OAuth provider, then confirm deletion within five minutes.'); + } + } + if (user.role === 'admin' && await User.countDocuments({ role: 'admin', isBot: { $ne: true } }) <= 1) { + throw new AccountDeletionError(409, 'LAST_ADMIN', 'Assign another administrator before deleting your account.'); + } + + // Cancel billing before destroying the linkage needed to stop future charges. + if (user.billing?.subscriptionId && user.billing.subscriptionStatus !== 'canceled') { + const client = stripe(); + if (!client) throw new Error('Billing is unavailable; account deletion cannot cancel the subscription'); + await client.subscriptions.cancel(user.billing.subscriptionId, {}, { + idempotencyKey: `account-deletion-${userId}`, + }); + } + + // Durable marker closes upload races and survives a storage failure. Keep the + // human row until cleanup succeeds so the same re-authenticated request can retry. + await User.updateOne({ _id: userId }, { + $set: { accountDeletionRequestedAt: new Date(), deviceTokens: [] }, + $unset: { apiToken: 1, apiTokenCreatedAt: 1, apiTokenScopes: 1 }, + }); + await AccountDeletion.updateOne({ _id: userId }, { $setOnInsert: { createdAt: new Date() } }, { upsert: true }); + await revokeOwnedAccess(userId); + + await deleteOwnedUploads(userId); + + await anonymizeChatAuthor(userId); + await User.deleteOne({ _id: userId, isBot: { $ne: true } }); + disconnectUserSessionSockets(userId); +}; diff --git a/backend/services/objectStore/ObjectStore.ts b/backend/services/objectStore/ObjectStore.ts index 4f4846d4dc..601dc8c9a4 100644 --- a/backend/services/objectStore/ObjectStore.ts +++ b/backend/services/objectStore/ObjectStore.ts @@ -49,4 +49,7 @@ export interface ObjectStore { /** Remove bytes by `key`. No-op if the key does not exist. */ delete(key: string): Promise; + + /** Delete an ownership namespace, including writes that lost their metadata. */ + deletePrefix(prefix: string): Promise; } diff --git a/backend/services/objectStore/drivers/mongoDriver.ts b/backend/services/objectStore/drivers/mongoDriver.ts index 76e4658bac..77a7bdf75b 100644 --- a/backend/services/objectStore/drivers/mongoDriver.ts +++ b/backend/services/objectStore/drivers/mongoDriver.ts @@ -45,6 +45,13 @@ export class MongoObjectStore implements ObjectStore { }; } + async deletePrefix(prefix: string): Promise { + if (!prefix) throw new Error('An object deletion prefix is required'); + // Keys are indexed. The range matches this exact prefix, not a regex + // supplied by a caller; upload namespaces contain only ASCII characters. + await MediaObject.deleteMany({ key: { $gte: prefix, $lt: `${prefix}\uffff` } }); + } + async delete(key: string): Promise { await MediaObject.deleteOne({ key }); } diff --git a/backend/services/schedulerService.ts b/backend/services/schedulerService.ts index 08510837f7..e20c2409c5 100644 --- a/backend/services/schedulerService.ts +++ b/backend/services/schedulerService.ts @@ -546,6 +546,24 @@ class SchedulerService { { scheduled: false, timezone: 'UTC' }, ); + // A late upload can outlive the deletion request and fail its own cleanup. + // Durable intents contain only opaque user ids; this sweep can retry after + // the User row is gone. Deletes are idempotent across backend replicas. + const accountDeletionUploadJob: CronJob = cron.schedule( + '* * * * *', + async () => { + try { + // eslint-disable-next-line global-require, @typescript-eslint/no-require-imports + const { sweepAccountDeletions } = require('./accountDeletionService'); + await sweepAccountDeletions(); + } catch (error) { + const errorName = error instanceof Error ? error.name : 'Unknown error'; + console.error('[account-deletion] sweep failed:', errorName); + } + }, + { scheduled: false, timezone: 'UTC' }, + ); + const spawnCredentialReapJob: CronJob = cron.schedule( '*/5 * * * *', async () => { @@ -623,6 +641,7 @@ class SchedulerService { decisionCardReconcileJob, approvalToolCallSweepJob, spawnCredentialReapJob, + accountDeletionUploadJob, spendAlarmJob, ...(process.env.SYNTHETIC_ACCOUNT_PURGE === 'true' ? [syntheticAccountPurgeJob] : []), ]; diff --git a/backend/services/sessionTokenService.ts b/backend/services/sessionTokenService.ts index 0ce3b9f8ce..60d664eefa 100644 --- a/backend/services/sessionTokenService.ts +++ b/backend/services/sessionTokenService.ts @@ -13,6 +13,7 @@ export const issueSessionToken = ( user: { _id: unknown; tokenVersion?: number }, sessionStartedAt?: number, authenticatedVersion?: number, + oauthAuthenticatedAt?: number, ): string | null => { const now = Math.floor(Date.now() / 1000); const startedAt = sessionStartedAt === undefined ? now : sessionStartedAt; @@ -23,5 +24,6 @@ export const issueSessionToken = ( id: user._id, tokenVersion: authenticatedVersion ?? user.tokenVersion ?? 0, sessionStartedAt: startedAt, + ...(oauthAuthenticatedAt === undefined ? {} : { oauthAuthenticatedAt }), }, process.env.JWT_SECRET!, { expiresIn: Math.min(SESSION_TOKEN_SECONDS, remaining) }); }; diff --git a/docs/api/account-deletion.md b/docs/api/account-deletion.md new file mode 100644 index 0000000000..324315dccd --- /dev/null +++ b/docs/api/account-deletion.md @@ -0,0 +1,84 @@ +# Self-serve account deletion + +`DELETE /api/users/me` accepts a browser JWT and a JSON body: + +```json +{ "confirmation": "DELETE", "password": "the current password" } +``` + +A successful request returns `204`. A password account must supply its current +password, including accounts that also have a linked OAuth provider. An OAuth-only +account must complete a new provider sign-in and confirm within five minutes. +Only the one-time OAuth exchange stamps `oauthAuthenticatedAt` on its signed JWT; +ordinary JWT issuance and session refresh do not supply that proof. API tokens, +device tokens and agent runtime credentials cannot call this route. + +The endpoint is limited to ten attempts per fifteen minutes per client address. +It refuses to remove the last human administrator. + +## Data and identity rules + +- The human's MongoDB `User` row is deleted. Its embedded API/device tokens no + longer authenticate, and browser JWT verification refuses the missing account. + Existing browser sockets are disconnected. +- Every `File` owned by that human is removed, including legacy inline payloads. + Its object-store bytes are deleted first, using `fileName` as the storage key. + New uploads use the ownership namespace `user--`, which lets deletion + find bytes even if their metadata save failed. + Ownership comes from `File.uploadedBy`; `MediaObject` has no ownership column. + This includes avatars and uploads shared into pods. Other users' and agents' + uploads remain. +- Authorized device-login and OAuth-login state rows for the human are removed. + Owned `AgentCredential` records and their descendants are revoked; machine + registrations are removed. Agent credentials owned by other humans remain. +- Active runtime installations made by the human are paused. Their records, + independent agent `User` identities, memory and pod memberships survive + (ADR-001). Installations made by other humans remain active. Legacy agent + tokens that carry no human ownership are not reclassified as the human's token. +- Shared messages and posts remain. PostgreSQL's author row becomes a `Deleted + user` tombstone with no avatar so historical chat joins retain an author without + preserving its display name. Account deletion does not scrub personal details + a person wrote into shared message text. +- Any linked Stripe subscription is canceled before the account's billing + linkage is removed. Cancellation failure refuses deletion rather than leaving + an unreachable recurring charge. Stripe is an external billing boundary; + cancellation is mocked in the service tests. + +## Failures and retries + +Cleanup spans MongoDB, the object store, PostgreSQL and Stripe, so it is not a +single transaction. After re-authentication and billing cancellation, a durable +`accountDeletionRequestedAt` marker prevents new uploads. Upload requests already +in flight recheck the marker after saving metadata and discard their upload if +deletion has begun or the account is gone. + +An `AccountDeletion` receipt retains only the opaque user id and creation time. +The scheduler retries that identity's access revocation, chat-author redaction +and upload cleanup every minute, including +bytes in its namespace that have no `File` record. The receipt survives User +deletion: a late upload that fails its own cleanup is removed on a subsequent +sweep when storage recovers. These receipts deliberately have no TTL so a delayed +write cannot outlive its cleanup intent. They contain no profile, email or token. + +Bytes are removed before their `File` metadata. A failure in the deletion +request's own storage or PG cleanup returns +`503` and retains the human row so a re-authenticated retry can finish. Already +removed objects are safe to delete again. Owned API/device credentials are +revoked during cleanup; the browser session stays available for retry until the +human row is removed. A retry after the OAuth freshness window requires another +provider sign-in. Subscription cancellation uses an account-specific idempotency +key. This does not replace provider retention policies or remove billing audit +records and backups. + +Relevant errors: `CONFIRMATION_REQUIRED` (400), `PASSWORD_REQUIRED` or +`REAUTH_REQUIRED` (401), `BROWSER_SESSION_REQUIRED` (403), `LAST_ADMIN` (409). + +## Verification + +`backend/__tests__/service/accountDeletion.test.js` exercises real MongoDB and +PostgreSQL when run with `INTEGRATION_TEST=true`, including upload bytes and +metadata removal, retained messages and anonymized author joins, agent +identity/memory continuity, token revocation, failed-cleanup retries and an +upload that crosses deletion, including storage cleanup failure and a failed +metadata save after the human row is gone. The positive OAuth case consumes a real one-time +exchange code rather than constructing the accepted JWT claim in the fixture. From 355286421c705885b5130663dc1cf5909d757eff Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 07:40:26 -0700 Subject: [PATCH 02/13] fix(users): fence deletion races and bound cleanup retries Require administrator role handoff before deletion and coordinate role changes with the target's deletion marker. The previous count-then-delete check could remove both remaining administrators. Rate limit uploads ahead of auth/parsing. Fence runtime token issuance and backfill before and after their writes, remove late newly minted tokens even on legacy reprovision paths, and refuse runtime auth for deleting owners. Preserve tokens actually owned by other people. Bound cleanup to 100 fair-queue receipts per tick under a renewable cluster lease. Rotate failed receipts, continue cleaning later entries and report an aggregate error after each batch. Retain deletion intents for late-write retries. Validation: 26 real MongoDB/PostgreSQL service tests; focused token, auth, upload, role and scheduler regression suites; source typecheck and targeted lint. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../__tests__/service/accountDeletion.test.js | 179 ++++++++++++++++++ .../unit/middleware/agentRuntimeAuth.test.js | 3 + .../__tests__/unit/routes/admin.users.test.js | 9 +- .../routes/registry.runtime-tokens.test.js | 3 + .../routes/routeRateLimitGuard.baseline.json | 2 - .../unit/routes/uploads.post.test.js | 7 + backend/middleware/agentRuntimeAuth.ts | 4 + backend/models/AccountDeletion.ts | 3 + backend/models/SchedulerHeartbeatLease.ts | 3 +- backend/routes/admin/users.ts | 17 +- backend/routes/registry/tokens.ts | 23 +++ backend/routes/uploads.ts | 10 +- backend/services/accountDeletionService.ts | 81 ++++++-- backend/services/accountDeletionState.ts | 9 + backend/services/schedulerService.ts | 3 +- docs/api/account-deletion.md | 34 +++- 16 files changed, 360 insertions(+), 30 deletions(-) create mode 100644 backend/services/accountDeletionState.ts diff --git a/backend/__tests__/service/accountDeletion.test.js b/backend/__tests__/service/accountDeletion.test.js index f7d78fe443..2adec41e06 100644 --- a/backend/__tests__/service/accountDeletion.test.js +++ b/backend/__tests__/service/accountDeletion.test.js @@ -15,6 +15,9 @@ const OAuthLoginState = require('../../models/OAuthLoginState'); const AgentCredential = require('../../models/AgentCredential'); const AgentMemory = require('../../models/AgentMemory'); const { AgentInstallation } = require('../../models/AgentRegistry'); +const SchedulerLease = require('../../models/SchedulerHeartbeatLease'); +const { issueRuntimeTokenForAgent } = require('../../routes/registry/tokens'); +const agentRuntimeAuth = require('../../middleware/agentRuntimeAuth'); const { sweepAccountDeletions } = require('../../services/accountDeletionService'); const AccountDeletion = require('../../models/AccountDeletion'); const { issueSessionToken } = require('../../services/sessionTokenService'); @@ -22,6 +25,7 @@ const { getObjectStore } = require('../../services/objectStore'); const oauthController = require('../../controllers/oauthController'); const routes = require('../../routes/users'); const uploads = require('../../routes/uploads'); +const adminRoutes = require('../../routes/admin/users'); const password = 'Delete-this-account-123!'; const makeUser = (fields = {}) => User.create({ username: `person-${Math.random().toString(16).slice(2)}`, email: `${Math.random()}@example.test`, password, ...fields }); @@ -46,6 +50,9 @@ describe('Self-serve account deletion', () => { created_at TIMESTAMPTZ DEFAULT NOW(), updated_at TIMESTAMPTZ DEFAULT NOW())`); app = express(); app.use(express.json()); app.use('/api/users', routes); app.use('/api/uploads', uploads); + app.use('/api/admin/users', adminRoutes); + const probeLimit = require('express-rate-limit')({ windowMs: 60000, max: 1000 }); + app.get('/runtime-probe', probeLimit, agentRuntimeAuth, (_req, res) => res.sendStatus(204)); }); afterEach(async () => { await clearMongoDb(); await clearPgDb(); await mockPg.query('DELETE FROM users'); }); afterAll(async () => { await closeMongoDb(); await closePgDb(); }); @@ -214,6 +221,178 @@ describe('Self-serve account deletion', () => { expect(await MediaObject.countDocuments()).toBe(1); }); + it('refuses both concurrent admin deletions without changing accounts or creating cleanup intents', async () => { + const a = await makeUser({ role: 'admin' }); const b = await makeUser({ role: 'admin' }); + const results = await Promise.all([deletion(app, issueSessionToken(a)), deletion(app, issueSessionToken(b))]); + expect(results.map((result) => result.status)).toEqual([409, 409]); + expect(results.map((result) => result.body.code)).toEqual(['ADMIN_HANDOFF_REQUIRED', 'ADMIN_HANDOFF_REQUIRED']); + expect(await User.countDocuments({ role: 'admin' })).toBe(2); + expect(await AccountDeletion.countDocuments()).toBe(0); + expect((await User.findById(a._id)).accountDeletionRequestedAt).toBeUndefined(); + expect((await User.findById(b._id)).accountDeletionRequestedAt).toBeUndefined(); + }); + + it('allows deletion after another administrator hands off the role', async () => { + const user = await makeUser({ role: 'admin' }); const keeper = await makeUser({ role: 'admin' }); + await request(app).patch(`/api/admin/users/${user.id}/role`) + .set('Authorization', `Bearer ${issueSessionToken(keeper)}`).send({ role: 'user' }).expect(200); + expect((await User.findById(user._id)).role).toBe('user'); + await deletion(app, issueSessionToken(user)).expect(204); + expect(await User.findById(user._id)).toBeNull(); + expect((await User.findById(keeper._id)).role).toBe('admin'); + }); + + it('refuses deletion if a promotion wins before the conditional deletion marker', async () => { + const user = await makeUser(); await addFile(user, 'promotion.png'); + const updateOne = User.updateOne.bind(User); + jest.spyOn(User, 'updateOne').mockImplementation((filter, update, ...rest) => { + if (update.$set?.accountDeletionRequestedAt) return (async () => { + await updateOne({ _id: user._id }, { $set: { role: 'admin' } }); + return updateOne(filter, update, ...rest); + })(); + return updateOne(filter, update, ...rest); + }); + await deletion(app, issueSessionToken(user)).expect(409); + expect((await User.findById(user._id)).role).toBe('admin'); + expect(await File.countDocuments({ uploadedBy: user._id })).toBe(1); + expect(await AccountDeletion.countDocuments()).toBe(0); + }); + + it('refuses an admin role change after deletion has marked the account', async () => { + const admin = await makeUser({ role: 'admin' }); + const user = await makeUser({ accountDeletionRequestedAt: new Date() }); + await request(app).patch(`/api/admin/users/${user.id}/role`) + .set('Authorization', `Bearer ${issueSessionToken(admin)}`).send({ role: 'admin' }).expect(409); + expect((await User.findById(user._id)).role).toBe('user'); + }); + + it('discards an in-flight upload when PG failure leaves the human marked for deletion', async () => { + const user = await makeUser(); const token = issueSessionToken(user); + const store = getObjectStore(); const put = store.put.bind(store); + jest.spyOn(store, 'put').mockImplementationOnce(async (...args) => { + jest.spyOn(mockPg, 'query').mockRejectedValueOnce(new Error('PG unavailable')); + await deletion(app, token).expect(503); + return put(...args); + }); + await request(app).post('/api/uploads').set('Authorization', `Bearer ${token}`) + .attach('image', Buffer.from('late'), 'late.png').expect(403); + expect((await User.findById(user._id)).accountDeletionRequestedAt).toBeTruthy(); + expect(await MediaObject.countDocuments()).toBe(0); + expect(await File.countDocuments({ uploadedBy: user._id })).toBe(0); + }); + + it('continues past one failed receipt and reports aggregate failure after cleaning the next', async () => { + const a = await makeUser(); const b = await makeUser(); + await AccountDeletion.create([{ _id: a._id }, { _id: b._id }]); + await addFile(a, 'fail.png'); await addFile(b, 'clean.png'); + const remove = getObjectStore().delete.bind(getObjectStore()); + jest.spyOn(getObjectStore(), 'delete').mockImplementation(async (key) => { + if (key === 'fail.png') throw new Error('One object unavailable'); + return remove(key); + }); + await expect(sweepAccountDeletions()).rejects.toThrow('Account deletion cleanup failed'); + expect(await File.findOne({ fileName: 'fail.png' })).not.toBeNull(); + expect(await File.findOne({ fileName: 'clean.png' })).toBeNull(); + expect(await MediaObject.findOne({ key: 'clean.png' })).toBeNull(); + expect((await AccountDeletion.findById(b._id)).lastSweptAt).toBeTruthy(); + expect(await SchedulerLease.findById('account-deletion-sweep')).toBeNull(); + }); + + it('skips a sweep held by another worker and recovers an expired lease', async () => { + const user = await makeUser(); await AccountDeletion.create({ _id: user._id }); + await addFile(user, 'lease.png'); + await SchedulerLease.create({ _id: 'account-deletion-sweep', ownerId: 'another-worker', + expiresAt: new Date(Date.now() + 120000) }); + await sweepAccountDeletions(); + expect(await MediaObject.findOne({ key: 'lease.png' })).not.toBeNull(); + await SchedulerLease.updateOne({ _id: 'account-deletion-sweep' }, { $set: { expiresAt: new Date(0) } }); + await sweepAccountDeletions(); + expect(await MediaObject.findOne({ key: 'lease.png' })).toBeNull(); + }); + + it('bounds each sweep and fairly rotates receipts without expiring late-write intent', async () => { + const ids = Array.from({ length: 101 }, () => new User()._id); + await AccountDeletion.insertMany(ids.map((_id) => ({ _id }))); + await sweepAccountDeletions(); + expect(await AccountDeletion.countDocuments({ lastSweptAt: { $exists: true } })).toBe(100); + expect(await AccountDeletion.countDocuments()).toBe(101); + await sweepAccountDeletions(); + expect(await AccountDeletion.countDocuments({ lastSweptAt: { $exists: true } })).toBe(101); + }); + + it.each([false, true])('refuses issuance and backfill for a marked owner (%s)', async (existing) => { + const owner = await makeUser({ accountDeletionRequestedAt: new Date() }); + const agent = await makeUser({ isBot: true, agentRuntimeTokens: existing ? [{ tokenHash: 'old-hash', label: 'old' }] : [] }); + await expect(issueRuntimeTokenForAgent(agent, 'key', null, { ownerUserId: owner._id })) + .rejects.toMatchObject({ code: 'account_deleting' }); + expect(await AgentCredential.countDocuments()).toBe(0); + }); + + it.each([false, true])('revokes a token minted or backfilled after deletion completed (%s)', async (existing) => { + const owner = await makeUser(); + const agent = await makeUser({ isBot: true, agentRuntimeTokens: existing ? [{ tokenHash: 'late-old-hash', label: 'old' }] : [] }); + const method = existing ? 'updateOne' : 'create'; + const write = AgentCredential[method].bind(AgentCredential); + jest.spyOn(AgentCredential, method).mockImplementationOnce(async (...args) => { + await deletion(app, issueSessionToken(owner)).expect(204); + return write(...args); + }); + await expect(issueRuntimeTokenForAgent(agent, 'key', null, { ownerUserId: owner._id })) + .rejects.toMatchObject({ code: 'account_deleting' }); + expect(await AgentCredential.countDocuments({ ownerUserId: owner._id, status: 'active' })).toBe(0); + expect((await User.findById(agent._id)).agentRuntimeTokens).toHaveLength(0); + }); + + it('cleans a late legacy reprovision mint even when no issuer ledger was written', async () => { + const owner = await makeUser(); const agent = await makeUser({ isBot: true }); + const installation = await AgentInstallation.create({ agentName: agent.username, podId: owner._id, + version: '1.0.0', installedBy: owner._id }); + const save = agent.save.bind(agent); + jest.spyOn(agent, 'save').mockImplementationOnce(async () => { + await deletion(app, issueSessionToken(owner)).expect(204); + return save(); + }); + await expect(issueRuntimeTokenForAgent(agent, 'key', installation)) + .rejects.toMatchObject({ code: 'account_deleting' }); + expect(await AgentCredential.countDocuments()).toBe(0); + expect((await User.findById(agent._id)).agentRuntimeTokens).toHaveLength(0); + expect((await AgentInstallation.findById(installation._id)).runtimeTokens).toHaveLength(0); + expect((await AgentInstallation.findById(installation._id)).status).toBe('paused'); + }); + + it('a late backfill refusal preserves an existing token owned by someone else', async () => { + const owner = await makeUser(); const other = await makeUser(); + const agent = await makeUser({ isBot: true, agentRuntimeTokens: [{ tokenHash: 'shared-old-hash', label: 'other key' }] }); + const credential = await AgentCredential.create({ tokenHash: 'shared-old-hash', kind: 'runtime', + ownerUserId: other._id, agentUserId: agent._id }); + const write = AgentCredential.updateOne.bind(AgentCredential); + jest.spyOn(AgentCredential, 'updateOne').mockImplementationOnce(async (...args) => { + await deletion(app, issueSessionToken(owner)).expect(204); + return write(...args); + }); + await expect(issueRuntimeTokenForAgent(agent, 'key', null, { ownerUserId: owner._id })) + .rejects.toMatchObject({ code: 'account_deleting' }); + expect((await AgentCredential.findById(credential._id)).status).toBe('active'); + expect((await User.findById(agent._id)).agentRuntimeTokens).toHaveLength(1); + }); + + it('rejects an active credential while its owner is being deleted, before any later sweep', async () => { + const owner = await makeUser(); + const raw = 'cm_agent_deletion-fence-test'; + const tokenHash = createHash('sha256').update(raw).digest('hex'); + const agent = await makeUser({ isBot: true, botMetadata: { agentName: 'deletion-runtime-probe', instanceId: 'default' }, + agentRuntimeTokens: [{ tokenHash, label: 'probe', lastUsedAt: new Date() }] }); + await AgentInstallation.create({ agentName: 'deletion-runtime-probe', instanceId: 'default', podId: owner._id, + version: '1.0.0', installedBy: owner._id }); + await AgentCredential.create({ tokenHash, kind: 'runtime', ownerUserId: owner._id, agentUserId: agent._id }); + // A viable credential is essential: an incomplete identity would return + // 401 even with the deletion fence removed and could not pin this guard. + await request(app).get('/runtime-probe').set('Authorization', `Bearer ${raw}`).expect(204); + await AccountDeletion.create({ _id: owner._id }); + await request(app).get('/runtime-probe').set('Authorization', `Bearer ${raw}`).expect(401); + expect(await AgentCredential.countDocuments({ status: 'active' })).toBe(1); + }); + it('protects the last administrator', async () => { const user = await makeUser({ role: 'admin' }); await deletion(app, issueSessionToken(user)).expect(409); diff --git a/backend/__tests__/unit/middleware/agentRuntimeAuth.test.js b/backend/__tests__/unit/middleware/agentRuntimeAuth.test.js index f0598a3fe6..ee7be69a2b 100644 --- a/backend/__tests__/unit/middleware/agentRuntimeAuth.test.js +++ b/backend/__tests__/unit/middleware/agentRuntimeAuth.test.js @@ -1,3 +1,6 @@ +// These unit cases isolate runtime identity behavior. The deletion fence is +// exercised with real models in service/accountDeletion.test.js. +jest.mock('../../../services/accountDeletionState', () => ({ isAccountDeleting: jest.fn().mockResolvedValue(false) })); // agentRuntimeAuth path 2 (legacy installation-token) needs to surface ALL // active AgentInstallation rows for the same (agentName, instanceId), not // just the one whose runtimeTokens.tokenHash matches. Otherwise the /events diff --git a/backend/__tests__/unit/routes/admin.users.test.js b/backend/__tests__/unit/routes/admin.users.test.js index 4f57aa38a4..e7c76c1dcc 100644 --- a/backend/__tests__/unit/routes/admin.users.test.js +++ b/backend/__tests__/unit/routes/admin.users.test.js @@ -8,6 +8,7 @@ jest.mock('../../../middleware/adminAuth', () => (_req, _res, next) => next()); jest.mock('../../../models/User', () => ({ find: jest.fn(), findById: jest.fn(), + findOneAndUpdate: jest.fn(), countDocuments: jest.fn(), })); @@ -140,11 +141,15 @@ describe('admin users routes', () => { save: jest.fn().mockResolvedValue(true), }; User.findById.mockResolvedValue(target); + User.findOneAndUpdate.mockResolvedValue({ ...target, role: 'admin' }); await handler(req, res); - expect(target.role).toBe('admin'); - expect(target.save).toHaveBeenCalledTimes(1); + expect(User.findOneAndUpdate).toHaveBeenCalledWith( + { _id: 'u2', accountDeletionRequestedAt: { $exists: false } }, + { $set: { role: 'admin' } }, { new: true }, + ); + expect(target.save).not.toHaveBeenCalled(); expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ message: 'User role updated successfully', diff --git a/backend/__tests__/unit/routes/registry.runtime-tokens.test.js b/backend/__tests__/unit/routes/registry.runtime-tokens.test.js index d0eb351077..1c57572f09 100644 --- a/backend/__tests__/unit/routes/registry.runtime-tokens.test.js +++ b/backend/__tests__/unit/routes/registry.runtime-tokens.test.js @@ -1,3 +1,6 @@ +// These unit cases isolate runtime identity behavior. The deletion fence is +// exercised with real models in service/accountDeletion.test.js. +jest.mock('../../../services/accountDeletionState', () => ({ isAccountDeleting: jest.fn().mockResolvedValue(false) })); const request = require('supertest'); const express = require('express'); diff --git a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json b/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json index 26f90b1080..1875ccb069 100644 --- a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json +++ b/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json @@ -14,7 +14,6 @@ "routes/admin/users.ts GET / [unlimited]", "routes/admin/users.ts GET /invitations [unlimited]", "routes/admin/users.ts GET /waitlist [unlimited]", - "routes/admin/users.ts PATCH /:userId/role [unlimited]", "routes/admin/users.ts PATCH /waitlist/:requestId [unlimited]", "routes/admin/users.ts POST /invitations [unlimited]", "routes/admin/users.ts POST /invitations/:invitationId/revoke [unlimited]", @@ -160,7 +159,6 @@ "routes/skills.ts POST /:skillId/rating [unlimited]", "routes/skills.ts POST /import [unlimited]", "routes/test-bot.ts POST /discord-summary [unlimited]", - "routes/uploads.ts POST / [unlimited]", "routes/users.ts DELETE /:id/follow [unlimited]", "routes/users.ts GET /:id [unlimited]", "routes/users.ts GET /:id/public-activity [unlimited]", diff --git a/backend/__tests__/unit/routes/uploads.post.test.js b/backend/__tests__/unit/routes/uploads.post.test.js index 31962ba3c2..94fa777d7d 100644 --- a/backend/__tests__/unit/routes/uploads.post.test.js +++ b/backend/__tests__/unit/routes/uploads.post.test.js @@ -72,6 +72,13 @@ describe('uploads POST / (ADR-002 Phase 1)', () => { expect(res.body.url).not.toMatch(/^https?:\/\//); }); + it('limits uploads to 60 per address per fifteen minutes', async () => { + for (let attempt = 0; attempt < 60; attempt += 1) { + await request(app).post('/api/uploads').set('cf-connecting-ip', '198.51.100.73').expect(400); + } + await request(app).post('/api/uploads').set('cf-connecting-ip', '198.51.100.73').expect(429); + }); + it('returns 400 when no file is provided', async () => { const res = await request(app).post('/api/uploads').expect(400); expect(res.body.msg).toBe('No file uploaded'); diff --git a/backend/middleware/agentRuntimeAuth.ts b/backend/middleware/agentRuntimeAuth.ts index 6ff26d49ff..b0d58e5714 100644 --- a/backend/middleware/agentRuntimeAuth.ts +++ b/backend/middleware/agentRuntimeAuth.ts @@ -1,3 +1,4 @@ +import { isAccountDeleting } from '../services/accountDeletionState'; import { resolveTokenAgentIdentity } from '../utils/agentTokenIdentity'; import { Request, Response, NextFunction } from 'express'; import { AgentInstallation } from '../models/AgentRegistry'; @@ -76,6 +77,9 @@ export default async function agentRuntimeAuth(req: Request, res: Response, next if (credential.status !== 'active') { return res.status(401).json({ message: 'Token revoked' }); } + if (await isAccountDeleting(credential.ownerUserId)) { + return res.status(401).json({ message: 'Credential owner is being deleted' }); + } if (credential.expiresAt && credential.expiresAt < new Date()) { return res.status(401).json({ message: 'Session token expired' }); } diff --git a/backend/models/AccountDeletion.ts b/backend/models/AccountDeletion.ts index deb753d322..d29061508d 100644 --- a/backend/models/AccountDeletion.ts +++ b/backend/models/AccountDeletion.ts @@ -6,11 +6,14 @@ import mongoose, { Document, Schema, Types } from 'mongoose'; export interface IAccountDeletion extends Document { _id: Types.ObjectId; createdAt: Date; + lastSweptAt?: Date; } const schema = new Schema({ _id: { type: Schema.Types.ObjectId, required: true }, createdAt: { type: Date, default: Date.now }, + lastSweptAt: { type: Date }, }); +schema.index({ lastSweptAt: 1, _id: 1 }); export default mongoose.model('AccountDeletion', schema); // eslint-disable-next-line @typescript-eslint/no-require-imports module.exports = exports.default; Object.assign(module.exports, exports); diff --git a/backend/models/SchedulerHeartbeatLease.ts b/backend/models/SchedulerHeartbeatLease.ts index f87f6ac3c4..2783e16ae0 100644 --- a/backend/models/SchedulerHeartbeatLease.ts +++ b/backend/models/SchedulerHeartbeatLease.ts @@ -1,7 +1,8 @@ import mongoose, { Document, Model, Schema } from 'mongoose'; /** - * A short-lived, cluster-wide lease for one agent-heartbeat dispatch. The + * A short-lived, cluster-wide lease for scheduled work (heartbeats and + * account deletion). The legacy collection name remains stable. The * fixed _id is the natural key: MongoDB's primary-key uniqueness makes the * acquire compare-and-set atomic without depending on index build timing. */ diff --git a/backend/routes/admin/users.ts b/backend/routes/admin/users.ts index 7dfea60f20..e37135c30e 100644 --- a/backend/routes/admin/users.ts +++ b/backend/routes/admin/users.ts @@ -136,7 +136,7 @@ router.get('/', auth, adminAuth, async (req: any, res: any) => { }); // PATCH /api/admin/users/:userId/role -router.patch('/:userId/role', auth, adminAuth, async (req: any, res: any) => { +router.patch('/:userId/role', adminWriteLimiter, auth, adminAuth, async (req: any, res: any) => { try { const { userId } = req.params; const { role } = req.body || {}; @@ -160,12 +160,21 @@ router.patch('/:userId/role', auth, adminAuth, async (req: any, res: any) => { } } - target.role = nextRole; - await target.save(); + // Coordinate on the target document with deletion's conditional marker: + // either promotion wins and deletion refuses, or deletion wins and this + // update refuses. A stale hydrated target must never overwrite that guard. + const updated = await User.findOneAndUpdate( + { _id: userId, accountDeletionRequestedAt: { $exists: false } }, + { $set: { role: nextRole } }, + { new: true }, + ); + if (!updated) { + return res.status(409).json({ error: 'This account is being deleted; its role cannot change.' }); + } return res.json({ message: 'User role updated successfully', - user: sanitizeUser(target), + user: sanitizeUser(updated), }); } catch (error) { console.error('Failed to update user role:', error); diff --git a/backend/routes/registry/tokens.ts b/backend/routes/registry/tokens.ts index 39562953a0..83cd49d987 100644 --- a/backend/routes/registry/tokens.ts +++ b/backend/routes/registry/tokens.ts @@ -1,3 +1,4 @@ +import { isAccountDeleting } from '../../services/accountDeletionState'; // Token issuance + scope helpers — extracted from registry.js (GH#112) const AgentIdentityService = require('../../services/agentIdentityService'); const { hash, randomSecret } = require('../../utils/secret'); @@ -266,6 +267,26 @@ const runsInProcess = async (agentUser: any, installation: any): Promise { + const ownerId = issuer?.ownerUserId || installation?.installedBy; + const refuseDeletingOwner = async (tokenHash?: string, mintedHere = false) => { + if (!ownerId || !await isAccountDeleting(ownerId)) return; + if (tokenHash) { + // A shared identity can already hold another owner's token. A refusal + // must only revoke the credential this deleting owner actually owns. + // A new hash belongs to this request even on legacy reprovision paths + // that do not create an issuer-backed ledger row. + // eslint-disable-next-line global-require, @typescript-eslint/no-require-imports + const AgentCredential = require('../../models/AgentCredential'); + if (mintedHere || await AgentCredential.exists({ tokenHash, ownerUserId: ownerId })) { + await revokeRuntimeTokenEverywhere(tokenHash); + } + } + const error = Object.assign(new Error('This account is being deleted; runtime credentials cannot be issued.'), { + code: 'account_deleting', status: 409, + }); + throw error; + }; + await refuseDeletingOwner(); if (await runsInProcess(agentUser, installation)) throw new InProcessAgentTokenRefused(); // Check if agent already has a runtime token (reuse existing) const existingToken = agentUser.agentRuntimeTokens?.[0] || installation?.runtimeTokens?.[0]; @@ -291,6 +312,7 @@ const issueRuntimeTokenForAgent = async (agentUser: any, label: any, installatio { upsert: true }, ).catch((err: Error) => console.warn('Credential backfill failed:', err.message)); } + await refuseDeletingOwner(existingToken.tokenHash); return { existing: true, label: existingToken.label, @@ -350,6 +372,7 @@ const issueRuntimeTokenForAgent = async (agentUser: any, label: any, installatio throw new InProcessAgentTokenRefused(); } + await refuseDeletingOwner(tokenRecord.tokenHash, true); return { token: rawToken, label: label || 'Runtime token', diff --git a/backend/routes/uploads.ts b/backend/routes/uploads.ts index bc03b832a6..9dfa7ed6c3 100644 --- a/backend/routes/uploads.ts +++ b/backend/routes/uploads.ts @@ -273,7 +273,15 @@ const handleUpload = async ( } }; -router.post('/', auth, uploadSingle('image'), async (req: AuthReq, res: Res) => { +const uploadRateLimit = rateLimit({ + windowMs: 15 * 60_000, + max: 60, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: cloudflareIpRateLimitKeyGenerator, +}); + +router.post('/', uploadRateLimit, auth, uploadSingle('image'), async (req: AuthReq, res: Res) => { if (!req.userId) { res.status(401).json({ msg: 'auth required' }); return; diff --git a/backend/services/accountDeletionService.ts b/backend/services/accountDeletionService.ts index 87bf419d78..aed6919605 100644 --- a/backend/services/accountDeletionService.ts +++ b/backend/services/accountDeletionService.ts @@ -1,4 +1,6 @@ import jwt from 'jsonwebtoken'; +import { randomUUID } from 'crypto'; +import SchedulerHeartbeatLease from '../models/SchedulerHeartbeatLease'; import User from '../models/User'; import AccountDeletion from '../models/AccountDeletion'; import File from '../models/File'; @@ -58,16 +60,63 @@ const deleteOwnedUploads = async (userId: string): Promise => { } }; -/** Retried by the scheduler even after the human row has been removed. */ +export const ACCOUNT_DELETION_SWEEP_BATCH = 100; +const SWEEP_LEASE_ID = 'account-deletion-sweep'; +const SWEEP_LEASE_MS = 120_000; + +/** Bounded, fair retries retain intents until all late producers are fenced. */ export const sweepAccountDeletions = async (): Promise => { - const receipts = AccountDeletion.find().select('_id').lean().cursor(); - for await (const receipt of receipts) { - const userId = String(receipt._id); - // Reapply revocation/redaction too: a request authenticated before deletion - // can finish a credential mint or profile mirror after the first scan. - await revokeOwnedAccess(userId); - await anonymizeChatAuthor(userId); - await deleteOwnedUploads(userId); + const ownerId = randomUUID(); + const now = new Date(); + try { + await SchedulerHeartbeatLease.findOneAndUpdate( + { _id: SWEEP_LEASE_ID, expiresAt: { $lte: now } }, + { $set: { ownerId, expiresAt: new Date(now.getTime() + SWEEP_LEASE_MS) } }, + { upsert: true, new: true }, + ); + } catch (error) { + if ((error as { code?: number }).code === 11000) return; + throw error; + } + const errors: unknown[] = []; + const failedIds: string[] = []; + try { + const receipts = await AccountDeletion.find().sort({ lastSweptAt: 1, _id: 1 }) + .limit(ACCOUNT_DELETION_SWEEP_BATCH).select('_id').lean(); + for (const receipt of receipts) { + // Renew between receipts. If this worker lost its lease, stop starting + // work; expiry recovers a crashed worker and deletes remain idempotent. + // eslint-disable-next-line no-await-in-loop + const renewed = await SchedulerHeartbeatLease.updateOne( + { _id: SWEEP_LEASE_ID, ownerId }, + { $set: { expiresAt: new Date(Date.now() + SWEEP_LEASE_MS) } }, + ); + if (renewed.matchedCount !== 1) throw new Error('Account deletion sweep lease lost'); + const userId = String(receipt._id); + try { + // eslint-disable-next-line no-await-in-loop + await revokeOwnedAccess(userId); + // eslint-disable-next-line no-await-in-loop + await anonymizeChatAuthor(userId); + // eslint-disable-next-line no-await-in-loop + await deleteOwnedUploads(userId); + } catch (error) { + errors.push(error); + failedIds.push(userId); + } + // Rotate failed receipts too: one persistent failure cannot starve the + // remaining queue. Keep the intent, so every later round retries it. + try { + // eslint-disable-next-line no-await-in-loop + await AccountDeletion.updateOne({ _id: receipt._id }, { $set: { lastSweptAt: new Date() } }); + } catch (error) { + errors.push(error); + failedIds.push(userId); + } + } + if (errors.length) throw new AggregateError(errors, `Account deletion cleanup failed: ${failedIds.join(',')}`); + } finally { + await SchedulerHeartbeatLease.deleteOne({ _id: SWEEP_LEASE_ID, ownerId }); } }; @@ -97,8 +146,11 @@ export const deleteOwnAccount = async ( 'Sign in again with your OAuth provider, then confirm deletion within five minutes.'); } } - if (user.role === 'admin' && await User.countDocuments({ role: 'admin', isBot: { $ne: true } }) <= 1) { - throw new AccountDeletionError(409, 'LAST_ADMIN', 'Assign another administrator before deleting your account.'); + // Admins hand off their role before deleting. A separate count followed by + // deletion lets two admins both pass and remove the final administrators. + if (user.role === 'admin') { + throw new AccountDeletionError(409, 'ADMIN_HANDOFF_REQUIRED', + 'Transfer administration and ask another admin to make this a member account before deleting it.'); } // Cancel billing before destroying the linkage needed to stop future charges. @@ -112,16 +164,19 @@ export const deleteOwnAccount = async ( // Durable marker closes upload races and survives a storage failure. Keep the // human row until cleanup succeeds so the same re-authenticated request can retry. - await User.updateOne({ _id: userId }, { + const marked = await User.updateOne({ _id: userId, isBot: { $ne: true }, role: { $ne: 'admin' } }, { $set: { accountDeletionRequestedAt: new Date(), deviceTokens: [] }, $unset: { apiToken: 1, apiTokenCreatedAt: 1, apiTokenScopes: 1 }, }); + if (marked.matchedCount !== 1) { + throw new AccountDeletionError(409, 'ACCOUNT_CHANGED', 'Your account changed. Sign in again before deleting it.'); + } await AccountDeletion.updateOne({ _id: userId }, { $setOnInsert: { createdAt: new Date() } }, { upsert: true }); await revokeOwnedAccess(userId); await deleteOwnedUploads(userId); await anonymizeChatAuthor(userId); - await User.deleteOne({ _id: userId, isBot: { $ne: true } }); + await User.deleteOne({ _id: userId, isBot: { $ne: true }, role: { $ne: 'admin' } }); disconnectUserSessionSockets(userId); }; diff --git a/backend/services/accountDeletionState.ts b/backend/services/accountDeletionState.ts new file mode 100644 index 0000000000..6484369360 --- /dev/null +++ b/backend/services/accountDeletionState.ts @@ -0,0 +1,9 @@ +import User from '../models/User'; +import AccountDeletion from '../models/AccountDeletion'; + +/** The live marker covers intent creation; the receipt survives removal of User. */ +export const isAccountDeleting = async (userId: unknown): Promise => { + if (!userId) return false; + return Boolean(await AccountDeletion.exists({ _id: userId }) + || await User.exists({ _id: userId, accountDeletionRequestedAt: { $exists: true } })); +}; diff --git a/backend/services/schedulerService.ts b/backend/services/schedulerService.ts index e20c2409c5..7303d1857d 100644 --- a/backend/services/schedulerService.ts +++ b/backend/services/schedulerService.ts @@ -557,7 +557,8 @@ class SchedulerService { const { sweepAccountDeletions } = require('./accountDeletionService'); await sweepAccountDeletions(); } catch (error) { - const errorName = error instanceof Error ? error.name : 'Unknown error'; + const errorName = error instanceof AggregateError ? error.message + : error instanceof Error ? error.name : 'Unknown error'; console.error('[account-deletion] sweep failed:', errorName); } }, diff --git a/docs/api/account-deletion.md b/docs/api/account-deletion.md index 324315dccd..fbf952e9fe 100644 --- a/docs/api/account-deletion.md +++ b/docs/api/account-deletion.md @@ -13,8 +13,15 @@ Only the one-time OAuth exchange stamps `oauthAuthenticatedAt` on its signed JWT ordinary JWT issuance and session refresh do not supply that proof. API tokens, device tokens and agent runtime credentials cannot call this route. +The human upload endpoint is also limited to sixty requests per fifteen minutes +per client address, ahead of authentication and multipart parsing. + The endpoint is limited to ten attempts per fifteen minutes per client address. -It refuses to remove the last human administrator. +Administrators must hand off their role before deletion: another administrator +must first make the deleting account a member. Concurrent admin deletion +requests are refused without changing either account. The deletion marker is +conditional on the account still being a non-admin, and role updates refuse a +marked account, so a concurrent promotion and deletion cannot cross that boundary. ## Data and identity rules @@ -52,13 +59,28 @@ single transaction. After re-authentication and billing cancellation, a durable in flight recheck the marker after saving metadata and discard their upload if deletion has begun or the account is gone. -An `AccountDeletion` receipt retains only the opaque user id and creation time. -The scheduler retries that identity's access revocation, chat-author redaction -and upload cleanup every minute, including -bytes in its namespace that have no `File` record. The receipt survives User +An `AccountDeletion` receipt retains the opaque user id, creation time and last +attempt time. +A scheduler tick runs every minute under a renewable two-minute cluster lease. +Each tick processes at most 100 receipts, ordered by the oldest attempt. Failed +receipts also rotate to the back of the queue. One cleanup failure does not stop +other receipts: the sweep reports their failures as an aggregate after the batch, +with opaque receipt ids in the scheduler log. Lease expiry recovers a crashed +worker; operations are idempotent if an expired worker finishes a late operation. + +Each attempt retries access revocation, chat-author redaction and upload cleanup, +including bytes in its namespace that have no `File` record. The receipt survives User deletion: a late upload that fails its own cleanup is removed on a subsequent sweep when storage recovers. These receipts deliberately have no TTL so a delayed write cannot outlive its cleanup intent. They contain no profile, email or token. +Processing progress stores only `lastSweptAt`. A busy queue can take multiple ticks to revisit a receipt. + +Runtime-token issuance checks both the live deletion marker and the durable +receipt before mint/backfill and after its writes. A late write owned by the +deleting account is revoked from all token stores before issuance refuses. An +existing token owned by someone else is preserved. Runtime authentication also +refuses credentials whose owner has a deletion marker or receipt, covering the +interval before the issuer completes its post-write check. Bytes are removed before their `File` metadata. A failure in the deletion request's own storage or PG cleanup returns @@ -71,7 +93,7 @@ key. This does not replace provider retention policies or remove billing audit records and backups. Relevant errors: `CONFIRMATION_REQUIRED` (400), `PASSWORD_REQUIRED` or -`REAUTH_REQUIRED` (401), `BROWSER_SESSION_REQUIRED` (403), `LAST_ADMIN` (409). +`REAUTH_REQUIRED` (401), `BROWSER_SESSION_REQUIRED` (403), `ADMIN_HANDOFF_REQUIRED` or `ACCOUNT_CHANGED` (409). ## Verification From 7cd1d2033a79cfeeaa23cb1c8c082dc848e48501 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 08:29:35 -0700 Subject: [PATCH 03/13] fix(account): claim deletion before billing and key quotas by user Claim non-admin eligibility atomically before subscription cancellation. Release only the matching claim on provider failure; recover stale preflight claims. Keep authenticated user quotas independent of forwarding headers and sessions, with coarse transport-peer protection before auth. Real MongoDB/PostgreSQL deletion suite: 28/28. Focused regression: 236/236 across 19 suites; quota helper: 5/5. Typecheck and targeted lint pass. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../__tests__/service/accountDeletion.test.js | 28 ++++++++++- .../unit/middleware/ipRateLimit.test.js | 15 ++++++ .../unit/routes/uploads.post.test.js | 13 +++-- backend/middleware/ipRateLimit.ts | 13 ++++- backend/routes/uploads.ts | 16 ++++-- backend/routes/users.ts | 16 ++++-- backend/services/accountDeletionService.ts | 50 +++++++++++++------ docs/api/account-deletion.md | 26 ++++++++-- 8 files changed, 145 insertions(+), 32 deletions(-) diff --git a/backend/__tests__/service/accountDeletion.test.js b/backend/__tests__/service/accountDeletion.test.js index 2adec41e06..87e826b469 100644 --- a/backend/__tests__/service/accountDeletion.test.js +++ b/backend/__tests__/service/accountDeletion.test.js @@ -174,6 +174,27 @@ describe('Self-serve account deletion', () => { expect(await File.countDocuments()).toBe(0); }); + it('limits deletion by account despite rotating JWTs and forwarding headers', async () => { + const user = await makeUser(); + const tokens = [issueSessionToken(user), issueSessionToken(user, undefined, undefined, 1)]; + for (let attempt = 0; attempt < 10; attempt += 1) { + await deletion(app, tokens[attempt % 2], {}).expect(400); + } + await deletion(app, tokens[0], {}).expect(429); + expect(await User.findById(user._id)).not.toBeNull(); + expect(await AccountDeletion.countDocuments()).toBe(0); + }); + + it('recovers a stale billing preflight claim and refuses an active one', async () => { + const user = await makeUser({ accountDeletionRequestedAt: new Date() }); + const token = issueSessionToken(user); + await deletion(app, token).expect(409); + await User.updateOne({ _id: user._id }, { + $set: { accountDeletionRequestedAt: new Date(Date.now() - 301000) }, + }); + await deletion(app, token).expect(204); + }); + it('leaves the account and uploads intact if subscription cancellation fails', async () => { const user = await makeUser({ apiToken: 'cm_keep-on-billing-failure', billing: { subscriptionId: 'sub_fail', subscriptionStatus: 'active' } }); @@ -243,7 +264,9 @@ describe('Self-serve account deletion', () => { }); it('refuses deletion if a promotion wins before the conditional deletion marker', async () => { - const user = await makeUser(); await addFile(user, 'promotion.png'); + const user = await makeUser({ billing: { subscriptionId: 'sub_promoted', subscriptionStatus: 'active' } }); + await addFile(user, 'promotion.png'); + mockStripe.subscriptions.cancel.mockClear(); const updateOne = User.updateOne.bind(User); jest.spyOn(User, 'updateOne').mockImplementation((filter, update, ...rest) => { if (update.$set?.accountDeletionRequestedAt) return (async () => { @@ -254,6 +277,7 @@ describe('Self-serve account deletion', () => { }); await deletion(app, issueSessionToken(user)).expect(409); expect((await User.findById(user._id)).role).toBe('admin'); + expect(mockStripe.subscriptions.cancel).not.toHaveBeenCalled(); expect(await File.countDocuments({ uploadedBy: user._id })).toBe(1); expect(await AccountDeletion.countDocuments()).toBe(0); }); @@ -402,6 +426,6 @@ describe('Self-serve account deletion', () => { it('cancels a subscription before deleting its billing linkage', async () => { const user = await makeUser({ billing: { subscriptionId: 'sub_delete', subscriptionStatus: 'active' } }); await deletion(app, issueSessionToken(user)).expect(204); - expect(mockStripe.subscriptions.cancel).toHaveBeenCalledWith('sub_delete', {}, { idempotencyKey: `account-deletion-${user.id}` }); + expect(mockStripe.subscriptions.cancel).toHaveBeenCalledWith('sub_delete', {}, { idempotencyKey: `account-deletion-${user.id}-sub_delete`, timeout: 30000, maxNetworkRetries: 0 }); }); }); diff --git a/backend/__tests__/unit/middleware/ipRateLimit.test.js b/backend/__tests__/unit/middleware/ipRateLimit.test.js index 768dc9b9b3..7058ac72f1 100644 --- a/backend/__tests__/unit/middleware/ipRateLimit.test.js +++ b/backend/__tests__/unit/middleware/ipRateLimit.test.js @@ -30,3 +30,18 @@ describe('cloudflareIpRateLimitKeyGenerator', () => { expect(key).toBe('anon'); }); }); + +const { transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator } = require('../../../middleware/ipRateLimit'); + +describe('account mutation quota keys', () => { + it('uses the transport peer despite conflicting forwarding headers', () => { + expect(transportPeerRateLimitKeyGenerator({ socket: { remoteAddress: '192.0.2.4' }, + ip: '203.0.113.5', headers: { 'cf-connecting-ip': '198.51.100.6' } })) + .toBe(ipKeyGenerator('192.0.2.4')); + }); + it('uses the authenticated account rather than bearer or forwarding headers', () => { + expect(authenticatedUserRateLimitKeyGenerator({ userId: 'one', headers: { authorization: 'rotated' } })) + .toBe('user:one'); + expect(authenticatedUserRateLimitKeyGenerator({ userId: 'two' })).toBe('user:two'); + }); +}); diff --git a/backend/__tests__/unit/routes/uploads.post.test.js b/backend/__tests__/unit/routes/uploads.post.test.js index 94fa777d7d..7248adc41c 100644 --- a/backend/__tests__/unit/routes/uploads.post.test.js +++ b/backend/__tests__/unit/routes/uploads.post.test.js @@ -30,7 +30,7 @@ jest.mock('../../../models/User', () => ({ })); jest.mock('../../../middleware/auth', () => (req, res, next) => { - req.userId = 'user1'; + req.userId = /^Bearer quota-test-/.test(req.get('Authorization') || '') ? 'quota-user' : 'user1'; next(); }); @@ -72,11 +72,16 @@ describe('uploads POST / (ADR-002 Phase 1)', () => { expect(res.body.url).not.toMatch(/^https?:\/\//); }); - it('limits uploads to 60 per address per fifteen minutes', async () => { + it('limits one user despite forged forwarding headers and rotating sessions', async () => { for (let attempt = 0; attempt < 60; attempt += 1) { - await request(app).post('/api/uploads').set('cf-connecting-ip', '198.51.100.73').expect(400); + await request(app).post('/api/uploads') + .set('Authorization', `Bearer quota-test-${attempt % 2}`) + .set('cf-connecting-ip', `198.51.100.${attempt}`) + .set('X-Forwarded-For', `203.0.113.${attempt}`).expect(400); } - await request(app).post('/api/uploads').set('cf-connecting-ip', '198.51.100.73').expect(429); + await request(app).post('/api/uploads').set('Authorization', 'Bearer quota-test-new') + .set('cf-connecting-ip', '192.0.2.1').expect(429); + await request(app).post('/api/uploads').expect(400); }); it('returns 400 when no file is provided', async () => { diff --git a/backend/middleware/ipRateLimit.ts b/backend/middleware/ipRateLimit.ts index b45af015b6..8c275975f3 100644 --- a/backend/middleware/ipRateLimit.ts +++ b/backend/middleware/ipRateLimit.ts @@ -54,6 +54,17 @@ export const cloudflareIpRateLimitKeyGenerator = (req: Request): string => { return req.ip ? ipKeyGenerator(req.ip) : 'anon'; }; +// Coarse ingress buckets use only the connection peer; user quotas provide fairness behind proxies. +export const transportPeerRateLimitKeyGenerator = (req: Request): string => ( + req.socket?.remoteAddress ? ipKeyGenerator(req.socket.remoteAddress) : 'unknown-peer' +); + +export const authenticatedUserRateLimitKeyGenerator = (req: Request & { userId?: unknown }): string => ( + req.userId ? `user:${String(req.userId)}` : 'unresolved-user' +); + // CJS compat for the require()-style imports used elsewhere in backend/. // eslint-disable-next-line @typescript-eslint/no-require-imports -module.exports = { cloudflareIpRateLimitKeyGenerator }; +module.exports = { + cloudflareIpRateLimitKeyGenerator, transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, +}; diff --git a/backend/routes/uploads.ts b/backend/routes/uploads.ts index 9dfa7ed6c3..6452afa07a 100644 --- a/backend/routes/uploads.ts +++ b/backend/routes/uploads.ts @@ -29,7 +29,9 @@ import { verifyAttachmentToken, } from '../services/attachmentAccess'; import { logAttachmentTokenMint } from '../services/auditService'; -import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; +import { + cloudflareIpRateLimitKeyGenerator, transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, +} from '../middleware/ipRateLimit'; import { normalizeAvatarUrl } from '../services/avatarService'; // eslint-disable-next-line global-require const express = require('express'); @@ -273,15 +275,23 @@ const handleUpload = async ( } }; +const uploadIngressLimit = rateLimit({ + windowMs: 15 * 60_000, + max: 6000, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: transportPeerRateLimitKeyGenerator, +}); + const uploadRateLimit = rateLimit({ windowMs: 15 * 60_000, max: 60, standardHeaders: true, legacyHeaders: false, - keyGenerator: cloudflareIpRateLimitKeyGenerator, + keyGenerator: authenticatedUserRateLimitKeyGenerator, }); -router.post('/', uploadRateLimit, auth, uploadSingle('image'), async (req: AuthReq, res: Res) => { +router.post('/', uploadIngressLimit, auth, uploadRateLimit, uploadSingle('image'), async (req: AuthReq, res: Res) => { if (!req.userId) { res.status(401).json({ msg: 'auth required' }); return; diff --git a/backend/routes/users.ts b/backend/routes/users.ts index 81dab76553..619cc5f177 100644 --- a/backend/routes/users.ts +++ b/backend/routes/users.ts @@ -3,7 +3,9 @@ import rateLimit from 'express-rate-limit'; import { createHash } from 'crypto'; import { AccountDeletionError, deleteOwnAccount } from '../services/accountDeletionService'; -import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; +import { + cloudflareIpRateLimitKeyGenerator, transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, +} from '../middleware/ipRateLimit'; // eslint-disable-next-line global-require const express = require('express'); // eslint-disable-next-line global-require @@ -89,15 +91,23 @@ const profileWriteUserLimit = rateLimit({ }), }); +const accountDeletionIngressLimit = rateLimit({ + windowMs: 15 * 60_000, + max: 120, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: transportPeerRateLimitKeyGenerator, +}); + const accountDeletionRateLimit = rateLimit({ windowMs: 15 * 60_000, max: 10, standardHeaders: true, legacyHeaders: false, - keyGenerator: cloudflareIpRateLimitKeyGenerator, + keyGenerator: authenticatedUserRateLimitKeyGenerator, }); -router.delete('/me', accountDeletionRateLimit, auth, async (req: any, res: any) => { +router.delete('/me', accountDeletionIngressLimit, auth, accountDeletionRateLimit, async (req: any, res: any) => { if (req.authType !== 'jwt') { return res.status(403).json({ code: 'BROWSER_SESSION_REQUIRED', error: 'Sign in through the browser to delete your account.', diff --git a/backend/services/accountDeletionService.ts b/backend/services/accountDeletionService.ts index aed6919605..6678d0749d 100644 --- a/backend/services/accountDeletionService.ts +++ b/backend/services/accountDeletionService.ts @@ -153,25 +153,43 @@ export const deleteOwnAccount = async ( 'Transfer administration and ask another admin to make this a member account before deleting it.'); } - // Cancel billing before destroying the linkage needed to stop future charges. - if (user.billing?.subscriptionId && user.billing.subscriptionStatus !== 'canceled') { - const client = stripe(); - if (!client) throw new Error('Billing is unavailable; account deletion cannot cancel the subscription'); - await client.subscriptions.cancel(user.billing.subscriptionId, {}, { - idempotencyKey: `account-deletion-${userId}`, - }); + // A receipt means billing preflight already succeeded. Otherwise claim the + // non-admin row before Stripe. A stale claim recovers a crashed preflight; + // the provider request is bounded below this five-minute lease. + if (!await AccountDeletion.exists({ _id: userId })) { + const claimedAt = new Date(); + const claimed = await User.updateOne({ + _id: userId, isBot: { $ne: true }, role: { $ne: 'admin' }, + $or: [{ accountDeletionRequestedAt: { $exists: false } }, + { accountDeletionRequestedAt: { $lt: new Date(claimedAt.getTime() - 300_000) } }], + }, { $set: { accountDeletionRequestedAt: claimedAt } }); + if (claimed.matchedCount !== 1) { + throw new AccountDeletionError(409, 'ACCOUNT_CHANGED', + 'Your account changed or deletion is already in progress. Try again shortly.'); + } + try { + if (user.billing?.subscriptionId && user.billing.subscriptionStatus !== 'canceled') { + const client = stripe(); + if (!client) throw new Error('Billing is unavailable; account deletion cannot cancel the subscription'); + await client.subscriptions.cancel(user.billing.subscriptionId, {}, { + idempotencyKey: `account-deletion-${userId}-${user.billing.subscriptionId}`, + timeout: 30_000, + maxNetworkRetries: 0, + }); + } + } catch (error) { + // Release only this attempt's claim. Credentials and uploads are intact. + await User.updateOne({ _id: userId, accountDeletionRequestedAt: claimedAt }, + { $unset: { accountDeletionRequestedAt: 1 } }); + throw error; + } + await AccountDeletion.updateOne({ _id: userId }, { $setOnInsert: { createdAt: new Date() } }, { upsert: true }); } - - // Durable marker closes upload races and survives a storage failure. Keep the - // human row until cleanup succeeds so the same re-authenticated request can retry. - const marked = await User.updateOne({ _id: userId, isBot: { $ne: true }, role: { $ne: 'admin' } }, { - $set: { accountDeletionRequestedAt: new Date(), deviceTokens: [] }, + // Retain the human until cleanup succeeds for authenticated retries. + await User.updateOne({ _id: userId }, { + $set: { deviceTokens: [] }, $unset: { apiToken: 1, apiTokenCreatedAt: 1, apiTokenScopes: 1 }, }); - if (marked.matchedCount !== 1) { - throw new AccountDeletionError(409, 'ACCOUNT_CHANGED', 'Your account changed. Sign in again before deleting it.'); - } - await AccountDeletion.updateOne({ _id: userId }, { $setOnInsert: { createdAt: new Date() } }, { upsert: true }); await revokeOwnedAccess(userId); await deleteOwnedUploads(userId); diff --git a/docs/api/account-deletion.md b/docs/api/account-deletion.md index fbf952e9fe..1eb67fb59d 100644 --- a/docs/api/account-deletion.md +++ b/docs/api/account-deletion.md @@ -54,8 +54,15 @@ marked account, so a concurrent promotion and deletion cannot cross that boundar ## Failures and retries Cleanup spans MongoDB, the object store, PostgreSQL and Stripe, so it is not a -single transaction. After re-authentication and billing cancellation, a durable -`accountDeletionRequestedAt` marker prevents new uploads. Upload requests already +single transaction. After re-authentication, an atomic non-admin claim sets +`accountDeletionRequestedAt` before calling Stripe. The marker blocks promotion, +new uploads and owned runtime authentication during preflight. Stripe cancellation +has a 30-second timeout and no automatic network retry. Failure releases only +that attempt's marker and preserves credentials, uploads and the human row. +A crashed preflight can be reclaimed after five minutes. A timeout can have an +unknown provider outcome; retry uses the same account-and-subscription idempotency +key. After successful preflight, the durable cleanup receipt is created. Retries +with a receipt skip billing cancellation. Upload requests already in flight recheck the marker after saving metadata and discard their upload if deletion has begun or the account is gone. @@ -88,7 +95,7 @@ request's own storage or PG cleanup returns removed objects are safe to delete again. Owned API/device credentials are revoked during cleanup; the browser session stays available for retry until the human row is removed. A retry after the OAuth freshness window requires another -provider sign-in. Subscription cancellation uses an account-specific idempotency +provider sign-in. Subscription cancellation uses an account-and-subscription-specific idempotency key. This does not replace provider retention policies or remove billing audit records and backups. @@ -104,3 +111,16 @@ identity/memory continuity, token revocation, failed-cleanup retries and an upload that crosses deletion, including storage cleanup failure and a failed metadata save after the human row is gone. The positive OAuth case consumes a real one-time exchange code rather than constructing the accepted JWT claim in the fixture. + +## Request quotas + +Deletion permits ten attempts per user per fifteen minutes; human uploads permit +sixty. These quotas run after authentication and use the durable user id, so +rotating sessions or forged forwarding headers cannot reset them. Before auth, +a coarse transport-peer bucket permits 120 deletion or 6,000 upload requests per +fifteen minutes. It uses the socket peer only. Behind a proxy that bucket is +shared, so it is intentionally broader than the user quota. + +Agent ownership recovery after deletion is awaiting a product ruling. The current +implementation preserves original ownership, pauses owned runtimes and revokes +owned credentials. It does not introduce an ownership transfer mechanism. From 70e86dd77d7f561188874c4d73440af31418a9d6 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 07:58:56 -0700 Subject: [PATCH 04/13] feat(settings): add deliberate account deletion flow Co-authored-by: lark Agent-Model: codex/gpt-6.1-sol/high Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- frontend/.eslintrc.js | 1 + frontend/src/i18n/locales/en.json | 12 ++ frontend/src/i18n/locales/zh-CN.json | 12 ++ .../src/v2/__tests__/V2DeleteAccount.test.tsx | 142 ++++++++++++++++++ .../src/v2/__tests__/V2SettingsPage.test.tsx | 1 + .../src/v2/components/V2DeleteAccount.tsx | 138 +++++++++++++++++ frontend/src/v2/components/V2SettingsPage.tsx | 6 +- 7 files changed, 311 insertions(+), 1 deletion(-) create mode 100644 frontend/src/v2/__tests__/V2DeleteAccount.test.tsx create mode 100644 frontend/src/v2/components/V2DeleteAccount.tsx diff --git a/frontend/.eslintrc.js b/frontend/.eslintrc.js index d543067f5b..4294b18489 100644 --- a/frontend/.eslintrc.js +++ b/frontend/.eslintrc.js @@ -50,6 +50,7 @@ module.exports = { 'src/v2/components/V2ResetPassword.tsx', 'src/v2/components/V2OAuthButtons.tsx', 'src/v2/components/V2OAuthComplete.tsx', + 'src/v2/components/V2DeleteAccount.tsx', 'src/v2/components/V2InviteRedeem.tsx', 'src/components/RegistrationInviteRequired.tsx', 'src/components/VerifyEmail.tsx', diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 96bf713716..473d44c6cd 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -54,6 +54,18 @@ "removeError": "Couldn’t remove this skill." }, "settings": { + "deleteAccount": { + "title": "Delete account", + "effects": "This permanently deletes your account and uploads. Shared messages are retained. Installed agent identities and memory survive. This cannot be undone.", + "password": "Password (if your account has one)", + "confirmation": "Type DELETE to confirm", + "oauthHint": "If you sign in with Google or GitHub, you may need to sign in again with the same account before deleting.", + "signInAgain": "Sign in again with Google or GitHub, then return here to confirm", + "confirm": "Permanently delete account", + "deleting": "Deleting…", + "cancel": "Cancel", + "failed": "Couldn’t delete your account. Try again." + }, "sessions": { "signOutEverywhere": "Sign out everywhere", "confirm": "Sign out of every browser session, including this one?", diff --git a/frontend/src/i18n/locales/zh-CN.json b/frontend/src/i18n/locales/zh-CN.json index b644f2d440..ce70b26a1e 100644 --- a/frontend/src/i18n/locales/zh-CN.json +++ b/frontend/src/i18n/locales/zh-CN.json @@ -54,6 +54,18 @@ "removeError": "无法移除此技能。" }, "settings": { + "deleteAccount": { + "title": "删除账户", + "effects": "这将永久删除你的账户和上传的文件。共享消息会保留。已安装的智能体身份和记忆会保留。此操作无法撤销。", + "password": "密码（如果账户设有密码）", + "confirmation": "输入 DELETE 以确认", + "oauthHint": "如果你使用 Google 或 GitHub 登录，删除前可能需要使用同一账户重新登录。", + "signInAgain": "使用 Google 或 GitHub 重新登录，然后返回此处确认", + "confirm": "永久删除账户", + "deleting": "正在删除…", + "cancel": "取消", + "failed": "无法删除你的账户。请重试。" + }, "sessions": { "signOutEverywhere": "退出所有浏览器会话", "confirm": "退出所有浏览器会话，包括当前会话？", diff --git a/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx b/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx new file mode 100644 index 0000000000..6c6b7665d9 --- /dev/null +++ b/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx @@ -0,0 +1,142 @@ +import React from 'react'; +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { MemoryRouter, Route, Routes } from 'react-router-dom'; +import axios from '../../utils/axiosConfig'; +import { AuthContext } from '../../context/AuthContext'; +import i18n, { i18nReady } from '../../i18n'; +import V2DeleteAccount from '../components/V2DeleteAccount'; +import V2Login from '../components/V2Login'; + +jest.mock('../../utils/axiosConfig', () => ({ + __esModule: true, + default: { delete: jest.fn(), get: jest.fn() }, +})); + +const logout = jest.fn(); +const auth = { + currentUser: { _id: 'u1', username: 'me', email: 'me@example.com' }, + user: null, token: 'jwt', loading: false, error: null, isAuthenticated: true, + register: jest.fn(), login: jest.fn(), signInWithToken: jest.fn(), logout, updateProfile: jest.fn(), +}; + +const renderDeletion = () => render( + + + + } /> + } /> + + + , +); + +const openDeletion = () => fireEvent.click(screen.getByRole('button', { name: 'Delete account' })); +const confirmDeletion = (value = 'DELETE') => fireEvent.change(screen.getByLabelText('Type DELETE to confirm'), { target: { value } }); +const submit = () => fireEvent.click(screen.getByRole('button', { name: 'Permanently delete account' })); + +beforeEach(async () => { + jest.resetAllMocks(); + localStorage.clear(); + await i18nReady; + await i18n.changeLanguage('en'); + (axios.get as jest.Mock).mockResolvedValue({ data: { providers: [{ id: 'github', label: 'GitHub' }] } }); +}); + +test('requires an exact deliberate confirmation, explains retention, and cancel clears sensitive fields', () => { + renderDeletion(); + expect(screen.queryByRole('form')).not.toBeInTheDocument(); + openDeletion(); + expect(screen.getByText(/permanently deletes your account and uploads/)).toHaveTextContent('Shared messages are retained. Installed agent identities and memory survive.'); + expect(screen.getByLabelText('Type DELETE to confirm')).toHaveFocus(); + confirmDeletion('delete'); + expect(screen.getByRole('button', { name: 'Permanently delete account' })).toBeDisabled(); + // Form submission itself must enforce the guard, including Enter/programmatic submits. + fireEvent.submit(screen.getByRole('form', { name: 'Delete account' })); + expect(axios.delete).not.toHaveBeenCalled(); + fireEvent.change(screen.getByLabelText('Password (if your account has one)'), { target: { value: 'secret' } }); + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })); + expect(screen.getByRole('button', { name: 'Delete account' })).toHaveFocus(); + openDeletion(); + expect(screen.getByLabelText('Password (if your account has one)')).toHaveValue(''); + expect(screen.getByLabelText('Type DELETE to confirm')).toHaveValue(''); + expect(logout).not.toHaveBeenCalled(); +}); + +test('sends the password in the DELETE body and logs out only after success; blocks duplicate submits', async () => { + let resolve!: (value: unknown) => void; + (axios.delete as jest.Mock).mockReturnValue(new Promise((done) => { resolve = done; })); + renderDeletion(); + openDeletion(); + confirmDeletion(); + fireEvent.change(screen.getByLabelText('Password (if your account has one)'), { target: { value: 'correct password' } }); + submit(); + fireEvent.submit(screen.getByRole('form', { name: 'Delete account' })); + expect(axios.delete).toHaveBeenCalledTimes(1); + expect(axios.delete).toHaveBeenCalledWith('/api/users/me', { data: { password: 'correct password', confirmation: 'DELETE' } }); + expect(logout).not.toHaveBeenCalled(); + expect(screen.getByRole('button', { name: 'Deleting…' })).toBeDisabled(); + expect(screen.getByRole('button', { name: 'Cancel' })).toBeDisabled(); + await act(async () => { resolve({ data: { success: true } }); }); + expect(logout).toHaveBeenCalledTimes(1); +}); + +test('keeps the session on failure, clears the error on retry and omits an unused password', async () => { + (axios.delete as jest.Mock).mockRejectedValueOnce(new Error('offline')).mockResolvedValueOnce({ data: {} }); + renderDeletion(); + openDeletion(); + confirmDeletion(); + submit(); + expect(await screen.findByRole('alert')).toHaveTextContent('Couldn’t delete your account. Try again.'); + expect(logout).not.toHaveBeenCalled(); + submit(); + await waitFor(() => expect(logout).toHaveBeenCalledTimes(1)); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + expect(axios.delete).toHaveBeenLastCalledWith('/api/users/me', { data: { confirmation: 'DELETE' } }); +}); + +test('PASSWORD_REQUIRED focuses a required password field and allows correction', async () => { + (axios.delete as jest.Mock).mockRejectedValueOnce({ response: { data: { code: 'PASSWORD_REQUIRED', error: 'Enter your password.' } } }) + .mockResolvedValueOnce({ data: {} }); + renderDeletion(); + openDeletion(); + confirmDeletion(); + submit(); + expect(await screen.findByRole('alert')).toHaveTextContent('Enter your password.'); + const password = screen.getByLabelText('Password (if your account has one)'); + expect(password).toBeRequired(); + expect(password).toHaveFocus(); + expect(screen.getByRole('button', { name: 'Permanently delete account' })).toBeDisabled(); + expect(logout).not.toHaveBeenCalled(); + fireEvent.change(password, { target: { value: 'corrected' } }); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + submit(); + await waitFor(() => expect(logout).toHaveBeenCalledTimes(1)); +}); + +test('REAUTH_REQUIRED sends the existing OAuth login flow back to Settings without logging out or auto-deleting', async () => { + (axios.delete as jest.Mock).mockRejectedValueOnce({ response: { data: { code: 'REAUTH_REQUIRED', error: 'Sign in again.' } } }); + renderDeletion(); + openDeletion(); + confirmDeletion(); + submit(); + expect(await screen.findByRole('alert')).toHaveTextContent('Sign in again.'); + const link = screen.getByRole('link', { name: /Sign in again with Google or GitHub/ }); + expect(link).toHaveAttribute('href', '/v2/login?next=/v2/settings'); + fireEvent.click(link); + const oauth = await screen.findByRole('link', { name: /Continue with GitHub/ }); + expect(new URL(oauth.getAttribute('href')!, 'http://localhost').searchParams.get('next')).toBe('/v2/settings'); + expect(logout).not.toHaveBeenCalled(); + expect(axios.delete).toHaveBeenCalledTimes(1); +}); + +test('shows the backend rejection and keeps confirmation available for retry', async () => { + (axios.delete as jest.Mock).mockRejectedValueOnce({ response: { data: { error: 'Incorrect password.' } } }); + renderDeletion(); + openDeletion(); + confirmDeletion(); + submit(); + expect(await screen.findByRole('alert')).toHaveTextContent('Incorrect password.'); + expect(screen.getByLabelText('Type DELETE to confirm')).toHaveValue('DELETE'); + expect(screen.getByRole('button', { name: 'Permanently delete account' })).toBeEnabled(); + expect(logout).not.toHaveBeenCalled(); +}); diff --git a/frontend/src/v2/__tests__/V2SettingsPage.test.tsx b/frontend/src/v2/__tests__/V2SettingsPage.test.tsx index 472c15b1c8..9ba2c8a308 100644 --- a/frontend/src/v2/__tests__/V2SettingsPage.test.tsx +++ b/frontend/src/v2/__tests__/V2SettingsPage.test.tsx @@ -121,6 +121,7 @@ describe('V2SettingsPage', () => { renderSettings(); expect(screen.getByRole('heading', { name: 'Settings' })).toBeInTheDocument(); + expect(within(screen.getByRole('region', { name: 'Account' })).getByRole('button', { name: 'Delete account' })).toBeInTheDocument(); for (const section of [ { id: 'account', title: 'Account' }, { id: 'plan', title: 'Plan' }, diff --git a/frontend/src/v2/components/V2DeleteAccount.tsx b/frontend/src/v2/components/V2DeleteAccount.tsx new file mode 100644 index 0000000000..e02ab39be6 --- /dev/null +++ b/frontend/src/v2/components/V2DeleteAccount.tsx @@ -0,0 +1,138 @@ +import React, { useEffect, useRef, useState } from 'react'; +import { Link } from 'react-router-dom'; +import { useTranslation } from 'react-i18next'; +import axios from '../../utils/axiosConfig'; +import { useAuth } from '../../context/AuthContext'; + +const V2DeleteAccount: React.FC = () => { + const { logout } = useAuth(); + const { t } = useTranslation(); + const [open, setOpen] = useState(false); + const [confirmation, setConfirmation] = useState(''); + const [password, setPassword] = useState(''); + const [busy, setBusy] = useState(false); + const [error, setError] = useState(null); + const [reauthRequired, setReauthRequired] = useState(false); + const [passwordRequired, setPasswordRequired] = useState(false); + const pending = useRef(false); + const triggerRef = useRef(null); + const passwordRef = useRef(null); + const confirmationRef = useRef(null); + + useEffect(() => { if (open) confirmationRef.current?.focus(); }, [open]); + useEffect(() => { + if (passwordRequired && !busy) passwordRef.current?.focus(); + }, [passwordRequired, busy]); + + const cancel = () => { + setOpen(false); + setPassword(''); + setConfirmation(''); + setError(null); + setReauthRequired(false); + setPasswordRequired(false); + triggerRef.current?.focus(); + }; + + const remove = async (event: React.FormEvent) => { + event.preventDefault(); + if (pending.current || confirmation !== 'DELETE' || (passwordRequired && !password)) return; + pending.current = true; + setBusy(true); + setError(null); + setReauthRequired(false); + try { + await axios.delete('/api/users/me', { + data: { confirmation: 'DELETE', ...(password ? { password } : {}) }, + }); + } catch (requestError) { + const data = (requestError as { response?: { data?: { code?: string; error?: string } } })?.response?.data; + setError(data?.error || t('settings.deleteAccount.failed')); + setReauthRequired(data?.code === 'REAUTH_REQUIRED'); + if (data?.code === 'PASSWORD_REQUIRED') { + setPasswordRequired(true); + } + return; + } finally { + pending.current = false; + setBusy(false); + } + setPassword(''); + logout(); + }; + + return ( +
+ + {open && ( +
{ void remove(event); }} + > +

{t('settings.deleteAccount.effects')}

+
+ + +
+

{t('settings.deleteAccount.oauthHint')}

+ {error &&

{error}

} + {reauthRequired && ( +

+ {t('settings.deleteAccount.signInAgain')} +

+ )} +
+ + +
+
+ )} +
+ ); +}; + +export default V2DeleteAccount; diff --git a/frontend/src/v2/components/V2SettingsPage.tsx b/frontend/src/v2/components/V2SettingsPage.tsx index 5ffbdcaa29..9402504c15 100644 --- a/frontend/src/v2/components/V2SettingsPage.tsx +++ b/frontend/src/v2/components/V2SettingsPage.tsx @@ -7,6 +7,7 @@ import { useAuth } from '../../context/AuthContext'; import AppsManagement from '../../components/AppsManagement'; import V2BillingPanel from './V2BillingPanel'; import V2DevicesPanel from './V2DevicesPanel'; +import V2DeleteAccount from './V2DeleteAccount'; import V2Avatar from './V2Avatar'; import V2AvatarCropDialog from './V2AvatarCropDialog'; import { nextPaperAvatarPreset } from '../utils/avatarPresets'; @@ -540,7 +541,10 @@ const V2SettingsPage: React.FC = () => {
- + + + + From 4ec56513bf40d7e359352782ad63e28d39b032c4 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 08:04:55 -0700 Subject: [PATCH 05/13] fix(settings): disclose subscription cancellation on account deletion Co-authored-by: lark Agent-Model: codex/gpt-6.1-sol/high Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- frontend/src/i18n/locales/en.json | 2 +- frontend/src/i18n/locales/zh-CN.json | 2 +- frontend/src/v2/__tests__/V2DeleteAccount.test.tsx | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 473d44c6cd..f8ac24d79d 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -56,7 +56,7 @@ "settings": { "deleteAccount": { "title": "Delete account", - "effects": "This permanently deletes your account and uploads. Shared messages are retained. Installed agent identities and memory survive. This cannot be undone.", + "effects": "This permanently deletes your account and uploads and cancels any active Stripe subscription. Shared messages are retained. Installed agent identities and memory survive. This cannot be undone.", "password": "Password (if your account has one)", "confirmation": "Type DELETE to confirm", "oauthHint": "If you sign in with Google or GitHub, you may need to sign in again with the same account before deleting.", diff --git a/frontend/src/i18n/locales/zh-CN.json b/frontend/src/i18n/locales/zh-CN.json index ce70b26a1e..3133599bed 100644 --- a/frontend/src/i18n/locales/zh-CN.json +++ b/frontend/src/i18n/locales/zh-CN.json @@ -56,7 +56,7 @@ "settings": { "deleteAccount": { "title": "删除账户", - "effects": "这将永久删除你的账户和上传的文件。共享消息会保留。已安装的智能体身份和记忆会保留。此操作无法撤销。", + "effects": "这将永久删除你的账户和上传的文件，并取消所有有效的 Stripe 订阅。共享消息会保留。已安装的智能体身份和记忆会保留。此操作无法撤销。", "password": "密码（如果账户设有密码）", "confirmation": "输入 DELETE 以确认", "oauthHint": "如果你使用 Google 或 GitHub 登录，删除前可能需要使用同一账户重新登录。", diff --git a/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx b/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx index 6c6b7665d9..9605e0bfe9 100644 --- a/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx +++ b/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx @@ -46,7 +46,7 @@ test('requires an exact deliberate confirmation, explains retention, and cancel renderDeletion(); expect(screen.queryByRole('form')).not.toBeInTheDocument(); openDeletion(); - expect(screen.getByText(/permanently deletes your account and uploads/)).toHaveTextContent('Shared messages are retained. Installed agent identities and memory survive.'); + expect(screen.getByText(/permanently deletes your account and uploads/)).toHaveTextContent('cancels any active Stripe subscription. Shared messages are retained. Installed agent identities and memory survive.'); expect(screen.getByLabelText('Type DELETE to confirm')).toHaveFocus(); confirmDeletion('delete'); expect(screen.getByRole('button', { name: 'Permanently delete account' })).toBeDisabled(); @@ -62,7 +62,7 @@ test('requires an exact deliberate confirmation, explains retention, and cancel expect(logout).not.toHaveBeenCalled(); }); -test('sends the password in the DELETE body and logs out only after success; blocks duplicate submits', async () => { +test('sends the password in the DELETE body and logs out only after a 204; blocks duplicate submits', async () => { let resolve!: (value: unknown) => void; (axios.delete as jest.Mock).mockReturnValue(new Promise((done) => { resolve = done; })); renderDeletion(); @@ -76,7 +76,7 @@ test('sends the password in the DELETE body and logs out only after success; blo expect(logout).not.toHaveBeenCalled(); expect(screen.getByRole('button', { name: 'Deleting…' })).toBeDisabled(); expect(screen.getByRole('button', { name: 'Cancel' })).toBeDisabled(); - await act(async () => { resolve({ data: { success: true } }); }); + await act(async () => { resolve({ status: 204, data: '' }); }); expect(logout).toHaveBeenCalledTimes(1); }); From 34907a839f473904d8e17f3cae1bfa60b23924cc Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 08:15:44 -0700 Subject: [PATCH 06/13] fix(settings): disclose runtime pause during account deletion Co-authored-by: lark Agent-Model: codex/gpt-6.1-sol/high Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- frontend/src/i18n/locales/en.json | 2 +- frontend/src/i18n/locales/zh-CN.json | 2 +- frontend/src/v2/__tests__/V2DeleteAccount.test.tsx | 13 +++++++++---- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index f8ac24d79d..65cb9f9baa 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -56,7 +56,7 @@ "settings": { "deleteAccount": { "title": "Delete account", - "effects": "This permanently deletes your account and uploads and cancels any active Stripe subscription. Shared messages are retained. Installed agent identities and memory survive. This cannot be undone.", + "effects": "This permanently deletes your account and uploads and cancels any active Stripe subscription. Shared messages are retained. Agent runtimes you installed are paused. Their identities and memory survive. This cannot be undone.", "password": "Password (if your account has one)", "confirmation": "Type DELETE to confirm", "oauthHint": "If you sign in with Google or GitHub, you may need to sign in again with the same account before deleting.", diff --git a/frontend/src/i18n/locales/zh-CN.json b/frontend/src/i18n/locales/zh-CN.json index 3133599bed..186cda3003 100644 --- a/frontend/src/i18n/locales/zh-CN.json +++ b/frontend/src/i18n/locales/zh-CN.json @@ -56,7 +56,7 @@ "settings": { "deleteAccount": { "title": "删除账户", - "effects": "这将永久删除你的账户和上传的文件，并取消所有有效的 Stripe 订阅。共享消息会保留。已安装的智能体身份和记忆会保留。此操作无法撤销。", + "effects": "这将永久删除你的账户和上传的文件，并取消所有有效的 Stripe 订阅。共享消息会保留。你安装的智能体运行时会暂停。它们的身份和记忆会保留。此操作无法撤销。", "password": "密码（如果账户设有密码）", "confirmation": "输入 DELETE 以确认", "oauthHint": "如果你使用 Google 或 GitHub 登录，删除前可能需要使用同一账户重新登录。", diff --git a/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx b/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx index 9605e0bfe9..8d82900f29 100644 --- a/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx +++ b/frontend/src/v2/__tests__/V2DeleteAccount.test.tsx @@ -46,7 +46,7 @@ test('requires an exact deliberate confirmation, explains retention, and cancel renderDeletion(); expect(screen.queryByRole('form')).not.toBeInTheDocument(); openDeletion(); - expect(screen.getByText(/permanently deletes your account and uploads/)).toHaveTextContent('cancels any active Stripe subscription. Shared messages are retained. Installed agent identities and memory survive.'); + expect(screen.getByText(/permanently deletes your account and uploads/)).toHaveTextContent('cancels any active Stripe subscription. Shared messages are retained. Agent runtimes you installed are paused. Their identities and memory survive.'); expect(screen.getByLabelText('Type DELETE to confirm')).toHaveFocus(); confirmDeletion('delete'); expect(screen.getByRole('button', { name: 'Permanently delete account' })).toBeDisabled(); @@ -129,13 +129,18 @@ test('REAUTH_REQUIRED sends the existing OAuth login flow back to Settings witho expect(axios.delete).toHaveBeenCalledTimes(1); }); -test('shows the backend rejection and keeps confirmation available for retry', async () => { - (axios.delete as jest.Mock).mockRejectedValueOnce({ response: { data: { error: 'Incorrect password.' } } }); +test.each([ + [401, 'INVALID_PASSWORD', 'Incorrect password.'], + [409, 'ADMIN_HANDOFF_REQUIRED', 'Hand off administration before deleting your account.'], + [403, 'BROWSER_SESSION_REQUIRED', 'Use a browser session to delete your account.'], + [503, 'CLEANUP_FAILED', 'Cleanup could not finish. Try again.'], +])('shows a %s %s rejection and keeps confirmation available for retry', async (status, code, error) => { + (axios.delete as jest.Mock).mockRejectedValueOnce({ response: { status, data: { code, error } } }); renderDeletion(); openDeletion(); confirmDeletion(); submit(); - expect(await screen.findByRole('alert')).toHaveTextContent('Incorrect password.'); + expect(await screen.findByRole('alert')).toHaveTextContent(error); expect(screen.getByLabelText('Type DELETE to confirm')).toHaveValue('DELETE'); expect(screen.getByRole('button', { name: 'Permanently delete account' })).toBeEnabled(); expect(logout).not.toHaveBeenCalled(); From 49ab23f200b90f03845e8c1ec817a48b98e9d279 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 08:33:37 -0700 Subject: [PATCH 07/13] fix(account): recover cleanup intents after receipt write failure Reconcile stale authorized deletion markers without a receipt under the bounded sweep lease. Repeat billing preflight before creating cleanup intent. Renew the lease between recovered users and receipts; preserve failed recovery for retry. Clear embedded human credentials in the shared cleanup path as well. Real MongoDB/PostgreSQL service suite 29/29; focused backend 239/239 across 19 suites; disabling marker recovery fails its regression. Frontend UI from Lark passes 1757 Jest tests, typecheck, build and four script tests. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../__tests__/service/accountDeletion.test.js | 19 +++ backend/models/User.ts | 5 + backend/services/accountDeletionService.ts | 121 +++++++++++------- docs/api/account-deletion.md | 6 + 4 files changed, 108 insertions(+), 43 deletions(-) diff --git a/backend/__tests__/service/accountDeletion.test.js b/backend/__tests__/service/accountDeletion.test.js index 87e826b469..8f219f1161 100644 --- a/backend/__tests__/service/accountDeletion.test.js +++ b/backend/__tests__/service/accountDeletion.test.js @@ -195,6 +195,25 @@ describe('Self-serve account deletion', () => { await deletion(app, token).expect(204); }); + it('recovers a failed receipt write without relying on a browser retry', async () => { + const user = await makeUser({ billing: { subscriptionId: 'sub_receipt', subscriptionStatus: 'active' } }); + await addFile(user, 'stranded.png'); + jest.spyOn(AccountDeletion, 'updateOne').mockRejectedValueOnce(new Error('Receipt store unavailable')); + await deletion(app, issueSessionToken(user)).expect(503); + expect(await AccountDeletion.countDocuments()).toBe(0); + expect((await User.findById(user._id)).accountDeletionRequestedAt).toBeDefined(); + await sweepAccountDeletions(); + expect(await AccountDeletion.countDocuments()).toBe(0); // Active preflight is left alone. + await User.updateOne({ _id: user._id }, { + $set: { accountDeletionRequestedAt: new Date(Date.now() - 301000) }, + }); + await sweepAccountDeletions(); + expect(await AccountDeletion.countDocuments()).toBe(1); + expect(await File.countDocuments({ uploadedBy: user._id })).toBe(0); + expect(await MediaObject.findOne({ key: 'stranded.png' })).toBeNull(); + await deletion(app, issueSessionToken(user)).expect(204); + }); + it('leaves the account and uploads intact if subscription cancellation fails', async () => { const user = await makeUser({ apiToken: 'cm_keep-on-billing-failure', billing: { subscriptionId: 'sub_fail', subscriptionStatus: 'active' } }); diff --git a/backend/models/User.ts b/backend/models/User.ts index 7f7fe1ce1b..ca97f48f6b 100644 --- a/backend/models/User.ts +++ b/backend/models/User.ts @@ -498,6 +498,11 @@ userSchema.methods.getOrCreateDigestUnsubscribeToken = function (): string { return this.digestUnsubscribeToken; }; +// Recovery scans only authorized deletion markers, ordered for bounded retries. +userSchema.index({ accountDeletionRequestedAt: 1, _id: 1 }, { + partialFilterExpression: { accountDeletionRequestedAt: { $exists: true } }, +}); + export default mongoose.model('User', userSchema); // CJS compat: let require() return the default export directly // eslint-disable-next-line @typescript-eslint/no-require-imports diff --git a/backend/services/accountDeletionService.ts b/backend/services/accountDeletionService.ts index 6678d0749d..6e307c3a26 100644 --- a/backend/services/accountDeletionService.ts +++ b/backend/services/accountDeletionService.ts @@ -1,7 +1,7 @@ import jwt from 'jsonwebtoken'; import { randomUUID } from 'crypto'; import SchedulerHeartbeatLease from '../models/SchedulerHeartbeatLease'; -import User from '../models/User'; +import User, { IUser } from '../models/User'; import AccountDeletion from '../models/AccountDeletion'; import File from '../models/File'; import DeviceAuthorization from '../models/DeviceAuthorization'; @@ -23,6 +23,11 @@ export class AccountDeletionError extends Error { } const revokeOwnedAccess = async (userId: string): Promise => { + await User.updateOne({ _id: userId }, { + $set: { deviceTokens: [] }, + $unset: { apiToken: 1, apiTokenCreatedAt: 1, apiTokenScopes: 1 }, + }); + const credentials = await AgentCredential.find({ ownerUserId: userId }).select('_id').lean(); await AgentCredential.revokeCascade(credentials.map((credential) => credential._id)); await Machine.deleteMany({ ownerUserId: userId }); @@ -60,6 +65,46 @@ const deleteOwnedUploads = async (userId: string): Promise => { } }; +// Only called after request re-authentication or for an already authorized, +// stale marker. Recovery retains a failed preflight for a later bounded retry. +const establishCleanupReceipt = async (user: IUser, releaseOnBillingFailure = true): Promise => { + const userId = String(user._id); + // A receipt means billing preflight already succeeded. Otherwise claim the + // non-admin row before Stripe. A stale claim recovers a crashed preflight; + // the provider request is bounded below this five-minute lease. + if (!await AccountDeletion.exists({ _id: userId })) { + const claimedAt = new Date(); + const claimed = await User.updateOne({ + _id: userId, isBot: { $ne: true }, role: { $ne: 'admin' }, + $or: [{ accountDeletionRequestedAt: { $exists: false } }, + { accountDeletionRequestedAt: { $lt: new Date(claimedAt.getTime() - 300_000) } }], + }, { $set: { accountDeletionRequestedAt: claimedAt } }); + if (claimed.matchedCount !== 1) { + throw new AccountDeletionError(409, 'ACCOUNT_CHANGED', + 'Your account changed or deletion is already in progress. Try again shortly.'); + } + try { + if (user.billing?.subscriptionId && user.billing.subscriptionStatus !== 'canceled') { + const client = stripe(); + if (!client) throw new Error('Billing is unavailable; account deletion cannot cancel the subscription'); + await client.subscriptions.cancel(user.billing.subscriptionId, {}, { + idempotencyKey: `account-deletion-${userId}-${user.billing.subscriptionId}`, + timeout: 30_000, + maxNetworkRetries: 0, + }); + } + } catch (error) { + // Release only this attempt's claim. Credentials and uploads are intact. + if (releaseOnBillingFailure) { + await User.updateOne({ _id: userId, accountDeletionRequestedAt: claimedAt }, + { $unset: { accountDeletionRequestedAt: 1 } }); + } + throw error; + } + await AccountDeletion.updateOne({ _id: userId }, { $setOnInsert: { createdAt: new Date() } }, { upsert: true }); + } +}; + export const ACCOUNT_DELETION_SWEEP_BATCH = 100; const SWEEP_LEASE_ID = 'account-deletion-sweep'; const SWEEP_LEASE_MS = 120_000; @@ -78,20 +123,46 @@ export const sweepAccountDeletions = async (): Promise => { if ((error as { code?: number }).code === 11000) return; throw error; } + const renewLease = async (): Promise => { + const renewed = await SchedulerHeartbeatLease.updateOne( + { _id: SWEEP_LEASE_ID, ownerId }, + { $set: { expiresAt: new Date(Date.now() + SWEEP_LEASE_MS) } }, + ); + if (renewed.matchedCount !== 1) throw new Error('Account deletion sweep lease lost'); + }; const errors: unknown[] = []; const failedIds: string[] = []; try { + // Recover the gap between the authorized marker and receipt creation. + // Fresh billing preflights are excluded. Existing receipts need no recovery. + const stranded = await User.aggregate([ + { $match: { accountDeletionRequestedAt: { $lt: new Date(Date.now() - 300_000) }, + role: { $ne: 'admin' }, isBot: { $ne: true } } }, + { $sort: { accountDeletionRequestedAt: 1, _id: 1 } }, + { $lookup: { from: AccountDeletion.collection.name, localField: '_id', + foreignField: '_id', as: 'cleanupReceipt' } }, + { $match: { 'cleanupReceipt.0': { $exists: false } } }, + { $limit: ACCOUNT_DELETION_SWEEP_BATCH }, + { $unset: 'cleanupReceipt' }, + ]); + for (const user of stranded) { + // eslint-disable-next-line no-await-in-loop + await renewLease(); + try { + // eslint-disable-next-line no-await-in-loop + await establishCleanupReceipt(user, false); + } catch (error) { + errors.push(error); + failedIds.push(String(user._id)); + } + } const receipts = await AccountDeletion.find().sort({ lastSweptAt: 1, _id: 1 }) .limit(ACCOUNT_DELETION_SWEEP_BATCH).select('_id').lean(); for (const receipt of receipts) { // Renew between receipts. If this worker lost its lease, stop starting // work; expiry recovers a crashed worker and deletes remain idempotent. // eslint-disable-next-line no-await-in-loop - const renewed = await SchedulerHeartbeatLease.updateOne( - { _id: SWEEP_LEASE_ID, ownerId }, - { $set: { expiresAt: new Date(Date.now() + SWEEP_LEASE_MS) } }, - ); - if (renewed.matchedCount !== 1) throw new Error('Account deletion sweep lease lost'); + await renewLease(); const userId = String(receipt._id); try { // eslint-disable-next-line no-await-in-loop @@ -153,43 +224,7 @@ export const deleteOwnAccount = async ( 'Transfer administration and ask another admin to make this a member account before deleting it.'); } - // A receipt means billing preflight already succeeded. Otherwise claim the - // non-admin row before Stripe. A stale claim recovers a crashed preflight; - // the provider request is bounded below this five-minute lease. - if (!await AccountDeletion.exists({ _id: userId })) { - const claimedAt = new Date(); - const claimed = await User.updateOne({ - _id: userId, isBot: { $ne: true }, role: { $ne: 'admin' }, - $or: [{ accountDeletionRequestedAt: { $exists: false } }, - { accountDeletionRequestedAt: { $lt: new Date(claimedAt.getTime() - 300_000) } }], - }, { $set: { accountDeletionRequestedAt: claimedAt } }); - if (claimed.matchedCount !== 1) { - throw new AccountDeletionError(409, 'ACCOUNT_CHANGED', - 'Your account changed or deletion is already in progress. Try again shortly.'); - } - try { - if (user.billing?.subscriptionId && user.billing.subscriptionStatus !== 'canceled') { - const client = stripe(); - if (!client) throw new Error('Billing is unavailable; account deletion cannot cancel the subscription'); - await client.subscriptions.cancel(user.billing.subscriptionId, {}, { - idempotencyKey: `account-deletion-${userId}-${user.billing.subscriptionId}`, - timeout: 30_000, - maxNetworkRetries: 0, - }); - } - } catch (error) { - // Release only this attempt's claim. Credentials and uploads are intact. - await User.updateOne({ _id: userId, accountDeletionRequestedAt: claimedAt }, - { $unset: { accountDeletionRequestedAt: 1 } }); - throw error; - } - await AccountDeletion.updateOne({ _id: userId }, { $setOnInsert: { createdAt: new Date() } }, { upsert: true }); - } - // Retain the human until cleanup succeeds for authenticated retries. - await User.updateOne({ _id: userId }, { - $set: { deviceTokens: [] }, - $unset: { apiToken: 1, apiTokenCreatedAt: 1, apiTokenScopes: 1 }, - }); + await establishCleanupReceipt(user); await revokeOwnedAccess(userId); await deleteOwnedUploads(userId); diff --git a/docs/api/account-deletion.md b/docs/api/account-deletion.md index 1eb67fb59d..e458d433c3 100644 --- a/docs/api/account-deletion.md +++ b/docs/api/account-deletion.md @@ -69,6 +69,12 @@ deletion has begun or the account is gone. An `AccountDeletion` receipt retains the opaque user id, creation time and last attempt time. A scheduler tick runs every minute under a renewable two-minute cluster lease. +Before receipt cleanup, it recovers at most 100 stale authorized user markers +whose receipt write failed. It repeats billing preflight with the same idempotency +key before creating a receipt; it never cleans uploads before billing succeeds. +Fresh claims are excluded. Failed recovered preflights retain their marker and +rotate by claim time for another attempt. A retained human can finish deletion +through a re-authenticated browser retry after cleanup succeeds. Each tick processes at most 100 receipts, ordered by the oldest attempt. Failed receipts also rotate to the back of the queue. One cleanup failure does not stop other receipts: the sweep reports their failures as an aggregate after the batch, From 58d932dd12ec3d256d52b63fdb3ada46a130eac7 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 08:57:37 -0700 Subject: [PATCH 08/13] test(account): probe quotas on their configured identity The global CF-header probe correctly exposed four intentionally shared buckets: new socket-peer ingress limits and authenticated user quotas for deletion/upload. Capture exact construction policies by getKey (preserved through Express's middleware wrapper) and probe each new policy's identity with CF held constant. Pin the four policy/route pairs, count A,A,B for every instance, and retain the legacy CF separation controls. No limiter is excluded from measurement. The updated seven-test census passes. Record Lily's ruling to preserve agent identities under original ownership and leave them detached after deletion. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../rateLimitIpKeySeparation.test.js | 90 +++++++++++++------ docs/api/account-deletion.md | 7 +- 2 files changed, 67 insertions(+), 30 deletions(-) diff --git a/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js b/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js index 471b8e0839..eb25c00199 100644 --- a/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js +++ b/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js @@ -1,13 +1,15 @@ /** * TASK-126 — behavioural probe: no rate limiter mounted in the app may put two - * callers that differ only in `cf-connecting-ip` into one bucket. + * callers that differ in the configured identity into one bucket. Existing + * CF policies vary only cf-connecting-ip; explicit account-deletion/upload peer + * and authenticated-user policies vary only socket peer or trusted user id. + * The new policies are pinned to their exact function identities and routes. * * Why each walked instance is re-mounted on a stub instead of being exercised * through the loaded app — all four measured, not assumed: - * - Every entry in the loaded router stack is a Layer whose `handle.name` is - * `''` and whose `entry.name` is ``, for middleware generally and - * for limiters specifically. Identity (`resetKey`/`getKey` own properties) - * is the only census instrument; anything name-keyed mis-attributes. + * - Function names do not identify limiters. Express may wrap a limiter + * in a new function while copying resetKey/getKey. The router census uses + * those own properties; policy capture keys on the unchanged getKey method. * - On a route carrying two limiters both write `RateLimit-Remaining` and the * last one that runs wins. Proved on `POST /api/webhooks/groupme/:integrationId`: * the 3000-max IP limiter reported `remaining` 599/598/597 (the 600-max @@ -18,9 +20,10 @@ * `profileWriteUserLimit`) never runs for a header-free request. * - Three instances never answer at all through the app: their handlers need * Mongo and unit mode runs without it. - * On the stub there are no params and no auth header, so every key function - * takes its IP branch (each one falls back to the Cloudflare-aware helper), and - * the 204 terminal answers for all of them — no exemption list, no database. + * Existing policies receive no params, auth header or user id, so their IP + * branch remains the CF probe. The four explicit new policies receive either a + * simulated socket peer or an authenticated user id. Every limiter still runs + * a counted A,A,B sequence; no instance is exempted and no database is needed. * * The A,A,B sequence is deliberate: two requests cannot separate "different * buckets" from "not counting at all", so A2 is the instrument's own control. @@ -31,12 +34,31 @@ * everything handed to it would pass all 84 and establish nothing. */ /* eslint-disable import/no-unresolved, import/extensions */ +// Express wraps middleware functions, but copies getKey unchanged. Capture +// policy by that store method while retaining the real limiter and store. New +// socket-peer/user quotas must be measured on their declared identity dimension. +const mockLimiterKeys = new WeakMap(); +jest.mock('express-rate-limit', () => { + const actual = jest.requireActual('express-rate-limit'); + const wrapped = (options) => { + const instance = actual(options); + mockLimiterKeys.set(instance.getKey, options?.keyGenerator); + return instance; + }; + Object.assign(wrapped, actual); + wrapped.rateLimit = wrapped; + wrapped.default = wrapped; + return wrapped; +}); + const fs = require('fs'); const express = require('express'); const request = require('supertest'); const rateLimit = require('express-rate-limit'); const { ipKeyGenerator } = require('express-rate-limit'); -const { cloudflareIpRateLimitKeyGenerator } = require('../../../middleware/ipRateLimit'); +const { + cloudflareIpRateLimitKeyGenerator, transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, +} = require('../../../middleware/ipRateLimit'); // Captured at MODULE LOAD, before any test can flip it. A baseline re-read inside // a test moves with a leak instead of catching it: measured, with the per-request @@ -110,9 +132,15 @@ const remainingOf = (res) => { return m ? Number(m[1]) : null; }; -const single = (instance) => { +const single = (instance, dimension = 'cf') => { const stub = express(); - stub.all('/', instance, (req, res) => { + stub.all('/', (req, _res, next) => { + if (dimension === 'user') req.userId = req.get('x-probe-identity'); + if (dimension === 'peer') { + Object.defineProperty(req.socket, 'remoteAddress', { value: req.get('x-probe-identity'), configurable: true }); + } + next(); + }, instance, (req, res) => { if (typeof req.rateLimit?.remaining === 'number') { res.set('x-probe-remaining', String(req.rateLimit.remaining)); } @@ -121,12 +149,12 @@ const single = (instance) => { return stub; }; -const hit = async (stub, ip) => { +const hit = async (stub, ip, identity = ip) => { const entry = process.env.NODE_ENV; let outcome; try { process.env.NODE_ENV = 'development'; - const res = await request(stub).get('/').set('cf-connecting-ip', ip) + const res = await request(stub).get('/').set('cf-connecting-ip', ip).set('x-probe-identity', identity) .timeout({ response: 5000, deadline: 8000 }); outcome = { remaining: remainingOf(res), status: res.status }; } catch (err) { @@ -139,13 +167,13 @@ const hit = async (stub, ip) => { return outcome; }; -const observe = async (instance, tag) => { +const observe = async (instance, tag, dimension = 'cf') => { const a = `198.51.100.${tag}`; const b = `203.0.113.${tag}`; - const stub = single(instance); - const first = await hit(stub, a); - const second = await hit(stub, a); - const other = await hit(stub, b); + const stub = single(instance, dimension); + const first = await hit(stub, a, a); + const second = await hit(stub, a, a); + const other = await hit(stub, dimension === 'cf' ? b : a, b); return { r1: first.remaining, r2: second.remaining, r3: other.remaining, status: first.status, error: first.error, }; @@ -179,6 +207,14 @@ describe('TASK-126 ip key separation', () => { expect(distinct.size).toBe(walked.length); }); + it('pins the four new explicit peer/user policies without exempting them from counting', () => { + for (const key of [transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator]) { + expect(walked.filter((w) => mockLimiterKeys.get(w.instance.getKey) === key) + .map((w) => `${w.method.toUpperCase()} ${w.path}`).sort()) + .toEqual(['DELETE /api/users/me', 'POST /api/uploads/']); + } + }); + it('negative control: the instrument catches a limiter keyed the way the swapped sites were', async () => { // The defect's own spelling, not a cousin of it (vera 73704): the 21 sites // keyed `req.ip ? ipKeyGenerator(req.ip) : 'anon'`. A bare `req.ip` collapses @@ -226,14 +262,11 @@ describe('TASK-126 ip key separation', () => { expect(rec.r3).toBe(rec.r1); }); - // What this measures (wren 73671, ruling (b)), not what it might be read as: - // a request with no credential, no param and no body — built from nothing and - // carrying only `cf-connecting-ip` — is bucketed per `cf-connecting-ip` on - // every instance. That is stricter than a per-arm check: a limiter with no IP - // arm at all keys such a request on a CONSTANT, one bucket for every anonymous - // caller, which is the same defect as raw `req.ip` behind cloudflared — and it - // collapses A and B here exactly the same way. - it('every walked instance counts and separates; none collapses', async () => { + // Legacy policies keep the original anonymous CF-header probe. The four new + // policies use the declared socket peer or authenticated-user dimension while + // holding the CF header constant. Pinning their routes prevents reclassifying + // a legacy raw-req.ip regression as an intentional peer policy. + it('every walked instance counts and separates its configured identity; none collapses', async () => { const results = []; // Serial, not `Promise.all` (wren 73692): with 84 chains started in one tick, // only the first reads the suite's NODE_ENV; the other 83 read the @@ -246,7 +279,10 @@ describe('TASK-126 ip key separation', () => { purpose: the NODE_ENV restore of one iteration must complete before the next reads it, which is the property under test. */ for (const [i, w] of walked.entries()) { - results[i] = { path: w.path, method: w.method, ...(await observe(w.instance, i + 1)) }; + const key = mockLimiterKeys.get(w.instance.getKey); + const dimension = key === transportPeerRateLimitKeyGenerator ? 'peer' + : key === authenticatedUserRateLimitKeyGenerator ? 'user' : 'cf'; + results[i] = { path: w.path, method: w.method, dimension, ...(await observe(w.instance, i + 1, dimension)) }; } /* eslint-enable no-await-in-loop, no-restricted-syntax */ expect(process.env.NODE_ENV).toBe(ENTRY_NODE_ENV); diff --git a/docs/api/account-deletion.md b/docs/api/account-deletion.md index e458d433c3..f0dba07b7b 100644 --- a/docs/api/account-deletion.md +++ b/docs/api/account-deletion.md @@ -127,6 +127,7 @@ a coarse transport-peer bucket permits 120 deletion or 6,000 upload requests per fifteen minutes. It uses the socket peer only. Behind a proxy that bucket is shared, so it is intentionally broader than the user quota. -Agent ownership recovery after deletion is awaiting a product ruling. The current -implementation preserves original ownership, pauses owned runtimes and revokes -owned credentials. It does not introduce an ownership transfer mechanism. +Lily ruled on 2026-10-25 to preserve agent identities and leave them detached. +Original ownership remains, owned runtimes pause and owned credentials are revoked. +Self-serve re-attachment requires the original owner, so it is unavailable after +that owner is deleted. No ownership transfer or administrator recovery is introduced. From 0027085a72691e228626ec5f1c7cfed83c2cc8e5 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 10:25:10 -0700 Subject: [PATCH 09/13] fix(account): restrict pending deletion sessions and isolate quotas Refuse marked accounts in the shared session verifier for HTTP, sockets, attachments and hosted connections. Keep only reauthenticated DELETE retry and minimal read-only AuthContext bootstrap available; disconnect existing sockets. Remove the human's Mongo/PG pod memberships while retaining shared pods and opaque creator references. Independent agent memberships remain. Replace shared socket-peer ingress quotas with hashed presented-credential buckets before auth and stable account quotas after it. Anonymous exhaustion cannot consume an authenticated user's bucket. Rotating invalid secrets remain an ingress abuse boundary, not a claimed protection of this coarse quota. Real MongoDB/PostgreSQL service31/31; session gap reproduced before fix; restoring shared-peer key fails the anonymous-exhaustion regression. Typecheck/lint pass. Expanded local337-case run:336 pass, one unexpected404 in an existing profile mock; affected fixture passes6/6 isolated. Current-head CI remains required. Isolated production UI recovers from503+reload with minimal pending identity, other APIs403, and logs out only after204. Docs name TASK-003 admin-demotion race follow-up and TASK-004 receipt-retirement bounds. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../__tests__/service/accountDeletion.test.js | 47 +++++++++++++++++++ .../unit/middleware/ipRateLimit.test.js | 16 ++++--- .../rateLimitIpKeySeparation.test.js | 30 ++++++------ .../services/sessionAccountService.test.js | 5 ++ backend/controllers/authController.ts | 4 ++ backend/middleware/auth.ts | 21 +++++++-- backend/middleware/ipRateLimit.ts | 15 ++++-- backend/routes/uploads.ts | 8 ++-- backend/routes/users.ts | 8 ++-- backend/services/accountDeletionService.ts | 4 ++ backend/services/sessionAccountService.ts | 7 ++- backend/services/toolBrokerService.ts | 2 + docs/api/account-deletion.md | 41 +++++++++++++--- 13 files changed, 161 insertions(+), 47 deletions(-) diff --git a/backend/__tests__/service/accountDeletion.test.js b/backend/__tests__/service/accountDeletion.test.js index 8f219f1161..aae40c1376 100644 --- a/backend/__tests__/service/accountDeletion.test.js +++ b/backend/__tests__/service/accountDeletion.test.js @@ -9,6 +9,8 @@ const mockStripe = { subscriptions: { cancel: jest.fn().mockResolvedValue({ stat jest.mock('../../services/billingService', () => ({ stripe: () => mockStripe })); const User = require('../../models/User'); const File = require('../../models/File'); +const Pod = require('../../models/Pod'); +const authController = require('../../controllers/authController'); const MediaObject = require('../../models/MediaObject'); const DeviceAuthorization = require('../../models/DeviceAuthorization'); const OAuthLoginState = require('../../models/OAuthLoginState'); @@ -23,6 +25,8 @@ const AccountDeletion = require('../../models/AccountDeletion'); const { issueSessionToken } = require('../../services/sessionTokenService'); const { getObjectStore } = require('../../services/objectStore'); const oauthController = require('../../controllers/oauthController'); +const auth = require('../../middleware/auth'); +const { socketAuthMiddleware } = require('../../middleware/socketAuth'); const routes = require('../../routes/users'); const uploads = require('../../routes/uploads'); const adminRoutes = require('../../routes/admin/users'); @@ -51,6 +55,12 @@ describe('Self-serve account deletion', () => { app = express(); app.use(express.json()); app.use('/api/users', routes); app.use('/api/uploads', uploads); app.use('/api/admin/users', adminRoutes); + app.get('/session-probe', auth, (_req, res) => res.sendStatus(204)); + app.post('/session-probe', auth, (_req, res) => res.sendStatus(204)); + const recovery = express.Router(); + recovery.get('/user', auth, authController.getCurrentUser); + recovery.get('/profile', auth, (_req, res) => res.sendStatus(204)); + app.use('/api/auth', recovery); const probeLimit = require('express-rate-limit')({ windowMs: 60000, max: 1000 }); app.get('/runtime-probe', probeLimit, agentRuntimeAuth, (_req, res) => res.sendStatus(204)); }); @@ -63,6 +73,8 @@ describe('Self-serve account deletion', () => { tokenHash: createHash('sha256').update(deviceToken).digest('hex'), label: 'Laptop' }] }); const other = await makeUser(); const agent = await makeUser({ isBot: true }); + const sharedPod = await Pod.create({ name: 'Keep shared pod', createdBy: human._id, + members: [human._id, other._id, agent._id] }); const memory = await AgentMemory.create({ agentName: 'surviving-agent', instanceId: 'default', sections: { long_term: { content: 'Remember this', visibility: 'private', byteSize: 13 } } }); const ownInstallation = await AgentInstallation.create({ agentName: 'surviving-agent', @@ -84,9 +96,15 @@ describe('Self-serve account deletion', () => { ['123456789012345678901234', 'Shared', 'chat', String(other._id)]); await mockPg.query('INSERT INTO messages (id,pod_id,user_id,content) VALUES ($1,$2,$3,$4)', ['1', '123456789012345678901234', String(human._id), 'Shared history']); + await mockPg.query('INSERT INTO pod_members (pod_id,user_id) VALUES ($1,$2)', + ['123456789012345678901234', String(human._id)]); const token = issueSessionToken(human); await deletion(app, token).expect(204); expect(await User.findById(human._id)).toBeNull(); + const retainedPod = await Pod.findById(sharedPod._id); + expect(retainedPod.members.map(String).sort()).toEqual([String(other._id), String(agent._id)].sort()); + expect(String(retainedPod.createdBy)).toBe(String(human._id)); + expect((await mockPg.query('SELECT * FROM pod_members WHERE user_id = $1', [String(human._id)])).rows).toHaveLength(0); expect(await File.countDocuments({ uploadedBy: human._id })).toBe(0); expect(await MediaObject.findOne({ key: 'own.png' })).toBeNull(); expect(await MediaObject.countDocuments()).toBe(2); @@ -126,6 +144,7 @@ describe('Self-serve account deletion', () => { expect(after).not.toBeNull(); expect(after.apiToken).toBe('cm_no-deletion'); expect(after.accountDeletionRequestedAt).toBeUndefined(); + await request(app).get('/session-probe').set('Authorization', `Bearer ${issueSessionToken(user)}`).expect(204); }); it('requires fresh OAuth sign-in, not JWT issue time or refresh', async () => { @@ -174,6 +193,15 @@ describe('Self-serve account deletion', () => { expect(await File.countDocuments()).toBe(0); }); + it('does not let anonymous deletion attempts exhaust a proxy-wide bucket', async () => { + for (let attempt = 0; attempt < 121; attempt += 1) { + const response = await request(app).delete('/api/users/me').send({ confirmation: 'DELETE' }); + expect([401, 429]).toContain(response.status); + } + const user = await makeUser(); + await deletion(app, issueSessionToken(user), {}).expect(400); + }); + it('limits deletion by account despite rotating JWTs and forwarding headers', async () => { const user = await makeUser(); const tokens = [issueSessionToken(user), issueSessionToken(user, undefined, undefined, 1)]; @@ -223,10 +251,29 @@ describe('Self-serve account deletion', () => { const after = await User.findById(user._id).select('+apiToken'); expect(after.apiToken).toBe('cm_keep-on-billing-failure'); expect(after.accountDeletionRequestedAt).toBeUndefined(); + await request(app).get('/session-probe').set('Authorization', `Bearer ${issueSessionToken(user)}`).expect(204); expect(await MediaObject.findOne({ key: 'billing.png' })).not.toBeNull(); expect(await File.countDocuments({ uploadedBy: user._id })).toBe(1); }); + it('blocks ordinary HTTP and socket access after failed cleanup but permits reauthenticated deletion retry', async () => { + const user = await makeUser(); const token = issueSessionToken(user); + await request(app).get('/session-probe').set('Authorization', `Bearer ${token}`).expect(204); + jest.spyOn(mockPg, 'query').mockRejectedValueOnce(new Error('PG unavailable')); + await deletion(app, token).expect(503); + await request(app).get('/session-probe').set('Authorization', `Bearer ${token}`).expect(403); + await request(app).post('/session-probe').set('Authorization', `Bearer ${token}`).expect(403); + await request(app).get('/api/auth/profile').set('Authorization', `Bearer ${token}`).expect(403); + const bootstrap = await request(app).get('/api/auth/user').set('Authorization', `Bearer ${token}`).expect(200); + expect(Object.keys(bootstrap.body).sort()).toEqual(['_id', 'username', 'email', 'role', 'accountDeletionPending'].sort()); + expect(bootstrap.body.accountDeletionPending).toBe(true); + const next = jest.fn(); + await socketAuthMiddleware({ handshake: { auth: { token } } }, next); + expect(next).toHaveBeenCalledWith(expect.any(Error)); + await deletion(app, token, { confirmation: 'DELETE', password: 'wrong' }).expect(401); + await deletion(app, token).expect(204); + }); + it('keeps a PG failure retryable and only removes the human after anonymization succeeds', async () => { const user = await makeUser(); const token = issueSessionToken(user); jest.spyOn(mockPg, 'query').mockRejectedValueOnce(new Error('PG unavailable')); diff --git a/backend/__tests__/unit/middleware/ipRateLimit.test.js b/backend/__tests__/unit/middleware/ipRateLimit.test.js index 7058ac72f1..73a9c7e4d7 100644 --- a/backend/__tests__/unit/middleware/ipRateLimit.test.js +++ b/backend/__tests__/unit/middleware/ipRateLimit.test.js @@ -31,17 +31,21 @@ describe('cloudflareIpRateLimitKeyGenerator', () => { }); }); -const { transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator } = require('../../../middleware/ipRateLimit'); +const { authenticatedUserRateLimitKeyGenerator } = require('../../../middleware/ipRateLimit'); describe('account mutation quota keys', () => { - it('uses the transport peer despite conflicting forwarding headers', () => { - expect(transportPeerRateLimitKeyGenerator({ socket: { remoteAddress: '192.0.2.4' }, - ip: '203.0.113.5', headers: { 'cf-connecting-ip': '198.51.100.6' } })) - .toBe(ipKeyGenerator('192.0.2.4')); - }); it('uses the authenticated account rather than bearer or forwarding headers', () => { expect(authenticatedUserRateLimitKeyGenerator({ userId: 'one', headers: { authorization: 'rotated' } })) .toBe('user:one'); expect(authenticatedUserRateLimitKeyGenerator({ userId: 'two' })).toBe('user:two'); }); }); + +const { presentedCredentialRateLimitKeyGenerator } = require('../../../middleware/ipRateLimit'); +it('isolates anonymous traffic from credential buckets without retaining bearer text', () => { + const key = (authorization) => presentedCredentialRateLimitKeyGenerator({ header: (name) => name === 'Authorization' ? authorization : undefined }); + expect(key()).toBe('no-credential'); + expect(key('Bearer first-secret')).not.toBe(key()); + expect(key('Bearer first-secret')).not.toBe(key('Bearer second-secret')); + expect(key('Bearer first-secret')).not.toContain('first-secret'); +}); diff --git a/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js b/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js index eb25c00199..83707eecda 100644 --- a/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js +++ b/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js @@ -1,8 +1,8 @@ /** * TASK-126 — behavioural probe: no rate limiter mounted in the app may put two * callers that differ in the configured identity into one bucket. Existing - * CF policies vary only cf-connecting-ip; explicit account-deletion/upload peer - * and authenticated-user policies vary only socket peer or trusted user id. + * CF policies vary only cf-connecting-ip; explicit account-deletion/upload + * policies vary only the presented credential or trusted user id. * The new policies are pinned to their exact function identities and routes. * * Why each walked instance is re-mounted on a stub instead of being exercised @@ -21,8 +21,8 @@ * - Three instances never answer at all through the app: their handlers need * Mongo and unit mode runs without it. * Existing policies receive no params, auth header or user id, so their IP - * branch remains the CF probe. The four explicit new policies receive either a - * simulated socket peer or an authenticated user id. Every limiter still runs + * branch remains the CF probe. Explicit new policies receive a presented + * credential or authenticated user id. Every limiter still runs * a counted A,A,B sequence; no instance is exempted and no database is needed. * * The A,A,B sequence is deliberate: two requests cannot separate "different @@ -36,7 +36,7 @@ /* eslint-disable import/no-unresolved, import/extensions */ // Express wraps middleware functions, but copies getKey unchanged. Capture // policy by that store method while retaining the real limiter and store. New -// socket-peer/user quotas must be measured on their declared identity dimension. +// Authenticated-user quotas must be measured on their declared identity dimension. const mockLimiterKeys = new WeakMap(); jest.mock('express-rate-limit', () => { const actual = jest.requireActual('express-rate-limit'); @@ -57,7 +57,7 @@ const request = require('supertest'); const rateLimit = require('express-rate-limit'); const { ipKeyGenerator } = require('express-rate-limit'); const { - cloudflareIpRateLimitKeyGenerator, transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, + cloudflareIpRateLimitKeyGenerator, presentedCredentialRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, } = require('../../../middleware/ipRateLimit'); // Captured at MODULE LOAD, before any test can flip it. A baseline re-read inside @@ -136,9 +136,7 @@ const single = (instance, dimension = 'cf') => { const stub = express(); stub.all('/', (req, _res, next) => { if (dimension === 'user') req.userId = req.get('x-probe-identity'); - if (dimension === 'peer') { - Object.defineProperty(req.socket, 'remoteAddress', { value: req.get('x-probe-identity'), configurable: true }); - } + if (dimension === 'credential') req.headers.authorization = `Bearer ${req.get('x-probe-identity')}`; next(); }, instance, (req, res) => { if (typeof req.rateLimit?.remaining === 'number') { @@ -207,8 +205,8 @@ describe('TASK-126 ip key separation', () => { expect(distinct.size).toBe(walked.length); }); - it('pins the four new explicit peer/user policies without exempting them from counting', () => { - for (const key of [transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator]) { + it('pins the explicit credential/user policies without exempting them from counting', () => { + for (const key of [presentedCredentialRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator]) { expect(walked.filter((w) => mockLimiterKeys.get(w.instance.getKey) === key) .map((w) => `${w.method.toUpperCase()} ${w.path}`).sort()) .toEqual(['DELETE /api/users/me', 'POST /api/uploads/']); @@ -262,10 +260,10 @@ describe('TASK-126 ip key separation', () => { expect(rec.r3).toBe(rec.r1); }); - // Legacy policies keep the original anonymous CF-header probe. The four new - // policies use the declared socket peer or authenticated-user dimension while + // Legacy policies keep the original anonymous CF-header probe. New policies + // use their presented-credential or authenticated-user dimension while // holding the CF header constant. Pinning their routes prevents reclassifying - // a legacy raw-req.ip regression as an intentional peer policy. + // a legacy raw-req.ip regression as an intentional per-user policy. it('every walked instance counts and separates its configured identity; none collapses', async () => { const results = []; // Serial, not `Promise.all` (wren 73692): with 84 chains started in one tick, @@ -280,8 +278,8 @@ describe('TASK-126 ip key separation', () => { next reads it, which is the property under test. */ for (const [i, w] of walked.entries()) { const key = mockLimiterKeys.get(w.instance.getKey); - const dimension = key === transportPeerRateLimitKeyGenerator ? 'peer' - : key === authenticatedUserRateLimitKeyGenerator ? 'user' : 'cf'; + const dimension = key === authenticatedUserRateLimitKeyGenerator ? 'user' + : key === presentedCredentialRateLimitKeyGenerator ? 'credential' : 'cf'; results[i] = { path: w.path, method: w.method, dimension, ...(await observe(w.instance, i + 1, dimension)) }; } /* eslint-enable no-await-in-loop, no-restricted-syntax */ diff --git a/backend/__tests__/unit/services/sessionAccountService.test.js b/backend/__tests__/unit/services/sessionAccountService.test.js index 3f825bc2ed..ea05fd3f98 100644 --- a/backend/__tests__/unit/services/sessionAccountService.test.js +++ b/backend/__tests__/unit/services/sessionAccountService.test.js @@ -60,6 +60,11 @@ describe('sessionAccountService', () => { expect(sessionRefusal(await loadSessionAccount(String(user._id)))).toBe('banned'); }); + it('projects and refuses a pending deletion marker', async () => { + const user = await row({ accountDeletionRequestedAt: new Date() }); + expect(sessionRefusal(await loadSessionAccount(String(user._id)))).toBe('deleting'); + }); + it('refuses a row that no longer exists', async () => { expect(sessionRefusal(await loadSessionAccount('6a8f6dc7a1dccf2e02f31015'))).toBe('missing'); }); diff --git a/backend/controllers/authController.ts b/backend/controllers/authController.ts index 8a38ed72e7..3db0fc438f 100644 --- a/backend/controllers/authController.ts +++ b/backend/controllers/authController.ts @@ -938,6 +938,10 @@ exports.getCurrentUser = async (req: any, res: any) => { if (!user) { return res.status(404).json({ error: 'User not found' }); } + if (user.accountDeletionRequestedAt) { + return res.json({ _id: user._id, username: user.username, email: user.email, + role: user.role, accountDeletionPending: true }); + } res.json(user); } catch (err: any) { res.status(500).json({ error: err.message }); diff --git a/backend/middleware/auth.ts b/backend/middleware/auth.ts index 4d938c9ea2..12e9657e74 100644 --- a/backend/middleware/auth.ts +++ b/backend/middleware/auth.ts @@ -51,7 +51,7 @@ export default async function auth(req: Request, res: Response, next: NextFuncti // bearers share its public cm_ prefix but are stored only as a hash so a // database read cannot impersonate a CLI session. let user: any = await User.findOne({ apiToken: token }).select( - '_id username email role apiTokenScopes apiTokenCreatedAt banned', + '_id username email role apiTokenScopes apiTokenCreatedAt banned accountDeletionRequestedAt', ); let isDeviceToken = false; @@ -66,7 +66,7 @@ export default async function auth(req: Request, res: Response, next: NextFuncti revokedAt: { $in: [null] }, }, }, - }).select('_id username email role banned'); + }).select('_id username email role banned accountDeletionRequestedAt'); isDeviceToken = Boolean(user); } @@ -75,6 +75,10 @@ export default async function auth(req: Request, res: Response, next: NextFuncti return res.status(403).json({ msg: 'This account has been suspended.' }); } + if (user.accountDeletionRequestedAt) { + return res.status(403).json({ code: 'ACCOUNT_DELETING', msg: 'Account deletion is pending.' }); + } + req.userId = user._id.toString(); req.user = { id: user._id.toString(), @@ -129,6 +133,17 @@ export default async function auth(req: Request, res: Response, next: NextFuncti } if (refusal === 'revoked') return res.status(401).json({ msg: 'Session has been revoked' }); + if (refusal === 'deleting') { + // These canonical routes are the complete recovery surface. DELETE still + // requires password/fresh OAuth in its service. The read-only bootstrap + // lets AuthContext reopen Settings after provider sign-in or reload. + const retry = req.method === 'DELETE' && req.baseUrl === '/api/users' && req.path === '/me'; + const bootstrap = req.method === 'GET' && req.baseUrl === '/api/auth' && req.path === '/user'; + if (!retry && !bootstrap) { + return res.status(403).json({ code: 'ACCOUNT_DELETING', msg: 'Account deletion is pending.' }); + } + } + req.userId = id; req.user = { id, @@ -140,7 +155,7 @@ export default async function auth(req: Request, res: Response, next: NextFuncti req.sessionTokenVersion = decoded.tokenVersion === undefined ? 0 : decoded.tokenVersion as number; req.sessionStartedAt = typeof decoded.sessionStartedAt === 'number' ? decoded.sessionStartedAt : decoded.iat as number | undefined; - touchLastActive(id); + if (!account!.accountDeletionRequestedAt) touchLastActive(id); next(); } catch (err: unknown) { console.error('Token validation error:', (err as Error).message); diff --git a/backend/middleware/ipRateLimit.ts b/backend/middleware/ipRateLimit.ts index 8c275975f3..05b268e961 100644 --- a/backend/middleware/ipRateLimit.ts +++ b/backend/middleware/ipRateLimit.ts @@ -1,4 +1,5 @@ import type { Request } from 'express'; +import { createHash } from 'crypto'; import { ipKeyGenerator } from 'express-rate-limit'; const firstHeaderValue = (value: string | string[] | undefined): string | undefined => { @@ -54,10 +55,14 @@ export const cloudflareIpRateLimitKeyGenerator = (req: Request): string => { return req.ip ? ipKeyGenerator(req.ip) : 'anon'; }; -// Coarse ingress buckets use only the connection peer; user quotas provide fairness behind proxies. -export const transportPeerRateLimitKeyGenerator = (req: Request): string => ( - req.socket?.remoteAddress ? ipKeyGenerator(req.socket.remoteAddress) : 'unknown-peer' -); +// Pre-auth traffic bound per presented secret, not per proxy or forwarding +// header. Anonymous requests share only the anonymous bucket, never a signed-in +// user's. Rotating invalid secrets can evade this coarse bound; the post-auth +// user quota is the durable account limit. Never retain plaintext credentials. +export const presentedCredentialRateLimitKeyGenerator = (req: Request): string => { + const token = req.header('Authorization')?.replace('Bearer ', '') || req.header('x-auth-token'); + return token ? `credential:${createHash('sha256').update(token).digest('hex')}` : 'no-credential'; +}; export const authenticatedUserRateLimitKeyGenerator = (req: Request & { userId?: unknown }): string => ( req.userId ? `user:${String(req.userId)}` : 'unresolved-user' @@ -66,5 +71,5 @@ export const authenticatedUserRateLimitKeyGenerator = (req: Request & { userId?: // CJS compat for the require()-style imports used elsewhere in backend/. // eslint-disable-next-line @typescript-eslint/no-require-imports module.exports = { - cloudflareIpRateLimitKeyGenerator, transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, + cloudflareIpRateLimitKeyGenerator, presentedCredentialRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, }; diff --git a/backend/routes/uploads.ts b/backend/routes/uploads.ts index 6452afa07a..df20253ae7 100644 --- a/backend/routes/uploads.ts +++ b/backend/routes/uploads.ts @@ -30,7 +30,7 @@ import { } from '../services/attachmentAccess'; import { logAttachmentTokenMint } from '../services/auditService'; import { - cloudflareIpRateLimitKeyGenerator, transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, + cloudflareIpRateLimitKeyGenerator, presentedCredentialRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, } from '../middleware/ipRateLimit'; import { normalizeAvatarUrl } from '../services/avatarService'; // eslint-disable-next-line global-require @@ -275,12 +275,12 @@ const handleUpload = async ( } }; -const uploadIngressLimit = rateLimit({ +const uploadCredentialRateLimit = rateLimit({ windowMs: 15 * 60_000, max: 6000, standardHeaders: true, legacyHeaders: false, - keyGenerator: transportPeerRateLimitKeyGenerator, + keyGenerator: presentedCredentialRateLimitKeyGenerator, }); const uploadRateLimit = rateLimit({ @@ -291,7 +291,7 @@ const uploadRateLimit = rateLimit({ keyGenerator: authenticatedUserRateLimitKeyGenerator, }); -router.post('/', uploadIngressLimit, auth, uploadRateLimit, uploadSingle('image'), async (req: AuthReq, res: Res) => { +router.post('/', uploadCredentialRateLimit, auth, uploadRateLimit, uploadSingle('image'), async (req: AuthReq, res: Res) => { if (!req.userId) { res.status(401).json({ msg: 'auth required' }); return; diff --git a/backend/routes/users.ts b/backend/routes/users.ts index 619cc5f177..91d9c00dca 100644 --- a/backend/routes/users.ts +++ b/backend/routes/users.ts @@ -4,7 +4,7 @@ import rateLimit from 'express-rate-limit'; import { createHash } from 'crypto'; import { AccountDeletionError, deleteOwnAccount } from '../services/accountDeletionService'; import { - cloudflareIpRateLimitKeyGenerator, transportPeerRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, + cloudflareIpRateLimitKeyGenerator, presentedCredentialRateLimitKeyGenerator, authenticatedUserRateLimitKeyGenerator, } from '../middleware/ipRateLimit'; // eslint-disable-next-line global-require const express = require('express'); @@ -91,12 +91,12 @@ const profileWriteUserLimit = rateLimit({ }), }); -const accountDeletionIngressLimit = rateLimit({ +const accountDeletionCredentialRateLimit = rateLimit({ windowMs: 15 * 60_000, max: 120, standardHeaders: true, legacyHeaders: false, - keyGenerator: transportPeerRateLimitKeyGenerator, + keyGenerator: presentedCredentialRateLimitKeyGenerator, }); const accountDeletionRateLimit = rateLimit({ @@ -107,7 +107,7 @@ const accountDeletionRateLimit = rateLimit({ keyGenerator: authenticatedUserRateLimitKeyGenerator, }); -router.delete('/me', accountDeletionIngressLimit, auth, accountDeletionRateLimit, async (req: any, res: any) => { +router.delete('/me', accountDeletionCredentialRateLimit, auth, accountDeletionRateLimit, async (req: any, res: any) => { if (req.authType !== 'jwt') { return res.status(403).json({ code: 'BROWSER_SESSION_REQUIRED', error: 'Sign in through the browser to delete your account.', diff --git a/backend/services/accountDeletionService.ts b/backend/services/accountDeletionService.ts index 6e307c3a26..d2ff0de28b 100644 --- a/backend/services/accountDeletionService.ts +++ b/backend/services/accountDeletionService.ts @@ -4,6 +4,7 @@ import SchedulerHeartbeatLease from '../models/SchedulerHeartbeatLease'; import User, { IUser } from '../models/User'; import AccountDeletion from '../models/AccountDeletion'; import File from '../models/File'; +import Pod from '../models/Pod'; import DeviceAuthorization from '../models/DeviceAuthorization'; import OAuthLoginState from '../models/OAuthLoginState'; import AgentCredential from '../models/AgentCredential'; @@ -23,6 +24,7 @@ export class AccountDeletionError extends Error { } const revokeOwnedAccess = async (userId: string): Promise => { + disconnectUserSessionSockets(userId); await User.updateOne({ _id: userId }, { $set: { deviceTokens: [] }, $unset: { apiToken: 1, apiTokenCreatedAt: 1, apiTokenScopes: 1 }, @@ -31,6 +33,7 @@ const revokeOwnedAccess = async (userId: string): Promise => { const credentials = await AgentCredential.find({ ownerUserId: userId }).select('_id').lean(); await AgentCredential.revokeCascade(credentials.map((credential) => credential._id)); await Machine.deleteMany({ ownerUserId: userId }); + await Pod.updateMany({ members: userId }, { $pull: { members: userId } }); await AgentInstallation.updateMany( { installedBy: userId, status: 'active' }, { $set: { status: 'paused' } }, @@ -44,6 +47,7 @@ const anonymizeChatAuthor = async (userId: string): Promise => { // Shared messages remain. Their PG author join keeps an anonymous tombstone; // it must not preserve the deleted person's display name or avatar. if (pool) { + await pool.query('DELETE FROM pod_members WHERE user_id = $1', [userId]); await pool.query( 'UPDATE users SET username = $2, profile_picture = NULL, updated_at = $3 WHERE _id = $1', [userId, 'Deleted user', new Date()], diff --git a/backend/services/sessionAccountService.ts b/backend/services/sessionAccountService.ts index 1465a2e508..7fada24473 100644 --- a/backend/services/sessionAccountService.ts +++ b/backend/services/sessionAccountService.ts @@ -24,7 +24,7 @@ import User from '../models/User'; * nothing, which is why the fields are selected here rather than at each call * site. */ -export type SessionAccountRefusal = 'missing' | 'banned' | 'bot'; +export type SessionAccountRefusal = 'missing' | 'banned' | 'bot' | 'deleting'; export type SessionTokenRefusal = SessionAccountRefusal | 'revoked'; export interface SessionAccount { @@ -35,12 +35,14 @@ export interface SessionAccount { email?: string; role?: string; tokenVersion?: number; + accountDeletionRequestedAt?: Date; } export const loadSessionAccount = async (userId: string): Promise => { // Express auth also needs identity fields for the same req.user shape as // API-token auth. Keep one live-row read for all session verifiers. - const account = await User.findById(userId).select('banned isBot username email role tokenVersion').lean(); + const account = await User.findById(userId) + .select('banned isBot username email role tokenVersion accountDeletionRequestedAt').lean(); return (account as SessionAccount | null) || null; }; @@ -63,5 +65,6 @@ export function sessionRefusal( if (claims && (claims.tokenVersion === undefined ? 0 : claims.tokenVersion) !== (account.tokenVersion ?? 0)) { return 'revoked'; } + if (account.accountDeletionRequestedAt) return 'deleting'; return null; } diff --git a/backend/services/toolBrokerService.ts b/backend/services/toolBrokerService.ts index c7f6a70a37..5312e9bee3 100644 --- a/backend/services/toolBrokerService.ts +++ b/backend/services/toolBrokerService.ts @@ -677,12 +677,14 @@ const HOSTED_OWNER_REFUSALS: Record = { banned: 'connection_owner_banned', missing: 'connection_owner_missing', bot: 'connection_owner_bot', + deleting: 'connection_owner_deleting', }; const HOSTED_OWNER_MESSAGES: Record = { banned: 'the person who connected this connection is suspended', missing: 'the person who connected this connection no longer exists', bot: 'this connection is owned by a bot account, which cannot be its granter', + deleting: 'the person who connected this connection is deleting their account', }; const assertHostedOwnerUsable = async (ownerUserId?: string): Promise => { diff --git a/docs/api/account-deletion.md b/docs/api/account-deletion.md index f0dba07b7b..40041ebf00 100644 --- a/docs/api/account-deletion.md +++ b/docs/api/account-deletion.md @@ -42,10 +42,14 @@ marked account, so a concurrent promotion and deletion cannot cross that boundar independent agent `User` identities, memory and pod memberships survive (ADR-001). Installations made by other humans remain active. Legacy agent tokens that carry no human ownership are not reclassified as the human's token. +- The deleted human is removed from `Pod.members` and its PG `pod_members` mirror. Pods they created remain, + keeping only the opaque historical `createdBy` reference; no ownership transfer + is performed. Other humans' and agents' memberships remain. - Shared messages and posts remain. PostgreSQL's author row becomes a `Deleted user` tombstone with no avatar so historical chat joins retain an author without preserving its display name. Account deletion does not scrub personal details - a person wrote into shared message text. + a person wrote into shared message text. Attachments they uploaded are removed + even when shared in someone else's message; retained text may have a missing attachment. - Any linked Stripe subscription is canceled before the account's billing linkage is removed. Cancellation failure refuses deletion rather than leaving an unreachable recurring charge. Stripe is an external billing boundary; @@ -99,8 +103,14 @@ Bytes are removed before their `File` metadata. A failure in the deletion request's own storage or PG cleanup returns `503` and retains the human row so a re-authenticated retry can finish. Already removed objects are safe to delete again. Owned API/device credentials are -revoked during cleanup; the browser session stays available for retry until the -human row is removed. A retry after the OAuth freshness window requires another +revoked during cleanup; the browser JWT is refused for ordinary HTTP, socket handshakes/packets, +attachment access and hosted connector use while the deletion marker remains. +Existing browser sockets are disconnected during access cleanup. Two canonical +HTTP routes remain: re-authenticated `DELETE /api/users/me`, and read-only +`GET /api/auth/user`, which returns only id, username, email, role and +`accountDeletionPending: true` so AuthContext can reopen Settings after reload or +fresh OAuth. This bootstrap grants no pod/profile/refresh access. Password or +fresh OAuth proof is still required on every deletion retry. A retry after the OAuth freshness window requires another provider sign-in. Subscription cancellation uses an account-and-subscription-specific idempotency key. This does not replace provider retention policies or remove billing audit records and backups. @@ -122,12 +132,29 @@ exchange code rather than constructing the accepted JWT claim in the fixture. Deletion permits ten attempts per user per fifteen minutes; human uploads permit sixty. These quotas run after authentication and use the durable user id, so -rotating sessions or forged forwarding headers cannot reset them. Before auth, -a coarse transport-peer bucket permits 120 deletion or 6,000 upload requests per -fifteen minutes. It uses the socket peer only. Behind a proxy that bucket is -shared, so it is intentionally broader than the user quota. +rotating sessions or forged forwarding headers cannot reset them. Before auth, a coarse quota keys on a SHA-256 digest of the presented bearer +(120 deletion / 6,000 upload attempts per fifteen minutes), never its plaintext. +Requests without credentials share a separate anonymous bucket. Exhausting that +bucket cannot block an authenticated account. Rotating invalid credentials can +evade this coarse bound; it does not replace ingress abuse protection. The +post-auth quota remains keyed by stable user id across valid session rotation. +No quota keys on the shared proxy socket or untrusted forwarding headers. Lily ruled on 2026-10-25 to preserve agent identities and leave them detached. Original ownership remains, owned runtimes pause and owned credentials are revoked. Self-serve re-attachment requires the original owner, so it is unavailable after that owner is deleted. No ownership transfer or administrator recovery is introduced. + +## Remaining bounds + +Receipts deliberately retain the opaque id after cleanup because not every late +producer is fenced. With N retained receipts, a healthy 100-per-minute sweep can +take roughly ceil(N/100) ticks per full revisit; work grows with lifetime +deletions. Retiring receipts requires a proven producer horizon, not an arbitrary +TTL. TASK-004 tracks that capacity/retirement work. + +The existing administrator-demotion handler still counts then updates. Concurrent +administrators can demote one another and leave zero administrators. This PR +only makes role promotion versus account-deletion eligibility atomic on the +target document; it does not fix the independent administrator-demotion race. +TASK-003 tracks that defect separately. From 224d08d7bb4292bf98c0ad1aafe0cd9fc36c5326 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 10:28:45 -0700 Subject: [PATCH 10/13] test(account): rate limit session recovery probes Mount the existing test probe limiter before authentication on the four new service-fixture routes. CodeQL flagged these local probes as unbounded DB/auth handlers. Production credential/user quotas are unchanged. Service31/31 pass. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- backend/__tests__/service/accountDeletion.test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/backend/__tests__/service/accountDeletion.test.js b/backend/__tests__/service/accountDeletion.test.js index aae40c1376..c7c3bc2f8f 100644 --- a/backend/__tests__/service/accountDeletion.test.js +++ b/backend/__tests__/service/accountDeletion.test.js @@ -55,13 +55,13 @@ describe('Self-serve account deletion', () => { app = express(); app.use(express.json()); app.use('/api/users', routes); app.use('/api/uploads', uploads); app.use('/api/admin/users', adminRoutes); - app.get('/session-probe', auth, (_req, res) => res.sendStatus(204)); - app.post('/session-probe', auth, (_req, res) => res.sendStatus(204)); + const probeLimit = require('express-rate-limit')({ windowMs: 60000, max: 1000 }); + app.get('/session-probe', probeLimit, auth, (_req, res) => res.sendStatus(204)); + app.post('/session-probe', probeLimit, auth, (_req, res) => res.sendStatus(204)); const recovery = express.Router(); - recovery.get('/user', auth, authController.getCurrentUser); - recovery.get('/profile', auth, (_req, res) => res.sendStatus(204)); + recovery.get('/user', probeLimit, auth, authController.getCurrentUser); + recovery.get('/profile', probeLimit, auth, (_req, res) => res.sendStatus(204)); app.use('/api/auth', recovery); - const probeLimit = require('express-rate-limit')({ windowMs: 60000, max: 1000 }); app.get('/runtime-probe', probeLimit, agentRuntimeAuth, (_req, res) => res.sendStatus(204)); }); afterEach(async () => { await clearMongoDb(); await clearPgDb(); await mockPg.query('DELETE FROM users'); }); From fe00b8d9dc2fce18f589f540a215aca55bb191dd Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 10:41:38 -0700 Subject: [PATCH 11/13] Preserve historical DM membership during account deletion Exclude strict 1:1 DM types from Mongo and PostgreSQL membership removal. Retain opaque historical member ids while deleting the human account, anonymizing its PG author and pausing owned runtime installations. Add service regressions for agent-room and agent-dm membership in both stores. The previous cleanup fails both cases. Real Mongo7/PostgreSQL16: 33/33. Default service: 33/33 before pause assertion; final targeted repeat 2/2. One intervening targeted run hit the known intermittent route404. Source typecheck and targeted lint pass. Reviewer lifecycle clarification remains pending: Pod has no archive field. This local commit does not claim to satisfy the new HOLD until re-reviewed. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../__tests__/service/accountDeletion.test.js | 26 +++++++++++++++++++ backend/services/accountDeletionService.ts | 14 ++++++++-- docs/api/account-deletion.md | 11 +++++--- 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/backend/__tests__/service/accountDeletion.test.js b/backend/__tests__/service/accountDeletion.test.js index c7c3bc2f8f..c8a10b9895 100644 --- a/backend/__tests__/service/accountDeletion.test.js +++ b/backend/__tests__/service/accountDeletion.test.js @@ -134,6 +134,32 @@ describe('Self-serve account deletion', () => { await request(app).get('/api/uploads/other.png').expect(200); }); + it.each(['agent-room', 'agent-dm'])('keeps the two historical members of a deleted human’s %s', async (type) => { + const human = await makeUser(); + const agent = await makeUser({ isBot: true }); + const pod = await Pod.create({ name: 'Historical DM', type, createdBy: agent._id, + members: [human._id, agent._id], joinPolicy: 'invite-only' }); + await mockPg.query('INSERT INTO pods (id,name,type,created_by) VALUES ($1,$2,$3,$4)', + [String(pod._id), 'Historical DM', type, String(agent._id)]); + for (const member of [human, agent]) { + await mockPg.query('INSERT INTO pod_members (pod_id,user_id) VALUES ($1,$2)', + [String(pod._id), String(member._id)]); + } + const installation = await AgentInstallation.create({ agentName: 'historical-dm-agent', + podId: pod._id, version: '1.0.0', installedBy: human._id }); + await deletion(app, issueSessionToken(human)).expect(204); + expect((await AgentInstallation.findById(installation._id)).status).toBe('paused'); + const expected = [String(human._id), String(agent._id)].sort(); + const mongoMembers = (await Pod.findById(pod._id)).members.map(String).sort(); + const rows = (await mockPg.query('SELECT user_id FROM pod_members WHERE pod_id = $1', [String(pod._id)])).rows; + expect({ mongoMembers, pgMembers: rows.map((row) => row.user_id).sort() }) + .toEqual({ mongoMembers: expected, pgMembers: expected }); + await sweepAccountDeletions(); + expect((await Pod.findById(pod._id)).members.map(String).sort()).toEqual(expected); + expect(await User.findById(human._id)).toBeNull(); + expect(await User.findById(agent._id)).not.toBeNull(); + }); + it('requires explicit confirmation, the right password and a browser session', async () => { const user = await makeUser({ apiToken: 'cm_no-deletion' }); const token = issueSessionToken(user); await deletion(app, token, { password }).expect(400); diff --git a/backend/services/accountDeletionService.ts b/backend/services/accountDeletionService.ts index d2ff0de28b..502cfb3dc3 100644 --- a/backend/services/accountDeletionService.ts +++ b/backend/services/accountDeletionService.ts @@ -13,6 +13,7 @@ import { AgentInstallation } from '../models/AgentRegistry'; import { getObjectStore } from './objectStore'; import { disconnectUserSessionSockets } from './sessionSocketService'; import { stripe } from './billingService'; +import { DM_POD_TYPES_GUARD } from './agentIdentityService'; // eslint-disable-next-line @typescript-eslint/no-require-imports const { pool } = require('../config/db-pg'); @@ -33,7 +34,10 @@ const revokeOwnedAccess = async (userId: string): Promise => { const credentials = await AgentCredential.find({ ownerUserId: userId }).select('_id').lean(); await AgentCredential.revokeCascade(credentials.map((credential) => credential._id)); await Machine.deleteMany({ ownerUserId: userId }); - await Pod.updateMany({ members: userId }, { $pull: { members: userId } }); + // DM membership is historical: removing one side would break strict 1:1. + // The deleted account cannot authenticate; its owned runtimes pause below. + await Pod.updateMany({ members: userId, type: { $nin: [...DM_POD_TYPES_GUARD] } }, + { $pull: { members: userId } }); await AgentInstallation.updateMany( { installedBy: userId, status: 'active' }, { $set: { status: 'paused' } }, @@ -47,7 +51,13 @@ const anonymizeChatAuthor = async (userId: string): Promise => { // Shared messages remain. Their PG author join keeps an anonymous tombstone; // it must not preserve the deleted person's display name or avatar. if (pool) { - await pool.query('DELETE FROM pod_members WHERE user_id = $1', [userId]); + const dmTypes = [...DM_POD_TYPES_GUARD]; + const dmTypeParams = dmTypes.map((_type, index) => `$${index + 2}`).join(', '); + await pool.query( + `DELETE FROM pod_members WHERE user_id = $1 + AND pod_id NOT IN (SELECT id FROM pods WHERE type IN (${dmTypeParams}))`, + [userId, ...dmTypes], + ); await pool.query( 'UPDATE users SET username = $2, profile_picture = NULL, updated_at = $3 WHERE _id = $1', [userId, 'Deleted user', new Date()], diff --git a/docs/api/account-deletion.md b/docs/api/account-deletion.md index 40041ebf00..235727f803 100644 --- a/docs/api/account-deletion.md +++ b/docs/api/account-deletion.md @@ -42,9 +42,14 @@ marked account, so a concurrent promotion and deletion cannot cross that boundar independent agent `User` identities, memory and pod memberships survive (ADR-001). Installations made by other humans remain active. Legacy agent tokens that carry no human ownership are not reclassified as the human's token. -- The deleted human is removed from `Pod.members` and its PG `pod_members` mirror. Pods they created remain, - keeping only the opaque historical `createdBy` reference; no ownership transfer - is performed. Other humans' and agents' memberships remain. +- The deleted human is removed from ordinary pods' `Pod.members` and PG + `pod_members` mirror. Strict 1:1 `agent-room` and `agent-dm` pods retain both + historical member IDs in both stores; removing one would corrupt their shape. + The deleted account cannot authenticate, its PG author is anonymized, and its + owned runtime installations are paused. This preserves conversation history, + not an active human account. Pods have no separate archive lifecycle field. + Pods they created retain only the opaque historical `createdBy` reference; no + ownership transfer occurs. Other humans' and agents' memberships remain. - Shared messages and posts remain. PostgreSQL's author row becomes a `Deleted user` tombstone with no avatar so historical chat joins retain an author without preserving its display name. Account deletion does not scrub personal details From 6103f701336fc7964462edf0c476fb1cdbeb63ba Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 10:51:38 -0700 Subject: [PATCH 12/13] Render deleted DM members without reviving their accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preserve opaque missing-member ids with a Deleted user label across pod reads and runtime context. Exercise post-deletion DM reads, membership checks and observer fan-out while another owner’s installation stays active. Rhea confirmed retained historical DM membership and owned-only pause meet the intended lifecycle, subject to read compatibility. No pod archive primitive is added. Both Mongo and PG keep the historical pair. Validation: 33/33 real Mongo7/PostgreSQL16 service cases; 130/132 focused cases passed before updating two obsolete populate-call assertions; the updated controller and extra route suites pass 90/90. Source typecheck and targeted lint pass. New read assertions fail 2/2 before the fix. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- .../__tests__/service/accountDeletion.test.js | 24 +++++++++++++++++++ .../unit/controllers/podController.test.js | 4 ++-- backend/controllers/podController.ts | 15 ++++++------ backend/routes/agentsRuntime.ts | 7 +++--- backend/routes/podInvites.ts | 3 ++- backend/routes/pods.ts | 3 ++- backend/services/podContextService.ts | 19 +++++++++------ backend/services/podMemberPopulate.ts | 10 ++++++++ docs/api/account-deletion.md | 2 ++ 9 files changed, 66 insertions(+), 21 deletions(-) create mode 100644 backend/services/podMemberPopulate.ts diff --git a/backend/__tests__/service/accountDeletion.test.js b/backend/__tests__/service/accountDeletion.test.js index c8a10b9895..5254bb3e5e 100644 --- a/backend/__tests__/service/accountDeletion.test.js +++ b/backend/__tests__/service/accountDeletion.test.js @@ -145,10 +145,14 @@ describe('Self-serve account deletion', () => { await mockPg.query('INSERT INTO pod_members (pod_id,user_id) VALUES ($1,$2)', [String(pod._id), String(member._id)]); } + const otherOwner = await makeUser(); + const otherInstallation = await AgentInstallation.create({ agentName: 'other-owner-dm-agent', + podId: pod._id, version: '1.0.0', installedBy: otherOwner._id }); const installation = await AgentInstallation.create({ agentName: 'historical-dm-agent', podId: pod._id, version: '1.0.0', installedBy: human._id }); await deletion(app, issueSessionToken(human)).expect(204); expect((await AgentInstallation.findById(installation._id)).status).toBe('paused'); + expect((await AgentInstallation.findById(otherInstallation._id)).status).toBe('active'); const expected = [String(human._id), String(agent._id)].sort(); const mongoMembers = (await Pod.findById(pod._id)).members.map(String).sort(); const rows = (await mockPg.query('SELECT user_id FROM pod_members WHERE pod_id = $1', [String(pod._id)])).rows; @@ -158,6 +162,26 @@ describe('Self-serve account deletion', () => { expect((await Pod.findById(pod._id)).members.map(String).sort()).toEqual(expected); expect(await User.findById(human._id)).toBeNull(); expect(await User.findById(agent._id)).not.toBeNull(); + const DMService = require('../../services/dmService'); + const podController = require('../../controllers/podController'); + const res = { status: jest.fn().mockReturnThis(), json: jest.fn().mockReturnThis() }; + await podController.getPodById({ params: { id: String(pod._id), type }, userId: String(agent._id) }, res); + expect(res.status).not.toHaveBeenCalled(); + const readPod = res.json.mock.calls[0][0]; + const historicalMember = readPod.members.find((member) => String(member._id) === String(human._id)); + expect(historicalMember.username).toBe('Deleted user'); + expect(await DMService.canViewPod(String(agent._id), readPod)).toBe(true); + if (type === 'agent-dm') { + await Pod.create({ name: 'Observer shared pod', createdBy: otherOwner._id, + members: [otherOwner._id, agent._id] }); + expect(await DMService.canViewPod(String(otherOwner._id), readPod)).toBe(true); + expect(await DMService.listPodIdsForListing(String(otherOwner._id))).toContain(String(pod._id)); + } else { + expect(await DMService.canViewPod(String(otherOwner._id), readPod)).toBe(false); + } + const PodContextService = require('../../services/podContextService'); + const context = await PodContextService.getPodContext({ podId: String(pod._id), userId: String(agent._id), skillMode: 'none' }); + expect(context.members).toEqual(expect.arrayContaining([{ name: 'Deleted user', isAgent: false, self: false }])); }); it('requires explicit confirmation, the right password and a browser session', async () => { diff --git a/backend/__tests__/unit/controllers/podController.test.js b/backend/__tests__/unit/controllers/podController.test.js index 6e1e3b6ea0..46494d5b99 100644 --- a/backend/__tests__/unit/controllers/podController.test.js +++ b/backend/__tests__/unit/controllers/podController.test.js @@ -120,7 +120,7 @@ describe('podController', () => { expect(save).toHaveBeenCalled(); expect(savedPod.populate).toHaveBeenCalledWith('createdBy', 'username profilePicture'); - expect(savedPod.populate).toHaveBeenCalledWith('members', 'username profilePicture isBot'); + expect(savedPod.populate).toHaveBeenCalledWith(expect.objectContaining({ path: 'members', select: 'username profilePicture isBot' })); expect(AgentInstallation.install).toHaveBeenCalledWith('commonly-bot', 'p1', expect.objectContaining({ installedBy: 'creator', instanceId: 'default', @@ -290,7 +290,7 @@ describe('podController', () => { expect(pod.members).toEqual(['creator']); expect(pod.save).toHaveBeenCalled(); expect(pod.populate).toHaveBeenCalledWith('createdBy', 'username profilePicture'); - expect(pod.populate).toHaveBeenCalledWith('members', 'username profilePicture isBot'); + expect(pod.populate).toHaveBeenCalledWith(expect.objectContaining({ path: 'members', select: 'username profilePicture isBot' })); expect(res.json).toHaveBeenCalledWith(pod); }); diff --git a/backend/controllers/podController.ts b/backend/controllers/podController.ts index 9b6a27b82c..54e188e2fe 100644 --- a/backend/controllers/podController.ts +++ b/backend/controllers/podController.ts @@ -1,3 +1,4 @@ +import { podMemberPopulate } from '../services/podMemberPopulate'; const mongoose = require('mongoose'); const Pod = require('../models/Pod'); const Message = require('../models/Message'); @@ -248,7 +249,7 @@ exports.getAllPods = async (req: any, res: any) => { let pods = await Pod.find(listedQuery) .populate('createdBy', 'username profilePicture') - .populate('members', 'username profilePicture isBot') + .populate(podMemberPopulate('username profilePicture isBot')) .populate('parentPod', 'name _id') .sort({ updatedAt: -1 }); @@ -313,7 +314,7 @@ exports.getPodsByType = async (req: any, res: any) => { const pods = await Pod.find({ type }) .populate('createdBy', 'username profilePicture') - .populate('members', 'username profilePicture isBot') + .populate(podMemberPopulate('username profilePicture isBot')) .sort({ updatedAt: -1 }); // Personal pod types (agent-admin, agent-room, agent-dm) are always @@ -344,7 +345,7 @@ exports.getPodById = async (req: any, res: any) => { // Get the pod with populated data const pod = await Pod.findById(id) .populate('createdBy', 'username profilePicture') - .populate('members', 'username profilePicture isBot') + .populate(podMemberPopulate('username profilePicture isBot')) .populate('parentPod', 'name _id'); if (!pod) { @@ -449,7 +450,7 @@ exports.createPod = async (req: any, res: any) => { // Populate the user data await pod.populate('createdBy', 'username profilePicture'); - await pod.populate('members', 'username profilePicture isBot'); + await pod.populate(podMemberPopulate('username profilePicture isBot')); // Also create in PostgreSQL if available try { @@ -573,7 +574,7 @@ exports.joinPod = async (req: any, res: any) => { const updatedPod = await Pod.findById(id) .populate('createdBy', 'username profilePicture') - .populate('members', 'username profilePicture isBot'); + .populate(podMemberPopulate('username profilePicture isBot')); return res.json(updatedPod); } catch (err: any) { @@ -626,7 +627,7 @@ exports.leavePod = async (req: any, res: any) => { // Populate the user data await pod.populate('createdBy', 'username profilePicture'); - await pod.populate('members', 'username profilePicture isBot'); + await pod.populate(podMemberPopulate('username profilePicture isBot')); res.json(pod); } catch (err: any) { @@ -694,7 +695,7 @@ exports.removeMember = async (req: any, res: any) => { } await pod.populate('createdBy', 'username profilePicture'); - await pod.populate('members', 'username profilePicture isBot'); + await pod.populate(podMemberPopulate('username profilePicture isBot')); return res.json(pod); } catch (err: any) { diff --git a/backend/routes/agentsRuntime.ts b/backend/routes/agentsRuntime.ts index 446f1b4df4..8643530c7b 100644 --- a/backend/routes/agentsRuntime.ts +++ b/backend/routes/agentsRuntime.ts @@ -1,3 +1,4 @@ +import { podMemberPopulate } from '../services/podMemberPopulate'; export {}; // TASK-099: the refusal classes a driver may name on release. The enum is the @@ -3295,7 +3296,7 @@ router.post('/pods', phase4RateLimit, agentRuntimeAuth, async (req: any, res: an // Global dedup by name: if a pod with this name already exists anywhere, join it and return it const existingPod = await Pod.findOne({ name }) .populate('createdBy', 'username profilePicture') - .populate('members', 'username profilePicture'); + .populate(podMemberPopulate('username profilePicture')); if (existingPod) { // DM pods are strictly 1:1 (ADR-001 §3.10). VALID_POD_TYPES above gates // the type the caller ASKED for, but this dedup branch matches on name @@ -3387,7 +3388,7 @@ router.post('/pods', phase4RateLimit, agentRuntimeAuth, async (req: any, res: an if (!isMember) { existingPod.members.push(agentUser._id); await existingPod.save(); - await existingPod.populate('members', 'username profilePicture'); + await existingPod.populate(podMemberPopulate('username profilePicture')); } // Ensure an AgentInstallation exists so the agent appears in the Agents section if (sourceInstall) { @@ -3439,7 +3440,7 @@ router.post('/pods', phase4RateLimit, agentRuntimeAuth, async (req: any, res: an const pod = await newPod.save(); await pod.populate('createdBy', 'username profilePicture'); - await pod.populate('members', 'username profilePicture'); + await pod.populate(podMemberPopulate('username profilePicture')); if (process.env.PG_HOST && PGPod) { try { diff --git a/backend/routes/podInvites.ts b/backend/routes/podInvites.ts index b87f15557d..224097a05a 100644 --- a/backend/routes/podInvites.ts +++ b/backend/routes/podInvites.ts @@ -1,3 +1,4 @@ +import { podMemberPopulate } from '../services/podMemberPopulate'; // Pod invite tokens — shareable URLs that let a user join a pod without // going through the invite-only joinPolicy gate. Tokens are random // (16-byte hex), DB-backed (PodInvite), and bound to a single pod by their @@ -274,7 +275,7 @@ router.post('/invites/:token/redeem', inviteWriteRateLimit, auth, async (req: an await invite.save(); const updated = await Pod.findById(pod._id) .populate('createdBy', 'username profilePicture') - .populate('members', 'username profilePicture isBot'); + .populate(podMemberPopulate('username profilePicture isBot')); return res.json({ ok: true, alreadyMember, pod: updated }); } catch (err: any) { console.error('Redeem invite failed:', err.message); diff --git a/backend/routes/pods.ts b/backend/routes/pods.ts index ef2c10eeb6..9e0260744a 100644 --- a/backend/routes/pods.ts +++ b/backend/routes/pods.ts @@ -1,3 +1,4 @@ +import { podMemberPopulate } from '../services/podMemberPopulate'; import path from 'path'; import fs from 'fs'; import { createHash } from 'crypto'; @@ -613,7 +614,7 @@ router.get('/:id/children', auth, async (req: AuthReq, res: Res) => { if (!canViewParent) return res.status(403).json({ msg: 'Not authorized to view child pods' }); const children = await Pod.find({ parentPod: parentId }) .populate('createdBy', 'username profilePicture') - .populate('members', 'username profilePicture') + .populate(podMemberPopulate('username profilePicture')) .sort({ name: 1 }); const visibleChildren = []; for (const child of children) { diff --git a/backend/services/podContextService.ts b/backend/services/podContextService.ts index 83e669d6e8..8cc067cda4 100644 --- a/backend/services/podContextService.ts +++ b/backend/services/podContextService.ts @@ -404,13 +404,18 @@ class PodContextService { const memberDocs = await User.find({ _id: { $in: memberIds } }) .select('_id username isBot botMetadata') .lean(); - members = (memberDocs as Array>).map((u) => ({ - name: u.isBot - ? resolveAgentDisplayLabel(u, (u.username as string) || 'agent') - : ((u.username as string) || 'member'), - isAgent: !!u.isBot, - self: toObjectIdString(u._id) === selfId, - })); + const usersById = new Map((memberDocs as Array>) + .map((user) => [toObjectIdString(user._id), user])); + members = memberIds.map((id) => { + const user = usersById.get(id); + return { + name: user?.isBot + ? resolveAgentDisplayLabel(user, (user.username as string) || 'agent') + : (user ? ((user.username as string) || 'member') : 'Deleted user'), + isAgent: !!user?.isBot, + self: id === selfId, + }; + }); } // Files a human uploaded into this pod, so an agent orienting via diff --git a/backend/services/podMemberPopulate.ts b/backend/services/podMemberPopulate.ts new file mode 100644 index 0000000000..3a1295c529 --- /dev/null +++ b/backend/services/podMemberPopulate.ts @@ -0,0 +1,10 @@ +// Historical DM membership survives account deletion. Mongoose normally drops +// missing User refs from populated arrays; keep the opaque id and a safe label +// so every pod read preserves the pair without reviving the deleted account. +export const podMemberPopulate = (select: string) => ({ + path: 'members', + select, + transform: (member: unknown, id: unknown) => member || { + _id: id, username: 'Deleted user', profilePicture: null, isBot: false, + }, +}); diff --git a/docs/api/account-deletion.md b/docs/api/account-deletion.md index 235727f803..d18fb7ca16 100644 --- a/docs/api/account-deletion.md +++ b/docs/api/account-deletion.md @@ -48,6 +48,8 @@ marked account, so a concurrent promotion and deletion cannot cross that boundar The deleted account cannot authenticate, its PG author is anonymized, and its owned runtime installations are paused. This preserves conversation history, not an active human account. Pods have no separate archive lifecycle field. + Pod reads and runtime context render a missing member as `Deleted user`; + membership and agent-DM observer fan-out still use the historical IDs. Pods they created retain only the opaque historical `createdBy` reference; no ownership transfer occurs. Other humans' and agents' memberships remain. - Shared messages and posts remain. PostgreSQL's author row becomes a `Deleted From 83cbb4675079a780b298c729862eb404a8c3d4a4 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 11:03:41 -0700 Subject: [PATCH 13/13] Rate-limit pod lifecycle writes exposed by DM read review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bound create, leave and member-removal routes before authentication with a shared 60-per-minute presented-token budget using the existing pod key policy. Remove the three retired unlimited baseline rows. CodeQL flagged these three real unbounded database routes after the DM read-helper change. New counted regressions fail on the old registration, verify the cap on every route and preserve another token’s budget. Validation: 32/32 route, pre-auth guard and global limiter census cases. Jest printed its completed summary but retained open handles; stopped the completed process. Source typecheck and targeted lint pass. This head movement supersedes Rhea’s code PASS on6103f701; a fresh review and current-head CI are required. No merge press. Written by Sprint Impl, a Commonly agent https://commonly.me/v2/pods/6addd4d47f8fc798a6f66bbb#message-85722 Co-authored-by: sprint-impl Agent-Model: codex/gpt-6.1-sol/medium --- backend/__tests__/unit/routes/pods.test.js | 15 +++++++++++++++ .../routes/routeRateLimitGuard.baseline.json | 3 --- backend/routes/pods.ts | 16 +++++++++++++--- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/backend/__tests__/unit/routes/pods.test.js b/backend/__tests__/unit/routes/pods.test.js index e1d455c982..fac3a84fa8 100644 --- a/backend/__tests__/unit/routes/pods.test.js +++ b/backend/__tests__/unit/routes/pods.test.js @@ -58,6 +58,21 @@ describe('pods routes', () => { expect(limited.body.msg).toMatch(/20 pod joins per 60s/); }); + it.each([ + ['post', '/api/pods', 201, 'createPod'], + ['post', '/api/pods/123/leave', 200, 'leavePod'], + ['delete', '/api/pods/123/members/456', 200, 'removeMember'], + ])('bounds lifecycle writes on %s %s without consuming another token’s budget', async (method, url, status, handler) => { + const token = `Bearer lifecycle-${handler}`; + const callsBefore = controllers[handler].mock.calls.length; + for (let i = 0; i < 60; i += 1) { + await request(app)[method](url).set('Authorization', token).expect(status); + } + await request(app)[method](url).set('Authorization', token).expect(429); + expect(controllers[handler].mock.calls.length - callsBefore).toBe(60); + await request(app)[method](url).set('Authorization', `${token}-other`).expect(status); + }); + it('DELETE /api/pods/123/members/456 calls removeMember', async () => { await request(app).delete('/api/pods/123/members/456').expect(200); expect(controllers.removeMember).toHaveBeenCalled(); diff --git a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json b/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json index 1875ccb069..633010fe60 100644 --- a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json +++ b/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json @@ -100,7 +100,6 @@ "routes/pg-status.ts GET / [unlimited]", "routes/pg-status.ts POST /sync-user [unlimited]", "routes/pods.ts DELETE /:id [unlimited]", - "routes/pods.ts DELETE /:id/members/:memberId [unlimited]", "routes/pods.ts DELETE /announcement/:id [unlimited]", "routes/pods.ts DELETE /external-link/:id [unlimited]", "routes/pods.ts GET /:id/children [unlimited]", @@ -113,8 +112,6 @@ "routes/pods.ts GET /:podId/files [unlimited]", "routes/pods.ts GET /:type/:id [unlimited]", "routes/pods.ts GET /external-link/:linkId/qrcode [unlimited]", - "routes/pods.ts POST / [unlimited]", - "routes/pods.ts POST /:id/leave [unlimited]", "routes/pods.ts POST /announcement [unlimited]", "routes/pods.ts POST /external-link [unlimited]", "routes/registry/admin.ts DELETE /admin/installations/:installationId [unlimited]", diff --git a/backend/routes/pods.ts b/backend/routes/pods.ts index 9e0260744a..2b5a1aec61 100644 --- a/backend/routes/pods.ts +++ b/backend/routes/pods.ts @@ -119,8 +119,18 @@ const podListingRateLimit = rateLimit({ legacyHeaders: false, keyGenerator: podJoinRateLimitKey, }); +// Bound lifecycle writes before authentication. Share a per-presented-token +// budget across create, leave and member removal, matching the existing pod +// ingress key policy; authorization still decides which writes are allowed. +const podLifecycleRateLimit = rateLimit({ + windowMs: 60_000, + limit: 60, + standardHeaders: 'draft-7', + legacyHeaders: false, + keyGenerator: podJoinRateLimitKey, +}); router.get('/', podListingRateLimit, auth, getAllPods); -router.post('/', auth, createPod); +router.post('/', podLifecycleRateLimit, auth, createPod); router.post('/announcement', auth, async (req: AuthReq, res: Res) => { try { @@ -394,8 +404,8 @@ router.post('/:id/join', rateLimit({ keyGenerator: podJoinRateLimitKey, handler: (_req: any, res: any) => res.status(429).json({ msg: 'rate limit exceeded: 20 pod joins per 60s' }), }), auth, joinPod); -router.post('/:id/leave', auth, leavePod); -router.delete('/:id/members/:memberId', auth, removeMember); +router.post('/:id/leave', podLifecycleRateLimit, auth, leavePod); +router.delete('/:id/members/:memberId', podLifecycleRateLimit, auth, removeMember); // Pod admin actions hit Mongo on every call but don't need much volume. // Capping per-IP keeps a stuck client from re-pinging contacts at full