fix: authentication issues for certificate endpoints and improve test coverage

- Updated UsersPage tests to check for specific URL formats instead of regex patterns. - Increased timeout for Go coverage report generation to handle larger repositories. - Cleaned up generated artifacts before running CodeQL analysis to reduce false positives. - Removed outdated QA testing report for authentication fixes on the certificates page. - Added final report confirming successful resolution of authentication issues with certificate endpoints. - Deleted previous test output files to maintain a clean test results directory.
2026-01-03 03:08:43 +00:00
parent 8f15fdd97f
commit 3aaa059a15
41 changed files with 3019 additions and 2298 deletions
--- a/docs/plans/patch-coverage-codecov.md
+++ b/docs/plans/patch-coverage-codecov.md
@@ -0,0 +1,385 @@
+# Patch Coverage Only (Codecov)
+
+This plan is strictly about adding deterministic tests (and only micro-hooks if absolutely required) to push patch coverage ≥85%.
+
+## What’s failing
+
+Codecov **patch** coverage is failing at **82.64%**. The failure is specifically driven by missing coverage on newly-changed lines across **10 flagged backend files** (listed below).
+
+### 10 flagged backend files (scope boundary)
+
+1. `backend/internal/caddy/config.go`
+2. `backend/internal/services/dns_provider_service.go`
+3. `backend/internal/caddy/manager.go`
+4. `backend/internal/utils/url_testing.go`
+5. `backend/internal/network/safeclient.go`
+6. `backend/internal/api/routes/routes.go`
+7. `backend/internal/services/notification_service.go`
+8. `backend/internal/crypto/encryption.go`
+9. `backend/internal/api/handlers/settings_handler.go`
+10. `backend/internal/security/url_validator.go`
+
+----
+
+## Phase 0 — Extract missing patch line ranges (required, exact steps)
+
+Goal: for each of the 10 files, record the **exact line ranges** Codecov considers “missing” on the patch, then map each range to a specific branch and a specific new test.
+
+### A) From GitHub PR Codecov view (fastest, authoritative)
+
+1. Open the PR in GitHub.
+2. Find the Codecov status check / comment, then open the **Codecov “Files changed”** (or “View report”) link.
+3. In Codecov, switch to **Patch** (not Project).
+4. Filter to the 10 backend files listed above.
+5. For each file:
+   - Click the file.
+   - Identify **red** (uncovered) and **yellow** (partial) lines.
+   - Note the **exact line numbers/ranges** as shown in Codecov.
+6. Paste those ranges into the table template in “Phase 0 Output”.
+
+### B) From local `go tool cover -html` (best for understanding branches)
+
+1. Run the VS Code task `shell: Test: Backend with Coverage`.
+2. Generate an HTML view:
+   - `cd backend`
+   - `go tool cover -html=coverage.txt -o ../test-results/backend_cover.html`
+3. Open `test-results/backend_cover.html` in a browser.
+4. For each of the 10 files:
+   - Use in-page search for the filename (or navigate via the coverage page UI).
+   - Identify red/uncovered spans and record the **exact line ranges**.
+5. Cross-check that those spans overlap with the PR diff (next section) so you’re targeting **patch lines**, not legacy debt.
+
+### C) From `git diff main...HEAD` (ground truth for “patch” lines)
+
+1. List hunks for a file:
+   - `git diff -U0 main...HEAD -- backend/internal/caddy/config.go`
+2. For each hunk header like:
+   - `@@ -OLDSTART,OLDCOUNT +NEWSTART,NEWCOUNT @@`
+3. Convert it into a line range on the **new** file:
+   - If `NEWCOUNT` is 0, that hunk adds no new lines.
+   - Otherwise the patch line range is:
+     - `NEWSTART` through `NEWSTART + NEWCOUNT - 1`
+4. Repeat for each of the 10 files.
+
+### Phase 0 Output: table to paste missing line ranges into
+
+Paste and fill this as you extract line ranges (Codecov is the source of truth):
+
+| File | Missing patch line ranges (Codecov) | Partial patch line ranges (Codecov) | Notes (branch / error path) |
+|------|-------------------------------------|-------------------------------------|-----------------------------|
+| `backend/internal/caddy/config.go` | TBD | TBD | |
+| `backend/internal/services/dns_provider_service.go` | TBD | TBD | |
+| `backend/internal/caddy/manager.go` | TBD | TBD | |
+| `backend/internal/utils/url_testing.go` | TBD | TBD | |
+| `backend/internal/network/safeclient.go` | TBD | TBD | |
+| `backend/internal/api/routes/routes.go` | TBD | TBD | |
+| `backend/internal/services/notification_service.go` | TBD | TBD | |
+| `backend/internal/crypto/encryption.go` | TBD | TBD | |
+| `backend/internal/api/handlers/settings_handler.go` | TBD | TBD | |
+| `backend/internal/security/url_validator.go` | TBD | TBD | |
+
+----
+
+## File-by-file plan (tests only, implementation-ready)
+
+### 1) `backend/internal/caddy/config.go`
+
+**Targeted branches (functions/paths in this file)**
+
+- `GenerateConfig`:
+  - wildcard vs non-wildcard split (`hasWildcard(cleanDomains)`)
+  - DNS-provider policy creation vs HTTP-challenge policy creation
+  - `sslProvider` switch: `letsencrypt` vs `zerossl` vs default/both
+  - `acmeStaging` CA override branch
+  - “DNS provider config missing → skip policy” (`dnsProviderMap` lookup miss)
+  - IP filtering for ACME issuers (`net.ParseIP` branch)
+- `getCrowdSecAPIKey`: env-var priority / fallback branches
+- `hasWildcard`: true/false detection
+
+**Tests to add (exact paths + exact test function names)**
+
+- Add file: `backend/internal/caddy/config_patch_coverage_test.go`
+  - `TestGenerateConfig_DNSChallenge_LetsEncrypt_StagingCAAndPropagationTimeout`
+  - `TestGenerateConfig_DNSChallenge_ZeroSSL_IssuerShape`
+  - `TestGenerateConfig_DNSChallenge_SkipsPolicyWhenProviderConfigMissing`
+  - `TestGenerateConfig_HTTPChallenge_ExcludesIPDomains`
+  - `TestGetCrowdSecAPIKey_EnvPriority`
+  - `TestHasWildcard_TrueFalse`
+
+**Determinism**
+
+- Use pure in-memory inputs; no Docker/Caddy process calls.
+- Avoid comparing raw JSON strings; unmarshal and assert keys/values.
+- When asserting lists (subjects), use set semantics or sort before compare.
+
+### 2) `backend/internal/services/dns_provider_service.go`
+
+**Targeted branches (functions/paths in this file)**
+
+- `(*dnsProviderService).Create`:
+  - invalid provider type
+  - credential validation error
+  - defaults (`PropagationTimeout == 0`, `PollingInterval == 0`)
+  - `IsDefault` true → unset previous defaults branch
+- `(*dnsProviderService).Update`:
+  - credentials nil/empty vs non-empty
+  - `IsDefault` toggle branches
+- `(*dnsProviderService).Delete`: `RowsAffected == 0` not found
+- `(*dnsProviderService).Test`: decryption failure returns a structured `TestResult` (no error)
+- `(*dnsProviderService).GetDecryptedCredentials`: decrypt error vs invalid JSON error
+
+**Tests to add (exact paths + exact test function names)**
+
+- Update file: `backend/internal/services/dns_provider_service_test.go`
+  - `TestDNSProviderServiceCreate_DefaultsAndUnsetsExistingDefault`
+  - `TestDNSProviderServiceUpdate_DoesNotOverwriteCredentialsWhenEmpty`
+  - `TestDNSProviderServiceDelete_NotFound`
+  - `TestDNSProviderServiceTest_DecryptionError_ReturnsResultNotErr`
+  - `TestDNSProviderServiceGetDecryptedCredentials_InvalidJSON_ReturnsErrDecryptionFailed`
+
+**Determinism**
+
+- Use in-memory sqlite via existing test helpers.
+- Avoid map ordering assumptions in comparisons.
+
+### 3) `backend/internal/caddy/manager.go`
+
+**Targeted branches (functions/paths in this file)**
+
+- `(*Manager).ApplyConfig`:
+  - DNS provider load present vs empty (`len(dnsProviders) > 0`)
+  - encryption key discovery:
+    - `CHARON_ENCRYPTION_KEY` set
+    - fallback keys (`ENCRYPTION_KEY`, `CERBERUS_ENCRYPTION_KEY`)
+    - no key → warning branch
+  - decrypt/parse branches per provider:
+    - `CredentialsEncrypted == ""` skip
+    - decrypt error skip
+    - JSON unmarshal error skip
+
+**Tests to add (exact paths + exact test function names)**
+
+- Add file: `backend/internal/caddy/manager_patch_coverage_test.go`
+  - `TestManagerApplyConfig_DNSProviders_NoKey_SkipsDecryption`
+  - `TestManagerApplyConfig_DNSProviders_UsesFallbackEnvKeys`
+  - `TestManagerApplyConfig_DNSProviders_SkipsDecryptOrJSONFailures`
+
+**Determinism**
+
+- Do not hit real filesystem/Caddy Admin API; override `generateConfigFunc` / `validateConfigFunc` to capture inputs and short-circuit.
+- Do not use `t.Parallel()` unless you fully isolate and restore package-level hooks.
+- Use `t.Setenv` for env changes.
+
+### 4) `backend/internal/utils/url_testing.go`
+
+**Targeted branches (functions/paths in this file)**
+
+- `TestURLConnectivity`:
+  - scheme validation (`http/https` only)
+  - production path (no transport): validation error mapping
+  - test path (custom transport): “skip DNS/IP validation” branch
+  - redirect handling: `validateRedirectTarget` (localhost allowlist vs validation failure)
+- `ssrfSafeDialer`: address parse error, DNS resolution error, “any private IP blocks” branch
+
+**Tests to add (exact paths + exact test function names)**
+
+- Update file: `backend/internal/utils/url_testing_test.go`
+  - `TestValidateRedirectTarget_AllowsLocalhost`
+  - `TestValidateRedirectTarget_BlocksInvalidExternalRedirect`
+  - `TestURLConnectivity_TestPath_ReconstructsURLAndSkipsDNSValidation`
+
+**Determinism**
+
+- Use `httptest.Server` + injected `http.RoundTripper` to avoid real DNS/network.
+- Avoid timing flakiness: assert “latency > 0” only when a local server is used.
+
+### 5) `backend/internal/network/safeclient.go`
+
+**Targeted branches (functions/paths in this file)**
+
+- `IsPrivateIP`: fast-path checks and CIDR-block scan
+- `safeDialer`:
+  - bad `host:port` parse
+  - localhost allow/deny branch (`AllowLocalhost`)
+  - DNS lookup error / no IPs
+  - “any private IP blocks” validation branch
+- `validateRedirectTarget`: missing hostname, private IP block
+
+**Tests to add (exact paths + exact test function names)**
+
+- Update file: `backend/internal/network/safeclient_test.go`
+  - `TestValidateRedirectTarget_MissingHostname_ReturnsErr`
+  - `TestValidateRedirectTarget_BlocksPrivateIPRedirect`
+  - `TestNewSafeHTTPClient_WithMaxRedirects_EnforcesRedirectLimit`
+
+**Determinism**
+
+- Prefer redirect tests built with `httptest.Server`.
+- Keep tests self-contained by controlling `Location` headers.
+
+### 6) `backend/internal/api/routes/routes.go`
+
+**Targeted branches (functions/paths in this file)**
+
+- `Register`:
+  - DNS providers are conditionally registered only when `cfg.EncryptionKey != ""`.
+  - Branch when `crypto.NewEncryptionService(cfg.EncryptionKey)` fails (routes should remain unavailable).
+  - Branch when encryption service initializes successfully (routes should exist).
+
+**Tests to add (exact paths + exact test function names)**
+
+- Update file: `backend/internal/api/routes/routes_test.go`
+  - `TestRegister_DNSProviders_NotRegisteredWhenEncryptionKeyMissing`
+  - `TestRegister_DNSProviders_NotRegisteredWhenEncryptionKeyInvalid`
+  - `TestRegister_DNSProviders_RegisteredWhenEncryptionKeyValid`
+
+**Determinism**
+
+- Validate route presence by inspecting `router.Routes()` and asserting `"/dns-providers"` paths exist or do not exist.
+- Use a known-good base64 32-byte key literal for “valid” cases.
+
+### 7) `backend/internal/services/notification_service.go`
+
+**Targeted branches (functions/paths in this file)**
+
+- `normalizeURL`: Discord webhook URL normalization vs passthrough
+- `SendExternal`:
+  - event-type preference filtering branches
+  - http/https destination validation using `security.ValidateExternalURL` (skip invalid destinations)
+  - JSON-template path vs shoutrrr path
+- `sendJSONPayload`:
+  - template selection (`minimal`/`detailed`/`custom`/default)
+  - template size limit
+  - URL validation failure branch
+  - template parse/execute error branches
+  - JSON unmarshal error
+  - service-specific validation branches (discord/slack/gotify)
+
+**Tests to add (exact paths + exact test function names)**
+
+- Update file: `backend/internal/services/notification_service_json_test.go`
+  - `TestNormalizeURL_DiscordWebhook_ConvertsToDiscordScheme`
+  - `TestSendExternal_SkipsInvalidHTTPDestination`
+  - `TestSendJSONPayload_InvalidTemplate_ReturnsErr`
+
+**Determinism**
+
+- Avoid real external calls: use `httptest.Server` and validation-only failures.
+- For `SendExternal` (async goroutine), use an `atomic` counter + short `time.After` guard to assert “no send happened”.
+
+### 8) `backend/internal/crypto/encryption.go`
+
+**Targeted branches (functions/paths in this file)**
+
+- `NewEncryptionService`: invalid base64, wrong key length
+- `(*EncryptionService).Encrypt`: nonce generation
+- `(*EncryptionService).Decrypt`: invalid base64, ciphertext too short, auth/tag failure (`gcm.Open`)
+
+**Tests to add (exact paths + exact test function names)**
+
+- Update file: `backend/internal/crypto/encryption_test.go`
+  - `TestDecrypt_InvalidCiphertext` (extend table cases if patch adds new error branches)
+  - `TestDecrypt_TamperedCiphertext` (ensure it hits the new/changed patch lines)
+
+**Determinism**
+
+- Avoid any network/file IO.
+- Do not rely on ciphertext equality; only assert round-trips and error substrings.
+
+### 9) `backend/internal/api/handlers/settings_handler.go`
+
+**Targeted branches (functions/handlers in this file)**
+
+- `ValidatePublicURL`:
+  - admin-only gate (403)
+  - JSON bind error (400)
+  - invalid URL format (400)
+  - success response includes optional warning field
+- `TestPublicURL`:
+  - admin-only gate (403)
+  - JSON bind error (400)
+  - format validation error (400)
+  - SSRF validation failure returns 200 with `reachable=false`
+  - connectivity error returns 200 with `reachable=false`
+  - success path returns 200 with `reachable` and `latency`
+
+**Tests to add (exact paths + exact test function names)**
+
+- Update file: `backend/internal/api/handlers/settings_handler_test.go`
+  - `TestSettingsHandler_ValidatePublicURL_NonAdmin_Returns403`
+  - `TestSettingsHandler_TestPublicURL_BindError_Returns400`
+  - `TestSettingsHandler_TestPublicURL_SSRFValidationFailure_Returns200ReachableFalse`
+
+**Determinism**
+
+- Use gin test mode + httptest recorder; don’t hit real DNS.
+- For `TestPublicURL`, choose inputs that fail at validation (e.g., `http://10.0.0.1`) so the handler exits before any real network.
+
+### 10) `backend/internal/security/url_validator.go`
+
+**Targeted branches (functions/paths in this file)**
+
+- `ValidateExternalURL`:
+  - scheme enforcement (https-only unless `WithAllowHTTP`)
+  - credentials-in-URL rejection
+  - hostname length guard and suspicious pattern rejection
+  - port parsing and privileged-port blocking (non-standard ports <1024)
+  - `WithAllowLocalhost` early-return path for localhost/127.0.0.1/::1
+  - DNS resolution failure path
+  - private IP block path (including IPv4-mapped IPv6 detection)
+
+**Tests to add (exact paths + exact test function names)**
+
+- Update file: `backend/internal/security/url_validator_test.go`
+  - `TestValidateExternalURL_HostnameTooLong_ReturnsErr`
+  - `TestValidateExternalURL_SuspiciousHostnamePattern_ReturnsErr`
+  - `TestValidateExternalURL_PrivilegedPortBlocked_ReturnsErr`
+  - `TestValidateExternalURL_URLWithCredentials_ReturnsErr`
+
+**Determinism**
+
+- Prefer validation-only failures (length/pattern/credentials/port) to avoid reliance on DNS.
+- Avoid asserting exact DNS error strings; assert on stable substrings for validation errors.
+
+----
+
+## Patch Coverage Hit List (consolidated)
+
+Fill the missing ranges from Phase 0, then implement only the tests listed here.
+
+| File | Missing patch line ranges | Test(s) that hit it |
+|------|---------------------------|---------------------|
+| `backend/internal/caddy/config.go` | TBD: fill from Codecov | `TestGenerateConfig_DNSChallenge_LetsEncrypt_StagingCAAndPropagationTimeout`; `TestGenerateConfig_DNSChallenge_ZeroSSL_IssuerShape`; `TestGenerateConfig_DNSChallenge_SkipsPolicyWhenProviderConfigMissing`; `TestGenerateConfig_HTTPChallenge_ExcludesIPDomains`; `TestGetCrowdSecAPIKey_EnvPriority`; `TestHasWildcard_TrueFalse` |
+| `backend/internal/services/dns_provider_service.go` | TBD: fill from Codecov | `TestDNSProviderServiceCreate_DefaultsAndUnsetsExistingDefault`; `TestDNSProviderServiceUpdate_DoesNotOverwriteCredentialsWhenEmpty`; `TestDNSProviderServiceDelete_NotFound`; `TestDNSProviderServiceTest_DecryptionError_ReturnsResultNotErr`; `TestDNSProviderServiceGetDecryptedCredentials_InvalidJSON_ReturnsErrDecryptionFailed` |
+| `backend/internal/caddy/manager.go` | TBD: fill from Codecov | `TestManagerApplyConfig_DNSProviders_NoKey_SkipsDecryption`; `TestManagerApplyConfig_DNSProviders_UsesFallbackEnvKeys`; `TestManagerApplyConfig_DNSProviders_SkipsDecryptOrJSONFailures` |
+| `backend/internal/utils/url_testing.go` | TBD: fill from Codecov | `TestValidateRedirectTarget_AllowsLocalhost`; `TestValidateRedirectTarget_BlocksInvalidExternalRedirect`; `TestURLConnectivity_TestPath_ReconstructsURLAndSkipsDNSValidation` |
+| `backend/internal/network/safeclient.go` | TBD: fill from Codecov | `TestValidateRedirectTarget_MissingHostname_ReturnsErr`; `TestValidateRedirectTarget_BlocksPrivateIPRedirect`; `TestNewSafeHTTPClient_WithMaxRedirects_EnforcesRedirectLimit` |
+| `backend/internal/api/routes/routes.go` | TBD: fill from Codecov | `TestRegister_DNSProviders_NotRegisteredWhenEncryptionKeyMissing`; `TestRegister_DNSProviders_NotRegisteredWhenEncryptionKeyInvalid`; `TestRegister_DNSProviders_RegisteredWhenEncryptionKeyValid` |
+| `backend/internal/services/notification_service.go` | TBD: fill from Codecov | `TestNormalizeURL_DiscordWebhook_ConvertsToDiscordScheme`; `TestSendExternal_SkipsInvalidHTTPDestination`; `TestSendJSONPayload_InvalidTemplate_ReturnsErr` |
+| `backend/internal/crypto/encryption.go` | TBD: fill from Codecov | `TestDecrypt_InvalidCiphertext`; `TestDecrypt_TamperedCiphertext` |
+| `backend/internal/api/handlers/settings_handler.go` | TBD: fill from Codecov | `TestSettingsHandler_ValidatePublicURL_NonAdmin_Returns403`; `TestSettingsHandler_TestPublicURL_BindError_Returns400`; `TestSettingsHandler_TestPublicURL_SSRFValidationFailure_Returns200ReachableFalse` |
+| `backend/internal/security/url_validator.go` | TBD: fill from Codecov | `TestValidateExternalURL_HostnameTooLong_ReturnsErr`; `TestValidateExternalURL_SuspiciousHostnamePattern_ReturnsErr`; `TestValidateExternalURL_PrivilegedPortBlocked_ReturnsErr`; `TestValidateExternalURL_URLWithCredentials_ReturnsErr` |
+
+----
+
+## Minimal refactors (only if required)
+
+Do not refactor unless Phase 0 shows patch-missing lines are in branches that are effectively unreachable from tests.
+
+- `backend/internal/services/dns_provider_service.go`: if `json.Marshal` failure branches in `Create`/`Update` are patch-missing and cannot be triggered via normal inputs, add a micro-hook (package-level `var marshalJSON = json.Marshal`) and restore it in tests.
+- `backend/internal/caddy/manager.go`: if any error branches depend on OS/JSON behaviors, use the existing package-level hooks (e.g., `readFileFunc`, `writeFileFunc`, `jsonMarshalFunc`, `generateConfigFunc`) and **always restore** them with `defer`.
+
+Notes:
+
+- Avoid `t.Parallel()` in any test that touches package-level vars/hooks unless you fully isolate state.
+- Any global override must be restored, even on failure paths.
+
+----
+
+## Validation checklist (run after tests are implemented)
+
+- `shell: Test: Backend with Coverage`
+- `shell: Lint: Pre-commit (All Files)`
+- `shell: Security: CodeQL Go Scan (CI-Aligned) [~60s]`
+- `shell: Security: CodeQL JS Scan (CI-Aligned) [~90s]`
+- `shell: Security: Trivy Scan`