diff --git a/CHANGELOG.md b/CHANGELOG.md index d6f2c22..4d63473 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- **Idempotency-Key support on POST routes** (P3-G). + Clients may send an `Idempotency-Key: ` + header on any POST under `/v1/*`. The first response (status + + JSON body) is cached in the new `dbo.IdempotencyKey` table for + 24h, keyed by `sha256(authKey:method:path)` + the raw key value. + Retries with the same body replay the cached response and carry + an `Idempotency-Replay: true` response header; retries with a + DIFFERENT body return `409 { code: "idempotency_key_reused" }`. + No-op for POSTs without the header — legacy clients are unaffected. - **Sequelize associations across the full entity graph** (PR #54). Every FK now has a `hasMany`/`belongsTo` pair in `db.config.js`, enabling `include`-based eager loading and the auto-generated diff --git a/app/middleware/idempotency.js b/app/middleware/idempotency.js new file mode 100644 index 0000000..c7b8e63 --- /dev/null +++ b/app/middleware/idempotency.js @@ -0,0 +1,214 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Aaron K. Clark +"use strict"; + +/** + * Stripe-style Idempotency-Key support for POST endpoints. + * + * Why + * POSTs that create resources (TimeEntry, Customer, Invoice, etc.) + * are unsafe to retry blindly — a network blip during the client's + * read of the response makes the retry indistinguishable from a + * first attempt, and the server creates a duplicate row. + * + * A client that picks an `Idempotency-Key` header on the original + * request can replay the exact same call freely: this middleware + * stores the first response for 24h and replays it for any matching + * retry within that window. If the client sends the SAME key with + * a DIFFERENT body, we return 409 to flag the misuse. + * + * Scope + * The cache key is sha256(authKey || ':' || method || path). Two + * different operators (or two different routes) cannot collide + * even if they pick the same Idempotency-Key string. We index the + * body separately so we can tell "same retry" from "same key, + * different intent". + * + * Cleanup + * Each request opportunistically prunes rows past `ikExpiresAt`. + * No background sweeper job needed — at typical write rates the + * table stays small. Tradeoff: a quiet period leaves a few expired + * rows around; ignored on the next write. + */ + +const crypto = require('crypto'); +const db = require('../config/db.config.js'); +const log = require('../config/logger.js'); + +const TTL_MS = 24 * 60 * 60 * 1000; // 24 hours +// Keys are client-picked; reject anything that looks like garbage. +// Stripe accepts up to 255 chars; we mirror that and require +// printable ASCII to avoid `\0` injection into the SQL replacement +// (Sequelize parameterizes, but defense in depth). +const KEY_PATTERN = /^[\x21-\x7e]{1,255}$/; + +function sha256(s) { + return crypto.createHash('sha256').update(String(s)).digest('hex'); +} + +/** + * Stable JSON serializer so that two semantically-identical bodies + * (e.g., `{a:1,b:2}` vs `{b:2,a:1}`) hash to the same value. Without + * this, a client that reorders its JSON fields on retry would trip + * the body-mismatch 409. + */ +function canonicalJson(value) { + if (value === null || typeof value !== 'object') { + return JSON.stringify(value); + } + if (Array.isArray(value)) { + return '[' + value.map(canonicalJson).join(',') + ']'; + } + const keys = Object.keys(value).sort(); + return '{' + keys.map(k => JSON.stringify(k) + ':' + canonicalJson(value[k])).join(',') + '}'; +} + +function hashBody(body) { + // null body (no req.body, e.g., a POST with no JSON) collapses + // to the literal string "null" so two no-body retries still + // match each other. + return sha256(canonicalJson(body == null ? null : body)); +} + +function buildScope(req) { + // attachAuth runs upstream so req.authKey is populated when the + // header was supplied. For requests where attachAuth hasn't run + // we fall back to the raw header — gives a stable hash key but + // not a security boundary (the upstream auth check is the + // boundary; idempotency just dedups). + const authKey = (req && (req.authKey || (req.get && req.get('authKey')))) || ''; + return sha256(authKey + ':' + req.method + ':' + (req.path || '')); +} + +async function pruneExpired(sequelize) { + try { + await sequelize.query( + 'DELETE FROM "dbo"."IdempotencyKey" WHERE "ikExpiresAt" < now()', + ); + } catch (error) { + // Pruning is best-effort. Log and continue. + log.warn({ err: error }, 'IdempotencyKey: prune failed'); + } +} + +/** + * Express middleware. Mount on routes that should support idempotent + * retries. If the request lacks an `Idempotency-Key` header the + * middleware is a no-op (passes through to the handler). + * + * Behavior on header present: + * - First time we've seen this (scope, key): proceed to handler, + * then write the response to the cache before returning. + * - Same (scope, key), same body hash: replay the cached response. + * - Same (scope, key), DIFFERENT body hash: 409 Conflict with a + * stable `{message, code: "idempotency_key_reused"}` body. + * - Storage failure: log + proceed (the dedup is best-effort; we + * never want it to break a write that would otherwise succeed). + */ +async function idempotency(req, res, next) { + const rawKey = req.get && req.get('Idempotency-Key'); + if (!rawKey) return next(); + if (!KEY_PATTERN.test(rawKey)) { + return res.status(400).json({ + message: 'Invalid Idempotency-Key header — must be 1-255 printable ASCII chars.', + }); + } + + if (!db.sequelize || typeof db.sequelize.query !== 'function') { + // Test env or misconfiguration. Don't block writes. + return next(); + } + + const scope = buildScope(req); + const bodyHash = hashBody(req.body); + + // Best-effort prune. Awaited so we don't pile up overlapping + // DELETEs under load; cheap because the index covers it. + pruneExpired(db.sequelize).catch(() => {}); + + let existing; + try { + const rows = await db.sequelize.query( + `SELECT "ikRequestHash" AS "requestHash", + "ikResponseStatus" AS "status", + "ikResponseBody" AS "body" + FROM "dbo"."IdempotencyKey" + WHERE "ikScope" = :scope AND "ikKey" = :key + AND "ikExpiresAt" >= now()`, + { + replacements: { scope, key: rawKey }, + type: db.Sequelize.QueryTypes.SELECT, + }, + ); + existing = rows && rows[0]; + } catch (error) { + log.warn({ err: error }, 'IdempotencyKey: lookup failed, proceeding without dedup'); + return next(); + } + + if (existing) { + if (existing.requestHash !== bodyHash) { + return res.status(409).json({ + message: 'Idempotency-Key was reused with a different request body.', + code: 'idempotency_key_reused', + }); + } + // Replay the cached response verbatim. Set a header so + // clients can tell a replay apart from a fresh write — useful + // for observability and for client-side write counters. + res.setHeader('Idempotency-Replay', 'true'); + return res.status(existing.status).json(existing.body); + } + + // First time seeing this key. Intercept the handler's response + // so we can persist it BEFORE the bytes flush to the client. We + // wrap res.json (the controllers' uniform exit) and store there. + const originalJson = res.json.bind(res); + res.json = function patchedJson(body) { + // Statuscode could have been set via res.status() prior to + // .json(). Default to 200 if nothing explicit. + const status = res.statusCode || 200; + // Only persist successful or client-error writes. 5xx + // responses indicate the request never succeeded and we + // want the retry to actually re-run. + if (status >= 200 && status < 500) { + const expiresAt = new Date(Date.now() + TTL_MS); + // Fire and forget — the response shouldn't block on the + // cache write. If the INSERT loses a race with a + // concurrent retry the UNIQUE constraint catches it. + db.sequelize.query( + `INSERT INTO "dbo"."IdempotencyKey" + ("ikScope", "ikKey", "ikRequestHash", + "ikResponseStatus", "ikResponseBody", "ikExpiresAt") + VALUES (:scope, :key, :requestHash, + :status, :body::jsonb, :expiresAt) + ON CONFLICT ("ikScope", "ikKey") DO NOTHING`, + { + replacements: { + scope, + key: rawKey, + requestHash: bodyHash, + status, + body: JSON.stringify(body), + expiresAt, + }, + }, + ).catch((error) => { + log.warn({ err: error }, 'IdempotencyKey: store failed'); + }); + } + return originalJson(body); + }; + + return next(); +} + +module.exports = { + idempotency, + // Exported for unit tests: + canonicalJson, + hashBody, + buildScope, + KEY_PATTERN, + TTL_MS, +}; diff --git a/app/migrations/20260519000000-idempotency-keys.js b/app/migrations/20260519000000-idempotency-keys.js new file mode 100644 index 0000000..fa41a4a --- /dev/null +++ b/app/migrations/20260519000000-idempotency-keys.js @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Aaron K. Clark +// +// Adds the IdempotencyKey table for Stripe-style idempotent POSTs. +// +// Background (audit issue #73, P3-G): +// POST endpoints that create resources (TimeEntry, Customer, Job, +// Invoice, payment rows, etc.) can be inadvertently double-submitted +// when a network blip causes a client to retry after the server has +// already accepted the original. Without server-side dedup the +// retry creates a second row. +// +// The middleware in app/middleware/idempotency.js stores the +// response body + status of the FIRST request keyed by +// (authKey hash, method+path hash, Idempotency-Key header). A +// retry with the same key replays the stored response. A retry +// with a DIFFERENT body but the same key returns 409 to flag the +// collision. +// +// Schema: +// ikId bigserial PK +// ikScope text — sha256(authKey || ':' || method || path) +// ikKey text — the raw Idempotency-Key header value +// ikRequestHash text — sha256 of the canonical JSON request body +// ikResponseStatus int +// ikResponseBody jsonb +// ikCreatedAt timestamptz default now() +// ikExpiresAt timestamptz — createdAt + 24h +// +// UNIQUE (ikScope, ikKey) — within a single (auth, route) the +// client picks the key, so the constraint catches both reused-key +// replays and concurrent first-writes via INSERT ... ON CONFLICT. +// +// Cleanup: rows past ikExpiresAt are pruned best-effort by the +// middleware on each request (cheap DELETE WHERE expired). No +// background job needed; a tiny burst of expired rows is fine. + +'use strict'; + +module.exports = { + async up(queryInterface, Sequelize) { + const SCHEMA = 'dbo'; + const sequelize = queryInterface.sequelize; + + await sequelize.query(` + CREATE TABLE IF NOT EXISTS "${SCHEMA}"."IdempotencyKey" ( + "ikId" BIGSERIAL PRIMARY KEY, + "ikScope" TEXT NOT NULL, + "ikKey" TEXT NOT NULL, + "ikRequestHash" TEXT NOT NULL, + "ikResponseStatus" INTEGER NOT NULL, + "ikResponseBody" JSONB NOT NULL, + "ikCreatedAt" TIMESTAMPTZ NOT NULL DEFAULT now(), + "ikExpiresAt" TIMESTAMPTZ NOT NULL + ); + `); + + await sequelize.query(` + CREATE UNIQUE INDEX IF NOT EXISTS "IdempotencyKey_scope_key_uidx" + ON "${SCHEMA}"."IdempotencyKey" ("ikScope", "ikKey"); + `); + // Cheap range scan for the cleanup pass. + await sequelize.query(` + CREATE INDEX IF NOT EXISTS "IdempotencyKey_expires_idx" + ON "${SCHEMA}"."IdempotencyKey" ("ikExpiresAt"); + `); + }, + + async down(queryInterface, Sequelize) { + const SCHEMA = 'dbo'; + const sequelize = queryInterface.sequelize; + await sequelize.query(`DROP TABLE IF EXISTS "${SCHEMA}"."IdempotencyKey"`); + }, +}; diff --git a/app/routers/router.js b/app/routers/router.js index 7b12739..62b5f9f 100644 --- a/app/routers/router.js +++ b/app/routers/router.js @@ -5,6 +5,7 @@ const router = express.Router(); const swaggerUi = require('swagger-ui-express'); const { attachAuth } = require('../middleware/auth.js'); +const { idempotency } = require('../middleware/idempotency.js'); const customer = require('../controllers/customercontroller.js'); const health = require('../controllers/healthcontroller.js'); @@ -57,6 +58,17 @@ router.get('/healthz', health.healthz); // adoption in follow-up PRs). router.use('/v1', attachAuth); +// Idempotency-Key support for POSTs. The middleware is a no-op +// for non-POST methods and for POSTs that don't send the header; +// it only kicks in when a client opts in by setting the header. We +// mount it AFTER attachAuth so the cache scope can be hashed +// together with the calling auth key (avoids cross-tenant collisions +// for clients that happen to pick the same Idempotency-Key value). +router.use('/v1', (req, res, next) => { + if (req.method !== 'POST') return next(); + return idempotency(req, res, next); +}); + // Identity probe — returns what the calling authKey resolves to. // Distinguishes "header missing" (403) from "header present but // unknown" (200 with authenticated:false) so a strict guard diff --git a/tests/api/idempotency.test.js b/tests/api/idempotency.test.js new file mode 100644 index 0000000..dfc1fe1 --- /dev/null +++ b/tests/api/idempotency.test.js @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Aaron K. Clark +// +// HTTP-level smoke tests for the idempotency middleware mount. +// +// The DB is mocked out; the goal here is to verify that: +// (a) requests WITHOUT an Idempotency-Key header pass through +// cleanly — the middleware must not break legacy clients. +// (b) requests WITH a malformed Idempotency-Key are rejected with +// 400 before any handler runs. +// (c) the middleware is only applied to POSTs — GET should never +// hit the rejection path even if the header is present. +// +// Full first-write-then-replay coverage requires a real DB and lives +// in the integration suite (gated behind P5-M). + +import { describe, test, expect, vi, beforeAll } from 'vitest'; +import request from 'supertest'; +import express from 'express'; + +// Stub the DB so the middleware's `db.sequelize.query` path doesn't +// blow up on an unreachable Postgres. With no `.query` Function we +// would hit the "no sequelize handle" early-out; we provide one that +// resolves to no rows so the lookup branch decides "not cached" and +// we continue to the handler. +vi.mock('../../app/config/db.config.js', () => ({ + sequelize: { + query: vi.fn().mockResolvedValue([]), + QueryTypes: { SELECT: 'SELECT' }, + }, + Sequelize: { QueryTypes: { SELECT: 'SELECT' } }, + Customer: { + create: vi.fn().mockResolvedValue({ custId: 1 }), + findByPk: vi.fn().mockResolvedValue(null), + findAll: vi.fn().mockResolvedValue([]), + findAndCountAll: vi.fn().mockResolvedValue({ count: 0, rows: [] }), + }, + ApiKey: {}, + ApiMaster: {}, +})); + +let app; + +beforeAll(async () => { + const router = (await import('../../app/routers/router.js')).default + || require('../../app/routers/router.js'); + app = express(); + app.use(express.json()); + app.use('/', router); +}); + +describe('Idempotency middleware: mounted on POST routes', () => { + test('POST without Idempotency-Key passes through unchanged', async () => { + // No header, no special handling. Whatever the controller + // returns (400/403/etc — depends on the inline auth check) + // is fine; the idempotency layer must not insert itself. + const res = await request(app) + .post('/v1/customer') + .set('authKey', 'any') + .send({ custCompanyName: 'Test' }); + expect(res.status).not.toBe(409); + // The middleware-injected reject path has a specific code; + // confirm it's not present. + expect(res.body && res.body.code).not.toBe('idempotency_key_reused'); + }); + + test('POST with whitespace in Idempotency-Key returns 400', async () => { + const res = await request(app) + .post('/v1/customer') + .set('authKey', 'any') + .set('Idempotency-Key', 'has space') + .send({}); + expect(res.status).toBe(400); + }); + + test('POST with a valid Idempotency-Key reaches the handler (no cache hit on empty mock)', async () => { + const res = await request(app) + .post('/v1/customer') + .set('authKey', 'any') + .set('Idempotency-Key', '01HFTESTKEY12345') + .send({ custCompanyName: 'Acme' }); + // Whatever the controller decides (likely 403 from inline + // auth, since the mock returns []), the idempotency layer + // should NOT have short-circuited with 400/409. That's the + // signal the middleware ran and decided "no cache hit". + expect(res.status).not.toBe(400); + expect(res.status).not.toBe(409); + }); + + test('GET requests are never gated by the middleware', async () => { + // Even with a malformed header, a GET should sail through — + // the middleware is wrapped in a method check that no-ops + // for non-POST. This is the regression pin: we never want + // to break a legacy GET because someone forgot to strip + // the header on a non-POST retry. + const res = await request(app) + .get('/v1/customer/bycompany/1?limit=1') + .set('authKey', 'any') + .set('Idempotency-Key', ''); + expect(res.status).not.toBe(400); + }); +}); diff --git a/tests/unit/idempotency.test.js b/tests/unit/idempotency.test.js new file mode 100644 index 0000000..73a050d Binary files /dev/null and b/tests/unit/idempotency.test.js differ