From 7a16eb4f70a32dfd15fdab2d2f4de0e15719727f Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 15 Apr 2026 11:09:16 -0700 Subject: [PATCH] =?UTF-8?q?fix(auth):=20#168=20=E2=80=94=20CanvasOrBearer?= =?UTF-8?q?=20middleware=20for=20PUT=20/canvas/viewport=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #168 by the route-split path from #194's review. #167 put PUT /canvas/viewport behind strict AdminAuth, breaking canvas drag/zoom persist because the canvas uses session cookies not bearer tokens. New narrow middleware CanvasOrBearer: - Accepts a valid bearer (same contract as AdminAuth) OR - Accepts a request whose Origin exactly matches CORS_ORIGINS - Lazy-bootstrap fail-open preserved for fresh installs Applied ONLY to PUT /canvas/viewport. The softer check is acceptable there because viewport corruption is cosmetic-only — worst case a user refreshes the page. This middleware must NOT be used on routes that leak prompts (#165), create resources (#164), or write files (#190) — see #194 review for why. The other canvas-facing routes mentioned in #168 (Events tab, Bundle Export/Import) remain behind strict AdminAuth pending a proper session-cookie-accepting AdminAuth (#168 follow-up for Phase H). 6 new tests cover: bootstrap fail-open, no-creds 401, canvas origin match, wrong origin 401, empty origin rejected, localhost default. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../internal/middleware/wsauth_middleware.go | 81 ++++++++++++ .../middleware/wsauth_middleware_test.go | 123 ++++++++++++++++++ platform/internal/router/router.go | 17 +-- 3 files changed, 213 insertions(+), 8 deletions(-) diff --git a/platform/internal/middleware/wsauth_middleware.go b/platform/internal/middleware/wsauth_middleware.go index e74aefac..9eee64f7 100644 --- a/platform/internal/middleware/wsauth_middleware.go +++ b/platform/internal/middleware/wsauth_middleware.go @@ -4,6 +4,8 @@ import ( "database/sql" "log" "net/http" + "os" + "strings" "github.com/Molecule-AI/molecule-monorepo/platform/internal/wsauth" "github.com/gin-gonic/gin" @@ -84,3 +86,82 @@ func AdminAuth(database *sql.DB) gin.HandlerFunc { c.Next() } } + +// CanvasOrBearer is a softer admin-auth variant used ONLY for cosmetic +// canvas routes where forging the request has zero security impact (PUT +// /canvas/viewport: worst case an attacker resets the shared viewport +// position, user refreshes the page, problem solved). +// +// Accepts either: +// +// 1. A valid bearer token (same contract as AdminAuth) — covers molecli, +// agent-to-platform calls, and anyone using the API directly. +// 2. A browser Origin header that matches CORS_ORIGINS (canvas itself). +// This is NOT a strict auth boundary — curl can forge Origin — but for +// cosmetic-only routes the trade-off is acceptable. Non-cosmetic routes +// MUST NOT use this middleware (see #194 review on why it would re-open +// #164 CRITICAL if applied to /bundles/import). +// +// Lazy-bootstrap fail-open preserved: zero-token installs pass everything +// through so fresh self-hosted / dev sessions aren't bricked. +func CanvasOrBearer(database *sql.DB) gin.HandlerFunc { + return func(c *gin.Context) { + ctx := c.Request.Context() + + hasLive, err := wsauth.HasAnyLiveTokenGlobal(ctx, database) + if err != nil { + log.Printf("wsauth: CanvasOrBearer HasAnyLiveTokenGlobal failed: %v — allowing request", err) + c.Next() + return + } + if !hasLive { + c.Next() + return + } + + // Path 1: valid bearer. + if tok := wsauth.BearerTokenFromHeader(c.GetHeader("Authorization")); tok != "" { + if err := wsauth.ValidateAnyToken(ctx, database, tok); err == nil { + c.Next() + return + } + } + + // Path 2: canvas origin match. Read CORS_ORIGINS at request time so + // tests can override via t.Setenv. canvasOriginAllowed returns true + // iff Origin is non-empty AND exactly matches one of the configured + // origins. Empty Origin (same-origin / server-to-server) does NOT + // pass this check — those callers must use the bearer path. + if canvasOriginAllowed(c.GetHeader("Origin")) { + c.Next() + return + } + + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "admin auth required"}) + } +} + +// canvasOriginAllowed returns true if origin matches any entry in the +// CORS_ORIGINS env var (comma-separated) or the localhost defaults. +// Exact-match only; no prefix or wildcard logic — that's handled by the +// real CORS middleware upstream. The intent here is "did this request come +// from the canvas page the user is already logged into?" — a binary check. +func canvasOriginAllowed(origin string) bool { + if origin == "" { + return false + } + allowed := []string{"http://localhost:3000", "http://localhost:3001"} + if v := os.Getenv("CORS_ORIGINS"); v != "" { + for _, o := range strings.Split(v, ",") { + if o = strings.TrimSpace(o); o != "" { + allowed = append(allowed, o) + } + } + } + for _, a := range allowed { + if a == origin { + return true + } + } + return false +} diff --git a/platform/internal/middleware/wsauth_middleware_test.go b/platform/internal/middleware/wsauth_middleware_test.go index 5fbb861f..783065a4 100644 --- a/platform/internal/middleware/wsauth_middleware_test.go +++ b/platform/internal/middleware/wsauth_middleware_test.go @@ -678,3 +678,126 @@ func TestAdminAuth_Issue120_PatchWorkspace_NoBearer_Returns401(t *testing.T) { t.Errorf("unmet sqlmock expectations: %v", err) } } + +// ── CanvasOrBearer (#168) ──────────────────────────────────────────────────── +// Narrow softer variant of AdminAuth used ONLY on PUT /canvas/viewport. +// Accepts bearer or a matching Origin header. MUST NOT be used anywhere a +// forged request would leak data or create resources. + +func TestCanvasOrBearer_NoTokens_FailOpen(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock: %v", err) + } + defer mockDB.Close() + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0)) + + r := gin.New() + r.PUT("/canvas/viewport", CanvasOrBearer(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"ok": true}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodPut, "/canvas/viewport", nil) + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("bootstrap fail-open: got %d, want 200 (%s)", w.Code, w.Body.String()) + } +} + +func TestCanvasOrBearer_TokensExist_NoCreds_Returns401(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock: %v", err) + } + defer mockDB.Close() + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + r := gin.New() + r.PUT("/canvas/viewport", CanvasOrBearer(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"ok": true}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodPut, "/canvas/viewport", nil) + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("no creds: got %d, want 401", w.Code) + } +} + +func TestCanvasOrBearer_TokensExist_CanvasOrigin_Passes(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock: %v", err) + } + defer mockDB.Close() + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + t.Setenv("CORS_ORIGINS", "https://acme.moleculesai.app,https://bob.moleculesai.app") + + r := gin.New() + r.PUT("/canvas/viewport", CanvasOrBearer(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"ok": true}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodPut, "/canvas/viewport", nil) + req.Header.Set("Origin", "https://acme.moleculesai.app") + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("canvas origin: got %d, want 200 (%s)", w.Code, w.Body.String()) + } +} + +func TestCanvasOrBearer_TokensExist_WrongOrigin_Returns401(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock: %v", err) + } + defer mockDB.Close() + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + t.Setenv("CORS_ORIGINS", "https://acme.moleculesai.app") + + r := gin.New() + r.PUT("/canvas/viewport", CanvasOrBearer(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"ok": true}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodPut, "/canvas/viewport", nil) + req.Header.Set("Origin", "https://evil.example.com") + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("wrong origin: got %d, want 401", w.Code) + } +} + +func TestCanvasOriginAllowed_EmptyOriginRejected(t *testing.T) { + if canvasOriginAllowed("") { + t.Error("empty Origin must not pass") + } +} + +func TestCanvasOriginAllowed_LocalhostDefault(t *testing.T) { + t.Setenv("CORS_ORIGINS", "") + if !canvasOriginAllowed("http://localhost:3000") { + t.Error("localhost:3000 should be allowed by default") + } + if canvasOriginAllowed("http://evil.example.com") { + t.Error("random origin should not be allowed") + } +} diff --git a/platform/internal/router/router.go b/platform/internal/router/router.go index fc3e579c..9db76af4 100644 --- a/platform/internal/router/router.go +++ b/platform/internal/router/router.go @@ -294,16 +294,17 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi th := handlers.NewTerminalHandler(dockerCli) wsAuth.GET("/terminal", th.HandleConnect) - // Canvas Viewport — #166: PUT gated behind AdminAuth so an anon caller - // can't reset the shared viewport state for all users. GET remains open - // because the canvas bootstraps without a bearer and needs the initial - // viewport for first paint. + // Canvas Viewport — #166 + #168: GET stays fully open for bootstrap. + // PUT uses CanvasOrBearer (accepts Origin-match OR bearer token) so the + // browser canvas can persist drag/zoom state without a bearer, while + // bearer-carrying clients (molecli, integration tests) still work. + // Viewport corruption is cosmetic-only — worst case a user refreshes + // the page — so the softer check is acceptable here. This middleware + // MUST NOT be used on routes that leak prompts, create workspaces, + // or write files (#164/#165/#190 class). vh := handlers.NewViewportHandler() r.GET("/canvas/viewport", vh.Get) - { - viewportAdmin := r.Group("", middleware.AdminAuth(db.DB)) - viewportAdmin.PUT("/canvas/viewport", vh.Save) - } + r.PUT("/canvas/viewport", middleware.CanvasOrBearer(db.DB), vh.Save) // Templates tmplh := handlers.NewTemplatesHandler(configsDir, dockerCli)