diff --git a/internal/api/oidc_handlers.go b/internal/api/oidc_handlers.go index 6304259ca..632709870 100644 --- a/internal/api/oidc_handlers.go +++ b/internal/api/oidc_handlers.go @@ -304,7 +304,7 @@ func (r *Router) handleOIDCCallback(w http.ResponseWriter, req *http.Request) { LogAuditEventForTenant(GetOrgID(req.Context()), "oidc_login", username, GetClientIP(req), req.URL.Path, true, "OIDC login success") - target := entry.ReturnTo + target := sanitizeOIDCReturnTo(entry.ReturnTo) if target == "" { target = "/" } @@ -343,7 +343,14 @@ func sanitizeOIDCReturnTo(raw string) string { if trimmed == "" { return "" } - if !strings.HasPrefix(trimmed, "/") || strings.HasPrefix(trimmed, "//") { + if !strings.HasPrefix(trimmed, "/") { + return "" + } + if len(trimmed) > 1 && (trimmed[1] == '/' || trimmed[1] == '\\') { + return "" + } + parsed, err := url.Parse(trimmed) + if err != nil || parsed.IsAbs() || parsed.Host != "" { return "" } return trimmed diff --git a/internal/api/oidc_handlers_test.go b/internal/api/oidc_handlers_test.go index 150b58183..b1aab3715 100644 --- a/internal/api/oidc_handlers_test.go +++ b/internal/api/oidc_handlers_test.go @@ -98,6 +98,16 @@ func TestSanitizeOIDCReturnTo(t *testing.T) { raw: " /dashboard ", want: "/dashboard", }, + { + name: "invalid slash backslash", + raw: "/\\evil", + want: "", + }, + { + name: "invalid triple slash", + raw: "///evil", + want: "", + }, } for _, tc := range tests { diff --git a/internal/api/security.go b/internal/api/security.go index f74eaa2d9..3541f8850 100644 --- a/internal/api/security.go +++ b/internal/api/security.go @@ -72,7 +72,7 @@ func CheckCSRF(w http.ResponseWriter, r *http.Request) bool { Str("path", r.URL.Path). Str("session", safePrefixForLog(cookie.Value, 8)+"..."). Msg("Missing CSRF token") - clearCSRFCookie(w) + clearCSRFCookie(w, r) if newToken := issueNewCSRFCookie(w, r, cookie.Value); newToken != "" { w.Header().Set("X-CSRF-Token", newToken) log.Debug().Str("new_token", safePrefixForLog(newToken, 8)+"...").Msg("Issued new CSRF token after missing") @@ -87,7 +87,7 @@ func CheckCSRF(w http.ResponseWriter, r *http.Request) bool { Str("session", safePrefixForLog(cookie.Value, 8)+"..."). Str("provided_token", safePrefixForLog(csrfToken, 8)+"..."). Msg("Invalid CSRF token") - clearCSRFCookie(w) + clearCSRFCookie(w, r) if newToken := issueNewCSRFCookie(w, r, cookie.Value); newToken != "" { w.Header().Set("X-CSRF-Token", newToken) log.Debug().Str("new_token", safePrefixForLog(newToken, 8)+"...").Msg("Issued new CSRF token after invalid") @@ -102,16 +102,19 @@ func CheckCSRF(w http.ResponseWriter, r *http.Request) bool { return true } -func clearCSRFCookie(w http.ResponseWriter) { +func clearCSRFCookie(w http.ResponseWriter, r *http.Request) { if w == nil { return } + secure, sameSite := getCookieSettings(r) http.SetCookie(w, &http.Cookie{ Name: "pulse_csrf", Value: "", Path: "/", MaxAge: -1, HttpOnly: false, + Secure: secure, + SameSite: sameSite, }) } diff --git a/internal/api/security_test.go b/internal/api/security_test.go index 4650b3249..4ef313803 100644 --- a/internal/api/security_test.go +++ b/internal/api/security_test.go @@ -775,13 +775,14 @@ func TestGetSessionUsername(t *testing.T) { func TestClearCSRFCookie(t *testing.T) { t.Run("nil writer does not panic", func(t *testing.T) { - clearCSRFCookie(nil) + clearCSRFCookie(nil, nil) // Should not panic }) t.Run("sets cookie with maxage -1", func(t *testing.T) { w := httptest.NewRecorder() - clearCSRFCookie(w) + req := httptest.NewRequest("POST", "https://example.com/api/test", nil) + clearCSRFCookie(w, req) cookies := w.Result().Cookies() if len(cookies) != 1 { @@ -798,6 +799,9 @@ func TestClearCSRFCookie(t *testing.T) { if cookie.MaxAge != -1 { t.Errorf("cookie MaxAge = %d, want -1", cookie.MaxAge) } + if !cookie.Secure { + t.Errorf("cookie Secure = %v, want true for HTTPS requests", cookie.Secure) + } }) } diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index 04e14bcc5..09120d483 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -187,7 +187,7 @@ type NotificationManager struct { allowedPrivateMu sync.RWMutex // Protects allowedPrivateNets } -type appriseExecFunc func(ctx context.Context, path string, args []string) ([]byte, error) +type appriseExecFunc func(ctx context.Context, args []string) ([]byte, error) // copyEmailConfig returns a defensive copy of EmailConfig including its slices to avoid data races. func copyEmailConfig(cfg EmailConfig) EmailConfig { @@ -309,8 +309,8 @@ func NormalizeAppriseConfig(cfg AppriseConfig) AppriseConfig { return normalized } -func defaultAppriseExec(ctx context.Context, path string, args []string) ([]byte, error) { - cmd := exec.CommandContext(ctx, path, args...) +func defaultAppriseExec(ctx context.Context, args []string) ([]byte, error) { + cmd := exec.CommandContext(ctx, "apprise", args...) return cmd.CombinedOutput() } @@ -1491,7 +1491,7 @@ func (n *NotificationManager) sendAppriseViaCLI(cfg AppriseConfig, title, body s execFn = defaultAppriseExec } - output, err := execFn(ctx, cfg.CLIPath, args) + output, err := execFn(ctx, args) if err != nil { if len(output) > 0 { log.Debug(). diff --git a/internal/notifications/notifications_additional_test.go b/internal/notifications/notifications_additional_test.go index 22adb3c4d..88fce8550 100644 --- a/internal/notifications/notifications_additional_test.go +++ b/internal/notifications/notifications_additional_test.go @@ -18,11 +18,8 @@ func TestSendResolvedAppriseCLI(t *testing.T) { defer manager.Stop() var called bool - manager.appriseExec = func(ctx context.Context, path string, args []string) ([]byte, error) { + manager.appriseExec = func(ctx context.Context, args []string) ([]byte, error) { called = true - if path != "apprise" { - t.Fatalf("expected CLI path apprise, got %q", path) - } if len(args) == 0 || args[len(args)-1] != "target-1" { t.Fatalf("expected target to be passed, got %v", args) } diff --git a/internal/notifications/notifications_resolved_apprise_test.go b/internal/notifications/notifications_resolved_apprise_test.go index 986eacc69..fa8176a42 100644 --- a/internal/notifications/notifications_resolved_apprise_test.go +++ b/internal/notifications/notifications_resolved_apprise_test.go @@ -77,7 +77,7 @@ func TestSendResolvedApprise_CLI(t *testing.T) { nm := NewNotificationManager("") defer nm.Stop() - nm.appriseExec = func(ctx context.Context, path string, args []string) ([]byte, error) { + nm.appriseExec = func(ctx context.Context, args []string) ([]byte, error) { return nil, nil } diff --git a/internal/notifications/notifications_security_cli_test.go b/internal/notifications/notifications_security_cli_test.go index 5abf09093..b465187ba 100644 --- a/internal/notifications/notifications_security_cli_test.go +++ b/internal/notifications/notifications_security_cli_test.go @@ -3,6 +3,7 @@ package notifications import ( "context" "errors" + "os/exec" "testing" "time" ) @@ -24,7 +25,7 @@ func TestSendAppriseViaCLIExecError(t *testing.T) { nm := NewNotificationManager("") defer nm.Stop() - nm.appriseExec = func(ctx context.Context, path string, args []string) ([]byte, error) { + nm.appriseExec = func(ctx context.Context, args []string) ([]byte, error) { return []byte("boom"), errors.New("exec failed") } @@ -42,7 +43,7 @@ func TestSendAppriseViaCLISuccess(t *testing.T) { nm := NewNotificationManager("") defer nm.Stop() - nm.appriseExec = func(ctx context.Context, path string, args []string) ([]byte, error) { + nm.appriseExec = func(ctx context.Context, args []string) ([]byte, error) { return []byte("ok"), nil } @@ -60,7 +61,7 @@ func TestSendAppriseViaCLISuccessNoOutput(t *testing.T) { nm := NewNotificationManager("") defer nm.Stop() - nm.appriseExec = func(ctx context.Context, path string, args []string) ([]byte, error) { + nm.appriseExec = func(ctx context.Context, args []string) ([]byte, error) { return nil, nil } @@ -75,10 +76,14 @@ func TestSendAppriseViaCLISuccessNoOutput(t *testing.T) { } func TestDefaultAppriseExecRunsCommand(t *testing.T) { + if _, err := exec.LookPath("apprise"); err != nil { + t.Skip("apprise not installed") + } + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() - if _, err := defaultAppriseExec(ctx, "true", nil); err != nil { + if _, err := defaultAppriseExec(ctx, []string{"--help"}); err != nil { t.Fatalf("expected defaultAppriseExec to run, got %v", err) } } diff --git a/internal/notifications/notifications_test.go b/internal/notifications/notifications_test.go index 69237d5e8..def92a160 100644 --- a/internal/notifications/notifications_test.go +++ b/internal/notifications/notifications_test.go @@ -231,10 +231,7 @@ func TestSendGroupedAppriseInvokesExecutor(t *testing.T) { done := make(chan struct{}, 1) var capturedArgs []string - nm.appriseExec = func(ctx context.Context, path string, args []string) ([]byte, error) { - if path != "apprise" { - t.Fatalf("expected CLI path 'apprise', got %q", path) - } + nm.appriseExec = func(ctx context.Context, args []string) ([]byte, error) { capturedArgs = append([]string(nil), args...) select { case done <- struct{}{}: @@ -736,10 +733,7 @@ func TestSendTestNotificationApprise(t *testing.T) { var once sync.Once var capturedArgs []string - nm.appriseExec = func(ctx context.Context, path string, args []string) ([]byte, error) { - if path != "apprise" { - t.Fatalf("expected CLI path 'apprise', got %q", path) - } + nm.appriseExec = func(ctx context.Context, args []string) ([]byte, error) { capturedArgs = append([]string(nil), args...) // Use sync.Once to safely close channel even if callback is invoked multiple times once.Do(func() { close(done) }) @@ -794,10 +788,7 @@ func TestSendTestAppriseWithConfig(t *testing.T) { done := make(chan struct{}) var once sync.Once - var cliPath string - - nm.appriseExec = func(ctx context.Context, path string, args []string) ([]byte, error) { - cliPath = path + nm.appriseExec = func(ctx context.Context, args []string) ([]byte, error) { // Use sync.Once to safely close channel even if callback is invoked multiple times once.Do(func() { close(done) }) return []byte("ok"), nil @@ -818,9 +809,6 @@ func TestSendTestAppriseWithConfig(t *testing.T) { t.Fatalf("timeout waiting for Apprise test execution") } - if cliPath != "apprise" { - t.Fatalf("expected default CLI path 'apprise', got %q", cliPath) - } } func TestSendTestNotificationAppriseHTTP(t *testing.T) {