From 9df2e59e8d7f2ed67c40793b271f85ab276ba868 Mon Sep 17 00:00:00 2001 From: Eric Fitzgerald Date: Wed, 29 Jul 2026 23:35:19 -0400 Subject: [PATCH] fix(config): let TMI_OAUTH_CALLBACK_URL outrank the database row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On AWS, GET /oauth2/providers advertised redirect_uri=http://localhost:8080/oauth2/callback for Google, GitHub and Microsoft, so OAuth sign-in could not complete — even though the deployment's TMI_OAUTH_CALLBACK_URL was correctly set to https://api.tmi.dev/oauth2/callback. auth.oauth_callback_url was the one singleton setting that hardcoded Source: "config" instead of routing through settingSource(); the other 130 all do. Source feeds Explicit, and SettingsService.getConfigSetting discards any operational setting that is not Explicit, so GetString("auth.oauth_callback_url") always fell through to the database regardless of what the operator configured. The database held the localhost struct default, and because Handlers.oauthCallbackURL returns the runtime value whenever it is non-empty, that stale row won and the correct env-backed value was never reached. Route Source through settingSource("TMI_OAUTH_CALLBACK_URL") and record EnvVar, matching every other env-backed key. Only TMI_OAUTH_CALLBACK_URL is named, deliberately. auth/config.go also honors a legacy OAUTH_CALLBACK_URL, but this package's OAuthConfig.CallbackURL struct tag does not bind it -- so counting it would report "environment" while Value was still the localhost default, making that default Explicit and letting it outrank a correct database row. That inverts a fallback into an override and is a worse failure than the one being fixed; a test pins the behavior, and settingSource's doc comment now states the constraint. Tests cover all three paths: env set wins, unset defers to the database, and the unbound legacy name does not mark the default Explicit. Verified failing before the fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc --- internal/config/migratable_settings.go | 25 ++++++++- internal/config/migratable_settings_test.go | 57 +++++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) 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") +}