Proxy Host ACL and Security Headers drop down hotfix#776
Proxy Host ACL and Security Headers drop down hotfix#776Wikid82 merged 16 commits intodevelopmentfrom
Conversation
Wikid82
added
bug
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
✅ Supply Chain Verification Results✅ PASSED 📦 SBOM Summary
🔍 Vulnerability Scan
📎 Artifacts
Generated by Supply Chain Verification workflow • View Details |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…y options and improve token resolution
- Add tests to normalize string numeric ACL IDs in AccessListSelector. - Implement regression tests for ProxyHostForm to ensure numeric ACL values are submitted correctly. - Introduce a recovery function for ACL lockout scenarios in auth setup. - Create new tests for ACL creation and security header profiles to ensure dropdown coverage. - Add regression tests for ACL and Security Headers dropdown behavior in ProxyHostForm. - Establish a security shard setup to validate emergency token configurations and reset security states. - Enhance emergency operations tests to ensure ACL selections persist across create/edit flows.
… to ensure valid numeric IDs
…UUID resolution in AccessListSelector
…port access_list structure
… UUIDs and improve form data normalization
…n-major-updates fix(deps): update module github.com/gin-gonic/gin to v1.12.0 (feature/beta-release)
…onents - Added new test suite for AccessListSelector to cover token normalization and emitted values. - Updated existing tests for AccessListSelector to handle prefixed and numeric-string form values. - Introduced tests for ProxyHostForm to validate DNS detection, including error handling and success scenarios. - Enhanced ProxyHostForm tests to cover token normalization for security headers and ensure proper handling of existing host values. - Implemented additional tests for ProxyHostForm to verify domain updates based on selected containers and prompt for new base domains.
There was a problem hiding this comment.
Pull request overview
This hotfix addresses a bug where ACL and Security Headers dropdown selections in the Proxy Host create/edit form were not being persisted after save or surviving a modal reopen. The root cause was in the frontend select binding (value coercion between string/number/null) and the backend's lack of UUID-to-ID resolution for these fields. The fix introduces a token-based selection system (id:N/uuid:X/none) consistently across the form components, adds UUID resolution on the backend for both fields, preloads AccessList associations, and adds comprehensive tests.
Changes:
- Frontend: Token-based select value system in
AccessListSelector.tsxandProxyHostForm.tsxto correctly handlenumber | string | nullform values across create/edit flows; z-index and pointer-events fix inSelect.tsx - Backend: UUID-to-ID resolution in
proxy_host_handler.gofor bothaccess_list_idandsecurity_header_profile_id;AccessListpreload added toproxyhost_service.go; minor dependency upgrades ingo.mod/go.sum - Tests: Comprehensive frontend unit, backend handler, and Playwright E2E tests for ACL/security headers dropdown regression; restructured global/security-shard test setup
Reviewed changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/src/components/ui/Select.tsx |
Raises z-index to z-[70] and adds pointer-events-auto to fix dropdown click blocking |
frontend/src/components/AccessListSelector.tsx |
Full rewrite to token-based value handling supporting `number |
frontend/src/components/ProxyHostForm.tsx |
Extracts buildInitialFormData, adds normalization/token helpers, fixes security headers select binding |
frontend/src/api/proxyHosts.ts |
Widens access_list_id and security_header_profile_id to `number |
backend/internal/api/handlers/proxy_host_handler.go |
Adds resolveAccessListReference and resolveSecurityHeaderProfileReference with UUID-to-ID lookup |
backend/internal/services/proxyhost_service.go |
Adds AccessList preload in GetByUUID and List |
backend/go.mod / backend/go.sum |
Upgrades gin, sonic, go-yaml, ugorji codec; adds MongoDB driver as indirect dep |
tests/global-setup.ts |
Removes emergency token validation and security reset from global setup |
tests/security-shard.setup.ts |
New file: dedicated security shard setup with emergency reset and verification |
tests/auth.setup.ts |
Adds recoverFromAclLockout helper for 403 recovery during auth setup |
playwright.config.js |
Adds security-shard-setup project as dependency for security tests |
tests/security-enforcement/acl-dropdown-regression.spec.ts |
New E2E regression spec for ACL/security headers persistence |
tests/security-enforcement/acl-creation.spec.ts |
New E2E spec for ACL and security header profile baseline creation |
tests/security/emergency-operations.spec.ts |
Adds helper functions and new ACL dropdown parity regression test |
tests/proxy-host-dropdown-fix.spec.ts |
Moves old tests under .skip, rewrites with new helpers |
frontend/src/components/__tests__/ProxyHostForm.test.tsx |
New unit tests for presets, security headers token resolution, domain handling |
frontend/src/components/__tests__/ProxyHostForm-dropdown-changes.test.tsx |
New unit tests for null transitions, host switching, UUID-only payloads |
frontend/src/components/__tests__/ProxyHostForm-token-coverage.test.tsx |
New file: focused token normalization branch coverage |
frontend/src/components/__tests__/ProxyHostForm-uptime.test.tsx |
New test for sync error fallback toast |
frontend/src/components/__tests__/ProxyHostForm-dns.test.tsx |
New DNS detection branch tests |
frontend/src/components/__tests__/AccessListSelector.test.tsx |
Extensive new tests for token value handling |
frontend/src/components/__tests__/AccessListSelector-token-coverage.test.tsx |
New file: token branch coverage with Select mock |
backend/internal/api/handlers/proxy_host_handler_test.go |
New tests for resolveAccessListReference, resolveSecurityHeaderProfileReference, and create reference resolution |
backend/internal/api/handlers/proxy_host_handler_update_test.go |
New tests for ACL and security header ID transition scenarios |
docs/plans/current_spec.md |
Replaced with ACL+Security Headers hotfix specification |
docs/issues/manual_test_acl_security_headers_dropdown_hotfix.md |
New manual test plan |
CHANGELOG.md |
Adds hotfix entry |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
| if err == gorm.ErrRecordNotFound { | ||
| return nil, fmt.Errorf("access list not found") | ||
| } | ||
| return nil, fmt.Errorf("failed to resolve access list") | ||
| } | ||
|
|
||
| id := acl.ID | ||
| return &id, nil | ||
| } | ||
|
|
||
| func (h *ProxyHostHandler) resolveSecurityHeaderProfileReference(value any) (*uint, error) { | ||
| if value == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| parsedID, _, parseErr := parseNullableUintField(value, "security_header_profile_id") | ||
| if parseErr == nil { | ||
| return parsedID, nil | ||
| } | ||
|
|
||
| uuidValue, isString := value.(string) | ||
| if !isString { | ||
| return nil, parseErr | ||
| } | ||
|
|
||
| trimmed := strings.TrimSpace(uuidValue) | ||
| if trimmed == "" { | ||
| return nil, nil | ||
| } | ||
|
|
||
| if _, err := uuid.Parse(trimmed); err != nil { | ||
| return nil, parseErr | ||
| } | ||
|
|
||
| var profile models.SecurityHeaderProfile | ||
| if err := h.db.Select("id").Where("uuid = ?", trimmed).First(&profile).Error; err != nil { | ||
| if err == gorm.ErrRecordNotFound { |
There was a problem hiding this comment.
The new resolveAccessListReference and resolveSecurityHeaderProfileReference functions use err == gorm.ErrRecordNotFound for error comparison, but the established codebase convention is to use errors.Is(err, gorm.ErrRecordNotFound) (as seen consistently in security_handler.go and settings_handler.go). Using direct equality (==) instead of errors.Is can fail to match wrapped errors, which means the "not found" branch may never be taken and the function would return a generic "failed to resolve" error instead of the more specific "not found" message.
| function normalizeSecurityHeaderReference(value: unknown): number | string | null | undefined { | ||
| const numericValue = normalizeNullableID(value) | ||
| if (numericValue !== undefined) { | ||
| return numericValue | ||
| } | ||
|
|
||
| if (typeof value !== 'string') { | ||
| return undefined | ||
| } | ||
|
|
||
| const trimmed = value.trim() | ||
| return trimmed === '' ? null : trimmed | ||
| } | ||
|
|
There was a problem hiding this comment.
The two functions normalizeAccessListReference and normalizeSecurityHeaderReference in ProxyHostForm.tsx have completely identical implementations. This is unnecessary code duplication — one shared utility function would suffice and reduce maintenance burden. If the intention is to allow divergence in the future, at minimum the functions are currently indistinguishable and should be consolidated.
| function normalizeSecurityHeaderReference(value: unknown): number | string | null | undefined { | |
| const numericValue = normalizeNullableID(value) | |
| if (numericValue !== undefined) { | |
| return numericValue | |
| } | |
| if (typeof value !== 'string') { | |
| return undefined | |
| } | |
| const trimmed = value.trim() | |
| return trimmed === '' ? null : trimmed | |
| } | |
| const normalizeSecurityHeaderReference = normalizeAccessListReference |
| github.com/twitchyliquid64/golang-asm v0.15.1 // indirect | ||
| github.com/ugorji/go/codec v1.3.0 // indirect | ||
| github.com/ugorji/go/codec v1.3.1 // indirect | ||
| go.mongodb.org/mongo-driver/v2 v2.5.0 // indirect |
There was a problem hiding this comment.
A new transitive dependency go.mongodb.org/mongo-driver/v2 v2.5.0 appears in go.mod as an indirect dependency. MongoDB driver is unexpected for what appears to be a SQLite/GORM-based backend. This likely came in transitively via the gin v1.12.0 upgrade. The presence of a large, unneeded transitive dependency should be verified — if it's not actually needed, go mod tidy should be run to confirm its necessity or remove it. This significantly increases binary size and attack surface.
| go.mongodb.org/mongo-driver/v2 v2.5.0 // indirect |
| trimmed := strings.TrimSpace(uuidValue) | ||
| if trimmed == "" { | ||
| return nil, nil | ||
| } | ||
|
|
||
| var acl models.AccessList | ||
| if err := h.db.Select("id").Where("uuid = ?", trimmed).First(&acl).Error; err != nil { | ||
| if err == gorm.ErrRecordNotFound { | ||
| return nil, fmt.Errorf("access list not found") | ||
| } | ||
| return nil, fmt.Errorf("failed to resolve access list") | ||
| } | ||
|
|
||
| id := acl.ID | ||
| return &id, nil |
There was a problem hiding this comment.
The resolveAccessListReference function does not validate UUID format before attempting the database query (unlike resolveSecurityHeaderProfileReference which validates with uuid.Parse). This means any arbitrary non-numeric string (e.g. a malformed UUID or an SQL injection attempt) will be passed directly to the WHERE clause. While GORM uses parameterized queries preventing SQL injection, the asymmetry between the two resolve functions is a design inconsistency. If the intent is to accept only valid UUIDs as string references, UUID validation should be added here as well.
| access_list_id: host?.access_list?.uuid ?? host?.access_list_id, | ||
| security_header_profile_id: host?.security_header_profile?.uuid ?? host?.security_header_profile_id, |
There was a problem hiding this comment.
The buildInitialFormData function uses host?.access_list?.uuid ?? host?.access_list_id to initialize the form. This means that when editing a host that has a non-null access_list_id but whose preloaded access_list object is absent (e.g., due to an ACL being deleted), the form will fall back to the raw access_list_id (which may be a numeric ID). However, the resolveSelectToken function later converts numeric IDs to id:N tokens, so the SelectItem values will not match unless the selector also builds id:N tokens. This should work correctly based on the current code, but it's a subtle dependency worth noting in code comments for future maintainers.
|
|
||
| useEffect(() => { | ||
| setFormData(buildInitialFormData(host)) | ||
| }, [host?.uuid]) |
There was a problem hiding this comment.
The useEffect that resets form data on host?.uuid change (line 260-262) could cause problems for newly created hosts that don't yet have a UUID. In create mode (host is undefined), host?.uuid is undefined, so the effect will only run on mount. However, if a host is being created and somehow the parent component passes a partial host object initially without a UUID and then adds one, the form state won't reset. More critically, if the same form component is reused for two different create operations in sequence where neither has a UUID, the form state won't reset. Depending on how the component is used, this edge case may or may not be an issue, but it warrants review.
| useEffect(() => { | |
| setFormData(buildInitialFormData(host)) | |
| }, [host?.uuid]) | |
| const hostToken = host ? getEntityToken(host) : null | |
| useEffect(() => { | |
| setFormData(buildInitialFormData(host)) | |
| }, [hostToken]) |
| const statusContext = await playwrightRequest.newContext({ | ||
| baseURL, | ||
| extraHTTPHeaders: { | ||
| 'X-Emergency-Token': emergencyToken, | ||
| }, | ||
| }); | ||
|
|
||
| try { | ||
| const response = await statusContext.get('/api/v1/security/status', { timeout: 5000 }); | ||
| expect(response.ok()).toBeTruthy(); | ||
|
|
||
| const status = await response.json(); | ||
| expect(status.acl?.enabled).toBeFalsy(); | ||
| expect(status.waf?.enabled).toBeFalsy(); | ||
| expect(status.rate_limit?.enabled).toBeFalsy(); | ||
| } finally { | ||
| await statusContext.dispose(); | ||
| } |
There was a problem hiding this comment.
In security-shard.setup.ts, the verifySecurityDisabled function passes the raw emergencyToken in extraHTTPHeaders for the status request. This exposes the emergency token in request headers for a non-emergency endpoint (/api/v1/security/status). While this appears to be a test setup file, tokens should not be sent to endpoints that don't require them. The status check should either use proper authentication (credentials like the username/password used for the reset) or omit the emergency token header.
No description provided.