48 lines
2.9 KiB
Markdown
48 lines
2.9 KiB
Markdown
# 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.
|