fix: implement security severity policy and enhance CodeQL checks for blocking findings

2026-02-25 15:05:41 +00:00
parent 0917edb863
commit cb16ac05a2
11 changed files with 727 additions and 43 deletions
@@ -103,6 +103,18 @@ type SetupRequest struct {
 	Password string `json:"password" binding:"required,min=8"`
 }

+func isSetupConflictError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	errText := strings.ToLower(err.Error())
+	return strings.Contains(errText, "unique constraint failed") ||
+		strings.Contains(errText, "duplicate key") ||
+		strings.Contains(errText, "database is locked") ||
+		strings.Contains(errText, "database table is locked")
+}
+
 // Setup creates the initial admin user and configures the ACME email.
 func (h *UserHandler) Setup(c *gin.Context) {
 	// 1. Check if setup is allowed
@@ -160,6 +172,17 @@ func (h *UserHandler) Setup(c *gin.Context) {
 	})

 	if err != nil {
+		var postTxCount int64
+		if countErr := h.DB.Model(&models.User{}).Count(&postTxCount).Error; countErr == nil && postTxCount > 0 {
+			c.JSON(http.StatusForbidden, gin.H{"error": "Setup already completed"})
+			return
+		}
+
+		if isSetupConflictError(err) {
+			c.JSON(http.StatusConflict, gin.H{"error": "Setup conflict: setup already in progress or completed"})
+			return
+		}
+
 		c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to complete setup: " + err.Error()})
 		return
 	}
@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"strconv"
+	"sync"
 	"testing"
 	"time"

@@ -15,15 +16,11 @@ import (
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"gorm.io/driver/sqlite"
 	"gorm.io/gorm"
 )

 func setupUserHandler(t *testing.T) (*UserHandler, *gorm.DB) {
-	// Use unique DB for each test to avoid pollution
-	dbName := "file:" + t.Name() + "?mode=memory&cache=shared"
-	db, err := gorm.Open(sqlite.Open(dbName), &gorm.Config{})
-	require.NoError(t, err)
+	db := OpenTestDB(t)
 	_ = db.AutoMigrate(&models.User{}, &models.Setting{}, &models.SecurityAudit{})
 	return NewUserHandler(db), db
 }
@@ -131,6 +128,224 @@ func TestUserHandler_Setup(t *testing.T) {
 	assert.Equal(t, http.StatusForbidden, w.Code)
 }

+func TestUserHandler_Setup_OneWayInvariant_ReentryRejectedAndSingleUser(t *testing.T) {
+	handler, db := setupUserHandler(t)
+	gin.SetMode(gin.TestMode)
+	r := gin.New()
+	r.POST("/setup", handler.Setup)
+
+	initialBody := map[string]string{
+		"name":     "Admin",
+		"email":    "admin@example.com",
+		"password": "password123",
+	}
+	initialJSON, _ := json.Marshal(initialBody)
+
+	firstReq := httptest.NewRequest(http.MethodPost, "/setup", bytes.NewBuffer(initialJSON))
+	firstReq.Header.Set("Content-Type", "application/json")
+	firstResp := httptest.NewRecorder()
+	r.ServeHTTP(firstResp, firstReq)
+	require.Equal(t, http.StatusCreated, firstResp.Code)
+
+	secondBody := map[string]string{
+		"name":     "Different Admin",
+		"email":    "different@example.com",
+		"password": "password123",
+	}
+	secondJSON, _ := json.Marshal(secondBody)
+	secondReq := httptest.NewRequest(http.MethodPost, "/setup", bytes.NewBuffer(secondJSON))
+	secondReq.Header.Set("Content-Type", "application/json")
+	secondResp := httptest.NewRecorder()
+	r.ServeHTTP(secondResp, secondReq)
+
+	require.Equal(t, http.StatusForbidden, secondResp.Code)
+
+	var userCount int64
+	require.NoError(t, db.Model(&models.User{}).Count(&userCount).Error)
+	assert.Equal(t, int64(1), userCount)
+}
+
+func TestUserHandler_Setup_ConcurrentAttemptInvariant(t *testing.T) {
+	handler, db := setupUserHandler(t)
+	gin.SetMode(gin.TestMode)
+	r := gin.New()
+	r.POST("/setup", handler.Setup)
+
+	concurrency := 6
+	start := make(chan struct{})
+	statuses := make(chan int, concurrency)
+
+	var wg sync.WaitGroup
+	for i := 0; i < concurrency; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			<-start
+
+			body := map[string]string{
+				"name":     "Admin",
+				"email":    "admin@example.com",
+				"password": "password123",
+			}
+			jsonBody, _ := json.Marshal(body)
+
+			req := httptest.NewRequest(http.MethodPost, "/setup", bytes.NewBuffer(jsonBody))
+			req.Header.Set("Content-Type", "application/json")
+			resp := httptest.NewRecorder()
+			r.ServeHTTP(resp, req)
+			statuses <- resp.Code
+		}()
+	}
+
+	close(start)
+	wg.Wait()
+	close(statuses)
+
+	createdCount := 0
+	forbiddenOrConflictCount := 0
+	for status := range statuses {
+		if status == http.StatusCreated {
+			createdCount++
+			continue
+		}
+
+		if status == http.StatusForbidden || status == http.StatusConflict {
+			forbiddenOrConflictCount++
+			continue
+		}
+
+		t.Fatalf("unexpected setup concurrency status: %d", status)
+	}
+
+	assert.Equal(t, 1, createdCount)
+	assert.Equal(t, concurrency-1, forbiddenOrConflictCount)
+
+	var userCount int64
+	require.NoError(t, db.Model(&models.User{}).Count(&userCount).Error)
+	assert.Equal(t, int64(1), userCount)
+}
+
+func TestUserHandler_Setup_ResponseSecretEchoContract(t *testing.T) {
+	handler, _ := setupUserHandler(t)
+	gin.SetMode(gin.TestMode)
+	r := gin.New()
+	r.POST("/setup", handler.Setup)
+
+	body := map[string]string{
+		"name":     "Admin",
+		"email":    "admin@example.com",
+		"password": "password123",
+	}
+	jsonBody, _ := json.Marshal(body)
+
+	req := httptest.NewRequest(http.MethodPost, "/setup", bytes.NewBuffer(jsonBody))
+	req.Header.Set("Content-Type", "application/json")
+	resp := httptest.NewRecorder()
+	r.ServeHTTP(resp, req)
+	require.Equal(t, http.StatusCreated, resp.Code)
+
+	var payload map[string]any
+	require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &payload))
+
+	userValue, ok := payload["user"]
+	require.True(t, ok)
+	userMap, ok := userValue.(map[string]any)
+	require.True(t, ok)
+
+	_, hasAPIKey := userMap["api_key"]
+	_, hasPassword := userMap["password"]
+	_, hasPasswordHash := userMap["password_hash"]
+	_, hasInviteToken := userMap["invite_token"]
+
+	assert.False(t, hasAPIKey)
+	assert.False(t, hasPassword)
+	assert.False(t, hasPasswordHash)
+	assert.False(t, hasInviteToken)
+}
+
+func TestUserHandler_GetProfile_SecretEchoContract(t *testing.T) {
+	handler, db := setupUserHandler(t)
+
+	user := &models.User{
+		UUID:         uuid.NewString(),
+		Email:        "profile@example.com",
+		Name:         "Profile User",
+		APIKey:       "real-secret-api-key",
+		InviteToken:  "invite-secret-token",
+		PasswordHash: "hashed-password-value",
+	}
+	require.NoError(t, db.Create(user).Error)
+
+	gin.SetMode(gin.TestMode)
+	r := gin.New()
+	r.Use(func(c *gin.Context) {
+		c.Set("userID", user.ID)
+		c.Next()
+	})
+	r.GET("/profile", handler.GetProfile)
+
+	req := httptest.NewRequest(http.MethodGet, "/profile", http.NoBody)
+	resp := httptest.NewRecorder()
+	r.ServeHTTP(resp, req)
+
+	require.Equal(t, http.StatusOK, resp.Code)
+	var payload map[string]any
+	require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &payload))
+
+	_, hasAPIKey := payload["api_key"]
+	_, hasPassword := payload["password"]
+	_, hasPasswordHash := payload["password_hash"]
+	_, hasInviteToken := payload["invite_token"]
+
+	assert.False(t, hasAPIKey)
+	assert.False(t, hasPassword)
+	assert.False(t, hasPasswordHash)
+	assert.False(t, hasInviteToken)
+	assert.Equal(t, "********", payload["api_key_masked"])
+}
+
+func TestUserHandler_ListUsers_SecretEchoContract(t *testing.T) {
+	handler, db := setupUserHandlerWithProxyHosts(t)
+
+	user := &models.User{
+		UUID:         uuid.NewString(),
+		Email:        "user@example.com",
+		Name:         "User",
+		Role:         "user",
+		APIKey:       "raw-api-key",
+		InviteToken:  "raw-invite-token",
+		PasswordHash: "raw-password-hash",
+	}
+	require.NoError(t, db.Create(user).Error)
+
+	gin.SetMode(gin.TestMode)
+	r := gin.New()
+	r.Use(func(c *gin.Context) {
+		c.Set("role", "admin")
+		c.Next()
+	})
+	r.GET("/users", handler.ListUsers)
+
+	req := httptest.NewRequest(http.MethodGet, "/users", http.NoBody)
+	resp := httptest.NewRecorder()
+	r.ServeHTTP(resp, req)
+
+	require.Equal(t, http.StatusOK, resp.Code)
+	var users []map[string]any
+	require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &users))
+	require.Len(t, users, 1)
+
+	_, hasAPIKey := users[0]["api_key"]
+	_, hasPassword := users[0]["password"]
+	_, hasPasswordHash := users[0]["password_hash"]
+	_, hasInviteToken := users[0]["invite_token"]
+
+	assert.False(t, hasAPIKey)
+	assert.False(t, hasPassword)
+	assert.False(t, hasPasswordHash)
+	assert.False(t, hasInviteToken)
+}
+
 func TestUserHandler_Setup_DBError(t *testing.T) {
 	// Can't easily mock DB error with sqlite memory unless we close it or something.
 	// But we can try to insert duplicate email if we had a unique constraint and pre-seeded data,
@@ -443,9 +658,7 @@ func TestUserHandler_UpdateProfile_Errors(t *testing.T) {
 // ============= User Management Tests (Admin functions) =============

 func setupUserHandlerWithProxyHosts(t *testing.T) (*UserHandler, *gorm.DB) {
-	dbName := "file:" + t.Name() + "?mode=memory&cache=shared"
-	db, err := gorm.Open(sqlite.Open(dbName), &gorm.Config{})
-	require.NoError(t, err)
+	db := OpenTestDB(t)
 	_ = db.AutoMigrate(&models.User{}, &models.Setting{}, &models.ProxyHost{}, &models.SecurityAudit{})
 	return NewUserHandler(db), db
 }
@@ -638,7 +638,7 @@ func RegisterWithDeps(router *gin.Engine, db *gorm.DB, cfg config.Config, caddyM
 	proxyHostHandler.RegisterRoutes(protected)

 	remoteServerHandler := handlers.NewRemoteServerHandler(remoteServerService, notificationService)
-	remoteServerHandler.RegisterRoutes(api)
+	remoteServerHandler.RegisterRoutes(protected)

 	// Initial Caddy Config Sync
 	go func() {
@@ -1,6 +1,7 @@
 package routes

 import (
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -16,6 +17,16 @@ import (
 	"gorm.io/gorm"
 )

+func materializeRoutePath(path string) string {
+	segments := strings.Split(path, "/")
+	for i, segment := range segments {
+		if strings.HasPrefix(segment, ":") {
+			segments[i] = "1"
+		}
+	}
+	return strings.Join(segments, "/")
+}
+
 func TestRegister(t *testing.T) {
 	gin.SetMode(gin.TestMode)
 	router := gin.New()
@@ -179,6 +190,70 @@ func TestRegister_ProxyHostsRequireAuth(t *testing.T) {
 	assert.Contains(t, w.Body.String(), "Authorization header required")
 }

+func TestRegister_StateChangingRoutesDenyByDefaultWithExplicitAllowlist(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	router := gin.New()
+
+	db, err := gorm.Open(sqlite.Open("file::memory:?cache=shared&_test_mutation_auth_guard"), &gorm.Config{})
+	require.NoError(t, err)
+
+	cfg := config.Config{JWTSecret: "test-secret"}
+	require.NoError(t, Register(router, db, cfg))
+
+	mutatingMethods := map[string]bool{
+		http.MethodPost:   true,
+		http.MethodPut:    true,
+		http.MethodPatch:  true,
+		http.MethodDelete: true,
+	}
+
+	publicMutationAllowlist := map[string]bool{
+		http.MethodPost + " /api/v1/auth/login":               true,
+		http.MethodPost + " /api/v1/auth/register":            true,
+		http.MethodPost + " /api/v1/setup":                    true,
+		http.MethodPost + " /api/v1/invite/accept":            true,
+		http.MethodPost + " /api/v1/security/events":          true,
+		http.MethodPost + " /api/v1/emergency/security-reset": true,
+	}
+
+	for _, route := range router.Routes() {
+		if !strings.HasPrefix(route.Path, "/api/v1/") {
+			continue
+		}
+		if !mutatingMethods[route.Method] {
+			continue
+		}
+
+		key := route.Method + " " + route.Path
+		if publicMutationAllowlist[key] {
+			continue
+		}
+
+		requestPath := materializeRoutePath(route.Path)
+		var body io.Reader = http.NoBody
+		if route.Method == http.MethodPost || route.Method == http.MethodPut || route.Method == http.MethodPatch {
+			body = strings.NewReader("{}")
+		}
+
+		req := httptest.NewRequest(route.Method, requestPath, body)
+		if route.Method == http.MethodPost || route.Method == http.MethodPut || route.Method == http.MethodPatch {
+			req.Header.Set("Content-Type", "application/json")
+		}
+
+		w := httptest.NewRecorder()
+		router.ServeHTTP(w, req)
+
+		assert.Contains(
+			t,
+			[]int{http.StatusUnauthorized, http.StatusForbidden},
+			w.Code,
+			"state-changing endpoint must deny unauthenticated access unless explicitly allowlisted: %s (materialized path: %s)",
+			key,
+			requestPath,
+		)
+	}
+}
+
 func TestRegister_DNSProviders_NotRegisteredWhenEncryptionKeyMissing(t *testing.T) {
 	gin.SetMode(gin.TestMode)
 	router := gin.New()
@@ -364,6 +439,42 @@ func TestRegister_AuthenticatedRoutes(t *testing.T) {
 	}
 }

+func TestRegister_StateChangingRoutesRequireAuthentication(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	router := gin.New()
+
+	db, err := gorm.Open(sqlite.Open("file::memory:?cache=shared&_test_mutating_auth_routes"), &gorm.Config{})
+	require.NoError(t, err)
+
+	cfg := config.Config{JWTSecret: "test-secret"}
+	require.NoError(t, Register(router, db, cfg))
+
+	stateChangingPaths := []struct {
+		method string
+		path   string
+	}{
+		{http.MethodPost, "/api/v1/backups"},
+		{http.MethodPost, "/api/v1/settings"},
+		{http.MethodPatch, "/api/v1/settings"},
+		{http.MethodPatch, "/api/v1/config"},
+		{http.MethodPost, "/api/v1/user/profile"},
+		{http.MethodPost, "/api/v1/remote-servers"},
+		{http.MethodPost, "/api/v1/remote-servers/test"},
+		{http.MethodPut, "/api/v1/remote-servers/1"},
+		{http.MethodDelete, "/api/v1/remote-servers/1"},
+		{http.MethodPost, "/api/v1/remote-servers/1/test"},
+	}
+
+	for _, tc := range stateChangingPaths {
+		t.Run(tc.method+"_"+tc.path, func(t *testing.T) {
+			w := httptest.NewRecorder()
+			req := httptest.NewRequest(tc.method, tc.path, nil)
+			router.ServeHTTP(w, req)
+			assert.Equal(t, http.StatusUnauthorized, w.Code, "State-changing route %s %s should require auth", tc.method, tc.path)
+		})
+	}
+}
+
 func TestRegister_AdminRoutes(t *testing.T) {
 	gin.SetMode(gin.TestMode)
 	router := gin.New()