From 22be89b2ff86465abb90dab0344ef9366ab181b3 Mon Sep 17 00:00:00 2001 From: Noste <83548733+Noooste@users.noreply.github.com> Date: Mon, 8 Jun 2026 00:11:45 +0200 Subject: [PATCH] fix(backend): prevent OIDC login loop from empty cookie name (#76) Signed-off-by: Noooste <83548733+Noooste@users.noreply.github.com> --- backend/internal/auth/auth.go | 15 +++- backend/internal/auth/auth_test.go | 26 ++++++- backend/internal/config/config.go | 4 ++ backend/internal/config/config_test.go | 97 ++++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 4 deletions(-) diff --git a/backend/internal/auth/auth.go b/backend/internal/auth/auth.go index 787cb6a..c5bc6b9 100644 --- a/backend/internal/auth/auth.go +++ b/backend/internal/auth/auth.go @@ -322,8 +322,21 @@ func extractRoles(claims map[string]interface{}, path string) []string { return nil } -// extractStringArray converts an interface{} to []string if possible +// extractStringArray converts an interface{} to []string if possible. +// +// A scalar string is treated as a single-element list: IdPs commonly emit a +// single role as a bare string (e.g. "garage_role": "garage-ui-admin") rather +// than a one-element array, and discarding it would make admin_role checks +// fail with a spurious 403, see https://github.com/Noooste/garage-ui/issues/75 func extractStringArray(value interface{}) []string { + // Try a scalar string (single role emitted as a bare value) + if str, ok := value.(string); ok { + if str == "" { + return nil + } + return []string{str} + } + // Try direct string array if strArray, ok := value.([]string); ok { return strArray diff --git a/backend/internal/auth/auth_test.go b/backend/internal/auth/auth_test.go index ec891bc..944d57b 100644 --- a/backend/internal/auth/auth_test.go +++ b/backend/internal/auth/auth_test.go @@ -564,8 +564,11 @@ func TestExtractRolesFromAccessToken_IntermediateNodeNotMap(t *testing.T) { } } -func TestExtractRolesFromAccessToken_FinalValueWrongType(t *testing.T) { - // Final value is a plain string, not an array — extractStringArray returns nil. +func TestExtractRolesFromAccessToken_ScalarStringRoleReturnsSingleElement(t *testing.T) { + // A role_attribute_path that resolves to a scalar string (common when an IdP + // emits a single role, e.g. "garage_role": "garage-ui-admin") must be treated + // as a one-element role list, not silently discarded. Discarding it caused + // admin_role (singular) + scalar claim to yield roles=[] and a spurious 403. tok := makeAccessToken(t, map[string]any{ "roles": "admin", }) @@ -574,8 +577,25 @@ func TestExtractRolesFromAccessToken_FinalValueWrongType(t *testing.T) { OIDC: config.OIDCConfig{RoleAttributePath: "roles"}, }, } + got := svc.ExtractRolesFromAccessToken(tok) + if len(got) != 1 || got[0] != "admin" { + t.Errorf("got %v, want [admin]", got) + } +} + +func TestExtractRolesFromAccessToken_EmptyScalarStringReturnsNil(t *testing.T) { + // An empty scalar must not produce a [""] role, which would never match a + // configured admin role and only muddies logs. + tok := makeAccessToken(t, map[string]any{ + "roles": "", + }) + svc := &Service{ + authConfig: &config.AuthConfig{ + OIDC: config.OIDCConfig{RoleAttributePath: "roles"}, + }, + } if got := svc.ExtractRolesFromAccessToken(tok); got != nil { - t.Errorf("expected nil for non-array roles, got %v", got) + t.Errorf("expected nil for empty scalar role, got %v", got) } } diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index b5a1350..8c1ec0f 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -168,6 +168,10 @@ func Load(configPath string, opts ...LoadOption) (*Config, error) { viper.SetDefault("garage.force_path_style", true) viper.SetDefault("logging.level", "info") viper.SetDefault("logging.format", "text") + viper.SetDefault("auth.oidc.cookie_name", "garage_session") + viper.SetDefault("auth.oidc.cookie_http_only", true) + viper.SetDefault("auth.oidc.cookie_same_site", "lax") + viper.SetDefault("auth.oidc.session_max_age", 86400) // If garage.toml path is provided, parse it and set values as viper // defaults. Defaults sit below config-file and env-var values in viper's diff --git a/backend/internal/config/config_test.go b/backend/internal/config/config_test.go index c9a4118..e6e7ae3 100644 --- a/backend/internal/config/config_test.go +++ b/backend/internal/config/config_test.go @@ -490,6 +490,103 @@ func TestLoad_EnvOverridesToml(t *testing.T) { } } +// oidcValidYAML is a minimal configuration that enables OIDC and passes +// Validate, but deliberately omits auth.oidc.cookie_name. +const oidcValidYAML = ` +server: + host: "0.0.0.0" + port: 8080 + root_url: "https://garage.example.com" +garage: + endpoint: http://garage:3900 + admin_endpoint: http://garage:3903 + admin_token: supersecret +auth: + oidc: + enabled: true + client_id: "garage-ui" + issuer_url: "https://idp.example.com/realms/main" + scopes: + - openid + admin_roles: + - "garage-ui-admin" +` + +func TestLoad_OIDCCookieNameDefaultsWhenUnset(t *testing.T) { + resetViper(t) + path := writeConfigFile(t, oidcValidYAML) + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load: %v", err) + } + // An empty cookie name makes Fiber silently drop the session Set-Cookie + // (net/http rejects empty cookie names), which manifests as an OIDC login + // loop. A non-empty default prevents that footgun. + if cfg.Auth.OIDC.CookieName != "garage_session" { + t.Errorf("CookieName = %q, want garage_session (default)", cfg.Auth.OIDC.CookieName) + } +} + +func TestLoad_OIDCCookieNameExplicitValueWins(t *testing.T) { + resetViper(t) + path := writeConfigFile(t, oidcValidYAML+" cookie_name: \"custom_session\"\n") + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.Auth.OIDC.CookieName != "custom_session" { + t.Errorf("CookieName = %q, want custom_session (explicit override)", cfg.Auth.OIDC.CookieName) + } +} + +func TestLoad_OIDCCookieDefaultsWhenUnset(t *testing.T) { + resetViper(t) + path := writeConfigFile(t, oidcValidYAML) + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load: %v", err) + } + // HTTPOnly must default to true: a session cookie readable from JavaScript + // is an XSS token-theft risk. + if !cfg.Auth.OIDC.CookieHTTPOnly { + t.Errorf("CookieHTTPOnly = false, want true (default)") + } + // SessionMaxAge must default to a positive value so the cookie's MaxAge + // agrees with the 24h JWT instead of becoming a session-only cookie. + if cfg.Auth.OIDC.SessionMaxAge != 86400 { + t.Errorf("SessionMaxAge = %d, want 86400 (default)", cfg.Auth.OIDC.SessionMaxAge) + } + if cfg.Auth.OIDC.CookieSameSite != "lax" { + t.Errorf("CookieSameSite = %q, want lax (default)", cfg.Auth.OIDC.CookieSameSite) + } +} + +func TestLoad_OIDCCookieDefaultsCanBeOverridden(t *testing.T) { + resetViper(t) + yaml := oidcValidYAML + + " cookie_http_only: false\n" + + " session_max_age: 3600\n" + + " cookie_same_site: \"strict\"\n" + path := writeConfigFile(t, yaml) + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.Auth.OIDC.CookieHTTPOnly { + t.Errorf("CookieHTTPOnly = true, want false (explicit override)") + } + if cfg.Auth.OIDC.SessionMaxAge != 3600 { + t.Errorf("SessionMaxAge = %d, want 3600 (explicit override)", cfg.Auth.OIDC.SessionMaxAge) + } + if cfg.Auth.OIDC.CookieSameSite != "strict" { + t.Errorf("CookieSameSite = %q, want strict (explicit override)", cfg.Auth.OIDC.CookieSameSite) + } +} + func TestEffectiveAdminRoles(t *testing.T) { tests := []struct { name string