From ef109b9513288f55f0541c38c16788ff0d5a0470 Mon Sep 17 00:00:00 2001 From: clawbot Date: Tue, 17 Mar 2026 02:52:34 -0700 Subject: [PATCH] fix: address review findings for observability PR 1. Security: Replace insecure extractRemoteIP() in audit service with middleware.RealIP() which validates trusted proxies before trusting X-Real-IP/X-Forwarded-For headers. Export RealIP from middleware. Update audit tests to verify anti-spoofing behavior. 2. Audit coverage: Add audit instrumentation to all 9 handlers that had dead action constants: HandleEnvVarSave, HandleLabelAdd, HandleLabelEdit, HandleLabelDelete, HandleVolumeAdd, HandleVolumeEdit, HandleVolumeDelete, HandlePortAdd, HandlePortDelete. 3. README: Fix API path from /api/audit to /api/v1/audit. 4. README: Fix duplicate numbering in DI order section (items 10-11 were listed twice, now correctly numbered 10-16). --- README.md | 12 ++++----- internal/handlers/app.go | 38 +++++++++++++++++++++++++++- internal/middleware/middleware.go | 8 +++--- internal/middleware/realip_test.go | 6 ++--- internal/service/audit/audit.go | 33 +++--------------------- internal/service/audit/audit_test.go | 27 +++++++++++++++++++- 6 files changed, 79 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index ed27973..de6467f 100644 --- a/README.md +++ b/README.md @@ -67,11 +67,11 @@ Uses Uber fx for dependency injection. Components are wired in this order: 9. `docker` - Docker client 10. `notify` - Notification service 11. `audit` - Audit logging service -10. `deploy` - Deployment service -11. `webhook` - Webhook processing -12. `middleware` - HTTP middleware -13. `handlers` - HTTP handlers -14. `server` - HTTP server +12. `deploy` - Deployment service +13. `webhook` - Webhook processing +14. `middleware` - HTTP middleware +15. `handlers` - HTTP handlers +16. `server` - HTTP server ### Request Flow @@ -247,7 +247,7 @@ All user-facing actions are recorded in an `audit_log` SQLite table with: Audited actions include login/logout, app CRUD, deployments, container start/stop/restart, rollbacks, deployment cancellation, and webhook receipt. -The audit log is available via the API at `GET /api/audit?limit=N` (max 500, +The audit log is available via the API at `GET /api/v1/audit?limit=N` (max 500, default 50). ### Structured Logging diff --git a/internal/handlers/app.go b/internal/handlers/app.go index 57b4814..78ff716 100644 --- a/internal/handlers/app.go +++ b/internal/handlers/app.go @@ -1023,6 +1023,10 @@ func (h *Handlers) HandleEnvVarSave() http.HandlerFunc { return } + h.auditLog(request, models.AuditActionEnvVarSave, + models.AuditResourceEnvVar, application.ID, + fmt.Sprintf("saved %d env vars", len(modelPairs))) + h.respondJSON(writer, request, map[string]bool{"ok": true}, http.StatusOK) } } @@ -1039,7 +1043,13 @@ func (h *Handlers) HandleLabelAdd() http.HandlerFunc { label.Key = key label.Value = value - return label.Save(ctx) + err := label.Save(ctx) + if err == nil { + h.auditLog(request, models.AuditActionLabelAdd, + models.AuditResourceLabel, application.ID, "added label: "+key) + } + + return err }, ) } @@ -1068,6 +1078,9 @@ func (h *Handlers) HandleLabelDelete() http.HandlerFunc { deleteErr := label.Delete(request.Context()) if deleteErr != nil { h.log.Error("failed to delete label", "error", deleteErr) + } else { + h.auditLog(request, models.AuditActionLabelDelete, + models.AuditResourceLabel, appID, "deleted label: "+label.Key) } http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) @@ -1125,6 +1138,10 @@ func (h *Handlers) HandleVolumeAdd() http.HandlerFunc { saveErr := volume.Save(request.Context()) if saveErr != nil { h.log.Error("failed to add volume", "error", saveErr) + } else { + h.auditLog(request, models.AuditActionVolumeAdd, + models.AuditResourceVolume, application.ID, + "added volume: "+hostPath+":"+containerPath) } http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) @@ -1154,6 +1171,10 @@ func (h *Handlers) HandleVolumeDelete() http.HandlerFunc { deleteErr := volume.Delete(request.Context()) if deleteErr != nil { h.log.Error("failed to delete volume", "error", deleteErr) + } else { + h.auditLog(request, models.AuditActionVolumeDelete, + models.AuditResourceVolume, appID, + "deleted volume: "+volume.HostPath+":"+volume.ContainerPath) } http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) @@ -1203,6 +1224,10 @@ func (h *Handlers) HandlePortAdd() http.HandlerFunc { saveErr := port.Save(request.Context()) if saveErr != nil { h.log.Error("failed to save port", "error", saveErr) + } else { + h.auditLog(request, models.AuditActionPortAdd, + models.AuditResourcePort, application.ID, + fmt.Sprintf("added port: %d:%d/%s", hostPort, containerPort, protocol)) } http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) @@ -1249,6 +1274,10 @@ func (h *Handlers) HandlePortDelete() http.HandlerFunc { deleteErr := port.Delete(request.Context()) if deleteErr != nil { h.log.Error("failed to delete port", "error", deleteErr) + } else { + h.auditLog(request, models.AuditActionPortDelete, + models.AuditResourcePort, appID, + fmt.Sprintf("deleted port: %d:%d/%s", port.HostPort, port.ContainerPort, port.Protocol)) } http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) @@ -1324,6 +1353,9 @@ func (h *Handlers) HandleLabelEdit() http.HandlerFunc { saveErr := label.Save(request.Context()) if saveErr != nil { h.log.Error("failed to update label", "error", saveErr) + } else { + h.auditLog(request, models.AuditActionLabelEdit, + models.AuditResourceLabel, appID, "edited label: "+key) } http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) @@ -1382,6 +1414,10 @@ func (h *Handlers) HandleVolumeEdit() http.HandlerFunc { saveErr := volume.Save(request.Context()) if saveErr != nil { h.log.Error("failed to update volume", "error", saveErr) + } else { + h.auditLog(request, models.AuditActionVolumeEdit, + models.AuditResourceVolume, appID, + "edited volume: "+hostPath+":"+containerPath) } http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index d5ff91e..668fb05 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -105,7 +105,7 @@ func (m *Middleware) Logging() func(http.Handler) http.Handler { "request_id", reqID, "referer", request.Referer(), "proto", request.Proto, - "remoteIP", realIP(request), + "remoteIP", RealIP(request), "status", lrw.statusCode, "bytes", lrw.bytesWritten, "latency_ms", latency.Milliseconds(), @@ -170,11 +170,11 @@ func isTrustedProxy(ip net.IP) bool { return false } -// realIP extracts the client's real IP address from the request. +// RealIP extracts the client's real IP address from the request. // Proxy headers (X-Real-IP, X-Forwarded-For) are only trusted when the // direct connection originates from an RFC1918/loopback address. // Otherwise, headers are ignored and RemoteAddr is used (fail closed). -func realIP(r *http.Request) string { +func RealIP(r *http.Request) string { addr := ipFromHostPort(r.RemoteAddr) remoteIP := net.ParseIP(addr) @@ -365,7 +365,7 @@ func (m *Middleware) LoginRateLimit() func(http.Handler) http.Handler { writer http.ResponseWriter, request *http.Request, ) { - ip := realIP(request) + ip := RealIP(request) limiter := loginLimiter.getLimiter(ip) if !limiter.Allow() { diff --git a/internal/middleware/realip_test.go b/internal/middleware/realip_test.go index f38aa48..cc9ae11 100644 --- a/internal/middleware/realip_test.go +++ b/internal/middleware/realip_test.go @@ -1,4 +1,4 @@ -package middleware //nolint:testpackage // tests unexported realIP function +package middleware //nolint:testpackage // tests RealIP via internal package access import ( "context" @@ -126,9 +126,9 @@ func TestRealIP(t *testing.T) { //nolint:funlen // table-driven test req.Header.Set("X-Forwarded-For", tt.xff) } - got := realIP(req) + got := RealIP(req) if got != tt.want { - t.Errorf("realIP() = %q, want %q", got, tt.want) + t.Errorf("RealIP() = %q, want %q", got, tt.want) } }) } diff --git a/internal/service/audit/audit.go b/internal/service/audit/audit.go index 2ebd1f5..6c884c5 100644 --- a/internal/service/audit/audit.go +++ b/internal/service/audit/audit.go @@ -5,15 +5,14 @@ import ( "context" "database/sql" "log/slog" - "net" "net/http" - "strings" "go.uber.org/fx" "sneak.berlin/go/upaas/internal/database" "sneak.berlin/go/upaas/internal/logger" "sneak.berlin/go/upaas/internal/metrics" + "sneak.berlin/go/upaas/internal/middleware" "sneak.berlin/go/upaas/internal/models" ) @@ -98,42 +97,16 @@ func (svc *Service) Log(ctx context.Context, entry LogEntry) { } // LogFromRequest records an audit log entry, extracting the remote IP from -// the HTTP request. +// the HTTP request using the middleware's trusted-proxy-aware IP resolution. func (svc *Service) LogFromRequest( ctx context.Context, request *http.Request, entry LogEntry, ) { - entry.RemoteIP = extractRemoteIP(request) + entry.RemoteIP = middleware.RealIP(request) svc.Log(ctx, entry) } -// extractRemoteIP extracts the client IP from the request, preferring -// X-Real-IP and X-Forwarded-For headers from trusted proxies. -func extractRemoteIP(r *http.Request) string { - // Check X-Real-IP first - if ip := strings.TrimSpace(r.Header.Get("X-Real-IP")); ip != "" { - return ip - } - - // Check X-Forwarded-For (leftmost = client) - if xff := r.Header.Get("X-Forwarded-For"); xff != "" { - if parts := strings.SplitN(xff, ",", 2); len(parts) > 0 { //nolint:mnd // split limit - if ip := strings.TrimSpace(parts[0]); ip != "" { - return ip - } - } - } - - // Fall back to RemoteAddr - host, _, err := net.SplitHostPort(r.RemoteAddr) - if err != nil { - return r.RemoteAddr - } - - return host -} - // Recent returns the most recent audit log entries. func (svc *Service) Recent( ctx context.Context, diff --git a/internal/service/audit/audit_test.go b/internal/service/audit/audit_test.go index 7ee242f..830569b 100644 --- a/internal/service/audit/audit_test.go +++ b/internal/service/audit/audit_test.go @@ -103,13 +103,15 @@ func TestAuditServiceLogFromRequest(t *testing.T) { assert.Equal(t, "app-1", entries[0].ResourceID.String) } -func TestAuditServiceLogFromRequestWithXRealIP(t *testing.T) { +func TestAuditServiceLogFromRequestWithXRealIPTrustedProxy(t *testing.T) { t.Parallel() svc, db := setupTestAuditService(t) ctx := context.Background() + // When the request comes from a trusted proxy (RFC1918), X-Real-IP is honoured. request := httptest.NewRequest(http.MethodPost, "/apps", nil) + request.RemoteAddr = "10.0.0.1:1234" request.Header.Set("X-Real-IP", "203.0.113.50") svc.LogFromRequest(ctx, request, audit.LogEntry{ @@ -124,6 +126,29 @@ func TestAuditServiceLogFromRequestWithXRealIP(t *testing.T) { assert.Equal(t, "203.0.113.50", entries[0].RemoteIP.String) } +func TestAuditServiceLogFromRequestWithXRealIPUntrustedProxy(t *testing.T) { + t.Parallel() + + svc, db := setupTestAuditService(t) + ctx := context.Background() + + // When the request comes from a public IP, X-Real-IP is ignored (anti-spoof). + request := httptest.NewRequest(http.MethodPost, "/apps", nil) + request.RemoteAddr = "203.0.113.99:1234" + request.Header.Set("X-Real-IP", "10.0.0.1") + + svc.LogFromRequest(ctx, request, audit.LogEntry{ + Username: "admin", + Action: models.AuditActionAppCreate, + ResourceType: models.AuditResourceApp, + }) + + entries, err := models.FindAuditEntries(ctx, db, 10) + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Equal(t, "203.0.113.99", entries[0].RemoteIP.String) +} + func TestAuditServiceRecent(t *testing.T) { t.Parallel()