From 5a0f5aa68b3680f7ef3eb5e3ab01cb1ce1590dcc Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 1 Apr 2026 12:00:31 +0100 Subject: [PATCH] Harden SAML URL validation paths --- internal/api/identity_sso_handlers.go | 33 +++++++++++++++- internal/api/saml_service.go | 53 +++++++++++++++++++------- internal/api/saml_service_test.go | 30 +++++++++++++++ internal/api/sso_handlers_crud_test.go | 2 + internal/config/sso.go | 23 +++++++++-- internal/config/sso_test.go | 15 ++++++++ 6 files changed, 137 insertions(+), 19 deletions(-) diff --git a/internal/api/identity_sso_handlers.go b/internal/api/identity_sso_handlers.go index 8b833a3a8..b29860a43 100644 --- a/internal/api/identity_sso_handlers.go +++ b/internal/api/identity_sso_handlers.go @@ -68,12 +68,26 @@ func validateURL(urlStr string, allowedSchemes []string) bool { if err != nil { return false } + schemeAllowed := false for _, scheme := range allowedSchemes { if strings.EqualFold(parsed.Scheme, scheme) { - return true + schemeAllowed = true + break } } - return false + if !schemeAllowed { + return false + } + + switch strings.ToLower(parsed.Scheme) { + case "http", "https": + _, err := securityutil.NormalizeAbsoluteHTTPURL(urlStr) + return err == nil + case "data": + return parsed.Opaque != "" + default: + return false + } } // SSOProviderResponse represents an SSO provider for API responses @@ -313,6 +327,10 @@ func (r *Router) handleCreateSSOProvider(w http.ResponseWriter, req *http.Reques writeErrorResponse(w, http.StatusBadRequest, "validation_error", "Invalid SAML SSO URL", nil) return } + if provider.SAML.IDPSLOURL != "" && !validateURL(provider.SAML.IDPSLOURL, []string{"https", "http"}) { + writeErrorResponse(w, http.StatusBadRequest, "validation_error", "Invalid SAML SLO URL", nil) + return + } } // Security: Validate icon URL if provided @@ -431,6 +449,10 @@ func (r *Router) handleUpdateSSOProvider(w http.ResponseWriter, req *http.Reques writeErrorResponse(w, http.StatusBadRequest, "validation_error", "Invalid SAML SSO URL", nil) return } + if updated.SAML.IDPSLOURL != "" && !validateURL(updated.SAML.IDPSLOURL, []string{"https", "http"}) { + writeErrorResponse(w, http.StatusBadRequest, "validation_error", "Invalid SAML SLO URL", nil) + return + } } // Security: Validate icon URL if provided @@ -836,6 +858,13 @@ func (r *Router) testSAMLConnection(ctx context.Context, cfg *SAMLTestConfig) SS Error: "invalid_url", } } + if cfg.IDPSLOURL != "" && !validateURL(cfg.IDPSLOURL, []string{"https", "http"}) { + return SSOTestResponse{ + Success: false, + Message: "Invalid SLO URL format", + Error: "invalid_url", + } + } return SSOTestResponse{ Success: true, Message: "SSO URL is valid (manual configuration)", diff --git a/internal/api/saml_service.go b/internal/api/saml_service.go index ada5bcf7a..58cd27bfb 100644 --- a/internal/api/saml_service.go +++ b/internal/api/saml_service.go @@ -11,7 +11,6 @@ import ( "fmt" "io" "net/http" - "net/url" "os" "strings" "sync" @@ -171,7 +170,7 @@ func (s *SAMLService) buildManualMetadata() (*saml.EntityDescriptor, error) { return nil, errors.New("idp sso url is required for manual configuration") } - ssoURL, err := url.Parse(s.config.IDPSSOURL) + ssoURL, err := securityutil.NormalizeAbsoluteHTTPURL(s.config.IDPSSOURL) if err != nil { return nil, fmt.Errorf("invalid idp sso url: %w", err) } @@ -181,7 +180,7 @@ func (s *SAMLService) buildManualMetadata() (*saml.EntityDescriptor, error) { entityID = s.config.IDPIssuer } if entityID == "" { - entityID = s.config.IDPSSOURL + entityID = ssoURL.String() } metadata := &saml.EntityDescriptor{ @@ -209,14 +208,15 @@ func (s *SAMLService) buildManualMetadata() (*saml.EntityDescriptor, error) { // Add SLO endpoint if configured if s.config.IDPSLOURL != "" { - sloURL, err := url.Parse(s.config.IDPSLOURL) - if err == nil { - metadata.IDPSSODescriptors[0].SingleLogoutServices = []saml.Endpoint{ - { - Binding: saml.HTTPRedirectBinding, - Location: sloURL.String(), - }, - } + sloURL, err := securityutil.NormalizeAbsoluteHTTPURL(s.config.IDPSLOURL) + if err != nil { + return nil, fmt.Errorf("invalid idp slo url: %w", err) + } + metadata.IDPSSODescriptors[0].SingleLogoutServices = []saml.Endpoint{ + { + Binding: saml.HTTPRedirectBinding, + Location: sloURL.String(), + }, } } @@ -422,6 +422,10 @@ func (s *SAMLService) MakeAuthRequest(relayState string) (string, error) { if relayState == "" { relayState = "/" } + if len(s.idpMetadata.IDPSSODescriptors) == 0 || + len(s.idpMetadata.IDPSSODescriptors[0].SingleSignOnServices) == 0 { + return "", errors.New("idp does not support single sign-on") + } // Use the simple redirect method redirectURL, err := s.sp.MakeRedirectAuthenticationRequest(relayState) @@ -434,7 +438,11 @@ func (s *SAMLService) MakeAuthRequest(relayState string) (string, error) { Str("redirect_url", redirectURL.String()). Msg("Created SAML AuthnRequest") - return redirectURL.String(), nil + validatedURL, err := validateSAMLRedirectTarget(redirectURL.String(), s.idpMetadata.IDPSSODescriptors[0].SingleSignOnServices) + if err != nil { + return "", fmt.Errorf("failed to validate auth redirect: %w", err) + } + return validatedURL, nil } // ProcessResponse processes a SAML response and extracts user information @@ -570,7 +578,26 @@ func (s *SAMLService) MakeLogoutRequest(nameID, sessionIdx string) (string, erro // Build redirect URL redirectURL := req.Redirect("") - return redirectURL.String(), nil + return validateSAMLRedirectTarget(redirectURL.String(), s.idpMetadata.IDPSSODescriptors[0].SingleLogoutServices) +} + +func validateSAMLRedirectTarget(rawURL string, allowedEndpoints []saml.Endpoint) (string, error) { + validatedURL, err := securityutil.NormalizeAbsoluteHTTPURL(rawURL) + if err != nil { + return "", err + } + for _, endpoint := range allowedEndpoints { + endpointURL, err := securityutil.NormalizeAbsoluteHTTPURL(endpoint.Location) + if err != nil { + continue + } + if strings.EqualFold(validatedURL.Scheme, endpointURL.Scheme) && + strings.EqualFold(validatedURL.Host, endpointURL.Host) && + validatedURL.Path == endpointURL.Path { + return validatedURL.String(), nil + } + } + return "", fmt.Errorf("redirect target does not match configured SAML endpoint") } // RefreshMetadata reloads IdP metadata (useful for key rotation) diff --git a/internal/api/saml_service_test.go b/internal/api/saml_service_test.go index 99e1ba2d0..7de7a7570 100644 --- a/internal/api/saml_service_test.go +++ b/internal/api/saml_service_test.go @@ -14,6 +14,7 @@ import ( "testing" "time" + "github.com/crewjam/saml" "github.com/rcourtman/pulse-go-rewrite/internal/config" ) @@ -101,6 +102,17 @@ func TestBuildManualMetadataAndCertificate(t *testing.T) { if len(metadata.IDPSSODescriptors[0].KeyDescriptors) == 0 { t.Fatal("expected key descriptor with certificate") } + + cfg.IDPSSOURL = "https://user:pass@idp.example.com/sso" + if _, err := service.buildManualMetadata(); err == nil { + t.Fatal("expected error for idp sso URL with embedded credentials") + } + + cfg.IDPSSOURL = "https://idp.example.com/sso" + cfg.IDPSLOURL = "https://user:pass@idp.example.com/slo" + if _, err := service.buildManualMetadata(); err == nil { + t.Fatal("expected error for idp slo URL with embedded credentials") + } } func TestLoadSPCredentials(t *testing.T) { @@ -192,6 +204,24 @@ func TestSAMLServiceBasicFlows(t *testing.T) { } } +func TestValidateSAMLRedirectTarget(t *testing.T) { + allowed := []saml.Endpoint{ + {Location: "https://idp.example.com/sso"}, + } + + got, err := validateSAMLRedirectTarget("https://idp.example.com/sso?SAMLRequest=test", allowed) + if err != nil { + t.Fatalf("validate redirect: %v", err) + } + if !strings.Contains(got, "SAMLRequest=test") { + t.Fatalf("unexpected validated redirect: %s", got) + } + + if _, err := validateSAMLRedirectTarget("https://evil.example.com/sso?SAMLRequest=test", allowed); err == nil { + t.Fatal("expected error for unexpected redirect target") + } +} + func TestFetchMetadataFromURL(t *testing.T) { server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) diff --git a/internal/api/sso_handlers_crud_test.go b/internal/api/sso_handlers_crud_test.go index f4e389f63..156fb52c2 100644 --- a/internal/api/sso_handlers_crud_test.go +++ b/internal/api/sso_handlers_crud_test.go @@ -41,7 +41,9 @@ func TestValidateURL(t *testing.T) { }{ {"https://example.com", []string{"https"}, true}, {"http://example.com", []string{"https"}, false}, + {"https://user:pass@example.com", []string{"https"}, false}, {"ftp://example.com", []string{"http", "https"}, false}, + {"data:image/png;base64,abc", []string{"data"}, true}, {"not-a-url", []string{"https"}, false}, {"", []string{"https"}, false}, } diff --git a/internal/config/sso.go b/internal/config/sso.go index f5e271073..d855ac0fc 100644 --- a/internal/config/sso.go +++ b/internal/config/sso.go @@ -4,6 +4,8 @@ import ( "fmt" "net/url" "strings" + + "github.com/rcourtman/pulse-go-rewrite/internal/securityutil" ) // SSOProviderType defines the type of SSO provider @@ -298,14 +300,20 @@ func validateSAMLProvider(cfg *SAMLProviderConfig) error { } if cfg.IDPMetadataURL != "" { - if _, err := url.ParseRequestURI(cfg.IDPMetadataURL); err != nil { - return fmt.Errorf("invalid idp metadata url: %w", err) + if err := validateAbsoluteHTTPURL("idp metadata url", cfg.IDPMetadataURL); err != nil { + return err } } if cfg.IDPSSOURL != "" { - if _, err := url.ParseRequestURI(cfg.IDPSSOURL); err != nil { - return fmt.Errorf("invalid idp sso url: %w", err) + if err := validateAbsoluteHTTPURL("idp sso url", cfg.IDPSSOURL); err != nil { + return err + } + } + + if cfg.IDPSLOURL != "" { + if err := validateAbsoluteHTTPURL("idp slo url", cfg.IDPSLOURL); err != nil { + return err } } @@ -321,6 +329,13 @@ func validateSAMLProvider(cfg *SAMLProviderConfig) error { return nil } +func validateAbsoluteHTTPURL(fieldName, raw string) error { + if _, err := securityutil.NormalizeAbsoluteHTTPURL(raw); err != nil { + return fmt.Errorf("invalid %s: %w", fieldName, err) + } + return nil +} + // Clone creates a deep copy of the SSO configuration func (c *SSOConfig) Clone() *SSOConfig { if c == nil { diff --git a/internal/config/sso_test.go b/internal/config/sso_test.go index 1e66f47da..93500d264 100644 --- a/internal/config/sso_test.go +++ b/internal/config/sso_test.go @@ -710,6 +710,21 @@ func TestValidateSAMLProvider(t *testing.T) { }, wantErr: true, }, + { + name: "rejects SSO URL with embedded credentials", + cfg: &SAMLProviderConfig{ + IDPSSOURL: "https://user:pass@idp.example.com/sso", + }, + wantErr: true, + }, + { + name: "rejects invalid SLO URL", + cfg: &SAMLProviderConfig{ + IDPSSOURL: "https://idp.example.com/sso", + IDPSLOURL: "https://user:pass@idp.example.com/slo", + }, + wantErr: true, + }, { name: "signing enabled without cert", cfg: &SAMLProviderConfig{