diff --git a/src/__tests__/proxy.integration.test.ts b/src/__tests__/proxy.integration.test.ts index 8a6e892..ff2c94e 100644 --- a/src/__tests__/proxy.integration.test.ts +++ b/src/__tests__/proxy.integration.test.ts @@ -10,7 +10,6 @@ import { InMemoryRateLimiter } from '../services/rateLimiter.js'; import { InMemoryUsageStore } from '../services/usageStore.js'; import { InMemoryApiRegistry } from '../data/apiRegistry.js'; import { ApiKey, ApiRegistryEntry } from '../types/gateway.js'; -import { errorHandler } from '../middleware/errorHandler.js'; // ── Test fixtures ─────────────────────────────────────────────────────────── @@ -362,7 +361,6 @@ describe('Proxy /v1/call', () => { expect(body.message ?? body.error).toMatch(/bad gateway/i); await new Promise((resolve) => tmpServer.close(() => resolve())); - lookupSpy.mockRestore(); }); }); diff --git a/src/repositories/apiKeyRepository.ts b/src/repositories/apiKeyRepository.ts index 84b4c7b..bc327bc 100644 --- a/src/repositories/apiKeyRepository.ts +++ b/src/repositories/apiKeyRepository.ts @@ -2,6 +2,21 @@ import { randomBytes, timingSafeEqual } from "crypto"; import bcrypt from "bcryptjs"; import { config } from "../config/index.js"; +/** + * Typed error returned when an API key prefix is found in the store but the + * full-key hash comparison fails. Callers should map this to a 401 response + * so the distinction between "prefix not found" and "hash mismatch" is never + * observable externally (no timing oracle — both paths yield the same status). + */ +export class InvalidKeyError extends Error { + public readonly code = 'INVALID_KEY' as const; + constructor(message = 'Invalid API key') { + super(message); + this.name = 'InvalidKeyError'; + Object.setPrototypeOf(this, InvalidKeyError.prototype); + } +} + export interface ApiKeyRecord { id: string; apiId: string; @@ -100,24 +115,46 @@ export const apiKeyRepository = { constantTimeCompare(k.prefix, prefix), ); + // No records share this prefix — key does not exist at all. + if (candidates.length === 0) return null; + for (const candidate of candidates) { - if (!candidate.revoked && verifyHash(key, candidate.keyHash)) { - // Return a copy without sensitive data + if (verifyHash(key, candidate.keyHash)) { + if (candidate.revoked) { + // Prefix + hash matched a revoked key — let the caller handle 403. + return { + id: candidate.id, + apiId: candidate.apiId, + userId: candidate.userId, + prefix: candidate.prefix, + keyHash: '[REDACTED]', + scopes: candidate.scopes, + rateLimitPerMinute: candidate.rateLimitPerMinute, + createdAt: candidate.createdAt, + revoked: candidate.revoked, + }; + } + // Return a copy without the raw hash so callers never see the secret. return { id: candidate.id, apiId: candidate.apiId, userId: candidate.userId, prefix: candidate.prefix, - keyHash: "[REDACTED]", + keyHash: '[REDACTED]', scopes: candidate.scopes, rateLimitPerMinute: candidate.rateLimitPerMinute, createdAt: candidate.createdAt, - revoked: candidate.revoked + revoked: candidate.revoked, }; } } - return null; + // Prefix was found in the store but no candidate's hash matched the supplied + // key. Throw a typed error so callers can distinguish this from "key not + // found" while still mapping both outcomes to the same 401 response — this + // avoids leaking whether a prefix exists (timing oracle) while keeping + // error-handling explicit. + throw new InvalidKeyError(); }, rotate(id: string, userId: string): { success: true; newKey: string; prefix: string } | { success: false; error: 'not_found' | 'forbidden' | 'revoked' } { const index = apiKeys.findIndex(k => k.id === id); diff --git a/src/routes/gatewayRoutes.test.ts b/src/routes/gatewayRoutes.test.ts index 155332b..9cc3bfe 100644 --- a/src/routes/gatewayRoutes.test.ts +++ b/src/routes/gatewayRoutes.test.ts @@ -4,6 +4,7 @@ import { createGatewayRouter } from "./gatewayRoutes.js"; import { createRateLimiter } from "../services/rateLimiter.js"; import { errorHandler } from "../middleware/errorHandler.js"; import { requestIdMiddleware } from "../middleware/requestId.js"; +import type { ApiKey } from "../types/gateway.js"; describe("gateway route - rate limiting", () => { let now = 0; @@ -198,3 +199,157 @@ describe("gateway route - body size limits", () => { expect(res.body).toHaveProperty("message", "Request body too large"); }); }); + +// --------------------------------------------------------------------------- +// Bug #421 — prefix-exists-but-hash-mismatch must return 401, not 500 +// --------------------------------------------------------------------------- +describe("gateway route - API key prefix / hash mismatch (bug #421)", () => { + /** + * Build a minimal app wired to a controlled apiKeys Map. + * Billing is set to fail fast (402) so we never attempt a real upstream + * request — we only care about the auth layer here. + */ + function buildApp(apiKeys: Map) { + const deps = { + billing: { deductCredit: async () => ({ success: false, balance: 0 }) }, + rateLimiter: { check: async () => ({ allowed: true }) }, + usageStore: { record: () => true }, + upstreamUrl: "http://example.invalid", + apiKeys, + } as any; + + const app = express(); + app.use(requestIdMiddleware); + app.use("/gateway", createGatewayRouter(deps)); + app.use(errorHandler); + return app; + } + + const API_ID = "my-api"; + + /** + * Construct a key whose first 16 characters (the prefix) are identical to + * `validKey` but whose remaining characters differ — prefix matches but the + * SHA-256 hash of the full key will not match. + */ + function buildMismatchedKey(validKey: string): string { + // Keep the same 16-char prefix, replace the rest so the hash diverges. + const prefix = validKey.slice(0, 16); + const differentSuffix = "X".repeat(validKey.length - 16); + return prefix + differentSuffix; + } + + test("returns 401 (not 500) when prefix matches but hash mismatches", async () => { + const validKey = "test-key-abcdefgh"; // 17 chars; prefix = "test-key-abcdefg" + const apiKeys = new Map(); + apiKeys.set(validKey, { key: "k1", apiId: API_ID, developerId: "dev1" }); + + const app = buildApp(apiKeys); + const mismatchedKey = buildMismatchedKey(validKey); + + const res = await request(app) + .get(`/gateway/${API_ID}`) + .set("x-api-key", mismatchedKey); + + // Must be 401, never 500. + expect(res.status).toBe(401); + expect(res.body).toHaveProperty("code", "UNAUTHORIZED"); + // Generic message — must not reveal whether the prefix was found. + expect(res.body.message).toMatch(/invalid API key/i); + }); + + test("returns 401 when prefix does not exist at all (no-prefix path)", async () => { + const validKey = "test-key-abcdefgh"; + const apiKeys = new Map(); + apiKeys.set(validKey, { key: "k1", apiId: API_ID, developerId: "dev1" }); + + const app = buildApp(apiKeys); + + // Completely different prefix — no candidate will be found. + const res = await request(app) + .get(`/gateway/${API_ID}`) + .set("x-api-key", "totally-unknown-key-xyz"); + + expect(res.status).toBe(401); + expect(res.body).toHaveProperty("code", "UNAUTHORIZED"); + expect(res.body.message).toMatch(/invalid API key/i); + }); + + test("happy path — exact key match passes auth and reaches billing", async () => { + const validKey = "test-key-abcdefgh"; + const apiKeys = new Map(); + apiKeys.set(validKey, { key: "k1", apiId: API_ID, developerId: "dev1" }); + + const app = buildApp(apiKeys); + + // Billing is stubbed to fail (402) so we know auth succeeded. + const res = await request(app) + .get(`/gateway/${API_ID}`) + .set("x-api-key", validKey); + + expect(res.status).toBe(402); + expect(res.status).not.toBe(401); + expect(res.status).not.toBe(500); + }); + + test("returns 401 when the matching key belongs to a different apiId", async () => { + const validKey = "test-key-abcdefgh"; + const apiKeys = new Map(); + // Key is registered under "other-api", not "my-api". + apiKeys.set(validKey, { key: "k1", apiId: "other-api", developerId: "dev1" }); + + const app = buildApp(apiKeys); + + const res = await request(app) + .get(`/gateway/${API_ID}`) + .set("x-api-key", validKey); + + expect(res.status).toBe(401); + expect(res.body).toHaveProperty("code", "UNAUTHORIZED"); + }); + + test("returns 403 when key is revoked (prefix + hash match a revoked key)", async () => { + const validKey = "test-key-abcdefgh"; + const apiKeys = new Map(); + apiKeys.set(validKey, { + key: "k1", + apiId: API_ID, + developerId: "dev1", + revoked: true, + }); + + const app = buildApp(apiKeys); + + const res = await request(app) + .get(`/gateway/${API_ID}`) + .set("x-api-key", validKey); + + expect(res.status).toBe(403); + expect(res.body).toHaveProperty("code", "FORBIDDEN"); + }); + + test("401 response body is identical for both mismatch and no-prefix cases (no timing oracle via body)", async () => { + const validKey = "test-key-abcdefgh"; + const apiKeys = new Map(); + apiKeys.set(validKey, { key: "k1", apiId: API_ID, developerId: "dev1" }); + + const app = buildApp(apiKeys); + + const [mismatchRes, unknownRes] = await Promise.all([ + request(app) + .get(`/gateway/${API_ID}`) + .set("x-api-key", buildMismatchedKey(validKey)), + request(app) + .get(`/gateway/${API_ID}`) + .set("x-api-key", "totally-unknown-key-xyz"), + ]); + + // Status codes must be identical. + expect(mismatchRes.status).toBe(401); + expect(unknownRes.status).toBe(401); + + // Response codes must be identical — client must not be able to distinguish. + expect(mismatchRes.body.code).toBe(unknownRes.body.code); + expect(mismatchRes.body.message).toBe(unknownRes.body.message); + }); +}); diff --git a/src/routes/gatewayRoutes.ts b/src/routes/gatewayRoutes.ts index 663e217..2a5982b 100644 --- a/src/routes/gatewayRoutes.ts +++ b/src/routes/gatewayRoutes.ts @@ -1,9 +1,9 @@ -import { randomUUID } from 'node:crypto'; +import { randomUUID, timingSafeEqual, createHash } from 'node:crypto'; import express, { Router, type Request, type Response, type NextFunction } from 'express'; import { z } from 'zod'; import { startUpstreamTimer, type UpstreamOutcome } from '../metrics.js'; import { validate } from '../middleware/validate.js'; -import type { GatewayDeps } from '../types/gateway.js'; +import type { GatewayDeps, ApiKey } from '../types/gateway.js'; import { buildHopByHopSet } from '../lib/hopByHop.js'; import { BadGatewayError, @@ -14,6 +14,68 @@ import { UnauthorizedError, } from '../errors/index.js'; +/** Length of the key prefix used for candidate pre-filtering (matches repository). */ +const API_KEY_PREFIX_LENGTH = 16; + +/** + * Derive the SHA-256 hex digest of an API key value. + * This matches the hash strategy used by createMapBackedGatewayApiKeyAuthMiddleware. + */ +function sha256Hex(value: string): string { + return createHash('sha256').update(value).digest('hex'); +} + +/** + * Constant-time string equality check. + * Buffers are always the same length (padded to the longer) so the comparison + * time is not a function of where the strings diverge — no timing oracle. + */ +function timingSafeStringEqual(a: string, b: string): boolean { + const bufA = Buffer.from(a); + const bufB = Buffer.from(b); + if (bufA.length !== bufB.length) return false; + return timingSafeEqual(bufA, bufB); +} + +/** + * Resolve an API key string to its registered record using prefix-based + * candidate filtering followed by constant-time hash comparison. + * + * Returns: + * - `{ record }` on a valid match + * - `{ error: 'not_found' }` when no candidate shares the prefix + * - `{ error: 'hash_mismatch' }` when a prefix was found but the hash did not + * match — callers MUST map both error variants to the same 401 response so + * the difference is never observable externally. + * + * The explicit `hash_mismatch` discriminant exists purely for internal + * observability (logging, metrics) without leaking any information to clients. + */ +function resolveApiKey( + apiKeyHeader: string, + apiKeys: Map, +): { record: ApiKey } | { error: 'not_found' | 'hash_mismatch' } { + const prefix = apiKeyHeader.slice(0, API_KEY_PREFIX_LENGTH); + const inboundHash = sha256Hex(apiKeyHeader); + + let prefixFound = false; + + for (const [rawKey, record] of apiKeys) { + if (!timingSafeStringEqual(rawKey.slice(0, API_KEY_PREFIX_LENGTH), prefix)) { + continue; + } + // At least one candidate shares the prefix. + prefixFound = true; + + const storedHash = sha256Hex(rawKey); + if (timingSafeStringEqual(inboundHash, storedHash)) { + return { record }; + } + } + + return { error: prefixFound ? 'hash_mismatch' : 'not_found' }; +} + const CREDIT_COST_PER_CALL = 1; const DEFAULT_TIMEOUT_MS = 30_000; const DEFAULT_MAX_BODY_SIZE = '1mb'; @@ -47,8 +109,19 @@ export function createGatewayRouter(deps: GatewayDeps): Router { return; } - const keyRecord = apiKeys.get(apiKeyHeader); - if (!keyRecord || keyRecord.apiId !== req.params.apiId) { + // Use prefix-based candidate lookup + constant-time hash comparison so + // that a prefix match with a wrong hash produces a clean 401 instead of + // propagating an unhandled exception as a 500. Both 'not_found' and + // 'hash_mismatch' intentionally map to the same generic 401 message so + // clients cannot distinguish the two cases (no timing oracle). + const resolved = resolveApiKey(apiKeyHeader, apiKeys); + if ('error' in resolved) { + next(new UnauthorizedError('Unauthorized: invalid API key')); + return; + } + + const keyRecord = resolved.record; + if (keyRecord.apiId !== req.params.apiId) { next(new UnauthorizedError('Unauthorized: invalid API key')); return; } diff --git a/src/services/revenueSettlementService.ts b/src/services/revenueSettlementService.ts index 725ef6f..66d4d0c 100644 --- a/src/services/revenueSettlementService.ts +++ b/src/services/revenueSettlementService.ts @@ -18,10 +18,19 @@ export interface RevenueSettlementOptions { horizonUrl?: string; fetchImpl?: typeof fetch; horizonRequestTimeoutMs?: number; + /** Maximum retry attempts for transient Horizon errors (default: 3). */ horizonMaxRetries?: number; + /** Base delay in ms for Horizon retry backoff (default: 500). */ horizonRetryBaseDelayMs?: number; } +export interface ReconcileResult { + checked: number; + completed: number; + failed: number; + errors: number; +} + interface HorizonTransactionResponse { successful?: boolean; } @@ -168,6 +177,101 @@ export class RevenueSettlementService { return { processed, settledAmount, errors }; } + /** + * Reconcile all pending settlements by checking their transaction status on Horizon. + * - Marks confirmed transactions as 'completed'. + * - Marks 404/unsuccessful transactions as 'failed'. + * - Leaves pending entries unchanged when Horizon returns a transient error (retries with backoff). + * Returns a summary of the reconciliation pass. + */ + async reconcilePendingSettlements(): Promise { + const previousReconcile = this.reconcileTail.catch(() => undefined); + let releaseReconcile!: () => void; + this.reconcileTail = new Promise((resolve) => { + releaseReconcile = resolve; + }); + + await previousReconcile; + + try { + return await this.reconcileOnce(); + } finally { + releaseReconcile(); + } + } + + private async reconcileOnce(): Promise { + const pendingSettlements = await this.settlementStore.listPending?.() ?? []; + + const result: ReconcileResult = { checked: 0, completed: 0, failed: 0, errors: 0 }; + + if (pendingSettlements.length === 0) return result; + + const horizonBase = (this.options.horizonUrl ?? 'https://horizon-testnet.stellar.org/').replace(/\/$/, ''); + const fetchFn = this.options.fetchImpl ?? fetch; + const timeoutMs = this.options.horizonRequestTimeoutMs ?? 10_000; + const maxRetries = this.options.horizonMaxRetries ?? 3; + const retryBaseDelayMs = this.options.horizonRetryBaseDelayMs ?? 500; + + for (const settlement of pendingSettlements) { + if (!settlement.tx_hash) continue; + + result.checked++; + + try { + const horizonResult = await withRetry( + async () => { + const url = `${horizonBase}/transactions/${encodeURIComponent(settlement.tx_hash!)}`; + const res = await fetchFn(url, { + signal: AbortSignal.timeout(timeoutMs), + }); + + // 404 → transaction not found → treat as failed (not retriable) + if (res.status === 404) { + return { found: false, successful: false } as const; + } + + // Transient server errors → throw so withRetry can back off + if (RETRIABLE_HTTP_STATUSES.has(res.status)) { + throw new TransientError(`Horizon returned ${res.status}`); + } + + const body = await res.json() as HorizonTransactionResponse; + return { found: true, successful: body.successful === true } as const; + }, + { + maxAttempts: maxRetries + 1, + baseDelayMs: retryBaseDelayMs, + shouldRetry: isTransientNetworkError, + }, + ); + + if (horizonResult.successful) { + await this.settlementStore.updateStatus(settlement.id, 'completed', settlement.tx_hash); + const now = new Date().toISOString(); + // Best-effort: set completed_at if the store supports it + const store = this.settlementStore as SettlementStore & { + setCompletedAt?: (id: string, completedAt: string) => void; + }; + store.setCompletedAt?.(settlement.id, now); + result.completed++; + } else { + await this.settlementStore.updateStatus(settlement.id, 'failed', undefined); + result.failed++; + } + } catch (error) { + // Transient errors exhausted retries — leave status as pending for next run + result.errors++; + console.error( + `[reconcile] Failed to check Horizon status for settlement ${settlement.id}:`, + this.getErrorMessage(error), + ); + } + } + + return result; + } + private async recordFailedSettlement( settlementId: string, developerId: string, diff --git a/src/services/settlementStore.ts b/src/services/settlementStore.ts index b4dcc90..d8e59e7 100644 --- a/src/services/settlementStore.ts +++ b/src/services/settlementStore.ts @@ -33,6 +33,17 @@ export class InMemorySettlementStore implements SettlementStore { .sort((a, b) => new Date(a.created_at).getTime() - new Date(b.created_at).getTime()); } + listPending(): Settlement[] { + return this.getPendingSettlements(); + } + + setCompletedAt(id: string, completedAt: string): void { + const s = this.settlements.find((s) => s.id === id); + if (s) { + s.completed_at = completedAt; + } + } + /** Helper for tests */ clear(): void { this.settlements = []; diff --git a/src/types/developer.ts b/src/types/developer.ts index fb0fd6f..b8dea36 100644 --- a/src/types/developer.ts +++ b/src/types/developer.ts @@ -57,4 +57,5 @@ export interface SettlementStore { create(settlement: Settlement): Awaitable; updateStatus(id: string, status: Settlement['status'], txHash?: string | null): Awaitable; getDeveloperSettlements(developerId: string): Awaitable; + listPending?(): Awaitable; }