refactor: remove concurrent listeners and rework cookie logic#950
Open
steveiliop56 wants to merge 8 commits into
Open
refactor: remove concurrent listeners and rework cookie logic#950steveiliop56 wants to merge 8 commits into
steveiliop56 wants to merge 8 commits into
Conversation
dosubot
Bot
added
the
size:L
Contributor
📝 WalkthroughWalkthroughReplaces the explicit ChangesSubdomain Trust Model Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuthController
participant BootstrapApp
participant GetCookieDomain
BootstrapApp->>GetCookieDomain: GetCookieDomain(AppURL, subdomainsEnabled)
GetCookieDomain-->>BootstrapApp: cookieDomain string
Client->>OAuthController: OAuth callback with redirect_uri
OAuthController->>OAuthController: isRedirectSafe(redirectURI)
Note over OAuthController: parse redirect URI and AppURL<br/>check scheme equality<br/>allow exact host match OR<br/>subdomain suffix if SubdomainsEnabled
OAuthController-->>Client: allow / reject redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/layout/layout.tsx`:
- Line 43: The domain warning condition on line 43 only checks for an exact
match against app.appUrl, but it should also respect the subdomainsEnabled flag
to allow valid subdomain origins without triggering the warning. Update the
condition in the if statement that checks ignoreDomainWarning,
ui.warningsEnabled, and currentUrl to also validate that currentUrl matches
either the canonical app.appUrl or is a valid subdomain when subdomainsEnabled
is true, using the same domain suffix matching predicate used in the redirect
and cookie trust model.
In `@internal/bootstrap/app_bootstrap.go`:
- Around line 165-169: The code logs that cookies will be "set for the current
domain only" when SubdomainsEnabled is false, but then calls
utils.GetCookieDomain() which still returns a domain value that gets set in the
cookie Domain attribute. When subdomains are disabled, the Domain attribute
should be completely omitted from the cookie (not set to a non-dotted domain
like "example.com") to achieve true host-only behavior. Modify the logic around
the utils.GetCookieDomain() call so that when app.config.Auth.SubdomainsEnabled
is false, the cookieDomain is set to an empty string instead of retrieving a
domain value, ensuring the Domain attribute is not included in the cookie.
- Around line 284-298: After updating app.runtime.AppURL and
app.runtime.CookieDomain in the Tailscale listening block, you must also refresh
the OAuth provider RedirectURL values that were initially assigned earlier based
on the original AppURL. Locate where the default provider RedirectURLs are set
(around lines 145-147 mentioned in the comment) and add code in the Tailscale
block to re-assign these provider redirect URLs using the new tailscaleUrl
value, ensuring OAuth callbacks correctly target the Tailscale hostname instead
of the previously configured host.
In `@internal/bootstrap/router_bootstrap.go`:
- Around line 132-141: The getListenerFunc method currently falls through to
Unix or HTTP listeners when tailscale.listen is true but tailscaleService is
nil, which violates the fail-closed principle. Modify the getListenerFunc method
to return an error when Tailscale listening is configured but the
tailscaleService is unavailable, preventing the silent fallback to standard
listeners. Update the function signature to return both a listener function and
an error, then handle the returned error in the Setup method before starting the
listener goroutine to fail early with an appropriate error message.
In `@internal/controller/oauth_controller.go`:
- Line 317: The oauth_controller.go file is logging full redirectURI values
which can expose sensitive query parameters from upstream applications. In the
error logging statements at lines involving redirect URI validation (around the
"Failed to parse redirect URI" log and similar rejection paths at lines 322 and
334), replace the logging of the complete redirectURI variable with safer
alternatives such as logging only the URI scheme and host components or the
length of the URI. This prevents sensitive data in query parameters from being
exposed in logs while still providing useful debugging information.
- Around line 326-331: The url.Parse() call for controller.runtime.AppURL can
succeed on malformed URLs without proper scheme and host validation. After the
error check for the url.Parse() call on au, add explicit validation to ensure
both the scheme and host are not empty, matching the validation pattern used for
redirectURI validation (which checks these fields around lines 297-305). This
validation should occur before the parsed URL is used downstream, and should
return false if either the scheme or host is missing or invalid. Additionally,
add a test case covering the malformed URL scenario where appURL is
"https:/example.com" (single slash) and redirectURI is
"https://sub.example.com", which should evaluate to false.
In `@internal/service/auth_service.go`:
- Around line 708-713: The getCookieDomain methods in both AuthService and
OAuthController do not correctly enforce cookie domain isolation. When
SubdomainsEnabled is false, the methods should return an empty string (not the
domain value) to create a host-only cookie that restricts to the current host
only, which matches the documented behavior. When SubdomainsEnabled is true,
remove the leading dot before the domain since RFC 6265 specifies that leading
dots are ignored. Apply these changes to both AuthService.getCookieDomain() and
OAuthController.getCookieDomain() methods.
In `@internal/utils/app_utils.go`:
- Around line 32-54: The function currently returns `u.Host` which includes port
numbers, violating RFC 6265 for Set-Cookie Domain attributes and causing
frontend redirect validation failures. Additionally, the manual `parts[1:]`
slicing incorrectly handles multi-label public suffixes by stripping too many
parts (e.g., `example.co.uk` becomes `co.uk`). Replace all references to
`u.Host` with `u.Hostname()` to eliminate ports, and use the domain value
returned directly from `publicsuffix.DomainFromListWithOptions` instead of
manually reconstructing it from sliced parts. Update the test cases to expect
port-free results and to correctly validate multi-label public suffix domains as
acceptable cookie domains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 31468088-60a0-4afc-92e4-10b80af1211d
📒 Files selected for processing (17)
frontend/src/components/layout/layout.tsxfrontend/src/lib/hooks/redirect-uri.tsfrontend/src/pages/continue-page.tsxfrontend/src/schemas/app-context-schema.tsinternal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/controller/context_controller.gointernal/controller/context_controller_test.gointernal/controller/oauth_controller.gointernal/controller/oauth_controller_test.gointernal/model/config.gointernal/model/runtime.gointernal/service/auth_service.gointernal/service/tailscale_service.gointernal/test/test.gointernal/utils/app_utils.gointernal/utils/app_utils_test.go
💤 Files with no reviewable changes (1)
- internal/model/runtime.go
Comment on lines
+284
to
+298
| // force tailscale app url if listening on a tailscale address | ||
| if app.services.tailscaleService != nil && app.config.Tailscale.Listen { | ||
| tailscaleUrl := "https://" + app.services.tailscaleService.GetHostname() | ||
| if tailscaleUrl != app.runtime.AppURL { | ||
| app.log.App.Info().Msg("Listening on tailscale, replacing app url with tailscale hostname") | ||
| app.runtime.AppURL = tailscaleUrl | ||
| // also update cookie domain | ||
| cookieDomain, err := utils.GetCookieDomain(tailscaleUrl, app.config.Auth.SubdomainsEnabled) | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to get cookie domain: %w", err) | ||
| } | ||
|
|
||
| app.runtime.CookieDomain = cookieDomain | ||
| } |
Contributor
There was a problem hiding this comment.
Refresh default OAuth callback URLs after replacing AppURL.
Default provider RedirectURLs are assigned earlier from the original runtime.AppURL at Lines 145-147. When Lines 284-298 replace runtime.AppURL for Tailscale listening, providers that relied on the default callback keep pointing at the old host, so OAuth callbacks can target an address this instance no longer serves.
Possible fix direction
+defaultedRedirectURLs := make(map[string]bool)
for id, provider := range app.runtime.OAuthProviders {
...
if provider.RedirectURL == "" {
+ defaultedRedirectURLs[id] = true
provider.RedirectURL = app.runtime.AppURL + "/api/oauth/callback/" + id
}
...
}
...
if tailscaleUrl != app.runtime.AppURL {
app.log.App.Info().Msg("Listening on tailscale, replacing app url with tailscale hostname")
app.runtime.AppURL = tailscaleUrl
...
app.runtime.CookieDomain = cookieDomain
+
+ for id, provider := range app.runtime.OAuthProviders {
+ if defaultedRedirectURLs[id] {
+ provider.RedirectURL = app.runtime.AppURL + "/api/oauth/callback/" + id
+ app.runtime.OAuthProviders[id] = provider
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/bootstrap/app_bootstrap.go` around lines 284 - 298, After updating
app.runtime.AppURL and app.runtime.CookieDomain in the Tailscale listening
block, you must also refresh the OAuth provider RedirectURL values that were
initially assigned earlier based on the original AppURL. Locate where the
default provider RedirectURLs are set (around lines 145-147 mentioned in the
comment) and add code in the Tailscale block to re-assign these provider
redirect URLs using the new tailscaleUrl value, ensuring OAuth callbacks
correctly target the Tailscale hostname instead of the previously configured
host.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/oauth_controller.go`:
- Around line 338-341: The port comparison in the conditional at the Port()
method calls fails because url.Parse returns an empty string for Port() when no
explicit port is specified, causing URLs like "https://example.com" (empty port
string) to incorrectly not match "https://example.com:443" (port "443"). Fix
this by normalizing the ports explicitly: extract the port using Port() for both
u and au, and if the port is empty, assign the default port based on the URL
scheme (443 for https, 80 for http), then compare the normalized ports.
Additionally, also compare the Hostname() values to ensure the hosts match as
well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3345f794-cd8b-4e20-b56c-7fc8f6e434ec
📒 Files selected for processing (5)
internal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/controller/oauth_controller.gointernal/service/auth_service.gointernal/service/tailscale_service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/bootstrap/app_bootstrap.go
- internal/bootstrap/router_bootstrap.go
Comment on lines
+338
to
+341
| if u.Port() != au.Port() { | ||
| controller.log.App.Warn().Msg("Redirect URI port does not match app URL port") | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
Port comparison fails for default vs explicit port.
url.Parse returns an empty string for Port() when the port is not explicitly specified in the URL. Comparing "https://example.com" (port = "") against "https://example.com:443" (port = "443") will fail even though they resolve to the same endpoint.
Use Hostname() for host comparison and normalize ports explicitly:
Proposed fix
- if u.Port() != au.Port() {
- controller.log.App.Warn().Msg("Redirect URI port does not match app URL port")
- return false
- }
-
- if strings.EqualFold(u.Host, au.Host) {
+ // Normalize ports: empty means default (443 for https, 80 for http)
+ getEffectivePort := func(u *url.URL) string {
+ if p := u.Port(); p != "" {
+ return p
+ }
+ if u.Scheme == "https" {
+ return "443"
+ }
+ return "80"
+ }
+
+ if getEffectivePort(u) != getEffectivePort(au) {
+ controller.log.App.Warn().Msg("Redirect URI port does not match app URL port")
+ return false
+ }
+
+ if strings.EqualFold(u.Hostname(), au.Hostname()) {
return true
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/oauth_controller.go` around lines 338 - 341, The port
comparison in the conditional at the Port() method calls fails because url.Parse
returns an empty string for Port() when no explicit port is specified, causing
URLs like "https://example.com" (empty port string) to incorrectly not match
"https://example.com:443" (port "443"). Fix this by normalizing the ports
explicitly: extract the port using Port() for both u and au, and if the port is
empty, assign the default port based on the URL scheme (443 for https, 80 for
http), then compare the normalized ports. Additionally, also compare the
Hostname() values to ensure the hosts match as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements