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()