refactor: remove concurrent listeners and rework cookie logic#950
Merged
Conversation
dosubot
Bot
added
the
size:L
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the explicit ChangesSubdomain Trust Model Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant ContinuePage
participant useRedirectUri
participant OAuthController as isRedirectSafe
Client->>ContinuePage: OAuth callback with redirect_uri
ContinuePage->>useRedirectUri: useRedirectUri(redirect_uri, cookieDomain,<br/>appUrl, subdomainsEnabled)
useRedirectUri->>useRedirectUri: isTrustedDomain check<br/>parse both URLs, verify scheme,<br/>verify port, check hostname
useRedirectUri-->>ContinuePage: {valid, trusted}
ContinuePage->>OAuthController: isRedirectSafe(redirectURI)<br/>backend validation
OAuthController->>OAuthController: parse URLs, enforce<br/>scheme+port, allow exact<br/>host or SubdomainsEnabled suffix
OAuthController-->>ContinuePage: safe or unsafe
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 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
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
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/service/auth_service.go`:
- Around line 547-558: The svc.UpdateConfig(cfg) call is currently executed
unconditionally on every invocation, causing unnecessary writes to a shared
service instance. Move the svc.UpdateConfig(cfg) call inside the if
cfg.RedirectURL == "" conditional block so that the config is only persisted
when the RedirectURL was actually missing and needed to be set. This ensures
updates only happen when the configuration has been modified, reducing
concurrent write pressure on the shared service instance.
🪄 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: f828b36a-1bed-4f2f-a79c-eeb5b91ec816
📒 Files selected for processing (8)
internal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/controller/context_controller.gointernal/controller/oauth_controller.gointernal/controller/user_controller.gointernal/service/auth_service.gointernal/service/oauth_broker_service.gointernal/service/oauth_service.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/controller/oauth_controller.go
- internal/bootstrap/app_bootstrap.go
- internal/bootstrap/router_bootstrap.go
dosubot
Bot
added
size:XL
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR refactors runtime listener selection to a single active mode (Tailscale, Unix socket, or HTTP), and reworks redirect/cookie-domain handling to be driven by the configured App URL plus the subdomainsEnabled setting instead of a trusted-domains list. It also adds Tailscale Funnel listener support and aligns the frontend app-context/redirect validation logic with the backend.
Changes:
- Simplify server startup to choose exactly one listener mode and add Tailscale Funnel listener support.
- Replace trusted-domain redirect allowlisting with scheme/host/port checks against
AppURL, plus conditional subdomain allowance based oncookieDomainandsubdomainsEnabled. - Rework cookie domain resolution + app context payload/schema to support subdomain-enabled vs host-only cookie behavior.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/utils/app_utils.go | Refactors cookie-domain derivation into a single function that accounts for subdomain enablement. |
| internal/utils/app_utils_test.go | Updates tests for new cookie-domain behavior/signature and adds new cases. |
| internal/test/test.go | Updates test config defaults to include SubdomainsEnabled and removes TrustedDomains. |
| internal/service/tailscale_service.go | Adds Funnel mode warning and listener creation via ListenFunnel when enabled. |
| internal/service/oauth_service.go | Adds config getter/updater to allow redirect URL updates post-construction. |
| internal/service/oauth_broker_service.go | Renames/extends OAuth service interface to support runtime config updates. |
| internal/service/auth_service.go | Removes pointer-to-interface usage for OAuth sessions and centralizes cookie-domain selection. |
| internal/model/runtime.go | Removes TrustedDomains from runtime config. |
| internal/model/config.go | Removes concurrent listener config, adds tailscale.funnel and tailscale.listen. |
| internal/controller/user_controller.go | Returns HTTP 401 for missing user context during TOTP/Tailscale auth flows. |
| internal/controller/oauth_controller.go | Reworks cookie domain behavior and strengthens redirect URI validation. |
| internal/controller/oauth_controller_test.go | Replaces trusted-domains tests with AppURL/cookieDomain/subdomain-driven redirect validation tests. |
| internal/controller/context_controller.go | App context response replaces trustedDomains with subdomainsEnabled; reduces logging for missing context. |
| internal/controller/context_controller_test.go | Updates expected app context response for new schema. |
| internal/bootstrap/router_bootstrap.go | Removes concurrent listener machinery; introduces single listener selection. |
| internal/bootstrap/app_bootstrap.go | Normalizes AppURL, computes cookie domain with subdomain toggle, updates OAuth redirect URLs, and selects one listener. |
| frontend/src/schemas/app-context-schema.ts | Updates schema to replace trustedDomains with subdomainsEnabled. |
| frontend/src/pages/continue-page.tsx | Passes AppURL + subdomain flag into redirect validation and adjusts subtitle formatting. |
| frontend/src/lib/hooks/redirect-uri.ts | Ports backend redirect safety logic (scheme/port/host + conditional subdomain allowance). |
| frontend/src/components/layout/layout.tsx | Replaces trusted-domains UI warning gating with AppURL-based gating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scottmckendry
previously approved these changes
Jun 22, 2026
scottmckendry
left a comment
scottmckendry
left a comment
Member
There was a problem hiding this comment.
more of a question rather than anything that needs to be changed. Ping me if you need a fresh review 🚀
scottmckendry
approved these changes
Jun 22, 2026
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
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
Bug Fixes