akanealw/Charon
1
0
Fork 0
Code Issues Pull Requests Actions 16 Packages Projects Releases Wiki Activity

fix: enhance certificate deletion handling with UUID validation and logging improvements

Browse Source
This commit is contained in:
GitHub Actions
2026-04-11 17:54:42 +00:00
parent bb99dacecd
commit 42bc897610
4 changed files with 50 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
backend/internal/api/handlers/certificate_handler.go
View File

@@ -10,6 +10,7 @@ import (
"time"
"github.com/gin-gonic/gin"
"github.com/google/uuid"
"gorm.io/gorm"
"github.com/Wikid82/charon/backend/internal/logger"
@@ -367,7 +368,7 @@ func (h *CertificateHandler) Export(c *gin.Context) {
c.JSON(http.StatusNotFound, gin.H{"error": "certificate not found"})
return
}
logger.Log().WithError(err).Error("failed to export certificate")
logger.Log().WithError(fmt.Errorf("%s", util.SanitizeForLog(err.Error()))).Error("failed to export certificate")
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to export certificate"})
return
}
@@ -423,19 +424,21 @@ func (h *CertificateHandler) Delete(c *gin.Context) {
return
}
// UUID path - value isn't numeric, validate it looks like a UUID
if idStr == "" || idStr == "0" || len(idStr) < 32 {
// UUID path - parse to validate format and produce a canonical, safe string
parsedUUID, parseErr := uuid.Parse(idStr)
if parseErr != nil {
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid id"})
return
}
certUUID := parsedUUID.String()
inUse, err := h.service.IsCertificateInUseByUUID(idStr)
inUse, err := h.service.IsCertificateInUseByUUID(certUUID)
if err != nil {
if err == services.ErrCertNotFound {
c.JSON(http.StatusNotFound, gin.H{"error": "certificate not found"})
return
}
logger.Log().WithError(err).WithField("certificate_uuid", idStr).Error("failed to check certificate usage")
logger.Log().WithError(err).WithField("certificate_uuid", util.SanitizeForLog(certUUID)).Error("failed to check certificate usage")
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to check certificate usage"})
return
}
@@ -459,7 +462,7 @@ func (h *CertificateHandler) Delete(c *gin.Context) {
}
}
if err := h.service.DeleteCertificate(idStr); err != nil {
if err := h.service.DeleteCertificate(certUUID); err != nil {
if err == services.ErrCertInUse {
c.JSON(http.StatusConflict, gin.H{"error": "certificate is in use by one or more proxy hosts"})
return
@@ -468,12 +471,12 @@ func (h *CertificateHandler) Delete(c *gin.Context) {
c.JSON(http.StatusNotFound, gin.H{"error": "certificate not found"})
return
}
logger.Log().WithError(err).WithField("certificate_uuid", idStr).Error("failed to delete certificate")
logger.Log().WithError(err).WithField("certificate_uuid", util.SanitizeForLog(certUUID)).Error("failed to delete certificate")
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to delete certificate"})
return
}
h.sendDeleteNotification(c, idStr)
h.sendDeleteNotification(c, certUUID)
c.JSON(http.StatusOK, gin.H{"message": "certificate deleted"})
}
@@ -482,11 +485,15 @@ func (h *CertificateHandler) sendDeleteNotification(c *gin.Context, certRef stri
return
}
// Re-validate to produce a CodeQL-safe value (breaks taint from user input).
// Callers already pass validated data; this is defense-in-depth.
safeRef := sanitizeCertRef(certRef)
h.notificationMu.Lock()
lastTime, exists := h.lastNotificationTime[certRef]
if exists && time.Since(lastTime) < 10*time.Second {
h.notificationMu.Unlock()
logger.Log().WithField("certificate_ref", certRef).Debug("notification rate limited")
logger.Log().WithField("certificate_ref", safeRef).Debug("notification rate limited")
return
}
h.lastNotificationTime[certRef] = time.Now()
@@ -495,10 +502,22 @@ func (h *CertificateHandler) sendDeleteNotification(c *gin.Context, certRef stri
h.notificationService.SendExternal(c.Request.Context(),
"cert",
"Certificate Deleted",
fmt.Sprintf("Certificate %s deleted", certRef),
fmt.Sprintf("Certificate %s deleted", safeRef),
map[string]any{
"Ref": certRef,
"Ref": safeRef,
"Action": "deleted",
},
)
}
// sanitizeCertRef re-validates a certificate reference (UUID or numeric ID)
// and returns a safe string representation. Returns a placeholder if invalid.
func sanitizeCertRef(ref string) string {
if parsed, err := uuid.Parse(ref); err == nil {
return parsed.String()
}
if n, err := strconv.ParseUint(ref, 10, 64); err == nil {
return strconv.FormatUint(n, 10)
}
return "[invalid-ref]"
}

10
backend/internal/api/handlers/certificate_handler_coverage_test.go
View File

@@ -36,7 +36,7 @@ func TestCertificateHandler_Delete_InvalidID(t *testing.T) {
r.Use(mockAuthMiddleware())
svc := services.NewCertificateService("/tmp", db, nil)
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/invalid", http.NoBody)
w := httptest.NewRecorder()
@@ -52,7 +52,7 @@ func TestCertificateHandler_Delete_NotFound(t *testing.T) {
r.Use(mockAuthMiddleware())
svc := services.NewCertificateService("/tmp", db, nil)
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/9999", http.NoBody)
w := httptest.NewRecorder()
@@ -74,7 +74,7 @@ func TestCertificateHandler_Delete_NoBackupService(t *testing.T) {
// No backup service
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -97,7 +97,7 @@ func TestCertificateHandler_Delete_CheckUsageDBError(t *testing.T) {
r.Use(mockAuthMiddleware())
svc := services.NewCertificateService("/tmp", db, nil)
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -137,7 +137,7 @@ func TestCertificateHandler_Delete_ZeroID(t *testing.T) {
r.Use(mockAuthMiddleware())
svc := services.NewCertificateService("/tmp", db, nil)
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/0", http.NoBody)
w := httptest.NewRecorder()

6
backend/internal/api/handlers/certificate_handler_security_test.go
View File

@@ -32,7 +32,7 @@ func TestCertificateHandler_Delete_RequiresAuth(t *testing.T) {
})
svc := services.NewCertificateService("/tmp", db, nil)
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/1", http.NoBody)
w := httptest.NewRecorder()
@@ -135,7 +135,7 @@ func TestCertificateHandler_Delete_DiskSpaceCheck(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBackup, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, fmt.Sprintf("/api/certificates/%d", cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -186,7 +186,7 @@ func TestCertificateHandler_Delete_NotificationRateLimiting(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBackup, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
// Delete first cert
req1 := httptest.NewRequest(http.MethodDelete, fmt.Sprintf("/api/certificates/%d", cert1.ID), http.NoBody)

24
backend/internal/api/handlers/certificate_handler_test.go
View File

@@ -41,7 +41,7 @@ func setupCertTestRouter(t *testing.T, db *gorm.DB) *gin.Engine {
svc := services.NewCertificateService("/tmp", db, nil)
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
return r
}
@@ -123,7 +123,7 @@ func TestDeleteCertificate_CreatesBackup(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBackupService, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -174,7 +174,7 @@ func TestDeleteCertificate_BackupFailure(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBackupService, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -229,7 +229,7 @@ func TestDeleteCertificate_InUse_NoBackup(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBackupService, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -557,7 +557,7 @@ func TestDeleteCertificate_InvalidID(t *testing.T) {
r.Use(mockAuthMiddleware())
svc := services.NewCertificateService("/tmp", db, nil)
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/invalid", http.NoBody)
w := httptest.NewRecorder()
@@ -582,7 +582,7 @@ func TestDeleteCertificate_ZeroID(t *testing.T) {
r.Use(mockAuthMiddleware())
svc := services.NewCertificateService("/tmp", db, nil)
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/0", http.NoBody)
w := httptest.NewRecorder()
@@ -621,7 +621,7 @@ func TestDeleteCertificate_LowDiskSpace(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBackupService, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -672,7 +672,7 @@ func TestDeleteCertificate_DiskSpaceCheckError(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBackupService, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -726,7 +726,7 @@ func TestDeleteCertificate_ExpiredLetsEncrypt_NotInUse(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBS, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -784,7 +784,7 @@ func TestDeleteCertificate_ValidLetsEncrypt_NotInUse(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBS, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -822,7 +822,7 @@ func TestDeleteCertificate_UsageCheckError(t *testing.T) {
r.Use(mockAuthMiddleware())
svc := services.NewCertificateService("/tmp", db, nil)
h := NewCertificateHandler(svc, nil, nil)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert.ID), http.NoBody)
w := httptest.NewRecorder()
@@ -867,7 +867,7 @@ func TestDeleteCertificate_NotificationRateLimit(t *testing.T) {
}
h := NewCertificateHandler(svc, mockBackupService, ns)
r.DELETE("/api/certificates/:id", h.Delete)
r.DELETE("/api/certificates/:uuid", h.Delete)
// Delete first certificate
req := httptest.NewRequest(http.MethodDelete, "/api/certificates/"+toStr(cert1.ID), http.NoBody)