Skip to content

Commit 2271d82

Browse files
WhammyLeafWhammyLeaf
committed
support allowed domains for fixed redirect mode
Signed-off-by: Christian Troelsen <[email protected]> add some tests
1 parent 0bfe704 commit 2271d82

File tree

3 files changed

+174
-28
lines changed

3 files changed

+174
-28
lines changed

‎config.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,11 @@ import (
1111
// Config holds OAuth configuration
1212
type Config struct {
1313
// OAuth settings
14-
Mode string // "native" or "proxy"
15-
Provider string // "hmac", "okta", "google", "azure"
16-
RedirectURIs string // Redirect URIs allowlist (single or comma-separated)
17-
FixedRedirectURI string // Optional fixed redirect URI used for proxying callbacks
14+
Mode string // "native" or "proxy"
15+
Provider string // "hmac", "okta", "google", "azure"
16+
RedirectURIs string // Redirect URIs allowlist (single or comma-separated)
17+
FixedRedirectURI string // Optional fixed redirect URI used for proxying callbacks
18+
AllowedClientRedirectDomains string // Optional comma-separated list of domain suffixes allowed for client redirect URIs in fixed redirect mode (in addition to localhost)
1819

1920
// OIDC configuration
2021
Issuer string
@@ -204,6 +205,12 @@ func (b *ConfigBuilder) WithFixedRedirectURI(uri string) *ConfigBuilder {
204205
return b
205206
}
206207

208+
// WithAllowedClientRedirectDomains sets allowed client redirect domains
209+
func (b *ConfigBuilder) WithAllowedClientRedirectDomains(domains string) *ConfigBuilder {
210+
b.config.AllowedClientRedirectDomains = domains
211+
return b
212+
}
213+
207214
// WithIssuer sets the OIDC issuer
208215
func (b *ConfigBuilder) WithIssuer(issuer string) *ConfigBuilder {
209216
b.config.Issuer = issuer
@@ -327,6 +334,7 @@ func FromEnv() (*Config, error) {
327334
WithProvider(getEnv("OAUTH_PROVIDER", "")).
328335
WithRedirectURIs(getEnv("OAUTH_REDIRECT_URIS", "")).
329336
WithFixedRedirectURI(getEnv("OAUTH_FIXED_REDIRECT_URI", "")).
337+
WithAllowedClientRedirectDomains(getEnv("OAUTH_ALLOWED_CLIENT_REDIRECT_DOMAINS", "")).
330338
WithIssuer(getEnv("OIDC_ISSUER", "")).
331339
WithAudience(getEnv("OIDC_AUDIENCE", "")).
332340
WithClientID(getEnv("OIDC_CLIENT_ID", "")).

‎handlers.go‎

Lines changed: 70 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ type OAuth2Config struct {
4343
// FixedRedirectURI is an optional fixed redirect URI used when proxying callbacks
4444
FixedRedirectURI string
4545

46+
// AllowedClientRedirectDomains is an optional comma-separated list of domain suffixes
47+
// that are allowed for client redirect URIs in fixed redirect mode (in addition to localhost).
48+
AllowedClientRedirectDomains string
49+
4650
// OIDC configuration
4751
Issuer string
4852
Audience string
@@ -186,22 +190,23 @@ func NewOAuth2ConfigFromConfig(cfg *Config, version string) *OAuth2Config {
186190
}
187191

188192
return &OAuth2Config{
189-
Enabled: true,
190-
Mode: cfg.Mode,
191-
Provider: cfg.Provider,
192-
RedirectURIs: cfg.RedirectURIs,
193-
FixedRedirectURI: cfg.FixedRedirectURI,
194-
Issuer: cfg.Issuer,
195-
Audience: cfg.Audience,
196-
ClientID: cfg.ClientID,
197-
ClientSecret: cfg.ClientSecret,
198-
Scopes: scopes,
199-
MCPHost: mcpHost,
200-
MCPPort: mcpPort,
201-
MCPURL: mcpURL,
202-
Scheme: scheme,
203-
Version: version,
204-
stateSigningKey: cfg.JWTSecret,
193+
Enabled: true,
194+
Mode: cfg.Mode,
195+
Provider: cfg.Provider,
196+
RedirectURIs: cfg.RedirectURIs,
197+
FixedRedirectURI: cfg.FixedRedirectURI,
198+
AllowedClientRedirectDomains: cfg.AllowedClientRedirectDomains,
199+
Issuer: cfg.Issuer,
200+
Audience: cfg.Audience,
201+
ClientID: cfg.ClientID,
202+
ClientSecret: cfg.ClientSecret,
203+
Scopes: scopes,
204+
MCPHost: mcpHost,
205+
MCPPort: mcpPort,
206+
MCPURL: mcpURL,
207+
Scheme: scheme,
208+
Version: version,
209+
stateSigningKey: cfg.JWTSecret,
205210
}
206211
}
207212

@@ -353,11 +358,11 @@ func (h *OAuth2Handler) HandleAuthorize(w http.ResponseWriter, r *http.Request)
353358
return
354359
}
355360

356-
// Security: For fixed redirect mode, only allow localhost or loopback addresses
357-
// This prevents open redirect attacks while still supporting development tools
358-
if !isLocalhostURI(clientRedirectURI) {
359-
h.logger.Warn("SECURITY: Fixed redirect mode only allows localhost URIs, rejecting: %s from %s", clientRedirectURI, r.RemoteAddr)
360-
http.Error(w, "Fixed redirect mode only allows localhost redirect URIs for security. Use allowlist mode for production.", http.StatusBadRequest)
361+
// Security: For fixed redirect mode, only allow localhost or explicitly configured domain suffixes.
362+
// This prevents open redirect attacks while still supporting development tools and trusted hosts.
363+
if !h.isAllowedClientRedirectURI(clientRedirectURI) {
364+
h.logger.Warn("SECURITY: Fixed redirect mode only allows localhost or configured domains, rejecting: %s from %s", clientRedirectURI, r.RemoteAddr)
365+
http.Error(w, "Invalid redirect_uri for fixed redirect mode", http.StatusBadRequest)
361366
return
362367
}
363368
redirectURI = strings.TrimSpace(h.config.FixedRedirectURI)
@@ -466,9 +471,9 @@ func (h *OAuth2Handler) HandleCallback(w http.ResponseWriter, r *http.Request) {
466471

467472
if hasState && hasRedirect {
468473
// Re-validate redirect URI for defense in depth
469-
// Even though state is HMAC-signed, validate the redirect URI is localhost
470-
if !isLocalhostURI(originalRedirectURI) {
471-
h.logger.Warn("SECURITY: Callback redirect URI is not localhost (possible key compromise): %s", originalRedirectURI)
474+
// Even though state is HMAC-signed, validate the redirect URI is localhost or an allowed domain
475+
if !h.isAllowedClientRedirectURI(originalRedirectURI) {
476+
h.logger.Warn("SECURITY: Callback redirect URI is not allowed (possible key compromise): %s", originalRedirectURI)
472477
http.Error(w, "Invalid redirect URI in state", http.StatusBadRequest)
473478
return
474479
}
@@ -801,6 +806,47 @@ func isLocalhostURI(uri string) bool {
801806
return hostname == "localhost" || hostname == "127.0.0.1" || hostname == "::1"
802807
}
803808

809+
// isAllowedClientRedirectURI checks whether a client redirect URI is allowed in fixed redirect mode.
810+
func (h *OAuth2Handler) isAllowedClientRedirectURI(uri string) bool {
811+
// Always allow localhost URIs for development tools
812+
if isLocalhostURI(uri) {
813+
return true
814+
}
815+
816+
// For non-localhost URIs, require explicit domain suffix configuration
817+
if h.config.AllowedClientRedirectDomains == "" {
818+
return false
819+
}
820+
821+
parsedURI, err := url.Parse(uri)
822+
if err != nil {
823+
return false
824+
}
825+
826+
// Only allow HTTPS for non-localhost URIs
827+
if parsedURI.Scheme != "https" {
828+
return false
829+
}
830+
831+
host := strings.ToLower(parsedURI.Hostname())
832+
if host == "" {
833+
return false
834+
}
835+
836+
// Check if host matches any configured suffix (exact match or subdomain)
837+
for _, suffix := range strings.Split(h.config.AllowedClientRedirectDomains, ",") {
838+
suffix = strings.TrimSpace(strings.ToLower(suffix))
839+
if suffix == "" {
840+
continue
841+
}
842+
if host == suffix || strings.HasSuffix(host, "."+suffix) {
843+
return true
844+
}
845+
}
846+
847+
return false
848+
}
849+
804850
// isValidRedirectURI validates redirect URI against allowlist for security
805851
func (h *OAuth2Handler) isValidRedirectURI(uri string) bool {
806852
if h.config.RedirectURIs == "" {

‎security_test.go‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,98 @@ func TestRedirectURIValidation(t *testing.T) {
7171
}
7272
}
7373

74+
func TestIsAllowedClientRedirectURI(t *testing.T) {
75+
tests := []struct {
76+
name string
77+
allowed string
78+
uri string
79+
isAllowed bool
80+
}{
81+
{
82+
name: "Localhost HTTP allowed without domains",
83+
allowed: "",
84+
uri: "http://localhost:8080/callback",
85+
isAllowed: true,
86+
},
87+
{
88+
name: "Localhost IPv4 allowed without domains",
89+
allowed: "",
90+
uri: "http://127.0.0.1:3000/callback",
91+
isAllowed: true,
92+
},
93+
{
94+
name: "Localhost IPv6 allowed without domains",
95+
allowed: "",
96+
uri: "http://[::1]:9000/callback",
97+
isAllowed: true,
98+
},
99+
{
100+
name: "Non-localhost without allowed domains rejected",
101+
allowed: "",
102+
uri: "https://example.com/callback",
103+
isAllowed: false,
104+
},
105+
{
106+
name: "HTTPS exact domain match allowed",
107+
allowed: "example.com",
108+
uri: "https://example.com/callback",
109+
isAllowed: true,
110+
},
111+
{
112+
name: "HTTPS subdomain match allowed",
113+
allowed: "example.com",
114+
uri: "https://app.example.com/callback",
115+
isAllowed: true,
116+
},
117+
{
118+
name: "Multiple domains with spaces allowed",
119+
allowed: "example.com, dummy.com ",
120+
uri: "https://client1.dummy.com/proxy/40073/callback",
121+
isAllowed: true,
122+
},
123+
{
124+
name: "Partial suffix does not match",
125+
allowed: "example.com",
126+
uri: "https://evil-example.com/callback",
127+
isAllowed: false,
128+
},
129+
{
130+
name: "HTTP non-localhost rejected even if domain configured",
131+
allowed: "example.com",
132+
uri: "http://example.com/callback",
133+
isAllowed: false,
134+
},
135+
{
136+
name: "Non-HTTPS scheme rejected",
137+
allowed: "example.com",
138+
uri: "custom://example.com/callback",
139+
isAllowed: false,
140+
},
141+
{
142+
name: "Invalid URI rejected",
143+
allowed: "example.com",
144+
uri: "not-a-valid-uri",
145+
isAllowed: false,
146+
},
147+
}
148+
149+
for _, tt := range tests {
150+
t.Run(tt.name, func(t *testing.T) {
151+
handler := &OAuth2Handler{
152+
config: &OAuth2Config{
153+
AllowedClientRedirectDomains: tt.allowed,
154+
},
155+
logger: &defaultLogger{},
156+
}
157+
158+
got := handler.isAllowedClientRedirectURI(tt.uri)
159+
if got != tt.isAllowed {
160+
t.Errorf("isAllowedClientRedirectURI(%q) = %v, want %v (allowed=%q)", tt.uri, got, tt.isAllowed, tt.allowed)
161+
}
162+
})
163+
}
164+
}
165+
74166
func TestOAuthParameterValidation(t *testing.T) {
75167
handler := &OAuth2Handler{logger: &defaultLogger{}}
76168

0 commit comments

Comments
 (0)