From 02e8e9b94d67eacaa25720345f3f3f45f5961667 Mon Sep 17 00:00:00 2001 From: Tanvir Ahmed Date: Mon, 14 Sep 2026 00:48:24 +0000 Subject: [PATCH] feat: Add configurable cookie Domain for cross-subdomain sessions Cookies were always host-only, so a session issued by api.example.com was never sent to app.example.com. Add `session.domain` (and the `AUTHULA_COOKIE_DOMAIN` env var) and apply it to the session, CSRF, TOTP and impersonation cookies on both set and clear. The value is validated against BaseURL at startup since net/http silently drops invalid domains. Also fix CSRF and admin cookie clears that did not mirror the attributes they were set with, and consolidate SameSite parsing in util.ParseSameSite. --- .env.example | 2 + config/options.go | 53 ++++++ config/options_test.go | 179 ++++++++++++++++++ env/env_config.go | 1 + models/config.go | 7 +- .../admin/handlers/impersonation_handlers.go | 20 +- .../handlers/impersonation_handlers_test.go | 45 +++-- plugins/admin/hooks.go | 17 +- plugins/admin/hooks_test.go | 74 ++++++++ plugins/csrf/hooks.go | 9 +- plugins/csrf/plugin.go | 53 +++--- plugins/csrf/plugin_test.go | 97 ++++++++++ plugins/session/plugin.go | 19 +- plugins/session/plugin_test.go | 34 ++++ plugins/totp/handlers/enable_handler.go | 4 +- .../handlers/verify_backup_code_handler.go | 7 +- .../verify_backup_code_handler_test.go | 8 +- plugins/totp/handlers/verify_totp_handler.go | 7 +- .../totp/handlers/verify_totp_handler_test.go | 12 +- plugins/totp/hooks.go | 4 +- plugins/totp/routes.go | 2 + plugins/totp/types/types.go | 15 -- util/cookie.go | 19 ++ util/cookie_test.go | 33 ++++ 24 files changed, 618 insertions(+), 103 deletions(-) create mode 100644 plugins/admin/hooks_test.go create mode 100644 util/cookie.go create mode 100644 util/cookie_test.go diff --git a/.env.example b/.env.example index 45dfe0de..f66de173 100644 --- a/.env.example +++ b/.env.example @@ -32,6 +32,8 @@ AUTHULA_BASE_URL= AUTHULA_SECRET= # postgresql: "postgresql://username:password@localhost:5432/authula?sslmode=disable" AUTHULA_DATABASE_URL= +# Cookie Domain attribute shared by all auth cookies (e.g. example.com). Leave empty for host-only cookies. +AUTHULA_COOKIE_DOMAIN= # Email template overrides (optional) - Override default email templates via environment variables # Each email type has three templates: SUBJECT, TEXT (plain text), HTML diff --git a/config/options.go b/config/options.go index a6d3b26b..1b7ce6f3 100644 --- a/config/options.go +++ b/config/options.go @@ -2,8 +2,11 @@ package config import ( "fmt" + "net" "net/http" + "net/url" "os" + "strings" "time" "github.com/Authula/authula/env" @@ -91,6 +94,16 @@ func NewConfig(options ...ConfigOption) *models.Config { panic(fmt.Errorf("BasePath must start with '/', got: %q", config.BasePath)) } + // Validate cookie domain. + if envValue := os.Getenv(env.EnvCookieDomain); envValue != "" { + config.Session.Domain = envValue + } + domain, err := normalizeCookieDomain(config.Session.Domain, config.BaseURL) + if err != nil { + panic(fmt.Errorf("invalid session cookie domain: %w", err)) + } + config.Session.Domain = domain + // Validate event bus configuration if err := validateEventBusConfig(&config.EventBus); err != nil { panic(fmt.Errorf("invalid event bus configuration: %w", err)) @@ -178,6 +191,11 @@ func WithSession(config models.SessionConfig) ConfigOption { if config.CookieName != "" { c.Session.CookieName = config.CookieName } + if envValue := os.Getenv(env.EnvCookieDomain); envValue != "" { + c.Session.Domain = envValue + } else if config.Domain != "" { + c.Session.Domain = config.Domain + } if config.ExpiresIn != 0 { c.Session.ExpiresIn = config.ExpiresIn } @@ -308,6 +326,41 @@ func WithCoreServiceHooks(config *models.CoreServiceHooksConfig) ConfigOption { } } +// normalizeCookieDomain strips a leading dot from the cookie domain and checks that a browser +// would accept it for cookies set by baseURL: the domain must be a bare hostname (no scheme, +// port or path) and the BaseURL host must be that domain or one of its subdomains. Go's +// net/http silently drops an invalid Domain attribute, so this fails at startup instead. +func normalizeCookieDomain(domain, baseURL string) (string, error) { + if domain == "" { + return "", nil + } + + domain = strings.ToLower(strings.TrimPrefix(domain, ".")) + if domain == "" { + return "", fmt.Errorf("domain must not be empty after trimming the leading dot") + } + if strings.ContainsAny(domain, ":/?#@ \t") { + return "", fmt.Errorf("domain %q must be a bare hostname without scheme, port or path", domain) + } + + // Browsers reject cookies with explicit Domain attributes set to IP addresses. + // For IP-based local dev (e.g. 127.0.0.1), domain must remain empty (host-only). + if ip := net.ParseIP(domain); ip != nil { + return "", fmt.Errorf("domain %q cannot be an IP address; browsers require IP cookies to be host-only (leave domain empty)", domain) + } + + parsedBaseURL, err := url.Parse(baseURL) + if err != nil { + return "", fmt.Errorf("could not parse BaseURL %q: %w", baseURL, err) + } + host := strings.ToLower(parsedBaseURL.Hostname()) + if host != domain && !strings.HasSuffix(host, "."+domain) { + return "", fmt.Errorf("BaseURL host %q is not %q or one of its subdomains; browsers would reject the cookie", host, domain) + } + + return domain, nil +} + // validateEventBusConfig validates that the event bus provider has the correct configuration func validateEventBusConfig(config *models.EventBusConfig) error { provider := config.Provider diff --git a/config/options_test.go b/config/options_test.go index 41e75a85..2164186d 100644 --- a/config/options_test.go +++ b/config/options_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + "github.com/Authula/authula/env" "github.com/Authula/authula/models" ) @@ -68,3 +69,181 @@ func TestWithSessionCookieMaxAge(t *testing.T) { }) } } + +func TestWithSessionCookieDomain(t *testing.T) { + tests := []struct { + name string + baseURL string + domain string + wantDomain string + wantPanic bool + }{ + { + name: "empty domain keeps cookies host-only", + baseURL: "http://localhost:8080", + domain: "", + wantDomain: "", + }, + { + name: "parent domain of the base url host is accepted", + baseURL: "https://api.example.com", + domain: "example.com", + wantDomain: "example.com", + }, + { + name: "domain equal to the base url host is accepted", + baseURL: "https://example.com", + domain: "example.com", + wantDomain: "example.com", + }, + { + name: "leading dot is stripped", + baseURL: "https://api.example.com", + domain: ".example.com", + wantDomain: "example.com", + }, + { + name: "domain is lower-cased", + baseURL: "https://api.example.com:8443", + domain: "Example.COM", + wantDomain: "example.com", + }, + { + name: "base url host outside the domain panics", + baseURL: "http://localhost:8080", + domain: "example.com", + wantPanic: true, + }, + { + name: "look-alike suffix without a dot boundary panics", + baseURL: "https://notexample.com", + domain: "example.com", + wantPanic: true, + }, + { + name: "domain with a scheme panics", + baseURL: "https://api.example.com", + domain: "https://example.com", + wantPanic: true, + }, + { + name: "domain with a port panics", + baseURL: "https://api.example.com", + domain: "example.com:8080", + wantPanic: true, + }, + { + name: "bare dot panics", + baseURL: "https://api.example.com", + domain: ".", + wantPanic: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + defer func() { + recovered := recover() + if tt.wantPanic && recovered == nil { + t.Fatal("expected NewConfig to panic") + } + if !tt.wantPanic && recovered != nil { + t.Fatalf("expected NewConfig not to panic, got %v", recovered) + } + }() + + config := NewConfig( + WithBaseURL(tt.baseURL), + WithSession(models.SessionConfig{Domain: tt.domain}), + ) + + if config.Session.Domain != tt.wantDomain { + t.Fatalf("expected cookie domain %q, got %q", tt.wantDomain, config.Session.Domain) + } + }) + } +} + +// Uses t.Setenv, so it must not run in parallel with the other config tests. +func TestWithSessionCookieDomainFromEnv(t *testing.T) { + tests := []struct { + name string + envDomain string + configDomain string + baseURL string + wantDomain string + wantPanic bool + }{ + { + name: "env var sets the domain when config leaves it empty", + envDomain: "example.com", + baseURL: "https://api.example.com", + wantDomain: "example.com", + }, + { + name: "env var overrides the domain from config", + envDomain: "example.com", + configDomain: "other.com", + baseURL: "https://api.example.com", + wantDomain: "example.com", + }, + { + name: "config domain is used when env var is unset", + envDomain: "", + configDomain: "example.com", + baseURL: "https://api.example.com", + wantDomain: "example.com", + }, + { + name: "env var is normalized like config", + envDomain: ".Example.COM", + baseURL: "https://api.example.com", + wantDomain: "example.com", + }, + { + name: "env var is validated against base url", + envDomain: "example.com", + baseURL: "http://localhost:8080", + wantPanic: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv(env.EnvCookieDomain, tt.envDomain) + + defer func() { + recovered := recover() + if tt.wantPanic && recovered == nil { + t.Fatal("expected NewConfig to panic") + } + if !tt.wantPanic && recovered != nil { + t.Fatalf("expected NewConfig not to panic, got %v", recovered) + } + }() + + config := NewConfig( + WithBaseURL(tt.baseURL), + WithSession(models.SessionConfig{Domain: tt.configDomain}), + ) + + if config.Session.Domain != tt.wantDomain { + t.Fatalf("expected cookie domain %q, got %q", tt.wantDomain, config.Session.Domain) + } + }) + } +} + +// Standalone mode builds the config without WithSession, so the env var must still apply. +func TestNewConfigCookieDomainFromEnvWithoutSessionOption(t *testing.T) { + t.Setenv(env.EnvCookieDomain, "example.com") + t.Setenv(env.EnvBaseURL, "https://api.example.com") + + config := NewConfig() + + if config.Session.Domain != "example.com" { + t.Fatalf("expected cookie domain %q, got %q", "example.com", config.Session.Domain) + } +} diff --git a/env/env_config.go b/env/env_config.go index 75e80516..abf092f3 100644 --- a/env/env_config.go +++ b/env/env_config.go @@ -51,6 +51,7 @@ const ( EnvBaseURL = "AUTHULA_BASE_URL" EnvSecret = "AUTHULA_SECRET" EnvDatabaseURL = "AUTHULA_DATABASE_URL" + EnvCookieDomain = "AUTHULA_COOKIE_DOMAIN" // ENVIRONMENT diff --git a/models/config.go b/models/config.go index 40a4294b..7e093c96 100644 --- a/models/config.go +++ b/models/config.go @@ -48,7 +48,12 @@ type LoggerConfig struct { } type SessionConfig struct { - CookieName string `json:"cookie_name" toml:"cookie_name"` + CookieName string `json:"cookie_name" toml:"cookie_name"` + // Domain is the Domain attribute applied to every auth cookie Authula issues (session, CSRF, + // TOTP, impersonation). Empty means host-only: the cookie is bound to the exact host that set + // it. Set it to the parent domain (e.g. "example.com") to share cookies between subdomains such + // as api.example.com and app.example.com. BaseURL must be that domain or one of its subdomains. + Domain string `json:"domain" toml:"domain"` ExpiresIn time.Duration `json:"expires_in" toml:"expires_in"` // Sliding window per activity UpdateAge time.Duration `json:"update_age" toml:"update_age"` // How often to check/update CookieMaxAge time.Duration `json:"cookie_max_age" toml:"cookie_max_age"` // Absolute max age of the cookie diff --git a/plugins/admin/handlers/impersonation_handlers.go b/plugins/admin/handlers/impersonation_handlers.go index 6710a991..0ad41910 100644 --- a/plugins/admin/handlers/impersonation_handlers.go +++ b/plugins/admin/handlers/impersonation_handlers.go @@ -123,13 +123,14 @@ func (h *StartImpersonationHandler) Handler() http.HandlerFunc { } sessionConfig := h.globalConfig.Session - sameSite := sameSiteFromSessionConfig(&sessionConfig) + sameSite := util.ParseSameSite(sessionConfig.SameSite) if result.OriginalCookieToken != "" { http.SetCookie(w, &http.Cookie{ Name: sessionConfig.CookieName + constants.OriginalSessionCookieSuffix, Value: result.OriginalCookieToken, Path: "/", + Domain: sessionConfig.Domain, HttpOnly: sessionConfig.HttpOnly, Secure: sessionConfig.Secure, SameSite: sameSite, @@ -217,12 +218,13 @@ func (h *StopImpersonationHandler) Handler() http.HandlerFunc { } sessionConfig := h.globalConfig.Session - sameSite := sameSiteFromSessionConfig(&sessionConfig) + sameSite := util.ParseSameSite(sessionConfig.SameSite) http.SetCookie(w, &http.Cookie{ Name: sessionConfig.CookieName, Value: result.OriginalSessionToken, Path: "/", + Domain: sessionConfig.Domain, HttpOnly: sessionConfig.HttpOnly, Secure: sessionConfig.Secure, SameSite: sameSite, @@ -233,6 +235,7 @@ func (h *StopImpersonationHandler) Handler() http.HandlerFunc { Name: sessionConfig.CookieName + constants.OriginalSessionCookieSuffix, Value: "", Path: "/", + Domain: sessionConfig.Domain, HttpOnly: sessionConfig.HttpOnly, Secure: sessionConfig.Secure, SameSite: sameSite, @@ -270,16 +273,3 @@ func respondImpersonationError(reqCtx *models.RequestContext, err error) { func mapImpersonationErrorStatus(err error) int { return mapAdminHttpErrorStatus(err) } - -func sameSiteFromSessionConfig(sessionConfig *models.SessionConfig) http.SameSite { - switch sessionConfig.SameSite { - case "strict": - return http.SameSiteStrictMode - case "none": - return http.SameSiteNoneMode - case "lax": - return http.SameSiteLaxMode - default: - return http.SameSiteLaxMode - } -} diff --git a/plugins/admin/handlers/impersonation_handlers_test.go b/plugins/admin/handlers/impersonation_handlers_test.go index 005ff39b..7d9ee4b8 100644 --- a/plugins/admin/handlers/impersonation_handlers_test.go +++ b/plugins/admin/handlers/impersonation_handlers_test.go @@ -21,6 +21,7 @@ func testGlobalConfig() *models.Config { return &models.Config{ Session: models.SessionConfig{ CookieName: "authula.session_token", + Domain: "example.com", CookieMaxAge: time.Hour, HttpOnly: true, Secure: false, @@ -354,18 +355,40 @@ func TestStopImpersonationHandler(t *testing.T) { func assertRestoredSessionCookie(t *testing.T, cookies []*http.Cookie, wantToken string) { t.Helper() - cookieName := testGlobalConfig().Session.CookieName + sessionConfig := testGlobalConfig().Session + cookieName := sessionConfig.CookieName + originalCookieName := cookieName + adminconstants.OriginalSessionCookieSuffix + + var restored, cleared *http.Cookie for _, c := range cookies { - if c.Name != cookieName { - continue - } - if c.Value != wantToken { - t.Fatalf("expected restored cookie value %q, got %q", wantToken, c.Value) - } - if c.MaxAge != int(time.Hour.Seconds()) { - t.Fatalf("expected restored cookie max age %d, got %d", int(time.Hour.Seconds()), c.MaxAge) + switch c.Name { + case cookieName: + restored = c + case originalCookieName: + cleared = c } - return } - t.Fatalf("expected restored session cookie %q to be set", cookieName) + + if restored == nil { + t.Fatalf("expected restored session cookie %q to be set", cookieName) + } + if restored.Value != wantToken { + t.Fatalf("expected restored cookie value %q, got %q", wantToken, restored.Value) + } + if restored.MaxAge != int(time.Hour.Seconds()) { + t.Fatalf("expected restored cookie max age %d, got %d", int(time.Hour.Seconds()), restored.MaxAge) + } + if restored.Domain != sessionConfig.Domain { + t.Fatalf("expected restored cookie domain %q, got %q", sessionConfig.Domain, restored.Domain) + } + + if cleared == nil { + t.Fatalf("expected original session cookie %q to be cleared", originalCookieName) + } + if cleared.MaxAge != -1 || cleared.Value != "" { + t.Fatalf("expected original cookie to be expired, got value %q max age %d", cleared.Value, cleared.MaxAge) + } + if cleared.Domain != sessionConfig.Domain { + t.Fatalf("expected cleared original cookie domain %q, got %q", sessionConfig.Domain, cleared.Domain) + } } diff --git a/plugins/admin/hooks.go b/plugins/admin/hooks.go index 9820be95..9e2681a1 100644 --- a/plugins/admin/hooks.go +++ b/plugins/admin/hooks.go @@ -6,6 +6,7 @@ import ( "github.com/Authula/authula/models" adminconstants "github.com/Authula/authula/plugins/admin/constants" + "github.com/Authula/authula/util" ) func (p *AdminPlugin) Hooks() []models.Hook { @@ -162,11 +163,19 @@ func (p *AdminPlugin) addImpersonationWhitelistScopes(reqCtx *models.RequestCont return nil } +// clearOriginalCookie expires the stashed original-session cookie. It mirrors the attributes the +// cookie was issued with (see StartImpersonationHandler); a clear with a different Domain, Path +// or Secure flag is ignored by the browser and the cookie would linger. func (p *AdminPlugin) clearOriginalCookie(w http.ResponseWriter, cookieName string) { + sessionConfig := p.pluginCtx.GetConfig().Session http.SetCookie(w, &http.Cookie{ - Name: cookieName + adminconstants.OriginalSessionCookieSuffix, - Value: "", - Path: "/", - MaxAge: -1, + Name: cookieName + adminconstants.OriginalSessionCookieSuffix, + Value: "", + Path: "/", + Domain: sessionConfig.Domain, + HttpOnly: sessionConfig.HttpOnly, + Secure: sessionConfig.Secure, + SameSite: util.ParseSameSite(sessionConfig.SameSite), + MaxAge: -1, }) } diff --git a/plugins/admin/hooks_test.go b/plugins/admin/hooks_test.go new file mode 100644 index 00000000..db22421e --- /dev/null +++ b/plugins/admin/hooks_test.go @@ -0,0 +1,74 @@ +package admin + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/Authula/authula/models" + adminconstants "github.com/Authula/authula/plugins/admin/constants" + "github.com/Authula/authula/plugins/admin/types" +) + +// The browser only deletes a cookie when the clear carries the same scoping attributes it was set with. +func TestAdminPlugin_ClearOriginalCookieMirrorsSessionCookieAttributes(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + session models.SessionConfig + wantSameSite http.SameSite + }{ + { + name: "host-only lax cookie", + session: models.SessionConfig{CookieName: "authula.session_token", HttpOnly: true, SameSite: "lax"}, + wantSameSite: http.SameSiteLaxMode, + }, + { + name: "domain-scoped secure strict cookie", + session: models.SessionConfig{CookieName: "authula.session_token", Domain: "example.com", HttpOnly: true, Secure: true, SameSite: "strict"}, + wantSameSite: http.SameSiteStrictMode, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + globalConfig := &models.Config{Session: tt.session} + plugin := New(types.AdminPluginConfig{}) + plugin.pluginCtx = &models.PluginContext{GetConfig: func() *models.Config { return globalConfig }} + + w := httptest.NewRecorder() + plugin.clearOriginalCookie(w, tt.session.CookieName) + + cookies := w.Result().Cookies() + if len(cookies) != 1 { + t.Fatalf("expected 1 cookie, got %d", len(cookies)) + } + c := cookies[0] + + if c.Name != tt.session.CookieName+adminconstants.OriginalSessionCookieSuffix { + t.Errorf("Name = %q", c.Name) + } + if c.Value != "" || c.MaxAge != -1 { + t.Errorf("expected expired cookie, got Value=%q MaxAge=%d", c.Value, c.MaxAge) + } + if c.Path != "/" { + t.Errorf("Path = %q, want /", c.Path) + } + if c.Domain != tt.session.Domain { + t.Errorf("Domain = %q, want %q", c.Domain, tt.session.Domain) + } + if c.HttpOnly != tt.session.HttpOnly { + t.Errorf("HttpOnly = %v, want %v", c.HttpOnly, tt.session.HttpOnly) + } + if c.Secure != tt.session.Secure { + t.Errorf("Secure = %v, want %v", c.Secure, tt.session.Secure) + } + if c.SameSite != tt.wantSameSite { + t.Errorf("SameSite = %v, want %v", c.SameSite, tt.wantSameSite) + } + }) + } +} diff --git a/plugins/csrf/hooks.go b/plugins/csrf/hooks.go index 6362e0aa..602325eb 100644 --- a/plugins/csrf/hooks.go +++ b/plugins/csrf/hooks.go @@ -113,13 +113,6 @@ func (p *CSRFPlugin) validateCSRFTokenHook(reqCtx *models.RequestContext) error } func (p *CSRFPlugin) clearCSRFTokenHook(reqCtx *models.RequestContext) error { - http.SetCookie(reqCtx.ResponseWriter, &http.Cookie{ - Name: p.pluginConfig.CookieName, - Value: "", - Path: "/", - HttpOnly: false, - Secure: p.pluginConfig.Secure, - MaxAge: -1, - }) + p.clearCSRFCookie(reqCtx) return nil } diff --git a/plugins/csrf/plugin.go b/plugins/csrf/plugin.go index 600294c6..db8b09a9 100644 --- a/plugins/csrf/plugin.go +++ b/plugins/csrf/plugin.go @@ -221,38 +221,39 @@ func (p *CSRFPlugin) Middleware() func(http.Handler) http.Handler { } } -// setCSRFCookie sets the CSRF token cookie with hardcoded security settings. -// Both HttpOnly and Secure are hardcoded to ensure the Double-Submit Cookie pattern works correctly: -// - HttpOnly=false: Allows JavaScript to read the cookie value -// - Secure: Set to true only for HTTPS requests (allows development over HTTP on localhost) -// Also sets the token in a response header so the client can read and use it. +// setCSRFCookie issues the CSRF token cookie and echoes the token in the response header +// so the client can read and use it. func (p *CSRFPlugin) setCSRFCookie(reqCtx *models.RequestContext, token string) { - var samesite http.SameSite - switch p.pluginConfig.SameSite { - case "strict": - samesite = http.SameSiteStrictMode - case "none": - samesite = http.SameSiteNoneMode - case "lax": - samesite = http.SameSiteLaxMode - default: - samesite = http.SameSiteLaxMode - } + http.SetCookie(reqCtx.ResponseWriter, p.csrfCookie(reqCtx, token, int(p.pluginConfig.MaxAge.Seconds()))) + p.exposeCSRFToken(reqCtx, token) +} + +// clearCSRFCookie expires the CSRF cookie. It reuses csrfCookie so the clear carries exactly the +// same Domain, Path, Secure and SameSite attributes as the set; browsers only delete a cookie +// when those match. +func (p *CSRFPlugin) clearCSRFCookie(reqCtx *models.RequestContext) { + http.SetCookie(reqCtx.ResponseWriter, p.csrfCookie(reqCtx, "", -1)) +} - // Secure must be true for HTTPS requests or when SameSite=None - secure := reqCtx.Request.URL.Scheme == "https" || samesite == http.SameSiteNoneMode || p.pluginConfig.Secure +// csrfCookie builds the CSRF cookie with the attributes shared by set and clear. +// HttpOnly is always false because the Double-Submit Cookie pattern requires JavaScript to read the value. +// Secure is forced on for HTTPS requests and for SameSite=None (which browsers reject without Secure), +// and is otherwise taken from config so development over plain HTTP keeps working. +// Domain follows the session cookie so both are scoped to the same hosts. +func (p *CSRFPlugin) csrfCookie(reqCtx *models.RequestContext, value string, maxAge int) *http.Cookie { + sameSite := util.ParseSameSite(p.pluginConfig.SameSite) + secure := reqCtx.Request.URL.Scheme == "https" || sameSite == http.SameSiteNoneMode || p.pluginConfig.Secure - http.SetCookie(reqCtx.ResponseWriter, &http.Cookie{ + return &http.Cookie{ Name: p.pluginConfig.CookieName, - Value: token, + Value: value, Path: "/", - HttpOnly: false, // Hardcoded: Required for Double-Submit Cookie pattern + Domain: p.globalConfig.Session.Domain, + HttpOnly: false, Secure: secure, - SameSite: samesite, - MaxAge: int(p.pluginConfig.MaxAge.Seconds()), - }) - - p.exposeCSRFToken(reqCtx, token) + SameSite: sameSite, + MaxAge: maxAge, + } } // exposeCSRFToken places the token in the response header. Clients on another origin diff --git a/plugins/csrf/plugin_test.go b/plugins/csrf/plugin_test.go index 3da11dab..28cfbd3d 100644 --- a/plugins/csrf/plugin_test.go +++ b/plugins/csrf/plugin_test.go @@ -379,6 +379,7 @@ func TestCSRFPlugin_CSRFValidation(t *testing.T) { func TestCSRFPlugin_TokenRotation(t *testing.T) { p := New(CSRFPluginConfig{}) p.logger = &MockLogger{} + p.globalConfig = &models.Config{} hooks := p.Hooks() afterHook := hooks[2] @@ -473,6 +474,7 @@ func TestCSRFPlugin_SetCSRFCookie(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { p := New(CSRFPluginConfig{SameSite: tt.sameSite}) + p.globalConfig = &models.Config{} w := httptest.NewRecorder() r := httptest.NewRequest("GET", tt.scheme+"://example.com/test", nil) ctx := &models.RequestContext{Request: r, ResponseWriter: w} @@ -507,6 +509,101 @@ func TestCSRFPlugin_SetCSRFCookie(t *testing.T) { } } +func TestCSRFPlugin_CookieDomain(t *testing.T) { + tests := []struct { + name string + domain string + wantDomain string + }{ + {name: "host-only when no domain is configured", domain: "", wantDomain: ""}, + {name: "follows the session cookie domain", domain: "example.com", wantDomain: "example.com"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := New(CSRFPluginConfig{}) + p.globalConfig = &models.Config{Session: models.SessionConfig{Domain: tt.domain}} + + setRecorder := httptest.NewRecorder() + p.setCSRFCookie(&models.RequestContext{ + Request: httptest.NewRequest("GET", "https://api.example.com/test", nil), + ResponseWriter: setRecorder, + }, "test_token") + setCookies := setRecorder.Result().Cookies() + if len(setCookies) != 1 { + t.Fatalf("expected 1 cookie on set, got %d", len(setCookies)) + } + if setCookies[0].Domain != tt.wantDomain { + t.Errorf("set Domain = %q, want %q", setCookies[0].Domain, tt.wantDomain) + } + + clearRecorder := httptest.NewRecorder() + p.clearCSRFCookie(&models.RequestContext{ + Request: httptest.NewRequest("POST", "https://api.example.com/sign-out", nil), + ResponseWriter: clearRecorder, + }) + clearCookies := clearRecorder.Result().Cookies() + if len(clearCookies) != 1 { + t.Fatalf("expected 1 cookie on clear, got %d", len(clearCookies)) + } + if clearCookies[0].Domain != tt.wantDomain { + t.Errorf("clear Domain = %q, want %q", clearCookies[0].Domain, tt.wantDomain) + } + }) + } +} + +// The browser only deletes a cookie when the clear carries the same scoping attributes as the set. +func TestCSRFPlugin_ClearCookieMatchesSetAttributes(t *testing.T) { + tests := []struct { + name string + sameSite string + secure bool + scheme string + }{ + {name: "lax over http", sameSite: "lax", scheme: "http"}, + {name: "strict with secure flag over http", sameSite: "strict", secure: true, scheme: "http"}, + {name: "none over https", sameSite: "none", scheme: "https"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := New(CSRFPluginConfig{SameSite: tt.sameSite, Secure: tt.secure}) + p.globalConfig = &models.Config{Session: models.SessionConfig{Domain: "example.com"}} + + setRecorder := httptest.NewRecorder() + p.setCSRFCookie(&models.RequestContext{ + Request: httptest.NewRequest("GET", tt.scheme+"://api.example.com/test", nil), + ResponseWriter: setRecorder, + }, "test_token") + set := setRecorder.Result().Cookies()[0] + + clearRecorder := httptest.NewRecorder() + p.clearCSRFCookie(&models.RequestContext{ + Request: httptest.NewRequest("POST", tt.scheme+"://api.example.com/sign-out", nil), + ResponseWriter: clearRecorder, + }) + clear := clearRecorder.Result().Cookies()[0] + + if clear.Value != "" || clear.MaxAge != -1 { + t.Errorf("clear must expire the cookie, got Value=%q MaxAge=%d", clear.Value, clear.MaxAge) + } + if clear.Name != set.Name || clear.Path != set.Path || clear.Domain != set.Domain { + t.Errorf("clear scope (%s %s %s) != set scope (%s %s %s)", clear.Name, clear.Path, clear.Domain, set.Name, set.Path, set.Domain) + } + if clear.Secure != set.Secure { + t.Errorf("clear Secure = %v, set Secure = %v", clear.Secure, set.Secure) + } + if clear.SameSite != set.SameSite { + t.Errorf("clear SameSite = %v, set SameSite = %v", clear.SameSite, set.SameSite) + } + if clear.HttpOnly != set.HttpOnly { + t.Errorf("clear HttpOnly = %v, set HttpOnly = %v", clear.HttpOnly, set.HttpOnly) + } + }) + } +} + func TestCSRFPlugin_Middleware(t *testing.T) { p := New(CSRFPluginConfig{}) initPlugin(t, p) diff --git a/plugins/session/plugin.go b/plugins/session/plugin.go index dccc1d34..30ab1cda 100644 --- a/plugins/session/plugin.go +++ b/plugins/session/plugin.go @@ -157,27 +157,15 @@ func (p *SessionPlugin) writeErrorResponse(w http.ResponseWriter, statusCode int }) } -func (p *SessionPlugin) getSameSiteMode() http.SameSite { - switch p.globalConfig.Session.SameSite { - case "strict": - return http.SameSiteStrictMode - case "none": - return http.SameSiteNoneMode - case "lax": - return http.SameSiteLaxMode - default: - return http.SameSiteLaxMode - } -} - func (p *SessionPlugin) SetSessionCookie(w http.ResponseWriter, sessionToken string, expiresAt time.Time) { http.SetCookie(w, &http.Cookie{ Name: p.globalConfig.Session.CookieName, Value: sessionToken, Path: "/", + Domain: p.globalConfig.Session.Domain, HttpOnly: p.globalConfig.Session.HttpOnly, Secure: p.globalConfig.Session.Secure, - SameSite: p.getSameSiteMode(), + SameSite: util.ParseSameSite(p.globalConfig.Session.SameSite), MaxAge: p.cookieMaxAge(expiresAt), }) } @@ -187,9 +175,10 @@ func (p *SessionPlugin) ClearSessionCookie(w http.ResponseWriter) { Name: p.globalConfig.Session.CookieName, Value: "", Path: "/", + Domain: p.globalConfig.Session.Domain, HttpOnly: p.globalConfig.Session.HttpOnly, Secure: p.globalConfig.Session.Secure, - SameSite: p.getSameSiteMode(), + SameSite: util.ParseSameSite(p.globalConfig.Session.SameSite), MaxAge: -1, }) } diff --git a/plugins/session/plugin_test.go b/plugins/session/plugin_test.go index 5a76c0ab..6d0ea9e1 100644 --- a/plugins/session/plugin_test.go +++ b/plugins/session/plugin_test.go @@ -233,6 +233,40 @@ func TestSessionPlugin_ClearSessionCookie(t *testing.T) { assert.Equal(t, -1, cookie.MaxAge) } +func TestSessionPlugin_CookieDomain(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + domain string + wantDomain string + }{ + {name: "host-only when no domain is configured", domain: "", wantDomain: ""}, + {name: "shares the configured parent domain", domain: "example.com", wantDomain: "example.com"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + plugin := newTestPlugin(new(internaltests.MockSessionService), new(internaltests.MockTokenService)) + plugin.globalConfig.Session.Domain = tt.domain + + setRecorder := httptest.NewRecorder() + plugin.SetSessionCookie(setRecorder, "plaintext-token", time.Now().UTC().Add(time.Hour)) + setCookies := setRecorder.Result().Cookies() + require.Len(t, setCookies, 1) + assert.Equal(t, tt.wantDomain, setCookies[0].Domain) + + clearRecorder := httptest.NewRecorder() + plugin.ClearSessionCookie(clearRecorder) + clearCookies := clearRecorder.Result().Cookies() + require.Len(t, clearCookies, 1) + assert.Equal(t, tt.wantDomain, clearCookies[0].Domain, "clear must carry the same Domain as the set or the browser keeps the cookie") + }) + } +} + func TestSessionPlugin_ValidateSessionHook(t *testing.T) { t.Parallel() diff --git a/plugins/totp/handlers/enable_handler.go b/plugins/totp/handlers/enable_handler.go index 0e3314c9..99cc8ad4 100644 --- a/plugins/totp/handlers/enable_handler.go +++ b/plugins/totp/handlers/enable_handler.go @@ -7,6 +7,7 @@ import ( "github.com/Authula/authula/plugins/totp/constants" "github.com/Authula/authula/plugins/totp/types" "github.com/Authula/authula/plugins/totp/usecases" + "github.com/Authula/authula/util" ) type EnableHandler struct { @@ -34,10 +35,11 @@ func (h *EnableHandler) Handler() http.HandlerFunc { Name: constants.CookieTOTPPending, Value: result.PendingToken, Path: "/", + Domain: h.GlobalConfig.Session.Domain, MaxAge: int(h.PluginConfig.PendingTokenExpiry.Seconds()), HttpOnly: true, Secure: h.PluginConfig.SecureCookie, - SameSite: types.ParseSameSite(h.PluginConfig.SameSite), + SameSite: util.ParseSameSite(h.PluginConfig.SameSite), }) } diff --git a/plugins/totp/handlers/verify_backup_code_handler.go b/plugins/totp/handlers/verify_backup_code_handler.go index e744fef6..a6bbcb99 100644 --- a/plugins/totp/handlers/verify_backup_code_handler.go +++ b/plugins/totp/handlers/verify_backup_code_handler.go @@ -12,6 +12,7 @@ import ( ) type VerifyBackupCodeHandler struct { + GlobalConfig *models.Config PluginConfig *types.TOTPPluginConfig UseCase *usecases.VerifyBackupCodeUseCase } @@ -83,10 +84,11 @@ func (h *VerifyBackupCodeHandler) Handler() http.HandlerFunc { Name: constants.CookieTOTPTrusted, Value: result.TrustedDeviceToken, Path: "/", + Domain: h.GlobalConfig.Session.Domain, MaxAge: int(h.PluginConfig.TrustedDeviceDuration.Seconds()), HttpOnly: true, Secure: h.PluginConfig.SecureCookie, - SameSite: types.ParseSameSite(h.PluginConfig.SameSite), + SameSite: util.ParseSameSite(h.PluginConfig.SameSite), }) } @@ -94,10 +96,11 @@ func (h *VerifyBackupCodeHandler) Handler() http.HandlerFunc { Name: constants.CookieTOTPPending, Value: "", Path: "/", + Domain: h.GlobalConfig.Session.Domain, MaxAge: -1, HttpOnly: true, Secure: h.PluginConfig.SecureCookie, - SameSite: types.ParseSameSite(h.PluginConfig.SameSite), + SameSite: util.ParseSameSite(h.PluginConfig.SameSite), }) reqCtx.SetJSONResponse(http.StatusOK, &types.VerifyBackupCodeResponse{ diff --git a/plugins/totp/handlers/verify_backup_code_handler_test.go b/plugins/totp/handlers/verify_backup_code_handler_test.go index 5d957bce..6d5bcc81 100644 --- a/plugins/totp/handlers/verify_backup_code_handler_test.go +++ b/plugins/totp/handlers/verify_backup_code_handler_test.go @@ -151,6 +151,7 @@ func (s *VerifyBackupCodeHandlerSuite) TestVerifyBackupCodeHandler_Table() { cleared := totptests.CookieFromRecorder(w, constants.CookieTOTPPending) require.NotNil(t, cleared) assert.Equal(t, -1, cleared.MaxAge) + assert.Equal(t, testCookieDomain, cleared.Domain) assert.Nil(t, totptests.CookieFromRecorder(w, constants.CookieTOTPTrusted)) }, }, @@ -185,6 +186,7 @@ func (s *VerifyBackupCodeHandlerSuite) TestVerifyBackupCodeHandler_Table() { trusted := totptests.CookieFromRecorder(w, constants.CookieTOTPTrusted) require.NotNil(t, trusted, "trusted device cookie should be set") assert.Equal(t, "trusted-tok", trusted.Value) + assert.Equal(t, testCookieDomain, trusted.Domain) }, }, } @@ -199,7 +201,11 @@ func (s *VerifyBackupCodeHandlerSuite) TestVerifyBackupCodeHandler_Table() { } uc := buildVerifyBackupCodeUseCase(m.tokenSvc, m.verifSvc, m.sessionSvc, m.userSvc, m.repo, m.passwordSvc, m.pluginCfg) - h := &VerifyBackupCodeHandler{UseCase: uc, PluginConfig: m.pluginCfg} + h := &VerifyBackupCodeHandler{ + GlobalConfig: &models.Config{Session: models.SessionConfig{Domain: testCookieDomain}}, + PluginConfig: m.pluginCfg, + UseCase: uc, + } req, w, reqCtx := m.newRequest(t) h.Handler().ServeHTTP(w, req) diff --git a/plugins/totp/handlers/verify_totp_handler.go b/plugins/totp/handlers/verify_totp_handler.go index 94071fb1..63b8b231 100644 --- a/plugins/totp/handlers/verify_totp_handler.go +++ b/plugins/totp/handlers/verify_totp_handler.go @@ -12,6 +12,7 @@ import ( ) type VerifyTOTPHandler struct { + GlobalConfig *models.Config PluginConfig *types.TOTPPluginConfig UseCase *usecases.VerifyTOTPUseCase } @@ -83,10 +84,11 @@ func (h *VerifyTOTPHandler) Handler() http.HandlerFunc { Name: constants.CookieTOTPTrusted, Value: result.TrustedDeviceToken, Path: "/", + Domain: h.GlobalConfig.Session.Domain, MaxAge: int(h.PluginConfig.TrustedDeviceDuration.Seconds()), HttpOnly: true, Secure: h.PluginConfig.SecureCookie, - SameSite: types.ParseSameSite(h.PluginConfig.SameSite), + SameSite: util.ParseSameSite(h.PluginConfig.SameSite), }) } @@ -94,10 +96,11 @@ func (h *VerifyTOTPHandler) Handler() http.HandlerFunc { Name: constants.CookieTOTPPending, Value: "", Path: "/", + Domain: h.GlobalConfig.Session.Domain, MaxAge: -1, HttpOnly: true, Secure: h.PluginConfig.SecureCookie, - SameSite: types.ParseSameSite(h.PluginConfig.SameSite), + SameSite: util.ParseSameSite(h.PluginConfig.SameSite), }) reqCtx.SetJSONResponse(http.StatusOK, &types.VerifyTOTPResponse{ diff --git a/plugins/totp/handlers/verify_totp_handler_test.go b/plugins/totp/handlers/verify_totp_handler_test.go index 7a0f97f2..bdb2b509 100644 --- a/plugins/totp/handlers/verify_totp_handler_test.go +++ b/plugins/totp/handlers/verify_totp_handler_test.go @@ -60,6 +60,9 @@ type VerifyTOTPHandlerSuite struct { suite.Suite } +// testCookieDomain is the session cookie domain the TOTP cookies must inherit. +const testCookieDomain = "example.com" + type verifyTOTPFixture struct { pluginCfg *types.TOTPPluginConfig tokenSvc *internaltests.MockTokenService @@ -185,6 +188,7 @@ func (s *VerifyTOTPHandlerSuite) TestVerifyTOTPHandler_Table() { cleared := totptests.CookieFromRecorder(w, constants.CookieTOTPPending) require.NotNil(t, cleared) assert.Equal(t, -1, cleared.MaxAge) + assert.Equal(t, testCookieDomain, cleared.Domain) assert.Nil(t, totptests.CookieFromRecorder(w, constants.CookieTOTPTrusted)) var resp types.VerifyTOTPResponse @@ -219,10 +223,12 @@ func (s *VerifyTOTPHandlerSuite) TestVerifyTOTPHandler_Table() { trusted := totptests.CookieFromRecorder(w, constants.CookieTOTPTrusted) require.NotNil(t, trusted, "trusted device cookie should be set") assert.Equal(t, "trusted-token", trusted.Value) + assert.Equal(t, testCookieDomain, trusted.Domain) pending := totptests.CookieFromRecorder(w, constants.CookieTOTPPending) require.NotNil(t, pending) assert.Equal(t, -1, pending.MaxAge) + assert.Equal(t, testCookieDomain, pending.Domain) }, }, } @@ -236,7 +242,11 @@ func (s *VerifyTOTPHandlerSuite) TestVerifyTOTPHandler_Table() { tt.prepare(m) } - h := &VerifyTOTPHandler{UseCase: buildVerifyTOTPUseCase(m.pluginCfg, m.tokenSvc, m.verifSvc, m.sessionSvc, m.userSvc, m.repo), PluginConfig: m.pluginCfg} + h := &VerifyTOTPHandler{ + GlobalConfig: &models.Config{Session: models.SessionConfig{Domain: testCookieDomain}}, + PluginConfig: m.pluginCfg, + UseCase: buildVerifyTOTPUseCase(m.pluginCfg, m.tokenSvc, m.verifSvc, m.sessionSvc, m.userSvc, m.repo), + } req, w, reqCtx := m.newRequest(t) h.Handler().ServeHTTP(w, req) diff --git a/plugins/totp/hooks.go b/plugins/totp/hooks.go index 285013c5..ee9c863a 100644 --- a/plugins/totp/hooks.go +++ b/plugins/totp/hooks.go @@ -8,6 +8,7 @@ import ( "github.com/Authula/authula/models" "github.com/Authula/authula/plugins/totp/constants" "github.com/Authula/authula/plugins/totp/types" + "github.com/Authula/authula/util" ) type TOTPHookID string @@ -101,10 +102,11 @@ func (p *TOTPPlugin) interceptSignInHook(reqCtx *models.RequestContext) error { Name: constants.CookieTOTPPending, Value: token, Path: "/", + Domain: p.globalConfig.Session.Domain, MaxAge: int(p.pluginConfig.PendingTokenExpiry.Seconds()), HttpOnly: true, Secure: p.pluginConfig.SecureCookie, - SameSite: types.ParseSameSite(p.pluginConfig.SameSite), + SameSite: util.ParseSameSite(p.pluginConfig.SameSite), }) // Clear session values — prevent session creation diff --git a/plugins/totp/routes.go b/plugins/totp/routes.go index 3861f7e9..aec3d0e8 100644 --- a/plugins/totp/routes.go +++ b/plugins/totp/routes.go @@ -24,6 +24,7 @@ func Routes(p *TOTPPlugin) []models.Route { UseCase: uc.GetTOTPURI, } verifyTOTPHandler := &handlers.VerifyTOTPHandler{ + GlobalConfig: p.globalConfig, PluginConfig: p.pluginConfig, UseCase: uc.VerifyTOTP, } @@ -31,6 +32,7 @@ func Routes(p *TOTPPlugin) []models.Route { UseCase: uc.GenerateBackupCodes, } verifyBackupCodeHandler := &handlers.VerifyBackupCodeHandler{ + GlobalConfig: p.globalConfig, PluginConfig: p.pluginConfig, UseCase: uc.VerifyBackupCode, } diff --git a/plugins/totp/types/types.go b/plugins/totp/types/types.go index 85c5ceb1..6f9b7786 100644 --- a/plugins/totp/types/types.go +++ b/plugins/totp/types/types.go @@ -1,8 +1,6 @@ package types import ( - "net/http" - "strings" "time" "github.com/Authula/authula/models" @@ -39,19 +37,6 @@ func (c *TOTPPluginConfig) ApplyDefaults() { } } -func ParseSameSite(s string) http.SameSite { - switch strings.ToLower(s) { - case "strict": - return http.SameSiteStrictMode - case "none": - return http.SameSiteNoneMode - case "lax": - return http.SameSiteLaxMode - default: - return http.SameSiteLaxMode - } -} - // Request payloads type DisableResponse struct { diff --git a/util/cookie.go b/util/cookie.go new file mode 100644 index 00000000..a33f9b40 --- /dev/null +++ b/util/cookie.go @@ -0,0 +1,19 @@ +package util + +import ( + "net/http" + "strings" +) + +// ParseSameSite maps a configured SameSite string ("lax", "strict" or "none", case-insensitive) +// to its http.SameSite value. Unknown or empty values fall back to Lax. +func ParseSameSite(value string) http.SameSite { + switch strings.ToLower(value) { + case "strict": + return http.SameSiteStrictMode + case "none": + return http.SameSiteNoneMode + default: + return http.SameSiteLaxMode + } +} diff --git a/util/cookie_test.go b/util/cookie_test.go new file mode 100644 index 00000000..bb3f4f76 --- /dev/null +++ b/util/cookie_test.go @@ -0,0 +1,33 @@ +package util + +import ( + "net/http" + "testing" +) + +func TestParseSameSite(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + value string + want http.SameSite + }{ + {name: "lax", value: "lax", want: http.SameSiteLaxMode}, + {name: "strict", value: "strict", want: http.SameSiteStrictMode}, + {name: "none", value: "none", want: http.SameSiteNoneMode}, + {name: "case insensitive", value: "Strict", want: http.SameSiteStrictMode}, + {name: "empty falls back to lax", value: "", want: http.SameSiteLaxMode}, + {name: "unknown falls back to lax", value: "whatever", want: http.SameSiteLaxMode}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := ParseSameSite(tt.value); got != tt.want { + t.Fatalf("ParseSameSite(%q) = %v, want %v", tt.value, got, tt.want) + } + }) + } +}