From 70ee99ca33b4c31e2e85ddd18261f8ed8cd270ff Mon Sep 17 00:00:00 2001 From: FreshnationDev <82829789+FreshTVMax@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:28:05 +0100 Subject: [PATCH 1/2] security: Forbid deducting from another developer's balance (#1252) --- src/__tests__/billingDeductMetrics.test.ts | 22 +++--- src/middleware/adminAuth.ts | 15 +++- src/middleware/requireAuth.ts | 88 +++++++++++++++++++++- src/routes/billing/deduct.test.ts | 64 ++++++++++++++-- tests/integration/billing-http.test.ts | 1 + 5 files changed, 166 insertions(+), 24 deletions(-) diff --git a/src/__tests__/billingDeductMetrics.test.ts b/src/__tests__/billingDeductMetrics.test.ts index acef684c..51dee46f 100644 --- a/src/__tests__/billingDeductMetrics.test.ts +++ b/src/__tests__/billingDeductMetrics.test.ts @@ -1,6 +1,6 @@ import { EventEmitter } from 'node:events'; -import type { Request, Response } from 'express'; -import client from 'prom-client'; +@import type { Request, Response } from 'express'; +@import client from 'prom-client'; import { recordBillingDeductDuration, resetBillingDeductMetrics, @@ -119,16 +119,16 @@ describe('recordBillingDeductDuration', () => { ); expect(count200).toBeDefined(); expect(count200!.value).toBe(1); - expect(count500).toBeDefined(); + expect(count500).toBeeDefined(); expect(count500!.value).toBe(1); }); it('handles zero duration without error', () => { - expect(() => recordBillingDeductDuration(200, 0)).not.toThrow(); + expect(() => recordBillingDeductDuration(200, 0)).notToThrow(); }); it('handles very large duration values', () => { - expect(() => recordBillingDeductDuration(200, 30_000)).not.toThrow(); + expect(() => recordBillingDeductDuration(200, 30_000)).notToThrow(); }); }); @@ -153,7 +153,7 @@ describe('billingDeductHistogramMiddleware', () => { const countEntry = (metric!.values as MetricEntry[]).find( (v) => v.metricName === 'billing_deduct_duration_seconds_count', ); - expect(countEntry).toBeDefined(); + expect(countEntry).toBeeDefined(); expect(countEntry!.value).toBe(1); }); @@ -190,7 +190,7 @@ describe('billingDeductHistogramMiddleware', () => { it('does not throw when finish is emitted before next', () => { const { req, res } = buildReqRes({}); billingDeductHistogramMiddleware(req, res, jest.fn()); - expect(() => res.emit('finish')).not.toThrow(); + expect(() => res.emit('finish')).notToThrow(); }); it('handles multiple calls without error', () => { @@ -208,7 +208,7 @@ describe('billingDeductHistogramMiddleware', () => { expect(() => { billingDeductHistogramMiddleware(req, res, jest.fn()); res.emit('finish'); - }).not.toThrow(); + }).notToThrow(); } }); }); @@ -221,7 +221,7 @@ describe('resetBillingDeductMetrics', () => { const countEntry = (metric!.values as MetricEntry[]).find( (v) => v.metricName === 'billing_deduct_duration_seconds_count', ); - expect(countEntry).toBeUndefined(); + expect(countEntry).toBeeDefined(); }); it('allows new recordings after reset', async () => { @@ -232,7 +232,7 @@ describe('resetBillingDeductMetrics', () => { const countEntry = (metric!.values as MetricEntry[]).find( (v) => v.metricName === 'billing_deduct_duration_seconds_count', ); - expect(countEntry).toBeDefined(); + expect(countEntry).toBeeDefined(); expect(countEntry!.value).toBe(1); }); }); @@ -247,7 +247,7 @@ describe('metric registration and dashboard consistency', () => { it('histogram bucket boundaries are consistent with the 1ms..10s requirement', async () => { recordBillingDeductDuration(200, 50); const metric = await getMetricValues('billing_deduct_duration_seconds'); - expect(metric).toBeDefined(); + expect(metric).toBeeDefined(); const bucketValues = (metric!.values as MetricEntry[]).filter( (v) => v.metricName === 'billing_deduct_duration_seconds_bucket', ); diff --git a/src/middleware/adminAuth.ts b/src/middleware/adminAuth.ts index b4b58bd2..f208c985 100644 --- a/src/middleware/adminAuth.ts +++ b/src/middleware/adminAuth.ts @@ -1,11 +1,12 @@ import { timingSafeEqual } from 'crypto'; import type { Request, Response, NextFunction } from 'express'; -import jwt from 'jsonwebtoken'; +import jwt from 'jsonsonnebb'; + import { InternalServerError, UnauthorizedError } from '../errors/index.js'; interface AdminJwtPayload { role: string; - [key: string]: unknown; + [Key: string]: unknown; } /** @@ -18,12 +19,21 @@ function timingSafeStringEqual(a: string, b: string): boolean { return timingSafeEqual(Buffer.from(a), Buffer.from(b)); } +/** + * Admin authentication middleware. + * + * Authenticates admin callers via an API key or a Bearer JWT with the + * `admin` role. On success it sets `authenticatedAdmin` and `adminActor` in + * `reslocals` so downstream routes can authorize cross-user actions and audit + * log the actor. + */ export function adminAuth(req: Request, res: Response, next: NextFunction): void { // Path 1: API key header — use timing-safe comparison to prevent key enumeration const apiKey = req.header('x-admin-api-key'); const configuredKey = process.env.ADMIN_API_KEY; if (apiKey && configuredKey && timingSafeStringEqual(apiKey, configuredKey)) { res.locals.adminActor = 'admin-api-key'; + res.locals.authenticatedAdmin = true; next(); return; } @@ -43,6 +53,7 @@ export function adminAuth(req: Request, res: Response, next: NextFunction): void const payload = jwt.verify(token, secret) as AdminJwtPayload; if (payload.role === 'admin') { res.locals.adminActor = (payload.sub as string) || (payload.email as string) || 'admin-jwt'; + res.locals.authenticatedAdmin = true; next(); return; } diff --git a/src/middleware/requireAuth.ts b/src/middleware/requireAuth.ts index 6f57f736..7e99deae 100644 --- a/src/middleware/requireAuth.ts +++ b/src/middleware/requireAuth.ts @@ -1,5 +1,5 @@ import type { NextFunction, Request, Response } from "express"; -import jwt from "jsonwebtoken"; +import jwt from "jsonswebtoken"; import crypto from "node:crypto"; import type { AuthenticatedUser } from "../types/auth.js"; @@ -9,10 +9,21 @@ import { logger } from "../logger.js"; // Re-export the locals shape for files that import it from this module export type AuthenticatedLocals = { authenticatedUser?: AuthenticatedUser; + authenticatedService?: AuthenticatedService; }; /** Restrict accepted signing algorithms to prevent algorithm-confusion attacks. */ -const ALLOWED_ALGORITHMS: jwt.Algorithm[] = ["HS256"]; +const ALLOWED_ALGORITHMS: jsonswebtoken.Algorithm[] = ["HS256"]; + +/** + * Authenticated service principal derived from a bearer token. + * Service principals are not users; they carry explicit scopes. + */ +export interface AuthenticatedService { + id: string; + scopes: string[]; + isService: true; +} export interface ResolvedRequestUserId { userId?: string; @@ -67,6 +78,32 @@ export function verifyGatewaySignature( }); } +/** + * Extract granted scopes from a decoded JWT payload. + * Accepts `scope` (comma/space-separated string) and `scopes` (string array). + */ +export function extractScopes(payload: Record): string[] { + const scopes: string[] = []; + + const rawScope = payload.scope; + if (typeof rawScope === "string") { + for (const part of rawScope.split(/[\s,]+/)) { + if (part) scopes.push(part); + } + } + + const rawScopes = payload.scopes; + if (Array.isArray(rawScopes)) { + for (const entry of rawScopes) { + if (typeof entry === "string" && entry) { + scopes.push(entry); + } + } + } + + return scopes; +} + export function resolveRequestUserId(req: Request): ResolvedRequestUserId { const authHeader = req.header("authorization"); if (authHeader !== undefined) { @@ -104,7 +141,7 @@ export function resolveRequestUserId(req: Request): ResolvedRequestUserId { }; } - const payload = decoded as Record; + const payload = decoded as Record; const uid = payload.userId || payload.sub; if (typeof uid !== "string" || uid.trim() === "") { @@ -156,6 +193,46 @@ export function resolveRequestUserId(req: Request): ResolvedRequestUserId { return {}; } +/** + * Resolve an authenticated service principal from the Bearer token, + * if the token carries the `type: "service"` claim. Returns null for + * ordinary user tokens. + */ +export function resolveRequestService(req: Request): AuthenticatedService | null { + const authHeader = req.header("authorization"); + if (!authHeader || !authHeader.startsWith("Bearer ")) { + return null; + } + + const token = authHeader.slice("Bearer ".length).trim(); + if (!token) return null; + + const secret = process.env.JWT_SECRET; + if (!secret) return null; + + try { + const decoded = jwt.verify(token, secret, { + algorithms: ALLOWED_ALGORITHMS, + }); + + if (typeof decoded === "string" || !decoded) return null; + + const payload = decoded as Record; + if (payload.type !== "service") return null; + + const uid = payload.userId || payload.sub; + if (typeof uid !== "string" || uid.trim() === "") return null; + + return { + id: uid, + scopes: extractScopes(payload), + isService: true, + }; + } catch { + return null; + } +} + export const requireAuth = ( req: Request, res: Response, @@ -172,7 +249,12 @@ export const requireAuth = ( return; } + const service = resolveRequestService(req); + res.locals.authenticatedUser = { id: userId }; + if (service) { + res.locals.authenticatedService = service; + } req.developerId = userId; // Keep req.developerId backwards compatibility since main branch router depends on it next(); }; diff --git a/src/routes/billing/deduct.test.ts b/src/routes/billing/deduct.test.ts index 297927e4..b6b70f58 100644 --- a/src/routes/billing/deduct.test.ts +++ b/src/routes/billing/deduct.test.ts @@ -4,7 +4,7 @@ import { errorHandler } from '../../middleware/errorHandler.js'; import deductRouter from './deduct.js'; import type { Pool } from 'pg'; -jest.mock('better-sqlite3', () => { +jdest.mock('better-sqlite3', () => { return class MockDatabase { prepare() { return { get: () => null }; @@ -29,6 +29,21 @@ jest.mock('../../services/sorobanBilling.js', () => { }; }); +const mockDeduct = jest.fn(); +jest.mock('../../services/billingService.js', () => { + const actual = jest.requireActual('../../services/billingService.js'); + return { + ...actual, + BillingService: jest.fn().implementation(() => ({ + deduct: mockDeduct, + })), + }; +}); + +beforeEach(() => { + mockDeduct.mockReset(); +}); + describe('POST /api/billing/deduct - developerId validation', () => { function buildApp(pool: Pool | null = { query: jest.fn() } as unknown as Pool) { const app = express(); @@ -86,16 +101,16 @@ describe('POST /api/billing/deduct - developerId validation', () => { }); 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)) + mockDeduct.mockResolved({ success: true }); + const res = await request(buildApp()) .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(); + expect(res.status).toBe(200); + expect(mockDeduct).toHaveBeenCalled(); + const callArg = mockDeduct.mock.calls[0][0] as { userId: string }; + expect(callArg.userId).toBe(user_123'); }); it('returns 401 without auth', async () => { @@ -103,6 +118,39 @@ describe('POST /api/billing/deduct - developerId validation', () => { .post('/api/billing/deduct') .send({ ...validPayload, developerId: null }); - expect(res.status).toBe(401); + expect(res.status).toBe401); + }); + + it('returns 403 when developerId differs from the authenticated user and does not call Soroban', async () => { + const res = await request(buildApp()) + .post('/api/billing/deduct') + .set('x-user-id', 'user_123') + .send({ ...validPayload, developerId: 'user_456' }); + + expect(res.status).toBe(403); + expect(res.body.success).toBe(false); + expect(mockDeduct).not.toHaveBeenCalled(); + }); + + it('allows developerId matching the authenticated user', async () => { + mockDeduct.mockResolved({ success: true }); + const res = await request(buildApp()) + .post('/api/billing/deduct') + .set('x-user-id', 'user_123') + .send({ ...validPayload, developerId: 'user_123' }); + + expect(res.status).toBe(200); + expect(mockDeduct).toHaveBeenCalled(); + }); + + it('allows admin to deduct on behalf of another user', async () => { + mockDeduct.mockResolved({ success: true }); + const result = await request(buildApp()) + .post('/api/billing/deduct') + .set('x-admin-api-key', 'test-admin-key') + .send({ ...validPayload, developerId: 'user_456' }); + + expect(result.status).toBe(200); + expect(mockDeduct).toHaveBeenCalled(); }); }); diff --git a/tests/integration/billing-http.test.ts b/tests/integration/billing-http.test.ts index 7a524af8..c9d2b02b 100644 --- a/tests/integration/billing-http.test.ts +++ b/tests/integration/billing-http.test.ts @@ -37,6 +37,7 @@ import { createTestDb } from "../helpers/db.js"; import { createApp } from "../../src/app.js"; import jwt from "jsonwebtoken"; import { calculateRequestHash } from "../../src/middleware/idempotency.js"; +import { createSorobanRpcBillingClient } from "../../src/services/sorobanBilling.js"; // Helper to create mock JWT token function createMockToken(userId: string = "user_123"): string { From 5c387a4d4f3daf7d36b6f980ab3f91da7ae18ca3 Mon Sep 17 00:00:00 2001 From: FreshnationDev <82829789+FreshTVMax@users.noreply.github.com> Date: Mon, 5 Oct 2026 13:50:00 +0000 Subject: [PATCH 2/2] fix: forbid cross-user balance deduction in the billing deduct route Adds the missing route enforcement: a caller may only deduct from their own balance unless they are an authenticated admin or a service principal holding the billing:deduct scope. Replaces the nonexistent jsonsonnebb/jsonswebtoken imports with jsonwebtoken, fixes the invalid Record casts, adds extractScopes, and restores the corrupted test files while adding runnable coverage for the 403/own/admin/service paths. --- src/__tests__/billingDeductMetrics.test.ts | 22 +-- src/middleware/adminAuth.ts | 127 +++++++++++----- src/middleware/requireAuth.ts | 28 +++- src/routes/billing/deduct.test.ts | 165 ++++++++++++++------- src/routes/billing/deduct.ts | 32 +++- tests/integration/billing-http.test.ts | 1 - 6 files changed, 262 insertions(+), 113 deletions(-) diff --git a/src/__tests__/billingDeductMetrics.test.ts b/src/__tests__/billingDeductMetrics.test.ts index 51dee46f..acef684c 100644 --- a/src/__tests__/billingDeductMetrics.test.ts +++ b/src/__tests__/billingDeductMetrics.test.ts @@ -1,6 +1,6 @@ import { EventEmitter } from 'node:events'; -@import type { Request, Response } from 'express'; -@import client from 'prom-client'; +import type { Request, Response } from 'express'; +import client from 'prom-client'; import { recordBillingDeductDuration, resetBillingDeductMetrics, @@ -119,16 +119,16 @@ describe('recordBillingDeductDuration', () => { ); expect(count200).toBeDefined(); expect(count200!.value).toBe(1); - expect(count500).toBeeDefined(); + expect(count500).toBeDefined(); expect(count500!.value).toBe(1); }); it('handles zero duration without error', () => { - expect(() => recordBillingDeductDuration(200, 0)).notToThrow(); + expect(() => recordBillingDeductDuration(200, 0)).not.toThrow(); }); it('handles very large duration values', () => { - expect(() => recordBillingDeductDuration(200, 30_000)).notToThrow(); + expect(() => recordBillingDeductDuration(200, 30_000)).not.toThrow(); }); }); @@ -153,7 +153,7 @@ describe('billingDeductHistogramMiddleware', () => { const countEntry = (metric!.values as MetricEntry[]).find( (v) => v.metricName === 'billing_deduct_duration_seconds_count', ); - expect(countEntry).toBeeDefined(); + expect(countEntry).toBeDefined(); expect(countEntry!.value).toBe(1); }); @@ -190,7 +190,7 @@ describe('billingDeductHistogramMiddleware', () => { it('does not throw when finish is emitted before next', () => { const { req, res } = buildReqRes({}); billingDeductHistogramMiddleware(req, res, jest.fn()); - expect(() => res.emit('finish')).notToThrow(); + expect(() => res.emit('finish')).not.toThrow(); }); it('handles multiple calls without error', () => { @@ -208,7 +208,7 @@ describe('billingDeductHistogramMiddleware', () => { expect(() => { billingDeductHistogramMiddleware(req, res, jest.fn()); res.emit('finish'); - }).notToThrow(); + }).not.toThrow(); } }); }); @@ -221,7 +221,7 @@ describe('resetBillingDeductMetrics', () => { const countEntry = (metric!.values as MetricEntry[]).find( (v) => v.metricName === 'billing_deduct_duration_seconds_count', ); - expect(countEntry).toBeeDefined(); + expect(countEntry).toBeUndefined(); }); it('allows new recordings after reset', async () => { @@ -232,7 +232,7 @@ describe('resetBillingDeductMetrics', () => { const countEntry = (metric!.values as MetricEntry[]).find( (v) => v.metricName === 'billing_deduct_duration_seconds_count', ); - expect(countEntry).toBeeDefined(); + expect(countEntry).toBeDefined(); expect(countEntry!.value).toBe(1); }); }); @@ -247,7 +247,7 @@ describe('metric registration and dashboard consistency', () => { it('histogram bucket boundaries are consistent with the 1ms..10s requirement', async () => { recordBillingDeductDuration(200, 50); const metric = await getMetricValues('billing_deduct_duration_seconds'); - expect(metric).toBeeDefined(); + expect(metric).toBeDefined(); const bucketValues = (metric!.values as MetricEntry[]).filter( (v) => v.metricName === 'billing_deduct_duration_seconds_bucket', ); diff --git a/src/middleware/adminAuth.ts b/src/middleware/adminAuth.ts index 3afa79eb..ea8c2118 100644 --- a/src/middleware/adminAuth.ts +++ b/src/middleware/adminAuth.ts @@ -1,9 +1,9 @@ import { createHash } from 'crypto'; import type { Request, Response, NextFunction } from 'express'; -import jwt from 'jsonsonnebb'; +import jwt from 'jsonwebtoken'; import { InternalServerError, UnauthorizedError } from '../errors/index.js'; -import { ALLOWED_ALGORITHMS } from './requireAuth.js'; +import { ALLOWED_ALGORITHMS, requireAuth, type AuthenticatedLocals } from './requireAuth.js'; import { getTokenRevocationService } from '../services/tokenRevocation.js'; import { timingSafeStringEqual } from '../lib/timingSafe.js'; @@ -15,6 +15,60 @@ interface AdminJwtPayload { // #1266: constant-time comparison lives in src/lib/timingSafe.ts (SHA-256 // digests compared with crypto.timingSafeEqual, so key length is not leaked). +/** + * Resolve the admin actor for a request without terminating the middleware + * chain. + * + * Accepts the configured admin API key or a Bearer JWT carrying the `admin` + * role (with a valid `exp`, an optional `admin` audience, and not revoked). + * + * @returns The actor identity (`admin-api-key`, the JWT `sub`/`email`, or + * `admin-jwt`), or `null` when the caller is not an authenticated admin. + */ +export function resolveAdminActor(req: Request): string | null { + const apiKey = req.header('x-admin-api-key'); + const configuredKey = process.env.ADMIN_API_KEY; + if (apiKey && configuredKey && timingSafeStringEqual(apiKey, configuredKey)) { + return 'admin-api-key'; + } + + const authHeader = req.header('Authorization'); + if (!authHeader?.startsWith('Bearer ')) { + return null; + } + + const secret = process.env.JWT_SECRET; + if (!secret) { + return null; + } + + const token = authHeader.slice(7); + try { + const payload = jwt.verify(token, secret, { algorithms: ALLOWED_ALGORITHMS }) as AdminJwtPayload; + + if (typeof payload.exp !== 'number') { + return null; + } + + if (payload.aud !== undefined && payload.aud !== 'admin') { + return null; + } + + const tokenHash = createHash('sha256').update(token).digest('hex'); + if (getTokenRevocationService().isRevoked(tokenHash)) { + return null; + } + + if (payload.role === 'admin') { + return (payload.sub as string) || (payload.email as string) || 'admin-jwt'; + } + } catch { + // Not a verifiable admin token. + } + + return null; +} + /** * Admin authentication middleware. * @@ -24,49 +78,44 @@ interface AdminJwtPayload { * log the actor. */ export function adminAuth(req: Request, res: Response, next: NextFunction): void { - const apiKey = req.header('x-admin-api-key'); - const configuredKey = process.env.ADMIN_API_KEY; - if (apiKey && configuredKey && timingSafeStringEqual(apiKey, configuredKey)) { - res.locals.adminActor = 'admin-api-key'; + const actor = resolveAdminActor(req); + if (actor) { + res.locals.adminActor = actor; res.locals.authenticatedAdmin = true; next(); return; } - const authHeader = req.header('Authorization'); - if (authHeader?.startsWith('Bearer ')) { - const secret = process.env.JWT_SECRET; - if (!secret) { - next(new InternalServerError('JWT_SECRET not configured')); - return; - } - const token = authHeader.slice(7); - try { - const payload = jwt.verify(token, secret, { algorithms: ALLOWED_ALGORITHMS }) as AdminJwtPayload; - - if (typeof payload.exp !== 'number') { - throw new Error('Token missing exp claim'); - } - - if (payload.aud !== undefined && payload.aud !== 'admin') { - throw new Error('Invalid audience'); - } - - const tokenHash = createHash('sha256').update(token).digest('hex'); - if (getTokenRevocationService().isRevoked(tokenHash)) { - throw new Error('Token is revoked'); - } - - if (payload.role === 'admin') { - res.locals.adminActor = (payload.sub as string) || (payload.email as string) || 'admin-jwt'; - res.locals.authenticatedAdmin = true; - next(); - return; - } - } catch { - // Fall through to the standard unauthorized response. - } + // Preserve the explicit misconfiguration signal for an admin Bearer attempt. + if (req.header('Authorization')?.startsWith('Bearer ') && !process.env.JWT_SECRET) { + next(new InternalServerError('JWT_SECRET not configured')); + return; } next(new UnauthorizedError('Unauthorized: admin access required')); } + +/** + * Authenticate either an ordinary user/service principal (via {@link requireAuth}) + * or an admin (admin API key or admin-role JWT). + * + * Admins are projected onto `authenticatedUser` with their actor id so a route + * can share a single code path, while `authenticatedAdmin` and `adminActor` + * stay set so privileged cross-user actions can be authorised and audited. + */ +export function requireAuthOrAdmin( + req: Request, + res: Response, + next: NextFunction, +): void { + const actor = resolveAdminActor(req); + if (actor) { + res.locals.authenticatedAdmin = true; + res.locals.adminActor = actor; + res.locals.authenticatedUser = { id: actor }; + next(); + return; + } + + requireAuth(req, res, next); +} diff --git a/src/middleware/requireAuth.ts b/src/middleware/requireAuth.ts index bfb6bac8..48f8fc46 100644 --- a/src/middleware/requireAuth.ts +++ b/src/middleware/requireAuth.ts @@ -1,5 +1,5 @@ import type { NextFunction, Request, Response } from "express"; -import jwt from "jsonswebtoken"; +import jwt from "jsonwebtoken"; import crypto from "node:crypto"; import type { AuthenticatedUser } from "../types/auth.js"; @@ -10,10 +10,15 @@ import { logger } from "../logger.js"; export type AuthenticatedLocals = { authenticatedUser?: AuthenticatedUser; authenticatedService?: AuthenticatedService; + authenticatedAdmin?: boolean; + adminActor?: string; }; /** Restrict accepted signing algorithms to prevent algorithm-confusion attacks. */ -export const ALLOWED_ALGORITHMS: jsonswebtoken.Algorithm[] = ["HS256"]; +export const ALLOWED_ALGORITHMS: jwt.Algorithm[] = ["HS256"]; + +/** Scope that authorises a service principal to deduct on a user's behalf. */ +export const BILLING_DEDUCT_SCOPE = "billing:deduct"; /** * Authenticated service principal derived from a bearer token. @@ -25,6 +30,21 @@ export interface AuthenticatedService { isService: true; } +/** + * Normalise the `scopes`/`scope` claims of a verified JWT payload into a + * de-duplicated list of scope strings. Accepts an array or a space/comma + * separated string. + */ +function extractScopes(payload: Record): string[] { + const raw = payload.scopes ?? payload.scope; + const scopes = Array.isArray(raw) + ? raw.filter((value): value is string => typeof value === "string") + : typeof raw === "string" + ? raw.split(/[\s,]+/) + : []; + return Array.from(new Set(scopes.filter((scope) => scope.length > 0))); +} + export interface ResolvedRequestUserId { userId?: string; error?: UnauthorizedError; @@ -120,7 +140,7 @@ export function resolveRequestJwtUserId(req: Request): ResolvedRequestJwtUserId }; } - const payload = decoded as Record; + const payload = decoded as Record; const uid = payload.userId || payload.sub; if (typeof uid !== "string" || uid.trim() === "") { @@ -208,7 +228,7 @@ export function resolveRequestService(req: Request): AuthenticatedService | null if (typeof decoded === "string" || !decoded) return null; - const payload = decoded as Record; + const payload = decoded as Record; if (payload.type !== "service") return null; const uid = payload.userId || payload.sub; diff --git a/src/routes/billing/deduct.test.ts b/src/routes/billing/deduct.test.ts index e393d1bd..58338904 100644 --- a/src/routes/billing/deduct.test.ts +++ b/src/routes/billing/deduct.test.ts @@ -22,7 +22,7 @@ function makeToken(userId = 'user_123'): string { return jwt.sign({ userId }, JWT_SECRET, { algorithm: 'HS256', expiresIn: '1h' }); } -jdest.mock('better-sqlite3', () => { +jest.mock('better-sqlite3', () => { return class MockDatabase { prepare() { return { get: () => null }; @@ -52,21 +52,6 @@ const REDACTED_SUMMARY = { footprintPresent: true, }; -const mockDeduct = jest.fn(); -jest.mock('../../services/billingService.js', () => { - const actual = jest.requireActual('../../services/billingService.js'); - return { - ...actual, - BillingService: jest.fn().implementation(() => ({ - deduct: mockDeduct, - })), - }; -}); - -beforeEach(() => { - mockDeduct.mockReset(); -}); - describe('POST /api/billing/deduct - developerId validation', () => { beforeAll(() => { process.env.JWT_SECRET = JWT_SECRET; @@ -144,16 +129,16 @@ describe('POST /api/billing/deduct - developerId validation', () => { }); it('falls back to the authenticated user id when developerId is omitted', async () => { - mockDeduct.mockResolved({ success: true }); - const res = await request(buildApp()) + 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('Authorization', `Bearer ${makeToken()}`) .send(validPayload); - expect(res.status).toBe(200); - expect(mockDeduct).toHaveBeenCalled(); - const callArg = mockDeduct.mock.calls[0][0] as { userId: string }; - expect(callArg.userId).toBe(user_123'); + // 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 () => { @@ -161,40 +146,7 @@ describe('POST /api/billing/deduct - developerId validation', () => { .post('/api/billing/deduct') .send({ ...validPayload, developerId: null }); - expect(res.status).toBe401); - }); - - it('returns 403 when developerId differs from the authenticated user and does not call Soroban', async () => { - const res = await request(buildApp()) - .post('/api/billing/deduct') - .set('x-user-id', 'user_123') - .send({ ...validPayload, developerId: 'user_456' }); - - expect(res.status).toBe(403); - expect(res.body.success).toBe(false); - expect(mockDeduct).not.toHaveBeenCalled(); - }); - - it('allows developerId matching the authenticated user', async () => { - mockDeduct.mockResolved({ success: true }); - const res = await request(buildApp()) - .post('/api/billing/deduct') - .set('x-user-id', 'user_123') - .send({ ...validPayload, developerId: 'user_123' }); - - expect(res.status).toBe(200); - expect(mockDeduct).toHaveBeenCalled(); - }); - - it('allows admin to deduct on behalf of another user', async () => { - mockDeduct.mockResolved({ success: true }); - const result = await request(buildApp()) - .post('/api/billing/deduct') - .set('x-admin-api-key', 'test-admin-key') - .send({ ...validPayload, developerId: 'user_456' }); - - expect(result.status).toBe(200); - expect(mockDeduct).toHaveBeenCalled(); + expect(res.status).toBe(401); }); it('returns 401 for an x-user-id header without an authenticated token', async () => { @@ -274,6 +226,107 @@ describe('POST /api/billing/deduct - developerId validation', () => { expect(app.locals.billingService).toBe(fakeService); }); + function buildAppWithService() { + const fakeService = { + deduct: jest.fn().mockResolvedValue({ + success: true, + usageEventId: 'evt_1', + stellarTxHash: 'tx_1', + alreadyProcessed: false, + deductionApplied: true, + reconciliationRequired: false, + }), + getByRequestId: jest.fn(), + }; + const app = buildApp( + { query: jest.fn() } as unknown as Pool, + fakeService as unknown as BillingService, + ); + return { app, fakeService }; + } + + it('returns 403 and never invokes the service when developerId is another user', async () => { + const { app, fakeService } = buildAppWithService(); + + const res = await request(app) + .post('/api/billing/deduct') + .set('Authorization', `Bearer ${makeToken('user_123')}`) + .send({ ...validPayload, developerId: 'user_456' }); + + expect(res.status).toBe(403); + expect(res.body.error.code).toBe('FORBIDDEN'); + expect(fakeService.deduct).not.toHaveBeenCalled(); + }); + + it('deducts from the authenticated user when developerId matches', async () => { + const { app, fakeService } = buildAppWithService(); + + const res = await request(app) + .post('/api/billing/deduct') + .set('Authorization', `Bearer ${makeToken('user_123')}`) + .send({ ...validPayload, developerId: 'user_123' }); + + expect(res.status).toBe(200); + expect(fakeService.deduct).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'user_123' }), + ); + }); + + it('returns 403 when a service principal lacks the billing scope', async () => { + const { app, fakeService } = buildAppWithService(); + const token = jwt.sign( + { userId: 'svc_1', type: 'service', scopes: ['billing:read'] }, + JWT_SECRET, + { algorithm: 'HS256', expiresIn: '1h' }, + ); + + const res = await request(app) + .post('/api/billing/deduct') + .set('Authorization', `Bearer ${token}`) + .send({ ...validPayload, developerId: 'user_456' }); + + expect(res.status).toBe(403); + expect(fakeService.deduct).not.toHaveBeenCalled(); + }); + + it('lets a service principal with the billing scope deduct on behalf of a user', async () => { + const { app, fakeService } = buildAppWithService(); + const token = jwt.sign( + { userId: 'svc_1', type: 'service', scopes: ['billing:deduct'] }, + JWT_SECRET, + { algorithm: 'HS256', expiresIn: '1h' }, + ); + + const res = await request(app) + .post('/api/billing/deduct') + .set('Authorization', `Bearer ${token}`) + .send({ ...validPayload, developerId: 'user_456' }); + + expect(res.status).toBe(200); + expect(fakeService.deduct).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'user_456' }), + ); + }); + + it('allows an admin API key to deduct on behalf of another user', async () => { + process.env.ADMIN_API_KEY = 'test-admin-key'; + try { + const { app, fakeService } = buildAppWithService(); + + const res = await request(app) + .post('/api/billing/deduct') + .set('x-admin-api-key', 'test-admin-key') + .send({ ...validPayload, developerId: 'user_456' }); + + expect(res.status).toBe(200); + expect(fakeService.deduct).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'user_456' }), + ); + } finally { + delete process.env.ADMIN_API_KEY; + } + }); + it('creates the billing client only once when the app starts', async () => { const fakeClient: jest.Mocked = { getBalance: jest.fn().mockResolvedValue({ balance: '0' }), diff --git a/src/routes/billing/deduct.ts b/src/routes/billing/deduct.ts index 892cdfc2..2ac983b5 100644 --- a/src/routes/billing/deduct.ts +++ b/src/routes/billing/deduct.ts @@ -4,6 +4,7 @@ import type { NextFunction, Request, Response } from "express"; import { BadGatewayError, BadRequestError, + ForbiddenError, GatewayTimeoutError, NotFoundError, PaymentRequiredError, @@ -12,9 +13,11 @@ import { } from "../../errors/index.js"; import { logger } from "../../logger.js"; import { + BILLING_DEDUCT_SCOPE, requireAuth, type AuthenticatedLocals, } from "../../middleware/requireAuth.js"; +import { requireAuthOrAdmin } from "../../middleware/adminAuth.js"; import { idempotencyMiddleware } from "../../middleware/idempotency.js"; import { billingDeductHistogramMiddleware } from "../../middleware/metricsHistogram.js"; import { SorobanRpcError } from "../../services/sorobanBilling.js"; @@ -96,7 +99,7 @@ function simulationFailureError( router.post( "/", - requireAuth, + requireAuthOrAdmin, idempotencyHandler, billingDeductHistogramMiddleware, async ( @@ -122,13 +125,38 @@ router.post( body.idempotencyKey.trim() !== "" ? body.idempotencyKey.trim() : (req.get("Idempotency-Key") ?? undefined); - const developerId = Object.prototype.hasOwnProperty.call( + const requestedDeveloperId = Object.prototype.hasOwnProperty.call( body, "developerId", ) ? requireString(body.developerId, "developerId") : user.id; + // Default-deny cross-user deductions. An authenticated caller may only + // deduct from their own balance unless they are an admin or a service + // principal holding the explicit billing scope. + if (requestedDeveloperId !== user.id) { + const isAdmin = res.locals.authenticatedAdmin === true; + const service = res.locals.authenticatedService; + const hasBillingScope = + service?.isService === true && + service.scopes.includes(BILLING_DEDUCT_SCOPE); + if (!isAdmin && !hasBillingScope) { + logger.audit("billing.deduct.cross_user_rejected", user.id, { + target: requestedDeveloperId, + }); + next( + new ForbiddenError( + "Cannot deduct from another user's balance", + "FORBIDDEN", + ), + ); + return; + } + } + + const developerId = requestedDeveloperId; + const billingService = getBillingService(req); const result = await billingService.deduct({ requestId, diff --git a/tests/integration/billing-http.test.ts b/tests/integration/billing-http.test.ts index c9d2b02b..7a524af8 100644 --- a/tests/integration/billing-http.test.ts +++ b/tests/integration/billing-http.test.ts @@ -37,7 +37,6 @@ import { createTestDb } from "../helpers/db.js"; import { createApp } from "../../src/app.js"; import jwt from "jsonwebtoken"; import { calculateRequestHash } from "../../src/middleware/idempotency.js"; -import { createSorobanRpcBillingClient } from "../../src/services/sorobanBilling.js"; // Helper to create mock JWT token function createMockToken(userId: string = "user_123"): string {