diff --git a/CHANGELOG.md b/CHANGELOG.md index 342812a3..ea12fcb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed: Added robust validation and debug logging for Docker image tags to prevent invalid reference errors. - Fixed: Removed log masking for image references and added manifest validation to debug CI failures. +- **Proxy Hosts**: Fixed ACL and Security Headers dropdown selections so create/edit saves now keep the selected values (including clearing to none) after submit and reload. - **CI**: Fixed Docker image reference output so integration jobs never pull an empty image ref - **E2E Test Reliability**: Resolved test timeout issues affecting CI/CD pipeline stability - Fixed config reload overlay blocking test interactions diff --git a/backend/internal/api/handlers/proxy_host_handler.go b/backend/internal/api/handlers/proxy_host_handler.go index 2433b74a..1fd9b449 100644 --- a/backend/internal/api/handlers/proxy_host_handler.go +++ b/backend/internal/api/handlers/proxy_host_handler.go @@ -453,54 +453,12 @@ func (h *ProxyHostHandler) Update(c *gin.Context) { // Security Header Profile: update only if provided if v, ok := payload["security_header_profile_id"]; ok { - logger := middleware.GetRequestLogger(c) - // Sanitize user-provided values for log injection protection (CWE-117) - safeUUID := sanitizeForLog(uuidStr) - logger.WithField("host_uuid", safeUUID).WithField("raw_value", sanitizeForLog(fmt.Sprintf("%v", v))).Debug("Processing security_header_profile_id update") - - if v == nil { - logger.WithField("host_uuid", safeUUID).Debug("Setting security_header_profile_id to nil") - host.SecurityHeaderProfileID = nil - } else { - conversionSuccess := false - switch t := v.(type) { - case float64: - logger.Debug("Received security_header_profile_id as float64") - if id, ok := safeFloat64ToUint(t); ok { - host.SecurityHeaderProfileID = &id - conversionSuccess = true - logger.Info("Successfully converted security_header_profile_id from float64") - } else { - logger.Warn("Failed to convert security_header_profile_id from float64: value is negative or not a valid uint") - } - case int: - logger.Debug("Received security_header_profile_id as int") - if id, ok := safeIntToUint(t); ok { - host.SecurityHeaderProfileID = &id - conversionSuccess = true - logger.Info("Successfully converted security_header_profile_id from int") - } else { - logger.Warn("Failed to convert security_header_profile_id from int: value is negative") - } - case string: - logger.Debug("Received security_header_profile_id as string") - if n, err := strconv.ParseUint(t, 10, 32); err == nil { - id := uint(n) - host.SecurityHeaderProfileID = &id - conversionSuccess = true - logger.WithField("host_uuid", safeUUID).WithField("profile_id", id).Info("Successfully converted security_header_profile_id from string") - } else { - logger.Warn("Failed to parse security_header_profile_id from string") - } - default: - logger.Warn("Unsupported type for security_header_profile_id") - } - - if !conversionSuccess { - c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("invalid security_header_profile_id: unable to convert value %v of type %T to uint", v, v)}) - return - } + parsedID, _, parseErr := parseNullableUintField(v, "security_header_profile_id") + if parseErr != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": parseErr.Error()}) + return } + host.SecurityHeaderProfileID = parsedID } // Locations: replace only if provided diff --git a/backend/internal/api/handlers/proxy_host_handler_update_test.go b/backend/internal/api/handlers/proxy_host_handler_update_test.go index 698d8bd0..536f54a9 100644 --- a/backend/internal/api/handlers/proxy_host_handler_update_test.go +++ b/backend/internal/api/handlers/proxy_host_handler_update_test.go @@ -75,6 +75,145 @@ func createTestSecurityHeaderProfile(t *testing.T, db *gorm.DB, name string) mod return profile } +// createTestAccessList creates an access list for testing. +func createTestAccessList(t *testing.T, db *gorm.DB, name string) models.AccessList { + t.Helper() + acl := models.AccessList{ + UUID: uuid.NewString(), + Name: name, + Type: "ip", + Enabled: true, + } + require.NoError(t, db.Create(&acl).Error) + return acl +} + +func TestProxyHostUpdate_AccessListID_Transitions_NoUnrelatedMutation(t *testing.T) { + t.Parallel() + router, db := setupUpdateTestRouter(t) + + aclOne := createTestAccessList(t, db, "ACL One") + aclTwo := createTestAccessList(t, db, "ACL Two") + + host := models.ProxyHost{ + UUID: uuid.NewString(), + Name: "Access List Transition Host", + DomainNames: "acl-transition.test.com", + ForwardScheme: "http", + ForwardHost: "localhost", + ForwardPort: 8080, + Enabled: true, + SSLForced: true, + Application: "none", + AccessListID: &aclOne.ID, + } + require.NoError(t, db.Create(&host).Error) + + assertUnrelatedFields := func(t *testing.T, current models.ProxyHost) { + t.Helper() + assert.Equal(t, "Access List Transition Host", current.Name) + assert.Equal(t, "acl-transition.test.com", current.DomainNames) + assert.Equal(t, "localhost", current.ForwardHost) + assert.Equal(t, 8080, current.ForwardPort) + assert.True(t, current.SSLForced) + assert.Equal(t, "none", current.Application) + } + + runUpdate := func(t *testing.T, update map[string]any) { + t.Helper() + body, _ := json.Marshal(update) + req := httptest.NewRequest(http.MethodPut, "/api/v1/proxy-hosts/"+host.UUID, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + require.Equal(t, http.StatusOK, resp.Code) + } + + // value -> value + runUpdate(t, map[string]any{"access_list_id": aclTwo.ID}) + var updated models.ProxyHost + require.NoError(t, db.First(&updated, "uuid = ?", host.UUID).Error) + require.NotNil(t, updated.AccessListID) + assert.Equal(t, aclTwo.ID, *updated.AccessListID) + assertUnrelatedFields(t, updated) + + // value -> null + runUpdate(t, map[string]any{"access_list_id": nil}) + require.NoError(t, db.First(&updated, "uuid = ?", host.UUID).Error) + assert.Nil(t, updated.AccessListID) + assertUnrelatedFields(t, updated) + + // null -> value + runUpdate(t, map[string]any{"access_list_id": aclOne.ID}) + require.NoError(t, db.First(&updated, "uuid = ?", host.UUID).Error) + require.NotNil(t, updated.AccessListID) + assert.Equal(t, aclOne.ID, *updated.AccessListID) + assertUnrelatedFields(t, updated) +} + +func TestProxyHostUpdate_SecurityHeaderProfileID_Transitions_NoUnrelatedMutation(t *testing.T) { + t.Parallel() + router, db := setupUpdateTestRouter(t) + + profileOne := createTestSecurityHeaderProfile(t, db, "Security Profile One") + profileTwo := createTestSecurityHeaderProfile(t, db, "Security Profile Two") + + host := models.ProxyHost{ + UUID: uuid.NewString(), + Name: "Security Profile Transition Host", + DomainNames: "security-transition.test.com", + ForwardScheme: "http", + ForwardHost: "localhost", + ForwardPort: 9090, + Enabled: true, + SSLForced: true, + Application: "none", + SecurityHeaderProfileID: &profileOne.ID, + } + require.NoError(t, db.Create(&host).Error) + + assertUnrelatedFields := func(t *testing.T, current models.ProxyHost) { + t.Helper() + assert.Equal(t, "Security Profile Transition Host", current.Name) + assert.Equal(t, "security-transition.test.com", current.DomainNames) + assert.Equal(t, "localhost", current.ForwardHost) + assert.Equal(t, 9090, current.ForwardPort) + assert.True(t, current.SSLForced) + assert.Equal(t, "none", current.Application) + } + + runUpdate := func(t *testing.T, update map[string]any) { + t.Helper() + body, _ := json.Marshal(update) + req := httptest.NewRequest(http.MethodPut, "/api/v1/proxy-hosts/"+host.UUID, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + require.Equal(t, http.StatusOK, resp.Code) + } + + // value -> value + runUpdate(t, map[string]any{"security_header_profile_id": fmt.Sprintf("%d", profileTwo.ID)}) + var updated models.ProxyHost + require.NoError(t, db.First(&updated, "uuid = ?", host.UUID).Error) + require.NotNil(t, updated.SecurityHeaderProfileID) + assert.Equal(t, profileTwo.ID, *updated.SecurityHeaderProfileID) + assertUnrelatedFields(t, updated) + + // value -> null + runUpdate(t, map[string]any{"security_header_profile_id": ""}) + require.NoError(t, db.First(&updated, "uuid = ?", host.UUID).Error) + assert.Nil(t, updated.SecurityHeaderProfileID) + assertUnrelatedFields(t, updated) + + // null -> value + runUpdate(t, map[string]any{"security_header_profile_id": fmt.Sprintf("%d", profileOne.ID)}) + require.NoError(t, db.First(&updated, "uuid = ?", host.UUID).Error) + require.NotNil(t, updated.SecurityHeaderProfileID) + assert.Equal(t, profileOne.ID, *updated.SecurityHeaderProfileID) + assertUnrelatedFields(t, updated) +} + // TestProxyHostUpdate_EnableStandardHeaders_Null tests updating enable_standard_headers to null. func TestProxyHostUpdate_EnableStandardHeaders_Null(t *testing.T) { t.Parallel() diff --git a/docs/issues/manual_test_acl_security_headers_dropdown_hotfix.md b/docs/issues/manual_test_acl_security_headers_dropdown_hotfix.md new file mode 100644 index 00000000..23abaef4 --- /dev/null +++ b/docs/issues/manual_test_acl_security_headers_dropdown_hotfix.md @@ -0,0 +1,77 @@ +## Manual Test Plan — ACL + Security Headers Dropdown Hotfix + +- Date: 2026-02-27 +- Scope: Proxy Host create/edit dropdown persistence +- Goal: Confirm ACL and Security Headers selections save correctly, can be changed, and can be cleared without regressions. + +## Preconditions + +- [ ] Charon is running and reachable in browser +- [ ] At least 2 Access Lists exist +- [ ] At least 2 Security Headers profiles exist +- [ ] Tester has permission to create and edit Proxy Hosts + +## Test Cases + +### TC-001 — Create Host With Both Dropdowns Set + +- Steps: + 1. Open Proxy Hosts and start creating a new host. + 2. Fill required host fields. + 3. Select any Access List. + 4. Select any Security Headers profile. + 5. Save. + 6. Reopen the same host in edit mode. +- Expected: + - The selected Access List remains selected. + - The selected Security Headers profile remains selected. + +### TC-002 — Edit Host And Change Both Selections + +- Steps: + 1. Open an existing host that already has both values set. + 2. Change Access List to a different option. + 3. Change Security Headers to a different option. + 4. Save. + 5. Reopen the host. +- Expected: + - New Access List is persisted. + - New Security Headers profile is persisted. + - Previous values are not shown. + +### TC-003 — Clear Access List + +- Steps: + 1. Open an existing host with an Access List selected. + 2. Set Access List to no selection. + 3. Save. + 4. Reopen the host. +- Expected: + - Access List is empty (none). + - No old Access List value returns. + +### TC-004 — Clear Security Headers + +- Steps: + 1. Open an existing host with a Security Headers profile selected. + 2. Set Security Headers to no selection. + 3. Save. + 4. Reopen the host. +- Expected: + - Security Headers is empty (none). + - No old profile value returns. + +### TC-005 — Regression Guard: Repeated Edit Cycles + +- Steps: + 1. Repeat edit/save cycle 3 times on one host. + 2. Alternate between selecting values and clearing values for both dropdowns. + 3. After each save, reopen the host. +- Expected: + - Last saved choice is always what appears after reopen. + - No mismatch between what was selected and what is shown. + +## Execution Notes + +- Targeted tests for this hotfix are already passing. +- Full-suite, security, and coverage gates are deferred to CI/end pass. diff --git a/docs/plans/current_spec.md b/docs/plans/current_spec.md index 1a1b2618..a06508e1 100644 --- a/docs/plans/current_spec.md +++ b/docs/plans/current_spec.md @@ -1,155 +1,270 @@ +# ACL + Security Headers Hotfix Plan (Proxy Host Create/Edit) + ## 1. Introduction ### Overview -Compatibility rollout for Caddy `2.11.1` is already reflected in the build -default (`Dockerfile` currently sets `ARG CADDY_VERSION=2.11.1`). +Hotfix request: Proxy Host form dropdown selections for Access Control List (ACL) and Security Headers are not being applied/persisted for new or edited hosts. -This plan is now focused on rollout verification and regression-proofing, not -changing the default ARG. +Reported behavior: +1. Existing hosts with previously assigned ACL/Security Header profile retain old values. +2. Users cannot reliably remove or change those values in UI. +3. Newly created hosts cannot reliably apply ACL/Security Header profile. ### Objective -Establish deterministic, evidence-backed gates that prove published images and -security artifacts are fresh, digest-bound, and aligned across registries for -the Caddy `2.11.1` rollout. +Deliver an urgent but correct root-cause fix across frontend binding and backend persistence flow, with minimum user interruption and full validation gates. -## 2. Current State (Verified) +## 2. Research Findings (Current Architecture + Touchpoints) -1. `Dockerfile` default is already `CADDY_VERSION=2.11.1`. -2. `ARCHITECTURE.md` now reports Caddy `2.11.1`. -3. Existing scan artifacts can become stale if not explicitly tied to pushed - digests. +### Frontend Entry Points +1. `frontend/src/pages/ProxyHosts.tsx` + - `handleSubmit(data)` calls `updateHost(editingHost.uuid, data)` or `createHost(data)`. + - Renders `ProxyHostForm` modal for create/edit flows. +2. `frontend/src/components/ProxyHostForm.tsx` + - Local form state initializes `access_list_id` and `security_header_profile_id`. + - ACL control uses `AccessListSelector`. + - Security Headers control uses `Select` with `security_header_profile_id` mapping. + - Submission path: `handleSubmit` -> `onSubmit(payloadWithoutUptime)`. +3. `frontend/src/components/AccessListSelector.tsx` + - Converts select values between `string` and `number | null`. -## 3. Technical Specification (EARS) +### Frontend API/Hooks +1. `frontend/src/hooks/useProxyHosts.ts` + - `createHost` -> `createProxyHost`. + - `updateHost` -> `updateProxyHost`. +2. `frontend/src/api/proxyHosts.ts` + - `createProxyHost(host: Partial)` -> `POST /api/v1/proxy-hosts`. + - `updateProxyHost(uuid, host)` -> `PUT /api/v1/proxy-hosts/:uuid`. + - Contract fields: `access_list_id`, `security_header_profile_id`. -1. WHEN image builds run without an explicit `CADDY_VERSION` override, THE - SYSTEM SHALL continue producing Caddy `2.11.1`. -2. WHEN an image tag is pushed, THE SYSTEM SHALL validate index digest parity - between GHCR and Docker Hub for that same tag. -3. WHEN multi-arch images are published, THE SYSTEM SHALL validate per-arch - digest parity across GHCR and Docker Hub for each platform present. -4. WHEN vulnerability and SBOM scans execute, THE SYSTEM SHALL scan - `image@sha256:` instead of mutable tags. -5. WHEN scan artifacts are generated, THE SYSTEM SHALL prove artifacts were - produced after the push event in the same validation run. -6. IF a verification gate fails, THEN THE SYSTEM SHALL block rollout sign-off - until all gates pass. +### Backend Entry/Transformation/Persistence +1. Route registration + - `backend/internal/api/routes/routes.go`: `proxyHostHandler.RegisterRoutes(protected)`. +2. Handler + - `backend/internal/api/handlers/proxy_host_handler.go` + - `Create(c)` uses `ShouldBindJSON(&models.ProxyHost{})`. + - `Update(c)` uses `map[string]any` partial update parsing. + - Target fields: + - `payload["access_list_id"]` -> `parseNullableUintField` -> `host.AccessListID` + - `payload["security_header_profile_id"]` -> typed conversion -> `host.SecurityHeaderProfileID` +3. Service + - `backend/internal/services/proxyhost_service.go` + - `Create(host)` validates + `db.Create(host)`. + - `Update(host)` validates + `db.Model(...).Select("*").Updates(host)`. +4. Model + - `backend/internal/models/proxy_host.go` + - `AccessListID *uint \`json:"access_list_id"\`` + - `SecurityHeaderProfileID *uint \`json:"security_header_profile_id"\`` -## 4. Scope and Planned Edits +### Existing Tests Relevant to Incident +1. Frontend unit regression coverage already exists: + - `frontend/src/components/__tests__/ProxyHostForm-dropdown-changes.test.tsx` +2. E2E regression spec exists: + - `tests/proxy-host-dropdown-fix.spec.ts` +3. Backend update and security-header tests exist: + - `backend/internal/api/handlers/proxy_host_handler_update_test.go` + - `backend/internal/api/handlers/proxy_host_handler_security_headers_test.go` -### In scope -1. `docs/plans/current_spec.md` (this plan refresh). -2. `ARCHITECTURE.md` version sync is already complete (`2.11.1`); no pending - update is required in this plan. -3. Verification workflow/checklist updates needed to enforce deterministic gates. +## 3. Root-Cause-First Trace -### Out of scope -1. No functional Caddy build logic changes unless a verification failure proves - they are required. -2. No plugin list or patch-scenario refactors. +### Trace Model (Mandatory) +1. Entry Point: + - UI dropdown interactions in `ProxyHostForm` and `AccessListSelector`. +2. Transformation: + - Form state conversion (`string` <-> `number | null`) and payload construction in `ProxyHostForm`. + - API serialization via `frontend/src/api/proxyHosts.ts`. +3. Persistence: + - Backend `Update` parser (`proxy_host_handler.go`) and `ProxyHostService.Update` persistence. +4. Exit Point: + - Response body consumed by React Query invalidation/refetch in `useProxyHosts`. + - UI reflects updated values in table/form. -## 5. Deterministic Acceptance Gates +### Most Likely Failure Zones +1. Frontend select binding/conversion drift (top candidate) + - Shared symptom across ACL and Security Headers points to form/select layer. + - Candidate files: + - `frontend/src/components/ProxyHostForm.tsx` + - `frontend/src/components/AccessListSelector.tsx` + - `frontend/src/components/ui/Select.tsx` +2. Payload mutation or stale form object behavior + - Ensure payload carries updated `access_list_id` / `security_header_profile_id` values at submit time. +3. Backend partial-update parser edge behavior + - Ensure `nil`, numeric string, and number conversions are consistent between ACL and security header profile paths. -### Gate 1: Digest Freshness (pre/post push) -1. Capture pre-push index digest for target tag on GHCR and Docker Hub. -2. Push image. -3. Capture post-push index digest on GHCR and Docker Hub. -4. Pass criteria: - - Post-push index digest changed as expected from pre-push (or matches - intended new digest when creating new tag). - - GHCR and Docker Hub index digests are identical for the tag. - - Per-arch digests are identical across registries for each published - platform. +### Investigation Decision +Root-cause verification will be instrumented through failing-first Playwright scenario and targeted handler tests before applying code changes. -### Gate 2: Digest-Bound Rescan -1. Resolve the post-push index digest. -2. Run all security scans against immutable ref: - - `ghcr.io//@sha256:` - - Optional mirror check against Docker Hub digest ref. -3. Pass criteria: - - No scan uses mutable tags as the primary target. - - Artifact metadata and logs show digest reference. +## 4. EARS Requirements -### Gate 3: Artifact Freshness -1. Record push timestamp and digest capture timestamp. -2. Generate SBOM and vuln artifacts after push in the same run. -3. Pass criteria: - - Artifact generation timestamps are greater than push timestamp. - - Artifacts are newly created/overwritten in this run. - - Evidence ties each artifact to the scanned digest. +1. WHEN a user selects an ACL in the Proxy Host create/edit form, THE SYSTEM SHALL persist `access_list_id` and return it in API response. +2. WHEN a user changes ACL from one value to another, THE SYSTEM SHALL replace prior `access_list_id` with the new value. +3. WHEN a user selects "No Access Control", THE SYSTEM SHALL persist `access_list_id = null`. +4. WHEN a user selects a Security Headers profile in the Proxy Host create/edit form, THE SYSTEM SHALL persist `security_header_profile_id` and return it in API response. +5. WHEN a user changes Security Headers profile from one value to another, THE SYSTEM SHALL replace prior `security_header_profile_id` with the new value. +6. WHEN a user selects "None" for Security Headers, THE SYSTEM SHALL persist `security_header_profile_id = null`. +7. IF dropdown interaction fails to update internal form state, THEN THE SYSTEM SHALL prevent stale values from being persisted. +8. WHILE updating Proxy Host settings, THE SYSTEM SHALL maintain existing behavior for unrelated fields and not regress certificate, DNS challenge, or uptime-linked updates. -### Gate 4: Evidence Block (mandatory) -Every validation run must include a structured evidence block with: -1. Tag name. -2. Index digest. -3. Per-arch digests. -4. Scan tool versions. -5. Push and scan timestamps. -6. Artifact file names produced in this run. +Note: User-visible blocking error behavior is deferred unless required by confirmed root cause. -## 6. Implementation Plan +## 5. Technical Specification (Hotfix Scope) -### Phase 1: Baseline Capture -1. Confirm current `Dockerfile` default remains `2.11.1`. -2. Capture pre-push digest state for target tag across both registries. +### API Contract (No Breaking Change) +1. `POST /api/v1/proxy-hosts` + - Request fields include `access_list_id`, `security_header_profile_id` as nullable numeric fields. +2. `PUT /api/v1/proxy-hosts/:uuid` + - Partial payload accepts nullable updates for both fields. +3. Response must echo persisted values in snake_case: + - `access_list_id` + - `security_header_profile_id` -### Phase 2: Docs Sync -1. Confirm `ARCHITECTURE.md` remains synced at Caddy `2.11.1`. +### Data Model/DB +No schema migration expected. Existing nullable FK fields in `backend/internal/models/proxy_host.go` are sufficient. -### Phase 3: Push and Verification -1. Push validation tag. -2. Execute Gate 1 (digest freshness and parity). -3. Execute Gate 2 (digest-bound rescan). -4. Execute Gate 3 (artifact freshness). -5. Produce Gate 4 evidence block. +### Targeted Code Areas for Fix +1. Frontend + - `frontend/src/components/ProxyHostForm.tsx` + - `frontend/src/components/AccessListSelector.tsx` + - `frontend/src/components/ui/Select.tsx` (only if click/select propagation issue confirmed) + - `frontend/src/api/proxyHosts.ts` (only if serialization issue confirmed) +2. Backend + - `backend/internal/api/handlers/proxy_host_handler.go` (only if parsing/persistence mismatch confirmed) + - `backend/internal/services/proxyhost_service.go` (only if update write path proves incorrect) -### Phase 4: Sign-off -1. Mark rollout verified only when all gates pass. -2. If any gate fails, open follow-up remediation task before merge. +## 6. Edge Cases -## 7. Acceptance Criteria +1. Edit host with existing ACL/profile and switch to another value. +2. Edit host with existing ACL/profile and clear to null. +3. Create new host with ACL/profile set before first save. +4. Submit with stringified numeric values (defensive compatibility). +5. Submit with null values for both fields simultaneously. +6. Missing/deleted profile or ACL IDs in backend (validation errors). +7. Multiple rapid dropdown changes before save (last selection wins). -1. Plan and execution no longer assume Dockerfile default is beta. -2. Objective is rollout verification/regression-proofing for Caddy `2.11.1`. -3. `ARCHITECTURE.md` version metadata is included in required docs sync. -4. Digest freshness gate passes: - - Pre/post push validation completed. - - GHCR and Docker Hub index digest parity confirmed. - - Per-arch digest parity confirmed. -5. Digest-bound rescan gate passes with `image@sha256` scan targets. -6. Artifact freshness gate passes with artifacts produced after push in the same - run. -7. Evidence block is present and complete with: - - Tag - - Index digest - - Per-arch digests - - Scan tool versions - - Timestamps - - Artifact names +## 7. Risk Analysis -## 8. PR Slicing Strategy +### High Risk +1. Silent stale-state submission from form controls. +2. Regressing other Proxy Host settings due to broad payload mutation. + +### Medium Risk +1. Partial-update parser divergence between ACL and security profile behavior. +2. UI select portal/z-index interaction causing non-deterministic click handling. + +### Mitigations +1. Reproduce with Playwright first and capture exact failing action path. +2. Add/strengthen focused frontend tests around create/edit/clear flows. +3. Add/strengthen backend tests for nullable + conversion paths. +4. Keep hotfix minimal and avoid unrelated refactors. + +## 8. Implementation Plan (Urgent, Minimal Interruption) + +### Phase 1: Reproduction + Guardrails (Playwright First) +1. Execute targeted E2E spec for dropdown flow and create/edit persistence behavior. +2. Capture exact failure step and confirm whether failure is click binding, payload value, or backend persistence. +3. Add/adjust failing-first test if current suite does not capture observed production regression. + +### Phase 2: Frontend Fix +1. Patch select binding/state mapping for ACL and Security Headers in `ProxyHostForm`/`AccessListSelector`. +2. If needed, patch `ui/Select` interaction layering. +3. Ensure payload contains correct final `access_list_id` and `security_header_profile_id` values at submit. +4. Extend `ProxyHostForm` tests for create/edit/change/remove flows. + +### Phase 3: Backend Hardening (Conditional) +1. Only if frontend payload is correct but persistence is wrong: + - Backend fix MUST use field-scoped partial-update semantics for `access_list_id` and `security_header_profile_id` only (unless separately justified). + - Ensure write path persists null transitions reliably. +2. Add/adjust handler/service regression tests proving no unintended mutation of unrelated proxy host fields during these targeted updates. + +### Phase 4: Integration + Regression +1. Run complete targeted Proxy Host UI flow tests. +2. Validate list refresh and modal reopen reflect persisted values. +3. Validate no regressions in bulk ACL / bulk security-header operations. + +### Phase 5: Documentation + Handoff +1. Update changelog/release notes only for hotfix behavior. +2. Keep architecture docs unchanged unless root cause requires architectural note. +3. Handoff to Supervisor agent for review after plan approval and implementation. + +## 9. Acceptance Criteria + +1. ACL dropdown selection persists on create and edit. +2. Security Headers dropdown selection persists on create and edit. +3. Clearing ACL persists `null` and is reflected after reload. +4. Clearing Security Headers persists `null` and is reflected after reload. +5. Existing hosts can change from one ACL/profile to another without stale value retention. +6. New hosts can apply ACL/profile at creation time. +7. No regressions in unrelated proxy host fields. +8. All validation gates in Section 11 pass. +9. API create response returns persisted `access_list_id` and `security_header_profile_id` matching submitted values (including `null`). +10. API update response returns persisted `access_list_id` and `security_header_profile_id` after `value->value`, `value->null`, and `null->value` transitions. +11. Backend persistence verification confirms unrelated proxy host fields remain unchanged for targeted updates. + +## 10. PR Slicing Strategy ### Decision -Single PR. +Single PR (hotfix-first), with contingency split only if backend root cause is confirmed late. -### Trigger Reasons -1. Scope is narrow and cross-cutting risk is low. -2. Verification logic and docs sync are tightly coupled. -3. Review size remains small and rollback is straightforward. +### Rationale +1. Incident impact is immediate user-facing and concentrated in one feature path. +2. Frontend + targeted backend/test changes are tightly coupled for verification. +3. Single PR minimizes release coordination and user interruption. -### PR-1 -1. Scope: - - Refresh `docs/plans/current_spec.md` to verification-focused plan. - - Sync `ARCHITECTURE.md` Caddy version metadata. - - Add/adjust verification checklist content needed for gates. -2. Dependencies: - - Existing publish/scanning pipeline availability. -3. Validation gates: - - Gate 1 through Gate 4 all required. +### Contingency (Only if split becomes necessary) +1. PR-1: Frontend binding + tests + - Scope: `ProxyHostForm`, `AccessListSelector`, `ui/Select` (if required), related tests. + - Dependency: none. + - Acceptance: UI submit payload verified correct in unit + Playwright. +2. PR-2: Backend parser/persistence + tests (conditional) + - Scope: `proxy_host_handler.go`, `proxyhost_service.go`, handler/service tests. + - Dependency: PR-1 merged or rebased for aligned contract. + - Acceptance: API update/create persist both nullable IDs correctly. +3. PR-3: Regression hardening + docs + - Scope: extra regression coverage, release-note hotfix entry. + - Dependency: PR-1/PR-2. + - Acceptance: full DoD validation sequence passes. -## 9. Rollback and Contingency +## 11. Validation Plan (Mandatory Sequence) -1. If verification updates are incorrect or incomplete, revert PR-1. -2. If rollout evidence fails, hold release sign-off and keep last known-good - digest as active reference. -3. Re-run verification with corrected commands/artifacts before reattempting - sign-off. +0. E2E environment prerequisite + - Determine rebuild necessity per testing policy: if application/runtime or Docker input changes are present, rebuild is required. + - If rebuild is required or the container is unhealthy, run `.github/skills/scripts/skill-runner.sh docker-rebuild-e2e`. + - Record container health outcome before executing tests. +1. Playwright first + - Run targeted Proxy Host dropdown and create/edit persistence scenarios. +2. Local patch coverage preflight + - Generate `test-results/local-patch-report.md` and `test-results/local-patch-report.json`. +3. Unit and coverage + - Backend coverage run (threshold >= 85%). + - Frontend coverage run (threshold >= 85%). +4. Type checks + - Frontend TypeScript check. +5. Pre-commit + - `pre-commit run --all-files` with zero blocking failures. +6. Security scans + - CodeQL Go + JS (security-and-quality). + - Findings check gate. + - Trivy scan. + - Conditional GORM security scan if model/DB-layer changes are made. +7. Build verification + - Backend build + frontend build pass. + +## 12. File Review: `.gitignore`, `codecov.yml`, `.dockerignore`, `Dockerfile` + +Assessment for this hotfix: +1. `.gitignore`: no required change for ACL/Security Headers hotfix. +2. `codecov.yml`: no required change; current exclusions/thresholds are compatible. +3. `.dockerignore`: no required change unless new hotfix-only artifact paths are introduced. +4. `Dockerfile`: no required change; incident is application logic/UI binding, not image build pipeline. + +If implementation introduces new persistent test artifacts, update ignore files in the same PR. + +## 13. Rollback and Contingency + +1. If hotfix causes regression in proxy host save flow, revert hotfix commit and redeploy prior stable build. +2. If frontend-only fix is insufficient, activate conditional backend phase immediately. +3. If validation gates fail on security/coverage, hold merge until fixed; no partial exception for this incident. +4. Post-rollback smoke checks: + - Create host with ACL/profile. + - Edit to different ACL/profile values. + - Clear both values to `null`. + - Verify persisted values in API response and after UI reload. diff --git a/frontend/src/components/ProxyHostForm.tsx b/frontend/src/components/ProxyHostForm.tsx index e6548f0d..756c6351 100644 --- a/frontend/src/components/ProxyHostForm.tsx +++ b/frontend/src/components/ProxyHostForm.tsx @@ -101,9 +101,12 @@ interface ProxyHostFormProps { onCancel: () => void } -export default function ProxyHostForm({ host, onSubmit, onCancel }: ProxyHostFormProps) { - type ProxyHostFormState = Partial & { addUptime?: boolean; uptimeInterval?: number; uptimeMaxRetries?: number } - const [formData, setFormData] = useState({ +function buildInitialFormData(host?: ProxyHost): Partial & { + addUptime?: boolean + uptimeInterval?: number + uptimeMaxRetries?: number +} { + return { name: host?.name || '', domain_names: host?.domain_names || '', forward_scheme: host?.forward_scheme || 'http', @@ -123,7 +126,42 @@ export default function ProxyHostForm({ host, onSubmit, onCancel }: ProxyHostFor access_list_id: host?.access_list_id, security_header_profile_id: host?.security_header_profile_id, dns_provider_id: host?.dns_provider_id || null, - }) + } +} + +function normalizeNullableID(value: unknown): number | null | undefined { + if (value === undefined) { + return undefined + } + + if (value === null) { + return null + } + + if (typeof value === 'number') { + return Number.isFinite(value) ? value : null + } + + if (typeof value === 'string') { + const trimmed = value.trim() + if (trimmed === '') { + return null + } + + const parsed = Number.parseInt(trimmed, 10) + return Number.isNaN(parsed) ? null : parsed + } + + return null +} + +export default function ProxyHostForm({ host, onSubmit, onCancel }: ProxyHostFormProps) { + type ProxyHostFormState = Partial & { addUptime?: boolean; uptimeInterval?: number; uptimeMaxRetries?: number } + const [formData, setFormData] = useState(buildInitialFormData(host)) + + useEffect(() => { + setFormData(buildInitialFormData(host)) + }, [host?.uuid]) // Charon internal IP for config helpers (previously CPMP internal IP) const [charonInternalIP, setCharonInternalIP] = useState('') @@ -420,6 +458,10 @@ export default function ProxyHostForm({ host, onSubmit, onCancel }: ProxyHostFor // strip temporary uptime-only flags from payload by destructuring const { addUptime: _addUptime, uptimeInterval: _uptimeInterval, uptimeMaxRetries: _uptimeMaxRetries, ...payloadWithoutUptime } = payload as ProxyHostFormState void _addUptime; void _uptimeInterval; void _uptimeMaxRetries; + + payloadWithoutUptime.access_list_id = normalizeNullableID(payloadWithoutUptime.access_list_id) + payloadWithoutUptime.security_header_profile_id = normalizeNullableID(payloadWithoutUptime.security_header_profile_id) + const res = await onSubmit(payloadWithoutUptime) // if user asked to add uptime, request server to sync monitors @@ -824,7 +866,7 @@ export default function ProxyHostForm({ host, onSubmit, onCancel }: ProxyHostFor {/* Access Control List */} setFormData(prev => ({ ...prev, access_list_id: id }))} /> @@ -836,9 +878,9 @@ export default function ProxyHostForm({ host, onSubmit, onCancel }: ProxyHostFor