Feat: support allowed domains for fixed redirect mode#21
Conversation
WalkthroughAdds a new AllowedClientRedirectDomains config field (env + builder), threads it into OAuth2Config, changes fixed-redirect validation to allow localhost or configured domain suffixes (requiring HTTPS for non-localhost), and adds unit tests for the new validation. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
handlers.go (2)
361-369: Update log message to reflect broader domain support.Line 369 says "Validated localhost redirect URI" but the URI could now be a non-localhost domain from the allowed list, making the log message misleading during debugging.
Suggested fix
- h.logger.Info("OAuth2: Validated localhost redirect URI for proxy: %s", clientRedirectURI) + h.logger.Info("OAuth2: Validated client redirect URI for proxy: %s", clientRedirectURI)
474-481: Update log message to reflect broader domain support.Line 481 says "proxying callback to localhost client" but the redirect URI could now be a non-localhost domain from the allowed list.
Suggested fix
- h.logger.Info("OAuth2: State verified, proxying callback to localhost client: %s", originalRedirectURI) + h.logger.Info("OAuth2: State verified, proxying callback to client: %s", originalRedirectURI)
🧹 Nitpick comments (1)
handlers.go (1)
809-848: Implementation is secure; consider documenting domain suffix configuration risks.The domain suffix matching logic is correctly implemented:
- Requires HTTPS for non-localhost URIs
- Uses
.prefix for subdomain matching, preventing partial matches (e.g.,malicious-example.comwon't match suffixexample.com)- Defaults to rejecting all non-localhost URIs when unconfigured
Operational consideration: If an operator mistakenly configures a broad suffix like
comor a short domain, it could allow unintended redirect URIs. Consider adding documentation or a startup warning for very short domain suffixes (e.g., single-part TLDs) to help operators avoid misconfiguration.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config.gohandlers.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Never log raw OAuth tokens; only log SHA-256 hash of token formatted as fmt.Sprintf("%x", sha256.Sum256([]byte(token)))[:16]
Files:
config.gohandlers.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Security Scan
🔇 Additional comments (5)
config.go (3)
14-18: LGTM!The new
AllowedClientRedirectDomainsfield is well-documented and properly placed within the Config struct. The backward-compatible default (empty string) ensures existing deployments continue to allow only localhost redirects.
208-212: LGTM!The builder method follows the established fluent API pattern and is consistent with other builder methods in the file.
337-337: LGTM!Environment variable integration follows established naming conventions with the
OAUTH_prefix and properly defaults to an empty string for backward compatibility.handlers.go (2)
46-48: LGTM!The field is properly documented and mirrors the Config struct addition.
193-209: LGTM!The
AllowedClientRedirectDomainsfield is properly propagated from the source Config to OAuth2Config.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
WhammyLeaf
changed the title
WhammyLeaf
force-pushed
the
support-allowed-domains-for-fixed-redirect-mode
branch
from
9915c42 to
7547006
Compare
|
@tuannvm Thank you for merging the prior PRs. I have another one here I hope you can take a look at when you have the time 😄 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
handlers.go (2)
361-369: Misleading log message.Line 369 logs "Validated localhost redirect URI for proxy" but the URI could be a non-localhost configured domain that passed the
isAllowedClientRedirectURIcheck. This could cause confusion during debugging.Proposed fix
- h.logger.Info("OAuth2: Validated localhost redirect URI for proxy: %s", clientRedirectURI) + h.logger.Info("OAuth2: Validated client redirect URI for proxy: %s", clientRedirectURI)
474-481: Same log message inconsistency.Line 481 says "proxying callback to localhost client" but the URI could be a non-localhost configured domain.
Proposed fix
- h.logger.Info("OAuth2: State verified, proxying callback to localhost client: %s", originalRedirectURI) + h.logger.Info("OAuth2: State verified, proxying callback to client: %s", originalRedirectURI)
🧹 Nitpick comments (1)
handlers.go (1)
809-848: Domain suffix matching logic is secure.The implementation correctly prevents subdomain confusion attacks by requiring either an exact match or a
.-prefixed suffix match (e.g.,example.commatchesexample.comandsub.example.com, but notevilexample.com). HTTPS enforcement for non-localhost and fail-closed behavior when no domains are configured are good security practices.Consider validating configured domain suffixes.
If an administrator mistakenly configures
AllowedClientRedirectDomains=https://example.comorexample.com/path, the matching will silently fail. Optional validation during startup or first use could help catch misconfiguration early.Optional: Add suffix validation helper
// validateDomainSuffix checks if a configured suffix is a valid domain (no scheme, no path) func validateDomainSuffix(suffix string) bool { // Should not contain scheme if strings.Contains(suffix, "://") { return false } // Should not contain path if strings.Contains(suffix, "/") { return false } return len(suffix) > 0 }This could be called when parsing the configuration to log warnings for invalid entries.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config.gohandlers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- config.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Never log raw OAuth tokens; only log SHA-256 hash of token formatted as fmt.Sprintf("%x", sha256.Sum256([]byte(token)))[:16]
Files:
handlers.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Verification
🔇 Additional comments (2)
handlers.go (2)
46-49: LGTM!The new field is well-documented and the comment clearly explains its purpose and usage in fixed redirect mode.
193-210: LGTM!The new
AllowedClientRedirectDomainsfield is correctly propagated from the genericConfigtoOAuth2Config.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
WhammyLeaf
force-pushed
the
support-allowed-domains-for-fixed-redirect-mode
branch
2 times, most recently
from
2271d82 to
43a8be3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
handlers.go (2)
361-369: Log message is misleading for non-localhost URIs.Line 369 logs "Validated localhost redirect URI" but after this change, the URI could also be from an allowed domain suffix, not just localhost.
Suggested fix
- h.logger.Info("OAuth2: Validated localhost redirect URI for proxy: %s", clientRedirectURI) + h.logger.Info("OAuth2: Validated client redirect URI for proxy: %s", clientRedirectURI)
474-481: Log message is misleading for non-localhost URIs.Line 481 logs "proxying callback to localhost client" but the URI could be from an allowed domain, not localhost.
Suggested fix
- h.logger.Info("OAuth2: State verified, proxying callback to localhost client: %s", originalRedirectURI) + h.logger.Info("OAuth2: State verified, proxying callback to client: %s", originalRedirectURI)
Signed-off-by: Christian Troelsen <christian.troelsen@tryg.dk> add some tests expand formatting update again
WhammyLeaf
force-pushed
the
support-allowed-domains-for-fixed-redirect-mode
branch
from
43a8be3 to
be67bd0
Compare
|
@tuannvm Comments have been addressed 😄 |
|
@tuannvm Could you take a look at this? |
|
thanks @WhammyLeaf ! |
2 participants
Summary
This PR adds a new config variable
AllowedClientRedirectDomains.When
AllowedClientRedirectDomainsis set, to a comma-separated string of one or more valid domains, callback URIs with those domains will, in addition to localhost based callback URIs, be allowed for fixed redirect mode.When
AllowedClientRedirectDomainsis unset, the current behaviour remains unchanged: only localhost based callback URIs are allowed for fixed redirect mode.Changes
config.goupdated with newAllowedClientRedirectDomainsvariablehandleAuthorizeandhandleCallbackmethods inhandlers.gohave been updated to allow callback URIs that are not localhost as long as their domain is trusted.Testing
Related Issues
#22
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.