fix: enforce secure cookie settings and enhance URL validation in HTTP wrapper
This commit is contained in:
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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`
|
||||
|
||||
Reference in New Issue
Block a user