diff --git a/internal/handlers/agent_action_name_too_long_test.go b/internal/handlers/agent_action_name_too_long_test.go new file mode 100644 index 0000000..c4507fe --- /dev/null +++ b/internal/handlers/agent_action_name_too_long_test.go @@ -0,0 +1,54 @@ +package handlers + +// agent_action_name_too_long_test.go — BUG-AUTH-006 regression. +// +// The `name_too_long` agent_action sentence used to say "exceeds 64 +// characters" and "Shorten it to a short human label (1-64 chars)" — but +// the cap varies per endpoint: +// +// - resource names : 1-64 chars +// - PAT (API key) names : 1-120 chars +// - team names : 1-200 chars +// +// PATs hit 120 → message reads "must be 120 characters or fewer" → +// agent_action contradicts message → agent renders contradiction to user. +// Fix: keep agent_action cap-free; tell the agent to read the +// endpoint-specific limit from `message`. + +import ( + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAgentAction_NameTooLong_DoesNotBakeCap(t *testing.T) { + src, err := os.ReadFile("helpers.go") + require.NoError(t, err, "helpers.go must be readable from package dir") + body := string(src) + + // Scope the assertion to just the name_too_long registry entry — not + // other agent_action sentences in the same file. + idx := strings.Index(body, `"name_too_long":`) + require.GreaterOrEqual(t, idx, 0, "name_too_long entry not found in registry") + end := idx + 600 + if end > len(body) { + end = len(body) + } + window := body[idx:end] + + // BUG-AUTH-006: the sentence must NOT advertise "exceeds 64" or + // "(1-64 chars)" — those contradict the actual handler caps for PAT + // names (120) and team names (200). + assert.NotContains(t, window, "exceeds 64 characters", + "BUG-AUTH-006: agent_action must not bake the 64-char cap into the sentence") + assert.NotContains(t, window, "(1-64 chars)", + "BUG-AUTH-006: agent_action must not bake the 64-char range into the sentence") + + // Positive: the new sentence MUST tell the agent to read the + // per-endpoint cap from the `message` field. + assert.Contains(t, window, "message", + "BUG-AUTH-006: agent_action must direct the agent to read the limit from `message`") +} diff --git a/internal/handlers/auth_logout.go b/internal/handlers/auth_logout.go index cef2829..d966bae 100644 --- a/internal/handlers/auth_logout.go +++ b/internal/handlers/auth_logout.go @@ -94,12 +94,16 @@ func NewLogoutHandler(cfg *config.Config, rdb *redis.Client) *LogoutHandler { func (h *LogoutHandler) Logout(c *fiber.Ctx) error { requestID := middleware.GetRequestID(c) - // RequireAuth already validated the token — but we need the raw JWT to - // extract the jti and exp claims. Re-parse without secret validation is - // wrong; re-parse with the secret is the correct approach. + // BUG-AUTH-005: /auth/logout is idempotent per the OpenAPI contract + // ("safe to call without a valid token"). Missing / malformed / + // expired credentials therefore return 200 {ok:true} with no + // revocation work — the local token is already useless, so the + // dashboard's logout-on-expiry path must not surface a confusing 401 + // here. Tokens that DO parse cleanly carry a jti to revoke (the path + // below handles them). header := c.Get("Authorization") if len(header) < 8 || !strings.EqualFold(header[:7], "Bearer ") { - return respondError(c, fiber.StatusUnauthorized, "unauthorized", "Authorization header required") + return c.JSON(fiber.Map{"ok": true}) } tokenStr := header[7:] @@ -117,7 +121,8 @@ func (h *LogoutHandler) Logout(c *fiber.Ctx) error { return []byte(h.cfg.JWTSecret), nil }, jwt.WithValidMethods([]string{"HS256"})) if err != nil || !parsed.Valid { - return respondError(c, fiber.StatusUnauthorized, "unauthorized", "Token invalid or expired") + // BUG-AUTH-005: idempotent — token won't parse, nothing to revoke. + return c.JSON(fiber.Map{"ok": true}) } jti := claims.ID diff --git a/internal/handlers/auth_logout_coverage_test.go b/internal/handlers/auth_logout_coverage_test.go index 7bf12d8..410836c 100644 --- a/internal/handlers/auth_logout_coverage_test.go +++ b/internal/handlers/auth_logout_coverage_test.go @@ -72,6 +72,11 @@ func mintLogoutJWT(t *testing.T, jti string, expDelta time.Duration) string { return s } +// BUG-AUTH-005: /auth/logout is idempotent per the OpenAPI contract — +// "safe to call without a valid token." Missing / non-Bearer / wrong-secret +// credentials must return 200 {ok:true} (the local token is already +// useless, so the dashboard's logout-on-expiry path can't surface a +// confusing 401). Pre-fix all three returned 401. func TestLogout_MissingAuthorizationHeader(t *testing.T) { rdb, clean := setupCoverageRedis(t) defer clean() @@ -81,7 +86,7 @@ func TestLogout_MissingAuthorizationHeader(t *testing.T) { resp, err := app.Test(req, 5000) require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) + assert.Equal(t, http.StatusOK, resp.StatusCode, "BUG-AUTH-005: idempotent on missing auth") } func TestLogout_NonBearerAuthorization(t *testing.T) { @@ -94,7 +99,7 @@ func TestLogout_NonBearerAuthorization(t *testing.T) { resp, err := app.Test(req, 5000) require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) + assert.Equal(t, http.StatusOK, resp.StatusCode, "BUG-AUTH-005: idempotent on non-Bearer scheme") } func TestLogout_WrongSecretJWT(t *testing.T) { @@ -117,7 +122,7 @@ func TestLogout_WrongSecretJWT(t *testing.T) { resp, err := app.Test(req, 5000) require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) + assert.Equal(t, http.StatusOK, resp.StatusCode, "BUG-AUTH-005: idempotent on wrong-secret JWT (parse fail)") } func TestLogout_NoJTIIsNoOp(t *testing.T) { @@ -165,19 +170,24 @@ func TestLogout_HappyPath_WritesRedisRevocationKey(t *testing.T) { assert.LessOrEqual(t, ttl, 2*time.Hour+time.Second) } -func TestLogout_ExpiredButValidlySignedTokenStillRejectedByJWTParse(t *testing.T) { +// BUG-AUTH-005: an expired-but-validly-signed token is a no-op. The +// underlying jwt library refuses to parse it (exp in the past), so the +// handler treats it as "nothing to revoke" and returns 200 {ok:true}. +// Pre-fix this returned 401 — which broke the dashboard's +// logout-on-expiry path because the local token was already useless. +func TestLogout_ExpiredButValidlySignedTokenIsIdempotent(t *testing.T) { rdb, clean := setupCoverageRedis(t) defer clean() app := newLogoutApp(t, rdb) - // jwt library rejects expired tokens; the handler returns 401. tokenStr := mintLogoutJWT(t, uuid.New().String(), -time.Minute) req := httptest.NewRequest(http.MethodPost, "/auth/logout", nil) req.Header.Set("Authorization", "Bearer "+tokenStr) resp, err := app.Test(req, 5000) require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) + assert.Equal(t, http.StatusOK, resp.StatusCode, + "BUG-AUTH-005: expired token = no-op, returns 200 (idempotent)") } func TestLogout_RedisFailureReturns503(t *testing.T) { @@ -235,7 +245,12 @@ func TestLogout_TokenWithoutExpDefaultsTo24h(t *testing.T) { assert.LessOrEqual(t, ttl, 24*time.Hour+time.Second) } -func TestLogout_HS384RejectedAsUnexpectedSigningMethod(t *testing.T) { +// BUG-AUTH-005 + T10 P2-1: an HS384-signed token still fails the +// jwt.WithValidMethods(["HS256"]) gate at parse-time, so the handler +// treats it as "nothing to revoke" and returns 200 (idempotent contract). +// The alg pin is still enforced — a wrong-alg JWT never lands in the +// revocation set. Pre-AUTH-005 fix this returned 401. +func TestLogout_HS384TokenIsIdempotent(t *testing.T) { rdb, clean := setupCoverageRedis(t) defer clean() app := newLogoutApp(t, rdb) @@ -255,6 +270,13 @@ func TestLogout_HS384RejectedAsUnexpectedSigningMethod(t *testing.T) { resp, err := app.Test(req, 5000) require.NoError(t, err) defer resp.Body.Close() - assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, - "HS384 must be rejected; only HS256 is accepted") + assert.Equal(t, http.StatusOK, resp.StatusCode, + "BUG-AUTH-005: HS384 won't parse → idempotent 200, jti never revoked") + + // Verify the alg-pin still bars the jti from landing in Redis. + key := RevokedJTIKey(rc.ID) + exists, err := rdb.Exists(context.Background(), key).Result() + require.NoError(t, err) + assert.Equal(t, int64(0), exists, + "HS384 token must NOT result in a Redis revocation row (alg pin honoured)") } diff --git a/internal/handlers/helpers.go b/internal/handlers/helpers.go index 1d789d1..496d6e2 100644 --- a/internal/handlers/helpers.go +++ b/internal/handlers/helpers.go @@ -441,7 +441,12 @@ var codeToAgentAction = map[string]errorCodeMeta{ AgentAction: "Tell the user the confirm_slug field is required to confirm this destructive action — supply the slug exactly as shown in the prompt and retry — see https://instanode.dev/docs.", }, "name_too_long": { - AgentAction: "Tell the user the 'name' field exceeds 64 characters. Shorten it to a short human label (1-64 chars) and retry — see https://instanode.dev/docs.", + // BUG-AUTH-006: do NOT bake a numeric cap into this sentence — + // the cap varies per endpoint (resource names 1-64; PAT names + // up to 120; team names up to 200). Each handler's `message` + // field carries the endpoint-specific limit; agent_action just + // tells the agent to read that and shorten. + AgentAction: "Tell the user the 'name' field is too long. Read the exact limit from `message`, shorten the value to fit, and retry — see https://instanode.dev/docs.", }, "body_too_long": { AgentAction: "Tell the user the request body exceeded the per-endpoint cap. Shrink the payload — see https://instanode.dev/docs for per-endpoint limits.", diff --git a/internal/handlers/magic_link.go b/internal/handlers/magic_link.go index ee79f27..16dcfdf 100644 --- a/internal/handlers/magic_link.go +++ b/internal/handlers/magic_link.go @@ -393,9 +393,18 @@ func logMagicLinkSendResult(sendErr error, requestID string) { func (h *MagicLinkHandler) Callback(c *fiber.Ctx) error { requestID := middleware.GetRequestID(c) + // BUG-API-011: accept `?token=` as a fallback for `?t=` so a user who + // hand-typed (or an MCP tool that guessed) the longer param name still + // lands on the validation branch instead of "missing its token." The + // canonical magic-link URL we emit always uses `?t=` (15-char-long + // plaintext kept short for SMS-style copy-paste); `token` is purely a + // recovery alias and is never advertised. plaintext := strings.TrimSpace(c.Query("t")) if plaintext == "" { - return renderAuthError(c, fiber.StatusBadRequest, "Sign-in link is missing its token", "Open the link from your email exactly as we sent it.") + plaintext = strings.TrimSpace(c.Query("token")) + } + if plaintext == "" { + return renderAuthError(c, fiber.StatusBadRequest, "Sign-in link is missing its token", "Open the link from your email exactly as we sent it — the URL should include `?t=...`.") } hash := models.HashMagicLink(plaintext) diff --git a/internal/handlers/magic_link_token_alias_test.go b/internal/handlers/magic_link_token_alias_test.go new file mode 100644 index 0000000..c527941 --- /dev/null +++ b/internal/handlers/magic_link_token_alias_test.go @@ -0,0 +1,29 @@ +package handlers + +// magic_link_token_alias_test.go — BUG-API-011 regression. +// +// Pre-fix: GET /auth/email/callback?token= rendered +// "Sign-in link is missing its token" — but the token IS present, just +// under the longer param name. The canonical magic-link URL uses +// `?t=<plaintext>` (kept short for SMS-style copy-paste); `?token=` is a +// fallback alias that lets a hand-typing user / MCP tool that guessed +// the longer name still hit the validation branch. + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMagicLink_Callback_AcceptsTokenAlias(t *testing.T) { + src, err := os.ReadFile("magic_link.go") + require.NoError(t, err, "magic_link.go must be readable from package dir") + body := string(src) + + assert.Contains(t, body, `c.Query("t")`, + "BUG-API-011: canonical param `?t=` must still be read") + assert.Contains(t, body, `c.Query("token")`, + "BUG-API-011: `?token=` fallback must be read so the wrong-param-name UX no longer says 'missing'") +} diff --git a/internal/middleware/auth.go b/internal/middleware/auth.go index 75b83ee..c687eeb 100644 --- a/internal/middleware/auth.go +++ b/internal/middleware/auth.go @@ -96,6 +96,46 @@ const unauthorizedMessage = "Authentication required: missing, malformed, or exp // claims, invalid PAT). Kept as a single helper so adding RFC 6750 // WWW-Authenticate headers in a future PR happens in one place. func respondUnauthorized(c *fiber.Ctx) error { + return respondUnauthorizedWithReason(c, AuthErrorMissingCredentials) +} + +// AuthErrorReason names the specific 401 sub-classification an agent or +// dashboard can branch on. The top-level `error` keyword stays `unauthorized` +// for back-compat (any client matching on `error=="unauthorized"` continues +// to work); the sub-code lands in a new `error_code` field. Without this an +// agent inspecting a 401 from `/auth/me`, `/api/v1/whoami`, or any protected +// route had no way to distinguish "I never sent credentials" from "my JWT +// expired 30s ago" from "the signature is wrong" — they all rendered the +// same envelope (BUG-API-035/051). +type AuthErrorReason string + +const ( + // AuthErrorMissingCredentials — no `Authorization: Bearer ...` header. + // Remediation: log in to mint a session token. + AuthErrorMissingCredentials AuthErrorReason = "missing_credentials" + // AuthErrorMalformedToken — header present but the value is not a + // well-formed JWT/PAT (parse failure, wrong shape, signature mismatch). + // Remediation: log in to mint a fresh token; the existing one is + // corrupted, not just stale. + AuthErrorMalformedToken AuthErrorReason = "malformed_token" + // AuthErrorExpiredToken — JWT parsed, signature valid, but `exp` is in + // the past. Remediation: log in to mint a fresh token. + AuthErrorExpiredToken AuthErrorReason = "expired_token" + // AuthErrorInvalidClaims — JWT parsed and signature valid, but a + // required claim (uid/tid) is empty. Remediation: log in to mint a + // fresh, fully-populated token. + AuthErrorInvalidClaims AuthErrorReason = "invalid_claims" + // AuthErrorRevokedSession — token's jti is in the session-revocation + // set (the user explicitly logged out). Remediation: log in to mint a + // new session. + AuthErrorRevokedSession AuthErrorReason = "revoked_session" +) + +// respondUnauthorizedWithReason writes the canonical 401 envelope with an +// additional `error_code` sub-field carrying the AuthErrorReason. The +// top-level `error` keyword stays `unauthorized` so existing clients matching +// on it keep working unchanged. +func respondUnauthorizedWithReason(c *fiber.Ctx, reason AuthErrorReason) error { // B10 P2-4 (BugBash 2026-05-20): RFC 6750 §3 requires `WWW-Authenticate: // Bearer realm=...` on every 401 from a Bearer-protected resource. // Pre-fix only the audience-mismatch path set the header — every other @@ -108,6 +148,7 @@ func respondUnauthorized(c *fiber.Ctx) error { return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{ "ok": false, "error": "unauthorized", + "error_code": string(reason), "message": unauthorizedMessage, "request_id": GetRequestID(c), "retry_after_seconds": nil, @@ -266,7 +307,7 @@ func RequireAuth(cfg *config.Config) fiber.Handler { // would have to reason about. See PR #176 refactor note. header := c.Get("Authorization") if len(header) < 8 || header[:7] != "Bearer " { - return respondUnauthorized(c) + return respondUnauthorizedWithReason(c, AuthErrorMissingCredentials) } tokenStr := header[7:] @@ -276,7 +317,7 @@ func RequireAuth(cfg *config.Config) fiber.Handler { if IsAPIKey(tokenStr) { ok, err := AuthenticateAPIKey(c, tokenStr) if err != nil || !ok { - return respondUnauthorized(c) + return respondUnauthorizedWithReason(c, AuthErrorMalformedToken) } return c.Next() } @@ -296,11 +337,21 @@ func RequireAuth(cfg *config.Config) fiber.Handler { return []byte(cfg.JWTSecret), nil }, jwt.WithValidMethods([]string{"HS256"})) if err != nil || !parsed.Valid { - return respondUnauthorized(c) + // BUG-API-051: differentiate expired-vs-malformed so an agent + // can branch "refresh the session" from "ask the user to log + // in again." jwt/v4 returns *jwt.ValidationError whose Is() + // implementation matches the public ErrTokenExpired / + // ErrTokenNotValidYet sentinels; everything else is "the + // signature, alg, or shape was wrong" — that's malformed. + reason := AuthErrorMalformedToken + if errors.Is(err, jwt.ErrTokenExpired) || errors.Is(err, jwt.ErrTokenNotValidYet) { + reason = AuthErrorExpiredToken + } + return respondUnauthorizedWithReason(c, reason) } if claims.UserID == "" || claims.TeamID == "" { - return respondUnauthorized(c) + return respondUnauthorizedWithReason(c, AuthErrorInvalidClaims) } // RFC 8707 audience check — only enforced when the token actually @@ -323,7 +374,7 @@ func RequireAuth(cfg *config.Config) fiber.Handler { // Logged inside IsJTIRevoked; no additional log here to avoid duplication. _ = err } else if revoked { - return respondUnauthorized(c) + return respondUnauthorizedWithReason(c, AuthErrorRevokedSession) } } @@ -443,7 +494,7 @@ func optionalAuthImpl(cfg *config.Config, strict bool) fiber.Handler { } if len(header) < 8 || header[:7] != "Bearer " { if strict { - return respondUnauthorized(c) + return respondUnauthorizedWithReason(c, AuthErrorMalformedToken) } return c.Next() } @@ -470,7 +521,13 @@ func optionalAuthImpl(cfg *config.Config, strict bool) fiber.Handler { // Header present but JWT is bad — reject so the caller // learns their token is the problem instead of silently // downgrading to anonymous. T19 P1-7. - return respondUnauthorized(c) + reason := AuthErrorMalformedToken + if errors.Is(err, jwt.ErrTokenExpired) || errors.Is(err, jwt.ErrTokenNotValidYet) { + reason = AuthErrorExpiredToken + } else if err == nil && parsed != nil && parsed.Valid { + reason = AuthErrorInvalidClaims + } + return respondUnauthorizedWithReason(c, reason) } return c.Next() } diff --git a/internal/middleware/auth_error_code_test.go b/internal/middleware/auth_error_code_test.go new file mode 100644 index 0000000..84daca2 --- /dev/null +++ b/internal/middleware/auth_error_code_test.go @@ -0,0 +1,128 @@ +package middleware_test + +// auth_error_code_test.go — BUG-API-051 regression. +// +// RequireAuth's 401 envelope must carry an `error_code` sub-field that +// names which sub-case fired: +// +// - missing_credentials : no Authorization header / non-Bearer +// - malformed_token : header present but JWT/PAT won't parse +// - expired_token : JWT parsed cleanly but exp is in the past +// - invalid_claims : signature valid, uid/tid missing +// - revoked_session : jti in the session-revocation set +// +// Pre-fix every 401 from this middleware carried error=unauthorized with +// no sub-classification, so an agent had no way to branch "refresh the +// session" vs. "ask the user to log in again." We keep the top-level +// `error` keyword unchanged for back-compat. + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gofiber/fiber/v2" + "github.com/golang-jwt/jwt/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "instant.dev/internal/config" + "instant.dev/internal/middleware" +) + +const authErrorCodeTestSecret = "p1-bundle-test-secret-32-bytes!!" + +func newRequireAuthApp(t *testing.T) *fiber.App { + t.Helper() + cfg := &config.Config{JWTSecret: authErrorCodeTestSecret} + app := fiber.New() + app.Use(middleware.RequestID()) + app.Get("/protected", middleware.RequireAuth(cfg), func(c *fiber.Ctx) error { + return c.JSON(fiber.Map{"ok": true}) + }) + return app +} + +// readErrorEnvelope unmarshals the body of a 401 and returns the JSON map. +func readErrorEnvelope(t *testing.T, resp *http.Response) map[string]any { + t.Helper() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + var env map[string]any + require.NoError(t, json.Unmarshal(body, &env), "401 body must be JSON: %s", string(body)) + return env +} + +func TestRequireAuth_ErrorCode_MissingCredentials(t *testing.T) { + app := newRequireAuthApp(t) + + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) + env := readErrorEnvelope(t, resp) + assert.Equal(t, "unauthorized", env["error"], "top-level error keyword unchanged") + assert.Equal(t, "missing_credentials", env["error_code"], + "BUG-API-051: no Authorization header → error_code=missing_credentials") + assert.NotEmpty(t, env["request_id"], "request_id must be populated") + assert.NotEmpty(t, env["agent_action"]) +} + +func TestRequireAuth_ErrorCode_MalformedToken(t *testing.T) { + app := newRequireAuthApp(t) + + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + req.Header.Set("Authorization", "Bearer this.is.not-a-jwt") + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + env := readErrorEnvelope(t, resp) + assert.Equal(t, "malformed_token", env["error_code"], + "BUG-API-051: unparseable JWT → error_code=malformed_token") +} + +func TestRequireAuth_ErrorCode_ExpiredToken(t *testing.T) { + app := newRequireAuthApp(t) + + claims := jwt.MapClaims{ + "uid": "u-test", + "tid": "t-test", + "email": "test@example.com", + "jti": "jti-expired", + "iat": time.Now().Add(-2 * time.Hour).Unix(), + "exp": time.Now().Add(-time.Hour).Unix(), + } + tok := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) + s, err := tok.SignedString([]byte(authErrorCodeTestSecret)) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + req.Header.Set("Authorization", "Bearer "+s) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + env := readErrorEnvelope(t, resp) + assert.Equal(t, "expired_token", env["error_code"], + "BUG-API-051: expired JWT (exp in the past) → error_code=expired_token") +} + +func TestRequireAuth_ErrorCode_NonBearerScheme(t *testing.T) { + app := newRequireAuthApp(t) + + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + req.Header.Set("Authorization", "Basic dXNlcjpwYXNz") + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + env := readErrorEnvelope(t, resp) + assert.Equal(t, "missing_credentials", env["error_code"], + "non-Bearer scheme is treated as missing credentials (header isn't a Bearer at all)") +} diff --git a/internal/middleware/cors_preflight_allowlist.go b/internal/middleware/cors_preflight_allowlist.go new file mode 100644 index 0000000..392dc28 --- /dev/null +++ b/internal/middleware/cors_preflight_allowlist.go @@ -0,0 +1,98 @@ +package middleware + +import ( + "strings" + + "github.com/gofiber/fiber/v2" +) + +// cors_preflight_allowlist.go — close BUG-API-066 / BUG-API-067. +// +// Fiber's bundled CORS middleware (github.com/gofiber/fiber/v2/middleware/cors) +// sets Access-Control-Allow-Origin / -Methods / -Headers on the preflight +// response from its static Config, but does NOT cross-check the inbound +// Access-Control-Request-Method / Access-Control-Request-Headers against +// that allowlist. A browser (or a probing script) sending +// +// OPTIONS /any-route +// Origin: <legit> +// Access-Control-Request-Method: TRACE +// Access-Control-Request-Headers: Cookie +// +// still gets a 204 with Allow-Methods=GET,POST,...,OPTIONS even though +// TRACE and Cookie are absent from the allowlist. That is harmless on its +// own (a compliant browser would block the real request because TRACE +// isn't in the returned Allow-Methods), but it nudges security audits and +// vendor scanners to flag the API for "permissive preflight." It also +// teaches future maintainers the wrong model — that the preflight is a +// rubber stamp. +// +// PreflightAllowlist is a tiny pre-CORS gate. For OPTIONS requests carrying +// an Access-Control-Request-* header, it rejects (403) when: +// - the requested method is not in the allowed-methods list +// - any requested header is not in the allowed-headers list +// +// Same allowlist strings are passed in from router.go so the two stay in +// lockstep — no second source of truth to drift. Non-preflight OPTIONS +// (no Origin or no Access-Control-Request-Method) fall through unchanged. + +// PreflightAllowlist returns a Fiber handler that validates CORS preflight +// requests against the allowed-methods and allowed-headers lists. Pass the +// same comma-separated strings used in the downstream fiberCORS config. +func PreflightAllowlist(allowMethods, allowHeaders string) fiber.Handler { + methodSet := commaSet(allowMethods, true) // methods are case-insensitive + headerSet := commaSet(allowHeaders, false) // canonicalised lower-case + + return func(c *fiber.Ctx) error { + if c.Method() != fiber.MethodOptions { + return c.Next() + } + reqMethod := strings.TrimSpace(c.Get("Access-Control-Request-Method")) + if reqMethod == "" { + // Not a preflight (no AC-Request-Method) — let the CORS layer + // or downstream router decide. + return c.Next() + } + + // 1. Reject methods outside the allowlist (e.g. TRACE). + if _, ok := methodSet[strings.ToUpper(reqMethod)]; !ok { + return c.SendStatus(fiber.StatusForbidden) + } + + // 2. Reject headers outside the allowlist (e.g. Cookie, Authorization + // is fine because it's in the static config). The browser sends + // a comma-separated list in Access-Control-Request-Headers. + reqHeaders := c.Get("Access-Control-Request-Headers") + if reqHeaders != "" { + for _, h := range strings.Split(reqHeaders, ",") { + name := strings.ToLower(strings.TrimSpace(h)) + if name == "" { + continue + } + if _, ok := headerSet[name]; !ok { + return c.SendStatus(fiber.StatusForbidden) + } + } + } + return c.Next() + } +} + +// commaSet splits a comma-separated string into a set, trimming whitespace. +// When upper=true the keys are upper-cased; otherwise lower-cased. +func commaSet(raw string, upper bool) map[string]struct{} { + out := make(map[string]struct{}) + for _, tok := range strings.Split(raw, ",") { + t := strings.TrimSpace(tok) + if t == "" { + continue + } + if upper { + t = strings.ToUpper(t) + } else { + t = strings.ToLower(t) + } + out[t] = struct{}{} + } + return out +} diff --git a/internal/middleware/cors_preflight_allowlist_test.go b/internal/middleware/cors_preflight_allowlist_test.go new file mode 100644 index 0000000..347845c --- /dev/null +++ b/internal/middleware/cors_preflight_allowlist_test.go @@ -0,0 +1,150 @@ +package middleware_test + +// cors_preflight_allowlist_test.go — BUG-API-066 / BUG-API-067 regression. +// +// Fiber's CORS middleware sets Access-Control-Allow-* response headers +// but does NOT validate the inbound Access-Control-Request-Method / +// Access-Control-Request-Headers against the allowlist. A preflight +// asking for TRACE or `Cookie` therefore got a 204 with the allowlisted +// methods in the response — not a 403. PreflightAllowlist is a pre-CORS +// gate that rejects such preflights with 403. + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gofiber/fiber/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "instant.dev/internal/middleware" +) + +func newPreflightApp(t *testing.T) *fiber.App { + t.Helper() + app := fiber.New() + app.Use(middleware.PreflightAllowlist( + "GET,POST,PUT,PATCH,DELETE,OPTIONS", + "Content-Type,Authorization,X-Request-ID", + )) + app.Get("/whoami", func(c *fiber.Ctx) error { return c.JSON(fiber.Map{"ok": true}) }) + return app +} + +// BUG-API-066: a preflight asking for TRACE is rejected with 403. +func TestPreflightAllowlist_RejectsTRACEMethod(t *testing.T) { + app := newPreflightApp(t) + + req := httptest.NewRequest(http.MethodOptions, "/whoami", nil) + req.Header.Set("Origin", "https://instanode.dev") + req.Header.Set("Access-Control-Request-Method", "TRACE") + + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusForbidden, resp.StatusCode, + "BUG-API-066: TRACE preflight must be 403 (not 204)") +} + +// BUG-API-066: a preflight asking for CONNECT is also rejected (defence +// against the "any non-allowlisted method" class). +func TestPreflightAllowlist_RejectsCONNECTMethod(t *testing.T) { + app := newPreflightApp(t) + + req := httptest.NewRequest(http.MethodOptions, "/whoami", nil) + req.Header.Set("Origin", "https://instanode.dev") + req.Header.Set("Access-Control-Request-Method", "CONNECT") + + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusForbidden, resp.StatusCode, + "BUG-API-066: CONNECT preflight must be 403") +} + +// BUG-API-067: a preflight asking for `Cookie` in the request headers is +// rejected with 403. +func TestPreflightAllowlist_RejectsCookieHeader(t *testing.T) { + app := newPreflightApp(t) + + req := httptest.NewRequest(http.MethodOptions, "/whoami", nil) + req.Header.Set("Origin", "https://instanode.dev") + req.Header.Set("Access-Control-Request-Method", "POST") + req.Header.Set("Access-Control-Request-Headers", "Cookie") + + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusForbidden, resp.StatusCode, + "BUG-API-067: preflight with Cookie header must be 403") +} + +// BUG-API-067: even a mix of allowed + disallowed headers fails. +func TestPreflightAllowlist_RejectsMixedHeaders(t *testing.T) { + app := newPreflightApp(t) + + req := httptest.NewRequest(http.MethodOptions, "/whoami", nil) + req.Header.Set("Origin", "https://instanode.dev") + req.Header.Set("Access-Control-Request-Method", "POST") + req.Header.Set("Access-Control-Request-Headers", "Content-Type, Set-Cookie") + + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusForbidden, resp.StatusCode, + "BUG-API-067: any non-allowlisted header in the comma list is 403") +} + +// Sanity: a fully legitimate preflight passes through unchanged. +func TestPreflightAllowlist_AllowsLegitimatePreflight(t *testing.T) { + app := newPreflightApp(t) + + req := httptest.NewRequest(http.MethodOptions, "/whoami", nil) + req.Header.Set("Origin", "https://instanode.dev") + req.Header.Set("Access-Control-Request-Method", "POST") + req.Header.Set("Access-Control-Request-Headers", "Content-Type, Authorization") + + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + // No real CORS middleware downstream — OPTIONS falls through with + // no route → 404 or 405. The middleware must NOT 403. + assert.NotEqual(t, http.StatusForbidden, resp.StatusCode, + "legitimate preflight must not be 403") +} + +// Sanity: non-OPTIONS requests pass through without inspection. +func TestPreflightAllowlist_IgnoresGET(t *testing.T) { + app := newPreflightApp(t) + + req := httptest.NewRequest(http.MethodGet, "/whoami", nil) + req.Header.Set("Origin", "https://instanode.dev") + req.Header.Set("Access-Control-Request-Method", "TRACE") // not a real preflight + + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusOK, resp.StatusCode, + "non-preflight requests must not be inspected") +} + +// Sanity: an OPTIONS with no Access-Control-Request-Method is not a CORS +// preflight; it must fall through. +func TestPreflightAllowlist_IgnoresBareOPTIONS(t *testing.T) { + app := newPreflightApp(t) + + req := httptest.NewRequest(http.MethodOptions, "/whoami", nil) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.NotEqual(t, http.StatusForbidden, resp.StatusCode, + "bare OPTIONS (no Origin/AC-Request-Method) must not be 403") +} diff --git a/internal/middleware/cors_preflight_coverage_test.go b/internal/middleware/cors_preflight_coverage_test.go new file mode 100644 index 0000000..69a446e --- /dev/null +++ b/internal/middleware/cors_preflight_coverage_test.go @@ -0,0 +1,73 @@ +package middleware_test + +// cors_preflight_coverage_test.go — patch-coverage backfill for the two +// empty-token `continue` branches in cors_preflight_allowlist.go +// (lines 70 and 88) that the main BUG-API-066/067 test pair didn't hit. +// +// Both branches are reached when the comma-separated input contains an +// empty token between two non-empty values — a real browser would never +// emit that, but defensive parsing tolerates it. Without these tests the +// patch-coverage gate stays at 95% on this file. + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gofiber/fiber/v2" + "github.com/stretchr/testify/require" + + "instant.dev/internal/middleware" +) + +// TestPreflightAllowlist_SkipsEmptyHeaderToken exercises the inner +// `if name == "" { continue }` at cors_preflight_allowlist.go:70. The +// preflight asks for `Authorization,, Content-Type` (note the double +// comma producing an empty middle token) which must NOT 403 — the empty +// token is skipped and both real headers pass. +func TestPreflightAllowlist_SkipsEmptyHeaderToken(t *testing.T) { + app := fiber.New() + app.Use(middleware.PreflightAllowlist( + "GET,POST,OPTIONS", + "Content-Type,Authorization", + )) + app.Get("/x", func(c *fiber.Ctx) error { return c.JSON(fiber.Map{"ok": true}) }) + + req := httptest.NewRequest(http.MethodOptions, "/x", nil) + req.Header.Set("Origin", "https://instanode.dev") + req.Header.Set("Access-Control-Request-Method", "GET") + req.Header.Set("Access-Control-Request-Headers", "Authorization,, Content-Type") + + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + require.NotEqual(t, http.StatusForbidden, resp.StatusCode, + "empty middle token in AC-Request-Headers must be skipped, not rejected") +} + +// TestPreflightAllowlist_SkipsEmptyAllowlistToken exercises the inner +// `if t == "" { continue }` of commaSet at cors_preflight_allowlist.go:88. +// Constructed by passing an allowMethods string with a double-comma so +// commaSet must skip the empty entry. +func TestPreflightAllowlist_SkipsEmptyAllowlistToken(t *testing.T) { + app := fiber.New() + // Note the double comma in the methods list — commaSet must skip it + // so "GET" still registers as allowed. + app.Use(middleware.PreflightAllowlist( + "GET,,POST,OPTIONS", + "Content-Type", + )) + app.Get("/x", func(c *fiber.Ctx) error { return c.JSON(fiber.Map{"ok": true}) }) + + req := httptest.NewRequest(http.MethodOptions, "/x", nil) + req.Header.Set("Origin", "https://instanode.dev") + req.Header.Set("Access-Control-Request-Method", "GET") + + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + + require.NotEqual(t, http.StatusForbidden, resp.StatusCode, + "GET must remain allowed even when allowMethods has a stray empty token") +} diff --git a/internal/middleware/idempotency.go b/internal/middleware/idempotency.go index d35c510..d60212b 100644 --- a/internal/middleware/idempotency.go +++ b/internal/middleware/idempotency.go @@ -297,10 +297,23 @@ func idempotencyExplicit(c *fiber.Ctx, rdb *redis.Client, endpoint, scope, rawKe // refund the rate-limit counter here. The agent did the // wrong thing (reused a key for a different body) and // should still pay the cost of that mistake. + // + // BUG-API-013/406: every 4xx must carry the canonical + // envelope — request_id + agent_action — so an agent + // inspecting this 409 gets the same shape as the + // handler-emitted 400/402/etc. branches. The agent_action + // tells the agent to mint a NEW Idempotency-Key for the + // new body, not retry the same key (which would keep + // returning 409). retry_after_seconds is null: the + // conflict is permanent for this (key, body) pair — only + // re-keying resolves it. return c.Status(fiber.StatusConflict).JSON(fiber.Map{ - "ok": false, - "error": "idempotency_key_conflict", - "message": "Idempotency-Key already used with a different body", + "ok": false, + "error": "idempotency_key_conflict", + "message": "Idempotency-Key already used with a different body", + "request_id": GetRequestID(c), + "retry_after_seconds": nil, + "agent_action": "Tell the user this Idempotency-Key is already bound to a different request body. Mint a NEW Idempotency-Key (any RFC 4122 UUID) for the new body and retry — see https://instanode.dev/docs/idempotency.", }) } // Cache HIT — refund the rate-limit slot RateLimit burned on diff --git a/internal/middleware/idempotency_envelope_test.go b/internal/middleware/idempotency_envelope_test.go new file mode 100644 index 0000000..66a4f9f --- /dev/null +++ b/internal/middleware/idempotency_envelope_test.go @@ -0,0 +1,51 @@ +package middleware_test + +// idempotency_envelope_test.go — BUG-API-013 / BUG-API-406 regression. +// +// The 409 returned when an agent reuses an Idempotency-Key with a +// different body MUST carry the canonical envelope (ok, error, message, +// request_id, retry_after_seconds, agent_action) — pre-fix it only +// carried ok/error/message, which left an agent with no request_id to +// quote to support and no agent_action sentence to render to the user. +// +// Static-source assertion: we don't spin a Redis fake here (the +// envelope shape is what the rule-22 surface checklist wants protected; +// the full integration path is exercised in idempotency_test.go). + +import ( + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIdempotency409EnvelopeShape_DocumentedFields(t *testing.T) { + src, err := os.ReadFile("idempotency.go") + require.NoError(t, err, "must read idempotency.go from package dir") + body := string(src) + + wantFields := []string{ + `"ok"`, + `"error"`, + `"message"`, + `"request_id"`, + `"retry_after_seconds"`, + `"agent_action"`, + } + for _, f := range wantFields { + assert.Contains(t, body, f, + "BUG-API-013: idempotency.go must emit envelope field %s on 409", f) + } + // The agent_action sentence must name the canonical resolution + // (mint a new key) so the agent can self-recover. + assert.Contains(t, body, "Mint a NEW Idempotency-Key", + "BUG-API-013: 409 agent_action must tell the agent to mint a new key") + // Sanity: idempotency_key_conflict error code is still the keyword + // (back-compat — clients matching on `error` keep working). + assert.Contains(t, body, `"idempotency_key_conflict"`, + "top-level error keyword unchanged for client back-compat") + // Belt: no orphan call sites still using the old 4-field shape. + assert.Greater(t, strings.Count(body, "request_id"), 0) +} diff --git a/internal/middleware/optional_auth_strict_invalid_claims_test.go b/internal/middleware/optional_auth_strict_invalid_claims_test.go new file mode 100644 index 0000000..1a79677 --- /dev/null +++ b/internal/middleware/optional_auth_strict_invalid_claims_test.go @@ -0,0 +1,113 @@ +package middleware_test + +// optional_auth_strict_invalid_claims_test.go — patch-coverage backfill +// for the AuthErrorInvalidClaims branch added in PR #178 (BUG-API-051). +// +// auth.go: a token whose signature is valid AND `parsed.Valid` is true but +// either `UserID` or `TeamID` claim is empty falls through to: +// reason = AuthErrorInvalidClaims +// That branch is reached only when the JWT is well-formed enough to pass +// jwt-go's parse but still missing required claims. Existing strict tests +// cover Garbage, Expired, WrongSecret, NonBearer; none constructs a valid +// signature with empty uid/tid. + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + jwt "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "instant.dev/internal/testhelpers" +) + +// signSessionEmptyTeam returns a JWT with valid signature + valid exp but +// `tid` (team_id) blank — exercises the InvalidClaims arm. +func signSessionEmptyTeam(t *testing.T, secret, userID string) string { + t.Helper() + claims := jwt.MapClaims{ + "uid": userID, + "tid": "", + "jti": uuid.NewString(), + "iat": time.Now().Unix(), + "exp": time.Now().Add(time.Hour).Unix(), + } + tok, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(secret)) + require.NoError(t, err) + return tok +} + +// signSessionEmptyUID returns a JWT with valid signature but empty uid. +func signSessionEmptyUID(t *testing.T, secret, teamID string) string { + t.Helper() + claims := jwt.MapClaims{ + "uid": "", + "tid": teamID, + "jti": uuid.NewString(), + "iat": time.Now().Unix(), + "exp": time.Now().Add(time.Hour).Unix(), + } + tok, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(secret)) + require.NoError(t, err) + return tok +} + +// TestOptionalAuthStrict_EmptyTeamID_InvalidClaims — valid signature, empty +// `tid` → 401 in strict mode. +func TestOptionalAuthStrict_EmptyTeamID_InvalidClaims(t *testing.T) { + tok := signSessionEmptyTeam(t, testhelpers.TestJWTSecret, uuid.NewString()) + app := newOptionalAuthStrictApp(testhelpers.TestJWTSecret) + req := httptest.NewRequest(http.MethodGet, "/test", nil) + req.Header.Set("Authorization", "Bearer "+tok) + + resp, err := app.Test(req, 1000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, + "valid-sig but empty tid must 401 in strict mode (InvalidClaims branch)") +} + +// TestOptionalAuthStrict_EmptyUserID_InvalidClaims — valid signature, empty +// `uid` → 401 in strict mode. +func TestOptionalAuthStrict_EmptyUserID_InvalidClaims(t *testing.T) { + tok := signSessionEmptyUID(t, testhelpers.TestJWTSecret, uuid.NewString()) + app := newOptionalAuthStrictApp(testhelpers.TestJWTSecret) + req := httptest.NewRequest(http.MethodGet, "/test", nil) + req.Header.Set("Authorization", "Bearer "+tok) + + resp, err := app.Test(req, 1000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, + "valid-sig but empty uid must 401 in strict mode (InvalidClaims branch)") +} + +// TestOptionalAuthStrict_BothEmpty_InvalidClaims — both empty → still 401. +func TestOptionalAuthStrict_BothEmpty_InvalidClaims(t *testing.T) { + claims := jwt.MapClaims{ + "uid": "", + "tid": "", + "jti": uuid.NewString(), + "iat": time.Now().Unix(), + "exp": time.Now().Add(time.Hour).Unix(), + } + tok, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(testhelpers.TestJWTSecret)) + require.NoError(t, err) + + app := newOptionalAuthStrictApp(testhelpers.TestJWTSecret) + req := httptest.NewRequest(http.MethodGet, "/test", nil) + req.Header.Set("Authorization", "Bearer "+tok) + + resp, err := app.Test(req, 1000) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, + "valid-sig with both empty claims must 401") +} diff --git a/internal/router/router.go b/internal/router/router.go index c33a6af..70957cb 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -187,13 +187,23 @@ func NewWithHooks(cfg *config.Config, db *sql.DB, rdb *redis.Client, geoDbs *mid if cfg.Environment == "development" { corsAllowOrigins += ",http://localhost:5173,http://localhost:3000,http://localhost:5174" } + const corsAllowMethods = "GET,POST,PUT,PATCH,DELETE,OPTIONS" + const corsAllowHeaders = "Content-Type,Authorization,X-Request-ID,X-E2E-Test-Token,X-E2E-Source-IP" + // BUG-API-066/067: Fiber's CORS middleware sets Access-Control-Allow-* + // headers but does NOT validate the inbound preflight request — a + // browser asking for TRACE or Cookie still gets a 204 even though + // neither is in the allowlist. We pre-empt that by walking the + // preflight headers and 403'ing any value not in our allowlist BEFORE + // CORS responds. Same allowlist as the CORS Config below so the two + // stay in lockstep. + app.Use(middleware.PreflightAllowlist(corsAllowMethods, corsAllowHeaders)) app.Use(fiberCORS.New(fiberCORS.Config{ // Production origin (GitHub Pages serves instanode.dev). Local-dev // ports are appended only in development (see corsAllowOrigins above) // so the prod allowlist stays auditable and localhost-free. AllowOrigins: corsAllowOrigins, - AllowMethods: "GET,POST,PUT,PATCH,DELETE,OPTIONS", - AllowHeaders: "Content-Type,Authorization,X-Request-ID,X-E2E-Test-Token,X-E2E-Source-IP", + AllowMethods: corsAllowMethods, + AllowHeaders: corsAllowHeaders, ExposeHeaders: "X-Request-ID,X-Instant-Upgrade,X-Instant-Notice", })) app.Use(middleware.GeoEnrich(geoDbs)) @@ -628,9 +638,16 @@ func NewWithHooks(cfg *config.Config, db *sql.DB, rdb *redis.Client, geoDbs *mid // SetRevocationDB wires the Redis client into the middleware package once // so every RequireAuth call can query it without threading rdb through // every handler constructor. + // + // BUG-AUTH-005: per the OpenAPI contract, POST /auth/logout is + // idempotent — "safe to call without a valid token." We therefore do + // NOT gate it on RequireAuth; the handler itself returns 200 {ok:true} + // for missing/malformed/expired credentials (the local token is already + // useless, so the dashboard's logout-on-expiry hitting this surface + // must not 401). Tokens that DO parse cleanly are revoked normally. middleware.SetRevocationDB(rdb) logoutH := handlers.NewLogoutHandler(cfg, rdb) - app.Post("/auth/logout", middleware.RequireAuth(cfg), logoutH.Logout) + app.Post("/auth/logout", logoutH.Logout) // Billing billing := handlers.NewBillingHandler(db, cfg, breakingMailer)