From 0d9ee9c44a9a0df12a7769038ad0b7a7884f0ae1 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 22 Feb 2026 23:23:36 -0500 Subject: [PATCH 1/5] security: add algorithm pinning tests for JWT confusion attacks (closes #95) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified that all JWT validation paths in JWTAuthModule already enforce HS256 via both type assertion (*jwt.SigningMethodHMAC) and explicit algorithm check (token.Method.Alg() != jwt.SigningMethodHS256.Alg()). Added tests to module/jwt_auth_test.go that explicitly confirm tokens signed with HS384 or HS512 are rejected by: - Authenticate() — the AuthProvider interface method - handleRefresh via Handle() — the /auth/refresh endpoint - extractUserFromRequest via Handle() — all protected endpoints The api package (middleware.go, auth_handler.go) already had equivalent algorithm rejection tests in auth_handler_test.go. Co-Authored-By: Claude Opus 4.6 --- module/jwt_auth_test.go | 124 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/module/jwt_auth_test.go b/module/jwt_auth_test.go index 7b737fa4..41fb73fc 100644 --- a/module/jwt_auth_test.go +++ b/module/jwt_auth_test.go @@ -7,6 +7,8 @@ import ( "net/http/httptest" "testing" "time" + + "github.com/golang-jwt/jwt/v5" ) func setupJWTAuth(t *testing.T) *JWTAuthModule { @@ -692,3 +694,125 @@ func TestJWTAuth_UpdateRole_Admin(t *testing.T) { t.Errorf("expected role 'admin', got %v", updated["role"]) } } + +// --- Algorithm confusion / signing method pinning tests --- + +// TestJWTAuth_Authenticate_RejectsNonHS256 verifies that Authenticate() rejects +// tokens signed with algorithms other than HS256 (prevents algorithm confusion attacks). +func TestJWTAuth_Authenticate_RejectsNonHS256(t *testing.T) { + j := setupJWTAuth(t) + + user := &User{ID: "1", Email: "alg@example.com", Name: "Alg Test"} + + for _, method := range []jwt.SigningMethod{jwt.SigningMethodHS384, jwt.SigningMethodHS512} { + method := method + t.Run("rejects "+method.Alg(), func(t *testing.T) { + claims := jwt.MapClaims{ + "sub": user.ID, + "email": user.Email, + "iss": "test-issuer", + "iat": time.Now().Unix(), + "exp": time.Now().Add(24 * time.Hour).Unix(), + } + tok, err := jwt.NewWithClaims(method, claims).SignedString([]byte("test-secret-key")) + if err != nil { + t.Fatalf("failed to sign token with %s: %v", method.Alg(), err) + } + + valid, _, authErr := j.Authenticate(tok) + if authErr != nil { + t.Fatalf("expected nil error, got: %v", authErr) + } + if valid { + t.Errorf("expected token signed with %s to be rejected, but it was accepted", method.Alg()) + } + }) + } +} + +// TestJWTAuth_Authenticate_AcceptsHS256 verifies that valid HS256 tokens are accepted. +func TestJWTAuth_Authenticate_AcceptsHS256(t *testing.T) { + j := setupJWTAuth(t) + + user := &User{ID: "1", Email: "hs256@example.com", Name: "HS256 User"} + tok, err := j.generateToken(user) + if err != nil { + t.Fatalf("failed to generate token: %v", err) + } + + valid, _, err := j.Authenticate(tok) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !valid { + t.Error("expected HS256 token to be valid") + } +} + +// TestJWTAuth_HandleRefresh_RejectsNonHS256 verifies that the refresh handler +// rejects tokens signed with algorithms other than HS256. +func TestJWTAuth_HandleRefresh_RejectsNonHS256(t *testing.T) { + j := setupJWTAuthV1(t) + registerUserV1(t, j, "alg-refresh@example.com", "Alg Refresh", "pass123") + + for _, method := range []jwt.SigningMethod{jwt.SigningMethodHS384, jwt.SigningMethodHS512} { + method := method + t.Run("rejects "+method.Alg(), func(t *testing.T) { + claims := jwt.MapClaims{ + "sub": "1", + "email": "alg-refresh@example.com", + "type": "refresh", + "iss": "test-issuer", + "iat": time.Now().Unix(), + "exp": time.Now().Add(7 * 24 * time.Hour).Unix(), + } + tok, err := jwt.NewWithClaims(method, claims).SignedString([]byte("test-secret-key")) + if err != nil { + t.Fatalf("failed to sign token with %s: %v", method.Alg(), err) + } + + body, _ := json.Marshal(map[string]string{"refresh_token": tok}) + req := httptest.NewRequest(http.MethodPost, "/auth/refresh", bytes.NewReader(body)) + w := httptest.NewRecorder() + j.Handle(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("%s: expected 401, got %d", method.Alg(), w.Code) + } + }) + } +} + +// TestJWTAuth_ExtractUser_RejectsNonHS256 verifies that protected endpoints +// reject Authorization headers containing tokens signed with non-HS256 algorithms. +func TestJWTAuth_ExtractUser_RejectsNonHS256(t *testing.T) { + j := setupJWTAuth(t) + // Register a user so we have a valid user in the store. + registerUser(t, j, "protect@example.com", "Protected User", "pass123") + + for _, method := range []jwt.SigningMethod{jwt.SigningMethodHS384, jwt.SigningMethodHS512} { + method := method + t.Run("profile with "+method.Alg(), func(t *testing.T) { + claims := jwt.MapClaims{ + "sub": "1", + "email": "protect@example.com", + "iss": "test-issuer", + "iat": time.Now().Unix(), + "exp": time.Now().Add(24 * time.Hour).Unix(), + } + tok, err := jwt.NewWithClaims(method, claims).SignedString([]byte("test-secret-key")) + if err != nil { + t.Fatalf("failed to sign token with %s: %v", method.Alg(), err) + } + + req := httptest.NewRequest(http.MethodGet, "/auth/profile", nil) + req.Header.Set("Authorization", "Bearer "+tok) + w := httptest.NewRecorder() + j.Handle(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("%s: expected 401, got %d", method.Alg(), w.Code) + } + }) + } +} From 4873f1789cf833febdda54db6c0667aeaa041e34 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 22 Feb 2026 23:44:21 -0500 Subject: [PATCH 2/5] fix: clarify APIGateway rate limiter is instance-scoped, not global (closes #61) - Rename field globalLimiter -> instanceRateLimiter to make scope clear - Rename SetGlobalRateLimit -> SetRateLimit; keep SetGlobalRateLimit as a deprecated alias so existing callers continue to work - Add APIGatewayOption + WithRateLimit() functional option for DI at construction time (preferred over the setter) - Document on the struct that rate limiter state is never shared across instances, so multi-tenant deployments are not affected - Add TestAPIGateway_InstanceRateLimit_WithRateLimit and TestAPIGateway_InstanceRateLimiters_AreIsolated to cover the new option and prove per-instance isolation Co-Authored-By: Claude Opus 4.6 --- module/api_gateway.go | 63 +++++++++++++++++++++++++------- module/api_gateway_test.go | 73 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 121 insertions(+), 15 deletions(-) diff --git a/module/api_gateway.go b/module/api_gateway.go index e4090020..1a03d09d 100644 --- a/module/api_gateway.go +++ b/module/api_gateway.go @@ -48,6 +48,10 @@ type AuthConfig struct { // APIGateway is a composable gateway module that combines routing, auth, // rate limiting, and proxying into a single module. +// +// Each APIGateway instance maintains its own independent rate limiter state. +// Rate limiters are never shared across instances, so multiple APIGateway +// instances (e.g. in multi-tenant deployments) do not interfere with each other. type APIGateway struct { name string routes []GatewayRoute @@ -55,10 +59,10 @@ type APIGateway struct { auth *AuthConfig // internal state - sortedRoutes []GatewayRoute // sorted by prefix length (longest first) - proxies map[string]*httputil.ReverseProxy - rateLimiters map[string]*gatewayRateLimiter // keyed by path prefix - globalLimiter *gatewayRateLimiter + sortedRoutes []GatewayRoute // sorted by prefix length (longest first) + proxies map[string]*httputil.ReverseProxy + rateLimiters map[string]*gatewayRateLimiter // keyed by path prefix + instanceRateLimiter *gatewayRateLimiter // instance-scoped limiter applied before per-route limits } // gatewayRateLimiter is a simple per-client token bucket limiter for the gateway. @@ -90,13 +94,36 @@ func (rl *gatewayRateLimiter) allow(clientIP string) bool { return bucket.allow() } -// NewAPIGateway creates a new APIGateway module. -func NewAPIGateway(name string) *APIGateway { - return &APIGateway{ +// APIGatewayOption is a functional option for configuring an APIGateway at construction time. +type APIGatewayOption func(*APIGateway) + +// WithRateLimit sets an instance-level rate limit applied to all requests before per-route +// limits are checked. The limiter is scoped to this APIGateway instance and does not affect +// any other instance. +func WithRateLimit(cfg *RateLimitConfig) APIGatewayOption { + return func(g *APIGateway) { + if cfg != nil && cfg.RequestsPerMinute > 0 { + burst := cfg.BurstSize + if burst <= 0 { + burst = cfg.RequestsPerMinute + } + g.instanceRateLimiter = newGatewayRateLimiter(cfg.RequestsPerMinute, burst) + } + } +} + +// NewAPIGateway creates a new APIGateway module. Optional functional options can be +// provided to configure the instance at construction time (e.g. WithRateLimit). +func NewAPIGateway(name string, opts ...APIGatewayOption) *APIGateway { + g := &APIGateway{ name: name, proxies: make(map[string]*httputil.ReverseProxy), rateLimiters: make(map[string]*gatewayRateLimiter), } + for _, opt := range opts { + opt(g) + } + return g } // SetRoutes configures the gateway routes. @@ -146,17 +173,27 @@ func (g *APIGateway) SetRoutes(routes []GatewayRoute) error { return nil } -// SetGlobalRateLimit configures a global rate limit applied to all routes. -func (g *APIGateway) SetGlobalRateLimit(cfg *RateLimitConfig) { +// SetRateLimit configures an instance-level rate limit applied to all routes on this gateway. +// The limiter is scoped to this APIGateway instance and does not affect any other instance. +// Prefer injecting rate limit config via WithRateLimit at construction time when possible. +func (g *APIGateway) SetRateLimit(cfg *RateLimitConfig) { if cfg != nil && cfg.RequestsPerMinute > 0 { burst := cfg.BurstSize if burst <= 0 { burst = cfg.RequestsPerMinute } - g.globalLimiter = newGatewayRateLimiter(cfg.RequestsPerMinute, burst) + g.instanceRateLimiter = newGatewayRateLimiter(cfg.RequestsPerMinute, burst) } } +// SetGlobalRateLimit is deprecated: use SetRateLimit instead. +// The rate limiter has always been instance-scoped; this method was misleadingly named. +// +// Deprecated: Use SetRateLimit. +func (g *APIGateway) SetGlobalRateLimit(cfg *RateLimitConfig) { + g.SetRateLimit(cfg) +} + // SetCORS configures CORS settings. func (g *APIGateway) SetCORS(cfg *CORSConfig) { g.cors = cfg @@ -214,9 +251,9 @@ func (g *APIGateway) Handle(w http.ResponseWriter, r *http.Request) { clientIP := extractClientIP(r) - // Global rate limiting - if g.globalLimiter != nil { - if !g.globalLimiter.allow(clientIP) { + // Instance-level rate limiting (applied before per-route limits) + if g.instanceRateLimiter != nil { + if !g.instanceRateLimiter.allow(clientIP) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusTooManyRequests) _ = json.NewEncoder(w).Encode(map[string]string{ diff --git a/module/api_gateway_test.go b/module/api_gateway_test.go index 1941e691..da1f0876 100644 --- a/module/api_gateway_test.go +++ b/module/api_gateway_test.go @@ -219,14 +219,14 @@ func TestAPIGateway_CORS(t *testing.T) { } } -func TestAPIGateway_GlobalRateLimit(t *testing.T) { +func TestAPIGateway_InstanceRateLimit_SetRateLimit(t *testing.T) { backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) defer backend.Close() gw := NewAPIGateway("gw") - gw.SetGlobalRateLimit(&RateLimitConfig{RequestsPerMinute: 60, BurstSize: 2}) + gw.SetRateLimit(&RateLimitConfig{RequestsPerMinute: 60, BurstSize: 2}) _ = gw.SetRoutes([]GatewayRoute{ {PathPrefix: "/api", Backend: backend.URL}, }) @@ -252,6 +252,75 @@ func TestAPIGateway_GlobalRateLimit(t *testing.T) { } } +func TestAPIGateway_InstanceRateLimit_WithRateLimit(t *testing.T) { + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer backend.Close() + + gw := NewAPIGateway("gw", WithRateLimit(&RateLimitConfig{RequestsPerMinute: 60, BurstSize: 1})) + _ = gw.SetRoutes([]GatewayRoute{ + {PathPrefix: "/api", Backend: backend.URL}, + }) + + // First should succeed (burst=1) + req := httptest.NewRequest("GET", "/api/test", nil) + req.RemoteAddr = "10.0.0.2:1234" + w := httptest.NewRecorder() + gw.Handle(w, req) + if w.Code != http.StatusOK { + t.Errorf("first request expected 200, got %d", w.Code) + } + + // Second should be rate limited + req = httptest.NewRequest("GET", "/api/test", nil) + req.RemoteAddr = "10.0.0.2:1234" + w = httptest.NewRecorder() + gw.Handle(w, req) + if w.Code != http.StatusTooManyRequests { + t.Errorf("expected 429, got %d", w.Code) + } +} + +func TestAPIGateway_InstanceRateLimiters_AreIsolated(t *testing.T) { + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer backend.Close() + + cfg := &RateLimitConfig{RequestsPerMinute: 60, BurstSize: 1} + gw1 := NewAPIGateway("gw1", WithRateLimit(cfg)) + gw2 := NewAPIGateway("gw2", WithRateLimit(cfg)) + _ = gw1.SetRoutes([]GatewayRoute{{PathPrefix: "/api", Backend: backend.URL}}) + _ = gw2.SetRoutes([]GatewayRoute{{PathPrefix: "/api", Backend: backend.URL}}) + + // Exhaust gw1's burst for this client + req := httptest.NewRequest("GET", "/api/test", nil) + req.RemoteAddr = "10.0.0.3:1234" + w := httptest.NewRecorder() + gw1.Handle(w, req) + if w.Code != http.StatusOK { + t.Errorf("gw1 first request expected 200, got %d", w.Code) + } + + req = httptest.NewRequest("GET", "/api/test", nil) + req.RemoteAddr = "10.0.0.3:1234" + w = httptest.NewRecorder() + gw1.Handle(w, req) + if w.Code != http.StatusTooManyRequests { + t.Errorf("gw1 second request expected 429, got %d", w.Code) + } + + // gw2 should be unaffected — its burst is independent + req = httptest.NewRequest("GET", "/api/test", nil) + req.RemoteAddr = "10.0.0.3:1234" + w = httptest.NewRecorder() + gw2.Handle(w, req) + if w.Code != http.StatusOK { + t.Errorf("gw2 should be isolated from gw1; expected 200, got %d", w.Code) + } +} + func TestAPIGateway_PerRouteRateLimit(t *testing.T) { backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) From da0677072029bead4d8244a1ed6002bc9b4574f4 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 22 Feb 2026 23:44:30 -0500 Subject: [PATCH 3/5] Revert "fix: clarify APIGateway rate limiter is instance-scoped, not global (closes #61)" This reverts commit 4873f1789cf833febdda54db6c0667aeaa041e34. --- module/api_gateway.go | 63 +++++++------------------------- module/api_gateway_test.go | 73 ++------------------------------------ 2 files changed, 15 insertions(+), 121 deletions(-) diff --git a/module/api_gateway.go b/module/api_gateway.go index 1a03d09d..e4090020 100644 --- a/module/api_gateway.go +++ b/module/api_gateway.go @@ -48,10 +48,6 @@ type AuthConfig struct { // APIGateway is a composable gateway module that combines routing, auth, // rate limiting, and proxying into a single module. -// -// Each APIGateway instance maintains its own independent rate limiter state. -// Rate limiters are never shared across instances, so multiple APIGateway -// instances (e.g. in multi-tenant deployments) do not interfere with each other. type APIGateway struct { name string routes []GatewayRoute @@ -59,10 +55,10 @@ type APIGateway struct { auth *AuthConfig // internal state - sortedRoutes []GatewayRoute // sorted by prefix length (longest first) - proxies map[string]*httputil.ReverseProxy - rateLimiters map[string]*gatewayRateLimiter // keyed by path prefix - instanceRateLimiter *gatewayRateLimiter // instance-scoped limiter applied before per-route limits + sortedRoutes []GatewayRoute // sorted by prefix length (longest first) + proxies map[string]*httputil.ReverseProxy + rateLimiters map[string]*gatewayRateLimiter // keyed by path prefix + globalLimiter *gatewayRateLimiter } // gatewayRateLimiter is a simple per-client token bucket limiter for the gateway. @@ -94,36 +90,13 @@ func (rl *gatewayRateLimiter) allow(clientIP string) bool { return bucket.allow() } -// APIGatewayOption is a functional option for configuring an APIGateway at construction time. -type APIGatewayOption func(*APIGateway) - -// WithRateLimit sets an instance-level rate limit applied to all requests before per-route -// limits are checked. The limiter is scoped to this APIGateway instance and does not affect -// any other instance. -func WithRateLimit(cfg *RateLimitConfig) APIGatewayOption { - return func(g *APIGateway) { - if cfg != nil && cfg.RequestsPerMinute > 0 { - burst := cfg.BurstSize - if burst <= 0 { - burst = cfg.RequestsPerMinute - } - g.instanceRateLimiter = newGatewayRateLimiter(cfg.RequestsPerMinute, burst) - } - } -} - -// NewAPIGateway creates a new APIGateway module. Optional functional options can be -// provided to configure the instance at construction time (e.g. WithRateLimit). -func NewAPIGateway(name string, opts ...APIGatewayOption) *APIGateway { - g := &APIGateway{ +// NewAPIGateway creates a new APIGateway module. +func NewAPIGateway(name string) *APIGateway { + return &APIGateway{ name: name, proxies: make(map[string]*httputil.ReverseProxy), rateLimiters: make(map[string]*gatewayRateLimiter), } - for _, opt := range opts { - opt(g) - } - return g } // SetRoutes configures the gateway routes. @@ -173,27 +146,17 @@ func (g *APIGateway) SetRoutes(routes []GatewayRoute) error { return nil } -// SetRateLimit configures an instance-level rate limit applied to all routes on this gateway. -// The limiter is scoped to this APIGateway instance and does not affect any other instance. -// Prefer injecting rate limit config via WithRateLimit at construction time when possible. -func (g *APIGateway) SetRateLimit(cfg *RateLimitConfig) { +// SetGlobalRateLimit configures a global rate limit applied to all routes. +func (g *APIGateway) SetGlobalRateLimit(cfg *RateLimitConfig) { if cfg != nil && cfg.RequestsPerMinute > 0 { burst := cfg.BurstSize if burst <= 0 { burst = cfg.RequestsPerMinute } - g.instanceRateLimiter = newGatewayRateLimiter(cfg.RequestsPerMinute, burst) + g.globalLimiter = newGatewayRateLimiter(cfg.RequestsPerMinute, burst) } } -// SetGlobalRateLimit is deprecated: use SetRateLimit instead. -// The rate limiter has always been instance-scoped; this method was misleadingly named. -// -// Deprecated: Use SetRateLimit. -func (g *APIGateway) SetGlobalRateLimit(cfg *RateLimitConfig) { - g.SetRateLimit(cfg) -} - // SetCORS configures CORS settings. func (g *APIGateway) SetCORS(cfg *CORSConfig) { g.cors = cfg @@ -251,9 +214,9 @@ func (g *APIGateway) Handle(w http.ResponseWriter, r *http.Request) { clientIP := extractClientIP(r) - // Instance-level rate limiting (applied before per-route limits) - if g.instanceRateLimiter != nil { - if !g.instanceRateLimiter.allow(clientIP) { + // Global rate limiting + if g.globalLimiter != nil { + if !g.globalLimiter.allow(clientIP) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusTooManyRequests) _ = json.NewEncoder(w).Encode(map[string]string{ diff --git a/module/api_gateway_test.go b/module/api_gateway_test.go index da1f0876..1941e691 100644 --- a/module/api_gateway_test.go +++ b/module/api_gateway_test.go @@ -219,14 +219,14 @@ func TestAPIGateway_CORS(t *testing.T) { } } -func TestAPIGateway_InstanceRateLimit_SetRateLimit(t *testing.T) { +func TestAPIGateway_GlobalRateLimit(t *testing.T) { backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) defer backend.Close() gw := NewAPIGateway("gw") - gw.SetRateLimit(&RateLimitConfig{RequestsPerMinute: 60, BurstSize: 2}) + gw.SetGlobalRateLimit(&RateLimitConfig{RequestsPerMinute: 60, BurstSize: 2}) _ = gw.SetRoutes([]GatewayRoute{ {PathPrefix: "/api", Backend: backend.URL}, }) @@ -252,75 +252,6 @@ func TestAPIGateway_InstanceRateLimit_SetRateLimit(t *testing.T) { } } -func TestAPIGateway_InstanceRateLimit_WithRateLimit(t *testing.T) { - backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - })) - defer backend.Close() - - gw := NewAPIGateway("gw", WithRateLimit(&RateLimitConfig{RequestsPerMinute: 60, BurstSize: 1})) - _ = gw.SetRoutes([]GatewayRoute{ - {PathPrefix: "/api", Backend: backend.URL}, - }) - - // First should succeed (burst=1) - req := httptest.NewRequest("GET", "/api/test", nil) - req.RemoteAddr = "10.0.0.2:1234" - w := httptest.NewRecorder() - gw.Handle(w, req) - if w.Code != http.StatusOK { - t.Errorf("first request expected 200, got %d", w.Code) - } - - // Second should be rate limited - req = httptest.NewRequest("GET", "/api/test", nil) - req.RemoteAddr = "10.0.0.2:1234" - w = httptest.NewRecorder() - gw.Handle(w, req) - if w.Code != http.StatusTooManyRequests { - t.Errorf("expected 429, got %d", w.Code) - } -} - -func TestAPIGateway_InstanceRateLimiters_AreIsolated(t *testing.T) { - backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - })) - defer backend.Close() - - cfg := &RateLimitConfig{RequestsPerMinute: 60, BurstSize: 1} - gw1 := NewAPIGateway("gw1", WithRateLimit(cfg)) - gw2 := NewAPIGateway("gw2", WithRateLimit(cfg)) - _ = gw1.SetRoutes([]GatewayRoute{{PathPrefix: "/api", Backend: backend.URL}}) - _ = gw2.SetRoutes([]GatewayRoute{{PathPrefix: "/api", Backend: backend.URL}}) - - // Exhaust gw1's burst for this client - req := httptest.NewRequest("GET", "/api/test", nil) - req.RemoteAddr = "10.0.0.3:1234" - w := httptest.NewRecorder() - gw1.Handle(w, req) - if w.Code != http.StatusOK { - t.Errorf("gw1 first request expected 200, got %d", w.Code) - } - - req = httptest.NewRequest("GET", "/api/test", nil) - req.RemoteAddr = "10.0.0.3:1234" - w = httptest.NewRecorder() - gw1.Handle(w, req) - if w.Code != http.StatusTooManyRequests { - t.Errorf("gw1 second request expected 429, got %d", w.Code) - } - - // gw2 should be unaffected — its burst is independent - req = httptest.NewRequest("GET", "/api/test", nil) - req.RemoteAddr = "10.0.0.3:1234" - w = httptest.NewRecorder() - gw2.Handle(w, req) - if w.Code != http.StatusOK { - t.Errorf("gw2 should be isolated from gw1; expected 200, got %d", w.Code) - } -} - func TestAPIGateway_PerRouteRateLimit(t *testing.T) { backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) From 9dd269ced2bc2790fba8ea15f4b733f569a52bcc Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 22 Feb 2026 23:53:34 -0500 Subject: [PATCH 4/5] fix(server): implement multi-workflow mode API wiring (closes #71) Replace the dead TODO block in cmd/server/main.go with a working implementation that connects to PostgreSQL via store.NewPGStore, runs database migrations, bootstraps an optional admin user on first run, starts the api.NewRouter on a dedicated port (--multi-workflow-addr, default :8090), and shuts down cleanly alongside the single-config engine. New flags: --multi-workflow-addr New imports: apihandler (api package), bcrypt, time Co-Authored-By: Claude Opus 4.6 --- cmd/server/main.go | 128 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 99 insertions(+), 29 deletions(-) diff --git a/cmd/server/main.go b/cmd/server/main.go index 2e569c84..33feb7dd 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -14,6 +14,7 @@ import ( "path/filepath" "strings" "syscall" + "time" "github.com/CrisisTextLine/modular" "github.com/GoCodeAlone/workflow" @@ -21,6 +22,7 @@ import ( "github.com/GoCodeAlone/workflow/ai" copilotai "github.com/GoCodeAlone/workflow/ai/copilot" "github.com/GoCodeAlone/workflow/ai/llm" + apihandler "github.com/GoCodeAlone/workflow/api" "github.com/GoCodeAlone/workflow/audit" "github.com/GoCodeAlone/workflow/billing" "github.com/GoCodeAlone/workflow/bundle" @@ -62,6 +64,7 @@ import ( "github.com/GoCodeAlone/workflow/schema" evstore "github.com/GoCodeAlone/workflow/store" "github.com/google/uuid" + "golang.org/x/crypto/bcrypt" _ "modernc.org/sqlite" ) @@ -74,10 +77,11 @@ var ( anthropicModel = flag.String("anthropic-model", "", "Anthropic model name") // Multi-workflow mode flags - databaseDSN = flag.String("database-dsn", "", "PostgreSQL connection string for multi-workflow mode") - jwtSecret = flag.String("jwt-secret", "", "JWT signing secret for API authentication") - adminEmail = flag.String("admin-email", "", "Initial admin user email (first-run bootstrap)") - adminPassword = flag.String("admin-password", "", "Initial admin user password (first-run bootstrap)") + databaseDSN = flag.String("database-dsn", "", "PostgreSQL connection string for multi-workflow mode") + jwtSecret = flag.String("jwt-secret", "", "JWT signing secret for API authentication") + adminEmail = flag.String("admin-email", "", "Initial admin user email (first-run bootstrap)") + adminPassword = flag.String("admin-password", "", "Initial admin user password (first-run bootstrap)") + multiWorkflowAddr = flag.String("multi-workflow-addr", ":8090", "HTTP listen address for multi-workflow REST API") // License flags licenseKey = flag.String("license-key", "", "License key for the workflow engine (or set WORKFLOW_LICENSE_KEY env var)") @@ -1184,43 +1188,109 @@ func main() { })) if *databaseDSN != "" { - // Multi-workflow mode - logger.Info("Starting in multi-workflow mode") - - // TODO: Once the api package is implemented, this section will: - // 1. Connect to PostgreSQL using *databaseDSN - // 2. Run database migrations - // 3. Create store instances (UserStore, CompanyStore, ProjectStore, WorkflowStore, etc.) - // 4. Bootstrap admin user if *adminEmail and *adminPassword are set (first-run) - // 5. Create WorkflowEngineManager with stores - // 6. Create api.NewRouter() with stores, *jwtSecret, and engine manager - // 7. Mount API router at /api/v1/ alongside existing routes - - // For now, log the configuration and fall through to single-config mode - logger.Info("Multi-workflow mode configured", - "database_dsn_set", *databaseDSN != "", + // Multi-workflow mode: connect to PostgreSQL, run migrations, start the + // REST API router on a dedicated port alongside the single-config engine. + logger.Info("Starting in multi-workflow mode", + "database_dsn_set", true, "jwt_secret_set", *jwtSecret != "", "admin_email_set", *adminEmail != "", + "api_addr", *multiWorkflowAddr, ) + pgStore, pgErr := evstore.NewPGStore(context.Background(), evstore.PGConfig{URL: *databaseDSN}) + if pgErr != nil { + log.Fatalf("multi-workflow mode: failed to connect to PostgreSQL: %v", pgErr) + } + migrator := evstore.NewMigrator(pgStore.Pool()) + if mErr := migrator.Migrate(context.Background()); mErr != nil { + log.Fatalf("multi-workflow mode: database migration failed: %v", mErr) + } + logger.Info("multi-workflow mode: database migrations applied") + + // Bootstrap admin user on first run. + if *adminEmail != "" && *adminPassword != "" { + _, lookupErr := pgStore.Users().GetByEmail(context.Background(), *adminEmail) + if lookupErr != nil { + hash, hashErr := bcrypt.GenerateFromPassword([]byte(*adminPassword), bcrypt.DefaultCost) + if hashErr != nil { + log.Fatalf("multi-workflow mode: failed to hash admin password: %v", hashErr) + } + now := time.Now() + adminUser := &evstore.User{ + ID: uuid.New(), + Email: *adminEmail, + PasswordHash: string(hash), + DisplayName: "Admin", + Active: true, + CreatedAt: now, + UpdatedAt: now, + } + if createErr := pgStore.Users().Create(context.Background(), adminUser); createErr != nil { + logger.Warn("multi-workflow mode: failed to create admin user (may already exist)", "error", createErr) + } else { + logger.Info("multi-workflow mode: created bootstrap admin user", "email", *adminEmail) + } + } else { + logger.Info("multi-workflow mode: admin user already exists, skipping bootstrap", "email", *adminEmail) + } + } - // Suppress unused variable warnings until api package is ready - _ = databaseDSN - _ = jwtSecret - _ = adminEmail - _ = adminPassword - - logger.Warn("Multi-workflow mode requires the api package (not yet available); falling back to single-config mode") + engineBuilder := func(cfg *config.WorkflowConfig, lg *slog.Logger) (*workflow.StdEngine, modular.Application, error) { + eng, _, _, _, buildErr := buildEngine(cfg, lg) + if buildErr != nil { + return nil, nil, buildErr + } + app := eng.GetApp() + return eng, app, nil + } + engineMgr := workflow.NewWorkflowEngineManager(pgStore.Workflows(), pgStore.CrossWorkflowLinks(), logger, engineBuilder) + + apiRouter := apihandler.NewRouter(apihandler.Stores{ + Users: pgStore.Users(), + Sessions: pgStore.Sessions(), + Companies: pgStore.Companies(), + Projects: pgStore.Projects(), + Workflows: pgStore.Workflows(), + Memberships: pgStore.Memberships(), + Links: pgStore.CrossWorkflowLinks(), + Executions: pgStore.Executions(), + Logs: pgStore.Logs(), + Audit: pgStore.Audit(), + IAM: pgStore.IAM(), + }, apihandler.Config{JWTSecret: *jwtSecret}) + + apiServer := &http.Server{ //nolint:gosec // ReadHeaderTimeout set below + Addr: *multiWorkflowAddr, + Handler: apiRouter, + ReadHeaderTimeout: 10 * time.Second, + } + go func() { + logger.Info("multi-workflow API listening", "addr", *multiWorkflowAddr) + if sErr := apiServer.ListenAndServe(); sErr != nil && sErr != http.ErrServerClosed { + logger.Error("multi-workflow API server error", "error", sErr) + } + }() + defer func() { + shutdownCtx := context.Background() + if sErr := apiServer.Shutdown(shutdownCtx); sErr != nil { + logger.Warn("multi-workflow API server shutdown error", "error", sErr) + } + if sErr := engineMgr.StopAll(shutdownCtx); sErr != nil { + logger.Warn("multi-workflow engine manager shutdown error", "error", sErr) + } + pgStore.Close() + }() + _ = engineMgr // used via closure above } - // Existing single-config behavior + // Single-config mode always runs alongside multi-workflow mode (if enabled). cfg, err := loadConfig(logger) if err != nil { - log.Fatalf("Configuration error: %v", err) + log.Fatalf("Configuration error: %v", err) //nolint:gocritic // exitAfterDefer: intentional, cleanup is best-effort } app, err := setup(logger, cfg) if err != nil { - log.Fatalf("Setup error: %v", err) + log.Fatalf("Setup error: %v", err) //nolint:gocritic // exitAfterDefer: intentional, cleanup is best-effort } ctx, cancel := context.WithCancel(context.Background()) From d6a5b8c342bffaa78259f6efb4fbb3c8a7cc2924 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Feb 2026 01:23:19 -0500 Subject: [PATCH 5/5] fix(server): harden multi-workflow mode wiring based on review feedback (#110) * Initial plan * fix(server): address multi-workflow mode review feedback Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> * fix(gitignore): fix malformed db-shm pattern; untrack sqlite wal file Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --- .gitignore | 3 +- api/router.go | 6 ++++ api/workflow_handler.go | 64 ++++++++++++++++++++++++++++++++--------- cmd/server/main.go | 23 +++++++++++---- 4 files changed, 76 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index 19bc47e8..36bd0a4a 100644 --- a/.gitignore +++ b/.gitignore @@ -46,5 +46,6 @@ node_modules/ # Built admin UI assets (generated by `make build-ui`, embedded via go:embed) module/ui_dist/* -!module/ui_dist/.gitkeep*.db-shm +!module/ui_dist/.gitkeep +*.db-shm *.db-wal diff --git a/api/router.go b/api/router.go index 234817f4..b990c1fb 100644 --- a/api/router.go +++ b/api/router.go @@ -22,6 +22,11 @@ type Config struct { // OAuth providers keyed by provider name (e.g. "google", "okta"). OAuthProviders map[string]*OAuthProviderConfig + + // Orchestrator is an optional engine manager that is called when workflows + // are deployed or stopped via the API. When nil, only the database status + // is updated (no live engine is started or stopped). + Orchestrator WorkflowOrchestrator } // Stores groups all store interfaces needed by the API. @@ -109,6 +114,7 @@ func NewRouterWithIAM(stores Stores, cfg Config, iamResolver *iam.IAMResolver) h // --- Workflows --- wfH := NewWorkflowHandler(stores.Workflows, stores.Projects, stores.Memberships, permissions) + wfH.orchestrator = cfg.Orchestrator mux.Handle("POST /api/v1/projects/{pid}/workflows", mw.RequireAuth(http.HandlerFunc(wfH.Create))) mux.Handle("GET /api/v1/workflows", mw.RequireAuth(http.HandlerFunc(wfH.ListAll))) mux.Handle("GET /api/v1/projects/{pid}/workflows", mw.RequireAuth(http.HandlerFunc(wfH.ListInProject))) diff --git a/api/workflow_handler.go b/api/workflow_handler.go index 3a2aeb32..497ba319 100644 --- a/api/workflow_handler.go +++ b/api/workflow_handler.go @@ -1,6 +1,7 @@ package api import ( + "context" "encoding/json" "errors" "net/http" @@ -11,12 +12,21 @@ import ( "github.com/google/uuid" ) +// WorkflowOrchestrator is implemented by the multi-workflow engine manager to +// allow the API layer to start and stop live workflow engines. It is optional; +// when nil the Deploy and Stop handlers only update the database status. +type WorkflowOrchestrator interface { + DeployWorkflow(ctx context.Context, workflowID uuid.UUID) error + StopWorkflow(ctx context.Context, workflowID uuid.UUID) error +} + // WorkflowHandler handles workflow CRUD and lifecycle endpoints. type WorkflowHandler struct { - workflows store.WorkflowStore - projects store.ProjectStore - memberships store.MembershipStore - permissions *PermissionService + workflows store.WorkflowStore + projects store.ProjectStore + memberships store.MembershipStore + permissions *PermissionService + orchestrator WorkflowOrchestrator // optional; nil when no engine manager is wired } // NewWorkflowHandler creates a new WorkflowHandler. @@ -259,11 +269,24 @@ func (h *WorkflowHandler) Deploy(w http.ResponseWriter, r *http.Request) { WriteError(w, http.StatusInternalServerError, "internal error") return } - wf.Status = store.WorkflowStatusActive - wf.UpdatedAt = time.Now() - if err := h.workflows.Update(r.Context(), wf); err != nil { - WriteError(w, http.StatusInternalServerError, "internal error") - return + if h.orchestrator != nil { + if oErr := h.orchestrator.DeployWorkflow(r.Context(), id); oErr != nil { + WriteError(w, http.StatusInternalServerError, oErr.Error()) + return + } + // Re-fetch to get updated status written by the orchestrator. + wf, err = h.workflows.Get(r.Context(), id) + if err != nil { + WriteError(w, http.StatusInternalServerError, "internal error") + return + } + } else { + wf.Status = store.WorkflowStatusActive + wf.UpdatedAt = time.Now() + if err := h.workflows.Update(r.Context(), wf); err != nil { + WriteError(w, http.StatusInternalServerError, "internal error") + return + } } WriteJSON(w, http.StatusOK, wf) } @@ -284,11 +307,24 @@ func (h *WorkflowHandler) Stop(w http.ResponseWriter, r *http.Request) { WriteError(w, http.StatusInternalServerError, "internal error") return } - wf.Status = store.WorkflowStatusStopped - wf.UpdatedAt = time.Now() - if err := h.workflows.Update(r.Context(), wf); err != nil { - WriteError(w, http.StatusInternalServerError, "internal error") - return + if h.orchestrator != nil { + if oErr := h.orchestrator.StopWorkflow(r.Context(), id); oErr != nil { + WriteError(w, http.StatusInternalServerError, oErr.Error()) + return + } + // Re-fetch to get updated status written by the orchestrator. + wf, err = h.workflows.Get(r.Context(), id) + if err != nil { + WriteError(w, http.StatusInternalServerError, "internal error") + return + } + } else { + wf.Status = store.WorkflowStatusStopped + wf.UpdatedAt = time.Now() + if err := h.workflows.Update(r.Context(), wf); err != nil { + WriteError(w, http.StatusInternalServerError, "internal error") + return + } } WriteJSON(w, http.StatusOK, wf) } diff --git a/cmd/server/main.go b/cmd/server/main.go index 33feb7dd..5981031a 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "encoding/json" + "errors" "flag" "fmt" "log" @@ -1196,6 +1197,12 @@ func main() { "admin_email_set", *adminEmail != "", "api_addr", *multiWorkflowAddr, ) + + // Validate JWT secret meets minimum security requirements. + if len(*jwtSecret) < 32 { + log.Fatalf("multi-workflow mode: --jwt-secret must be at least 32 bytes (got %d)", len(*jwtSecret)) + } + pgStore, pgErr := evstore.NewPGStore(context.Background(), evstore.PGConfig{URL: *databaseDSN}) if pgErr != nil { log.Fatalf("multi-workflow mode: failed to connect to PostgreSQL: %v", pgErr) @@ -1209,7 +1216,8 @@ func main() { // Bootstrap admin user on first run. if *adminEmail != "" && *adminPassword != "" { _, lookupErr := pgStore.Users().GetByEmail(context.Background(), *adminEmail) - if lookupErr != nil { + switch { + case errors.Is(lookupErr, evstore.ErrNotFound): hash, hashErr := bcrypt.GenerateFromPassword([]byte(*adminPassword), bcrypt.DefaultCost) if hashErr != nil { log.Fatalf("multi-workflow mode: failed to hash admin password: %v", hashErr) @@ -1229,7 +1237,9 @@ func main() { } else { logger.Info("multi-workflow mode: created bootstrap admin user", "email", *adminEmail) } - } else { + case lookupErr != nil: + log.Fatalf("multi-workflow mode: failed to look up admin user: %v", lookupErr) + default: logger.Info("multi-workflow mode: admin user already exists, skipping bootstrap", "email", *adminEmail) } } @@ -1256,7 +1266,10 @@ func main() { Logs: pgStore.Logs(), Audit: pgStore.Audit(), IAM: pgStore.IAM(), - }, apihandler.Config{JWTSecret: *jwtSecret}) + }, apihandler.Config{ + JWTSecret: *jwtSecret, + Orchestrator: engineMgr, + }) apiServer := &http.Server{ //nolint:gosec // ReadHeaderTimeout set below Addr: *multiWorkflowAddr, @@ -1270,7 +1283,8 @@ func main() { } }() defer func() { - shutdownCtx := context.Background() + shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer shutdownCancel() if sErr := apiServer.Shutdown(shutdownCtx); sErr != nil { logger.Warn("multi-workflow API server shutdown error", "error", sErr) } @@ -1279,7 +1293,6 @@ func main() { } pgStore.Close() }() - _ = engineMgr // used via closure above } // Single-config mode always runs alongside multi-workflow mode (if enabled).