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

@@ -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

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)
 	}
 }
 

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

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