diff --git a/internal/config/migratable_settings.go b/internal/config/migratable_settings.go index fafac11c..e8cdc668 100644 --- a/internal/config/migratable_settings.go +++ b/internal/config/migratable_settings.go @@ -29,6 +29,11 @@ type MigratableSetting struct { } // settingSource returns "environment" if the given env var is set, otherwise "config". +// +// The env var named here must be one this package's struct tags actually bind. +// Reporting "environment" for a name the struct does not read would mark the +// struct DEFAULT as Explicit, and an Explicit default outranks the database — +// turning a fallback into an override. // SEM@ef979cc7527137e0448782341a4ffe8e944571f5: return 'environment' when the given env var is set, otherwise 'config' (pure) func settingSource(envVar string) string { if os.Getenv(envVar) != "" { @@ -185,14 +190,30 @@ func (c *Config) getMigratableFeatureFlags() []MigratableSetting { func (c *Config) getMigratableOAuthSettings() []MigratableSetting { settings := []MigratableSetting{} - // OAuth callback URL (non-sensitive, useful for diagnostics) + // OAuth callback URL (non-sensitive, useful for diagnostics). + // + // Source MUST go through settingSource, not a hardcoded "config". This is an + // operational (database-backed) key, and SettingsService.getConfigSetting + // discards any operational setting that is not Explicit — so a hardcoded + // "config" made GetString("auth.oauth_callback_url") always fall through to + // the database, no matter what the operator set. On AWS that let a stale + // http://localhost:8080/oauth2/callback row outrank a correctly-set + // TMI_OAUTH_CALLBACK_URL, and every provider (Google, GitHub, Microsoft) + // advertised a localhost redirect_uri, so OAuth sign-in could not complete. + // + // Only TMI_OAUTH_CALLBACK_URL is named, matching the OAuthConfig.CallbackURL + // struct tag. auth/config.go additionally honors a legacy OAUTH_CALLBACK_URL, + // but this package does not bind it, so counting it here would report + // "environment" while Value was still the localhost default — making that + // default Explicit and letting it outrank a correct database row. if c.Auth.OAuth.CallbackURL != "" { settings = append(settings, MigratableSetting{ Key: "auth.oauth_callback_url", Value: c.Auth.OAuth.CallbackURL, Type: "string", Description: "OAuth callback URL", - Source: "config", + Source: settingSource("TMI_OAUTH_CALLBACK_URL"), + EnvVar: "TMI_OAUTH_CALLBACK_URL", }) } diff --git a/internal/config/migratable_settings_test.go b/internal/config/migratable_settings_test.go index 870470bd..c3e7525d 100644 --- a/internal/config/migratable_settings_test.go +++ b/internal/config/migratable_settings_test.go @@ -184,3 +184,60 @@ func TestDefaultOperationalSettings_OnlyOperational(t *testing.T) { t.Error("expected websocket.inactivity_timeout_seconds among operational settings") } } + +// A callback URL supplied via TMI_OAUTH_CALLBACK_URL must be reported as +// Source "environment" so GetMigratableSettings marks it Explicit. +// +// This is load-bearing, not cosmetic. SettingsService.getConfigSetting drops +// any non-Explicit operational setting, so GetString("auth.oauth_callback_url") +// falls through to the database. On AWS that meant a stale +// http://localhost:8080/oauth2/callback row won over a correctly-set +// TMI_OAUTH_CALLBACK_URL, and every provider's redirect_uri (Google, GitHub, +// Microsoft) was advertised as localhost — OAuth sign-in could not complete. +// The setting used to hardcode Source: "config", unlike the 130 others that +// route through settingSource(). +func TestGetMigratableSettings_OAuthCallbackURLHonorsEnvironment(t *testing.T) { + t.Setenv("TMI_OAUTH_CALLBACK_URL", "https://api.example.test/oauth2/callback") + cfg := &Config{Auth: AuthConfig{OAuth: OAuthConfig{ + CallbackURL: "https://api.example.test/oauth2/callback", + }}} + found := findSetting(cfg.GetMigratableSettings(), "auth.oauth_callback_url") + require.NotNil(t, found) + assert.Equal(t, "environment", found.Source) + assert.True(t, found.Explicit, "env-supplied callback URL must outrank the database row") + assert.Equal(t, "TMI_OAUTH_CALLBACK_URL", found.EnvVar) +} + +// With no env var set, the struct default must stay non-Explicit so the +// database remains reachable for this admin-only, hot-reloadable setting. +func TestGetMigratableSettings_OAuthCallbackURLDefaultDefersToDatabase(t *testing.T) { + t.Setenv("TMI_OAUTH_CALLBACK_URL", "") + t.Setenv("OAUTH_CALLBACK_URL", "") + cfg := &Config{Auth: AuthConfig{OAuth: OAuthConfig{ + CallbackURL: "http://localhost:8080/oauth2/callback", + }}} + found := findSetting(cfg.GetMigratableSettings(), "auth.oauth_callback_url") + require.NotNil(t, found) + assert.Equal(t, "config", found.Source) + assert.False(t, found.Explicit, "an unset callback URL must not shadow the database") +} + +// The legacy OAUTH_CALLBACK_URL is honored by auth/config.go but is NOT bound by +// this package's OAuthConfig.CallbackURL struct tag. It must therefore NOT count +// as "environment" here: doing so would report Explicit while Value was still the +// localhost struct default, and an Explicit default outranks the database — the +// fallback would become an override, which is a worse failure than the bug this +// fix addresses. +func TestGetMigratableSettings_OAuthCallbackURLIgnoresUnboundLegacyEnvName(t *testing.T) { + t.Setenv("TMI_OAUTH_CALLBACK_URL", "") + t.Setenv("OAUTH_CALLBACK_URL", "https://legacy.example.test/oauth2/callback") + // Value is the struct default, because this package never reads the legacy name. + cfg := &Config{Auth: AuthConfig{OAuth: OAuthConfig{ + CallbackURL: "http://localhost:8080/oauth2/callback", + }}} + found := findSetting(cfg.GetMigratableSettings(), "auth.oauth_callback_url") + require.NotNil(t, found) + assert.Equal(t, "config", found.Source) + assert.False(t, found.Explicit, + "an env name this package does not bind must not make the default Explicit") +}