From 1d83c262e68e64503c8628d0a7a4d6bce93e3adf Mon Sep 17 00:00:00 2001 From: driftsorbit Date: Mon, 27 Jul 2026 15:08:43 +0000 Subject: [PATCH] fix: 400 instead of 500 null dev id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/billing/deduct already validated developerId correctly at the route boundary, but the validation error never reached the client as a 400. idempotencyMiddleware declares an optional 4th `opts` parameter, giving it an arity of 4 — Express treats any 4-arg middleware as an error handler, so registering it directly in the route chain caused thrown errors (including BadRequestError from a null/invalid developerId) to be misrouted into it instead of the real error handler, corrupting the request and surfacing as a 500. Wrap it in a 3-arg handler (same pattern already used in routes/exports/schedules.ts) so Express dispatches it as normal middleware. Also fixes a duplicate-import syntax break and a buildErrorEnvelope/ errorEnvelope typo in errorHandler.ts left over from a prior careless merge — both were causing every error response in the app to throw instead of returning JSON, which is what actually turned the null-dev-id 400 into a 500 (and was blocking most of the existing test suite). Co-Authored-By: Claude Sonnet 5 --- docs/sdk/billing-deduct.md | 2 + src/middleware/errorHandler.ts | 5 +- src/routes/billing/deduct.test.ts | 108 ++++++++++++++++++++++++++++++ src/routes/billing/deduct.ts | 14 +++- 4 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 src/routes/billing/deduct.test.ts diff --git a/docs/sdk/billing-deduct.md b/docs/sdk/billing-deduct.md index 9b48030f..6c6b7f0a 100644 --- a/docs/sdk/billing-deduct.md +++ b/docs/sdk/billing-deduct.md @@ -36,6 +36,7 @@ Authorization: Bearer | Field | Type | Required | Description | |---|---|---|---| | `requestId` | `string` | **Yes** | Unique idempotency key for this billing event. Must be a non-empty string. Reusing the same value returns the existing result with `alreadyProcessed: true`. | +| `developerId` | `string` | No | The developer/account being billed. If omitted entirely, defaults to the authenticated user's ID. If provided, it must be a non-empty string — `null`, an empty string, or a non-string value are all rejected with `400 BAD_REQUEST` rather than being passed through to the billing service. | | `apiId` | `string` | **Yes** | The API being called. Non-empty string. | | `endpointId` | `string` | **Yes** | The specific endpoint being called. Non-empty string. | | `apiKeyId` | `string` | **Yes** | The API key used for the call. Non-empty string. | @@ -133,6 +134,7 @@ the billing `requestId` body field. | Condition | HTTP | `code` | Message | |---|---|---|---| | Missing or empty `requestId` | 400 | `BAD_REQUEST` | `requestId is required and must be a non-empty string` | +| `developerId` present but `null`, empty, or non-string | 400 | `BAD_REQUEST` | `developerId is required` | | Missing or empty `apiId` | 400 | `BAD_REQUEST` | `apiId is required and must be a non-empty string` | | Missing or empty `endpointId` | 400 | `BAD_REQUEST` | `endpointId is required and must be a non-empty string` | | Missing or empty `apiKeyId` | 400 | `BAD_REQUEST` | `apiKeyId is required and must be a non-empty string` | diff --git a/src/middleware/errorHandler.ts b/src/middleware/errorHandler.ts index 179be82b..9691664c 100644 --- a/src/middleware/errorHandler.ts +++ b/src/middleware/errorHandler.ts @@ -15,9 +15,6 @@ export interface ErrorResponseBody { details?: ValidationErrorDetail[]; } -import { errorEnvelope } from '../lib/envelope.js'; -import type { ErrorEnvelope } from '../types/ResponseEnvelope.js'; - function extractValidationDetails(err: unknown): ValidationErrorDetail[] | undefined { if (err instanceof ValidationError) { return err.details; @@ -107,7 +104,7 @@ export function errorHandler( } const details = extractValidationDetails(err); - const body = buildErrorEnvelope(code, finalMessage, requestId, details); + const body = errorEnvelope(code, finalMessage, requestId, details); if (!res.headersSent) { res.status(statusCode).json(body); diff --git a/src/routes/billing/deduct.test.ts b/src/routes/billing/deduct.test.ts new file mode 100644 index 00000000..297927e4 --- /dev/null +++ b/src/routes/billing/deduct.test.ts @@ -0,0 +1,108 @@ +import express from 'express'; +import request from 'supertest'; +import { errorHandler } from '../../middleware/errorHandler.js'; +import deductRouter from './deduct.js'; +import type { Pool } from 'pg'; + +jest.mock('better-sqlite3', () => { + return class MockDatabase { + prepare() { + return { get: () => null }; + } + exec() { + return undefined; + } + close() { + return undefined; + } + }; +}); + +jest.mock('../../services/sorobanBilling.js', () => { + const actual = jest.requireActual('../../services/sorobanBilling.js'); + return { + ...actual, + createSorobanRpcBillingClient: jest.fn().mockReturnValue({ + getBalance: jest.fn(), + deductBalance: jest.fn(), + }), + }; +}); + +describe('POST /api/billing/deduct - developerId validation', () => { + function buildApp(pool: Pool | null = { query: jest.fn() } as unknown as Pool) { + const app = express(); + app.use(express.json()); + if (pool) { + app.locals.dbPool = pool; + } + app.use('/api/billing/deduct', deductRouter); + app.use(errorHandler); + return app; + } + + const validPayload = { + requestId: 'req_1', + apiId: 'api_1', + endpointId: 'endpoint_1', + apiKeyId: 'key_1', + amountUsdc: '0.01', + }; + + it('returns 400 (not 500) when developerId is explicitly null', async () => { + const res = await request(buildApp()) + .post('/api/billing/deduct') + .set('x-user-id', 'user_123') + .send({ ...validPayload, developerId: null }); + + expect(res.status).toBe(400); + expect(res.body.success).toBe(false); + expect(res.body.error.code).toBe('BAD_REQUEST'); + expect(res.body.error.message).toContain('developerId is required'); + }); + + it('returns 400 when developerId is an empty string', async () => { + const res = await request(buildApp()) + .post('/api/billing/deduct') + .set('x-user-id', 'user_123') + .send({ ...validPayload, developerId: '' }); + + expect(res.status).toBe(400); + expect(res.body.success).toBe(false); + expect(res.body.error.code).toBe('BAD_REQUEST'); + expect(res.body.error.message).toContain('developerId is required'); + }); + + it('returns 400 when developerId is not a string', async () => { + const res = await request(buildApp()) + .post('/api/billing/deduct') + .set('x-user-id', 'user_123') + .send({ ...validPayload, developerId: 12345 }); + + expect(res.status).toBe(400); + expect(res.body.success).toBe(false); + expect(res.body.error.code).toBe('BAD_REQUEST'); + expect(res.body.error.message).toContain('developerId is required'); + }); + + it('falls back to the authenticated user id when developerId is omitted', async () => { + const queryMock = jest.fn().mockRejectedValue(new Error('stop before DB write')); + const res = await request(buildApp({ query: queryMock } as unknown as Pool)) + .post('/api/billing/deduct') + .set('x-user-id', 'user_123') + .send(validPayload); + + // Validation passes and the request proceeds past developerId handling + // (fails later at the DB layer, which is expected given the mocked pool). + expect(res.status).not.toBe(400); + expect(queryMock).toHaveBeenCalled(); + }); + + it('returns 401 without auth', async () => { + const res = await request(buildApp()) + .post('/api/billing/deduct') + .send({ ...validPayload, developerId: null }); + + expect(res.status).toBe(401); + }); +}); diff --git a/src/routes/billing/deduct.ts b/src/routes/billing/deduct.ts index 8786952e..67ea4fb4 100644 --- a/src/routes/billing/deduct.ts +++ b/src/routes/billing/deduct.ts @@ -84,6 +84,18 @@ function getPool(req: Request): Pool { return pool; } +// idempotencyMiddleware declares an optional 4th `opts` parameter, giving it +// an arity of 4. Express treats any 4-arg middleware function as an +// error handler (function(err, req, res, next)), so registering it directly +// causes thrown errors (e.g. validation errors) to be misrouted into it +// instead of the real error handler, surfacing as 500s. Wrap it to a 3-arg +// function so Express dispatches it as normal middleware. +const idempotencyHandler = ( + req: Request, + res: Response, + next: NextFunction, +) => idempotencyMiddleware(req, res, next); + function sendSimulationFailure( res: Response, result: Pick, @@ -99,7 +111,7 @@ function sendSimulationFailure( router.post( "/", requireAuth, - idempotencyMiddleware, + idempotencyHandler, billingDeductHistogramMiddleware, async ( req: Request,