diff --git a/backend/internal/api/handlers/certificate_handler.go b/backend/internal/api/handlers/certificate_handler.go index b9d8e7f3..dffbcb52 100644 --- a/backend/internal/api/handlers/certificate_handler.go +++ b/backend/internal/api/handlers/certificate_handler.go @@ -165,6 +165,15 @@ func (h *CertificateHandler) Upload(c *gin.Context) { chainPEM = string(chainBytes) } + // Require key_file for non-PFX formats (PFX embeds the private key) + if keyPEM == "" { + format := services.DetectFormat(certBytes) + if format != services.FormatPFX { + c.JSON(http.StatusBadRequest, gin.H{"error": "key_file is required for PEM/DER certificate uploads"}) + return + } + } + cert, err := h.service.UploadCertificate(name, certPEM, keyPEM, chainPEM) if err != nil { logger.Log().WithError(err).Error("failed to upload certificate") diff --git a/backend/internal/api/handlers/certificate_handler_test.go b/backend/internal/api/handlers/certificate_handler_test.go index fc45f0ef..261d50b5 100644 --- a/backend/internal/api/handlers/certificate_handler_test.go +++ b/backend/internal/api/handlers/certificate_handler_test.go @@ -408,6 +408,11 @@ func TestCertificateHandler_Upload_MissingKeyFile_MultipartWithCert(t *testing.T h := NewCertificateHandler(svc, nil, nil) r.POST("/api/certificates", h.Upload) + certPEM, _, genErr := generateSelfSignedCertPEM() + if genErr != nil { + t.Fatalf("failed to generate self-signed cert: %v", genErr) + } + var body bytes.Buffer writer := multipart.NewWriter(&body) _ = writer.WriteField("name", "testcert") @@ -415,7 +420,7 @@ func TestCertificateHandler_Upload_MissingKeyFile_MultipartWithCert(t *testing.T if createErr != nil { t.Fatalf("failed to create form file: %v", createErr) } - _, _ = part.Write([]byte("-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----")) + _, _ = part.Write([]byte(certPEM)) _ = writer.Close() req := httptest.NewRequest(http.MethodPost, "/api/certificates", &body) @@ -426,7 +431,7 @@ func TestCertificateHandler_Upload_MissingKeyFile_MultipartWithCert(t *testing.T if w.Code != http.StatusBadRequest { t.Fatalf("expected 400 Bad Request, got %d, body=%s", w.Code, w.Body.String()) } - if !strings.Contains(w.Body.String(), "key_file") { + if !strings.Contains(w.Body.String(), "key_file is required") { t.Fatalf("expected error message about key_file, got: %s", w.Body.String()) } } diff --git a/backend/internal/services/certificate_service_coverage_test.go b/backend/internal/services/certificate_service_coverage_test.go index 86d3547e..3a96909e 100644 --- a/backend/internal/services/certificate_service_coverage_test.go +++ b/backend/internal/services/certificate_service_coverage_test.go @@ -495,10 +495,6 @@ func TestCertificateService_MigratePrivateKeys(t *testing.T) { // MigratePrivateKeys uses raw SQL referencing private_key column (gorm:"-" tag) require.NoError(t, db.Exec("ALTER TABLE ssl_certificates ADD COLUMN private_key TEXT DEFAULT ''").Error) - // Insert cert with plaintext key using raw SQL{})) - // MigratePrivateKeys uses raw SQL referencing private_key column (gorm:"-" tag) - require.NoError(t, db.Exec("ALTER TABLE ssl_certificates ADD COLUMN private_key TEXT DEFAULT ''").Error) - // Insert cert with plaintext key using raw SQL require.NoError(t, db.Exec( "INSERT INTO ssl_certificates (uuid, name, provider, domains, private_key) VALUES (?, ?, ?, ?, ?)", diff --git a/docs/plans/current_spec.md b/docs/plans/current_spec.md index 19b51633..9735a431 100644 --- a/docs/plans/current_spec.md +++ b/docs/plans/current_spec.md @@ -1252,3 +1252,124 @@ test('should reject empty friendly name', async ({ page }) => { | Other tests depend on `getAttribute('required')` pattern | Low | Grep for `getAttribute('required')` across all test files | | Disabled-button check is too permissive | Low | Tests still verify the submit guard logic is correct | | Key file optionality confuses users | Medium | UX already shows key file section only for non-PFX; label doesn't say "required" | + +--- + +## Appendix A: CI Test Failure Fix Plan (PR #928) + +**Date**: 2026-04-13 +**Branch**: `feature/beta-release` +**Trigger**: Two backend test failures blocking CI on PR #928. + +--- + +### Failure 1: `TestCertificateHandler_Upload_MissingKeyFile_MultipartWithCert` + +**File**: `backend/internal/api/handlers/certificate_handler_test.go` line 396 +**Symptom**: Test expects a 400 response mentioning `key_file`, but receives `{"error":"failed to parse certificate input: failed to parse certificate PEM: failed to parse certificate: x509: malformed certificate"}`. + +#### Root Cause + +Two compounding issues: + +1. **Malformed test fixture**: The test sends a dummy cert (`-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----`) that is not a valid X.509 certificate. The `UploadCertificate` service method immediately calls `ParseCertificateInput()` → `parsePEMInput()` → `parsePEMCertificates()`, which fails on the malformed data before any key presence check runs. + +2. **Missing key_file validation**: The handler (line 127) treats `key_file` as optional and passes `keyPEM=""` to the service without complaint. The service's `UploadCertificate()` (line 434) calls `ParseCertificateInput()` which also treats a missing key as acceptable (PFX embeds keys). **There is no validation anywhere that requires `key_file` for PEM/DER uploads.** Even with a valid cert, this test would fail because the upload would succeed (201) instead of returning 400. + +#### Required Changes + +| # | File | Location | Change | +|---|------|----------|--------| +| 1a | `backend/internal/api/handlers/certificate_handler.go` | After line ~152 (after chain file read, before `h.service.UploadCertificate` call) | Add key_file validation for non-PFX formats. Use `services.DetectFormat(certBytes)` to detect format. If format is `FormatPEM` or `FormatDER`, require `keyPEM != ""`. Return `400 Bad Request` with `{"error":"key_file is required for PEM/DER certificate uploads"}`. | +| 1b | `backend/internal/api/handlers/certificate_handler_test.go` | Line ~420 (the `part.Write` call) | Replace the malformed dummy cert with a valid self-signed certificate using the existing `generateSelfSignedCertPEM()` helper (already defined at line ~478 in the same file). Use only the cert portion; do not include the key file. | +| 1c | `backend/internal/api/handlers/certificate_handler_test.go` | Line ~432 (the `key_file` assertion) | Update the expected error substring from `"key_file"` to `"key_file is required"` to match the new handler message. | + +**Handler change detail** (insert after chain file block, before `h.service.UploadCertificate`): + +```go +// Require key_file for PEM/DER formats (PFX embeds the key) +if keyPEM == "" { + format := services.DetectFormat(certBytes) + if format == services.FormatPEM || format == services.FormatDER { + c.JSON(http.StatusBadRequest, gin.H{"error": "key_file is required for PEM/DER certificate uploads"}) + return + } +} +``` + +**Test change detail** (replace dummy cert with valid cert): + +```go +certPEM, _, err := generateSelfSignedCertPEM() +if err != nil { + t.Fatalf("failed to generate cert: %v", err) +} +part, createErr := writer.CreateFormFile("certificate_file", "cert.pem") +if createErr != nil { + t.Fatalf("failed to create form file: %v", createErr) +} +_, _ = part.Write([]byte(certPEM)) +``` + +--- + +### Failure 2: `TestCertificateService_MigratePrivateKeys/migrates_plaintext_key` + +**File**: `backend/internal/services/certificate_service_coverage_test.go` line ~500 +**Symptom**: `duplicate column name: private_key` when executing `ALTER TABLE ssl_certificates ADD COLUMN private_key TEXT DEFAULT ''`. + +#### Root Cause + +Copy-paste / merge artifact. The "migrates plaintext key" subtest contains a **duplicate** `ALTER TABLE ADD COLUMN` block. The code at lines 496–497 executes the first ALTER TABLE (succeeds), then lines 499–501 repeat it verbatim (fails with "duplicate column name"). Line 499 also contains a corrupted comment: `// Insert cert with plaintext key using raw SQL{}))` — the `{}))` is leftover garbage from a bad merge. + +**Current code (lines 496–504):** +```go +require.NoError(t, db.Exec("ALTER TABLE ssl_certificates ADD COLUMN private_key TEXT DEFAULT ''").Error) + +// Insert cert with plaintext key using raw SQL{})) +// MigratePrivateKeys uses raw SQL referencing private_key column (gorm:"-" tag) +require.NoError(t, db.Exec("ALTER TABLE ssl_certificates ADD COLUMN private_key TEXT DEFAULT ''").Error) + +// Insert cert with plaintext key using raw SQL +``` + +The `PrivateKey` field in the model has `gorm:"-"`, so `AutoMigrate` does not create the `private_key` column, meaning the first ALTER TABLE is required. The second is spurious. + +#### Required Changes + +| # | File | Location | Change | +|---|------|----------|--------| +| 2a | `backend/internal/services/certificate_service_coverage_test.go` | Lines ~499–501 | Delete the three duplicate/corrupted lines (the corrupted comment, the duplicate `gorm:"-"` comment, and the duplicate ALTER TABLE statement). | + +**After fix, the block should read:** +```go +require.NoError(t, db.Exec("ALTER TABLE ssl_certificates ADD COLUMN private_key TEXT DEFAULT ''").Error) + +// Insert cert with plaintext key using raw SQL +require.NoError(t, db.Exec( +``` + +--- + +### Commit Slicing Strategy + +**Decision**: Single PR, single commit. Both fixes are small, scoped to test infrastructure + one handler validation guard, and have no cross-domain risk. + +#### Commit 1: `fix(tests): resolve CI failures in certificate upload handler and migration tests` + +| Aspect | Detail | +|--------|--------| +| **Scope** | 3 files — 1 handler source, 2 test files | +| **Files** | `backend/internal/api/handlers/certificate_handler.go`, `backend/internal/api/handlers/certificate_handler_test.go`, `backend/internal/services/certificate_service_coverage_test.go` | +| **Dependencies** | None — changes are self-contained | +| **Validation gate** | `go test ./backend/internal/api/handlers/ -run TestCertificateHandler_Upload_MissingKeyFile_MultipartWithCert -v` and `go test ./backend/internal/services/ -run TestCertificateService_MigratePrivateKeys -v` must both pass | +| **Regression check** | `go test ./backend/internal/api/handlers/ -run TestCertificateHandler_Upload` (all Upload tests) and `go test ./backend/internal/services/ -run TestCertificateService` (all service tests) | +| **Rollback** | Standard `git revert` — no schema or migration changes | + +### Risk Assessment + +| Risk | Likelihood | Impact | Mitigation | +|------|-----------|--------|------------| +| key_file validation breaks PFX upload flow | Low | The guard explicitly excludes `FormatPFX` via `DetectFormat()` | +| `generateSelfSignedCertPEM()` helper unavailable in test scope | None | Helper is already defined in the same test file (line ~478) | +| Other tests rely on key_file being optional | Low | Grep for handler Upload tests to confirm no other test sends cert-only | diff --git a/docs/reports/qa_report_pr928.md b/docs/reports/qa_report_pr928.md new file mode 100644 index 00000000..86eb87c1 --- /dev/null +++ b/docs/reports/qa_report_pr928.md @@ -0,0 +1,47 @@ +# QA Audit Report — PR #928: CI Test Fix + +**Date:** 2026-04-13 +**Scope:** Targeted fix for two CI test failures across three files +**Auditor:** QA Security Agent + +## Modified Files + +| File | Change | +|---|---| +| `backend/internal/api/handlers/certificate_handler.go` | Added `key_file` validation guard for non-PFX uploads | +| `backend/internal/api/handlers/certificate_handler_test.go` | Added `TestCertificateHandler_Upload_MissingKeyFile_MultipartWithCert` test | +| `backend/internal/services/certificate_service_coverage_test.go` | Removed duplicate `ALTER TABLE` in `TestCertificateService_MigratePrivateKeys` | + +## Check Results + +| # | Check | Result | Details | +|---|---|---|---| +| 1 | **Backend Build** (`go build ./...`) | **PASS** | Clean build, no errors | +| 2a | **Targeted Test 1** (`TestCertificateHandler_Upload_MissingKeyFile_MultipartWithCert`) | **PASS** | Previously failing, now passes | +| 2b | **Targeted Test 2** (`TestCertificateService_MigratePrivateKeys`) | **PASS** | Previously failing (duplicate ALTER TABLE), now passes — all 3 subtests pass | +| 3a | **Regression: Handler Suite** (`TestCertificateHandler*`, 37 tests) | **PASS** | All 37 tests pass in 1.27s | +| 3b | **Regression: Service Suite** (`TestCertificateService*`) | **PASS** | All tests pass in 2.96s | +| 4 | **Go Vet** (`go vet ./...`) | **PASS** | No issues | +| 5 | **Golangci-lint** (handlers + services) | **PASS** | All warnings are pre-existing in unmodified files (crowdsec, audit_log). Zero new lint issues in modified files | +| 6 | **Security Review** | **PASS** | See analysis below | + +## Security Analysis + +The handler change (lines 169–175 of `certificate_handler.go`) was reviewed for: + +| Vector | Assessment | +|---|---| +| **Injection** | `services.DetectFormat()` operates on already-read `certBytes` (bounded by `maxFileSize` = 1MB via `io.LimitReader`). No additional I/O or shell invocation. | +| **Information Disclosure** | Returns a static error string `"key_file is required for PEM/DER certificate uploads"`. No user-controlled data reflected in the response. | +| **Auth Bypass** | Route `POST /certificates` is registered inside the `management` group (confirmed at `routes.go:696`), which requires authentication. The guard is additive — it rejects earlier, not later. | +| **DoS** | No new allocations or expensive operations. `DetectFormat` is a simple byte-header check on data already in memory. | + +**Verdict:** No new attack surface introduced. The change is a pure input validation tightening. + +## Warnings + +- **Pre-existing lint warnings** exist in unmodified files (`crowdsec_handler.go`, `crowdsec_*_test.go`, `audit_log_handler_test.go`). These are tracked separately and are not related to this PR. + +## Final Verdict + +**PASS** — All six checks pass. Both previously failing CI tests now succeed. No regressions detected in the broader handler and service suites. No security concerns with the changes.