Merge pull request #851 from Wikid82/feature/beta-release

fix(settings): allow empty string as a valid setting value
2026-03-16 23:24:37 -04:00
parent 071ae38d35 5084483984
commit 418fb7d17c
6 changed files with 733 additions and 736 deletions
--- a/backend/internal/api/handlers/settings_handler.go
+++ b/backend/internal/api/handlers/settings_handler.go
@@ -114,7 +114,7 @@ func isSensitiveSettingKey(key string) bool {

 type UpdateSettingRequest struct {
 	Key      string `json:"key" binding:"required"`
-	Value    string `json:"value" binding:"required"`
+	Value    string `json:"value"`
 	Category string `json:"category"`
 	Type     string `json:"type"`
 }
--- a/backend/internal/api/handlers/settings_handler_test.go
+++ b/backend/internal/api/handlers/settings_handler_test.go
@@ -438,6 +438,55 @@ func TestSettingsHandler_UpdateSetting_InvalidAdminWhitelist(t *testing.T) {
 	assert.Contains(t, w.Body.String(), "Invalid admin_whitelist")
 }

+func TestSettingsHandler_UpdateSetting_EmptyValueAccepted(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	db := setupSettingsTestDB(t)
+
+	handler := handlers.NewSettingsHandler(db)
+	router := newAdminRouter()
+	router.POST("/settings", handler.UpdateSetting)
+
+	payload := map[string]string{
+		"key":   "some.setting",
+		"value": "",
+	}
+	body, _ := json.Marshal(payload)
+
+	w := httptest.NewRecorder()
+	req, _ := http.NewRequest("POST", "/settings", bytes.NewBuffer(body))
+	req.Header.Set("Content-Type", "application/json")
+	router.ServeHTTP(w, req)
+
+	assert.Equal(t, http.StatusOK, w.Code)
+
+	var setting models.Setting
+	require.NoError(t, db.Where("key = ?", "some.setting").First(&setting).Error)
+	assert.Equal(t, "some.setting", setting.Key)
+	assert.Equal(t, "", setting.Value)
+}
+
+func TestSettingsHandler_UpdateSetting_MissingKeyRejected(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	db := setupSettingsTestDB(t)
+
+	handler := handlers.NewSettingsHandler(db)
+	router := newAdminRouter()
+	router.POST("/settings", handler.UpdateSetting)
+
+	payload := map[string]string{
+		"value": "some-value",
+	}
+	body, _ := json.Marshal(payload)
+
+	w := httptest.NewRecorder()
+	req, _ := http.NewRequest("POST", "/settings", bytes.NewBuffer(body))
+	req.Header.Set("Content-Type", "application/json")
+	router.ServeHTTP(w, req)
+
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+	assert.Contains(t, w.Body.String(), "Key")
+}
+
 func TestSettingsHandler_UpdateSetting_InvalidKeepaliveIdle(t *testing.T) {
 	gin.SetMode(gin.TestMode)
 	db := setupSettingsTestDB(t)
@@ -744,16 +793,27 @@ func TestSettingsHandler_Errors(t *testing.T) {
 	router.ServeHTTP(w, req)
 	assert.Equal(t, http.StatusBadRequest, w.Code)

-	// Missing Key/Value
+	// Value omitted — allowed since binding:"required" was removed; empty string is a valid value
 	payload := map[string]string{
 		"key": "some_key",
-		// value missing
+		// value intentionally absent; defaults to empty string
 	}
 	body, _ := json.Marshal(payload)
 	req, _ = http.NewRequest("POST", "/settings", bytes.NewBuffer(body))
 	req.Header.Set("Content-Type", "application/json")
 	w = httptest.NewRecorder()
 	router.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusOK, w.Code)
+
+	// Missing key — key is still binding:"required" so this must return 400
+	payloadNoKey := map[string]string{
+		"value": "some_value",
+	}
+	bodyNoKey, _ := json.Marshal(payloadNoKey)
+	req, _ = http.NewRequest("POST", "/settings", bytes.NewBuffer(bodyNoKey))
+	req.Header.Set("Content-Type", "application/json")
+	w = httptest.NewRecorder()
+	router.ServeHTTP(w, req)
 	assert.Equal(t, http.StatusBadRequest, w.Code)
 }

--- a/docs/plans/current_spec.md
+++ b/docs/plans/current_spec.md
--- a/docs/reports/qa_report.md
+++ b/docs/reports/qa_report.md
@@ -1,8 +1,9 @@
-# QA Audit Report — Dockerfile npm CVE Remediation
+# QA Audit Report — PR-1: Allow Empty Value in UpdateSetting

-**Date:** 2026-03-16
-**Scope:** `Dockerfile` — `frontend-builder` stage npm upgrade to address 6 HIGH CVEs in `node:24.14.0-alpine`
-**Reviewer:** QA Security Agent
+**Date:** 2026-03-17
+**Scope:** Remove `binding:"required"` from `Value` field in `UpdateSettingRequest`
+**File:** `backend/internal/api/handlers/settings_handler.go`
+**Purpose:** Allow admins to set a setting to an empty string value (required to fix the fresh-install CrowdSec enabling bug where `value` was legitimately empty).

 ---

--- a/docs/reports/qa_report_pr1_empty_value.md
+++ b/docs/reports/qa_report_pr1_empty_value.md
@@ -0,0 +1,175 @@
+# QA Audit Report — PR-1
+
+**Date:** 2026-03-17
+**Scope:** Remove `binding:"required"` from `Value` field in `UpdateSettingRequest`
+**File:** `backend/internal/api/handlers/settings_handler.go`
+**Purpose:** Allow admins to set a setting to an empty string value (required to fix the fresh-install CrowdSec enabling bug where `value` was legitimately empty).
+
+---
+
+## Summary
+
+| # | Check | Result | Notes |
+|---|-------|--------|-------|
+| 1 | Handler unit tests | ✅ PASS (after fix) | One stale test updated |
+| 2 | Full backend test suite | ✅ PASS | No regressions |
+| 3 | Lint (staticcheck) | ⚠️ SKIP | Pre-existing toolchain mismatch, not PR-1 |
+| 4 | Pre-commit hooks | ⚠️ SKIP | `.pre-commit-config.yaml` not present |
+| 5 | Security — admin auth check | ✅ PASS | Three independent protection layers confirmed |
+| 6 | Trivy scan | ⚠️ NOTE | 1 pre-existing HIGH unrelated to PR-1 |
+
+---
+
+## Check 1 — Handler Unit Tests
+
+**Command:** `go test ./internal/api/handlers/... -v`
+
+**Initial result:** FAIL
+`--- FAIL: TestSettingsHandler_Errors` at `settings_handler_test.go:805`
+
+**Root cause:** The sub-case "Missing Key/Value" sent a payload with `key` but no `value` and asserted `HTTP 400`. This assertion was valid against the old `binding:"required"` constraint on `Value`. After PR-1 removed that constraint, the handler correctly accepts an absent value (empty string) and returns `HTTP 200`, breaking the stale assertion.
+
+**Fix applied:** `backend/internal/api/handlers/settings_handler_test.go`
+
+- Updated the "value absent" sub-case to assert `HTTP 200` — empty string is now a valid value.
+- Added a new sub-case: payload with `value` but no `key` → asserts `HTTP 400` (key remains `binding:"required"`).
+
+**Result after fix:** ✅ PASS
+
+---
+
+## Check 2 — Full Backend Test Suite
+
+**Command:** `go test ./...`
+
+All packages pass:
+
+```
+ok  github.com/Wikid82/charon/backend/cmd/seed
+ok  github.com/Wikid82/charon/backend/internal/api
+ok  github.com/Wikid82/charon/backend/internal/api/handlers
+ok  github.com/Wikid82/charon/backend/internal/api/middleware
+ok  github.com/Wikid82/charon/backend/internal/api/routes
+ok  github.com/Wikid82/charon/backend/internal/api/tests
+ok  github.com/Wikid82/charon/backend/internal/caddy
+ok  github.com/Wikid82/charon/backend/internal/cerberus
+ok  github.com/Wikid82/charon/backend/internal/crowdsec
+ok  github.com/Wikid82/charon/backend/internal/crypto
+ok  github.com/Wikid82/charon/backend/internal/database
+ok  github.com/Wikid82/charon/backend/internal/models
+ok  github.com/Wikid82/charon/backend/internal/notifications
+ok  github.com/Wikid82/charon/backend/internal/services
+ok  github.com/Wikid82/charon/backend/internal/server
+... (all packages green, 0 failures)
+```
+
+**Result:** ✅ PASS — no regressions.
+
+---
+
+## Check 3 — Lint
+
+**Attempted:** `make lint-fast` → target does not exist.
+**Fallback:** `staticcheck ./...`
+
+**Output:**
+```
+-: module requires at least go1.26.1, but Staticcheck was built with go1.25.5 (compile)
+```
+
+Pre-existing toolchain version mismatch between the installed `staticcheck` binary (`go1.25.5`) and the module minimum (`go1.26.1`). Not caused by this PR.
+
+**Result:** ⚠️ SKIP — pre-existing environment issue, not a PR-1 blocker.
+
+---
+
+## Check 4 — Pre-commit Hooks
+
+**Command:** `pre-commit run --all-files`
+**Output:** `InvalidConfigError: .pre-commit-config.yaml is not a file`
+
+No `.pre-commit-config.yaml` exists at the repository root; `lefthook.yml` is the configured hook runner.
+
+**Result:** ⚠️ SKIP — `pre-commit` not configured for this repo.
+
+---
+
+## Check 5 — Security: Endpoint Authentication
+
+The change removes only an input validation constraint on the `Value` field. No authorization logic is touched. Verified the full protection chain:
+
+### Route registration (`backend/internal/api/routes/routes.go`)
+
+```
+protected := api.Group("/")
+protected.Use(authMiddleware)               // Layer 1: JWT required → 401 otherwise
+
+    management := protected.Group("/")
+    management.Use(RequireManagementAccess())  // Layer 2: blocks passthrough role → 403
+
+        management.POST("/settings", settingsHandler.UpdateSetting)
+        management.PATCH("/settings", settingsHandler.UpdateSetting)
+```
+
+### Handler guard (`settings_handler.go` line 124)
+
+```go
+func (h *SettingsHandler) UpdateSetting(c *gin.Context) {
+    if !requireAdmin(c) {   // Layer 3: admin role required → 403 otherwise
+        return
+    }
+    ...
+}
+```
+
+### Layer summary
+
+| Layer | Mechanism | Failure response |
+|-------|-----------|-----------------|
+| 1 | `authMiddleware` (JWT) | 401 Unauthorized |
+| 2 | `RequireManagementAccess()` middleware | 403 Forbidden (passthrough role) |
+| 3 | `requireAdmin(c)` in handler | 403 Forbidden (non-admin) |
+
+An unauthenticated or non-admin request cannot reach the `Value` binding logic.
+The PR-1 change affects only the binding validation of `Value`; the authorization path is unchanged.
+
+**Result:** ✅ PASS — endpoint is properly admin-gated by three independent layers.
+
+---
+
+## Check 6 — Trivy Security Scan
+
+`trivy` CLI not installed in this environment. Analysis based on cached `trivy-image-report.json`
+(generated 2026-02-25, Trivy v0.69.1, image `charon:local`).
+
+| Severity | Count | Details |
+|----------|-------|---------|
+| CRITICAL | 0 | None |
+| HIGH | 1 | CVE-2026-25793 — `github.com/slackhq/nebula v1.9.7` |
+
+**CVE-2026-25793 detail:**
+- **Package:** `github.com/slackhq/nebula v1.9.7`
+- **Fixed in:** `v1.10.3`
+- **Title:** Blocklist evasion via ECDSA Signature Malleability
+- **Relation to PR-1:** None — pre-existing transitive dependency issue, tracked separately.
+
+**Result:** ⚠️ NOTE — 1 pre-existing HIGH; no new vulnerabilities introduced by PR-1.
+
+---
+
+## Overall Verdict
+
+**PR-1 is safe to merge** with the accompanying test fix.
+
+The change is behaviorally correct: an empty string is a valid setting value for certain admin
+operations (specifically the CrowdSec enrollment flow on fresh installs). Authorization is
+unaffected — three independent layers continue to restrict this endpoint to authenticated admins.
+
+### Required: Test fix
+
+`backend/internal/api/handlers/settings_handler_test.go` — `TestSettingsHandler_Errors` updated
+to reflect the new contract (empty value → 200; missing key → 400 still enforced).
+
+### Tracked separately
+
+- `github.com/slackhq/nebula` should be bumped to `v1.10.3` to resolve CVE-2026-25793.
--- a/frontend/package-lock.json
+++ b/frontend/package-lock.json
@@ -457,23 +457,23 @@
      }
    },
    "node_modules/@babel/helpers": {
-      "version": "7.28.6",
-      "resolved": "https://registry.npmjs.org/@babel/helpers/-/helpers-7.28.6.tgz",
-      "integrity": "sha512-xOBvwq86HHdB7WUDTfKfT/Vuxh7gElQ+Sfti2Cy6yIWNW05P8iUslOVcZ4/sKbE+/jQaukQAdz/gf3724kYdqw==",
+      "version": "7.29.2",
+      "resolved": "https://registry.npmjs.org/@babel/helpers/-/helpers-7.29.2.tgz",
+      "integrity": "sha512-HoGuUs4sCZNezVEKdVcwqmZN8GoHirLUcLaYVNBK2J0DadGtdcqgr3BCbvH8+XUo4NGjNl3VOtSjEKNzqfFgKw==",
      "dev": true,
      "license": "MIT",
      "dependencies": {
        "@babel/template": "^7.28.6",
-        "@babel/types": "^7.28.6"
+        "@babel/types": "^7.29.0"
      },
      "engines": {
        "node": ">=6.9.0"
      }
    },
    "node_modules/@babel/parser": {
-      "version": "7.29.0",
-      "resolved": "https://registry.npmjs.org/@babel/parser/-/parser-7.29.0.tgz",
-      "integrity": "sha512-IyDgFV5GeDUVX4YdF/3CPULtVGSXXMLh1xVIgdCgxApktqnQV0r7/8Nqthg+8YLGaAtdyIlo2qIdZrbCv4+7ww==",
+      "version": "7.29.2",
+      "resolved": "https://registry.npmjs.org/@babel/parser/-/parser-7.29.2.tgz",
+      "integrity": "sha512-4GgRzy/+fsBa72/RZVJmGKPmZu9Byn8o4MoLpmNe1m8ZfYnz5emHLQz3U4gLud6Zwl0RZIcgiLD7Uq7ySFuDLA==",
      "dev": true,
      "license": "MIT",
      "dependencies": {
@@ -505,9 +505,9 @@
      }
    },
    "node_modules/@babel/runtime": {
-      "version": "7.28.6",
-      "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.28.6.tgz",
-      "integrity": "sha512-05WQkdpL9COIMz4LjTxGpPNCdlpyimKppYNoJ5Di5EUObifl8t4tuLuUBBZEpoLYOmfvIWrsp9fCl0HoPRVTdA==",
+      "version": "7.29.2",
+      "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.29.2.tgz",
+      "integrity": "sha512-JiDShH45zKHWyGe4ZNVRrCjBz8Nh9TMmZG1kh4QTK8hCBTWBi8Da+i7s1fJw7/lYpM4ccepSNfqzZ/QvABBi5g==",
      "license": "MIT",
      "engines": {
        "node": ">=6.9.0"
@@ -5235,9 +5235,9 @@
      "license": "MIT"
    },
    "node_modules/enhanced-resolve": {
-      "version": "5.20.0",
-      "resolved": "https://registry.npmjs.org/enhanced-resolve/-/enhanced-resolve-5.20.0.tgz",
-      "integrity": "sha512-/ce7+jQ1PQ6rVXwe+jKEg5hW5ciicHwIQUagZkp6IufBoY3YDgdTTY1azVs0qoRgVmvsNB+rbjLJxDAeHHtwsQ==",
+      "version": "5.20.1",
+      "resolved": "https://registry.npmjs.org/enhanced-resolve/-/enhanced-resolve-5.20.1.tgz",
+      "integrity": "sha512-Qohcme7V1inbAfvjItgw0EaxVX5q2rdVEZHRBrEQdRZTssLDGsL8Lwrznl8oQ/6kuTJONLaDcGjkNP247XEhcA==",
      "dev": true,
      "license": "MIT",
      "dependencies": {