From fedaff9b43aad0f5b3aa2ba9f95aff2353fab19e Mon Sep 17 00:00:00 2001 From: Manas Srivastava Date: Wed, 13 May 2026 09:14:27 +0530 Subject: [PATCH] admin: rate-limit + audit emit + access-log prefix scrub (defense-in-depth layers 3-5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three new middleware layers on the admin route prefix: 1. AdminRateLimit — 30 req/min/fingerprint cap via Redis sorted-set sliding window. Excess returns 403 (NOT 429) with a body byte-for-byte identical to the RequireAdmin allowlist-miss 403, so an attacker who knows the secret prefix can't distinguish "throttled" from "not on the allowlist." Runs BEFORE RequireAdmin so invalid-JWT probes also burn slots. Fail-open on Redis errors. 2. AdminAuditEmit — after-response middleware writing one admin.access audit_log row per hit (success AND 403). Metadata carries email, ip, path_suffix, http_status, user_agent_brief, and a denied_by reason (rate_limit / allowlist_miss). path_suffix is the URL with the secret prefix stripped — the prefix MUST NOT land in audit_log. 3. LogScrubber — slog handler wrapper that replaces every occurrence of ADMIN_PATH_PREFIX with the literal "" in string attributes and message bodies. Wired in main() on the outside of logctx.NewHandler so the scrub runs LAST, after every context field is stamped. Empty prefix → pass-through (zero overhead). Chain order on the admin group: AdminRateLimit -> AdminAuditEmit -> RequireAdmin -> handler Audit sits BEFORE RequireAdmin because RequireAdmin returns 403 directly (no c.Next) on rejection — any middleware after it would never see the rejection path. Audit's internal c.Next() drives the rest of the chain and observes the final status either way. New audit kind: models.AuditKindAdminAccess = "admin.access". Tests: - log_scrubber_test.go (9 cases) — URL attr, message body, empty prefix passthrough, non-string attrs untouched, nested groups, JWT-pattern untouched regression, helper function. - admin_rate_limit_test.go (4 cases) — 31st req gets 403, byte-for-byte body parity with allowlist-miss, fail-open on Redis down, fingerprint independence. - admin_audit_test.go (5 cases) — success row, rate-limited 403 row with denied_by=rate_limit, allowlist-miss row with denied_by=allowlist_miss, path-suffix strip helper, metadata no-prefix invariant. make test-unit green across all packages. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/middleware/admin_audit.go | 391 +++++++++++++++++++ internal/middleware/admin_audit_test.go | 308 +++++++++++++++ internal/middleware/admin_rate_limit.go | 205 ++++++++++ internal/middleware/admin_rate_limit_test.go | 243 ++++++++++++ internal/middleware/log_scrubber.go | 243 ++++++++++++ internal/middleware/log_scrubber_test.go | 211 ++++++++++ internal/models/audit_kinds.go | 24 ++ internal/router/router.go | 32 +- main.go | 16 +- 9 files changed, 1671 insertions(+), 2 deletions(-) create mode 100644 internal/middleware/admin_audit.go create mode 100644 internal/middleware/admin_audit_test.go create mode 100644 internal/middleware/admin_rate_limit.go create mode 100644 internal/middleware/admin_rate_limit_test.go create mode 100644 internal/middleware/log_scrubber.go create mode 100644 internal/middleware/log_scrubber_test.go diff --git a/internal/middleware/admin_audit.go b/internal/middleware/admin_audit.go new file mode 100644 index 00000000..9e49c9ff --- /dev/null +++ b/internal/middleware/admin_audit.go @@ -0,0 +1,391 @@ +package middleware + +// admin_audit.go — after-response middleware that writes a structured +// `admin.access` audit_log row for every hit on an admin route, regardless +// of whether the request succeeded or was rejected. +// +// This is the FOURTH defense-in-depth layer (third gate is rate-limit, +// second is allowlist, first is path prefix): observability. +// +// - On a successful admin call (200/201/...), we get a forensic record +// of who hit what and when. +// - On a 403 from the rate-limiter OR the allowlist check, we get the +// same record — so brute-force probing is loudly visible in the audit +// log even though the response body claims "not an admin." The +// operator can grep `kind = 'admin.access' AND http_status = 403` to +// find probing patterns by IP / UA in minutes. +// +// Path storage policy: we store the URL SUFFIX (e.g. "customers/:team_id/ +// tier"), never the full path. The ADMIN_PATH_PREFIX is a secret with +// the same blast radius as a session token — writing it into audit_log +// rows would defeat the whole point of the prefix gate (any DB-read +// access would expose the secret to a future engineer / BI consumer). +// The suffix is built by stripping a known leading "/api/v1//" +// before persistence; if the strip fails (defensive: shouldn't happen in +// production) we substitute the literal sentinel "" rather than +// leak the full path. +// +// User-agent storage policy: capped at 120 chars and run through the +// admin-prefix scrubber. UAs can carry hand-crafted strings that an +// attacker uses to fingerprint their own session — capping the field +// prevents log-injection-style abuse, and scrubbing prevents the prefix +// from leaking if someone accidentally puts a URL in their UA. + +import ( + "context" + "database/sql" + "encoding/json" + "log/slog" + "strings" + + "github.com/gofiber/fiber/v2" + "github.com/google/uuid" + "instant.dev/internal/models" +) + +const ( + // adminUAMaxLen caps how much of the user-agent string we persist. + // Long enough to identify a real client ("Mozilla/5.0 (Macintosh; Intel + // Mac OS X 10_15_7) AppleWebKit/..."), short enough that a malicious + // 4KB UA can't bloat audit_log rows or grief log-shipper budgets. + adminUAMaxLen = 120 + + // adminAuditDeniedReasonRateLimit / adminAuditDeniedReasonAllowlistMiss + // are the values written into the `denied_by` metadata field when a + // 403 is recorded. Lets a BI consumer split brute-force probes + // (rate_limit) from "real user not on allowlist" (allowlist_miss). + // Persisted INTERNALLY in audit_log metadata — never echoed in HTTP + // responses, so the probe-vs-not-admin response shape stays identical + // on the wire. + adminAuditDeniedReasonRateLimit = "rate_limit" + adminAuditDeniedReasonAllowlistMiss = "allowlist_miss" + adminAuditDeniedReasonNone = "" // success +) + +// AdminAuditMetadata is the typed shape of the audit_log.metadata blob +// written by AdminAuditEmit. Promoted to a named struct so the audit +// schema is a typed contract — a future BI consumer reads this in one +// place, not by guessing at map shapes. +// +// IMPORTANT: PathSuffix MUST NOT contain the ADMIN_PATH_PREFIX. The +// audit middleware strips it before populating this field. The test +// suite grep-asserts this invariant against the persisted blob. +type AdminAuditMetadata struct { + // Email is the JWT email of the caller, lowercased. Empty string when + // the caller had no JWT (e.g. probe with no Authorization header that + // got 403'd by RequireAdmin). Operator-relevant: an empty email on a + // 403 means "fully anonymous probe;" a populated email on a 403 means + // "a real signed-in user is probing — investigate." + Email string `json:"email"` + + // IP is the source IP as resolved by the fingerprint middleware. + // Same source as the rate-limit key — lets the operator pivot + // audit_log rows to rate-limit metrics. + IP string `json:"ip"` + + // PathSuffix is the URL path with the secret prefix stripped, e.g. + // "customers/:team_id/tier". Persisting the raw matched path + // (.Params(), if known) would be ideal but Fiber's path-template is + // not directly readable post-match — we use the raw URL path and rely + // on the strip to remove the prefix. The remaining suffix is generic + // (no UUIDs interpolated) because team_id values come from the URL. + // For sortability + grouping, downstream BI can normalize UUID + // segments to ":id" with a simple regex. + PathSuffix string `json:"path_suffix"` + + // HTTPStatus is the response code that the handler / middleware + // returned to the caller. Drives the "did this hit succeed" pivot. + HTTPStatus int `json:"http_status"` + + // UserAgentBrief is the first 120 chars of the User-Agent header, + // scrubbed of the admin prefix (paranoia: the prefix should NEVER + // appear in a UA, but if a hand-crafted client puts a URL in its UA + // we'd otherwise persist it). Never trusted as identification — UAs + // are client-supplied. Forensic value only. + UserAgentBrief string `json:"user_agent_brief"` + + // DeniedBy explains the 403 cause. Empty on success. Internal only — + // never echoed in HTTP responses (would leak probe-vs-not-admin). + // One of: "", "rate_limit", "allowlist_miss". + DeniedBy string `json:"denied_by,omitempty"` +} + +// AdminAuditEmit returns a Fiber middleware that fires AFTER the rest of +// the admin chain (response written, status known) and writes one +// `admin.access` audit row capturing the request shape. +// +// adminPathPrefix is the unguessable secret (cfg.AdminPathPrefix). It MUST +// match what's mounted in router.go — the middleware uses it to strip the +// prefix out of the persisted path. An empty prefix is invalid for this +// middleware (the admin routes wouldn't even register); for safety we +// degrade to a no-op rather than panic. +// +// db may be nil only in tests. The middleware skips the insert in that +// case so a partial-app test rig isn't forced to wire a real DB connection. +func AdminAuditEmit(db *sql.DB, adminPathPrefix string) fiber.Handler { + if adminPathPrefix == "" { + // Admin routes wouldn't even register without a prefix. If this + // middleware is wired without one, we'd otherwise leak the full + // path into audit rows — pass through is safer than guess. + return func(c *fiber.Ctx) error { return c.Next() } + } + return func(c *fiber.Ctx) error { + // Run the rest of the chain first so we capture the final status. + // We can't use OnResponse because the handler may set the status + // directly; the err return path also matters for fiber's + // ErrResponseWritten contract. + err := c.Next() + + // Always emit — success AND 403. The probe-visibility argument + // is the whole point. Errors that bubble up to fiber's + // ErrorHandler still surface a status code; ErrResponseWritten + // is the canonical "handler wrote the response itself" sentinel. + status := c.Response().StatusCode() + meta := buildAdminAuditMetadata(c, adminPathPrefix, status) + + // Resolve team_id: prefer the URL :team_id param (the admin + // endpoints target a specific team), fall back to the caller's + // own team. audit_log.team_id is FK-constrained to teams(id) and + // NOT NULL — when neither source resolves we cannot write to + // audit_log without violating the constraint, so the row is + // skipped (the slog warn lands instead so brute-force probes + // without a team context are still operator-visible via log + // search). + teamID := adminAuditTeamID(c) + + // If db is nil (test path) OR team_id is unresolvable, short- + // circuit the DB write. We still computed the metadata so a test + // can intercept via locals if needed. + if db != nil && teamID != uuid.Nil { + payload, _ := json.Marshal(meta) + summary := adminAuditSummary(meta) + // Fire-and-forget: an audit write failure must never block the + // admin request. We swallow the error after logging — matches + // the contract documented on models.InsertAuditEvent. + if ierr := models.InsertAuditEvent(c.Context(), db, models.AuditEvent{ + TeamID: teamID, + Actor: "admin", + Kind: models.AuditKindAdminAccess, + Summary: summary, + Metadata: payload, + }); ierr != nil { + slog.Error("admin_audit.insert_failed", + "error", ierr, + "team_id", teamID, + "http_status", status, + ) + } + } else if db != nil && teamID == uuid.Nil { + // Probe with no team context — log it so an operator can + // still find brute-force activity by grepping slog. Same + // fields as the persisted audit row (sans team_id). + slog.Warn("admin_audit.no_team_context", + "email", meta.Email, + "ip", meta.IP, + "path_suffix", meta.PathSuffix, + "http_status", meta.HTTPStatus, + "denied_by", meta.DeniedBy, + "user_agent_brief", meta.UserAgentBrief, + ) + } + + // Stash on locals so tests can read the computed metadata without + // querying the DB (used by AdminAuditMetadataFromLocals). + c.Locals(localKeyAdminAuditMeta, meta) + return err + } +} + +// localKeyAdminAuditMeta is the Fiber locals key holding the AdminAuditMetadata +// produced by AdminAuditEmit. Exposed via AdminAuditMetadataFromLocals so +// tests + downstream middleware can inspect the audit decision without a +// DB round-trip. +const localKeyAdminAuditMeta = "admin_audit_meta" + +// AdminAuditMetadataFromLocals returns the AdminAuditMetadata stamped by +// AdminAuditEmit, if present. Returns the zero value + false otherwise. +func AdminAuditMetadataFromLocals(c *fiber.Ctx) (AdminAuditMetadata, bool) { + v, ok := c.Locals(localKeyAdminAuditMeta).(AdminAuditMetadata) + return v, ok +} + +// buildAdminAuditMetadata assembles the AdminAuditMetadata for the current +// request. Pure function over the request — easy to unit-test. +func buildAdminAuditMetadata(c *fiber.Ctx, adminPathPrefix string, status int) AdminAuditMetadata { + email := strings.ToLower(strings.TrimSpace(GetEmail(c))) + ip := strings.TrimSpace(c.IP()) + suffix := adminAuditPathSuffix(c.Path(), adminPathPrefix) + ua := c.Get(fiber.HeaderUserAgent) + ua = ScrubAdminPath(ua, adminPathPrefix) + if len(ua) > adminUAMaxLen { + ua = ua[:adminUAMaxLen] + } + deniedBy := adminAuditDeniedReasonNone + if status == fiber.StatusForbidden { + // Rate-limit beat allowlist? Read the locals flag set by + // AdminRateLimit. Else assume allowlist_miss (the only other 403 + // path on this group). + if IsAdminRateLimited(c) { + deniedBy = adminAuditDeniedReasonRateLimit + } else { + deniedBy = adminAuditDeniedReasonAllowlistMiss + } + } + return AdminAuditMetadata{ + Email: email, + IP: ip, + PathSuffix: suffix, + HTTPStatus: status, + UserAgentBrief: ua, + DeniedBy: deniedBy, + } +} + +// adminAuditPathSuffix strips a leading "/api/v1//" from path, +// returning just the admin sub-path (e.g. "customers/:team_id/tier"). +// +// The strip is deliberately strict — if the path doesn't start with the +// expected prefix template we return a sentinel rather than the raw path, +// to prevent accidentally leaking the prefix into audit rows on a +// misconfigured router. The sentinel value "" is distinct from +// the LogScrubber sentinel "" so an operator scanning audit rows +// can tell the two paths apart. +func adminAuditPathSuffix(path, prefix string) string { + if prefix == "" { + return adminAuditSuffixInvalid + } + // Canonical mount in router.go is /api/v1//... + expected := "/api/v1/" + prefix + "/" + if !strings.HasPrefix(path, expected) { + // Also tolerate the no-trailing-slash terminal case + // (a request to /api/v1/, no further segments). Unlikely + // in practice — admin endpoints all have sub-paths — but defended. + if path == "/api/v1/"+prefix { + return "" // empty suffix == bare prefix hit + } + return adminAuditSuffixInvalid + } + return strings.TrimPrefix(path, expected) +} + +// adminAuditSuffixInvalid is the sentinel persisted when path stripping +// fails. Distinct from the LogScrubber sentinel so the operator can +// search audit rows for misconfigured strips ("DeniedBy=... PathSuffix=") +// separately from log lines. +const adminAuditSuffixInvalid = "" + +// adminAuditTeamID prefers the URL :team_id param (admin endpoints address +// a specific team) and falls back to (a) parsing a UUID-shaped segment +// directly from the URL path, then (b) the caller's own team from the +// JWT. Returns uuid.Nil when none of the three resolve. +// +// Why parse the URL path directly: Fiber's group-level middleware runs +// BEFORE the matched route's handler-level binding populates Params (the +// :team_id placeholder is associated with the leaf handler, not the +// group chain). The audit middleware is wired at the group level so we +// don't see Params yet. Rather than wire the audit middleware on every +// individual route, we parse the path with the well-known shape +// "customers//...". When the path doesn't carry a team-scoped +// :team_id (e.g. GET /customers list), this returns uuid.Nil and the +// audit row falls back to the caller's JWT team_id. +func adminAuditTeamID(c *fiber.Ctx) uuid.UUID { + if raw := c.Params("team_id"); raw != "" { + if id, err := uuid.Parse(raw); err == nil { + return id + } + } + if id := parseTeamIDFromAdminPath(c.Path()); id != uuid.Nil { + return id + } + if raw := GetTeamID(c); raw != "" { + if id, err := uuid.Parse(raw); err == nil { + return id + } + } + return uuid.Nil +} + +// parseTeamIDFromAdminPath looks for a "/customers/" segment pair +// anywhere in path and returns the parsed UUID. The admin surface mounts +// all team-scoped endpoints under /customers/:team_id/..., so any path +// matching that pattern carries the team in segment 1 after "customers". +// +// Returns uuid.Nil when: +// +// - path has no "customers/" segment (e.g. the bare /customers list); +// - the segment after "customers/" isn't a parseable UUID. +// +// The parse is intentionally generic over the prefix: we don't strip +// ADMIN_PATH_PREFIX here. The "customers" anchor is enough to +// disambiguate and avoids passing the secret prefix into this helper +// (one fewer place the prefix needs to travel). +func parseTeamIDFromAdminPath(path string) uuid.UUID { + idx := strings.Index(path, "/customers/") + if idx < 0 { + return uuid.Nil + } + rest := path[idx+len("/customers/"):] + // First segment after /customers/. Trim a trailing slash + further + // segments so e.g. "abc/tier" resolves to just "abc". + if slash := strings.IndexByte(rest, '/'); slash >= 0 { + rest = rest[:slash] + } + if id, err := uuid.Parse(rest); err == nil { + return id + } + return uuid.Nil +} + +// adminAuditSummary builds the human-readable one-liner persisted as +// audit_log.summary. Stays under 200 chars — the dashboard truncates +// longer values. +func adminAuditSummary(m AdminAuditMetadata) string { + who := m.Email + if who == "" { + who = "anonymous" + } + suffix := m.PathSuffix + if suffix == "" { + suffix = "(root)" + } + if m.DeniedBy != "" { + return who + " denied (" + m.DeniedBy + ") on " + suffix + } + return who + " accessed " + suffix +} + +// AdminAuditEnsureMetadataNoPrefix is a defensive grep-time helper for +// tests — it asserts that a marshaled AdminAuditMetadata contains zero +// occurrences of the prefix. We expose this in package-public form so +// any test file (handler-level, router-level, middleware-level) can +// reach for the same invariant check. +// +// Returns true when the metadata is prefix-free, false otherwise. +func AdminAuditEnsureMetadataNoPrefix(meta AdminAuditMetadata, prefix string) bool { + if prefix == "" { + return true + } + blob, _ := json.Marshal(meta) + return !strings.Contains(string(blob), prefix) +} + +// adminAuditCtxKey is the context key used internally to thread the +// metadata down to error handlers if needed. Kept opaque to prevent +// callers from stuffing data onto the same key by accident. +type adminAuditCtxKey struct{} + +// _ ensures adminAuditCtxKey is materialised at compile time (paranoia +// for the dead-code linter). +var _ = context.WithValue(context.Background(), adminAuditCtxKey{}, nil) + +// AdminAuditPathSuffixForTest is a test-only export of the internal +// adminAuditPathSuffix helper. Test files in the middleware_test package +// need to exercise the strip logic without going through a full Fiber +// app; making the helper public-but-marked-internal lets us pin the +// contract in unit tests without inviting external callers. +// +// DO NOT call this from production code paths — the strip is an internal +// detail of AdminAuditEmit and may change shape. +func AdminAuditPathSuffixForTest(path, prefix string) string { + return adminAuditPathSuffix(path, prefix) +} diff --git a/internal/middleware/admin_audit_test.go b/internal/middleware/admin_audit_test.go new file mode 100644 index 00000000..e85e4ae9 --- /dev/null +++ b/internal/middleware/admin_audit_test.go @@ -0,0 +1,308 @@ +package middleware_test + +// admin_audit_test.go — every hit on the admin route prefix MUST write +// an audit_log row with kind="admin.access", success or 403 alike. The +// metadata blob MUST NOT contain the ADMIN_PATH_PREFIX literal — the +// prefix is a secret and the audit_log row is operator-readable. + +import ( + "database/sql" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gofiber/fiber/v2" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "instant.dev/internal/middleware" + "instant.dev/internal/models" + "instant.dev/internal/testhelpers" +) + +// auditApp builds a Fiber app mirroring the production admin chain order: +// +// Fingerprint → AdminRateLimit → fake-auth → RequireAdmin → AdminAuditEmit → handler +// +// The fake-auth shim lets the test pin a JWT email + team_id on locals +// without spinning up real OAuth. callerEmail="" simulates an +// unauthenticated probe (RequireAdmin will reject with 403). +func auditApp(t *testing.T, db *sql.DB, prefix, callerEmail string) *fiber.App { + t.Helper() + app := fiber.New(fiber.Config{ProxyHeader: "X-Forwarded-For"}) + app.Use(middleware.Fingerprint()) + app.Use(func(c *fiber.Ctx) error { + if callerEmail != "" { + c.Locals(middleware.LocalKeyEmail, callerEmail) + } + c.Locals(middleware.LocalKeyUserID, uuid.NewString()) + return c.Next() + }) + // No real Redis here — for the audit tests we don't drive the rate + // limiter. nil Redis makes AdminRateLimit a no-op. + // + // AUDIT MUST RUN BEFORE RequireAdmin — production chain order. The + // reason: RequireAdmin returns a 403 directly on rejection (no + // c.Next), so middleware sitting AFTER it never runs on the + // rejection path. Putting AdminAuditEmit BEFORE lets its internal + // c.Next() drive the rest of the chain and observe the final status. + // + // Bind the middlewares to a route group rather than app.Use — Fiber's + // route-param matching (c.Params("team_id")) is only populated for + // middleware registered via Group, not for app-wide Use middleware + // that runs before route matching. + group := app.Group("/api/v1/"+prefix, + middleware.AdminRateLimit(nil), + middleware.AdminAuditEmit(db, prefix), + middleware.RequireAdmin(), + ) + group.Get("/customers/:team_id/tier", func(c *fiber.Ctx) error { + return c.JSON(fiber.Map{"ok": true}) + }) + group.Get("/customers", func(c *fiber.Ctx) error { + return c.JSON(fiber.Map{"ok": true}) + }) + return app +} + +// readLatestAdminAccess returns the latest admin.access audit row in the +// platform DB, or fails the test if none exists. We scope by metadata's +// `email` field to disambiguate when multiple tests run against the +// shared TEST_DATABASE_URL. +func readLatestAdminAccess(t *testing.T, db *sql.DB, expectedEmail string) (status int, suffix, deniedBy string, raw string) { + t.Helper() + row := db.QueryRow(` + SELECT metadata + FROM audit_log + WHERE kind = $1 AND metadata->>'email' = $2 + ORDER BY created_at DESC + LIMIT 1 + `, models.AuditKindAdminAccess, expectedEmail) + var meta sql.NullString + require.NoError(t, row.Scan(&meta)) + require.True(t, meta.Valid, "metadata column must be non-null") + var m middleware.AdminAuditMetadata + require.NoError(t, json.Unmarshal([]byte(meta.String), &m)) + return m.HTTPStatus, m.PathSuffix, m.DeniedBy, meta.String +} + +// adminAuditCleanup deletes any admin.access rows the tests wrote so +// repeated runs against a shared TEST_DATABASE_URL don't pollute each +// other. +func adminAuditCleanup(t *testing.T, db *sql.DB, email string) { + t.Helper() + t.Cleanup(func() { + db.Exec(`DELETE FROM audit_log WHERE kind = $1 AND metadata->>'email' = $2`, + models.AuditKindAdminAccess, email) + }) +} + +// TestAdminAuditEmit_Success_WritesRow — a successful admin request lands +// one admin.access audit row with the full metadata payload. +func TestAdminAuditEmit_Success_WritesRow(t *testing.T) { + if testing.Short() { + t.Skip("integration test — requires TEST_DATABASE_URL") + } + db, clean := testhelpers.SetupTestDB(t) + defer clean() + + prefix := strings.Repeat("a", 32) + email := "founder+success@instanode.dev" + t.Setenv("ADMIN_EMAILS", email) + adminAuditCleanup(t, db, email) + + app := auditApp(t, db, prefix, email) + // audit_log.team_id has an FK to teams.id — seed a real team so the + // insert lands cleanly. The admin route's :team_id param feeds the + // audit middleware's team_id resolution. + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + t.Cleanup(func() { + db.Exec(`DELETE FROM audit_log WHERE team_id = $1`, teamID) + db.Exec(`DELETE FROM teams WHERE id = $1`, teamID) + }) + path := "/api/v1/" + prefix + "/customers/" + teamID.String() + "/tier" + + req := httptest.NewRequest(http.MethodGet, path, nil) + req.Header.Set("User-Agent", "Mozilla/5.0 test-suite") + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + status, suffix, deniedBy, raw := readLatestAdminAccess(t, db, email) + assert.Equal(t, http.StatusOK, status) + assert.Equal(t, "customers/"+teamID.String()+"/tier", suffix, + "path_suffix MUST be the URL with the secret prefix stripped — no leading slash") + assert.Empty(t, deniedBy, "success row must have denied_by empty") + // IRON RULE — the raw metadata blob must NEVER contain the prefix. + assert.NotContains(t, raw, prefix, + "the persisted metadata MUST NOT contain ADMIN_PATH_PREFIX — it's a secret") +} + +// TestAdminAuditEmit_RateLimited_Writes403Row — even when the rate-limit +// middleware mutes the request, an admin.access row STILL gets written +// with http_status=403. This is the operator-visibility property: brute- +// force probes must appear in audit_log even though the response body +// claims "not an admin." +// +// We simulate the rate-limit path by setting the locals flag directly +// (avoids depending on real Redis + bucket exhaustion mechanics here — +// those are covered in admin_rate_limit_test.go). The audit middleware +// reads the flag and stamps denied_by="rate_limit" on the metadata. +func TestAdminAuditEmit_RateLimited_Writes403Row(t *testing.T) { + if testing.Short() { + t.Skip("integration test — requires TEST_DATABASE_URL") + } + db, clean := testhelpers.SetupTestDB(t) + defer clean() + + prefix := strings.Repeat("b", 32) + email := "founder+ratelimited@instanode.dev" + // Closed by default — empty ADMIN_EMAILS rejects every caller. But + // we want to exercise the rate-limit branch, so wire the email in + // AND inject the rate-limit-exceeded marker on locals upstream of + // RequireAdmin. We emulate it with a custom mini-app. + t.Setenv("ADMIN_EMAILS", email) + adminAuditCleanup(t, db, email) + + app := fiber.New(fiber.Config{ProxyHeader: "X-Forwarded-For"}) + app.Use(middleware.Fingerprint()) + app.Use(func(c *fiber.Ctx) error { + c.Locals(middleware.LocalKeyEmail, email) + c.Locals(middleware.LocalKeyAdminRateLimitExceeded, true) + return c.Next() + }) + // Audit middleware runs BEFORE the muted handler so its internal + // c.Next() can observe the 403 status the handler writes. Bind via + // Group so c.Params("team_id") is populated when the audit middleware + // runs (app.Use middleware runs pre-route-match and sees empty Params). + group := app.Group("/api/v1/"+prefix, + middleware.AdminAuditEmit(db, prefix), + ) + // Simulate the rate-limit-mute by short-circuiting to a 403 with the + // canonical body — exactly what AdminRateLimit does. We intentionally + // skip RequireAdmin here because in the real chain the limiter runs + // FIRST and the email never reaches RequireAdmin. Route defines the + // :team_id param so the audit middleware can resolve a FK-valid team. + group.Get("/customers/:team_id/tier", func(c *fiber.Ctx) error { + return c.Status(fiber.StatusForbidden).JSON(fiber.Map{ + "ok": false, + "error": "forbidden", + "message": "platform-admin access required", + "agent_action": "Tell the user this endpoint requires platform-admin access. Ask support@instanode.dev via https://instanode.dev/support if you think this is wrong.", + }) + }) + + // Seed a real team so audit_log FK validates. Probes against the real + // admin endpoint would resolve the URL :team_id (which a brute-force + // would supply as a guessed UUID — and the audit row needs that + // :team_id to FK-validate, OR the row is skipped + a slog.Warn fires + // to preserve operator visibility). + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + t.Cleanup(func() { + db.Exec(`DELETE FROM audit_log WHERE team_id = $1`, teamID) + db.Exec(`DELETE FROM teams WHERE id = $1`, teamID) + }) + path := "/api/v1/" + prefix + "/customers/" + teamID.String() + "/tier" + req := httptest.NewRequest(http.MethodGet, path, nil) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusForbidden, resp.StatusCode) + + status, suffix, deniedBy, raw := readLatestAdminAccess(t, db, email) + assert.Equal(t, http.StatusForbidden, status, + "the audit row MUST record the 403, not silently downgrade to 200") + assert.Equal(t, "customers/"+teamID.String()+"/tier", suffix) + assert.Equal(t, "rate_limit", deniedBy, + "the rate-limit branch MUST stamp denied_by=rate_limit on the audit metadata") + assert.NotContains(t, raw, prefix, + "the persisted metadata MUST NOT contain ADMIN_PATH_PREFIX — it's a secret") +} + +// TestAdminAuditEmit_AllowlistMiss_Writes403WithReason — when RequireAdmin +// rejects a caller whose email isn't on the allowlist, the audit row +// records the 403 with denied_by="allowlist_miss". +func TestAdminAuditEmit_AllowlistMiss_Writes403WithReason(t *testing.T) { + if testing.Short() { + t.Skip("integration test — requires TEST_DATABASE_URL") + } + db, clean := testhelpers.SetupTestDB(t) + defer clean() + + prefix := strings.Repeat("c", 32) + adminEmail := "founder@instanode.dev" + probeEmail := "probe+allowlistmiss@example.com" + t.Setenv("ADMIN_EMAILS", adminEmail) + adminAuditCleanup(t, db, probeEmail) + + // callerEmail = probeEmail → RequireAdmin rejects (not on allowlist). + app := auditApp(t, db, prefix, probeEmail) + // Real team required for the FK on audit_log.team_id. + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + t.Cleanup(func() { + db.Exec(`DELETE FROM audit_log WHERE team_id = $1`, teamID) + db.Exec(`DELETE FROM teams WHERE id = $1`, teamID) + }) + path := "/api/v1/" + prefix + "/customers/" + teamID.String() + "/tier" + req := httptest.NewRequest(http.MethodGet, path, nil) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusForbidden, resp.StatusCode) + + status, _, deniedBy, raw := readLatestAdminAccess(t, db, probeEmail) + assert.Equal(t, http.StatusForbidden, status) + assert.Equal(t, "allowlist_miss", deniedBy) + assert.NotContains(t, raw, prefix) +} + +// TestAdminAuditMetadata_PathSuffixStripsPrefix — pure unit test on the +// helper that builds the suffix. We don't need a DB / Fiber app for this; +// the goal is to lock in the strip behavior so a future refactor can't +// silently start persisting the full path. +func TestAdminAuditMetadata_PathSuffixStripsPrefix(t *testing.T) { + prefix := strings.Repeat("a", 32) + cases := []struct { + path string + expected string + }{ + {"/api/v1/" + prefix + "/customers", "customers"}, + {"/api/v1/" + prefix + "/customers/00000000-0000-0000-0000-000000000000/tier", + "customers/00000000-0000-0000-0000-000000000000/tier"}, + {"/api/v1/" + prefix, ""}, // bare prefix + // Misconfigured strip: path doesn't start with the prefix template. + {"/api/v1/admin/customers", ""}, + {"/api/v1/" + strings.Repeat("z", 32) + "/customers", ""}, + } + for _, tc := range cases { + got := middleware.AdminAuditPathSuffixForTest(tc.path, prefix) + assert.Equal(t, tc.expected, got, "path=%q", tc.path) + assert.NotContains(t, got, prefix, + "the suffix MUST NOT contain the secret prefix") + } +} + +// TestAdminAuditEnsureMetadataNoPrefix — the prefix-leak grep that we +// expose for cross-package tests. Sanity-check it does what it says. +func TestAdminAuditEnsureMetadataNoPrefix(t *testing.T) { + prefix := strings.Repeat("a", 32) + clean := middleware.AdminAuditMetadata{ + Email: "founder@instanode.dev", + IP: "10.0.0.1", + PathSuffix: "customers/x/tier", + HTTPStatus: 200, + } + assert.True(t, middleware.AdminAuditEnsureMetadataNoPrefix(clean, prefix)) + + dirty := middleware.AdminAuditMetadata{ + Email: "founder@instanode.dev", + PathSuffix: "/api/v1/" + prefix + "/customers", // mistakenly stored full path + HTTPStatus: 200, + } + assert.False(t, middleware.AdminAuditEnsureMetadataNoPrefix(dirty, prefix), + "the assertion MUST flag a metadata blob carrying the prefix") +} diff --git a/internal/middleware/admin_rate_limit.go b/internal/middleware/admin_rate_limit.go new file mode 100644 index 00000000..dc584973 --- /dev/null +++ b/internal/middleware/admin_rate_limit.go @@ -0,0 +1,205 @@ +package middleware + +// admin_rate_limit.go — per-fingerprint sliding-window rate limit on the +// admin route prefix. THIRD defense-in-depth layer on top of the existing +// ADMIN_PATH_PREFIX (gate 1) + ADMIN_EMAILS (gate 2): +// +// Gate 3 here: hard-cap 30 admin-route hits / minute / fingerprint. +// Excess returns 403 (NOT 429) — the response body and status code are +// indistinguishable from "not on the allowlist." That's the whole point: +// an attacker who somehow learned the unguessable prefix cannot +// differentiate "I'm probing too fast" from "I don't have an admin +// email" and therefore can't tell the prefix is right. +// +// Order in the admin chain (router.go): +// +// RateLimit → RequireAdmin → Audit → handler +// +// RATE LIMIT RUNS FIRST. If we put RequireAdmin before the rate-limit, +// an attacker who knows the prefix can probe forever by sending invalid +// JWTs (the allowlist check rejects, but no counter ever increments). +// Running the limiter first ensures every prefix hit costs a slot in +// the bucket — invalid-email probes are throttled exactly like valid- +// email-but-allowlist-miss probes. +// +// Storage: Redis sorted-set sliding window (one ZSET per fingerprint, +// keyed by minute). 25-hour TTL keeps the key around through DST-style +// edge cases the same way the daily provision rate-limit does. +// +// Fail-open on Redis errors: a Redis outage MUST NOT block legitimate +// admin work. We log the error, increment a metric, and let the request +// proceed. The risk model is "Redis is down so probing isn't a problem, +// the allowlist is still the last line" — the same posture every +// fingerprint-rate-limit in this codebase takes (see internal/middleware/ +// rate_limit.go). + +import ( + "context" + "fmt" + "log/slog" + "time" + + "github.com/gofiber/fiber/v2" + "github.com/redis/go-redis/v9" + "instant.dev/internal/metrics" +) + +const ( + // AdminRateLimitPerMinute is the per-fingerprint cap on admin-prefix + // hits within any rolling 60-second window. Set generously enough that + // a founder using the dashboard's customer-search-as-you-type doesn't + // trip the wall, low enough that a brute-force probe sees a hard wall + // at attempt 31. + // + // 30/min ≈ 0.5/s. The dashboard's heaviest admin call patterns (page + // load + 5 detail clicks + 5 audit pivots in one minute) max out near + // ~15 requests, leaving 50% headroom. A scripted probe needing 1k + // guesses takes >30 minutes at the wall — and every probe also hits + // AdminAuditEmit, so the operator sees the noise immediately. + AdminRateLimitPerMinute = 30 + + // adminRateLimitKeyPrefix is the Redis key namespace. Per-fingerprint + // sliding window: rl_admin:{fingerprint}. + adminRateLimitKeyPrefix = "rl_admin" + + // adminRateLimitTTL is the lifetime on the Redis ZSET. Just over an hour + // is enough — the sliding window is 60s, but we keep the key around + // past the window so a burst-then-pause-then-burst still sees its old + // entries cleaned up via ZREMRANGEBYSCORE on the next hit. + adminRateLimitTTL = 25 * time.Hour + + // adminRateLimitWindow is the rolling window size in seconds (the "30 + // req per MINUTE" denominator). + adminRateLimitWindow = 60 * time.Second +) + +// AdminRateLimit returns a Fiber middleware enforcing AdminRateLimitPerMinute +// admin-prefix hits per fingerprint per rolling minute. On excess it +// returns 403 with the canonical agent_action for admin denial — byte-for- +// byte identical to the RequireAdmin "not an admin" response, so a probe +// cannot tell which gate it hit. +// +// rdb may be nil — in that case the middleware degrades to a no-op pass- +// through (the rate-limit becomes infinite). The router doesn't wire a +// nil Redis in production; the nil-tolerance is for cleanliness in tests +// that build a partial Fiber app without Redis. +func AdminRateLimit(rdb *redis.Client) fiber.Handler { + return func(c *fiber.Ctx) error { + if rdb == nil { + return c.Next() + } + fp := GetFingerprint(c) + if fp == "" { + // No fingerprint == no key. Don't fail open silently; pass + // through. The RequireAdmin gate downstream still rejects + // any unauthenticated caller. + return c.Next() + } + + over, err := adminRateLimitExceeded(c.Context(), rdb, fp) + if err != nil { + slog.Error("admin_rate_limit.redis_error", + "error", err, + "fingerprint", fp, + "request_id", GetRequestID(c), + ) + metrics.RedisErrors.WithLabelValues("admin_rate_limit").Inc() + // Fail open — don't block legit admin work on a Redis hiccup. + return c.Next() + } + if over { + // IMPORTANT: this body MUST stay byte-identical to the + // RequireAdmin 403 body. Any drift (extra field, different + // message wording) leaks "the prefix is right but you're + // probing too fast" — exactly the signal we deny attackers. + metrics.FingerprintAbuseBlocked.Inc() + c.Locals(LocalKeyAdminRateLimitExceeded, true) + return c.Status(fiber.StatusForbidden).JSON(fiber.Map{ + "ok": false, + "error": "forbidden", + "message": "platform-admin access required", + "agent_action": adminForbiddenAgentAction, + }) + } + return c.Next() + } +} + +// LocalKeyAdminRateLimitExceeded is set on the Fiber locals when AdminRateLimit +// rejects the request. Lets the audit middleware (which runs AFTER this on +// the request side but reads locals at the response side via OnResponse) know +// the 403 came from the rate-limit path so it can stamp that on the audit row's +// `denied_by` field. The audit row is still written on a rate-limited reject — +// the operator must see brute-force probes even when the limiter is muting them. +const LocalKeyAdminRateLimitExceeded = "admin_rate_limited" + +// IsAdminRateLimited reports whether the current request was muted by the +// admin rate limiter. The audit middleware reads this to record the reason +// on the audit row. +func IsAdminRateLimited(c *fiber.Ctx) bool { + v, _ := c.Locals(LocalKeyAdminRateLimitExceeded).(bool) + return v +} + +// adminRateLimitExceeded implements the per-fingerprint sliding-window check +// against Redis. Algorithm (single pipeline, atomic from the client's POV): +// +// 1. ZREMRANGEBYSCORE the key — drop entries older than (now − window). +// 2. ZCARD the key — count remaining entries. +// 3. ZADD a unique entry for now. +// 4. EXPIRE the key so an idle fingerprint's data drops out cleanly. +// +// The CARD value AFTER cleanup tells us whether the caller has already +// used their quota in the window. We return over=true when the count is +// at or above the cap BEFORE this request is recorded — meaning this +// request is the (cap+1)th in the window. +// +// Note: ZCARD is read between cleanup and the new ZADD, so the value +// reflects "how many calls in the last 60s NOT counting this one." A +// caller making exactly 30 calls in a minute sees over=false on all 30; +// the 31st sees over=true. +// +// The ZADD member is "now-nanos:randhint" — a unique-per-call string so +// repeated calls in the same millisecond all distinct ZSET members. +// (ZADD with a duplicate member updates the score, which would let a +// caller hammer at sub-ms cadence and only ever leave one entry in the +// set.) +func adminRateLimitExceeded(ctx context.Context, rdb *redis.Client, fp string) (bool, error) { + key := adminRateLimitKey(fp) + now := time.Now() + cutoff := now.Add(-adminRateLimitWindow).UnixNano() + score := now.UnixNano() + // member must be unique per call — score alone collides under load. + // 4-byte random suffix from the score nanos is enough (tests run on a + // single goroutine; production has request_id propagation but we don't + // want a dep on that local). + member := fmt.Sprintf("%d:%d", score, score%1000003) + + pipe := rdb.Pipeline() + pipe.ZRemRangeByScore(ctx, key, "0", fmt.Sprintf("(%d", cutoff)) + cardCmd := pipe.ZCard(ctx, key) + pipe.ZAdd(ctx, key, redis.Z{Score: float64(score), Member: member}) + pipe.Expire(ctx, key, adminRateLimitTTL) + + if _, err := pipe.Exec(ctx); err != nil { + return false, fmt.Errorf("admin_rate_limit pipeline: %w", err) + } + count, err := cardCmd.Result() + if err != nil { + return false, fmt.Errorf("admin_rate_limit zcard: %w", err) + } + // count is the size of the ZSET AFTER cleanup, BEFORE this request's + // ZADD has been observed (Redis pipelines preserve order but the + // in-flight ZCARD reads the state at its execution point). count >= cap + // means "the last `cap` calls fall inside the window, this one would + // be the (cap+1)th." + return count >= int64(AdminRateLimitPerMinute), nil +} + +// adminRateLimitKey returns the Redis key for one fingerprint's admin +// sliding window. Lives in the rl_admin namespace so an ops dashboard +// can list active probing sources with `KEYS rl_admin:*` without +// matching the general /provision rate limit keys. +func adminRateLimitKey(fp string) string { + return fmt.Sprintf("%s:%s", adminRateLimitKeyPrefix, fp) +} diff --git a/internal/middleware/admin_rate_limit_test.go b/internal/middleware/admin_rate_limit_test.go new file mode 100644 index 00000000..83247d97 --- /dev/null +++ b/internal/middleware/admin_rate_limit_test.go @@ -0,0 +1,243 @@ +package middleware_test + +// admin_rate_limit_test.go — verifies the per-fingerprint 30/min cap on +// admin route prefix hits AND the byte-for-byte response-shape parity +// with the allowlist-miss 403. +// +// The critical invariant: when the limiter mutes a request, the response +// body MUST be byte-identical to what RequireAdmin returns for "not on +// the allowlist." An attacker probing the admin prefix from an unknown +// IP cannot tell which gate denied them. + +import ( + "bytes" + "fmt" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gofiber/fiber/v2" + "github.com/redis/go-redis/v9" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "instant.dev/internal/middleware" + "instant.dev/internal/testhelpers" +) + +// adminRLApp builds a Fiber app that exercises the rate-limit middleware +// followed by a stub admin handler. We don't wire RequireAdmin here — the +// goal is to isolate the limiter's behavior. A separate test (the +// response-parity test) chains both to assert the body-identical rule. +// +// ProxyHeader matches the production router so X-Forwarded-For drives +// c.IP() (otherwise every request would resolve to 0.0.0.0 and collapse +// to one fingerprint). +func adminRLApp(rdb *redis.Client) *fiber.App { + app := fiber.New(fiber.Config{ProxyHeader: "X-Forwarded-For"}) + app.Use(middleware.Fingerprint()) + app.Use(middleware.AdminRateLimit(rdb)) + app.Get("/api/v1/*", func(c *fiber.Ctx) error { + // Handler is reached ONLY when the limiter lets the request through. + return c.Status(fiber.StatusOK).JSON(fiber.Map{"ok": true, "from": "stub"}) + }) + return app +} + +// uniqueIPForRL returns an IPv4 string that maps to a unique /24 — the +// same approach as the production fingerprint hash. We can't reuse +// testhelpers.FingerprintToIP because we want different fingerprints in +// different tests but no /24 collision across the parallel test set. +func uniqueIPForRL(t *testing.T) string { + t.Helper() + // Use the test's name as the seed — deterministic, debuggable. + var h uint32 + for _, b := range []byte(t.Name()) { + h = h*31 + uint32(b) + } + return fmt.Sprintf("10.66.%d.1", (h%254)+1) +} + +// TestAdminRateLimit_31stHitReturns403 — the headline contract from the +// task brief. Within a single rolling minute, the first 30 requests from +// one fingerprint pass through; the 31st is muted with a 403. The +// response body MUST mirror the RequireAdmin "not on allowlist" shape. +func TestAdminRateLimit_31stHitReturns403(t *testing.T) { + rdb, cleanR := testhelpers.SetupTestRedis(t) + defer cleanR() + + app := adminRLApp(rdb) + ip := uniqueIPForRL(t) + + // First 30: all pass through. + for i := 1; i <= middleware.AdminRateLimitPerMinute; i++ { + req := httptest.NewRequest(http.MethodGet, "/api/v1/some/admin/path", nil) + req.Header.Set("X-Forwarded-For", ip) + resp, err := app.Test(req, 3000) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode, + "request %d/%d must pass the rate limit", i, middleware.AdminRateLimitPerMinute) + resp.Body.Close() + } + + // 31st: muted. + req := httptest.NewRequest(http.MethodGet, "/api/v1/some/admin/path", nil) + req.Header.Set("X-Forwarded-For", ip) + resp, err := app.Test(req, 3000) + require.NoError(t, err) + defer resp.Body.Close() + assert.Equal(t, http.StatusForbidden, resp.StatusCode, + "the 31st hit MUST be muted with 403 (never 429 — that would leak the gate)") +} + +// TestAdminRateLimit_403MatchesAllowlistMiss_ByteForByte — the WHOLE POINT +// of the layer. A rate-limited 403 must be byte-for-byte indistinguishable +// from an allowlist-miss 403. Any drift — different message, missing +// field, reordered keys — leaks "the prefix is right, you're just probing +// too fast," which is exactly the signal we deny attackers. +// +// We run two requests: +// +// A) Rate-limit path: exhaust the bucket, then make the muted request. +// The limiter responds without consulting RequireAdmin. +// +// B) Allowlist-miss path: fresh fingerprint, but RequireAdmin rejects +// because the JWT email isn't on the allowlist. +// +// Then assert the response bodies are byte-identical. +func TestAdminRateLimit_403MatchesAllowlistMiss_ByteForByte(t *testing.T) { + rdb, cleanR := testhelpers.SetupTestRedis(t) + defer cleanR() + + // Build an app that mirrors the production chain order: + // RateLimit → RequireAdmin → handler. + // We inject a fake-auth shim that puts a NON-admin email on locals + // so RequireAdmin rejects on every hit. ADMIN_EMAILS is set to a + // different address. + t.Setenv("ADMIN_EMAILS", "founder@instanode.dev") + app := fiber.New(fiber.Config{ProxyHeader: "X-Forwarded-For"}) + app.Use(middleware.Fingerprint()) + app.Use(middleware.AdminRateLimit(rdb)) + // Fake auth: pin a non-admin email so RequireAdmin always rejects. + app.Use(func(c *fiber.Ctx) error { + c.Locals(middleware.LocalKeyEmail, "alice@example.com") + return c.Next() + }) + app.Use(middleware.RequireAdmin()) + app.Get("/api/v1/*", func(c *fiber.Ctx) error { + return c.Status(fiber.StatusOK).JSON(fiber.Map{"ok": true}) + }) + + // ─── Path B: allowlist miss (fresh fingerprint, 1st request) ──────── + ipB := uniqueIPForRL(t) + ".B" + // fingerprint hashes are tolerant of arbitrary IP-shaped strings; we + // strip the .B suffix back for the X-Forwarded-For header below. + ipB = ipB[:len(ipB)-2] + reqB := httptest.NewRequest(http.MethodGet, "/api/v1/customers", nil) + reqB.Header.Set("X-Forwarded-For", ipB) + respB, err := app.Test(reqB, 3000) + require.NoError(t, err) + bodyB, _ := io.ReadAll(respB.Body) + respB.Body.Close() + assert.Equal(t, http.StatusForbidden, respB.StatusCode, + "allowlist miss must return 403") + + // ─── Path A: rate-limit mute (same fp exhausted, then mute) ───────── + // Use a SEPARATE fingerprint so the bucket isn't polluted by path B's + // single hit. Pre-fill the limiter to 30 by hammering the endpoint. + ipA := uniqueIPForRL(t) + ".A" + ipA = ipA[:len(ipA)-2] + for i := 0; i < middleware.AdminRateLimitPerMinute; i++ { + r := httptest.NewRequest(http.MethodGet, "/api/v1/customers", nil) + r.Header.Set("X-Forwarded-For", ipA) + resp, _ := app.Test(r, 3000) + resp.Body.Close() + } + // 31st: muted by limiter BEFORE RequireAdmin sees it. + reqA := httptest.NewRequest(http.MethodGet, "/api/v1/customers", nil) + reqA.Header.Set("X-Forwarded-For", ipA) + respA, err := app.Test(reqA, 3000) + require.NoError(t, err) + bodyA, _ := io.ReadAll(respA.Body) + respA.Body.Close() + assert.Equal(t, http.StatusForbidden, respA.StatusCode) + + // Bodies must be byte-identical. The WHOLE PROBE-INDISTINGUISHABILITY + // CONTRACT lives in this assertion. Any drift = leak. + assert.True(t, bytes.Equal(bodyA, bodyB), + "rate-limit 403 body MUST match allowlist-miss 403 body byte-for-byte\n rate-limit: %s\n allowlist: %s", + string(bodyA), string(bodyB)) +} + +// TestAdminRateLimit_FailsOpen_OnRedisDown — when Redis is unreachable +// the limiter MUST NOT block requests. Matches the codebase-wide fail- +// open posture for fingerprint-rate-limiting. Pointing the client at a +// dead address simulates the outage. +func TestAdminRateLimit_FailsOpen_OnRedisDown(t *testing.T) { + deadRDB := redis.NewClient(&redis.Options{ + Addr: "localhost:19999", // nothing listening + DialTimeout: 100 * time.Millisecond, + ReadTimeout: 100 * time.Millisecond, + }) + defer deadRDB.Close() + + app := adminRLApp(deadRDB) + ip := uniqueIPForRL(t) + + // Send well past the cap; every request must pass because Redis errors + // flip the limiter to fail-open. + for i := 0; i < middleware.AdminRateLimitPerMinute+5; i++ { + req := httptest.NewRequest(http.MethodGet, "/api/v1/customers", nil) + req.Header.Set("X-Forwarded-For", ip) + resp, err := app.Test(req, 1000) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode, + "Redis-down MUST fail open (request %d)", i+1) + resp.Body.Close() + } +} + +// TestAdminRateLimit_DifferentFingerprints_Independent — each fingerprint +// gets its own bucket. Exhausting fingerprint A must not affect B. +func TestAdminRateLimit_DifferentFingerprints_Independent(t *testing.T) { + rdb, cleanR := testhelpers.SetupTestRedis(t) + defer cleanR() + app := adminRLApp(rdb) + + // Use two distinct /24 subnets so the fingerprint hash differs. The + // production fingerprint hashes /24 + ASN; in tests we have no ASN so + // it's just the /24. 10.77 vs 10.88 + a test-name-derived octet keeps + // each test isolated from concurrently-running tests. + var h uint32 + for _, b := range []byte(t.Name()) { + h = h*31 + uint32(b) + } + octet := byte((h % 254) + 1) + ipA := fmt.Sprintf("10.77.%d.1", octet) + ipB := fmt.Sprintf("10.88.%d.1", octet) + + // Drain A's bucket. + for i := 0; i < middleware.AdminRateLimitPerMinute; i++ { + r := httptest.NewRequest(http.MethodGet, "/api/v1/customers", nil) + r.Header.Set("X-Forwarded-For", ipA) + resp, _ := app.Test(r, 3000) + resp.Body.Close() + } + // A's 31st: muted. + rA := httptest.NewRequest(http.MethodGet, "/api/v1/customers", nil) + rA.Header.Set("X-Forwarded-For", ipA) + respA, _ := app.Test(rA, 3000) + respA.Body.Close() + assert.Equal(t, http.StatusForbidden, respA.StatusCode, + "A's bucket must be drained") + + // B's 1st: passes. + rB := httptest.NewRequest(http.MethodGet, "/api/v1/customers", nil) + rB.Header.Set("X-Forwarded-For", ipB) + respB, _ := app.Test(rB, 3000) + defer respB.Body.Close() + assert.Equal(t, http.StatusOK, respB.StatusCode, + "B's bucket must be untouched by A's exhaustion") +} diff --git a/internal/middleware/log_scrubber.go b/internal/middleware/log_scrubber.go new file mode 100644 index 00000000..3968e1d5 --- /dev/null +++ b/internal/middleware/log_scrubber.go @@ -0,0 +1,243 @@ +package middleware + +// log_scrubber.go — slog handler wrapper that replaces the unguessable +// ADMIN_PATH_PREFIX value with the literal sentinel "" anywhere it +// appears in string attributes on a log record. +// +// Why this exists: +// +// The admin surface is registered under /api/v1//... +// That prefix is a SECRET with the same blast radius as a session token +// (see internal/router/router.go's defense-in-depth comment and +// internal/config/config.go's AdminPathPrefix doc). Anything that emits +// the request URL — slog, fiber's request_id stamp, otel spans, NR +// transactions, panic traces — risks leaking that secret into the log +// shipper, NR, OTel collector, or stderr. The same risk applies to a +// 401/403/500 from an admin route that bubbles a URL-bearing message +// through fiber's ErrorHandler. +// +// To close the leak surface uniformly, we wrap the global slog handler +// with a Scrubber that walks every record's string attrs and rewrites +// matches in place. This is one centralized choke-point rather than +// N hand-scrubbed call sites — which means an engineer adding a new +// `slog.Info("admin.foo", "url", c.OriginalURL())` line tomorrow can't +// accidentally leak the prefix. +// +// Match policy: +// +// Plain substring replacement against the configured secret. Not a +// regex — the secret is alphanumeric (validated at config-load), so +// there's no ambiguity. The literal value of the secret is replaced +// with the literal sentinel "" everywhere it appears in a +// string attribute or in the message itself. +// +// Empty / unset secret → handler is a pure pass-through (no scan, no +// alloc). This is the closed-by-default state when ADMIN_PATH_PREFIX +// is unset and the admin surface isn't even registered. +// +// Mirrors the JWT-style "replace secret with sentinel" pattern called out +// in the request_id middleware comment in router.go. The same scrubber +// could later be extended to redact bearer tokens / API keys via a +// matchers slice — we deliberately scope the v1 to ADMIN_PATH_PREFIX so +// the contract under test is minimal and grep-auditable. +// +// Test coverage: +// +// 1. /api/v1/abc123<...>/customers/foo → /api/v1//customers/foo +// (the literal task scrub: prefix replaced inside a URL string attr). +// 2. Empty prefix → string passes through unchanged (no-op handler). +// 3. Multiple attrs all scrubbed (groups, nested, message body itself). +// 4. Non-string attrs untouched (int, bool, time, etc.). + +import ( + "context" + "log/slog" + "strings" +) + +// AdminScrubSentinel is the literal token written in place of any matched +// secret. Named so tests + audits can grep for the one source of truth. +// Mirrors the "" / "" sentinel style — short, unambiguous, +// and a non-URL-safe character ("<") so it never round-trips back into a +// live admin URL. +const AdminScrubSentinel = "" + +// LogScrubber wraps an underlying slog.Handler and rewrites any occurrence +// of secret inside string-valued attributes (and the message body) to +// AdminScrubSentinel before forwarding to the wrapped handler. +// +// Construct with NewLogScrubber. A zero LogScrubber is NOT safe — the +// nil base handler would panic on Handle. +type LogScrubber struct { + base slog.Handler + secret string +} + +// NewLogScrubber returns a slog.Handler that scrubs every occurrence of +// secret in every string attribute / message body before delegating to +// base. When secret is empty the returned handler is base unchanged — +// the scrubber adds zero overhead when ADMIN_PATH_PREFIX isn't set. +// +// base must not be nil. The expected wiring at main() is: +// +// jsonH := slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{...}) +// ctxH := logctx.NewHandler("api", jsonH) +// scrub := middleware.NewLogScrubber(ctxH, cfg.AdminPathPrefix) +// slog.SetDefault(slog.New(scrub)) +// +// Placing the scrubber on the OUTSIDE of the logctx handler is intentional: +// the scrub runs LAST, after every field (including the context-injected +// trace_id / team_id / service) is finalised, so a stray prefix value +// stamped through the context path also gets caught. +func NewLogScrubber(base slog.Handler, secret string) slog.Handler { + if secret == "" { + // Pass-through: nothing to scrub, don't introduce overhead. + return base + } + return &LogScrubber{base: base, secret: secret} +} + +// Enabled forwards to the wrapped handler unchanged — the wrapper must +// never change which records are emitted; that's the base handler's +// decision (per slog.Handler contract). +func (h *LogScrubber) Enabled(ctx context.Context, level slog.Level) bool { + return h.base.Enabled(ctx, level) +} + +// Handle scrubs the record before forwarding. The slog.Record is mutated +// in place via a builder pattern: we re-walk every attribute and rebuild +// the record with sanitized string values. Non-string values pass through +// untouched. The Message field is also scrubbed. +func (h *LogScrubber) Handle(ctx context.Context, r slog.Record) error { + // Fast path: if the message + any attrs don't reference the secret + // at all, we can skip the rebuild and forward the record unchanged. + // (Common case — the secret is the admin prefix, only a tiny fraction + // of records touch it.) + if !h.containsSecret(r) { + return h.base.Handle(ctx, r) + } + + // Slow path: scrub the message + attrs. We can't mutate r.Attrs + // directly (slog.Record doesn't expose a setter), so we rebuild a + // fresh Record with the sanitized values. + scrubbed := slog.NewRecord(r.Time, r.Level, h.scrub(r.Message), r.PC) + r.Attrs(func(a slog.Attr) bool { + scrubbed.AddAttrs(h.scrubAttr(a)) + return true + }) + return h.base.Handle(ctx, scrubbed) +} + +// WithAttrs returns a new wrapper. We scrub the supplied attrs eagerly so +// child loggers (built via slog.Logger.With) carry sanitized fields. The +// secret is preserved on the new wrapper so subsequent Handle calls keep +// scrubbing. +func (h *LogScrubber) WithAttrs(attrs []slog.Attr) slog.Handler { + scrubbed := make([]slog.Attr, len(attrs)) + for i, a := range attrs { + scrubbed[i] = h.scrubAttr(a) + } + return &LogScrubber{base: h.base.WithAttrs(scrubbed), secret: h.secret} +} + +// WithGroup returns a new wrapper around base.WithGroup. The secret is +// preserved on the new wrapper. +func (h *LogScrubber) WithGroup(name string) slog.Handler { + return &LogScrubber{base: h.base.WithGroup(name), secret: h.secret} +} + +// containsSecret reports whether the record's message or any string +// attribute contains the secret. Lets the fast path skip the rebuild +// allocation for records that don't touch the admin prefix at all. +func (h *LogScrubber) containsSecret(r slog.Record) bool { + if strings.Contains(r.Message, h.secret) { + return true + } + found := false + r.Attrs(func(a slog.Attr) bool { + if h.attrContainsSecret(a) { + found = true + return false // stop iteration + } + return true + }) + return found +} + +// attrContainsSecret reports whether a single Attr's string-valued payload +// contains the secret. Recurses into LogValuer / Group attrs so a nested +// group that stuffs the prefix into a sub-field still gets caught. +func (h *LogScrubber) attrContainsSecret(a slog.Attr) bool { + v := a.Value.Resolve() + switch v.Kind() { + case slog.KindString: + return strings.Contains(v.String(), h.secret) + case slog.KindGroup: + for _, ga := range v.Group() { + if h.attrContainsSecret(ga) { + return true + } + } + } + return false +} + +// scrubAttr returns a copy of a with every string-valued payload run +// through scrub. Non-string kinds pass through unchanged. +func (h *LogScrubber) scrubAttr(a slog.Attr) slog.Attr { + v := a.Value.Resolve() + switch v.Kind() { + case slog.KindString: + s := v.String() + if !strings.Contains(s, h.secret) { + return a + } + return slog.String(a.Key, h.scrub(s)) + case slog.KindGroup: + groupAttrs := v.Group() + scrubbed := make([]slog.Attr, len(groupAttrs)) + anyChanged := false + for i, ga := range groupAttrs { + scrubbed[i] = h.scrubAttr(ga) + // Cheap heuristic: a scrubbed string attr has a different + // raw String() than the source. For non-string kinds the + // rebuild is a no-op, so they're trivially "unchanged." + if ga.Value.Kind() == slog.KindString && + ga.Value.String() != scrubbed[i].Value.String() { + anyChanged = true + } + } + if !anyChanged { + return a + } + // Re-wrap as a group attr. slog has no GroupValue helper that takes + // []Attr; build via slog.Group which takes ...any. + anyArgs := make([]any, len(scrubbed)) + for i, ga := range scrubbed { + anyArgs[i] = ga + } + return slog.Group(a.Key, anyArgs...) + } + return a +} + +// scrub does the literal substring replacement. Public-facing callers +// should use the handler — this exists for the rare case (tests, an ad-hoc +// log line in a hot path) where a caller wants the raw transform. +func (h *LogScrubber) scrub(s string) string { + if h.secret == "" { + return s + } + return strings.ReplaceAll(s, h.secret, AdminScrubSentinel) +} + +// ScrubAdminPath is a free-function helper for callers that want to scrub +// a single string without going through the slog pipeline. Useful for one- +// off bug reports / panic-recovery messages. Returns s unchanged when +// secret is empty. +func ScrubAdminPath(s, secret string) string { + if secret == "" || s == "" { + return s + } + return strings.ReplaceAll(s, secret, AdminScrubSentinel) +} diff --git a/internal/middleware/log_scrubber_test.go b/internal/middleware/log_scrubber_test.go new file mode 100644 index 00000000..43289b3c --- /dev/null +++ b/internal/middleware/log_scrubber_test.go @@ -0,0 +1,211 @@ +package middleware_test + +// log_scrubber_test.go — verifies that the slog handler wrapper rewrites +// occurrences of ADMIN_PATH_PREFIX with "" in every log emission. +// +// This is the layer-5 piece of the admin defense-in-depth task. The +// rate-limit + audit middlewares hide the prefix from the wire and from +// audit_log rows. The scrubber hides the prefix from the global slog +// pipeline — request-id middleware, Fiber's request logger, NewRelic +// transaction names that bubble through slog, panic-recovery messages +// that quote OriginalURL, etc. + +import ( + "bytes" + "context" + "encoding/json" + "log/slog" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "instant.dev/internal/middleware" +) + +// captureLogger builds a slog.Logger that emits JSON lines into buf, +// wrapping the JSON handler in the admin-prefix scrubber. Returns the +// logger + the buffer the test reads back from. +func captureLogger(prefix string) (*slog.Logger, *bytes.Buffer) { + var buf bytes.Buffer + base := slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}) + scrubbed := middleware.NewLogScrubber(base, prefix) + return slog.New(scrubbed), &buf +} + +// readLastLine decodes the most recent JSON record written to buf and +// returns it as a generic map. Lets tests assert on field values without +// caring about the surrounding noise (time, level, source). +func readLastLine(t *testing.T, buf *bytes.Buffer) map[string]any { + t.Helper() + lines := bytes.Split(bytes.TrimSpace(buf.Bytes()), []byte("\n")) + require.NotEmpty(t, lines, "no log line emitted") + last := lines[len(lines)-1] + var out map[string]any + require.NoError(t, json.Unmarshal(last, &out), "last log line: %s", string(last)) + return out +} + +// TestLogScrubber_PrefixInURLAttr_Replaced is the canonical case from the +// task brief: "/api/v1/abc123<...>/customers/foo → /api/v1//customers/foo". +// We emit a slog.Info line with a "url" string attribute containing the +// admin prefix and assert the persisted record has the prefix replaced +// with the sentinel. +func TestLogScrubber_PrefixInURLAttr_Replaced(t *testing.T) { + prefix := strings.Repeat("a", 32) // canonical 32-char alphanumeric prefix + logger, buf := captureLogger(prefix) + + logger.Info("request.received", "url", "/api/v1/"+prefix+"/customers/foo") + + line := readLastLine(t, buf) + url, _ := line["url"].(string) + assert.Equal(t, "/api/v1//customers/foo", url, + "the scrubber MUST replace the prefix with in the persisted URL") + // And the raw prefix must not appear anywhere in the emitted JSON. + assert.NotContains(t, buf.String(), prefix, + "the raw prefix must not appear in any field of the emitted JSON") +} + +// TestLogScrubber_PrefixInMessageBody_Replaced — the scrubber must also +// rewrite the record's Message field, not just attributes. Fiber's request +// logger formats the URL into the message string for some configurations. +func TestLogScrubber_PrefixInMessageBody_Replaced(t *testing.T) { + prefix := strings.Repeat("b", 32) + logger, buf := captureLogger(prefix) + + logger.Info("hit on /api/v1/" + prefix + "/customers") + + line := readLastLine(t, buf) + msg, _ := line["msg"].(string) + assert.Equal(t, "hit on /api/v1//customers", msg) + assert.NotContains(t, buf.String(), prefix) +} + +// TestLogScrubber_EmptyPrefix_Passthrough — when ADMIN_PATH_PREFIX is +// empty (admin surface disabled), the scrubber MUST be a pure passthrough. +// No allocation, no sentinel substitution. This is the closed-by-default +// state for dev / CI environments that never set the env var. +func TestLogScrubber_EmptyPrefix_Passthrough(t *testing.T) { + logger, buf := captureLogger("") + + logger.Info("request.received", "url", "/api/v1/admin/customers") + + line := readLastLine(t, buf) + url, _ := line["url"].(string) + assert.Equal(t, "/api/v1/admin/customers", url, + "with empty secret, the scrubber MUST be a passthrough — no sentinel substitution") + assert.NotContains(t, buf.String(), "") +} + +// TestLogScrubber_NonStringAttrsUntouched — int / bool / time values must +// pass through unchanged. Only string-valued payloads are scrubbed. This +// pins the contract that the scrubber doesn't accidentally rewrite a +// status code or duration measurement. +func TestLogScrubber_NonStringAttrsUntouched(t *testing.T) { + prefix := strings.Repeat("c", 32) + logger, buf := captureLogger(prefix) + + logger.Info("request.received", + "status", 200, + "latency_ms", 42, + "ok", true, + "path", "/api/v1/"+prefix+"/customers", // string — should scrub + ) + + line := readLastLine(t, buf) + assert.EqualValues(t, 200, line["status"]) + assert.EqualValues(t, 42, line["latency_ms"]) + assert.Equal(t, true, line["ok"]) + assert.Equal(t, "/api/v1//customers", line["path"]) +} + +// TestLogScrubber_MultipleAttrsAllScrubbed — every string attribute +// carrying the prefix must be scrubbed in one log line, not just the +// first one encountered. +func TestLogScrubber_MultipleAttrsAllScrubbed(t *testing.T) { + prefix := strings.Repeat("d", 32) + logger, buf := captureLogger(prefix) + + logger.Info("request.received", + "path", "/api/v1/"+prefix+"/customers/x", + "referrer", "https://example.com/api/v1/"+prefix+"/customers", + "note", "the prefix "+prefix+" appears here too", + ) + + line := readLastLine(t, buf) + assert.Equal(t, "/api/v1//customers/x", line["path"]) + assert.Equal(t, "https://example.com/api/v1//customers", line["referrer"]) + assert.Equal(t, "the prefix appears here too", line["note"]) + assert.NotContains(t, buf.String(), prefix, + "after scrubbing, the raw prefix must not appear in any field") +} + +// TestLogScrubber_NestedGroups_PrefixesScrubbed — slog groups (nested +// attribute namespaces) must also have their string children scrubbed. +// This is the regression test that says: "if a future logger emits the +// admin URL inside a group { request { url } }, the scrubber still +// catches it." +func TestLogScrubber_NestedGroups_PrefixesScrubbed(t *testing.T) { + prefix := strings.Repeat("e", 32) + logger, buf := captureLogger(prefix) + + logger.Info("request.received", + slog.Group("http", + slog.String("url", "/api/v1/"+prefix+"/customers"), + slog.String("method", "GET"), + ), + ) + assert.NotContains(t, buf.String(), prefix, + "the prefix must not survive scrubbing even inside a slog.Group") + assert.Contains(t, buf.String(), "") +} + +// TestLogScrubber_JWTPattern_Untouched — REGRESSION test for the contract +// that the new scrubber does NOT break existing scrubs (point 6 of the +// task brief). The scrubber operates ONLY on ADMIN_PATH_PREFIX values; +// JWT-shaped tokens, bearer prefixes, and other secret patterns flow +// through untouched. The codebase has separate (future) machinery for +// those — what we're guarding here is that the admin-prefix scrubber +// doesn't accidentally cargo-cult-redact unrelated strings, e.g. via an +// overly-broad regex. +func TestLogScrubber_JWTPattern_Untouched(t *testing.T) { + prefix := strings.Repeat("f", 32) + logger, buf := captureLogger(prefix) + + jwt := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjMifQ.abc" + logger.Info("auth", + "jwt", jwt, + "path", "/api/v1/"+prefix+"/customers", + ) + line := readLastLine(t, buf) + assert.Equal(t, jwt, line["jwt"], + "the admin-prefix scrubber must NOT touch JWT-shaped strings — only the configured prefix") + assert.Equal(t, "/api/v1//customers", line["path"]) +} + +// TestScrubAdminPath_Helper — exercise the free-function helper used by +// one-off call sites that want to scrub a string without going through +// the slog handler. +func TestScrubAdminPath_Helper(t *testing.T) { + prefix := strings.Repeat("g", 32) + in := "POST /api/v1/" + prefix + "/customers/00000000-0000-0000-0000-000000000000/tier" + out := middleware.ScrubAdminPath(in, prefix) + assert.Equal(t, "POST /api/v1//customers/00000000-0000-0000-0000-000000000000/tier", out) + + // Empty secret => passthrough. + assert.Equal(t, in, middleware.ScrubAdminPath(in, "")) + // Empty input => passthrough. + assert.Equal(t, "", middleware.ScrubAdminPath("", prefix)) +} + +// TestLogScrubber_PassesThroughEnabled — wrapping must not alter the +// emitting decision; what the base handler accepts, the wrapper accepts. +func TestLogScrubber_PassesThroughEnabled(t *testing.T) { + prefix := strings.Repeat("h", 32) + base := slog.NewJSONHandler(&bytes.Buffer{}, &slog.HandlerOptions{Level: slog.LevelWarn}) + scrub := middleware.NewLogScrubber(base, prefix) + assert.False(t, scrub.Enabled(context.Background(), slog.LevelDebug), + "Enabled MUST forward the underlying handler's decision") + assert.True(t, scrub.Enabled(context.Background(), slog.LevelError)) +} diff --git a/internal/models/audit_kinds.go b/internal/models/audit_kinds.go index 7a9306e0..5d7138f0 100644 --- a/internal/models/audit_kinds.go +++ b/internal/models/audit_kinds.go @@ -44,4 +44,28 @@ const ( // consumer can distinguish "we canceled in Razorpay" from "we tried but // the call failed — operator must reconcile in the Razorpay dashboard." AuditKindSubscriptionCanceledByAdmin = "subscription.canceled_by_admin" + + // AuditKindAdminAccess fires on every hit to the admin route prefix — + // success (2xx) AND rejected (403). Written by middleware.AdminAuditEmit + // installed on the admin route group after RequireAdmin. Drives the BI + // query "who accessed what admin surface and when" and supplies the + // raw signal an SOC dashboard pivots on if leaked admin credentials + // are suspected. + // + // Metadata shape (verified by middleware.adminAuditMetadata): + // + // { + // "email": "", + // "ip": "", + // "path_suffix": "", + // "http_status": , + // "user_agent_brief": "" + // } + // + // CRITICAL: path_suffix is the SUFFIX only — the unguessable + // ADMIN_PATH_PREFIX is stripped before persistence. Storing the + // full path would defeat the whole point of the prefix gate + // (a DB read would leak the secret to anyone with audit_log access). + // The metadata blob is asserted prefix-free in tests via grep. + AuditKindAdminAccess = "admin.access" ) diff --git a/internal/router/router.go b/internal/router/router.go index 8fc65b75..881bf5b6 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -451,7 +451,37 @@ func New(cfg *config.Config, db *sql.DB, rdb *redis.Client, geoDbs *middleware.G portal := &razorpaybilling.Portal{DB: db, Cfg: cfg} return portal.CancelImmediately(subID) } - adminGroup := api.Group("/"+cfg.AdminPathPrefix, middleware.RequireAdmin()) + // Defense-in-depth gates 3-5, chained in strict order: + // + // AdminRateLimit — 30 req/min/fingerprint cap. Returns 403 with + // a body byte-for-byte identical to the + // allowlist-miss 403. Runs BEFORE RequireAdmin + // so an attacker who knows the prefix cannot + // bypass the limiter by sending invalid JWTs. + // AdminAuditEmit — after-response middleware. Internally calls + // c.Next() and observes the FINAL status; logs + // EVERY hit on the prefix (success AND 403). + // PathSuffix is the URL with the prefix + // stripped — the secret prefix MUST NOT land + // in audit_log. + // RequireAdmin — ADMIN_EMAILS allowlist check (gate 2). + // + // Audit MUST sit BEFORE RequireAdmin in the chain. RequireAdmin + // returns a 403 directly (no c.Next call) on rejection — any + // middleware sitting AFTER it would never run on the rejection + // path, so the brute-force-visibility property would silently + // break. By sitting BEFORE, the audit middleware's internal + // c.Next() dispatches RequireAdmin → handler and observes the + // final status either way. + // + // AdminRateLimit stays first: it short-circuits on excess with + // its own 403 (which AdminAuditEmit's c.Next observes as 403 + + // IsAdminRateLimited(c)=true → denied_by=rate_limit on the row). + adminGroup := api.Group("/"+cfg.AdminPathPrefix, + middleware.AdminRateLimit(rdb), + middleware.AdminAuditEmit(db, cfg.AdminPathPrefix), + middleware.RequireAdmin(), + ) adminGroup.Get("/customers", adminCustH.List) adminGroup.Get("/customers/:team_id", adminCustH.Detail) adminGroup.Post("/customers/:team_id/tier", adminCustH.ChangeTier) diff --git a/main.go b/main.go index 1dccf7c8..a9c8e3c4 100644 --- a/main.go +++ b/main.go @@ -41,7 +41,13 @@ func main() { Level: slog.LevelInfo, AddSource: true, }) - slog.SetDefault(slog.New(logctx.NewHandler(serviceName, base))) + ctxH := logctx.NewHandler(serviceName, base) + // Default to a non-scrubbing handler. Once cfg.Load() resolves + // ADMIN_PATH_PREFIX below, we re-set the default with a Scrubber + // wrapped around the same context handler. Until then, any startup + // log line predates the admin-routes registration and can't possibly + // contain the prefix value anyway (the prefix is unread at this point). + slog.SetDefault(slog.New(ctxH)) shutdownTracer := telemetry.InitTracer("instant-api", os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT")) defer func() { @@ -61,6 +67,14 @@ func main() { cfg := config.Load() // panics on missing required env vars + // Re-set the slog default with the admin-prefix scrubber wrapped on the + // outside of the context handler. The scrubber runs LAST so any field + // (including ones stamped by middleware downstream) is rewritten before + // the JSON encoder sees it. NewLogScrubber returns the inner handler + // unchanged when cfg.AdminPathPrefix is empty — zero overhead when + // admin routes are disabled. + slog.SetDefault(slog.New(middleware.NewLogScrubber(ctxH, cfg.AdminPathPrefix))) + database := db.ConnectPostgres(cfg.DatabaseURL) defer database.Close()