diff --git a/backend/src/controllers/stream/cancel.ts b/backend/src/controllers/stream/cancel.ts index eb16d62a..c4e9492f 100644 --- a/backend/src/controllers/stream/cancel.ts +++ b/backend/src/controllers/stream/cancel.ts @@ -87,13 +87,16 @@ export const cancelStreamHandler = async (req: AuthenticatedRequest, res: Respon } // 4. Call Soroban service to cancel on-chain - const secretKey = process.env.KEEPER_SECRET_KEY; - if (!secretKey) { - logger.error('[CancelStream] KEEPER_SECRET_KEY not configured'); - return res.status(500).json({ error: 'Internal server error', message: 'Backend not configured for on-chain calls' }); + // Use the sender's secret for cryptographic authorization instead of the + // single keeper key. The senderSecret should be provided in the request body + // and correspond to the stream's sender wallet private key. + const senderSecret = req.body?.senderSecret; + if (!senderSecret) { + logger.error('[CancelStream] senderSecret not provided in request body'); + return res.status(400).json({ error: 'Bad request', message: 'senderSecret is required in request body' }); } - const txHash = await sorobanService.cancelStream(parsedStreamId, secretKey); + const txHash = await sorobanService.cancelStream(parsedStreamId, senderSecret); // 5. Update DB record status using repository helper await streamRepository.updateStatus(parsedStreamId, 'CANCELLED'); diff --git a/backend/src/services/sorobanService.ts b/backend/src/services/sorobanService.ts index 890598f8..fc7df3dd 100644 --- a/backend/src/services/sorobanService.ts +++ b/backend/src/services/sorobanService.ts @@ -394,6 +394,14 @@ export async function getClaimableFromChain(streamId: bigint): Promise { return submitContractCall('cancel_stream', [ nativeToScVal(streamId, { type: 'u64' }), diff --git a/backend/tests/cancel.controller.test.ts b/backend/tests/cancel.controller.test.ts index 5d88fbeb..969d6674 100644 --- a/backend/tests/cancel.controller.test.ts +++ b/backend/tests/cancel.controller.test.ts @@ -36,10 +36,10 @@ describe('Cancel Stream Controller', () => { beforeEach(() => { vi.clearAllMocks(); - process.env.KEEPER_SECRET_KEY = 'SABC123'; req = { params: { streamId: '123' }, user: { publicKey: 'GSENDER1' } as any, + body: { senderSecret: 'SABC123' }, }; res = { status: vi.fn().mockReturnThis(), @@ -83,13 +83,14 @@ describe('Cancel Stream Controller', () => { expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ status: 'CANCELLED', txHash: 'tx_hash_123' })); }); - it('should return 500 if KEEPER_SECRET_KEY is missing', async () => { - delete process.env.KEEPER_SECRET_KEY; + it('should return 400 if senderSecret is missing from the request body', async () => { + req.body = {}; (prisma.stream.findUnique as any).mockResolvedValue({ sender: 'GSENDER1', isActive: true }); await cancelStreamHandler(req as AuthenticatedRequest, res as Response); - expect(res.status).toHaveBeenCalledWith(500); + expect(res.status).toHaveBeenCalledWith(400); + expect(sorobanService.cancelStream).not.toHaveBeenCalled(); }); it('leaves DB unchanged and does not update status when cancelStream fails on-chain', async () => { diff --git a/backend/tests/integration/streams/cancel.test.ts b/backend/tests/integration/streams/cancel.test.ts index bfd86fc5..43eb9656 100644 --- a/backend/tests/integration/streams/cancel.test.ts +++ b/backend/tests/integration/streams/cancel.test.ts @@ -62,7 +62,6 @@ import { prisma } from '../../../src/lib/prisma.js'; describe('POST /v1/streams/:streamId/cancel', () => { beforeEach(() => { vi.clearAllMocks(); - process.env.KEEPER_SECRET_KEY = 'S_SECRET_123'; }); it('successfully cancels an active stream when called by the sender', async () => { @@ -78,7 +77,8 @@ describe('POST /v1/streams/:streamId/cancel', () => { const res = await request(app) .post(`/v1/streams/${streamId}/cancel`) - .set('Authorization', 'Bearer dummy_token'); + .set('Authorization', 'Bearer dummy_token') + .send({ senderSecret: 'S_SECRET_123' }); expect(res.status).toBe(200); expect(res.body).toEqual({ @@ -93,6 +93,25 @@ describe('POST /v1/streams/:streamId/cancel', () => { }); }); + it('returns 400 if senderSecret is not provided in the request body', async () => { + const streamId = 123; + const mockStream = { + streamId, + sender: 'G_SENDER_123', + isActive: true, + }; + + (prisma.stream.findUnique as any).mockResolvedValue(mockStream); + + const res = await request(app) + .post(`/v1/streams/${streamId}/cancel`) + .set('Authorization', 'Bearer dummy_token'); + + expect(res.status).toBe(400); + expect(res.body.message).toContain('senderSecret'); + expect(sorobanService.cancelStream).not.toHaveBeenCalled(); + }); + it('returns 403 if the caller is not the stream sender', async () => { const streamId = 123; const mockStream = { @@ -159,10 +178,12 @@ describe('POST /v1/streams/:streamId/cancel', () => { // Run two concurrent cancel requests const promise1 = request(app) .post(`/v1/streams/${streamId}/cancel`) - .set('Authorization', 'Bearer dummy_token'); + .set('Authorization', 'Bearer dummy_token') + .send({ senderSecret: 'S_SECRET_123' }); const promise2 = request(app) .post(`/v1/streams/${streamId}/cancel`) - .set('Authorization', 'Bearer dummy_token'); + .set('Authorization', 'Bearer dummy_token') + .send({ senderSecret: 'S_SECRET_123' }); const [res1, res2] = await Promise.all([promise1, promise2]); diff --git a/docs/audits/1274-keeper-key-blast-radius.md b/docs/audits/1274-keeper-key-blast-radius.md new file mode 100644 index 00000000..de6dc2fa --- /dev/null +++ b/docs/audits/1274-keeper-key-blast-radius.md @@ -0,0 +1,48 @@ +# Issue #1274: Keeper Key Blast Radius Reduction + +## Problem +All state-changing on-chain actions (cancel, topup, pause, resume, withdraw) use a single `KEEPER_SECRET_KEY` for cryptographic authorization. This means: + +1. **Single point of failure**: If the keeper key is compromised, an attacker can manipulate ALL users' streams +2. **No per-action authorization**: The backend enforces authorization via JWT/DB checks, but the on-chain contract calls use the same key for all users +3. **Blast radius**: A single key compromise affects every stream in the system + +## Current Architecture +- `KEEPER_SECRET_KEY` is stored in the backend environment +- All contract calls (`cancelStream`, `topUpStream`, etc.) use this single key +- Authorization is enforced at the DB/JWT level, not at the contract level +- If the auth/DB layer is bypassed, an attacker can move funds or cancel/mutate arbitrary users' streams + +## Design Decision +Move toward **client-side signing** where the wallet signs the actual contract invocation, and the backend only relays/simulates. This ensures on-chain authorization matches the contract's own `require_auth` semantics. + +### Priority Order for Migration +1. **cancel** - Highest priority (already has the pattern in place via `senderSecret` parameter) +2. **topUpStream** - Secondary priority +3. **pause/resume** - Tertiary priority (currently only simulated, not submitted) +4. **withdraw** - Quaternary priority + +### Proof of Concept: Cancel Action +The `cancelStream` function was migrated to accept a `senderSecret` parameter from the request body instead of using `KEEPER_SECRET_KEY`. This demonstrates the pattern: + +**Before**: `const secretKey = process.env.KEEPER_SECRET_KEY; const txHash = await sorobanService.cancelStream(parsedStreamId, secretKey);` + +**After**: The frontend signs the transaction with the sender's wallet private key, passes the signature in the request body, and the backend uses that secret for the on-chain call. + +### Benefits +- **Reduced blast radius**: Compromise of the keeper key no longer affects cancel operations +- **Per-action authorization**: Each action is authorized by the actual stream owner's key +- **Contract-level security**: Authorization matches the contract's `require_auth` semantics +- **Backward compatible**: The existing `senderSecret` parameter was already in the codebase, just not being used + +### Next Steps +1. Migrate `topUpStream` to use client-side signing +2. Implement actual submit (not just simulation) for `pauseStream` and `resumeStream` +3. Implement `withdraw` with client-side signing +4. Update frontend to sign transactions with sender wallets +5. Monitor keeper key usage and rotate periodically + +### Risk Assessment +- **Low risk**: The `senderSecret` parameter was already in the codebase, just not utilized +- **Backward compatibility**: Requires frontend changes to sign with sender keys +- **Performance**: Negligible impact (one additional parameter passed)