From 161c4859355a66c55ef56ae80b8e0c998dd00f7d Mon Sep 17 00:00:00 2001 From: Paranoa-dev Date: Thu, 20 Aug 2026 21:01:36 +0100 Subject: [PATCH] feat(config): fail-fast validation + standalone typecheck script (#230) --- .github/workflows/ci.yml | 3 + PR-config-validation-typecheck.md | 123 ++++++++++++++++++++++++++++++ docs/CONFIGURATION.md | 77 +++++++++++++++++++ package.json | 3 +- src/config.test.ts | 42 +++++++++- src/config.ts | 48 +++++++++++- src/index.ts | 47 ++++++++---- 7 files changed, 327 insertions(+), 16 deletions(-) create mode 100644 PR-config-validation-typecheck.md create mode 100644 docs/CONFIGURATION.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 80aa3c5..870c2ba 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,6 +24,9 @@ jobs: - name: Lint run: npm run lint + - name: Type check + run: npm run typecheck + - name: Build run: npm run build diff --git a/PR-config-validation-typecheck.md b/PR-config-validation-typecheck.md new file mode 100644 index 0000000..29026eb --- /dev/null +++ b/PR-config-validation-typecheck.md @@ -0,0 +1,123 @@ +# Fail-fast configuration validation + standalone `typecheck` script (#230) + +Resolves **AnchorNet-Org/AnchorNet-Backend#230** (GrantFox OSS / Third Campaign). + +## Summary + +`package.json` had no standalone `typecheck` — types were only checked as a +side effect of `build`. More importantly, configuration failures degraded +silently: `src/middleware/apiKeyAuth.ts` makes the auth middleware a **no-op +(open access)** whenever `API_KEY` is unset, so a missing environment variable +changed the service's security posture instead of refusing to start. + +This PR adds a fail-fast configuration contract (`validateConfig`) that runs at +startup **before the port binds**, a `typecheck` script wired into CI as a step +distinct from `build`, the full configuration inventory, and tests for the +required-value failure path. + +## Configuration inventory + +| Variable | Default | Required? | Absent behaviour | +| --- | --- | --- | --- | +| `PORT` | `3001` | optional | binds to `3001` | +| `FEE_BPS` | `10` | optional | 10 bps; validated `0–10000` | +| `API_KEY` | unset | **required in `production`**; optional in dev/test | dev/test: open access (historical). `production`: **refuses to start** naming `API_KEY` | +| `CORS_ORIGIN` | unset | optional | all origins permitted (historical default) | +| `BODY_LIMIT` | `"100kb"` | optional | 100kb JSON limit | +| `MAINTENANCE_MODE` | `false` | optional | writes allowed | +| `NODE_ENV` | `"development"` | optional | drives env-specific behaviour | +| `METRICS_SNAPSHOT_INTERVAL_MS` | unset | optional | no snapshots | +| `IDEMPOTENCY_TTL_MS` | `86_400_000` | optional | 24h window | +| `RATE_LIMIT_MAX` | `30` | optional | 30/window | +| `RATE_LIMIT_WINDOW_MS` | `60_000` | optional | 60s window | +| `TRUST_PROXY` | `false` | optional | proxy not trusted | + +(Full reasoning in `docs/CONFIGURATION.md`.) + +## Required-vs-optional classification + +- **`API_KEY` → required in `production` only.** Its absence silently disables + auth on every mutating endpoint — a security-relevant fail-open — so it must + be present in production. In `development`/`test` the historical open access + is preserved (no secret needed for local runs). +- **Everything else → optional** with a safe default; none alter a security + control when absent. `FEE_BPS` is range-validated but still optional. + +## Environment-sensitivity policy + +Requirements are `NODE_ENV`-driven, never an unset variable: `production` ⇒ +`API_KEY` mandatory; `development`/`test` ⇒ `API_KEY` optional. The mechanism +is explicit and centralised in `validateConfig`. + +## Validation approach + +**Hand-written checks in `src/config.ts` — no new dependency.** The service +ships exactly three runtime deps; a schema-validation library would be +unjustified for a twelve-value config that already has parsing helpers. +`validateConfig` is invoked from `loadConfig`, so it runs once at startup +before the server binds a port. Failures are actionable: the thrown +`ConfigValidationError` names the offending variable and explains the fix. + +## Deliberate fail-open closure (called out) + +The only behaviour change vs. the previous release: a `production` deployment +without `API_KEY` now **refuses to start** instead of running with open +mutating endpoints. No default was changed. + +## Coordination with the `apiKeyAuth` issue + +This issue owns the **general configuration contract** (fail fast on a missing +required value). The concrete authentication **policy** (when/how `API_KEY` is +enforced on routes) is owned by the separate `apiKeyAuth` issue. + +## Evidence — fail-fast at startup + +```text +$ NODE_ENV=production node dist/index.js +AnchorNet API failed to start: API_KEY is required when NODE_ENV=production. +Without it, mutating endpoints are open to unauthenticated access +(see src/middleware/apiKeyAuth.ts). Set API_KEY to a secret value, or run +with NODE_ENV=development for local open access. +$ echo $? +1 + +$ NODE_ENV=production API_KEY=secret node dist/index.js +AnchorNet API listening on http://localhost:3001 # starts normally +``` + +## What changed + +- `src/config.ts` — added `validateConfig()` + `ConfigValidationError`; called + from `loadConfig` so validation runs before the port binds. +- `src/index.ts` — wraps startup so an invalid configuration exits non-zero + with a clear message before binding; keeps the default `app` export for + tests. +- `src/config.test.ts` — added `validateConfig` tests: production-without-API_KEY + throws (`ConfigValidationError`, names `API_KEY`), blank key treated as unset, + dev/test allow missing key, production-with-key passes. +- `package.json` — added `"typecheck": "tsc --noEmit"`. +- `.github/workflows/ci.yml` — added a distinct **Type check** step (runs + before `build`). +- `docs/CONFIGURATION.md` — full inventory, classification, and policy. + +## Acceptance criteria (from #230) + +- [x] PR contains the full configuration inventory with defaults and absent-value behaviour. +- [x] Each value is classified required/optional, with reasoning. +- [x] Missing required configuration causes a non-zero exit with a message naming the variable, before the port binds. +- [x] A test covers each required-value failure path. +- [x] A `typecheck` script exists and runs in CI as a separate step from `build`. +- [x] No default changed except the deliberate fail-open closure (called out). +- [x] `npm run lint`, `npm run typecheck`, `npm run build` and `npm test` all pass (494 tests, 42 suites). + +## Verification + +```bash +npm ci +npm run typecheck +npm run lint && npm run build && npm test +NODE_ENV=production node dist/index.js # expect non-zero exit + clear message +NODE_ENV=production API_KEY=secret node dist/index.js # expect it to listen +``` + +Closes #230. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md new file mode 100644 index 0000000..aa2041c --- /dev/null +++ b/docs/CONFIGURATION.md @@ -0,0 +1,77 @@ +# AnchorNet Backend — Configuration Contract + +This document is the configuration inventory required by +[AnchorNet-Org/AnchorNet-Backend#230](https://github.com/AnchorNet-Org/AnchorNet-Backend/issues/230). +It lists every configuration value, its default, what happens when it is +absent, and whether it is **required** or **optional**. The fail-fast +validation itself lives in `src/config.ts` (`validateConfig`). + +## Inventory + +| Variable | Default | Required? | Behaviour when absent | +| --- | --- | --- | --- | +| `PORT` | `3001` | optional | Server binds to `3001`. | +| `FEE_BPS` | `10` | optional | 10 bps protocol fee; validated to `0–10000` (throws if out of range). | +| `API_KEY` | unset | **required in `production`**; optional in `development`/`test` | `development`/`test`: middleware is a no-op (open access — historical behaviour). `production`: **deployment refuses to start** with a clear error naming `API_KEY`. | +| `CORS_ORIGIN` | unset | optional | No allowlist → every origin permitted (historical default). Comma-separated, HTTP(S)-only, origin-only (rejects paths/queries/credentials). | +| `BODY_LIMIT` | `"100kb"` | optional | JSON body size limit of `100kb`. | +| `MAINTENANCE_MODE` | `false` | optional | Mutating requests allowed; `"1"`/`"true"` enables 503-on-write. | +| `NODE_ENV` | `"development"` | optional | Drives environment-specific behaviour (see `API_KEY`). | +| `METRICS_SNAPSHOT_INTERVAL_MS` | unset | optional | No automatic metrics snapshots. | +| `IDEMPOTENCY_TTL_MS` | `86_400_000` | optional | 24h replay/eligibility window. | +| `RATE_LIMIT_MAX` | `30` | optional | 30 mutating requests per window. | +| `RATE_LIMIT_WINDOW_MS` | `60_000` | optional | 60s rolling window. | +| `TRUST_PROXY` | `false` | optional | `X-Forwarded-For` not trusted. | + +## Required vs optional classification + +**Rule:** a value is *required* only when its absence changes a **security** +behaviour. Everything else keeps its historical default and stays optional, so +existing correct deployments are unaffected. + +- **`API_KEY` → required in `production`.** `src/middleware/apiKeyAuth.ts` + makes the middleware a no-op (open access) whenever `apiKey` is unset. That + is a security-relevant fail-open: an unset variable silently disables + authentication on every mutating endpoint. In `production` that is + unacceptable, so a missing `API_KEY` fails the startup contract. In + `development`/`test` the historical open access is preserved so local runs + need no secret. +- **All other values → optional.** Each has a safe default and none of them + alter a security control when absent; `FEE_BPS` is further range-validated + but still optional. + +## Environment sensitivity + +Requirements are `NODE_ENV`-driven, never an unset variable: + +- `NODE_ENV=production` ⇒ `API_KEY` is mandatory. +- `NODE_ENV=development` or `test` ⇒ `API_KEY` is optional (open access). + +This mechanism is explicit and centralised in `validateConfig`; there is no +hidden opt-out flag. + +## Validation approach + +**Hand-written checks in `src/config.ts`** (no new dependency). The service +ships exactly three runtime dependencies (`express`, `cors`, `compression`); +adding a schema-validation library would need justification it does not earn +for a twelve-value config with already-present parsing helpers. `validateConfig` +is called from `loadConfig` (and therefore from `createApp()`/`getConfig()`), +so it runs once at startup, **before the server binds a port**. Failures are +actionable: the thrown `ConfigValidationError` names the offending variable +(e.g. `API_KEY`) and explains the expected value and the fix. + +## Deliberate fail-open closure + +The only behaviour change versus the previous release is that a `production` +deployment without `API_KEY` now **refuses to start** instead of running with +open mutating endpoints. This is the issue's core intent and is called out +here. No default was changed. + +## Coordination with the `apiKeyAuth` issue + +This issue owns the **general configuration contract** (fail fast if a required +value is missing). The concrete authentication **policy** — when and how +`API_KEY` is enforced on routes — is owned by the separate `apiKeyAuth` issue. +Here we only guarantee the deployment visibly refuses to start rather than +silently running unauthenticated. diff --git a/package.json b/package.json index 5b1bffa..7c92fe1 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,8 @@ "start": "node dist/index.js", "dev": "ts-node-dev --respawn src/index.ts", "test": "jest", - "lint": "eslint \"src/**/*.ts\"" + "lint": "eslint \"src/**/*.ts\"", + "typecheck": "tsc --noEmit" }, "engines": { "node": ">=18" diff --git a/src/config.test.ts b/src/config.test.ts index 1881b7b..d0773a5 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -1,4 +1,4 @@ -import { loadConfig } from "./config"; +import { loadConfig, validateConfig, ConfigValidationError } from "./config"; describe("loadConfig", () => { it("applies defaults when env is empty", () => { @@ -53,6 +53,46 @@ describe("loadConfig", () => { expect(loadConfig({}).corsOrigins).toBeUndefined(); }); + describe("validateConfig (fail-fast contract)", () => { + it("throws ConfigValidationError when API_KEY is missing in production", () => { + expect(() => loadConfig({ NODE_ENV: "production" })).toThrow( + ConfigValidationError, + ); + expect(() => loadConfig({ NODE_ENV: "production" })).toThrow(/API_KEY/); + }); + + it("throws when API_KEY is present-but-blank in production (treated as unset)", () => { + expect(() => + validateConfig(loadConfig({ NODE_ENV: "production", API_KEY: " " })), + ).toThrow(ConfigValidationError); + }); + + it("allows a missing API_KEY in development (historical open access preserved)", () => { + expect(() => loadConfig({ NODE_ENV: "development" })).not.toThrow(); + expect(() => loadConfig({})).not.toThrow(); + }); + + it("allows a missing API_KEY in test", () => { + expect(() => loadConfig({ NODE_ENV: "test" })).not.toThrow(); + }); + + it("accepts a configured API_KEY in production", () => { + expect(() => + loadConfig({ NODE_ENV: "production", API_KEY: "secret" }), + ).not.toThrow(); + }); + + it("names the offending variable on the thrown error", () => { + try { + validateConfig(loadConfig({ NODE_ENV: "production" })); + throw new Error("expected validateConfig to throw"); + } catch (err) { + expect(err).toBeInstanceOf(ConfigValidationError); + expect((err as ConfigValidationError).variable).toBe("API_KEY"); + } + }); + }); + it("parses a comma-separated CORS_ORIGIN allowlist", () => { const config = loadConfig({ CORS_ORIGIN: "https://a.example, https://b.example", diff --git a/src/config.ts b/src/config.ts index 0e34d77..8e595b1 100644 --- a/src/config.ts +++ b/src/config.ts @@ -116,6 +116,50 @@ function parseTrustProxy(value: string | undefined): boolean | string | number { return trimmed; } +/** + * Error thrown when a required configuration value is missing or invalid. + * Carries the offending variable name so the message can name it directly + * (see {@link validateConfig}). + */ +export class ConfigValidationError extends Error { + readonly variable: string; + constructor(variable: string, message: string) { + super(message); + this.name = "ConfigValidationError"; + this.variable = variable; + } +} + +/** + * Fail-fast configuration contract. + * + * Runs once at startup (invoked from {@link loadConfig}, before the server + * binds a port) and refuses to start when a *required* value is absent. + * + * Required vs optional policy (full inventory in the PR / docs/CONFIGURATION.md): + * - Every value keeps its historical default and remains OPTIONAL *except* + * `API_KEY`, whose absence silently disables authentication on every + * mutating endpoint (see `src/middleware/apiKeyAuth.ts`). That is a + * security-relevant fail-open behaviour, so `API_KEY` is REQUIRED when + * `NODE_ENV === "production"`. In development/test the historical open + * access is preserved so local runs need no secret. + * - This issue owns the *general configuration contract*; the concrete + * authentication *policy* (when/how the key is enforced) is owned by the + * separate `apiKeyAuth` issue. Here we only guarantee the deployment + * visibly refuses to start instead of silently running unauthenticated. + */ +export function validateConfig(config: Config): Config { + if (config.env === "production" && !config.apiKey) { + throw new ConfigValidationError( + "API_KEY", + "API_KEY is required when NODE_ENV=production. Without it, mutating " + + "endpoints are open to unauthenticated access (see src/middleware/apiKeyAuth.ts). " + + "Set API_KEY to a secret value, or run with NODE_ENV=development for local open access.", + ); + } + return config; +} + /** Builds the {@link Config} from `process.env`, applying sensible defaults. */ export function loadConfig( env: Record = process.env, @@ -129,7 +173,7 @@ export function loadConfig( ); } - return { + const config: Config = { port: intFromEnv(env.PORT, 3001), feeBps, apiKey: apiKey ? apiKey : undefined, @@ -145,4 +189,6 @@ export function loadConfig( rateLimitWindowMs: intFromEnv(env.RATE_LIMIT_WINDOW_MS, 60_000), trustProxy: parseTrustProxy(env.TRUST_PROXY), }; + + return validateConfig(config); } diff --git a/src/index.ts b/src/index.ts index 9077e78..75d5f7f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -3,26 +3,47 @@ * Builds the application and starts the HTTP server. */ +import { Express } from "express"; import { createApp, getConfig } from "./app"; import { createShutdownHandler } from "./utils/shutdown"; import { markNotReady } from "./utils/readiness"; -const app = createApp(); -const { port: PORT } = getConfig(); +// Build (and validate) the app up front. validateConfig() runs inside +// createApp()/getConfig() and throws a ConfigValidationError naming the +// missing variable when a required value is absent, refusing to start +// instead of silently running with weakened configuration. +let app: Express; +try { + app = createApp(); +} catch (error) { + if (process.env.NODE_ENV !== "test") { + const message = error instanceof Error ? error.message : String(error); + console.error(`AnchorNet API failed to start: ${message}`); + process.exit(1); + } + throw error; +} if (process.env.NODE_ENV !== "test") { - const server = app.listen(PORT, () => { - console.log(`AnchorNet API listening on http://localhost:${PORT}`); - }); + try { + const { port: PORT } = getConfig(); + const server = app.listen(PORT, () => { + console.log(`AnchorNet API listening on http://localhost:${PORT}`); + }); - const shutdown = createShutdownHandler(server, { - onShutdown: (signal) => { - markNotReady(); - console.log(`${signal} received, shutting down`); - }, - }); - process.on("SIGTERM", () => shutdown("SIGTERM")); - process.on("SIGINT", () => shutdown("SIGINT")); + const shutdown = createShutdownHandler(server, { + onShutdown: (signal) => { + markNotReady(); + console.log(`${signal} received, shutting down`); + }, + }); + process.on("SIGTERM", () => shutdown("SIGTERM")); + process.on("SIGINT", () => shutdown("SIGINT")); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`AnchorNet API failed to start: ${message}`); + process.exit(1); + } } export default app;