Skip to content

feat: tailscale integration#847

Open
steveiliop56 wants to merge 14 commits into
mainfrom
feat/tailscale
Open

feat: tailscale integration#847
steveiliop56 wants to merge 14 commits into
mainfrom
feat/tailscale

Conversation

@steveiliop56
Copy link
Copy Markdown
Member

@steveiliop56 steveiliop56 commented May 10, 2026


View in Codesmith
Need help on this PR? Tag @codesmith with what you need.

  • Let Codesmith autofix CI failures and bot reviews

Summary by CodeRabbit

  • New Features

    • Added Tailscale authentication flow, server-side Tailscale listener, and multi-listener support (HTTP, Unix socket, Tailscale).
    • New Tailscale-based WHOIS lookup for implicit user context and a POST endpoint to complete Tailscale logins.
  • Refactor

    • Reworked client/server context formats into nested auth/oauth/ui/app structures and updated routing/auth gating to use the new auth state.
    • Improved domain trust/warning logic and cookie domain handling.
  • Localization

    • Added English strings for Tailscale login/logout flows.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 10, 2026

📝 Walkthrough

Walkthrough

Adds 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.

Changes

Tailscale Auth & Context Restructuring

Layer / File(s) Summary
Frontend Context Schemas
frontend/src/schemas/user-context-schema.ts, frontend/src/schemas/app-context-schema.ts
User and app context schemas refactored from flat fields to nested groups: auth, oauth, ui, app, totp, tailscale.
Backend Model Types
internal/model/context.go, internal/model/config.go, internal/model/runtime.go
New ProviderTailscale constant, TailscaleContext and TailscaleConfig types added; UserContext extended with Tailscale provider support and IsTailscale(), TailscaleNodeName() accessors; RuntimeConfig gains TrustedDomains.
Tailscale Service
internal/service/tailscale_service.go
New service wraps Tailscale tsnet client; exposes Whois, CreateListener, GetHostname; initializes with config-driven options, waits for IP connectivity, handles graceful shutdown.
Bootstrap & Listener Orchestration
internal/bootstrap/app_bootstrap.go, internal/bootstrap/router_bootstrap.go, internal/bootstrap/service_bootstrap.go
TailscaleService initialized during service bootstrap; listener policy computed via calculateListenerPolicy and started concurrently by runListeners; separate handlers for HTTP/Unix/Tailscale listeners; TrustedDomains extended with app URL and Tailscale hostname.
Context Middleware
internal/middleware/context_middleware.go, internal/middleware/context_middleware_test.go
ContextMiddleware wired with TailscaleService; cookieAuth accepts client IP and supports Tailscale WHOIS lookup; new tailscaleWhois helper normalizes email and maps WHOIS results to TailscaleContext; fallback WHOIS enrichment when no session auth found.
API Controllers
internal/controller/context_controller.go, internal/controller/user_controller.go, internal/controller/context_controller_test.go, internal/controller/proxy_controller_test.go, internal/controller/user_controller_test.go
Context endpoint schemas restructured to nested Auth/OAuth/UI/App with sub-types; new POST /user/tailscale endpoint for Tailscale login; AuthService extended with Tailscale field and CreateSession special-cases Tailscale provider for hostname-derived cookie domain; tests updated for nested schema expectations.
Frontend Pages
frontend/src/App.tsx, frontend/src/pages/login-page.tsx, frontend/src/pages/logout-page.tsx, frontend/src/pages/authorize-page.tsx, frontend/src/pages/continue-page.tsx, frontend/src/pages/forgot-password-page.tsx, frontend/src/pages/totp-page.tsx, frontend/src/components/layout/layout.tsx
All pages updated to destructure nested context; LoginPage adds useTailscale state and tailscaleMutate mutation for POST /api/user/tailscale with post-login routing; LogoutPage splits rendering into OAuth, Tailscale, and default username branches; Layout uses app.trustedDomains and ui.warningsEnabled for domain warning logic.
Translations & Dependencies
frontend/src/lib/i18n/locales/en.json, frontend/src/lib/i18n/locales/en-US.json, go.mod
i18n locales extended with Tailscale login/logout strings (loginTailscaleTitle, loginTailscaleDescription, logoutTailscaleSubtitle with {{deviceName}}); Go toolchain bumped to 1.26.1; tailscale.com v1.96.5 added to main requires with updated indirect dependencies.

Sequence Diagrams

sequenceDiagram
  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
Loading

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly Related PRs

Suggested Labels

lgtm

Suggested Reviewers

  • Rycochet
  • scottmckendry

Poem

🐰 I hopped through code with nimble feet,
Added tails and made contexts neat.
Listeners hum and WHOIS calls sing,
Trusted domains now wear a ring.
Tiny rabbit says: "Auth—what a treat!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat: tailscale integration' accurately summarizes the main change in the changeset, which adds comprehensive Tailscale authentication support across frontend and backend components.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/tailscale

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 10, 2026

Comment thread frontend/src/pages/login-page.tsx Outdated
Comment thread frontend/src/lib/i18n/locales/en-US.json Outdated
@steveiliop56 steveiliop56 marked this pull request as ready for review May 11, 2026 15:49
@dosubot dosubot Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label May 11, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Orphaned 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. If auth.tailscale == nil or utils.GetCookieDomain fails, the function returns an error, but the session row remains in the database (until dbCleanupRoutine eventually 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 value

Consider 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 value

Consider simplifying listener storage.

The code stores a pointer to a net.Listener (ln *net.Listener on line 26, then ts.ln = &ln on line 121). This is unconventional—typically you'd store the listener directly as ln 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 value

Consider caching the Tailscale cookie domain.

auth.tailscale.GetHostname() and utils.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 a tailscaleCookieDomain on AuthService or surfacing it on runtime alongside CookieDomain — removes the per-request URL parse and aligns with how the non-Tailscale branch reads auth.runtime.CookieDomain directly.

Also consider replacing the "tailscale" string literal with a typed constant (e.g., add ProviderTailscale next to ProviderOAuth in internal/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 win

Avoid the use* prefix for non-hook state.

useTailscale follows the naming convention reserved for React hooks (the react-hooks/rules-of-hooks ESLint rule treats any use* 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 calls useTailscale() would silently violate hook rules.

Suggest renaming to something like tailscaleSelected, showTailscaleCard, or isTailscaleMode.

♻️ 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 value

Minor: consider surfacing the listener type in error reporting.

If multiple listeners run concurrently and one fails, the consumer of lec only sees the wrapped error from serveHTTP/serveUnix/serveTailscale. Those already include "http"/"unix socket"/"tailscale" in the message via serve(...), so this is mostly fine — but the surrounding for { select case err := <-lec ... } loop in Setup() 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 from lec instead 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 win

Don’t log expected anonymous /context/user requests as errors.

This path now intentionally returns a normal unauthenticated payload, so ErrUserContextNotFound should 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

📥 Commits

Reviewing files that changed from the base of the PR and between a635179 and c298d0b.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (28)
  • frontend/src/App.tsx
  • frontend/src/components/layout/layout.tsx
  • frontend/src/lib/i18n/locales/en-US.json
  • frontend/src/lib/i18n/locales/en.json
  • frontend/src/pages/authorize-page.tsx
  • frontend/src/pages/continue-page.tsx
  • frontend/src/pages/forgot-password-page.tsx
  • frontend/src/pages/login-page.tsx
  • frontend/src/pages/logout-page.tsx
  • frontend/src/pages/totp-page.tsx
  • frontend/src/schemas/app-context-schema.ts
  • frontend/src/schemas/user-context-schema.ts
  • go.mod
  • internal/bootstrap/app_bootstrap.go
  • internal/bootstrap/router_bootstrap.go
  • internal/bootstrap/service_bootstrap.go
  • internal/controller/context_controller.go
  • internal/controller/context_controller_test.go
  • internal/controller/proxy_controller_test.go
  • internal/controller/user_controller.go
  • internal/controller/user_controller_test.go
  • internal/middleware/context_middleware.go
  • internal/middleware/context_middleware_test.go
  • internal/model/config.go
  • internal/model/context.go
  • internal/model/runtime.go
  • internal/service/auth_service.go
  • internal/service/tailscale_service.go

Comment thread internal/bootstrap/router_bootstrap.go
Comment thread internal/controller/user_controller.go
Comment thread internal/middleware/context_middleware.go
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/model/constants.go (1)

25-25: ⚡ Quick win

Use time.Duration type for type safety.

The constant is defined as an untyped integer with a comment indicating seconds. Go best practice is to use time.Duration for all time-related values to prevent confusion about units and enable type checking. Currently, the constant is multiplied by time.Second at the usage site in internal/bootstrap/router_bootstrap.go:205 (model.GracefulShutdownTimeout*time.Second). Using time.Duration would eliminate this multiplication and make the code clearer.

Proposed change

Add the time import at the top of the file, then update the constant:

const GracefulShutdownTimeout = 5 * time.Second

This would simplify the usage to just model.GracefulShutdownTimeout without needing the *time.Second multiplication.

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between c298d0b and b6df4b9.

📒 Files selected for processing (6)
  • internal/bootstrap/router_bootstrap.go
  • internal/controller/user_controller.go
  • internal/middleware/context_middleware.go
  • internal/model/constants.go
  • internal/service/auth_service.go
  • internal/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants