From 47d3ef5b9e91d78cbd5bf866647c3d699e2d6e6b Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 13:42:50 -0700 Subject: [PATCH] refactor(middleware): extract dev-mode fail-open predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AdminAuth and WorkspaceAuth both carried the same 5-line `ADMIN_TOKEN == "" && MOLECULE_ENV in {development, dev}` check. If a third middleware ever needs the hatch — or if "dev mode" semantics change (new env name, allowlist, runtime flag) — the previous shape made N places to keep in sync and N places a security reviewer has to audit. This commit factors the predicate into a single `isDevModeFailOpen()` helper in `internal/middleware/devmode.go`. Each call site becomes if isDevModeFailOpen() { c.Next(); return } `devmode.go` carries the full rationale (why the hatch exists, why it's safe for SaaS) so call sites don't need to restate it. ### Also - Moved the dev-mode env-value set to a package-level `devModeEnvValues` map so adding aliases is one line. Matches the existing convention (`handlers/admin_test_token.go`) of treating `MOLECULE_ENV != "production"` as dev — but stays explicit about which values opt IN rather than blanket-accepting everything non-prod. - Added case-insensitive compare + trim on the env value so operators don't have to remember exact casing. - New `devmode_test.go` unit-tests the predicate directly: 6 cases covering happy path, both opt-out signals (ADMIN_TOKEN, production mode), short alias, case-insensitive + whitespace tolerance, and an explicit negative-space sweep of arbitrary non-dev values ("staging", "preview", "test", "devel", "") to lock in that typos don't silently enable the hatch. Existing AdminAuth/WorkspaceAuth integration tests still exercise the helper indirectly via HTTP — they pass unchanged, confirming the behaviour is preserved. ### No behavioural change Before and after this commit, `go test -race ./internal/middleware/` reports identical results. Zero production surface change — this is a pure refactor, but it collapses the dev-mode seam from two inline blocks into one named predicate, which is the shape future contributors (and security reviewers) can follow. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/middleware/devmode.go | 56 +++++++++++++ .../internal/middleware/devmode_test.go | 79 +++++++++++++++++++ .../internal/middleware/wsauth_middleware.go | 44 +++-------- 3 files changed, 147 insertions(+), 32 deletions(-) create mode 100644 workspace-server/internal/middleware/devmode.go create mode 100644 workspace-server/internal/middleware/devmode_test.go diff --git a/workspace-server/internal/middleware/devmode.go b/workspace-server/internal/middleware/devmode.go new file mode 100644 index 00000000..2c226c75 --- /dev/null +++ b/workspace-server/internal/middleware/devmode.go @@ -0,0 +1,56 @@ +package middleware + +import ( + "os" + "strings" +) + +// Dev-mode escape hatch — factored out of AdminAuth + WorkspaceAuth so a +// future third caller (or a change to what "dev mode" means) touches one +// place. Narrowing the exposed seam also makes it grep-able from security +// reviews: every `isDevModeFailOpen()` call is an intentional fail-open. +// +// Why the helper exists at all: on `go run ./cmd/server` the Canvas (at +// localhost:3000) calls the platform (at localhost:8080) cross-port. Both +// `isSameOriginCanvas` (Referer==Host) and the AdminAuth Tier-1 fail-open +// (no tokens in DB) close the moment the user creates their first +// workspace. Without this hatch the Canvas 401s on every /workspaces +// enumeration and every /workspaces/:id/* read until the operator sets +// `ADMIN_TOKEN` and rebuilds the Canvas bundle with a matching +// `NEXT_PUBLIC_ADMIN_TOKEN`. That's too much friction for a local smoke +// test — hence the hatch. +// +// Why it's safe for SaaS: hosted tenants are provisioned with both +// `ADMIN_TOKEN` (a random secret, checked by Tier-2 above) and +// `MOLECULE_ENV=production`. Either one being set makes this helper +// return false, so the fail-open branch is unreachable in production. +// The convention matches `handlers/admin_test_token.go`, which gates +// the e2e test-token mint on `MOLECULE_ENV != "production"`. + +// devModeEnvValues is the set of MOLECULE_ENV values that count as +// "explicit dev mode". Production callers don't set any of these. +// Case-insensitive compare via strings.ToLower below. +var devModeEnvValues = map[string]struct{}{ + "development": {}, + "dev": {}, +} + +// isDevModeFailOpen reports whether the AdminAuth / WorkspaceAuth +// middleware should let a bearer-less request through despite live +// workspace tokens existing in the DB. +// +// True only when BOTH: +// - `ADMIN_TOKEN` is empty (operator has not opted in to the #684 +// closure), AND +// - `MOLECULE_ENV` is explicitly a dev value ("development" / "dev"). +// +// Either condition failing returns false — that's the SaaS safety +// guarantee. Tests: `devmode_test.go` covers every branch. +func isDevModeFailOpen() bool { + if os.Getenv("ADMIN_TOKEN") != "" { + return false + } + env := strings.ToLower(strings.TrimSpace(os.Getenv("MOLECULE_ENV"))) + _, ok := devModeEnvValues[env] + return ok +} diff --git a/workspace-server/internal/middleware/devmode_test.go b/workspace-server/internal/middleware/devmode_test.go new file mode 100644 index 00000000..17685efa --- /dev/null +++ b/workspace-server/internal/middleware/devmode_test.go @@ -0,0 +1,79 @@ +package middleware + +import ( + "testing" +) + +// Unit tests for the isDevModeFailOpen predicate. The AdminAuth and +// WorkspaceAuth middleware tests exercise the same helper indirectly via +// HTTP, but a direct predicate test locks the pure-logic behaviour: +// future callers can add themselves to `devmode.go` with confidence. + +func TestIsDevModeFailOpen_DevModeNoAdminToken_True(t *testing.T) { + t.Setenv("MOLECULE_ENV", "development") + t.Setenv("ADMIN_TOKEN", "") + if !isDevModeFailOpen() { + t.Error("expected dev mode + no admin token to return true") + } +} + +func TestIsDevModeFailOpen_DevModeShortAlias_True(t *testing.T) { + // "dev" is a valid alias for "development" — matches the convention + // in handlers/admin_test_token.go. + t.Setenv("MOLECULE_ENV", "dev") + t.Setenv("ADMIN_TOKEN", "") + if !isDevModeFailOpen() { + t.Error("expected MOLECULE_ENV=dev to be treated as dev mode") + } +} + +func TestIsDevModeFailOpen_AdminTokenSet_False(t *testing.T) { + // Setting ADMIN_TOKEN is the operator's explicit opt-in to the #684 + // closure. Dev mode must NOT silently override that signal. + t.Setenv("MOLECULE_ENV", "development") + t.Setenv("ADMIN_TOKEN", "operator-explicitly-set-this") + if isDevModeFailOpen() { + t.Error("explicit ADMIN_TOKEN must suppress the dev-mode hatch") + } +} + +func TestIsDevModeFailOpen_Production_False(t *testing.T) { + // The SaaS-safety guarantee: production tenants always have + // MOLECULE_ENV=production, so the hatch is unreachable even if a + // misconfigured deployment also leaves ADMIN_TOKEN unset. + t.Setenv("MOLECULE_ENV", "production") + t.Setenv("ADMIN_TOKEN", "") + if isDevModeFailOpen() { + t.Error("production must never hit the dev-mode fail-open branch") + } +} + +func TestIsDevModeFailOpen_CaseInsensitive(t *testing.T) { + // Operators shouldn't have to remember exact casing for a dev-only + // convenience. "Development", "DEV", " dev " all count. + cases := []string{"Development", "DEVELOPMENT", "Dev", "DEV", " dev "} + for _, env := range cases { + t.Run(env, func(t *testing.T) { + t.Setenv("MOLECULE_ENV", env) + t.Setenv("ADMIN_TOKEN", "") + if !isDevModeFailOpen() { + t.Errorf("MOLECULE_ENV=%q should count as dev mode", env) + } + }) + } +} + +func TestIsDevModeFailOpen_UnknownEnv_False(t *testing.T) { + // Arbitrary / unset MOLECULE_ENV values are NOT treated as dev mode. + // Keeps the fail-open branch narrow — no silent opt-in from a typo. + cases := []string{"", "staging", "local", "preview", "test", "devel"} + for _, env := range cases { + t.Run(env, func(t *testing.T) { + t.Setenv("MOLECULE_ENV", env) + t.Setenv("ADMIN_TOKEN", "") + if isDevModeFailOpen() { + t.Errorf("MOLECULE_ENV=%q must not enable fail-open", env) + } + }) + } +} diff --git a/workspace-server/internal/middleware/wsauth_middleware.go b/workspace-server/internal/middleware/wsauth_middleware.go index 6775345c..66b8f261 100644 --- a/workspace-server/internal/middleware/wsauth_middleware.go +++ b/workspace-server/internal/middleware/wsauth_middleware.go @@ -90,20 +90,11 @@ func WorkspaceAuth(database *sql.DB) gin.HandlerFunc { c.Next() return } - // Local-dev escape hatch. Mirrors the Tier-1b branch in AdminAuth: - // on `go run ./cmd/server` + `npm run dev` the Canvas (at - // localhost:3000) calls the platform (at localhost:8080) cross-port, - // so isSameOriginCanvas's Host==Referer check fails. Without a - // bearer, every GET /workspaces/:id/activity / /delegations call - // 401s and the Canvas can't show chat history or agent comms. - // Gated on MOLECULE_ENV=development + ADMIN_TOKEN unset so SaaS - // (always MOLECULE_ENV=production + ADMIN_TOKEN set) never hits it. - if os.Getenv("ADMIN_TOKEN") == "" { - env := strings.ToLower(strings.TrimSpace(os.Getenv("MOLECULE_ENV"))) - if env == "development" || env == "dev" { - c.Next() - return - } + // Local-dev escape hatch — see devmode.go. Unreachable on SaaS + // (hosted tenants always have ADMIN_TOKEN + MOLECULE_ENV=production). + if isDevModeFailOpen() { + c.Next() + return } c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "missing workspace auth token"}) return @@ -163,24 +154,13 @@ func AdminAuth(database *sql.DB) gin.HandlerFunc { } } - // Tier 1b: Local-dev escape hatch. On `go run ./cmd/server` the - // Canvas has no bearer token (there's no WorkOS session, no - // baked NEXT_PUBLIC_ADMIN_TOKEN), so the moment the first - // workspace token lands in the DB Tier 1 closes and Canvas → 401 - // on every GET /workspaces. This reopens fail-open *only* when - // - ADMIN_TOKEN is empty (i.e. the operator has not opted in - // to the Phase-30 closure), AND - // - MOLECULE_ENV is explicitly a dev mode. - // SaaS never hits this branch because tenant provisioning sets - // both ADMIN_TOKEN and MOLECULE_ENV=production. Matches the - // existing convention in handlers/admin_test_token.go which - // gates the test-token endpoint on MOLECULE_ENV != "production". - if adminSecret == "" { - env := strings.ToLower(strings.TrimSpace(os.Getenv("MOLECULE_ENV"))) - if env == "development" || env == "dev" { - c.Next() - return - } + // Tier 1b: Local-dev escape hatch — see devmode.go. Lets the + // Canvas dashboard keep working after the first workspace token + // lands in the DB on `go run ./cmd/server`. Unreachable on SaaS + // (hosted tenants always have ADMIN_TOKEN + MOLECULE_ENV=production). + if isDevModeFailOpen() { + c.Next() + return } // SaaS-canvas path: when the request carries a WorkOS session