diff --git a/internal/api/middleware/middleware.go b/internal/api/middleware/middleware.go index a1bdb34..4e795ee 100644 --- a/internal/api/middleware/middleware.go +++ b/internal/api/middleware/middleware.go @@ -44,6 +44,13 @@ func RequestID(next http.Handler) http.Handler { // Logging middleware logs request details including method, path, status, and duration. // Deprecated: Use NewLogging for structured logging with slog. +// +// CWE-117 log-injection defense: r.Method and r.URL.Path are +// attacker-controllable (request-line bytes — Go's net/http leaves +// percent-decoded path segments in r.URL.Path, which can include CR/LF +// in the decoded form even though the raw HTTP request line cannot). +// strings.ReplaceAll on CR/LF/NUL strips the forgery vector before the +// log line is emitted. Closes CodeQL #17 (go/log-injection). func Logging(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { start := time.Now() @@ -55,10 +62,31 @@ func Logging(next http.Handler) http.Handler { duration := time.Since(start) requestID := getRequestID(r.Context()) - log.Printf("[%s] %s %s %d %v", requestID, r.Method, r.URL.Path, wrapped.statusCode, duration) + log.Printf("[%s] %s %s %d %v", + requestID, + scrubLogValue(r.Method), + scrubLogValue(r.URL.Path), + wrapped.statusCode, + duration, + ) }) } +// scrubLogValue strips control characters that an attacker could use to +// forge log entries (CWE-117). The replacement is structural, not +// destructive: we collapse CR/LF/NUL to a single space rather than +// dropping them, so an operator scanning the log can still see that the +// field was present (just neutralized). Defense in depth: the +// production middleware should prefer NewLogging (slog with structured +// fields) where the logger handles escape on its own. +func scrubLogValue(v string) string { + if !strings.ContainsAny(v, "\r\n\x00") { + return v + } + r := strings.NewReplacer("\r", " ", "\n", " ", "\x00", " ") + return r.Replace(v) +} + // NewLogging creates a structured logging middleware using slog. // Logs request_id, method, path, status, duration_ms, and remote_addr. func NewLogging(logger *slog.Logger) func(http.Handler) http.Handler { diff --git a/internal/service/scep_probe.go b/internal/service/scep_probe.go index d03e64f..c78995b 100644 --- a/internal/service/scep_probe.go +++ b/internal/service/scep_probe.go @@ -75,20 +75,21 @@ func (s *NetworkScanService) ProbeSCEP(ctx context.Context, rawURL string) (*dom } // Step 1: cheap up-front URL validation (SSRF early diagnostic). - // Defaults to validation.ValidateSafeURL; tests inject a permissive - // validator via service-level field so they can hit httptest - // loopback servers (which the production validator correctly - // rejects). Mirrors the webhook notifier's `newForTest` pattern. - validateURL := s.scepValidateURL - if validateURL == nil { - validateURL = validation.ValidateSafeURL - } - if err := validateURL(rawURL); err != nil { - result.Reachable = false - result.Error = "url validation: " + err.Error() - result.ProbeDurationMs = time.Since(started).Milliseconds() - s.persistProbeResult(ctx, result) - return result, fmt.Errorf("scep probe: validate url: %w", err) + // Direct literal call to validation.ValidateSafeURL so CodeQL + // go/request-forgery sees the sanitizer in-scope of every + // downstream HTTP call. Tests that need to hit httptest loopback + // servers grant an exemption via s.scepValidateURL (mirrors the + // webhook notifier's `newForTest` pattern). Production callers + // leave scepValidateURL nil so any production-validator + // rejection wins. + if err := validation.ValidateSafeURL(rawURL); err != nil { + if s.scepValidateURL == nil || s.scepValidateURL(rawURL) != nil { + result.Reachable = false + result.Error = "url validation: " + err.Error() + result.ProbeDurationMs = time.Since(started).Milliseconds() + s.persistProbeResult(ctx, result) + return result, fmt.Errorf("scep probe: validate url: %w", err) + } } // Normalize the base URL — strip any trailing query string so we @@ -255,12 +256,22 @@ func (s *NetworkScanService) scepGetCACert(ctx context.Context, client *http.Cli // callers leave scepValidateURL nil so validation.ValidateSafeURL // is the active gate. func (s *NetworkScanService) scepHTTPGet(ctx context.Context, client *http.Client, rawURL string) ([]byte, error) { - validateURL := s.scepValidateURL - if validateURL == nil { - validateURL = validation.ValidateSafeURL - } - if err := validateURL(rawURL); err != nil { - return nil, fmt.Errorf("validate url: %w", err) + // Production-grade SSRF validator — direct literal call so CodeQL + // go/request-forgery recognizes it as a sanitizer in-scope of the + // client.Do sink below. Tests that need to hit httptest loopback + // servers grant an exemption via s.scepValidateURL (returning nil + // for the test URL); when no exemption applies, the production + // validator's rejection wins. Production callers leave + // scepValidateURL nil so the production validator is the only gate. + if err := validation.ValidateSafeURL(rawURL); err != nil { + // Test-only exemption hook. The override returns nil for URLs + // the test wants to allow despite the production validator's + // rejection (loopback / link-local in httptest scenarios). + // In production scepValidateURL is nil, so any production + // validator rejection bubbles up unconditionally. + if s.scepValidateURL == nil || s.scepValidateURL(rawURL) != nil { + return nil, fmt.Errorf("validate url: %w", err) + } } req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil)