feat: tailscale integration#847
Conversation
📝 WalkthroughWalkthroughAdds Tailscale authentication and tsnet support, restructures user/app contexts to nested shapes across frontend and backend, refactors bootstrap/router to run HTTP/Unix/Tailscale listeners, enriches middleware with Tailscale WHOIS, and updates pages, controllers, schemas, i18n, and dependencies. ChangesTailscale Auth & Context Restructuring
Sequence DiagramssequenceDiagram
participant User
participant LoginPage
participant Mutation as tailscaleMutate
participant API as POST /user/tailscale
participant Router
User->>LoginPage: Click Tailscale login
LoginPage->>LoginPage: Set useTailscale=true
User->>LoginPage: Submit device name
LoginPage->>Mutation: Invoke mutation
Mutation->>API: POST with Tailscale context
alt Success
API-->>Mutation: 200 OK
Mutation->>Router: Navigate /authorize or /continue
else Failure
API-->>Mutation: error response
Mutation->>LoginPage: Show error toast
end
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 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! |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/auth_service.go (1)
325-355:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOrphaned session row when Tailscale precondition fails.
auth.queries.CreateSession(ctx, session)writes the row at line 325, before the Tailscale-specific validation on lines 331–342. Ifauth.tailscale == nilorutils.GetCookieDomainfails, the function returns an error, but the session row remains in the database (untildbCleanupRoutineeventually purges it at expiry). The caller never gets the UUID, so the row is unreachable garbage.Either validate the precondition before the DB write, or delete the session on the error path.
🛠️ Suggested fix: validate before persisting
+ var tsCookieDomain string + if data.Provider == "tailscale" { + if auth.tailscale == nil { + return nil, fmt.Errorf("tailscale service not configured, cannot create session for tailscale user") + } + + auth.log.App.Trace().Str("url", fmt.Sprintf("https://%s", auth.tailscale.GetHostname())).Msg("Extracting root domain from Tailscale hostname") + + var err error + tsCookieDomain, err = utils.GetCookieDomain(fmt.Sprintf("https://%s", auth.tailscale.GetHostname())) + if err != nil { + return nil, fmt.Errorf("failed to get cookie domain for tailscale user: %w", err) + } + } + _, err = auth.queries.CreateSession(ctx, session) if err != nil { return nil, fmt.Errorf("failed to create session entry: %w", err) } if data.Provider == "tailscale" { - if auth.tailscale == nil { - return nil, fmt.Errorf("tailscale service not configured, cannot create session for tailscale user") - } - - auth.log.App.Trace().Str("url", fmt.Sprintf("https://%s", auth.tailscale.GetHostname())).Msg("Extracting root domain from Tailscale hostname") - - tsCookieDomain, err := utils.GetCookieDomain(fmt.Sprintf("https://%s", auth.tailscale.GetHostname())) - - if err != nil { - return nil, fmt.Errorf("failed to get cookie domain for tailscale user: %w", err) - } - return &http.Cookie{🤖 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/service/auth_service.go` around lines 325 - 355, Move or guard the Tailscale-specific preconditions so we don't persist an unreachable session: validate auth.tailscale != nil and call utils.GetCookieDomain(fmt.Sprintf("https://%s", auth.tailscale.GetHostname())) (or otherwise compute tsCookieDomain) before calling auth.queries.CreateSession(ctx, session); alternatively, if you prefer to keep CreateSession in place, ensure you delete the created session on any subsequent error path (call the session delete query using session.UUID) before returning the error so CreateSession does not leave an orphaned row.
🧹 Nitpick comments (6)
internal/model/config.go (1)
64-66: 💤 Low valueConsider documenting the relative path default.
The default
Dir: "./tailscale_state"uses a relative path, which is created relative to the working directory. In containerized environments or when the app is started from different directories, this could lead to inconsistent state directory locations.Consider documenting this behavior in the config description or using an absolute path strategy (e.g., based on a config directory or user's home directory).
🤖 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/model/config.go` around lines 64 - 66, The default Tailscale state directory is set to a relative path (TailscaleConfig.Dir = "./tailscale_state"), which can vary by working directory; update the config to either document this behavior in the TailscaleConfig comment/struct docstring or change the default to an absolute, deterministic location (e.g., derive from os.UserConfigDir or the app’s config dir via filepath.Join) and ensure any code that creates/uses Dir handles expansion and creation. Reference the TailscaleConfig struct and its Dir field when making the change so callers and config docs reflect the new default and behavior.internal/service/tailscale_service.go (1)
113-123: 💤 Low valueConsider simplifying listener storage.
The code stores a pointer to a
net.Listener(ln *net.Listeneron line 26, thents.ln = &lnon line 121). This is unconventional—typically you'd store the listener directly asln net.Listener. While the current pattern works, it adds unnecessary indirection.♻️ Proposed refactor to simplify listener storage
type TailscaleService struct { log *logger.Logger wg *sync.WaitGroup config model.Config ctx context.Context srv *tsnet.Server lc *local.Client - ln *net.Listener + ln net.Listener }Then update the methods:
func (ts *TailscaleService) watchAndClose() { <-ts.ctx.Done() ts.log.App.Debug().Msg("Shutting down Tailscale service") if ts.ln != nil { - (*ts.ln).Close() + ts.ln.Close() } if ts.srv != nil { ts.srv.Close() } } func (ts *TailscaleService) CreateListener() (net.Listener, error) { if ts.ln != nil { - return *ts.ln, nil + return ts.ln, nil } ln, err := ts.srv.ListenTLS("tcp", ":443") if err != nil { return nil, err } - ts.ln = &ln + ts.ln = ln return ln, nil }🤖 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/service/tailscale_service.go` around lines 113 - 123, Change the TailscaleService listener field from a pointer to a plain interface and remove pointer indirection: update the struct field ln from *net.Listener to net.Listener, then modify CreateListener to return ts.ln directly (if not nil) and assign ts.ln = ln (no &). Also update any other methods that reference ts.ln (e.g., CloseListener, any getters) to treat ln as a net.Listener (nil-check and direct use) instead of *net.Listener so all call sites compile.internal/service/auth_service.go (1)
331-355: 💤 Low valueConsider caching the Tailscale cookie domain.
auth.tailscale.GetHostname()andutils.GetCookieDomain(...)are evaluated on every Tailscale session creation, but the hostname is fixed for the process lifetime (it's the tsnet hostname configured at bootstrap). Computing this once at startup — for example, by storing atailscaleCookieDomainonAuthServiceor surfacing it onruntimealongsideCookieDomain— removes the per-request URL parse and aligns with how the non-Tailscale branch readsauth.runtime.CookieDomaindirectly.Also consider replacing the
"tailscale"string literal with a typed constant (e.g., addProviderTailscalenext toProviderOAuthininternal/model) to avoid stringly-typed comparisons across the codebase.🤖 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/service/auth_service.go` around lines 331 - 355, Cache the Tailscale cookie domain and replace the string literal: compute utils.GetCookieDomain(fmt.Sprintf("https://%s", auth.tailscale.GetHostname())) once at startup and store it (e.g., add a tailscaleCookieDomain field on AuthService or expose it on runtime alongside CookieDomain) so the Tailscale branch in the session-creation code uses that cached value instead of calling auth.tailscale.GetHostname() and utils.GetCookieDomain() per request; additionally, introduce a typed constant ProviderTailscale (e.g., in internal/model next to ProviderOAuth) and replace the `"tailscale"` string comparison in the if statement with that constant to avoid stringly-typed checks.frontend/src/pages/login-page.tsx (1)
49-49: ⚡ Quick winAvoid the
use*prefix for non-hook state.
useTailscalefollows the naming convention reserved for React hooks (thereact-hooks/rules-of-hooksESLint rule treats anyuse*identifier as a hook call). It works today only because it's never invoked, but it's misleading to readers and tooling — a future refactor that callsuseTailscale()would silently violate hook rules.Suggest renaming to something like
tailscaleSelected,showTailscaleCard, orisTailscaleMode.♻️ Proposed rename
- const [useTailscale, setUseTailscale] = useState(tailscale.nodeName !== ""); + const [showTailscaleCard, setShowTailscaleCard] = useState( + tailscale.nodeName !== "", + );Then update the usages at lines 263 and 291 accordingly.
🤖 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 `@frontend/src/pages/login-page.tsx` at line 49, The state variable useTailscale should be renamed to avoid the `use*` hook naming pattern: rename `useTailscale` to `isTailscaleMode` (or `tailscaleSelected`) and `setUseTailscale` to `setIsTailscaleMode`, update the useState declaration accordingly, and replace all usages of `useTailscale` and `setUseTailscale` (including the occurrences referenced in the review at the current usages around the former lines 263 and 291) so the component compiles and ESLint no longer flags a hook-name false positive.internal/bootstrap/router_bootstrap.go (1)
69-86: 💤 Low valueMinor: consider surfacing the listener type in error reporting.
If multiple listeners run concurrently and one fails, the consumer of
leconly sees the wrapped error fromserveHTTP/serveUnix/serveTailscale. Those already include"http"/"unix socket"/"tailscale"in the message viaserve(...), so this is mostly fine — but the surroundingfor { select case err := <-lec ... }loop inSetup()returns on the first non-nil error, which means the operator only sees one listener's failure even if several are misconfigured. Logging non-nil errors fromlecinstead of (or before) returning may be friendlier for diagnosing concurrent-listener setups.🤖 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/router_bootstrap.go` around lines 69 - 86, runListeners currently pushes raw errors from listenerFunc() onto lec, which loses which listenerType failed; update runListeners (and keep using listenerFromType) to wrap the returned error with the listenerType before sending it on lec (e.g. if err := listenerFunc(); err != nil { lec <- fmt.Errorf("listener %s: %w", listenerType, err) } else { lec <- nil }); additionally, in Setup's for/select loop that reads from lec, log non-nil errors (including the wrapped listener type) before returning so multiple concurrent listener failures are easier to diagnose.internal/controller/context_controller.go (1)
103-112: ⚡ Quick winDon’t log expected anonymous
/context/userrequests as errors.This path now intentionally returns a normal unauthenticated payload, so
ErrUserContextNotFoundshould be logged at debug/info instead of error to avoid noisy telemetry.💡 Possible adjustment
func (controller *ContextController) userContextHandler(c *gin.Context) { context, err := new(model.UserContext).NewFromGin(c) if err != nil { - controller.log.App.Error().Err(err).Msg("Failed to create user context from request") + if errors.Is(err, model.ErrUserContextNotFound) { + controller.log.App.Debug().Err(err).Msg("No authenticated user context on request") + } else { + controller.log.App.Error().Err(err).Msg("Failed to create user context from request") + } c.JSON(200, UserContextResponse{ Status: 401, Message: "Unauthorized", Auth: UCRAuth{Authenticated: false}, })You would also need to add
import "errors"above.🤖 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/context_controller.go` around lines 103 - 112, The handler ContextController.userContextHandler is currently logging all NewFromGin errors as controller.log.App.Error(), which unnecessarily treats expected anonymous requests (ErrUserContextNotFound) as errors; change the error handling to check for errors.Is(err, model.ErrUserContextNotFound) and log at Debug or Info instead (e.g., controller.log.App.Debug()/Info().Err(err).Msg(...)), while keeping other errors logged as Error and returning the same 401 payload; also add import "errors" to enable errors.Is.
🤖 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/bootstrap/router_bootstrap.go`:
- Around line 200-220: The shutdown handler in serve uses app.ctx (already
canceled) so server.Shutdown returns immediately; change shutdown to create a
fresh context with a bounded timeout (e.g., context.WithTimeout) before calling
server.Shutdown and defer cancel; do this both where the goroutine triggers
shutdown and in the error path where shutdown() is called; update the serve
function (referencing serve, shutdown, app.ctx, server, listener) to build a new
context (importing "context" and "time") and use it for Shutdown, then call
listener.Close() after Shutdown returns or times out.
In `@internal/controller/user_controller.go`:
- Around line 396-444: tailscaleHandler lacks the same rate-limiting and
account-lockout checks applied in loginHandler; add the same pre-session checks
and post-failure accounting used by loginHandler: call the shared
rate-limit/lockout check (the same function or middleware invoked in
loginHandler) using context.GetUsername() before creating the session, return
the same 401/429 responses when limit or lockout is hit, increment the
failed-attempt counter and record lockout state on failures, and emit the same
audit events (e.g., AuditLoginAttempt/AuditLockout or the equivalent logging
helpers used by loginHandler) so Tailscale logins follow the same throttling,
lockout, and audit behavior.
- Around line 396-444: In tailscaleHandler, update the erroneous log message
that references "TOTP" when CreateSession fails: locate the error branch where
controller.auth.CreateSession(c, sessionCookie) returns an error and change the
message in controller.log.App.Error().Err(err).Str("username",
context.GetUsername()).Msg(...) to say something like "Failed to create session
cookie after Tailscale login" (or similar) so it correctly references Tailscale
instead of TOTP.
In `@internal/middleware/context_middleware.go`:
- Around line 117-120: The anonymous Tailscale context is being attached without
setting the provider, leaving model.UserContext.Provider at its zero value
(ProviderLocal); update the c.Set call that constructs &model.UserContext (in
context_middleware.go) to set Provider: model.ProviderTailscale when Tailscale
is used so downstream helpers see the correct provider and return Tailscale base
auth fields.
---
Outside diff comments:
In `@internal/service/auth_service.go`:
- Around line 325-355: Move or guard the Tailscale-specific preconditions so we
don't persist an unreachable session: validate auth.tailscale != nil and call
utils.GetCookieDomain(fmt.Sprintf("https://%s", auth.tailscale.GetHostname()))
(or otherwise compute tsCookieDomain) before calling
auth.queries.CreateSession(ctx, session); alternatively, if you prefer to keep
CreateSession in place, ensure you delete the created session on any subsequent
error path (call the session delete query using session.UUID) before returning
the error so CreateSession does not leave an orphaned row.
---
Nitpick comments:
In `@frontend/src/pages/login-page.tsx`:
- Line 49: The state variable useTailscale should be renamed to avoid the `use*`
hook naming pattern: rename `useTailscale` to `isTailscaleMode` (or
`tailscaleSelected`) and `setUseTailscale` to `setIsTailscaleMode`, update the
useState declaration accordingly, and replace all usages of `useTailscale` and
`setUseTailscale` (including the occurrences referenced in the review at the
current usages around the former lines 263 and 291) so the component compiles
and ESLint no longer flags a hook-name false positive.
In `@internal/bootstrap/router_bootstrap.go`:
- Around line 69-86: runListeners currently pushes raw errors from
listenerFunc() onto lec, which loses which listenerType failed; update
runListeners (and keep using listenerFromType) to wrap the returned error with
the listenerType before sending it on lec (e.g. if err := listenerFunc(); err !=
nil { lec <- fmt.Errorf("listener %s: %w", listenerType, err) } else { lec <-
nil }); additionally, in Setup's for/select loop that reads from lec, log
non-nil errors (including the wrapped listener type) before returning so
multiple concurrent listener failures are easier to diagnose.
In `@internal/controller/context_controller.go`:
- Around line 103-112: The handler ContextController.userContextHandler is
currently logging all NewFromGin errors as controller.log.App.Error(), which
unnecessarily treats expected anonymous requests (ErrUserContextNotFound) as
errors; change the error handling to check for errors.Is(err,
model.ErrUserContextNotFound) and log at Debug or Info instead (e.g.,
controller.log.App.Debug()/Info().Err(err).Msg(...)), while keeping other errors
logged as Error and returning the same 401 payload; also add import "errors" to
enable errors.Is.
In `@internal/model/config.go`:
- Around line 64-66: The default Tailscale state directory is set to a relative
path (TailscaleConfig.Dir = "./tailscale_state"), which can vary by working
directory; update the config to either document this behavior in the
TailscaleConfig comment/struct docstring or change the default to an absolute,
deterministic location (e.g., derive from os.UserConfigDir or the app’s config
dir via filepath.Join) and ensure any code that creates/uses Dir handles
expansion and creation. Reference the TailscaleConfig struct and its Dir field
when making the change so callers and config docs reflect the new default and
behavior.
In `@internal/service/auth_service.go`:
- Around line 331-355: Cache the Tailscale cookie domain and replace the string
literal: compute utils.GetCookieDomain(fmt.Sprintf("https://%s",
auth.tailscale.GetHostname())) once at startup and store it (e.g., add a
tailscaleCookieDomain field on AuthService or expose it on runtime alongside
CookieDomain) so the Tailscale branch in the session-creation code uses that
cached value instead of calling auth.tailscale.GetHostname() and
utils.GetCookieDomain() per request; additionally, introduce a typed constant
ProviderTailscale (e.g., in internal/model next to ProviderOAuth) and replace
the `"tailscale"` string comparison in the if statement with that constant to
avoid stringly-typed checks.
In `@internal/service/tailscale_service.go`:
- Around line 113-123: Change the TailscaleService listener field from a pointer
to a plain interface and remove pointer indirection: update the struct field ln
from *net.Listener to net.Listener, then modify CreateListener to return ts.ln
directly (if not nil) and assign ts.ln = ln (no &). Also update any other
methods that reference ts.ln (e.g., CloseListener, any getters) to treat ln as a
net.Listener (nil-check and direct use) instead of *net.Listener so all call
sites compile.
🪄 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: 486730b4-a557-4c9b-bfe2-8552c62a905a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
frontend/src/App.tsxfrontend/src/components/layout/layout.tsxfrontend/src/lib/i18n/locales/en-US.jsonfrontend/src/lib/i18n/locales/en.jsonfrontend/src/pages/authorize-page.tsxfrontend/src/pages/continue-page.tsxfrontend/src/pages/forgot-password-page.tsxfrontend/src/pages/login-page.tsxfrontend/src/pages/logout-page.tsxfrontend/src/pages/totp-page.tsxfrontend/src/schemas/app-context-schema.tsfrontend/src/schemas/user-context-schema.tsgo.modinternal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/controller/context_controller.gointernal/controller/context_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller.gointernal/controller/user_controller_test.gointernal/middleware/context_middleware.gointernal/middleware/context_middleware_test.gointernal/model/config.gointernal/model/context.gointernal/model/runtime.gointernal/service/auth_service.gointernal/service/tailscale_service.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/model/constants.go (1)
25-25: ⚡ Quick winUse
time.Durationtype for type safety.The constant is defined as an untyped integer with a comment indicating seconds. Go best practice is to use
time.Durationfor all time-related values to prevent confusion about units and enable type checking. Currently, the constant is multiplied bytime.Secondat the usage site ininternal/bootstrap/router_bootstrap.go:205(model.GracefulShutdownTimeout*time.Second). Usingtime.Durationwould eliminate this multiplication and make the code clearer.Proposed change
Add the
timeimport at the top of the file, then update the constant:const GracefulShutdownTimeout = 5 * time.SecondThis would simplify the usage to just
model.GracefulShutdownTimeoutwithout needing the*time.Secondmultiplication.🤖 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/model/constants.go` at line 25, The constant GracefulShutdownTimeout is an untyped integer representing seconds; change it to a time.Duration to enforce units and remove callers' need to multiply by time.Second. Add the time import in internal/model/constants.go and set GracefulShutdownTimeout to a duration (e.g., 5 * time.Second) so call sites (e.g., where model.GracefulShutdownTimeout is used in shutdown logic) can use it directly without multiplying by time.Second. Ensure existing usages compile by removing any manual *time.Second multiplications.
🤖 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.
Nitpick comments:
In `@internal/model/constants.go`:
- Line 25: The constant GracefulShutdownTimeout is an untyped integer
representing seconds; change it to a time.Duration to enforce units and remove
callers' need to multiply by time.Second. Add the time import in
internal/model/constants.go and set GracefulShutdownTimeout to a duration (e.g.,
5 * time.Second) so call sites (e.g., where model.GracefulShutdownTimeout is
used in shutdown logic) can use it directly without multiplying by time.Second.
Ensure existing usages compile by removing any manual *time.Second
multiplications.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 78f27174-6610-4136-914e-c50011ad63a9
📒 Files selected for processing (6)
internal/bootstrap/router_bootstrap.gointernal/controller/user_controller.gointernal/middleware/context_middleware.gointernal/model/constants.gointernal/service/auth_service.gointernal/service/tailscale_service.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/middleware/context_middleware.go
- internal/controller/user_controller.go
- internal/service/tailscale_service.go
- internal/bootstrap/router_bootstrap.go
- internal/service/auth_service.go
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Refactor
Localization