diff --git a/.github/security-severity-policy.yml b/.github/security-severity-policy.yml new file mode 100644 index 00000000..81860a2a --- /dev/null +++ b/.github/security-severity-policy.yml @@ -0,0 +1,55 @@ +version: 1 +effective_date: 2026-02-25 +scope: + - local pre-commit manual security hooks + - github actions security workflows + +defaults: + blocking: + - critical + - high + medium: + mode: risk-based + default_action: report + require_sla: true + default_sla_days: 14 + escalation: + trigger: high-signal class or repeated finding + action: require issue + owner + due date + low: + action: report + +codeql: + severity_mapping: + error: high_or_critical + warning: medium_or_lower + note: informational + blocking_levels: + - error + warning_policy: + default_action: report + escalation_high_signal_rule_ids: + - go/request-forgery + - js/missing-rate-limiting + - js/insecure-randomness + +trivy: + blocking_severities: + - CRITICAL + - HIGH + medium_policy: + action: report + escalation: issue-with-sla + +grype: + blocking_severities: + - Critical + - High + medium_policy: + action: report + escalation: issue-with-sla + +enforcement_contract: + codeql_local_vs_ci: "local and ci block on codeql error-level findings only" + supply_chain_medium: "medium vulnerabilities are non-blocking by default and require explicit triage" + auth_regression_guard: "state-changing routes must remain protected by auth middleware" diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index e8277c11..2e3a3ece 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -122,10 +122,28 @@ jobs: exit 1 fi + # shellcheck disable=SC2016 + EFFECTIVE_LEVELS_JQ='[ + .runs[] as $run + | $run.results[] + | . as $result + | ($run.tool.driver.rules // []) as $rules + | (( + $result.level + // (if (($result.ruleIndex | type) == "number") then ($rules[$result.ruleIndex].defaultConfiguration.level // empty) else empty end) + // ([ + $rules[]? + | select((.id // "") == ($result.ruleId // "")) + | (.defaultConfiguration.level // empty) + ][0] // empty) + // "" + ) | ascii_downcase) + ]' + echo "Found SARIF file: $SARIF_FILE" - ERROR_COUNT=$(jq '[.runs[].results[] | select(.level == "error")] | length' "$SARIF_FILE") - WARNING_COUNT=$(jq '[.runs[].results[] | select(.level == "warning")] | length' "$SARIF_FILE") - NOTE_COUNT=$(jq '[.runs[].results[] | select(.level == "note")] | length' "$SARIF_FILE") + ERROR_COUNT=$(jq -r "${EFFECTIVE_LEVELS_JQ} | map(select(. == \"error\")) | length" "$SARIF_FILE") + WARNING_COUNT=$(jq -r "${EFFECTIVE_LEVELS_JQ} | map(select(. == \"warning\")) | length" "$SARIF_FILE") + NOTE_COUNT=$(jq -r "${EFFECTIVE_LEVELS_JQ} | map(select(. == \"note\")) | length" "$SARIF_FILE") { echo "**Findings:**" @@ -135,14 +153,32 @@ jobs: echo "" if [ "$ERROR_COUNT" -gt 0 ]; then - echo "❌ **CRITICAL:** High-severity security issues found!" + echo "❌ **BLOCKING:** CodeQL error-level security issues found" echo "" echo "### Top Issues:" echo '```' - jq -r '.runs[].results[] | select(.level == "error") | "\(.ruleId): \(.message.text)"' "$SARIF_FILE" | head -5 + # shellcheck disable=SC2016 + jq -r ' + .runs[] as $run + | $run.results[] + | . as $result + | ($run.tool.driver.rules // []) as $rules + | (( + $result.level + // (if (($result.ruleIndex | type) == "number") then ($rules[$result.ruleIndex].defaultConfiguration.level // empty) else empty end) + // ([ + $rules[]? + | select((.id // "") == ($result.ruleId // "")) + | (.defaultConfiguration.level // empty) + ][0] // empty) + // "" + ) | ascii_downcase) as $effectiveLevel + | select($effectiveLevel == "error") + | "\($effectiveLevel): \($result.ruleId // \"\"): \($result.message.text)" + ' "$SARIF_FILE" | head -5 echo '```' else - echo "✅ No high-severity issues found" + echo "✅ No blocking CodeQL issues found" fi } >> "$GITHUB_STEP_SUMMARY" @@ -169,9 +205,26 @@ jobs: exit 1 fi - ERROR_COUNT=$(jq '[.runs[].results[] | select(.level == "error")] | length' "$SARIF_FILE") + # shellcheck disable=SC2016 + ERROR_COUNT=$(jq -r '[ + .runs[] as $run + | $run.results[] + | . as $result + | ($run.tool.driver.rules // []) as $rules + | (( + $result.level + // (if (($result.ruleIndex | type) == "number") then ($rules[$result.ruleIndex].defaultConfiguration.level // empty) else empty end) + // ([ + $rules[]? + | select((.id // "") == ($result.ruleId // "")) + | (.defaultConfiguration.level // empty) + ][0] // empty) + // "" + ) | ascii_downcase) as $effectiveLevel + | select($effectiveLevel == "error") + ] | length' "$SARIF_FILE") if [ "$ERROR_COUNT" -gt 0 ]; then - echo "::error::CodeQL found $ERROR_COUNT high-severity security issues. Fix before merging." + echo "::error::CodeQL found $ERROR_COUNT blocking findings (effective-level=error). Fix before merging. Policy: .github/security-severity-policy.yml" exit 1 fi diff --git a/.github/workflows/nightly-build.yml b/.github/workflows/nightly-build.yml index 4e7a2da4..9230e796 100644 --- a/.github/workflows/nightly-build.yml +++ b/.github/workflows/nightly-build.yml @@ -355,10 +355,116 @@ jobs: sarif_file: 'trivy-nightly.sarif' category: 'trivy-nightly' - - name: Check for critical CVEs + - name: Security severity policy summary run: | - if grep -q "CRITICAL" trivy-nightly.sarif; then - echo "❌ Critical vulnerabilities found in nightly build" + { + echo "## 🔐 Nightly Supply Chain Severity Policy" + echo "" + echo "- Blocking: Critical, High" + echo "- Medium: non-blocking by default (report + triage SLA)" + echo "- Policy file: .github/security-severity-policy.yml" + } >> "$GITHUB_STEP_SUMMARY" + + - name: Check for Critical/High CVEs + run: | + set -euo pipefail + + jq -e . trivy-nightly.sarif >/dev/null + + CRITICAL_COUNT=$(jq -r ' + [ + .runs[] as $run + | ($run.tool.driver.rules // []) as $rules + | $run.results[]? + | . as $result + | ( + ( + if (($result.ruleIndex | type) == "number") then + ($rules[$result.ruleIndex].properties["security-severity"] // empty) + else + empty + end + ) + // ([ + $rules[]? + | select((.id // "") == ($result.ruleId // "")) + | .properties["security-severity"] + ][0] // empty) + // empty + ) as $securitySeverity + | (try ($securitySeverity | tonumber) catch empty) as $score + | select($score != null and $score >= 9.0) + ] | length + ' trivy-nightly.sarif) + + HIGH_COUNT=$(jq -r ' + [ + .runs[] as $run + | ($run.tool.driver.rules // []) as $rules + | $run.results[]? + | . as $result + | ( + ( + if (($result.ruleIndex | type) == "number") then + ($rules[$result.ruleIndex].properties["security-severity"] // empty) + else + empty + end + ) + // ([ + $rules[]? + | select((.id // "") == ($result.ruleId // "")) + | .properties["security-severity"] + ][0] // empty) + // empty + ) as $securitySeverity + | (try ($securitySeverity | tonumber) catch empty) as $score + | select($score != null and $score >= 7.0 and $score < 9.0) + ] | length + ' trivy-nightly.sarif) + + MEDIUM_COUNT=$(jq -r ' + [ + .runs[] as $run + | ($run.tool.driver.rules // []) as $rules + | $run.results[]? + | . as $result + | ( + ( + if (($result.ruleIndex | type) == "number") then + ($rules[$result.ruleIndex].properties["security-severity"] // empty) + else + empty + end + ) + // ([ + $rules[]? + | select((.id // "") == ($result.ruleId // "")) + | .properties["security-severity"] + ][0] // empty) + // empty + ) as $securitySeverity + | (try ($securitySeverity | tonumber) catch empty) as $score + | select($score != null and $score >= 4.0 and $score < 7.0) + ] | length + ' trivy-nightly.sarif) + + { + echo "- Structured SARIF counts: CRITICAL=${CRITICAL_COUNT}, HIGH=${HIGH_COUNT}, MEDIUM=${MEDIUM_COUNT}" + } >> "$GITHUB_STEP_SUMMARY" + + if [ "$CRITICAL_COUNT" -gt 0 ]; then + echo "❌ Critical vulnerabilities found in nightly build (${CRITICAL_COUNT})" exit 1 fi - echo "✅ No critical vulnerabilities found" + + if [ "$HIGH_COUNT" -gt 0 ]; then + echo "❌ High vulnerabilities found in nightly build (${HIGH_COUNT})" + exit 1 + fi + + if [ "$MEDIUM_COUNT" -gt 0 ]; then + echo "::warning::Medium vulnerabilities found in nightly build (${MEDIUM_COUNT}). Non-blocking by policy; triage with SLA per .github/security-severity-policy.yml" + fi + + echo "✅ No Critical/High vulnerabilities found" diff --git a/.github/workflows/quality-checks.yml b/.github/workflows/quality-checks.yml index 562c5c05..cef355c1 100644 --- a/.github/workflows/quality-checks.yml +++ b/.github/workflows/quality-checks.yml @@ -18,6 +18,27 @@ env: GOTOOLCHAIN: auto jobs: + auth-route-protection-contract: + name: Auth Route Protection Contract + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + with: + fetch-depth: 0 + ref: ${{ github.sha }} + + - name: Set up Go + uses: actions/setup-go@7a3fe6cf4cb3a834922a1244abfce67bcef6a0c5 # v6.2.0 + with: + go-version: ${{ env.GO_VERSION }} + cache-dependency-path: backend/go.sum + + - name: Run auth protection contract tests + run: | + set -euo pipefail + cd backend + go test ./internal/api/routes -run 'TestRegister_StateChangingRoutesRequireAuthentication|TestRegister_StateChangingRoutesDenyByDefaultWithExplicitAllowlist|TestRegister_AuthenticatedRoutes' -count=1 -v + codecov-trigger-parity-guard: name: Codecov Trigger/Comment Parity Guard runs-on: ubuntu-latest diff --git a/.github/workflows/supply-chain-pr.yml b/.github/workflows/supply-chain-pr.yml index 9c4e2b95..41eb6950 100644 --- a/.github/workflows/supply-chain-pr.yml +++ b/.github/workflows/supply-chain-pr.yml @@ -337,6 +337,27 @@ jobs: echo " Low: ${LOW_COUNT}" echo " Total: ${TOTAL_COUNT}" + - name: Security severity policy summary + if: steps.set-target.outputs.image_name != '' + run: | + CRITICAL_COUNT="${{ steps.vuln-summary.outputs.critical_count }}" + HIGH_COUNT="${{ steps.vuln-summary.outputs.high_count }}" + MEDIUM_COUNT="${{ steps.vuln-summary.outputs.medium_count }}" + + { + echo "## 🔐 Supply Chain Severity Policy" + echo "" + echo "- Blocking: Critical, High" + echo "- Medium: non-blocking by default (report + triage SLA)" + echo "- Policy file: .github/security-severity-policy.yml" + echo "" + echo "Current scan counts: Critical=${CRITICAL_COUNT}, High=${HIGH_COUNT}, Medium=${MEDIUM_COUNT}" + } >> "$GITHUB_STEP_SUMMARY" + + if [[ "${MEDIUM_COUNT}" -gt 0 ]]; then + echo "::warning::${MEDIUM_COUNT} medium vulnerabilities found. Non-blocking by policy; create/maintain triage issue with SLA per .github/security-severity-policy.yml" + fi + - name: Upload SARIF to GitHub Security if: steps.check-artifact.outputs.artifact_found == 'true' uses: github/codeql-action/upload-sarif@89a39a4e59826350b863aa6b6252a07ad50cf83e # v4 @@ -433,10 +454,11 @@ jobs: echo "✅ PR comment posted" - - name: Fail on critical vulnerabilities + - name: Fail on Critical/High vulnerabilities if: steps.set-target.outputs.image_name != '' run: | CRITICAL_COUNT="${{ steps.vuln-summary.outputs.critical_count }}" + HIGH_COUNT="${{ steps.vuln-summary.outputs.high_count }}" if [[ "${CRITICAL_COUNT}" -gt 0 ]]; then echo "🚨 Found ${CRITICAL_COUNT} CRITICAL vulnerabilities!" @@ -444,4 +466,10 @@ jobs: exit 1 fi - echo "✅ No critical vulnerabilities found" + if [[ "${HIGH_COUNT}" -gt 0 ]]; then + echo "🚨 Found ${HIGH_COUNT} HIGH vulnerabilities!" + echo "Please review the vulnerability report and address high severity issues before merging." + exit 1 + fi + + echo "✅ No Critical/High vulnerabilities found" diff --git a/backend/internal/api/handlers/user_handler.go b/backend/internal/api/handlers/user_handler.go index e7d82ded..6b1d884a 100644 --- a/backend/internal/api/handlers/user_handler.go +++ b/backend/internal/api/handlers/user_handler.go @@ -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 } diff --git a/backend/internal/api/handlers/user_handler_test.go b/backend/internal/api/handlers/user_handler_test.go index f62a583e..0629c2e6 100644 --- a/backend/internal/api/handlers/user_handler_test.go +++ b/backend/internal/api/handlers/user_handler_test.go @@ -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 } diff --git a/backend/internal/api/routes/routes.go b/backend/internal/api/routes/routes.go index 9dd443b6..cbd9881d 100644 --- a/backend/internal/api/routes/routes.go +++ b/backend/internal/api/routes/routes.go @@ -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() { diff --git a/backend/internal/api/routes/routes_test.go b/backend/internal/api/routes/routes_test.go index 4e336ed7..d5fcf600 100644 --- a/backend/internal/api/routes/routes_test.go +++ b/backend/internal/api/routes/routes_test.go @@ -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() diff --git a/docs/reports/qa_report.md b/docs/reports/qa_report.md index 9aa7c369..12e8cb41 100644 --- a/docs/reports/qa_report.md +++ b/docs/reports/qa_report.md @@ -260,6 +260,58 @@ PR-3 is **ready to merge** with no open QA blockers. --- +## Final QA/Security Gates Delta — Blocker Remediation Validation + +- Date: 2026-02-25 +- Scope: Current branch state after latest blocker remediations +- Verdict: **FAIL (single blocking gate remains)** + +### Exact Commands Run + +1. `.github/skills/scripts/skill-runner.sh docker-rebuild-e2e` +2. `.github/skills/scripts/skill-runner.sh test-e2e-playwright --project=firefox --grep="auth-api-enforcement|auth-middleware-cascade|authorization-rbac"` +3. `.github/skills/scripts/skill-runner.sh test-e2e-playwright --project=firefox --grep="Security Enforcement API|Auth Middleware Cascade|Cerberus ACL Role-Based Access Control"` +4. `bash scripts/local-patch-report.sh` (first attempt) +5. `go test ./internal/api/routes -run 'TestRegister_StateChangingRoutesDenyByDefaultWithExplicitAllowlist|TestRegister_StateChangingRoutesRequireAuthentication' -count=1` +6. `go test ./internal/api/handlers -run 'TestUserHandler_Setup_OneWayInvariant_ReentryRejectedAndSingleUser|TestUserHandler_Setup_ConcurrentAttemptInvariant|TestUserHandler_Setup_ResponseSecretEchoContract|TestUserHandler_GetProfile_SecretEchoContract|TestUserHandler_ListUsers_SecretEchoContract' -count=1` +7. `bash /projects/Charon/scripts/go-test-coverage.sh` +8. `bash /projects/Charon/scripts/frontend-test-coverage.sh` +9. `bash /projects/Charon/scripts/local-patch-report.sh` (rerun with coverage inputs present) +10. `bash /projects/Charon/.github/skills/scripts/skill-runner.sh security-scan-codeql go summary` +11. `bash /projects/Charon/.github/skills/scripts/skill-runner.sh security-scan-codeql javascript summary` +12. `pre-commit run --hook-stage manual codeql-check-findings --all-files` +13. `pre-commit run --all-files` (first run) +14. `bash /projects/Charon/.github/skills/scripts/skill-runner.sh security-scan-trivy vuln,secret,misconfig json` +15. `bash /projects/Charon/.github/skills/scripts/skill-runner.sh security-scan-docker-image charon:local` +16. `pre-commit run --all-files` (rerun) + +### Gate Results + +| Gate | Status | Evidence | +| --- | --- | --- | +| 1) E2E first (Playwright skill/task path) | PASS | E2E environment rebuilt and Playwright skill run completed with `7 passed` on Firefox. | +| 2) Local patch coverage preflight | PASS (WARN) | First run failed due missing `frontend/coverage/lcov.info`; after coverage generation, rerun produced required artifacts and warn-mode report. | +| 3) Focused backend regressions | PASS | Routes suite: `ok .../internal/api/routes`; handlers suite: `ok .../internal/api/handlers`. | +| 4) Coverage gates | PASS | Backend: statement `87.0%`, line `87.2%` (min 87%). Frontend: lines `88.97%` (min 87%). | +| 5) CodeQL CI-aligned Go + JS + manual findings hook | PASS | Go: `0 errors`; JS: `0 errors`; manual findings hook passed with no blocking findings. | +| 6) `pre-commit run --all-files` | **FAIL (blocking)** | `actionlint` failed on `.github/workflows/codeql.yml` (ShellCheck `SC2016`). | +| 7) Trivy filesystem + image scan | PASS | Filesystem scan completed with no blocking issues; image scan reported Critical=0, High=0, Medium=10, Low=4 (non-blocking by policy). | + +### Blocker Classification + +- **Real code defect (blocking):** `actionlint` failure in `.github/workflows/codeql.yml` (`SC2016`, single-quoted expression handling in shell block). +- **Environment/tooling-only (non-code) observations:** + - VS Code task runner returned `Task started but no terminal was found` for configured tasks in this session. + - `runTests` tool did not discover Go tests for targeted file inputs. + - Initial local patch preflight required coverage artifacts to be generated before successful rerun. + +### Final Gate Decision + +- **DO NOT APPROVE / DO NOT MERGE YET** +- Reason: one unresolved blocking gate remains (`pre-commit --all-files` -> `actionlint` on `.github/workflows/codeql.yml`). + +--- + ## QA/Security Delta — Post-Hardening E2E Remediation Pass - Date: 2026-02-25 diff --git a/scripts/pre-commit-hooks/codeql-check-findings.sh b/scripts/pre-commit-hooks/codeql-check-findings.sh index 03a012e6..df34a648 100755 --- a/scripts/pre-commit-hooks/codeql-check-findings.sh +++ b/scripts/pre-commit-hooks/codeql-check-findings.sh @@ -1,5 +1,5 @@ #!/bin/bash -# Check CodeQL SARIF results for HIGH/CRITICAL findings +# Check CodeQL SARIF results for blocking findings (CI-aligned) set -e RED='\033[0;31m' @@ -24,10 +24,10 @@ check_sarif() { # Check for findings using jq (if available) if command -v jq &> /dev/null; then - # Count high/critical severity findings. - # Note: CodeQL SARIF may omit result-level `level`; when absent, severity - # is defined on the rule metadata (`tool.driver.rules[].defaultConfiguration.level`). - HIGH_COUNT=$(jq -r '[ + # Count blocking findings. + # CI behavior: block only effective level=error (high/critical equivalent); + # warnings are reported but non-blocking unless escalated by policy. + BLOCKING_COUNT=$(jq -r '[ .runs[] as $run | $run.results[] | . as $result @@ -42,13 +42,31 @@ check_sarif() { ][0] // empty) // "" ) | ascii_downcase) as $effectiveLevel - | select($effectiveLevel == "error" or $effectiveLevel == "warning") + | select($effectiveLevel == "error") ] | length' "$sarif_file" 2>/dev/null || echo 0) - if [ "$HIGH_COUNT" -gt 0 ]; then - echo -e "${RED}❌ Found $HIGH_COUNT potential security issues in $lang code${NC}" + WARNING_COUNT=$(jq -r '[ + .runs[] as $run + | $run.results[] + | . as $result + | ($run.tool.driver.rules // []) as $rules + | (( + $result.level + // (if (($result.ruleIndex | type) == "number") then ($rules[$result.ruleIndex].defaultConfiguration.level // empty) else empty end) + // ([ + $rules[]? + | select((.id // "") == ($result.ruleId // "")) + | (.defaultConfiguration.level // empty) + ][0] // empty) + // "" + ) | ascii_downcase) as $effectiveLevel + | select($effectiveLevel == "warning") + ] | length' "$sarif_file" 2>/dev/null || echo 0) + + if [ "$BLOCKING_COUNT" -gt 0 ]; then + echo -e "${RED}❌ Found $BLOCKING_COUNT blocking CodeQL issues in $lang code${NC}" echo "" - echo "Summary:" + echo "Blocking summary (error-level):" jq -r ' .runs[] as $run | $run.results[] @@ -64,30 +82,34 @@ check_sarif() { ][0] // empty) // "" ) | ascii_downcase) as $effectiveLevel - | select($effectiveLevel == "error" or $effectiveLevel == "warning") + | select($effectiveLevel == "error") | "\($effectiveLevel): \($result.ruleId // ""): \($result.message.text) (\($result.locations[0].physicalLocation.artifactLocation.uri):\($result.locations[0].physicalLocation.region.startLine))" ' "$sarif_file" 2>/dev/null | head -10 echo "" echo "View full results: code $sarif_file" FAILED=1 else - echo -e "${GREEN}✅ No security issues found in $lang code${NC}" + echo -e "${GREEN}✅ No blocking CodeQL issues found in $lang code${NC}" + if [ "$WARNING_COUNT" -gt 0 ]; then + echo -e "${YELLOW}⚠️ Non-blocking warnings in $lang: $WARNING_COUNT (policy triage required)${NC}" + fi fi else - # Fallback: check if file has results - if grep -q '"results"' "$sarif_file" && ! grep -q '"results": \[\]' "$sarif_file"; then - echo -e "${YELLOW}⚠️ CodeQL findings detected in $lang (install jq for details)${NC}" - echo "View results: code $sarif_file" - FAILED=1 - else - echo -e "${GREEN}✅ No security issues found in $lang code${NC}" - fi + echo -e "${RED}❌ jq is required for semantic CodeQL severity evaluation (${lang})${NC}" + echo "Install jq and re-run: pre-commit run --hook-stage manual codeql-check-findings --all-files" + FAILED=1 fi } echo "🔒 Checking CodeQL findings..." echo "" + if ! command -v jq &> /dev/null; then + echo -e "${RED}❌ jq is required for CodeQL finding checks${NC}" + echo "Install jq and re-run: pre-commit run --hook-stage manual codeql-check-findings --all-files" + exit 1 + fi + check_sarif "codeql-results-go.sarif" "go" # Support both JS artifact names, preferring the CI-aligned canonical file. @@ -102,7 +124,7 @@ fi if [ $FAILED -eq 1 ]; then echo "" - echo -e "${RED}❌ CodeQL scan found security issues. Please fix before committing.${NC}" + echo -e "${RED}❌ CodeQL scan found blocking findings (error-level). Please fix before committing.${NC}" echo "" echo "To view results:" echo " - VS Code: Install SARIF Viewer extension"