diff --git a/backend/cmd/api/main_test.go b/backend/cmd/api/main_test.go index 69bc5a9c..d260b552 100644 --- a/backend/cmd/api/main_test.go +++ b/backend/cmd/api/main_test.go @@ -311,7 +311,8 @@ func TestMain_DefaultStartupGracefulShutdown_Subprocess(t *testing.T) { if err != nil { t.Fatalf("find free http port: %v", err) } - if err := os.MkdirAll(filepath.Dir(dbPath), 0o750); err != nil { + err = os.MkdirAll(filepath.Dir(dbPath), 0o750) + if err != nil { t.Fatalf("mkdir db dir: %v", err) } diff --git a/backend/cmd/localpatchreport/main.go b/backend/cmd/localpatchreport/main.go index 74d8ec0e..479b2d36 100644 --- a/backend/cmd/localpatchreport/main.go +++ b/backend/cmd/localpatchreport/main.go @@ -64,11 +64,13 @@ func main() { jsonOutPath := resolvePath(repoRoot, *jsonOutFlag) mdOutPath := resolvePath(repoRoot, *mdOutFlag) - if err := assertFileExists(backendCoveragePath, "backend coverage file"); err != nil { + err = assertFileExists(backendCoveragePath, "backend coverage file") + if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) } - if err := assertFileExists(frontendCoveragePath, "frontend coverage file"); err != nil { + err = assertFileExists(frontendCoveragePath, "frontend coverage file") + if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) } diff --git a/backend/cmd/localpatchreport/main_test.go b/backend/cmd/localpatchreport/main_test.go index df04b8f8..a7e2a758 100644 --- a/backend/cmd/localpatchreport/main_test.go +++ b/backend/cmd/localpatchreport/main_test.go @@ -235,7 +235,8 @@ func TestGitDiffAndWriters(t *testing.T) { t.Fatalf("expected empty diff for HEAD...HEAD, got: %q", diffContent) } - if _, err := gitDiff(repoRoot, "bad-baseline"); err == nil { + _, err = gitDiff(repoRoot, "bad-baseline") + if err == nil { t.Fatal("expected gitDiff failure for invalid baseline") } @@ -263,7 +264,8 @@ func TestGitDiffAndWriters(t *testing.T) { } jsonPath := filepath.Join(t.TempDir(), "report.json") - if err := writeJSON(jsonPath, report); err != nil { + err = writeJSON(jsonPath, report) + if err != nil { t.Fatalf("writeJSON should succeed: %v", err) } // #nosec G304 -- Test reads artifact path created by this test. @@ -276,7 +278,8 @@ func TestGitDiffAndWriters(t *testing.T) { } markdownPath := filepath.Join(t.TempDir(), "report.md") - if err := writeMarkdown(markdownPath, report, "backend/coverage.txt", "frontend/coverage/lcov.info"); err != nil { + err = writeMarkdown(markdownPath, report, "backend/coverage.txt", "frontend/coverage/lcov.info") + if err != nil { t.Fatalf("writeMarkdown should succeed: %v", err) } // #nosec G304 -- Test reads artifact path created by this test. diff --git a/backend/internal/api/handlers/auth_handler.go b/backend/internal/api/handlers/auth_handler.go index 28695ec8..32923426 100644 --- a/backend/internal/api/handlers/auth_handler.go +++ b/backend/internal/api/handlers/auth_handler.go @@ -127,18 +127,17 @@ func isLocalRequest(c *gin.Context) bool { // setSecureCookie sets an auth cookie with security best practices // - HttpOnly: prevents JavaScript access (XSS protection) -// - Secure: derived from request scheme to allow HTTP/IP logins when needed +// - Secure: always true to prevent cookie transmission over cleartext channels // - SameSite: Strict for HTTPS, Lax for HTTP/IP to allow forward-auth redirects func setSecureCookie(c *gin.Context, name, value string, maxAge int) { scheme := requestScheme(c) - secure := scheme == "https" + secure := true sameSite := http.SameSiteStrictMode if scheme != "https" { sameSite = http.SameSiteLaxMode } if isLocalRequest(c) { - secure = false sameSite = http.SameSiteLaxMode } @@ -152,7 +151,7 @@ func setSecureCookie(c *gin.Context, name, value string, maxAge int) { maxAge, // maxAge in seconds "/", // path domain, // domain (empty = current host) - secure, // secure (HTTPS only in production) + secure, // secure (always true) true, // httpOnly (no JS access) ) } diff --git a/backend/internal/api/handlers/auth_handler_test.go b/backend/internal/api/handlers/auth_handler_test.go index 4241adea..ca4b1daf 100644 --- a/backend/internal/api/handlers/auth_handler_test.go +++ b/backend/internal/api/handlers/auth_handler_test.go @@ -94,7 +94,7 @@ func TestSetSecureCookie_HTTP_Lax(t *testing.T) { cookies := recorder.Result().Cookies() require.Len(t, cookies, 1) c := cookies[0] - assert.False(t, c.Secure) + assert.True(t, c.Secure) assert.Equal(t, http.SameSiteLaxMode, c.SameSite) } @@ -115,7 +115,7 @@ func TestSetSecureCookie_ForwardedHTTPS_LocalhostForcesInsecure(t *testing.T) { cookies := recorder.Result().Cookies() require.Len(t, cookies, 1) cookie := cookies[0] - assert.False(t, cookie.Secure) + assert.True(t, cookie.Secure) assert.Equal(t, http.SameSiteLaxMode, cookie.SameSite) } @@ -136,7 +136,7 @@ func TestSetSecureCookie_ForwardedHTTPS_LoopbackForcesInsecure(t *testing.T) { cookies := recorder.Result().Cookies() require.Len(t, cookies, 1) cookie := cookies[0] - assert.False(t, cookie.Secure) + assert.True(t, cookie.Secure) assert.Equal(t, http.SameSiteLaxMode, cookie.SameSite) } @@ -158,7 +158,7 @@ func TestSetSecureCookie_ForwardedHostLocalhostForcesInsecure(t *testing.T) { cookies := recorder.Result().Cookies() require.Len(t, cookies, 1) cookie := cookies[0] - assert.False(t, cookie.Secure) + assert.True(t, cookie.Secure) assert.Equal(t, http.SameSiteLaxMode, cookie.SameSite) } @@ -180,7 +180,7 @@ func TestSetSecureCookie_OriginLoopbackForcesInsecure(t *testing.T) { cookies := recorder.Result().Cookies() require.Len(t, cookies, 1) cookie := cookies[0] - assert.False(t, cookie.Secure) + assert.True(t, cookie.Secure) assert.Equal(t, http.SameSiteLaxMode, cookie.SameSite) } diff --git a/backend/internal/notifications/http_wrapper.go b/backend/internal/notifications/http_wrapper.go index aa1da80b..3864b2b8 100644 --- a/backend/internal/notifications/http_wrapper.go +++ b/backend/internal/notifications/http_wrapper.go @@ -82,13 +82,22 @@ func (w *HTTPWrapper) Send(ctx context.Context, request HTTPWrapperRequest) (*HT return nil, err } + parsedValidatedURL, err := neturl.Parse(validatedURL) + if err != nil { + return nil, fmt.Errorf("destination URL validation failed") + } + + if err := w.guardDestination(parsedValidatedURL); err != nil { + return nil, err + } + headers := sanitizeOutboundHeaders(request.Headers) client := w.httpClientFactory(w.allowHTTP, w.maxRedirects) w.applyRedirectGuard(client) var lastErr error for attempt := 1; attempt <= w.retryPolicy.MaxAttempts; attempt++ { - httpReq, reqErr := http.NewRequestWithContext(ctx, http.MethodPost, validatedURL, bytes.NewReader(request.Body)) + httpReq, reqErr := http.NewRequestWithContext(ctx, http.MethodPost, parsedValidatedURL.String(), bytes.NewReader(request.Body)) if reqErr != nil { return nil, fmt.Errorf("create outbound request: %w", reqErr) } @@ -101,10 +110,27 @@ func (w *HTTPWrapper) Send(ctx context.Context, request HTTPWrapperRequest) (*HT httpReq.Header.Set("Content-Type", "application/json") } - if guardErr := w.guardOutboundRequestURL(httpReq); guardErr != nil { + validationOptions := []security.ValidationOption{} + if w.allowHTTP { + validationOptions = append(validationOptions, security.WithAllowHTTP(), security.WithAllowLocalhost()) + } + + safeURL, safeURLErr := security.ValidateExternalURL(httpReq.URL.String(), validationOptions...) + if safeURLErr != nil { + return nil, fmt.Errorf("destination URL validation failed") + } + + safeParsedURL, safeParseErr := neturl.Parse(safeURL) + if safeParseErr != nil { + return nil, fmt.Errorf("destination URL validation failed") + } + + if guardErr := w.guardDestination(safeParsedURL); guardErr != nil { return nil, guardErr } + httpReq.URL = safeParsedURL + resp, doErr := client.Do(httpReq) if doErr != nil { lastErr = doErr @@ -210,13 +236,79 @@ func (w *HTTPWrapper) guardOutboundRequestURL(httpReq *http.Request) error { return err } - if validatedURL != reqURL { + parsedValidatedURL, err := neturl.Parse(validatedURL) + if err != nil { return fmt.Errorf("destination URL validation failed") } + return w.guardDestination(parsedValidatedURL) +} + +func (w *HTTPWrapper) guardDestination(destinationURL *neturl.URL) error { + if destinationURL == nil { + return fmt.Errorf("destination URL validation failed") + } + + if destinationURL.User != nil || destinationURL.Fragment != "" { + return fmt.Errorf("destination URL validation failed") + } + + hostname := strings.TrimSpace(destinationURL.Hostname()) + if hostname == "" { + return fmt.Errorf("destination URL validation failed") + } + + if parsedIP := net.ParseIP(hostname); parsedIP != nil { + if !w.isAllowedDestinationIP(hostname, parsedIP) { + return fmt.Errorf("destination URL validation failed") + } + return nil + } + + resolvedIPs, err := net.LookupIP(hostname) + if err != nil || len(resolvedIPs) == 0 { + return fmt.Errorf("destination URL validation failed") + } + + for _, resolvedIP := range resolvedIPs { + if !w.isAllowedDestinationIP(hostname, resolvedIP) { + return fmt.Errorf("destination URL validation failed") + } + } + return nil } +func (w *HTTPWrapper) isAllowedDestinationIP(hostname string, ip net.IP) bool { + if ip == nil { + return false + } + + if ip.IsUnspecified() || ip.IsMulticast() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { + return false + } + + if ip.IsLoopback() { + return w.allowHTTP && isLocalDestinationHost(hostname) + } + + if network.IsPrivateIP(ip) { + return false + } + + return true +} + +func isLocalDestinationHost(host string) bool { + trimmedHost := strings.TrimSpace(host) + if strings.EqualFold(trimmedHost, "localhost") { + return true + } + + parsedIP := net.ParseIP(trimmedHost) + return parsedIP != nil && parsedIP.IsLoopback() +} + func shouldRetry(resp *http.Response, err error) bool { if err != nil { var netErr net.Error diff --git a/backend/internal/notifications/http_wrapper_test.go b/backend/internal/notifications/http_wrapper_test.go index 085f2b79..04f0a70f 100644 --- a/backend/internal/notifications/http_wrapper_test.go +++ b/backend/internal/notifications/http_wrapper_test.go @@ -274,3 +274,24 @@ func TestHTTPWrapperGuardOutboundRequestURLAllowsValidatedDestination(t *testing t.Fatalf("expected validated destination to pass guard, got: %v", err) } } + +func TestHTTPWrapperGuardOutboundRequestURLRejectsUserInfo(t *testing.T) { + wrapper := NewNotifyHTTPWrapper() + wrapper.allowHTTP = true + + httpReq := &http.Request{URL: &neturl.URL{Scheme: "http", Host: "127.0.0.1", User: neturl.UserPassword("user", "pass"), Path: "/hook"}} + err := wrapper.guardOutboundRequestURL(httpReq) + if err == nil || !strings.Contains(err.Error(), "destination URL validation failed") { + t.Fatalf("expected userinfo rejection, got: %v", err) + } +} + +func TestHTTPWrapperGuardOutboundRequestURLRejectsFragment(t *testing.T) { + wrapper := NewNotifyHTTPWrapper() + + httpReq := &http.Request{URL: &neturl.URL{Scheme: "https", Host: "example.com", Path: "/hook", Fragment: "frag"}} + err := wrapper.guardOutboundRequestURL(httpReq) + if err == nil || !strings.Contains(err.Error(), "destination URL validation failed") { + t.Fatalf("expected fragment rejection, got: %v", err) + } +} diff --git a/backend/internal/services/enhanced_security_notification_service.go b/backend/internal/services/enhanced_security_notification_service.go index 9754aef6..a6495d2d 100644 --- a/backend/internal/services/enhanced_security_notification_service.go +++ b/backend/internal/services/enhanced_security_notification_service.go @@ -394,8 +394,8 @@ func (s *EnhancedSecurityNotificationService) MigrateFromLegacyConfig() error { NotifySecurityRateLimitHits: legacyConfig.NotifyRateLimitHits, URL: legacyConfig.WebhookURL, } - if err := tx.Create(&provider).Error; err != nil { - return fmt.Errorf("create managed provider: %w", err) + if createErr := tx.Create(&provider).Error; createErr != nil { + return fmt.Errorf("create managed provider: %w", createErr) } } else if err != nil { return fmt.Errorf("query managed provider: %w", err) @@ -405,8 +405,8 @@ func (s *EnhancedSecurityNotificationService) MigrateFromLegacyConfig() error { provider.NotifySecurityACLDenies = legacyConfig.NotifyACLDenies provider.NotifySecurityRateLimitHits = legacyConfig.NotifyRateLimitHits provider.URL = legacyConfig.WebhookURL - if err := tx.Save(&provider).Error; err != nil { - return fmt.Errorf("update managed provider: %w", err) + if saveErr := tx.Save(&provider).Error; saveErr != nil { + return fmt.Errorf("update managed provider: %w", saveErr) } } @@ -430,7 +430,7 @@ func (s *EnhancedSecurityNotificationService) MigrateFromLegacyConfig() error { } // Upsert marker - if err := tx.Where("key = ?", newMarkerSetting.Key).First(&markerSetting).Error; err == gorm.ErrRecordNotFound { + if queryErr := tx.Where("key = ?", newMarkerSetting.Key).First(&markerSetting).Error; queryErr == gorm.ErrRecordNotFound { return tx.Create(&newMarkerSetting).Error } newMarkerSetting.ID = markerSetting.ID diff --git a/backend/internal/services/enhanced_security_notification_service_discord_only_test.go b/backend/internal/services/enhanced_security_notification_service_discord_only_test.go index 6a5611ce..a05230f4 100644 --- a/backend/internal/services/enhanced_security_notification_service_discord_only_test.go +++ b/backend/internal/services/enhanced_security_notification_service_discord_only_test.go @@ -60,8 +60,8 @@ func TestDiscordOnly_DispatchToProviderAcceptsDiscord(t *testing.T) { // Verify payload structure var payload models.SecurityEvent - err := json.NewDecoder(r.Body).Decode(&payload) - assert.NoError(t, err) + decodeErr := json.NewDecoder(r.Body).Decode(&payload) + assert.NoError(t, decodeErr) assert.Equal(t, "waf_block", payload.EventType) w.WriteHeader(http.StatusOK) diff --git a/backend/internal/services/notification_service.go b/backend/internal/services/notification_service.go index 99f7863f..e8a9ce5e 100644 --- a/backend/internal/services/notification_service.go +++ b/backend/internal/services/notification_service.go @@ -383,12 +383,12 @@ func (s *NotificationService) sendJSONPayload(ctx context.Context, p models.Noti } } - if _, err := s.httpWrapper.Send(ctx, notifications.HTTPWrapperRequest{ + if _, sendErr := s.httpWrapper.Send(ctx, notifications.HTTPWrapperRequest{ URL: p.URL, Headers: headers, Body: body.Bytes(), - }); err != nil { - return fmt.Errorf("failed to send webhook: %w", err) + }); sendErr != nil { + return fmt.Errorf("failed to send webhook: %w", sendErr) } return nil } diff --git a/docs/plans/current_spec.md b/docs/plans/current_spec.md index 4d2aa276..1a4bb74c 100644 --- a/docs/plans/current_spec.md +++ b/docs/plans/current_spec.md @@ -464,3 +464,65 @@ If compatibility uploads create noise, duplicate alert confusion, or unstable ch - **PR-1 (recommended single PR, low-risk additive):** add compatibility SARIF uploads in `docker-build.yml` (`scan-pr-image`) with SARIF existence guards, `continue-on-error` on compatibility uploads, and mandatory non-PR category hardening, plus brief inline rationale comments. - **PR-2 (cleanup PR, delayed):** remove `.github/workflows/docker-publish.yml:build-and-push` compatibility upload after stabilization window and verify no warning recurrence. + +--- + +## CodeQL Targeted Remediation Plan — Current Findings (2026-02-24) + +Status: Planned (minimal and surgical) +Scope: Three current findings only; no broad refactors; no suppression-first approach. + +### Implementation Order (behavior-safe) + +1. **Frontend low-risk correctness fix first** + - Resolve `js/comparison-between-incompatible-types` in `frontend/src/components/CredentialManager.tsx`. + - Reason: isolated UI logic change with lowest regression risk. + +2. **Cookie security hardening second** + - Resolve `go/cookie-secure-not-set` in `backend/internal/api/handlers/auth_handler.go`. + - Reason: auth behavior impact is manageable with existing token-in-response fallback. + +3. **SSRF/request-forgery hardening last** + - Resolve `go/request-forgery` in `backend/internal/notifications/http_wrapper.go`. + - Reason: highest security sensitivity; keep changes narrowly at request sink path. + +### File-Level Actions + +1. **`frontend/src/components/CredentialManager.tsx`** (`js/comparison-between-incompatible-types`) + - Remove the redundant null comparison that is always true in the guarded render path (line currently flagged around delete-confirm dialog open state). + - Keep existing dialog UX and delete flow unchanged. + - Prefer direct logic cleanup (real fix), not query suppression. + +2. **`backend/internal/api/handlers/auth_handler.go`** (`go/cookie-secure-not-set`) + - Ensure auth cookie emission is secure-by-default and does not set insecure auth cookies on non-HTTPS requests. + - Preserve login behavior by continuing to return token in response body for non-cookie fallback clients. + - Add/update targeted tests to verify: + - secure flag is set for HTTPS auth cookie, + - no insecure auth cookie path is emitted, + - login/refresh/logout flows remain functional. + +3. **`backend/internal/notifications/http_wrapper.go`** (`go/request-forgery`) + - Strengthen sink-adjacent outbound validation before network send: + - enforce parsed host/IP re-validation immediately before `client.Do`, + - verify resolved destination IPs are not loopback/private/link-local/multicast/unspecified, + - keep existing HTTPS/query-auth restrictions and retry behavior intact. + - Add/update focused wrapper tests for blocked internal targets and allowed public targets. + - Prefer explicit validation controls over suppression annotations. + +### Post-Fix Validation Commands (exact) + +1. **Targeted tests** + - `cd /projects/Charon && go test ./backend/internal/notifications -count=1` + - `cd /projects/Charon && go test ./backend/internal/api/handlers -count=1` + - `cd /projects/Charon/frontend && npm run test -- src/components/__tests__/CredentialManager.test.tsx` + +2. **Lint / type-check** + - `cd /projects/Charon && make lint-fast` + - `cd /projects/Charon/frontend && npm run type-check` + +3. **CodeQL scans (CI-aligned local scripts)** + - `cd /projects/Charon && bash scripts/pre-commit-hooks/codeql-go-scan.sh` + - `cd /projects/Charon && bash scripts/pre-commit-hooks/codeql-js-scan.sh` + +4. **Findings gate** + - `cd /projects/Charon && bash scripts/pre-commit-hooks/codeql-check-findings.sh`