refactor: rework file structure#325
Conversation
WalkthroughReplaces legacy handler/server architecture with a bootstrap-driven app: rewrites config and public types, introduces services (Auth/Docker/Ldap), an OAuth broker, controllers, middlewares, reorganized utils, frontend API route updates, assets rename, and removes many legacy handlers/providers/tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as CLI
participant Cmd as cmd/root
participant Boot as BootstrapApp
participant Svc as Services
participant Mid as Middlewares
participant Ctl as Controllers
participant HTTP as HTTP Server
CLI->>Cmd: tinyauth run
Cmd->>Boot: NewBootstrapApp(conf).Setup()
Boot->>Svc: Init Docker/Auth/OAuthBroker/Ldap
Boot->>Mid: Init Zerolog/UI/Context
Boot->>Ctl: Create controllers & register routes
Boot->>HTTP: Start server
sequenceDiagram
autonumber
actor User
participant FE as Frontend
participant OAuthCtl as OAuthController
participant Broker as OAuthBrokerService
participant Prov as OAuth Service
participant Auth as AuthService
User->>FE: Click "Login with Provider"
FE->>OAuthCtl: GET /api/oauth/url/:provider
OAuthCtl->>Broker: GetService(provider)
OAuthCtl->>Prov: GenerateState & GetAuthURL
OAuthCtl-->>FE: { url } + Set CSRF/redirect cookies
User->>Prov: Authorize
Prov-->>OAuthCtl: Redirect with code & state
OAuthCtl->>Prov: VerifyCode(code)
OAuthCtl->>Prov: Userinfo()
Prov-->>OAuthCtl: Claims
OAuthCtl->>Auth: CreateSessionCookie(claims)
OAuthCtl-->>FE: Redirect to app or continue URL
sequenceDiagram
autonumber
participant Client
participant ProxyCtrl as ProxyController
participant Ctx as ContextMiddleware
participant Dock as DockerService
participant Auth as AuthService
Client->>ProxyCtrl: request /api/auth/:proxy
ProxyCtrl->>Ctx: ContextMiddleware resolves user
ProxyCtrl->>Dock: GetLabels(id, domain)
alt IP bypass
ProxyCtrl-->>Client: 200 + injected headers
else Not authenticated / unauthorized
ProxyCtrl-->>Client: 401 or Redirect to /login?redirect_uri=...
else Authorized
ProxyCtrl-->>Client: 200 + user headers
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
==========================================
- Coverage 20.14% 0.00% -20.15%
==========================================
Files 24 31 +7
Lines 2214 2344 +130
==========================================
- Hits 446 0 -446
- Misses 1733 2344 +611
+ Partials 35 0 -35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 38
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
go.mod (1)
3-3: Go directive should not include a patch version.go.mod’s
godirective expectsmajor.minoronly. If you want to pin a specific toolchain patch, use thetoolchaindirective instead.Apply this diff:
-go 1.23.2 +go 1.23Optionally add a toolchain pin (if the project requires it):
+toolchain go1.23.2internal/service/ldap_service.go (1)
139-146: Retry logic bug: returning nil error on failure marks reconnect as successInside operation(), you return (nil, nil) when connect fails. That stops retries and treats reconnect as success. Return the actual error to trigger backoff. Also guard ldap.Conn.Close() when nil.
operation := func() (*ldapgo.Conn, error) { - ldap.Conn.Close() - conn, err := ldap.connect() - if err != nil { - return nil, nil - } - return conn, nil + if ldap.Conn != nil { + _ = ldap.Conn.Close() + } + conn, err := ldap.connect() + if err != nil { + return nil, err + } + return conn, nil } -_, err := backoff.Retry(context.TODO(), operation, backoff.WithBackOff(exp), backoff.WithMaxTries(3)) +_, err := backoff.Retry(context.TODO(), operation, backoff.WithBackOff(exp), backoff.WithMaxTries(3))This usage matches v5 API where Retry returns (result, error) and supports WithBackOff/WithMaxTries options. (pkg.go.dev, chromium.googlesource.com)
Also applies to: 148-154
internal/service/auth_service.go (3)
253-270: Delete cookie correctly (MaxAge = -1) instead of just clearing values.Clearing session.Values leaves an empty but valid cookie. Set MaxAge = -1 to instruct the browser to remove it, and also emit a Set-Cookie with -1 for safety.
func (auth *AuthService) DeleteSessionCookie(c *gin.Context) error { session, err := auth.GetSession(c) if err != nil { return err } // Delete all values in the session for key := range session.Values { delete(session.Values, key) } - err = session.Save(c.Request, c.Writer) + if session.Options == nil { + session.Options = auth.Store.Options + } + // Invalidate the cookie in the browser + session.Options.MaxAge = -1 + err = session.Save(c.Request, c.Writer) if err != nil { return err } + // Best-effort explicit removal as well + c.SetCookie(auth.Config.SessionCookieName, "", -1, "/", auth.Config.Domain, auth.Config.SecureCookie, true) return nil }
325-348: Trim whitespace when matching OAuth groups.CSV sources (headers/claims) often include spaces. Without trimming, "devs" vs " devs" fails to match.
// Split the groups by comma (no need to parse since they are from the API response) - oauthGroups := strings.Split(context.OAuthGroups, ",") + raw := strings.Split(context.OAuthGroups, ",") + oauthGroups := make([]string, 0, len(raw)) + for _, g := range raw { + g = strings.TrimSpace(g) + if g != "" { + oauthGroups = append(oauthGroups, g) + } + }If you adopt []string for OAuthGroups in config structs, this block simplifies further.
1-437: Critical Repo-Wide Follow-Ups to Complete the Auth RefactorBased on the verification script output, the following issues must be addressed before merging:
• Lingering old flag names
– Found an outdatedcookie-secureflag incmd/root.go:85that no longer maps to your newSecureCookiefield. Rename or alias this flag to avoid confusion.• Config typos
–FogotPasswordMessageremains ininternal/config/config.go:50and is still referenced ininternal/bootstrap/app_bootstrap.go:182. Rename toForgotPasswordMessage.
– The typePassowrdLabelsininternal/config/config.go:68–71should be corrected toPasswordLabels.• Service initialization gap
–AuthService.Init()is never called ininternal/bootstrap/app_bootstrap.goafterNewAuthService(...)on line 111. Either add an explicitauthService.Init()or registerauthServicewith your broker so it’s initialized in the existing loop (svc.Init()) on line 124.• Direct SetCookie calls bypassing conventions
– Detected rawc.SetCookie(...)invocations in:
•internal/service/auth_service.go:75
•internal/controller/oauth_controller.go:76, 82, 114, 197
These calls hard-code the path and skip your domain/SameSite defaults. Refactor them to use the centralsessions.CookieStore(or at least includefmt.Sprintf(".%s", Config.Domain)and your desired SameSite policy) to keep cookie behavior consistent.• Log level mapping
– ConfirmedGetLogLevel(...)ininternal/utils/app_utils.go:66–69exists and maps at leastdebugtozerolog.DebugLevel. Ensure mappings for all intended levels are present—but no immediate change required here.
♻️ Duplicate comments (1)
internal/middleware/ui_middleware.go (1)
35-56: Harden path handling and gracefully degrade when assets aren’t initialized.
- Guard against uninitialized UIFS/UIFileServer (e.g., when Init fails or isn’t called).
- Prefer HasPrefix over Split for routing decisions.
- Normalize and validate the path before fs.Stat to avoid oddities; use fs.ErrNotExist via errors.Is.
- This also addresses “Uncontrolled data used in path expression” flagged previously.
@@ -import ( +import ( + "errors" "io/fs" "net/http" "os" + "path" "strings" "tinyauth/internal/assets" "github.com/gin-gonic/gin" ) @@ func (m *UIMiddleware) Middleware() gin.HandlerFunc { return func(c *gin.Context) { - switch strings.Split(c.Request.URL.Path, "/")[1] { - case "api": - c.Next() - return - case "resources": - c.Next() - return - default: - _, err := fs.Stat(m.UIFS, strings.TrimPrefix(c.Request.URL.Path, "/")) - - if os.IsNotExist(err) { - c.Request.URL.Path = "/" - } - - m.UIFileServer.ServeHTTP(c.Writer, c.Request) - c.Abort() - return - } + // If assets were not initialized, act as a no-op. + if m.UIFS == nil || m.UIFileServer == nil { + c.Next() + return + } + + p := c.Request.URL.Path + if strings.HasPrefix(p, "/api") || strings.HasPrefix(p, "/resources") { + c.Next() + return + } + + // Clean path and check if asset exists; fall back to SPA index on 404. + reqPath := path.Clean("/" + strings.TrimPrefix(p, "/")) + assetPath := strings.TrimPrefix(reqPath, "/") + if _, err := fs.Stat(m.UIFS, assetPath); err != nil { + if errors.Is(err, fs.ErrNotExist) { + c.Request.URL.Path = "/" + } + } + + m.UIFileServer.ServeHTTP(c.Writer, c.Request) + c.Abort() } }
🧹 Nitpick comments (62)
frontend/src/context/user-context.tsx (2)
16-19: Endpoint update looks good; mirror in-query validation for stronger typing.The move to /api/context/user aligns with the new API layout. As with AppContext, validating in the query keeps the provider lean and ensures type-safe context values.
Please confirm that GET /api/context/user returns the unauthenticated shape (rather than a 401) on public pages, or adjust error boundaries accordingly.
- const { isFetching, data, error } = useSuspenseQuery({ - queryKey: ["user"], - queryFn: () => axios.get("/api/context/user").then((res) => res.data), - }); + const { isFetching, data, error } = useSuspenseQuery({ + queryKey: ["context", "user"], + queryFn: () => axios.get("/api/context/user").then((res) => res.data), + select: (raw) => { + const validated = userContextSchema.safeParse(raw); + if (!validated.success) throw validated.error; + return validated.data; + }, + staleTime: 60 * 1000, // user context might change more often; tune as needed + });Apply outside-this-hunk cleanup after the above:
- const validated = userContextSchema.safeParse(data); - - if (validated.success === false) { - throw validated.error; - } + // data is already validated and typed by react-query's `select`. return ( - <UserContext.Provider value={validated.data}> + <UserContext.Provider value={data}> {children} </UserContext.Provider> );
41-44: Tiny grammar nit in the error message.Prefer “a UserContextProvider” over “an UserContextProvider”.
- throw new Error( - "useUserContext must be used within an UserContextProvider", - ); + throw new Error( + "useUserContext must be used within a UserContextProvider", + );.gitignore (1)
28-29: Scope the ignore to the repo root to avoid accidental matches.The pattern “data” will ignore any path containing “data” (e.g., frontend/src/data). Prefer a root-scoped directory ignore.
-# data directory -data +# data directory +/data/.env.example (1)
28-28: LOG_LEVEL parser coverage verified
- Checked
GetLogLevelininternal/utils/app_utils.go(lines 66–83): it handles"debug","info","warn","error","fatal", and"panic", defaulting toInfoLevelfor unrecognized values.- Since
.env.exampleis typically a developer template, defaulting todebugis reasonable; if this file doubles as a production template, you may wish to revert it toinfo.- Nit (optional): per dotenv-linter, consider moving the
LOG_LEVELentry beforeOAUTH_WHITELISTin.env.examplefor consistency.frontend/src/pages/totp-page.tsx (1)
48-52: Optionally surface server error details for TOTP failures.Capturing AxiosError enables more granular messages (e.g., invalid code vs. rate limit). Small UX win.
Apply this diff to type the error and show a targeted message when available:
- onError: () => { - toast.error(t("totpFailTitle"), { - description: t("totpFailSubtitle"), - }); - }, + onError: (error: import("axios").AxiosError<{ message?: string }>) => { + toast.error(t("totpFailTitle"), { + description: + error.response?.data?.message ?? + t("totpFailSubtitle"), + }); + },go.mod (1)
71-71: Consolidate to a Single Validator Major VersionWe still have a direct v9 import that must be updated before removing v9 from
go.mod:
- File & Line:
cmd/root.go:11
Current import:"github.com/go-playground/validator"- To migrate to v10, update the import path and adapt any API changes:
- cmd/root.go:11: "github.com/go-playground/validator" + cmd/root.go:11: "github.com/go-playground/validator/v10"- Once all code imports use
/v10, remove the v9 entry fromgo.mod:- github.com/go-playground/validator v9.31.0+incompatible- Finally, run:
to prune the unused v9 module.go mod tidyIf another dependency transitively requires v9, you’ll still get both in the graph—but migrating your own imports to v10 will at least centralize your direct usage on a single major.
internal/utils/string_utils.go (2)
7-12: Capitalize: avoid double []rune conversion; use utf8.DecodeRune; add doc comment.Current implementation converts to []rune twice and allocates unnecessarily. Using utf8.DecodeRune is faster and clearer. Also add a doc comment for exported function.
Apply this diff:
-import ( - "strings" -) +import ( + "strings" + "unicode/utf8" +) -func Capitalize(str string) string { - if len(str) == 0 { - return "" - } - return strings.ToUpper(string([]rune(str)[0])) + string([]rune(str)[1:]) -} +// Capitalize uppercases the first Unicode rune of str and returns the result. +// If str is empty, it returns an empty string. +func Capitalize(str string) string { + if str == "" { + return "" + } + r, size := utf8.DecodeRuneInString(str) + return strings.ToUpper(string(r)) + str[size:] +}Note: Some runes can uppercase to multiple runes in edge locales; this mirrors the existing behavior while removing extra allocations. If locale-aware titlecasing is desired later, consider golang.org/x/text/cases.
14-30: CoalesceToString: support []string; drop redundant continue; add doc comment.This improves generality (claims often come as []string) and simplifies the loop.
Apply this diff:
-func CoalesceToString(value any) string { - switch v := value.(type) { - case []any: - strs := make([]string, 0, len(v)) - for _, item := range v { - if str, ok := item.(string); ok { - strs = append(strs, str) - continue - } - } - return strings.Join(strs, ",") - case string: - return v - default: - return "" - } -} +// CoalesceToString returns a comma-joined string for string-like inputs: +// - []any: collects only string elements (ignores non-strings) +// - []string: joins elements +// - string: returns as-is +// Any other type returns "". +func CoalesceToString(value any) string { + switch v := value.(type) { + case []any: + strs := make([]string, 0, len(v)) + for _, item := range v { + if str, ok := item.(string); ok { + strs = append(strs, str) + } + } + return strings.Join(strs, ",") + case []string: + return strings.Join(v, ",") + case string: + return v + default: + return "" + } +}internal/controller/resources_controller.go (1)
25-27: Support HEAD for static resources.File servers handle HEAD efficiently; adding a HEAD route improves correctness and tooling compatibility.
Apply this diff:
func (controller *ResourcesController) SetupRoutes() { controller.Router.GET("/resources/*resource", controller.resourcesHandler) + controller.Router.HEAD("/resources/*resource", controller.resourcesHandler) }internal/utils/fs_utils.go (1)
5-16: Avoid redundant os.Stat and the TOCTOU window; read once.os.ReadFile already returns a clear error if the path is invalid. Dropping Stat removes an extra syscall and the time-of-check/time-of-use race window.
Apply this diff:
func ReadFile(file string) (string, error) { - _, err := os.Stat(file) - if err != nil { - return "", err - } - - data, err := os.ReadFile(file) - if err != nil { - return "", err - } - - return string(data), nil + data, err := os.ReadFile(file) + if err != nil { + return "", err + } + return string(data), nil }internal/controller/health_controller.go (2)
15-18: Add back-compat alias for previous /healthcheck route (and keep HEAD lightweight).The legacy server exposed /api/healthcheck. This controller now mounts /health only, which can break monitors/clients. Also, the HEAD route currently shares the JSON body handler. HEAD responses should not emit a body.
Apply the diff to add /healthcheck aliases and make HEAD a no-body response:
@@ -import "github.com/gin-gonic/gin" +import ( + "net/http" + "github.com/gin-gonic/gin" +) @@ func (controller *HealthController) SetupRoutes() { controller.Router.GET("/health", controller.healthHandler) controller.Router.HEAD("/health", controller.healthHandler) + // Back-compat with previous endpoint name + controller.Router.GET("/healthcheck", controller.healthHandler) + controller.Router.HEAD("/healthcheck", controller.healthHandler) }And update the handler below (see next comment) to treat HEAD properly.
20-25: Unify response shape and avoid a body on HEAD.Previous handler returned an integer status (200) and "OK". The new one returns a string "ok" and "Healthy", changing types and text. To minimize breakage, keep the old shape and suppress the response body on HEAD.
Apply:
@@ -func (controller *HealthController) healthHandler(c *gin.Context) { - c.JSON(200, gin.H{ - "status": "ok", - "message": "Healthy", - }) -} +func (controller *HealthController) healthHandler(c *gin.Context) { + if c.Request.Method == http.MethodHead { + c.Status(http.StatusOK) + return + } + c.JSON(http.StatusOK, gin.H{ + "status": 200, + "message": "OK", + }) +}internal/middleware/ui_middleware.go (1)
13-16: Consider unexporting internal fields.Unless external packages need to mutate UIFS/UIFileServer, prefer unexported fields to reduce surface area.
-type UIMiddleware struct { - UIFS fs.FS - UIFileServer http.Handler -} +type UIMiddleware struct { + uifs fs.FS + uiFileServer http.Handler +}Note: Adjust references accordingly if you adopt this.
internal/utils/user_utils.go (1)
29-51: Optional: Short-circuit after composition if resulting users string is empty.This helps when file exists but is empty and conf is empty.
return ParseUsers(users)to
+ if strings.TrimSpace(users) == "" { + return []config.User{}, nil + } + return ParseUsers(users)internal/controller/context_controller.go (2)
58-62: Route layout changed from /user and /app to /context/ — add aliases to avoid breaking clients.*Legacy endpoints were /api/user and /api/app. This rework mounts at /context/user and /context/app, which is a breaking change for UIs and scripts.
func (controller *ContextController) SetupRoutes() { contextGroup := controller.Router.Group("/context") contextGroup.GET("/user", controller.userContextHandler) contextGroup.GET("/app", controller.appContextHandler) + + // Back-compat with previous endpoints + controller.Router.GET("/user", controller.userContextHandler) + controller.Router.GET("/app", controller.appContextHandler) }
10-33: Avoid duplicating response DTOs if shared types already exist.If types like UserContextResponse/AppContext already exist elsewhere (previously under internal/types or handlers), consider reusing them to reduce drift and maintenance.
Would you like a follow-up PR to consolidate these DTOs under internal/types and reuse across controllers?
internal/utils/label_utils.go (2)
35-43: Sanitization policy clarity.SanitizeHeader allows spaces and tabs; that’s correct for values, but for keys we now guard separately. Consider documenting this contract in comments or splitting into SanitizeHeaderKey and SanitizeHeaderValue for clarity.
I can provide a small refactor if you want distinct functions for key/value sanitization.
10-19: Optional: wrap Decode errors with context or log them.GetLabels previously logged parse errors; this version returns the raw error. If observability matters during discovery of mis-labeled resources, consider wrapping with context or logging at call sites.
No code change required if the new behavior is intentional.
internal/middleware/zerolog_middleware.go (4)
29-36: Clarify naming and boolean semantics of logPathlogPath returns true when logging should occur and false when it should be downgraded to debug. The name reads like it returns “path to log,” which is slightly ambiguous. Consider renaming to shouldLog or inverting the return to a skip predicate for readability.
-func (m *ZerologMiddleware) logPath(path string) bool { +func (m *ZerologMiddleware) shouldLog(methodAndPath string) bool { - for _, prefix := range loggerSkipPathsPrefix { - if strings.HasPrefix(path, prefix) { + for _, prefix := range loggerSkipPathsPrefix { + if strings.HasPrefix(methodAndPath, prefix) { return false } } return true } ... - if m.logPath(method + " " + path) { + if m.shouldLog(method + " " + path) {
55-61: 1xx responses aren’t logged; add a default branchRight now, 1xx statuses fall through without logging. Add a default to ensure every response is captured.
switch { case code >= 200 && code < 300: log.Info()...Msg("Request") case code >= 300 && code < 400: log.Warn()...Msg("Request") case code >= 400: log.Error()...Msg("Request") + default: + log.Info().Str("method", method).Str("path", path).Str("address", address).Str("clientIp", clientIP).Int("status", code).Str("latency", latency).Msg("Request") }
62-64: Keep log fields consistent for skipped pathsInclude clientIp in the debug branch so dashboards/queries can rely on a uniform schema.
- log.Debug().Str("method", method).Str("path", path).Str("address", address).Int("status", code).Str("latency", latency).Msg("Request") + log.Debug().Str("method", method).Str("path", path).Str("address", address).Str("clientIp", clientIP).Int("status", code).Str("latency", latency).Msg("Request")
12-17: Method+path matching for skips may be too narrowYou only skip GET/HEAD healthcheck and GET favicon. If the proxy or health probes use OPTIONS or if HEAD /favicon.ico is common, you’ll get unnecessary logs. Consider skipping by path alone or expanding the allowlist.
-var ( - loggerSkipPathsPrefix = []string{ - "GET /api/healthcheck", - "HEAD /api/healthcheck", - "GET /favicon.ico", - } -) +var loggerSkipPathsPrefix = []string{"/api/healthcheck", "/favicon.ico"} ... - if m.logPath(method + " " + path) { + if m.shouldLog(path) {Also applies to: 53-53
internal/service/oauth_broker_service.go (2)
30-44: Normalize provider names to avoid case-sensitivity bugsIf configs map contains “GitHub” or “GOOGLE”, it will fall into the default case. Normalize keys on insert and lookup.
func (broker *OAuthBrokerService) Init() error { - for name, cfg := range broker.Configs { + for rawName, cfg := range broker.Configs { + name := strings.ToLower(rawName) switch name { case "github": service := NewGithubOAuthService(cfg) broker.Services[name] = service case "google": service := NewGoogleOAuthService(cfg) broker.Services[name] = service default: service := NewGenericOAuthService(cfg) broker.Services[name] = service } }-func (broker *OAuthBrokerService) GetService(name string) (OAuthService, bool) { - service, exists := broker.Services[name] +func (broker *OAuthBrokerService) GetService(name string) (OAuthService, bool) { + service, exists := broker.Services[strings.ToLower(name)] return service, exists }-func (broker *OAuthBrokerService) GetUser(service string) (config.Claims, error) { - oauthService, exists := broker.Services[service] +func (broker *OAuthBrokerService) GetUser(service string) (config.Claims, error) { + oauthService, exists := broker.Services[strings.ToLower(service)] if !exists { - return config.Claims{}, errors.New("oauth service not found") + return config.Claims{}, errors.New("oauth service not found") } return oauthService.Userinfo() }Note: add
stringsto imports.Also applies to: 65-68, 70-76
48-52: Prefer structured fields over formatted messages in logsThis makes logs easier to query and consistent with the rest of the codebase.
- log.Error().Err(err).Msgf("Failed to initialize OAuth service: %s", name) + log.Error().Err(err).Str("service", name).Msg("Failed to initialize OAuth service") ... - log.Info().Msgf("Initialized OAuth service: %s", name) + log.Info().Str("service", name).Msg("Initialized OAuth service")cmd/root.go (1)
44-46: Minor: simplify level assignmentGetLogLevel already returns zerolog.Level; the extra cast is redundant.
- log.Logger = log.Level(zerolog.Level(utils.GetLogLevel(conf.LogLevel))) + log.Logger = log.Level(utils.GetLogLevel(conf.LogLevel))internal/service/docker_service.go (1)
36-55: Defensive guard against uninitialized clientEven after fixing Init, a guard helps avoid panics if Docker isn’t available or Init wasn’t called.
func (docker *DockerService) GetContainers() ([]container.Summary, error) { + if docker.Client == nil { + return nil, errors.New("docker client is not initialized") + } containers, err := docker.Client.ContainerList(docker.Context, container.ListOptions{}) ... } func (docker *DockerService) InspectContainer(containerId string) (container.InspectResponse, error) { + if docker.Client == nil { + return container.InspectResponse{}, errors.New("docker client is not initialized") + } ... } func (docker *DockerService) DockerConnected() bool { - _, err := docker.Client.Ping(docker.Context) + if docker.Client == nil { + return false + } + _, err := docker.Client.Ping(docker.Context) return err == nil }Note: add
errorsto imports.internal/service/google_oauth_service.go (2)
54-59: Harden state generation: handle crypto/rand errors and use RawURLEncoding.
- rand.Read error is ignored; if it fails, state may be all zeros.
- RawURLEncoding avoids trailing "=" padding in URLs.
func (oauth *GoogleOAuthService) GenerateState() string { b := make([]byte, 128) - rand.Read(b) - state := base64.URLEncoding.EncodeToString(b) + if _, err := rand.Read(b); err != nil { + // Fallback: extremely unlikely path; keep small constant to avoid leaking failures + b = []byte("state-fallback") + } + state := base64.RawURLEncoding.EncodeToString(b) return state }
31-41: Honor custom scopes when provided; default to GoogleOAuthScopes.Currently, NewGoogleOAuthService ignores config.Scopes. Supporting overrides keeps behavior consistent with other providers.
func NewGoogleOAuthService(config config.OAuthServiceConfig) *GoogleOAuthService { return &GoogleOAuthService{ Config: oauth2.Config{ ClientID: config.ClientID, ClientSecret: config.ClientSecret, RedirectURL: config.RedirectURL, - Scopes: GoogleOAuthScopes, + Scopes: func() []string { + if len(config.Scopes) > 0 { + return config.Scopes + } + return GoogleOAuthScopes + }(), Endpoint: endpoints.Google, }, } }internal/utils/security_utils.go (3)
52-59: Name shadowing: local variable ‘hkdf’ hides the imported package.This compiles but hurts readability and confuses tooling. Rename the variable to reader or kdf.
func DeriveKey(secret string, info string) (string, error) { hash := sha256.New - hkdf := hkdf.New(hash, []byte(secret), nil, []byte(info)) // I am not using a salt because I just want two different keys from one secret, maybe bad practice + // HKDF used to deterministically derive distinct keys from a single secret (no salt by design here). + reader := hkdf.New(hash, []byte(secret), nil, []byte(info)) key := make([]byte, 24) - _, err := io.ReadFull(hkdf, key) + _, err := io.ReadFull(reader, key)
52-56: Production comment hygiene.The inline “maybe bad practice” comment is speculative and will trigger audits. Replace with a clear rationale or remove it. See the diff above.
70-79: Validate input IP before CIDR checks.If ip is invalid, cidr.Contains(nil) returns false silently. Prefer an explicit error to aid callers and logs.
func FilterIP(filter string, ip string) (bool, error) { ipAddr := net.ParseIP(ip) + if ipAddr == nil { + return false, errors.New("invalid IP address") + } + if strings.Contains(filter, "/") { _, cidr, err := net.ParseCIDR(filter) if err != nil { return false, err } return cidr.Contains(ipAddr), nil }internal/controller/user_controller.go (2)
85-93: Unknown user detection is inconsistent with the rest of the codebase.Elsewhere (“unknown” is used), here an empty string is checked. Make the guard robust to both to avoid leaking different log messages.
- if userSearch.Type == "" { + if userSearch.Type == "" || userSearch.Type == "unknown" { log.Warn().Str("username", req.Username).Str("ip", clientIP).Msg("User not found") controller.Auth.RecordLoginAttempt(rateIdentifier, false) c.JSON(401, gin.H{ "status": 401, "message": "Unauthorized", }) return }
167-176: Nit: avoid naming local variable “context”.It can confuse readers since “context” is a ubiquitous package in Go. Consider userCtx or uc.
internal/bootstrap/app_bootstrap.go (2)
55-58: Cookie suffix is derived from only the first label; use full domain to reduce collision risk.If domain is example.com, using only “example” increases the chance of collisions across TLDs. Using the full domain string is simpler and safer.
- cookieId := utils.GenerateIdentifier(strings.Split(domain, ".")[0]) + cookieId := utils.GenerateIdentifier(domain)
209-216: Minor: avoid shadowing imported package names and use consistent plural naming.The local slice variable named controller shadows the controller import. Rename to controllers for clarity. Same note for middlewares loop var; mw is clearer.
If you want, I can push a mechanical rename patch.
Also applies to: 187-205, 194-201
internal/middleware/context_middleware.go (1)
36-57: Style: prefer early returns or small helper functions over goto.The flow is correct, but gotos reduce readability and complicate defer blocks. Consider extracting the basic auth path into a helper and return early from each branch.
Also applies to: 104-157
internal/controller/proxy_controller.go (4)
203-208: 401/403 status code drift vs legacy handler; verify intended behaviorPreviously, nginx/non‑browser paths used 401 for “unauthorized” (resource/group) and 403 for IP-deny. This controller returns 403 where legacy used 401, and 401 where legacy used 403. Please confirm desired semantics and align consistently (docs, tests, and clients may rely on this).
If you intend to preserve legacy behavior, the minimal diffs are:
- c.JSON(403, gin.H{ + c.JSON(401, gin.H{ "status": 401, "message": "Unauthorized", })- if req.Proxy == "nginx" || !isBrowser { - c.JSON(401, gin.H{ - "status": 401, - "message": "Unauthorized", - }) + if req.Proxy == "nginx" || !isBrowser { + c.JSON(403, gin.H{ + "status": 403, + "message": "Forbidden", + })Also applies to: 236-241, 116-121
299-301: Trust on X-Forwarded- without validation can enable open-redirect/header spoofing*proto/host/uri are taken from X-Forwarded-* for building RedirectURI and for resource derivation. If this handler is reachable directly (misconfigured proxy) these headers can be client-controlled. Consider:
- Gate by trusted proxy IPs or
- Prefer request URL if X-Forwarded-* are empty/untrusted, and
- Sanitize/validate host and require known domain suffix.
Also applies to: 78-89, 66-72
266-282: Authorization header overwrite: Basic auth can clobber upstream AuthorizationYou first propagate incoming Authorization, then (optionally) overwrite it with Basic. If both are relevant, downstream will only receive Basic. Consider using a different header for injected basic (e.g., X-Proxy-Authorization) or only set Basic when the original Authorization is empty.
- c.Header("Authorization", c.Request.Header.Get("Authorization")) + if authz := c.Request.Header.Get("Authorization"); authz != "" { + c.Header("Authorization", authz) + } ... - if labels.Basic.Username != "" && utils.GetSecret(labels.Basic.Password.Plain, labels.Basic.Password.File) != "" { + if labels.Basic.Username != "" && + utils.GetSecret(labels.Basic.Password.Plain, labels.Basic.Password.File) != "" && + c.Writer.Header().Get("Authorization") == "" {Also applies to: 93-105
200-206: Minor: reduce repeated Split(host, ".") and improve logsYou call strings.Split(host, ".")[0] multiple times; compute once. Also prefer structured fields consistently (resource, host).
- appAllowed := controller.Auth.ResourceAllowed(c, userContext, labels) + resource := strings.Split(host, ".")[0] + appAllowed := controller.Auth.ResourceAllowed(c, userContext, labels) ... - log.Warn().Str("user", userContext.Username).Str("resource", strings.Split(host, ".")[0]).Msg("User not allowed to access resource") + log.Warn().Str("user", userContext.Username).Str("resource", resource).Msg("User not allowed to access resource")Also applies to: 234-240
internal/service/generic_oauth_service.go (2)
64-69: Use crypto-strong state and handle rand.Read errorsrand.Read can fail; treat failures as errors instead of silently producing weak state.
func (generic *GenericOAuthService) GenerateState() string { b := make([]byte, 128) - rand.Read(b) + if _, err := rand.Read(b); err != nil { + // Fallback: minimal state to avoid empty value; log upstream if you have a logger here. + return base64.URLEncoding.EncodeToString([]byte(fmt.Sprintf("fallback-%d", time.Now().UnixNano()))) + } state := base64.URLEncoding.EncodeToString(b) return state }
42-61: Set HTTP client timeout to avoid hanging on network issuesThe custom HTTP client has no Timeout; add a sane default.
httpClient := &http.Client{ - Transport: transport, + Transport: transport, + Timeout: 10 * time.Second, }Also applies to: 50-56
internal/controller/oauth_controller.go (2)
108-115: Improve CSRF mismatch logging and robustnessErr may be nil in a mismatch case; log the condition explicitly and clear the CSRF cookie to avoid reuse.
- if err != nil || state != csrfCookie { - log.Warn().Err(err).Msg("CSRF token mismatch or cookie missing") + if err != nil || state != csrfCookie { + if err != nil { + log.Warn().Err(err).Msg("CSRF cookie missing") + } else { + log.Warn().Msg("CSRF token mismatch") + } + c.SetCookie(controller.Config.CSRFCookieName, "", -1, "/", "", controller.Config.SecureCookie, true) c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.Config.AppURL)) return }
171-177: Session cookie: consider SameSite and Secure attributesgin.Context.SetCookie can’t set SameSite. Consider switching to http.SetCookie for full control (SameSite=Lax/Strict) to mitigate CSRF on subsequent requests. Also ensure Secure=true in production.
internal/service/ldap_service.go (2)
34-55: Lifecycle: add a way to stop the heartbeat goroutineInit starts a ticker loop with no shutdown. Consider accepting a context in Init (or LdapService) and stop the loop on cancel to avoid leaks during app shutdown.
Also applies to: 40-53
76-100: Thread-safety: shared ldap.Conn accessed from multiple goroutines without syncheartbeat, Search, and Bind may race on Conn. Consider protecting Conn with a RWMutex or using atomic pointer swaps and local copies per operation.
Also applies to: 102-109, 110-128
internal/service/github_oauth_service.go (1)
59-64: Handle rand.Read error in state generationSame as GenericOAuth; ensure cryptographic state generation doesn’t silently fail.
b := make([]byte, 128) -rand.Read(b) +if _, err := rand.Read(b); err != nil { + return base64.URLEncoding.EncodeToString([]byte("fallback")) +}internal/config/config.go (7)
3-8: Unify the representation of “groups” (avoid any).Using any for Claims.Groups invites inconsistent shapes and type assertions later. Elsewhere we store OAuthGroups as string (comma-separated). Recommend a single representation.
- Easiest: standardize on []string for groups and convert to/from CSV once at the boundary.
- If upstream providers vary (string vs array), add a custom unmarshaller to accept both.
Example change:
type Claims struct { Name string `json:"name"` Email string `json:"email"` PreferredUsername string `json:"preferred_username"` - Groups any `json:"groups"` + Groups []string `json:"groups"` }If needed later, I can add a json.Unmarshaler that gracefully handles string or []string. Want me to push that?
10-12: Add build-time ldflags notes to version vars.These globals are fine, but a brief comment helps future maintainers remember they’re set via -ldflags at build time.
-var Version = "development" -var CommitHash = "n/a" -var BuildTimestamp = "n/a" +// Overridden at build time via -ldflags: -X tinyauth/internal/config.Version=... etc. +var Version = "development" +var CommitHash = "n/a" +var BuildTimestamp = "n/a"
14-16: Prefer consts and remove duplication with AuthServiceConfig.SessionCookieName.These cookie-name globals are static; make them const to prevent accidental mutation. Also, we already pass SessionCookieName through AuthServiceConfig, which may drift from these vars.
- Make them const
- Either remove SessionCookieName from AuthServiceConfig or ensure it defaults to these constants from a single place (bootstrap/config loader).
-var SessionCookieName = "tinyauth-session" -var CSRFCookieName = "tinyauth-csrf" -var RedirectCookieName = "tinyauth-redirect" +const SessionCookieName = "tinyauth-session" +const CSRFCookieName = "tinyauth-csrf" +const RedirectCookieName = "tinyauth-redirect"If we keep the field in AuthServiceConfig, set it from these consts during bootstrap to avoid divergence.
50-50: Typo: FogotPasswordMessage should be ForgotPasswordMessage.Spelling slips into the exported API. Rename the field; keep mapstructure:"forgot-password-message" as-is.
- FogotPasswordMessage string `mapstructure:"forgot-password-message"` + ForgotPasswordMessage string `mapstructure:"forgot-password-message"`I can follow up with a repo-wide rename to fix references.
92-101: Consider tags or dedicated loader for OAuthServiceConfig.If this struct is intended to be hydrated from file/env, add mapstructure/json tags to avoid bespoke wiring. If it’s purely internal, add a comment stating so.
109-112: Constrain UserSearch.Type to an enum to avoid typos.Free-form strings ("local", "ldap", "unknown") are fragile. Define a typed alias with constants.
-type UserSearch struct { - Username string - Type string // local, ldap or unknown -} +type UserType string +const ( + UserTypeLocal UserType = "local" + UserTypeLDAP UserType = "ldap" + UserTypeUnknown UserType = "unknown" +) +type UserSearch struct { + Username string + Type UserType +}
114-121: Normalize OAuthGroups to []string across structs.SessionCookie and UserContext carry OAuthGroups as CSV strings, while Claims.Groups likely becomes []string. Using []string here removes repeated split/join logic and reduces edge-case bugs (spaces, empty items).
type SessionCookie struct { Username string Name string Email string Provider string TotpPending bool - OAuthGroups string + OAuthGroups []string } ... type UserContext struct { Username string Name string Email string IsLoggedIn bool OAuth bool Provider string TotpPending bool - OAuthGroups string + OAuthGroups []string TotpEnabled bool }I can add helper utils to convert existing CSV values during session creation until callers are migrated.
Also applies to: 123-133
internal/service/auth_service.go (8)
85-110: LDAP search returns DN into Username field; consider renaming for clarity.SearchUser stores userDN into UserSearch.Username when type is "ldap". It works with VerifyUser (Bind expects DN), but the naming is misleading and easy to misuse.
- Option A: rename field to Identifier or DN for LDAP results.
- Option B: add a DN string field and keep Username for the original identifier.
112-140: Consolidate authentication failure logging.Minor: default branch already logs an "unknown type" message; the trailing "User authentication failed" log executes when search.Type == "ldap" and LDAP == nil, causing mixed semantics. Consider a single, consistent warn-level log at the final return.
142-151: Lower “user not found” log to debug to avoid log noise.Every miss on local users will emit a warn. This can spam logs during brute-force attempts or when LDAP is primary.
- log.Warn().Str("username", username).Msg("Local user not found") + log.Debug().Str("username", username).Msg("Local user not found")
223-251: Per-session MaxAge for TOTP flow and harden session field names.
- When TotpPending, you constrain logical expiry but cookie still uses global MaxAge. Set session.Options.MaxAge = sessionExpiry for that save.
- Optional: pull constant keys for "username"/"expiry" to avoid typos.
if data.TotpPending { sessionExpiry = 3600 } else { sessionExpiry = auth.Config.SessionExpiry } + // Align cookie lifetime with logical expiry + if session.Options == nil { + session.Options = auth.Store.Options + } + session.Options.MaxAge = sessionExpiry
272-309: Be defensive on expiry type and reduce double work.If some stores deserialize numbers differently, expiry could land as int (or float64). Handle those cases, and avoid a second DeleteSessionCookie call if the first fails (log it).
I can provide a small helper to coerce expiry to int64 safely if you’d like.
350-369: Fail-closed behavior for invalid Allowed regex.Currently returns true with an error when regex compilation fails. If callers ignore the error, routes might accidentally require auth (good) or produce surprising behavior. Consider:
- Log the error
- Return false, err to explicitly deny or have the caller decide
371-381: Avoid overloading config.User for BasicAuth credentials.config.User.Password is a hashed password for local users. Here, it carries the raw password. This dual use risks confusion or misuse.
- Introduce a small BasicCreds struct { Username, Password string } for this purpose.
- Or return a tuple and keep parsing separate from local user representation.
383-419: IP allow/block precedence is deny-first; document it.Block list takes precedence over allow list (a blocked IP that’s also allowed is denied). This is fine; a short comment clarifies intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (56)
.env.example(2 hunks).gitignore(1 hunks)air.toml(1 hunks)cmd/root.go(3 hunks)cmd/version.go(2 hunks)frontend/src/context/app-context.tsx(1 hunks)frontend/src/context/user-context.tsx(1 hunks)frontend/src/pages/logout-page.tsx(1 hunks)frontend/src/pages/totp-page.tsx(1 hunks)frontend/vite.config.ts(1 hunks)go.mod(1 hunks)internal/assets/assets.go(1 hunks)internal/auth/auth_test.go(0 hunks)internal/bootstrap/app_bootstrap.go(1 hunks)internal/config/config.go(5 hunks)internal/constants/constants.go(0 hunks)internal/controller/context_controller.go(1 hunks)internal/controller/health_controller.go(1 hunks)internal/controller/oauth_controller.go(1 hunks)internal/controller/proxy_controller.go(1 hunks)internal/controller/resources_controller.go(1 hunks)internal/controller/user_controller.go(1 hunks)internal/handlers/context.go(0 hunks)internal/handlers/handlers.go(0 hunks)internal/handlers/handlers_test.go(0 hunks)internal/handlers/oauth.go(0 hunks)internal/handlers/proxy.go(0 hunks)internal/handlers/user.go(0 hunks)internal/hooks/hooks.go(0 hunks)internal/middleware/context_middleware.go(1 hunks)internal/middleware/ui_middleware.go(1 hunks)internal/middleware/zerolog_middleware.go(1 hunks)internal/oauth/oauth.go(0 hunks)internal/providers/generic.go(0 hunks)internal/providers/github.go(0 hunks)internal/providers/google.go(0 hunks)internal/providers/providers.go(0 hunks)internal/server/server.go(0 hunks)internal/service/auth_service.go(14 hunks)internal/service/docker_service.go(3 hunks)internal/service/generic_oauth_service.go(1 hunks)internal/service/github_oauth_service.go(1 hunks)internal/service/google_oauth_service.go(1 hunks)internal/service/ldap_service.go(5 hunks)internal/service/oauth_broker_service.go(1 hunks)internal/types/api.go(0 hunks)internal/types/types.go(0 hunks)internal/utils/app_utils.go(1 hunks)internal/utils/fs_utils.go(1 hunks)internal/utils/label_utils.go(1 hunks)internal/utils/security_utils.go(1 hunks)internal/utils/string_utils.go(1 hunks)internal/utils/user_utils.go(1 hunks)internal/utils/utils.go(0 hunks)internal/utils/utils_test.go(0 hunks)main.go(1 hunks)
💤 Files with no reviewable changes (19)
- internal/providers/generic.go
- internal/handlers/proxy.go
- internal/types/types.go
- internal/providers/google.go
- internal/constants/constants.go
- internal/utils/utils_test.go
- internal/handlers/user.go
- internal/hooks/hooks.go
- internal/providers/github.go
- internal/handlers/context.go
- internal/handlers/handlers.go
- internal/auth/auth_test.go
- internal/server/server.go
- internal/oauth/oauth.go
- internal/handlers/oauth.go
- internal/handlers/handlers_test.go
- internal/types/api.go
- internal/utils/utils.go
- internal/providers/providers.go
🧰 Additional context used
🧬 Code graph analysis (28)
cmd/version.go (1)
internal/config/config.go (3)
Version(10-10)CommitHash(11-11)BuildTimestamp(12-12)
main.go (1)
internal/server/server.go (1)
zerolog(103-130)
internal/controller/health_controller.go (1)
internal/handlers/handlers.go (1)
HealthcheckHandler(31-36)
internal/utils/label_utils.go (3)
internal/config/config.go (1)
Labels(82-90)internal/utils/utils_test.go (3)
TestGetLabels(224-253)TestParseHeaders(388-413)TestSanitizeHeader(364-386)internal/utils/utils.go (2)
GetLabels(154-164)ParseHeaders(136-151)
internal/utils/string_utils.go (1)
internal/utils/utils_test.go (1)
TestCoalesceToString(515-548)
frontend/src/pages/totp-page.tsx (1)
frontend/src/schemas/totp-schema.ts (1)
TotpSchema(7-7)
internal/controller/resources_controller.go (2)
internal/config/config.go (1)
Config(18-59)internal/server/server.go (1)
NewServer(41-95)
internal/utils/fs_utils.go (2)
internal/utils/utils.go (4)
ReadFile(61-73)ParseSecretFile(214-225)ParseFileToLine(76-88)GetSecret(91-106)internal/utils/utils_test.go (3)
TestReadFile(53-78)TestParseSecretFile(415-426)TestGetSecret(93-133)
internal/middleware/context_middleware.go (6)
internal/config/config.go (2)
Config(18-59)UserContext(123-133)internal/service/auth_service.go (1)
AuthService(37-44)internal/service/oauth_broker_service.go (1)
OAuthBrokerService(18-21)internal/utils/string_utils.go (1)
Capitalize(7-12)internal/handlers/context.go (1)
AppContextHandler(10-35)internal/hooks/hooks.go (1)
UseUserContext(30-144)
internal/middleware/zerolog_middleware.go (2)
internal/bootstrap/app_bootstrap.go (1)
Middleware(20-23)internal/server/server.go (1)
zerolog(103-130)
internal/utils/app_utils.go (1)
internal/config/config.go (1)
UserContext(123-133)
internal/service/generic_oauth_service.go (2)
internal/config/config.go (3)
Config(18-59)OAuthServiceConfig(92-101)Claims(3-8)internal/oauth/oauth.go (2)
NewOAuth(20-44)GetAuthURL(46-48)
internal/service/oauth_broker_service.go (4)
internal/config/config.go (2)
Claims(3-8)OAuthServiceConfig(92-101)internal/service/github_oauth_service.go (1)
NewGithubOAuthService(36-46)internal/service/google_oauth_service.go (1)
NewGoogleOAuthService(31-41)internal/service/generic_oauth_service.go (1)
NewGenericOAuthService(25-40)
internal/controller/user_controller.go (5)
internal/config/config.go (2)
Config(18-59)SessionCookie(114-121)internal/service/auth_service.go (1)
AuthService(37-44)internal/utils/string_utils.go (1)
Capitalize(7-12)internal/utils/app_utils.go (1)
GetContext(50-64)internal/auth/auth.go (1)
CreateSessionCookie(219-253)
internal/utils/user_utils.go (3)
internal/config/config.go (1)
User(103-107)internal/utils/fs_utils.go (1)
ReadFile(5-17)internal/utils/app_utils.go (1)
ParseFileToLine(27-39)
internal/service/github_oauth_service.go (3)
internal/config/config.go (3)
Config(18-59)OAuthServiceConfig(92-101)Claims(3-8)internal/oauth/oauth.go (1)
NewOAuth(20-44)internal/providers/github.go (1)
GetGithubUser(30-102)
internal/middleware/ui_middleware.go (3)
internal/assets/assets.go (1)
FontendAssets(10-10)internal/bootstrap/app_bootstrap.go (1)
Middleware(20-23)internal/server/server.go (1)
NewServer(41-95)
internal/utils/security_utils.go (3)
internal/utils/fs_utils.go (1)
ReadFile(5-17)internal/utils/utils_test.go (1)
TestDeriveKey(497-513)internal/utils/utils.go (3)
GetBasicAuth(284-287)GetSecret(91-106)ParseSecretFile(214-225)
internal/bootstrap/app_bootstrap.go (17)
internal/config/config.go (6)
Config(18-59)SessionCookieName(14-14)CSRFCookieName(15-15)RedirectCookieName(16-16)Version(10-10)OAuthServiceConfig(92-101)internal/utils/user_utils.go (1)
GetUsers(29-51)internal/utils/app_utils.go (1)
GetUpperDomain(15-25)internal/utils/security_utils.go (2)
GenerateIdentifier(120-124)DeriveKey(52-68)internal/service/auth_service.go (2)
AuthServiceConfig(24-35)NewAuthService(46-53)internal/service/ldap_service.go (3)
LdapService(23-26)LdapServiceConfig(14-21)NewLdapService(28-32)internal/service/docker_service.go (1)
NewDockerService(21-23)internal/service/oauth_broker_service.go (1)
NewOAuthBrokerService(23-28)internal/middleware/context_middleware.go (2)
NewContextMiddleware(24-30)ContextMiddlewareConfig(14-16)internal/middleware/ui_middleware.go (1)
NewUIMiddleware(18-20)internal/middleware/zerolog_middleware.go (1)
NewZerologMiddleware(21-23)internal/controller/context_controller.go (2)
NewContextController(51-56)ContextControllerConfig(35-44)internal/controller/oauth_controller.go (2)
NewOAuthController(35-42)OAuthControllerConfig(21-26)internal/controller/proxy_controller.go (2)
NewProxyController(31-38)ProxyControllerConfig(20-22)internal/controller/user_controller.go (2)
NewUserController(34-40)UserControllerConfig(24-26)internal/controller/resources_controller.go (2)
NewResourcesController(18-23)ResourcesControllerConfig(9-11)internal/controller/health_controller.go (1)
NewHealthController(9-13)
internal/controller/proxy_controller.go (7)
internal/config/config.go (4)
Config(18-59)UnauthorizedQuery(135-140)UserContext(123-133)RedirectQuery(142-144)internal/service/docker_service.go (1)
DockerService(16-19)internal/service/auth_service.go (1)
AuthService(37-44)internal/utils/label_utils.go (3)
GetLabels(10-19)ParseHeaders(21-33)SanitizeHeader(35-43)internal/utils/security_utils.go (2)
GetSecret(17-32)GetBasicAuth(47-50)internal/utils/app_utils.go (1)
GetContext(50-64)internal/handlers/proxy.go (1)
ProxyHandler(15-282)
internal/service/google_oauth_service.go (3)
internal/config/config.go (3)
Config(18-59)OAuthServiceConfig(92-101)Claims(3-8)internal/oauth/oauth.go (2)
GetAuthURL(46-48)NewOAuth(20-44)internal/providers/google.go (2)
GoogleScopes(20-22)GetGoogleUser(24-56)
internal/controller/oauth_controller.go (5)
internal/config/config.go (6)
CSRFCookieName(15-15)RedirectCookieName(16-16)Config(18-59)UnauthorizedQuery(135-140)SessionCookie(114-121)RedirectQuery(142-144)internal/service/auth_service.go (1)
AuthService(37-44)internal/service/oauth_broker_service.go (1)
OAuthBrokerService(18-21)internal/utils/string_utils.go (2)
Capitalize(7-12)CoalesceToString(14-30)internal/handlers/oauth.go (2)
h(16-70)h(72-223)
internal/controller/context_controller.go (3)
internal/config/config.go (1)
Config(18-59)internal/utils/app_utils.go (1)
GetContext(50-64)internal/handlers/context.go (2)
UserContextHandler(37-64)AppContextHandler(10-35)
internal/service/auth_service.go (4)
internal/config/config.go (7)
User(103-107)SessionCookieName(14-14)Config(18-59)UserSearch(109-112)SessionCookie(114-121)UserContext(123-133)Labels(82-90)internal/service/docker_service.go (1)
DockerService(16-19)internal/service/ldap_service.go (1)
LdapService(23-26)internal/utils/security_utils.go (2)
CheckFilter(93-118)FilterIP(70-91)
cmd/root.go (4)
internal/config/config.go (2)
Config(18-59)Version(10-10)internal/utils/security_utils.go (1)
GetSecret(17-32)internal/utils/app_utils.go (1)
GetLogLevel(66-83)internal/bootstrap/app_bootstrap.go (1)
NewBootstrapApp(33-37)
internal/config/config.go (1)
internal/ldap/ldap.go (1)
Config(15-18)
internal/service/docker_service.go (2)
internal/utils/label_utils.go (1)
GetLabels(10-19)internal/config/config.go (1)
Labels(82-90)
internal/service/ldap_service.go (1)
internal/config/config.go (1)
Config(18-59)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 8-8: [UnorderedKey] The SECURE_COOKIE key should go before the USERS key
(UnorderedKey)
[warning] 28-28: [UnorderedKey] The LOG_LEVEL key should go before the OAUTH_WHITELIST key
(UnorderedKey)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/service/docker_service.go (1)
25-37: Fixed: client and context are now stored on the service (prevents nil deref).This addresses the earlier panic risk noted in prior review. Good change.
🧹 Nitpick comments (6)
internal/service/docker_service.go (6)
25-37: Prefer WithAPIVersionNegotiation and avoid shadowing package name.Minor cleanup: let the SDK negotiate API version for you and avoid reusing the identifier “client” for the local var.
Apply this diff:
-func (docker *DockerService) Init() error { - client, err := client.NewClientWithOpts(client.FromEnv) +func (docker *DockerService) Init() error { + cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) if err != nil { return err } ctx := context.Background() - client.NegotiateAPIVersion(ctx) - - docker.Client = client + // API version negotiation handled by client.WithAPIVersionNegotiation() + docker.Client = cli docker.Context = ctx return nil }
39-45: Guard against uninitialized service to avoid panics.If Init hasn’t been called, docker.Client is nil and this will panic.
Apply this diff:
func (docker *DockerService) GetContainers() ([]container.Summary, error) { + if docker.Client == nil || docker.Context == nil { + return nil, ErrNotInitialized + } containers, err := docker.Client.ContainerList(docker.Context, container.ListOptions{}) if err != nil { return nil, err } return containers, nil }Add this once in the file (see imports comment below):
var ErrNotInitialized = errors.New("docker service not initialized")
47-53: Same guard needed here.Mirror the nil check to prevent a panic in InspectContainer.
func (docker *DockerService) InspectContainer(containerId string) (container.InspectResponse, error) { + if docker.Client == nil || docker.Context == nil { + return container.InspectResponse{}, ErrNotInitialized + } inspect, err := docker.Client.ContainerInspect(docker.Context, containerId) if err != nil { return container.InspectResponse{}, err } return inspect, nil }
55-58: Add nil guard and a short timeout to Ping().Prevents panics and avoids blocking indefinitely if the daemon is slow/unreachable.
func (docker *DockerService) DockerConnected() bool { - _, err := docker.Client.Ping(docker.Context) + if docker.Client == nil || docker.Context == nil { + return false + } + ctx, cancel := context.WithTimeout(docker.Context, 2*time.Second) + defer cancel() + _, err := docker.Client.Ping(ctx) return err == nil }
3-14: Imports needed for suggested guards and timeout.If you adopt the guards/timeouts above, add these imports.
import ( "context" + "errors" + "time" "strings" "tinyauth/internal/config" "tinyauth/internal/utils" "slices"
73-96: Avoid N API-inspect calls; use summary labels and rename loop var to avoid import shadowing.Inspecting every container is expensive; Summary already has Labels. Also, the loop variable named “container” shadows the imported package and reduces readability.
- for _, container := range containers { - inspect, err := docker.InspectContainer(container.ID) - if err != nil { - log.Warn().Str("id", container.ID).Err(err).Msg("Error inspecting container, skipping") - continue - } - - labels, err := utils.GetLabels(inspect.Config.Labels) + for _, ctr := range containers { + labels, err := utils.GetLabels(ctr.Labels) if err != nil { - log.Warn().Str("id", container.ID).Err(err).Msg("Error getting container labels, skipping") + log.Warn().Str("id", ctr.ID).Err(err).Msg("Error getting container labels, skipping") continue } // Check if the container matches the ID or domain - if slices.Contains(labels.Domain, domain) { - log.Debug().Str("id", inspect.ID).Msg("Found matching container by domain") + if slices.Contains(labels.Domain, domain) { + log.Debug().Str("id", ctr.ID).Msg("Found matching container by domain") return labels, nil } - if strings.TrimPrefix(inspect.Name, "/") == app { - log.Debug().Str("id", inspect.ID).Msg("Found matching container by name") - return labels, nil - } + if slices.ContainsFunc(ctr.Names, func(n string) bool { return strings.TrimPrefix(n, "/") == app }) { + log.Debug().Str("id", ctr.ID).Msg("Found matching container by name") + return labels, nil + } }Optional: compare domains case-insensitively if labels/domains may vary in case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
internal/bootstrap/app_bootstrap.go(1 hunks)internal/controller/proxy_controller.go(1 hunks)internal/service/docker_service.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/proxy_controller.go
- internal/bootstrap/app_bootstrap.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/service/docker_service.go (3)
internal/utils/label_utils.go (1)
GetLabels(10-19)internal/config/config.go (1)
Labels(82-90)internal/docker/docker.go (2)
NewDocker(19-32)GetLabels(55-102)
🔇 Additional comments (2)
internal/service/docker_service.go (2)
60-101: No types.Labels usage found; ignore outdated suggestionVerified via ripgrep that there are no references to
types.Labelsin the codebase. The only caller ofDockerService.GetLabelsis ininternal/controller/proxy_controller.go, which already handles both errors and the zero‐value ofconfig.Labelscorrectly. You can safely disregard the original note about migrating offtypes.Labels.Likely an incorrect or invalid review comment.
9-10: slices import support verifiedgo.mod specifies Go version 1.23 (≥ 1.21), so the import of the standard "slices" package is valid. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/pages/login-page.tsx (2)
67-71: Optional: centralize API paths to avoid future sweeping renames.Defining route constants (or a typed API client) reduces churn across the app when endpoints move again.
Apply this minimal change here:
- mutationFn: (values: LoginSchema) => axios.post("/api/user/login", values), + mutationFn: (values: LoginSchema) => axios.post(USER_LOGIN_URL, values),And add a shared constant near your API utilities (recommended), or locally at the top of this file if you prefer incremental changes:
// e.g., frontend/src/lib/api/routes.ts export const USER_LOGIN_URL = "/api/user/login";
165-165: Use strict equality for length check.Minor: prefer
===to avoid unintended type coercion.- {configuredProviders.length == 0 && ( + {configuredProviders.length === 0 && (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/src/pages/login-page.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/login-page.tsx (2)
frontend/src/schemas/login-schema.ts (1)
LoginSchema(8-8)frontend/src/components/auth/login-form.tsx (1)
FormItem(59-81)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/service/auth_service.go (2)
223-244: Per-session expiry: set MaxAge dynamically (TOTP vs normal).You compute sessionExpiry but don’t apply it to the cookie. Set session.Options.MaxAge accordingly so browser expiry matches server-side checks.
Apply:
var sessionExpiry int @@ if data.TotpPending { sessionExpiry = 3600 } else { sessionExpiry = auth.Config.SessionExpiry } + // Ensure cookie expiry aligns with our chosen session expiry + if session.Options == nil { + session.Options = &sessions.Options{} + } + session.Options.MaxAge = sessionExpiry + session.Values["username"] = data.Username
253-270: DeleteSessionCookie should set MaxAge = -1 to expire the cookie.Clearing values and saving persists an empty cookie. Explicitly expire it.
Apply:
func (auth *AuthService) DeleteSessionCookie(c *gin.Context) error { session, err := auth.GetSession(c) if err != nil { return err } - // Delete all values in the session - for key := range session.Values { - delete(session.Values, key) - } + // Expire the cookie in the browser + if session.Options == nil { + session.Options = &sessions.Options{} + } + session.Options.MaxAge = -1 + // Clear server-side values too + for k := range session.Values { + delete(session.Values, k) + } err = session.Save(c.Request, c.Writer) if err != nil { return err } return nil }
♻️ Duplicate comments (14)
internal/controller/resources_controller.go (2)
33-42: Harden empty-dir handling and consider disabling directory listings.
- You guard at runtime, which is good, but you still constructed a file server backed by
http.Dir(config.ResourcesDir)in the constructor. IfResourcesDir == "",http.Dir("")points to the process working directory. Even if guarded, prefer not to create it at all.- If directory listings are not desired, wrap the filesystem to deny directory reads.
Minimal guard:
@@ -func NewResourcesController(config ResourcesControllerConfig, router *gin.RouterGroup) *ResourcesController { +func NewResourcesController(config ResourcesControllerConfig, router *gin.RouterGroup) *ResourcesController { @@ - fileServer := http.StripPrefix(prefix, http.FileServer(http.Dir(config.ResourcesDir))) + var fileServer http.Handler = http.NotFoundHandler() + if config.ResourcesDir != "" { + fileServer = http.StripPrefix(prefix, http.FileServer(http.Dir(config.ResourcesDir))) + } @@ }Optional no-directory-listing wrapper:
type noDirFS struct{ http.FileSystem } func (n noDirFS) Open(name string) (http.File, error) { f, err := n.FileSystem.Open(name) if err != nil { return nil, err } if st, err := f.Stat(); err == nil && st.IsDir() { _ = f.Close() return nil, fs.ErrPermission } return f, nil }Then build the file server with
http.FileServer(noDirFS{http.Dir(config.ResourcesDir)}).
19-27: Fix StripPrefix to include the router group base path; current handler will 404 when mounted under a non-root group (e.g., "/api").
http.StripPrefix("/resources", ...)ignoresrouter.BasePath(). If this controller is attached to a group with base path (e.g.,/api), requests like/api/resources/...won’t have the correct prefix stripped and will not resolve files. Compute the effective prefix once in the constructor.Apply:
@@ package controller import ( + "strings" "net/http" "github.com/gin-gonic/gin" ) @@ type ResourcesController struct { Config ResourcesControllerConfig Router *gin.RouterGroup FileServer http.Handler + prefix string } @@ func NewResourcesController(config ResourcesControllerConfig, router *gin.RouterGroup) *ResourcesController { - fileServer := http.StripPrefix("/resources", http.FileServer(http.Dir(config.ResourcesDir))) + // Build effective prefix: <basePath>/resources (basePath may be "/" or ""). + base := strings.TrimRight(router.BasePath(), "/") + prefix := base + "/resources" + fileServer := http.StripPrefix(prefix, http.FileServer(http.Dir(config.ResourcesDir))) return &ResourcesController{ Config: config, Router: router, FileServer: fileServer, + prefix: prefix, } }internal/assets/assets.go (1)
7-10: Keep a deprecated compatibility alias to avoid breaking older references (Assets, FontendAssets).This refactor renamed the exported variable; creating an alias costs nothing and eases upgrades.
//go:embed dist -var FrontendAssets embed.FS +var FrontendAssets embed.FS + +// Deprecated: use FrontendAssets. +var FontendAssets = FrontendAssets + +// Deprecated: use FrontendAssets. +var Assets = FrontendAssetsRun to confirm no lingering references remain:
#!/bin/bash rg -nP '\bassets\.(Assets|FontendAssets)\b' -S -g '!**/vendor/**'internal/middleware/ui_middleware.go (1)
22-33: Good: Init no longer swallows errors; avoids nil server panics.This addresses the prior concern. Thanks.
internal/controller/user_controller.go (3)
115-131: Good: TOTP-pending cookie errors are handled.Prevents misleading success responses when cookie write fails.
141-156: Good: Normal login cookie errors are handled.This resolves a prior gap.
198-205: Good: Gate on TotpPending instead of IsLoggedIn.Aligns with the intended 2-step flow.
cmd/root.go (1)
85-86: Nice: flag now matches mapstructure key (“secure-cookie”).Resolves the earlier mismatch that prevented the field from being populated.
internal/middleware/context_middleware.go (1)
80-84: Deleting invalid OAuth session cookie — niceWhen the provider is not registered, you now delete the stale cookie before falling back to basic auth. This prevents repeated work on every request.
internal/service/github_oauth_service.go (2)
71-79: Resolved: propagate Exchange errors (thank you for fixing this).Returning the actual error from oauth2.Exchange is correct and prevents masking failures.
87-103: Resolved: set Accept header and validate HTTP status for GitHub APIs.The new requests add Accept: application/vnd.github+json and guard on non-2xx before unmarshalling. Looks good.
Also applies to: 116-132
internal/config/config.go (1)
66-74: BC alias for misspelled PassowrdLabels.Previous public API exported PassowrdLabels. To avoid breakage, keep an alias to the corrected PasswordLabels.
Apply:
type PasswordLabels struct { Plain string File string } + +// Backwards-compatibility alias for the historical typo. +// Deprecated: use PasswordLabels. +type PassowrdLabels = PasswordLabelsinternal/service/auth_service.go (2)
55-67: Cookie store: validate key lengths, fix Domain, add SameSite (was flagged earlier).
- AES block key must be 16/24/32 bytes; validate EncryptionSecret up-front.
- Don’t auto-prefix domain with a dot; empty domain becomes "." which is invalid and breaks localhost.
- Add SameSite=Lax for auth flows.
Apply:
import ( "fmt" + "net/http" "regexp" "strings" "sync" "time" "tinyauth/internal/config" "tinyauth/internal/utils" @@ func (auth *AuthService) Init() error { - store := sessions.NewCookieStore([]byte(auth.Config.HMACSecret), []byte(auth.Config.EncryptionSecret)) + // Validate encryption key length (16/24/32 bytes) early + if l := len(auth.Config.EncryptionSecret); l != 0 && l != 16 && l != 24 && l != 32 { + return fmt.Errorf("invalid EncryptionSecret length: got %d, want 16/24/32", l) + } + if len(auth.Config.HMACSecret) < 32 { + return fmt.Errorf("HMACSecret too short: got %d, want >= 32", len(auth.Config.HMACSecret)) + } + store := sessions.NewCookieStore([]byte(auth.Config.HMACSecret), []byte(auth.Config.EncryptionSecret)) store.Options = &sessions.Options{ Path: "/", MaxAge: auth.Config.SessionExpiry, Secure: auth.Config.SecureCookie, HttpOnly: true, - Domain: fmt.Sprintf(".%s", auth.Config.Domain), + // Set host-only cookie when Domain is empty. Do not auto-prepend a dot. + Domain: auth.Config.Domain, + SameSite: http.SameSiteLaxMode, } auth.Store = store return nil }
69-77: Clearing broken cookie: don’t prepend dot to domain.Empty Domain becomes ".", which many browsers reject. Use the configured domain as-is.
Apply:
- c.SetCookie(auth.Config.SessionCookieName, "", -1, "/", fmt.Sprintf(".%s", auth.Config.Domain), auth.Config.SecureCookie, true) + c.SetCookie(auth.Config.SessionCookieName, "", -1, "/", auth.Config.Domain, auth.Config.SecureCookie, true)
🧹 Nitpick comments (27)
internal/controller/resources_controller.go (1)
29-31: Nice, route surface is small and explicit. Consider adding HEAD if clients probe assets with HEAD.Not required, but cheap to add:
func (controller *ResourcesController) SetupRoutes() { - controller.Router.GET("/resources/*resource", controller.resourcesHandler) + controller.Router.GET("/resources/*resource", controller.resourcesHandler) + controller.Router.HEAD("/resources/*resource", controller.resourcesHandler) }internal/middleware/ui_middleware.go (3)
35-44: Avoid brittle indexing into Split; normalize path and grab first segment safely.
strings.Split(path, "/")[1]relies on assumptions and can misbehave with unusual paths (//x, trailing slashes). Normalize and use SplitN.@@ - return func(c *gin.Context) { - switch strings.Split(c.Request.URL.Path, "/")[1] { + return func(c *gin.Context) { + p := strings.TrimPrefix(path.Clean(c.Request.URL.Path), "/") + first := p + if i := strings.IndexByte(p, '/'); i >= 0 { + first = p[:i] + } + switch first { case "api": c.Next() return case "resources": c.Next() returnNote: add the import:
-import ( +import ( "io/fs" "net/http" "os" "strings" "tinyauth/internal/assets" + "path"
45-56: SPA fallback is fine; consider handling other fs.Stat errors the same as NotExist.Non-NotExist errors (e.g., invalid path) currently fall through without rewriting; treating them like NotExist may improve resilience.
- _, err := fs.Stat(m.UIFS, strings.TrimPrefix(c.Request.URL.Path, "/")) - - if os.IsNotExist(err) { + _, err := fs.Stat(m.UIFS, strings.TrimPrefix(c.Request.URL.Path, "/")) + if err != nil { c.Request.URL.Path = "/" }
13-16: Optional: add a quick nil check to fail fast if Init wasn’t called.Defensive, in case wiring changes.
func (m *UIMiddleware) Middleware() gin.HandlerFunc { - return func(c *gin.Context) { + return func(c *gin.Context) { + if m.UIFileServer == nil || m.UIFS == nil { + c.AbortWithStatusJSON(http.StatusInternalServerError, gin.H{"status": 500, "message": "UI not initialized"}) + return + }internal/controller/user_controller.go (3)
163-172: Consider handling DeleteSessionCookie error (if it returns one).If
DeleteSessionCookiecan fail, return 500 to avoid implying logout succeeded when it didn’t.- controller.Auth.DeleteSessionCookie(c) + if err := controller.Auth.DeleteSessionCookie(c); err != nil { + log.Error().Err(err).Msg("Failed to delete session cookie") + c.JSON(500, gin.H{"status": 500, "message": "Internal Server Error"}) + return + }
221-224: Nit: message mentions “login attempts” in TOTP flow; consider “TOTP attempts”.Minor clarity tweak.
- "message": fmt.Sprintf("Too many failed login attempts. Try again in %d seconds", remainingTime), + "message": fmt.Sprintf("Too many failed TOTP attempts. Try again in %d seconds", remainingTime),
228-239: Handle missing or invalid local user before TOTP validation.If the user was removed or has an empty secret, short-circuit with 401 rather than validating against an empty secret.
user := controller.Auth.GetLocalUser(context.Username) - ok := totp.Validate(req.Code, user.TotpSecret) + if user.Username == "" || user.TotpSecret == "" { + log.Warn().Str("username", context.Username).Msg("TOTP attempted for unknown user or without secret") + controller.Auth.RecordLoginAttempt(rateIdentifier, false) + c.JSON(401, gin.H{"status": 401, "message": "Unauthorized"}) + return + } + + ok := totp.Validate(req.Code, user.TotpSecret)cmd/root.go (1)
120-135: Env binding loop looks good; consider a replacer to avoid manual binds next time.Not required, but
viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))would let you callviper.AutomaticEnv()without per-flagBindEnv.internal/utils/app_utils.go (2)
76-102: Consider allowing safe relative redirects in IsRedirectSafe (reduces open-redirect surface and UX friction)Requiring absolute URLs only is stricter than necessary and forces callers to embed full URLs. A common pattern is to allow relative-path redirects ("/foo") and, for absolute URLs, restrict to the same registrable domain.
Optional refinement:
- Accept relative paths starting with "/" that don’t start with "//".
- Keep the current same-registrable-domain check for absolute URLs.
Apply this diff:
func IsRedirectSafe(redirectURL string, domain string) bool { if redirectURL == "" { return false } parsedURL, err := url.Parse(redirectURL) if err != nil { return false } - if !parsedURL.IsAbs() { - return false - } - - upper, err := GetUpperDomain(redirectURL) - if err != nil { - return false - } - - if upper != domain { - return false - } - - return true + // Allow relative paths like "/next", but reject protocol-relative ("//") or bare paths w/o leading slash. + if !parsedURL.IsAbs() { + return strings.HasPrefix(redirectURL, "/") && !strings.HasPrefix(redirectURL, "//") + } + // Absolute URL: must match app's upper domain. + upper, err := GetUpperDomain(redirectURL) + if err != nil { + return false + } + return upper == domain }
60-74: Make GetContext robust to both pointer and value storageCall sites set a pointer (&config.UserContext). If any future code stores a value (config.UserContext), this function will currently fail the assertion. Handle both to improve ergonomics.
Apply this diff:
func GetContext(c *gin.Context) (config.UserContext, error) { userContextValue, exists := c.Get("context") if !exists { return config.UserContext{}, errors.New("no user context in request") } - userContext, ok := userContextValue.(*config.UserContext) - - if !ok { - return config.UserContext{}, errors.New("invalid user context in request") - } - - return *userContext, nil + switch v := userContextValue.(type) { + case *config.UserContext: + return *v, nil + case config.UserContext: + return v, nil + default: + return config.UserContext{}, errors.New("invalid user context in request") + } }internal/middleware/context_middleware.go (1)
45-56: Keep provider from cookie in TOTP-pending contextYou hard-code Provider: "username" during TOTP flow. If the cookie was issued with a different provider value (or future variants), this will mislabel the session context. Use the provider from the cookie.
Apply this diff:
if cookie.TotpPending { c.Set("context", &config.UserContext{ Username: cookie.Username, Name: cookie.Name, Email: cookie.Email, - Provider: "username", + Provider: cookie.Provider, TotpPending: true, TotpEnabled: true, }) c.Next() return }internal/bootstrap/app_bootstrap.go (1)
211-223: Avoid shadowing imported package name ‘controller’Using a local slice named controller shadows the imported package name controller, which reduces readability and can confuse tooling.
Apply this diff:
- controller := []Controller{ + controllers := []Controller{ contextController, oauthController, proxyController, userController, healthController, resourcesController, } - for _, ctrl := range controller { + for _, ctrl := range controllers { log.Debug().Msgf("Setting up %T controller", ctrl) ctrl.SetupRoutes() }internal/service/generic_oauth_service.go (2)
66-70: Harden state generation: check entropy errors and avoid paddingCheck rand.Read errors, and use RawURLEncoding to avoid trailing '=' in cookies/URLs.
Apply this diff:
func (generic *GenericOAuthService) GenerateState() string { - b := make([]byte, 128) - rand.Read(b) - state := base64.URLEncoding.EncodeToString(b) + b := make([]byte, 32) // 256 bits of entropy is plenty + if _, err := rand.Read(b); err != nil { + // Extremely unlikely; fall back to verifier which is already cryptographically strong + return oauth2.GenerateVerifier() + } + state := base64.RawURLEncoding.EncodeToString(b) return state }
90-100: Defensive checks and explicit JSON Accept for Userinfo
- Guard against nil Token to avoid surprising nil derefs if VerifyCode wasn’t called.
- Prefer an explicit request with Accept: application/json.
- (Optional) Consider a short timeout via context.WithTimeout.
Apply this diff:
- client := generic.Config.Client(generic.Context, generic.Token) - - res, err := client.Get(generic.UserinfoURL) + if generic.Token == nil { + return user, fmt.Errorf("no OAuth token available; call VerifyCode first") + } + client := generic.Config.Client(generic.Context, generic.Token) + req, err := http.NewRequestWithContext(generic.Context, http.MethodGet, generic.UserinfoURL, nil) + if err != nil { + return user, err + } + req.Header.Set("Accept", "application/json") + res, err := client.Do(req) if err != nil { return user, err } defer res.Body.Close()internal/controller/oauth_controller.go (4)
64-77: Avoid shadowing imported package ‘service’: rename local variableThe local variable service shadows the imported package tinyauth/internal/service. Rename to oauthSvc for clarity.
Apply this diff:
- service, exists := controller.Broker.GetService(req.Provider) + oauthSvc, exists := controller.Broker.GetService(req.Provider) if !exists { ... } - state := service.GenerateState() - authURL := service.GetAuthURL(state) + state := oauthSvc.GenerateState() + authURL := oauthSvc.GetAuthURL(state)
118-131: Same variable shadowing in callback handlerRepeat the rename here to keep consistency.
Apply this diff:
- service, exists := controller.Broker.GetService(req.Provider) + oauthSvc, exists := controller.Broker.GetService(req.Provider) if !exists { ... } - err = service.VerifyCode(code) + err = oauthSvc.VerifyCode(code)
168-170: Split email only once to avoid unexpected splitsUse SplitN to handle emails with multiple '@' safely and avoid double Split calls.
Apply this diff:
- name = fmt.Sprintf("%s (%s)", utils.Capitalize(strings.Split(user.Email, "@")[0]), strings.Split(user.Email, "@")[1]) + parts := strings.SplitN(user.Email, "@", 2) + local, domain := parts[0], "" + if len(parts) == 2 { + domain = parts[1] + } + name = fmt.Sprintf("%s (%s)", utils.Capitalize(local), domain)
79-85: Redirect URI validation is much better; consider allowing relative paths only (optional)You now gate redirects with utils.IsRedirectSafe(domain-matching). That’s good. If you want to further reduce cross-subdomain risks, consider accepting only relative paths (e.g., "/continue") here and in the callback (paired with the optional IsRedirectSafe change). That model eliminates cross-origin redirects entirely.
Also applies to: 180-186
internal/service/github_oauth_service.go (5)
92-92: Add a proper User-Agent header for GitHub API requests.GitHub requires a valid User-Agent; default Go UA is generic. Send a descriptive UA including version.
Apply:
-req.Header.Set("Accept", "application/vnd.github+json") +req.Header.Set("Accept", "application/vnd.github+json") +req.Header.Set("User-Agent", "tinyauth/"+config.Version) @@ -req.Header.Set("Accept", "application/vnd.github+json") +req.Header.Set("Accept", "application/vnd.github+json") +req.Header.Set("User-Agent", "tinyauth/"+config.Version)Also applies to: 121-121
20-23: Prefer verified primary email; fall back sensibly.GitHub’s email objects include a Verified flag. Prefer a verified primary; otherwise a verified email; finally fall back to first if none are verified.
Apply:
type GithubEmailResponse []struct { Email string `json:"email"` Primary bool `json:"primary"` + Verified bool `json:"verified"` } @@ - for _, email := range emails { - if email.Primary { - user.Email = email.Email - break - } - } + // Prefer verified primary + for _, email := range emails { + if email.Primary && email.Verified { + user.Email = email.Email + break + } + } + // If none selected, prefer any verified email + if user.Email == "" { + for _, email := range emails { + if email.Verified { + user.Email = email.Email + break + } + } + } @@ - // Use first available email if no primary email was found + // Use first available email if no verified email was found if user.Email == "" { user.Email = emails[0].Email }Also applies to: 145-159
100-103: Improve error messages with endpoint context.Include the endpoint path in the error to simplify troubleshooting.
Apply:
- if res.StatusCode < 200 || res.StatusCode >= 300 { - return user, fmt.Errorf("request failed with status: %s", res.Status) - } + if res.StatusCode < 200 || res.StatusCode >= 300 { + return user, fmt.Errorf("github GET /user failed: %s", res.Status) + } @@ - if res.StatusCode < 200 || res.StatusCode >= 300 { - return user, fmt.Errorf("request failed with status: %s", res.Status) - } + if res.StatusCode < 200 || res.StatusCode >= 300 { + return user, fmt.Errorf("github GET /user/emails failed: %s", res.Status) + }Also applies to: 129-132
37-47: Allow overriding default scopes from config.Hard-coding GithubOAuthScopes limits flexibility. Use config.Scopes when provided, fallback to defaults otherwise.
Apply:
func NewGithubOAuthService(config config.OAuthServiceConfig) *GithubOAuthService { + scopes := config.Scopes + if len(scopes) == 0 { + scopes = GithubOAuthScopes + } return &GithubOAuthService{ Config: oauth2.Config{ ClientID: config.ClientID, ClientSecret: config.ClientSecret, RedirectURL: config.RedirectURL, - Scopes: GithubOAuthScopes, + Scopes: scopes, Endpoint: endpoints.GitHub, }, } }
67-69: Minor: AccessTypeOffline is Google-specific; safe to drop for GitHub.GitHub ignores access_type; leaving it is harmless but unnecessary.
Apply:
-return github.Config.AuthCodeURL(state, oauth2.AccessTypeOffline, oauth2.S256ChallengeOption(github.Verifier)) +return github.Config.AuthCodeURL(state, oauth2.S256ChallengeOption(github.Verifier))internal/config/config.go (2)
14-16: Use constants for cookie names to prevent accidental mutation.These identifiers are effectively constants; switching to const improves safety.
Apply:
-var SessionCookieName = "tinyauth-session" -var CSRFCookieName = "tinyauth-csrf" -var RedirectCookieName = "tinyauth-redirect" +const SessionCookieName = "tinyauth-session" +const CSRFCookieName = "tinyauth-csrf" +const RedirectCookieName = "tinyauth-redirect"
50-51: Typo: FogotPasswordMessage (consider future rename plan).Spelling is off; consider introducing ForgotPasswordMessage in a future release, maintaining mapstructure compatibility or migration logic.
Happy to sketch a migration approach that preserves mapstructure compatibility and avoids breaking consumers.
internal/service/auth_service.go (2)
93-105: LDAP search: guard against empty DN results.If Search returns no error but an empty DN, subsequent Bind will fail ambiguously. Treat empty DN as “unknown”.
Apply:
if auth.LDAP != nil { userDN, err := auth.LDAP.Search(username) if err != nil { log.Warn().Err(err).Str("username", username).Msg("Failed to search for user in LDAP") return config.UserSearch{} } - return config.UserSearch{ + if strings.TrimSpace(userDN) == "" { + return config.UserSearch{Type: "unknown"} + } + return config.UserSearch{ Username: userDN, Type: "ldap", } }
330-334: Magic string “generic”: consider a typed constant.Makes provider checks less error-prone.
Apply:
- if context.Provider != "generic" { + const providerGeneric = "generic" + if context.Provider != providerGeneric {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
cmd/root.go(3 hunks)internal/assets/assets.go(1 hunks)internal/bootstrap/app_bootstrap.go(1 hunks)internal/config/config.go(5 hunks)internal/controller/oauth_controller.go(1 hunks)internal/controller/proxy_controller.go(1 hunks)internal/controller/resources_controller.go(1 hunks)internal/controller/user_controller.go(1 hunks)internal/middleware/context_middleware.go(1 hunks)internal/middleware/ui_middleware.go(1 hunks)internal/middleware/zerolog_middleware.go(1 hunks)internal/service/auth_service.go(14 hunks)internal/service/generic_oauth_service.go(1 hunks)internal/service/github_oauth_service.go(1 hunks)internal/service/google_oauth_service.go(1 hunks)internal/utils/app_utils.go(1 hunks)internal/utils/label_utils.go(1 hunks)internal/utils/user_utils.go(1 hunks)main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/utils/label_utils.go
- internal/middleware/zerolog_middleware.go
- internal/service/google_oauth_service.go
- internal/controller/proxy_controller.go
- main.go
- internal/utils/user_utils.go
🧰 Additional context used
🧬 Code graph analysis (12)
internal/middleware/context_middleware.go (7)
internal/config/config.go (2)
Config(18-59)UserContext(123-133)internal/service/auth_service.go (1)
AuthService(37-44)internal/service/oauth_broker_service.go (1)
OAuthBrokerService(18-21)internal/bootstrap/app_bootstrap.go (1)
Middleware(20-23)internal/utils/string_utils.go (1)
Capitalize(7-12)internal/handlers/context.go (1)
AppContextHandler(10-35)internal/hooks/hooks.go (1)
UseUserContext(30-144)
internal/service/github_oauth_service.go (2)
internal/config/config.go (3)
Config(18-59)OAuthServiceConfig(92-101)Claims(3-8)internal/providers/github.go (2)
GithubScopes(26-28)GetGithubUser(30-102)
internal/utils/app_utils.go (2)
internal/config/config.go (1)
UserContext(123-133)internal/utils/utils.go (2)
GetUpperDomain(48-58)ParseFileToLine(76-88)
internal/bootstrap/app_bootstrap.go (17)
internal/config/config.go (6)
Config(18-59)SessionCookieName(14-14)CSRFCookieName(15-15)RedirectCookieName(16-16)Version(10-10)OAuthServiceConfig(92-101)internal/utils/user_utils.go (1)
GetUsers(38-61)internal/utils/app_utils.go (1)
GetUpperDomain(16-35)internal/utils/security_utils.go (2)
GenerateIdentifier(120-124)DeriveKey(52-68)internal/service/auth_service.go (2)
AuthServiceConfig(24-35)NewAuthService(46-53)internal/service/ldap_service.go (3)
LdapService(23-26)LdapServiceConfig(14-21)NewLdapService(28-32)internal/service/docker_service.go (1)
NewDockerService(21-23)internal/service/oauth_broker_service.go (1)
NewOAuthBrokerService(23-28)internal/middleware/context_middleware.go (2)
NewContextMiddleware(24-30)ContextMiddlewareConfig(14-16)internal/middleware/ui_middleware.go (1)
NewUIMiddleware(18-20)internal/middleware/zerolog_middleware.go (1)
NewZerologMiddleware(21-23)internal/controller/context_controller.go (2)
NewContextController(51-56)ContextControllerConfig(35-44)internal/controller/oauth_controller.go (2)
NewOAuthController(36-43)OAuthControllerConfig(21-27)internal/controller/proxy_controller.go (2)
NewProxyController(31-38)ProxyControllerConfig(20-22)internal/controller/user_controller.go (2)
NewUserController(34-40)UserControllerConfig(24-26)internal/controller/resources_controller.go (2)
NewResourcesController(19-27)ResourcesControllerConfig(9-11)internal/controller/health_controller.go (1)
NewHealthController(9-13)
internal/controller/user_controller.go (5)
internal/config/config.go (2)
Config(18-59)SessionCookie(114-121)internal/service/auth_service.go (1)
AuthService(37-44)internal/utils/string_utils.go (1)
Capitalize(7-12)internal/utils/app_utils.go (1)
GetContext(60-74)internal/auth/auth.go (1)
CreateSessionCookie(219-253)
internal/service/generic_oauth_service.go (1)
internal/config/config.go (3)
Config(18-59)OAuthServiceConfig(92-101)Claims(3-8)
internal/config/config.go (1)
internal/ldap/ldap.go (1)
Config(15-18)
internal/controller/resources_controller.go (2)
internal/config/config.go (1)
Config(18-59)internal/server/server.go (1)
NewServer(41-95)
cmd/root.go (4)
internal/config/config.go (1)
Config(18-59)internal/utils/security_utils.go (1)
GetSecret(17-32)internal/utils/app_utils.go (1)
GetLogLevel(104-123)internal/bootstrap/app_bootstrap.go (1)
NewBootstrapApp(33-37)
internal/middleware/ui_middleware.go (3)
internal/assets/assets.go (1)
FrontendAssets(10-10)internal/bootstrap/app_bootstrap.go (1)
Middleware(20-23)internal/server/server.go (2)
NewServer(41-95)zerolog(103-130)
internal/controller/oauth_controller.go (6)
internal/config/config.go (6)
CSRFCookieName(15-15)RedirectCookieName(16-16)Config(18-59)UnauthorizedQuery(135-140)SessionCookie(114-121)RedirectQuery(142-144)internal/service/auth_service.go (1)
AuthService(37-44)internal/service/oauth_broker_service.go (1)
OAuthBrokerService(18-21)internal/utils/app_utils.go (1)
IsRedirectSafe(76-102)internal/utils/string_utils.go (2)
Capitalize(7-12)CoalesceToString(14-30)internal/oauth/oauth.go (2)
NewOAuth(20-44)GetAuthURL(46-48)
internal/service/auth_service.go (4)
internal/config/config.go (7)
User(103-107)SessionCookieName(14-14)Config(18-59)UserSearch(109-112)SessionCookie(114-121)UserContext(123-133)Labels(82-90)internal/service/docker_service.go (1)
DockerService(16-19)internal/service/ldap_service.go (1)
LdapService(23-26)internal/utils/security_utils.go (2)
CheckFilter(93-118)FilterIP(70-91)
⏰ 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: Analyze (go)
🔇 Additional comments (4)
internal/utils/app_utils.go (1)
104-123: Trace log level mapping — good catchExplicitly handling "trace" aligns with config validation and zerolog. Looks good.
internal/bootstrap/app_bootstrap.go (1)
47-59: The script above will locateGenerateIdentifierin theinternal/utilspackage so we can inspect if its behavior alongsideGetUpperDomainproperly handles apex domains, localhost, and IPs. Once we have that, we can confirm whether the original concerns about broken cookie names hold or if any additional adjustments are needed.internal/config/config.go (1)
92-101: Nice: OAuthServiceConfig consolidates provider settings.The shape looks good and aligns with the new services.
internal/service/auth_service.go (1)
300-308: LGTM: session cookie extraction and validation logic is clear.Checks for required fields and expiry are straightforward.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
internal/service/github_oauth_service.go (4)
61-69: Right-size the OAuth state; 32 random bytes are sufficient128 random bytes produce a ~172-char URL param and add no material security. 32 bytes is standard and plenty.
func (github *GithubOAuthService) GenerateState() string { - b := make([]byte, 128) + b := make([]byte, 32) _, err := rand.Read(b) if err != nil { return base64.RawURLEncoding.EncodeToString(fmt.Appendf(nil, "state-%d", time.Now().UnixNano())) } state := base64.RawURLEncoding.EncodeToString(b) return state }
104-106: Improve error messages with endpoint context and body snippetCurrent error loses which endpoint failed and any API error detail. Include the path and a short body snippet for diagnostics.
- if res.StatusCode < 200 || res.StatusCode >= 300 { - return user, fmt.Errorf("request failed with status: %s", res.Status) - } + if res.StatusCode < 200 || res.StatusCode >= 300 { + msg, _ := io.ReadAll(io.LimitReader(res.Body, 2048)) + return user, fmt.Errorf("GET /user failed: %s: %s", res.Status, string(msg)) + } @@ - if res.StatusCode < 200 || res.StatusCode >= 300 { - return user, fmt.Errorf("request failed with status: %s", res.Status) - } + if res.StatusCode < 200 || res.StatusCode >= 300 { + msg, _ := io.ReadAll(io.LimitReader(res.Body, 2048)) + return user, fmt.Errorf("GET /user/emails failed: %s: %s", res.Status, string(msg)) + }Also applies to: 133-135
50-58: Add HTTP client timeout to avoid indefinite hangsThe OAuth flow and subsequent API calls can hang forever on network issues. Set a sane timeout on the HTTP client used in the context.
func (github *GithubOAuthService) Init() error { - httpClient := &http.Client{} + httpClient := &http.Client{Timeout: 15 * time.Second} ctx := context.Background() ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient) verifier := oauth2.GenerateVerifier()
86-92: Guard against missing/invalid token before building the clientIf VerifyCode wasn’t called (or failed), Token may be nil/invalid. Fail fast with a clear error.
func (github *GithubOAuthService) Userinfo() (config.Claims, error) { var user config.Claims - client := github.Config.Client(github.Context, github.Token) + if github.Token == nil || !github.Token.Valid() || github.Token.AccessToken == "" { + return user, errors.New("github oauth: token not present; call VerifyCode first") + } + client := github.Config.Client(github.Context, github.Token)cmd/root.go (3)
7-12: Correct validator import path and shadowing fix — LGTMImporting validator via github.com/go-playground/validator/v10 and using a local variable v avoids the earlier shadowing and wrong module path issues. Thanks for addressing this.
36-41: Proper validator instantiation and struct validationUsing v := validator.New() and v.Struct(conf) is correct and addresses the earlier shadowing concern.
73-118: Secure-cookie flag name now matches config key — consider a backward-compat aliasRenaming the flag to secure-cookie fixes the prior mismatch with mapstructure:"secure-cookie". To avoid breaking existing deployments that still export COOKIE_SECURE or use --cookie-secure, add a deprecated flag + viper alias so the old inputs continue to work.
Proposed changes (place near the flag/env binding, before viper.BindPFlags):
@@ for _, opt := range configOptions { @@ viper.BindEnv(opt.name, envVar) } + // Backward-compat for the previous "cookie-secure" naming + // 1) Add a deprecated hidden flag so existing scripts don't break + rootCmd.Flags().Bool("cookie-secure", false, "[DEPRECATED] use --secure-cookie") + _ = rootCmd.Flags().MarkHidden("cookie-secure") + // 2) Bind the legacy env var and alias it to the new key + _ = viper.BindEnv("cookie-secure", "COOKIE_SECURE") + viper.RegisterAlias("secure-cookie", "cookie-secure")internal/service/google_oauth_service.go (1)
33-43: Reuse internal/oauth.NewOAuth to inherit TLS hardening, client plumbing, and PKCE (and wire InsecureSkipVerify).This file re-implements OAuth client/context/PKCE. You already ship internal/oauth.NewOAuth that centralizes these concerns. Compose/Embed it and pass through InsecureSkipVerify from config.
Apply this refactor:
@@ -import ( - "context" - "crypto/rand" - "encoding/base64" - "encoding/json" - "fmt" - "io" - "net/http" - "strings" - "time" - "tinyauth/internal/config" - - "golang.org/x/oauth2" - "golang.org/x/oauth2/endpoints" -) +import ( + "context" + "crypto/rand" + "encoding/base64" + "encoding/json" + "fmt" + "io" + "strings" + "time" + "tinyauth/internal/config" + taoauth "tinyauth/internal/oauth" + + "golang.org/x/oauth2" + "golang.org/x/oauth2/endpoints" +) @@ -type GoogleOAuthService struct { - Config oauth2.Config - Context context.Context - Token *oauth2.Token - Verifier string -} +type GoogleOAuthService struct { + taoauth.OAuth // embeds Config, Context, Verifier + Token *oauth2.Token + InsecureSkipVerify bool +} @@ -func NewGoogleOAuthService(config config.OAuthServiceConfig) *GoogleOAuthService { - return &GoogleOAuthService{ - Config: oauth2.Config{ - ClientID: config.ClientID, - ClientSecret: config.ClientSecret, - RedirectURL: config.RedirectURL, - Scopes: GoogleOAuthScopes, - Endpoint: endpoints.Google, - }, - } -} +func NewGoogleOAuthService(cfg config.OAuthServiceConfig) *GoogleOAuthService { + scopes := GoogleOAuthScopes + if len(cfg.Scopes) > 0 { + scopes = cfg.Scopes + } + svc := &GoogleOAuthService{ + InsecureSkipVerify: cfg.InsecureSkipVerify, + } + // Initialize embedded OAuth + svc.OAuth = *taoauth.NewOAuth(oauth2.Config{ + ClientID: cfg.ClientID, + ClientSecret: cfg.ClientSecret, + RedirectURL: cfg.RedirectURL, + Scopes: scopes, + Endpoint: endpoints.Google, + }, cfg.InsecureSkipVerify) + return svc +} @@ -func (google *GoogleOAuthService) Init() error { - httpClient := &http.Client{} - ctx := context.Background() - ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient) - verifier := oauth2.GenerateVerifier() - - google.Context = ctx - google.Verifier = verifier - return nil -} +// Init no longer needed; retained for interface compatibility. +func (google *GoogleOAuthService) Init() error { return nil }This keeps downstream code working while consolidating HTTP/TLS/PKCE in one place.
Also applies to: 45-54
internal/service/auth_service.go (1)
55-67: Validate secrets and set robust cookie options (Domain and SameSite).
- EncryptionSecret must be 16/24/32 bytes for AES; validate early.
- Ensure HMACSecret has sufficient entropy (e.g., >= 32 bytes).
- Don’t prefix Domain with “.”; set as-is or leave empty for host-only.
- Add SameSite=Lax to mitigate CSRF on session cookie.
Apply:
func (auth *AuthService) Init() error { - store := sessions.NewCookieStore([]byte(auth.Config.HMACSecret), []byte(auth.Config.EncryptionSecret)) + // Validate key lengths + if l := len(auth.Config.EncryptionSecret); l != 0 && l != 16 && l != 24 && l != 32 { + return fmt.Errorf("invalid EncryptionSecret length: got %d, want 16/24/32", l) + } + if len(auth.Config.HMACSecret) < 32 { + return fmt.Errorf("HMACSecret too short: got %d, want >= 32", len(auth.Config.HMACSecret)) + } + store := sessions.NewCookieStore([]byte(auth.Config.HMACSecret), []byte(auth.Config.EncryptionSecret)) store.Options = &sessions.Options{ Path: "/", MaxAge: auth.Config.SessionExpiry, Secure: auth.Config.SecureCookie, HttpOnly: true, - Domain: fmt.Sprintf(".%s", auth.Config.Domain), + // Use configured domain as-is; empty means host-only cookie (ideal for localhost) + Domain: auth.Config.Domain, + SameSite: http.SameSiteLaxMode, } auth.Store = store return nil }Add missing import:
import ( "fmt" "regexp" "strings" "sync" "time" "tinyauth/internal/config" "tinyauth/internal/utils" + "net/http"
🧹 Nitpick comments (11)
internal/service/github_oauth_service.go (3)
71-73: Drop AccessTypeOffline for GitHubThe access_type=offline hint is for providers like Google; GitHub ignores it. Keep PKCE, remove the extra param.
func (github *GithubOAuthService) GetAuthURL(state string) string { - return github.Config.AuthCodeURL(state, oauth2.AccessTypeOffline, oauth2.S256ChallengeOption(github.Verifier)) + return github.Config.AuthCodeURL(state, oauth2.S256ChallengeOption(github.Verifier)) }
91-98: Add context and User-Agent to GitHub API requests
- Bind requests to the service context for cancellation.
- GitHub recommends sending a descriptive User-Agent.
req, err := http.NewRequest("GET", "https://api.github.com/user", nil) if err != nil { return user, err } -req.Header.Set("Accept", "application/vnd.github+json") +req = req.WithContext(github.Context) +req.Header.Set("Accept", "application/vnd.github+json") +req.Header.Set("User-Agent", "tinyauth/1.0 (+https://github.com/steveiliop56/tinyauth)") res, err := client.Do(req) @@ -req, err = http.NewRequest("GET", "https://api.github.com/user/emails", nil) +req, err = http.NewRequest("GET", "https://api.github.com/user/emails", nil) if err != nil { return user, err } -req.Header.Set("Accept", "application/vnd.github+json") +req = req.WithContext(github.Context) +req.Header.Set("Accept", "application/vnd.github+json") +req.Header.Set("User-Agent", "tinyauth/1.0 (+https://github.com/steveiliop56/tinyauth)") res, err = client.Do(req)Also applies to: 120-127
149-164: Email selection logic is solid; consider preferring verified emailsLooks good: primary first, else first available. If you want to be stricter, include a Verified flag in GithubEmailResponse and prefer primary+verified before falling back.
type GithubEmailResponse []struct { Email string `json:"email"` Primary bool `json:"primary"` + Verified bool `json:"verified"` } @@ - for _, email := range emails { - if email.Primary { + for _, email := range emails { + if email.Primary && email.Verified { user.Email = email.Email break } } if len(emails) == 0 { return user, errors.New("no emails found") } // Use first available email if no primary email was found if user.Email == "" { - user.Email = emails[0].Email + // Prefer the first verified email if present + for _, e := range emails { + if e.Verified { + user.Email = e.Email + break + } + } + if user.Email == "" { + user.Email = emails[0].Email + } }cmd/root.go (1)
44-46: Nit: remove redundant type cast; optionally set a global log levelutils.GetLogLevel already returns zerolog.Level, so the explicit zerolog.Level(...) cast is unnecessary. If multiple loggers are created elsewhere, consider setting the global level as well.
Minimal cleanup:
- log.Logger = log.Level(zerolog.Level(utils.GetLogLevel(conf.LogLevel))) + log.Logger = log.Level(utils.GetLogLevel(conf.LogLevel))Optional broader improvement (ensure consistent filtering across all loggers):
+ zerolog.SetGlobalLevel(utils.GetLogLevel(conf.LogLevel)) - log.Logger = log.Level(zerolog.Level(utils.GetLogLevel(conf.LogLevel))) + log.Logger = log.Level(utils.GetLogLevel(conf.LogLevel))internal/service/ldap_service.go (2)
57-66: Harden connection setup: timeouts and StartTLS support (optional).
- DialURL without a custom Dialer has no explicit connect/read timeouts.
- For ldap:// endpoints, consider StartTLS to avoid cleartext binds when possible.
Sketch:
import ( "context" "crypto/tls" "fmt" + "net" + "strings" "time" @@ - conn, err := ldapgo.DialURL(ldap.Config.Address, ldapgo.DialWithTLSConfig(&tls.Config{ - InsecureSkipVerify: ldap.Config.Insecure, - MinVersion: tls.VersionTLS12, - })) + dialer := &net.Dialer{Timeout: 5 * time.Second, KeepAlive: 30 * time.Second} + tlsCfg := &tls.Config{InsecureSkipVerify: ldap.Config.Insecure, MinVersion: tls.VersionTLS12} + conn, err := ldapgo.DialURL( + ldap.Config.Address, + ldapgo.DialWithDialer(dialer), + ldapgo.DialWithTLSConfig(tlsCfg), + ) if err != nil { return nil, err } + // Upgrade to TLS when using ldap:// if desired + if strings.HasPrefix(strings.ToLower(ldap.Config.Address), "ldap://") && !ldap.Config.Insecure { + if err := conn.StartTLS(tlsCfg); err != nil { + _ = conn.Close() + return nil, fmt.Errorf("starttls failed: %w", err) + } + }
76-83: Sanity-check SearchFilter format to prevent surprising searches.If SearchFilter doesn’t contain a %s placeholder, fmt.Sprintf will ignore the username and search broadly. Consider validating once at Init().
Example:
func (ldap *LdapService) Init() error { _, err := ldap.connect() if err != nil { return fmt.Errorf("failed to connect to LDAP server: %w", err) } + if !strings.Contains(ldap.Config.SearchFilter, "%s") { + log.Warn().Str("filter", ldap.Config.SearchFilter).Msg("LDAP SearchFilter lacks %s placeholder; username will be ignored") + } go func() {Remember to import "strings".
internal/service/google_oauth_service.go (3)
81-90: Nil-token guard in Userinfo to avoid unauthenticated requests.If VerifyCode didn’t set Token (unexpected path or error handling elsewhere), Client(...) will inject no auth and the call will 401.
func (google *GoogleOAuthService) Userinfo() (config.Claims, error) { var user config.Claims - client := google.Config.Client(google.Context, google.Token) + if google.Token == nil { + return config.Claims{}, fmt.Errorf("no token set; call VerifyCode first") + } + client := google.Config.Client(google.Context, google.Token)
56-64: Consistent receiver naming.Only GenerateState uses receiver name oauth; others use google. Rename for consistency.
-func (oauth *GoogleOAuthService) GenerateState() string { +func (google *GoogleOAuthService) GenerateState() string { b := make([]byte, 128) _, err := rand.Read(b) if err != nil { return base64.RawURLEncoding.EncodeToString(fmt.Appendf(nil, "state-%d", time.Now().UnixNano())) } state := base64.RawURLEncoding.EncodeToString(b) return state }
35-41: Honor configured scopes when provided.NewGoogleOAuthService ignores cfg.Scopes. Default to GoogleOAuthScopes only when cfg.Scopes is empty.
- Scopes: GoogleOAuthScopes, + Scopes: func() []string { if len(config.Scopes) > 0 { return config.Scopes }; return GoogleOAuthScopes }(),If you adopt the larger refactor above, this change is already included there.
internal/controller/oauth_controller.go (2)
109-113: Clear CSRF cookie on mismatch to avoid stale tokens.When the CSRF check fails, clear the cookie before redirecting.
if err != nil || state != csrfCookie { log.Warn().Err(err).Msg("CSRF token mismatch or cookie missing") - c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.Config.AppURL)) + c.SetCookie(controller.Config.CSRFCookieName, "", -1, "/", controller.Config.Domain, controller.Config.SecureCookie, true) + c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.Config.AppURL)) return }
168-170: Guard name fallback against malformed emails.Split on “@” assumes a valid address; add a safe fallback to avoid index errors.
- name = fmt.Sprintf("%s (%s)", utils.Capitalize(strings.Split(user.Email, "@")[0]), strings.Split(user.Email, "@")[1]) + local, domain, ok := strings.Cut(user.Email, "@") + if !ok { + name = user.Email + } else { + name = fmt.Sprintf("%s (%s)", utils.Capitalize(local), domain) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
cmd/root.go(3 hunks)internal/controller/oauth_controller.go(1 hunks)internal/controller/user_controller.go(1 hunks)internal/service/auth_service.go(14 hunks)internal/service/generic_oauth_service.go(1 hunks)internal/service/github_oauth_service.go(1 hunks)internal/service/google_oauth_service.go(1 hunks)internal/service/ldap_service.go(5 hunks)internal/utils/security_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/controller/user_controller.go
- internal/service/generic_oauth_service.go
- internal/utils/security_utils.go
🧰 Additional context used
🧬 Code graph analysis (6)
internal/service/github_oauth_service.go (2)
internal/config/config.go (3)
Config(18-59)OAuthServiceConfig(92-101)Claims(3-8)internal/providers/github.go (2)
GithubScopes(26-28)GetGithubUser(30-102)
internal/service/google_oauth_service.go (3)
internal/config/config.go (3)
Config(18-59)OAuthServiceConfig(92-101)Claims(3-8)internal/oauth/oauth.go (2)
NewOAuth(20-44)GetAuthURL(46-48)internal/providers/google.go (1)
GetGoogleUser(24-56)
internal/service/ldap_service.go (1)
internal/config/config.go (1)
Config(18-59)
internal/service/auth_service.go (4)
internal/config/config.go (7)
User(103-107)SessionCookieName(14-14)Config(18-59)UserSearch(109-112)SessionCookie(114-121)UserContext(123-133)Labels(82-90)internal/service/docker_service.go (1)
DockerService(16-19)internal/service/ldap_service.go (1)
LdapService(23-26)internal/utils/security_utils.go (2)
CheckFilter(93-118)FilterIP(70-91)
internal/controller/oauth_controller.go (5)
internal/config/config.go (6)
CSRFCookieName(15-15)RedirectCookieName(16-16)Config(18-59)UnauthorizedQuery(135-140)SessionCookie(114-121)RedirectQuery(142-144)internal/service/auth_service.go (1)
AuthService(37-44)internal/service/oauth_broker_service.go (1)
OAuthBrokerService(18-21)internal/utils/app_utils.go (1)
IsRedirectSafe(76-102)internal/utils/string_utils.go (2)
Capitalize(7-12)CoalesceToString(14-30)
cmd/root.go (4)
internal/config/config.go (1)
Config(18-59)internal/utils/security_utils.go (1)
GetSecret(17-32)internal/utils/app_utils.go (1)
GetLogLevel(104-123)internal/bootstrap/app_bootstrap.go (1)
NewBootstrapApp(33-37)
🔇 Additional comments (9)
internal/service/github_oauth_service.go (1)
75-84: Good fix: propagate Exchange errorsReturning the exchange error ensures callers can handle failures correctly. Nice.
cmd/root.go (4)
25-28: Config unmarshal + fail-fast error handling looks solidUnmarshalling into config.Config and exiting on failure with log.Fatal is appropriate for startup-time config errors.
30-34: Secret resolution precedence handled correctlyResolving Secret and the OAuth client secrets via utils.GetSecret before validation ensures file-based secrets are honored and the validator sees final values.
47-55: Bootstrap setup and error path — LGTMDelegating to bootstrap.NewBootstrapApp(conf).Setup() with a clear fatal on error keeps the entrypoint clean.
120-136: Flag/env binding loop is clean and scalableThe data-driven registration plus explicit BindEnv with hyphen→underscore mapping is neat and maintainable.
internal/controller/oauth_controller.go (1)
79-85: Open-redirect hardening looks good (using IsRedirectSafe).You now validate redirect_uri both when setting and consuming the cookie. This addresses the previous open-redirect risk.
Also applies to: 180-186
internal/service/auth_service.go (3)
183-217: Rate limiter logic is solid; concurrency handled correctly.Clear separation of RLock in reads and Lock in updates; lock time computed in seconds from config. Looks good.
112-140: LDAP rebind pattern relies on a shared connection; confirm concurrency expectations.VerifyUser binds as the user, then rebinds with the service account. If LDAP service is used concurrently, this can clash. The mutex fix in LdapService will mitigate, but if you anticipate high concurrency, consider per-request connections.
Would you like me to sketch a small connection pool for LDAP binds to avoid global rebind entirely?
353-372: AuthEnabled error handling verified
The only call site in internal/controller/proxy_controller.go (lines 136–144) checks the returnederrand responds with a 500 or redirect, so invalid-regex errors cannot be silently ignored. No further changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/service/ldap_service.go (1)
110-128: Make heartbeat concurrency-safe and resilient to nil connection.Avoid nil deref and races by snapshotting the conn under RLock.
- _, err := ldap.Conn.Search(searchRequest) + ldap.connMu.RLock() + conn := ldap.Conn + ldap.connMu.RUnlock() + if conn == nil { + return errors.New("ldap connection is not established") + } + _, err := conn.Search(searchRequest)internal/config/config.go (1)
21-21: Use the supported IPv4 validator tagNo custom
ip4_addrvalidator was registered in the codebase, so go-playground/validator will reject the unknownip4_addrtag and fail to validate valid IPv4 addresses. Update the tag to the built-inipv4validator:• internal/config/config.go (around line 20)
- Address string `validate:"required,ip4_addr" mapstructure:"address"` + Address string `validate:"required,ipv4" mapstructure:"address"`
♻️ Duplicate comments (26)
frontend/src/pages/login-page.tsx (1)
68-68: Login POST: consider sending cookies explicitly (withCredentials) and re-check backend route wiringIf auth is cookie-based or sets CSRF cookies on login, passing withCredentials avoids surprises in cross-origin deployments. Dev proxy makes this same-origin, but prod/staging may not. Also do a quick pass to ensure POST /api/user/login is actually registered server-side.
Apply this diff locally:
- mutationFn: (values: LoginSchema) => axios.post("/api/user/login", values), + mutationFn: (values: LoginSchema) => + axios.post("/api/user/login", values, { withCredentials: true }),Run to verify global config and backend route:
#!/bin/bash set -euo pipefail echo "1) Frontend: do we already set axios withCredentials globally?" rg -nP '(axios\.defaults\.withCredentials|axios\.create\(\s*\{[^}]*withCredentials\s*:)' --type=ts --type=tsx || true echo -e "\n2) Backend: confirm user login route exists" rg -nPU -C3 '(?i)\b(user/login)\b' --type=go || true.env.example (1)
5-9: Reorder SECURE_COOKIE before USERS to satisfy dotenv-linter and reduce upgrade frictionAlso, ensure README/CHANGELOG clearly notes the COOKIE_SECURE → SECURE_COOKIE rename so operators update their envs.
Apply this reordering:
APP_URL=http://localhost:3000 -USERS=your_user_password_hash -USERS_FILE=users_file -SECURE_COOKIE=false +SECURE_COOKIE=false +USERS=your_user_password_hash +USERS_FILE=users_fileI can draft a short migration blurb for the docs if you want.
internal/service/ldap_service.go (4)
130-155: Fix backoff usage (wrong signature), add nil check on Close(), and synchronize reconnect.As written, this won’t compile (operation returns (*Conn, error); WithMaxTries doesn’t exist in v5) and can panic on Close() when Conn is nil. Also needs locking around replacement.
func (ldap *LdapService) reconnect() error { @@ - operation := func() (*ldapgo.Conn, error) { - ldap.Conn.Close() - conn, err := ldap.connect() - if err != nil { - return nil, err - } - return conn, nil - } + operation := func() error { + ldap.connMu.Lock() + if ldap.Conn != nil { + _ = ldap.Conn.Close() + ldap.Conn = nil + } + ldap.connMu.Unlock() + _, err := ldap.connect() // connect() sets ldap.Conn on success + return err + } @@ - _, err := backoff.Retry(context.TODO(), operation, backoff.WithBackOff(exp), backoff.WithMaxTries(3)) + err := backoff.Retry(context.Background(), operation, backoff.WithBackOff(exp), backoff.WithMaxRetries(3))
3-12: Add missing imports for concurrency and error handling (sync, errors).Upcoming fixes require sync.RWMutex and creating errors.
@@ -import ( - "context" - "crypto/tls" - "fmt" - "time" +import ( + "context" + "crypto/tls" + "errors" + "fmt" + "sync" + "time"
23-26: Guard shared LDAP connection with RWMutex to prevent data races and mid-flight rebinding.Search/Bind/heartbeat/reconnect concurrently use and replace ldap.Conn without synchronization. This can race, causing transient failures or panics.
type LdapService struct { Config LdapServiceConfig - Conn *ldapgo.Conn + // Protects access and replacement of Conn + connMu sync.RWMutex + Conn *ldapgo.Conn }
57-74: Lock assignment of the shared connection inside connect().Ensure connection replacement is atomic and visible safely to readers.
func (ldap *LdapService) connect() (*ldapgo.Conn, error) { - conn, err := ldapgo.DialURL(ldap.Config.Address, ldapgo.DialWithTLSConfig(&tls.Config{ + if ldap.Config.Insecure { + log.Warn().Msg("LDAP TLS certificate verification is disabled (InsecureSkipVerify=true)") + } + conn, err := ldapgo.DialURL(ldap.Config.Address, ldapgo.DialWithTLSConfig(&tls.Config{ InsecureSkipVerify: ldap.Config.Insecure, MinVersion: tls.VersionTLS12, })) @@ // Set and return the connection - ldap.Conn = conn + ldap.connMu.Lock() + ldap.Conn = conn + ldap.connMu.Unlock() return conn, nil }internal/assets/assets.go (1)
7-11: Preserve backward compatibility: keep a deprecated alias for FontendAssets.If external or internal callers still reference the old misspelling, this avoids breakage.
//go:embed dist var FrontendAssets embed.FS + +// Deprecated: use FrontendAssets. +var FontendAssets = FrontendAssetsTo confirm there are no lingering references to the old names:
#!/bin/bash rg -nP -S '\bassets\.(Assets|FontendAssets)\b' -g '!**/vendor/**'internal/controller/health_controller.go (1)
15-18: Align health routes with middleware skip listThe
SetupRoutesmethod registers only the/healthendpoints (and an optional/api/healthcheckalias), but the logging middleware’sloggerSkipPathsPrefixcurrently only skips"GET /api/health"and"HEAD /api/health". To prevent unnecessary log noise, we must update the skip list to include the actual registered paths.Files to update:
internal/controller/health_controller.go— decide whether to keep the/api/healthcheckalias.internal/middleware/zerolog_middleware.go— extendloggerSkipPathsPrefix.Suggested diffs:
--- a/internal/controller/health_controller.go +++ b/internal/controller/health_controller.go @@ func (controller *HealthController) SetupRoutes() { - controller.Router.GET("/health", controller.healthHandler) - controller.Router.HEAD("/health", controller.healthHandler) + controller.Router.GET("/health", controller.healthHandler) + controller.Router.HEAD("/health", controller.healthHandler) + // Back-compat alias (remove if not needed) + controller.Router.GET("/api/healthcheck", controller.healthHandler) + controller.Router.HEAD("/api/healthcheck", controller.healthHandler)--- a/internal/middleware/zerolog_middleware.go +++ b/internal/middleware/zerolog_middleware.go @@ var ( - loggerSkipPathsPrefix = []string{ - "GET /api/health", - "HEAD /api/health", + loggerSkipPathsPrefix = []string{ + "GET /api/health", + "HEAD /api/health", + "GET /health", + "HEAD /health", + // Skip alias if retained + "GET /api/healthcheck", + "HEAD /api/healthcheck", } )internal/utils/user_utils.go (3)
12-16: Good fix: empty/whitespace-only input now returns an empty set, not an error.This addresses the earlier edge case for blank input.
24-33: Nice: trims entries and skips empties.This resolves parsing failures on trailing commas or blank items.
49-58: Good: file read errors are now propagated.Callers will see the real cause instead of a generic parse error.
internal/controller/resources_controller.go (1)
19-27: Bug: StripPrefix ignores router BasePath; breaks under grouped routes (e.g., “/api”).Pre-wrapping with "/resources" fails when the controller is mounted under a non-root base path, leading to 404s.
Apply this refactor to compute and cache the effective prefix and avoid per-request allocations:
@@ package controller import ( "net/http" + "path" "github.com/gin-gonic/gin" ) @@ type ResourcesController struct { Config ResourcesControllerConfig Router *gin.RouterGroup - FileServer http.Handler + FileServer http.Handler + prefix string } func NewResourcesController(config ResourcesControllerConfig, router *gin.RouterGroup) *ResourcesController { - fileServer := http.StripPrefix("/resources", http.FileServer(http.Dir(config.ResourcesDir))) + // cache raw file server; apply StripPrefix at serve time using computed prefix + fileServer := http.FileServer(http.Dir(config.ResourcesDir)) + prefix := path.Join(router.BasePath(), "/resources") return &ResourcesController{ Config: config, Router: router, - FileServer: fileServer, + FileServer: fileServer, + prefix: prefix, } }internal/utils/label_utils.go (1)
22-38: Solid: header parsing now trims, rejects blank entries, and canonicalizes keys.This aligns with HTTP field-name rules and avoids CRLF issues in values.
internal/utils/app_utils.go (1)
22-35: Refactor GetUpperDomain to handle apex domains, IPs, and localhost correctlyThe current implementation of GetUpperDomain in internal/utils/app_utils.go incorrectly:
- Strips “example.com” down to “com”
- Rejects valid single-label hosts (e.g. “localhost”) and IP addresses, which are useful in dev/test
- Doesn’t distinguish between an empty host and other invalid inputs
Apply the following changes:
• File: internal/utils/app_utils.go
• Function: GetUpperDomain (lines ~22–35)host := appUrlParsed.Hostname() -if netIP := net.ParseIP(host); netIP != nil { - return "", errors.New("IP addresses are not allowed") -} - -urlParts := strings.Split(host, ".") - -if len(urlParts) < 2 { - return "", errors.New("invalid domain, must be at least second level domain") -} - -return strings.Join(urlParts[1:], "."), nil +// Empty host is always an error +if host == "" { + return "", errors.New("empty host") +} +// Allow IPs and return them unchanged (helpful for localhost/dev URLs) +if net.ParseIP(host) != nil { + return host, nil +} +parts := strings.Split(host, ".") +// For apex domains (example.com) and single-label hosts, return unchanged +if len(parts) < 3 { + return host, nil +} +// For deeper domains, drop only the left-most label +return strings.Join(parts[1:], "."), nilConsider adding table-driven tests covering:
https://sub.a.example.com→a.example.comhttps://example.com→example.comhttp://localhost:8080→localhosthttp://127.0.0.1:3000→127.0.0.1- malformed URLs (empty host or invalid URL) → error
internal/service/google_oauth_service.go (2)
45-53: Consider reusing shared OAuth plumbing to reduce surface area.Leverage internal/oauth.NewOAuth (if present) for TLS hardening, timeouts, and PKCE to avoid divergent behavior across providers.
Also applies to: 66-68, 70-79, 81-113
81-89: Defensive check: guard against nil token before building the client.If
VerifyCodewasn’t called or failed,google.Tokenmay be nil. Building the client without a token triggers underlying token retrieval attempts and confusing errors.func (google *GoogleOAuthService) Userinfo() (config.Claims, error) { var user config.Claims - client := google.Config.Client(google.Context, google.Token) + if google.Token == nil { + return config.Claims{}, fmt.Errorf("no token set; call VerifyCode first") + } + client := google.Config.Client(google.Context, google.Token)internal/controller/proxy_controller.go (2)
123-136: Good fix: return after redirect to avoid double-redirect pathThe early return after the error redirect prevents subsequent redirects from running. Matches prior feedback.
215-219: Username displayed on unauthorized page now matches legacy behaviorFor OAuth you now pass email; for basic you pass username. This restores expected UX.
Also applies to: 250-254
internal/controller/oauth_controller.go (1)
75-85: Do not prefix cookie Domain with a dot; allow empty for host-only cookiesUsing fmt.Sprintf(".%s", domain) yields "." on localhost and is non-standard. Pass the domain as-is; empty yields host-only cookie.
- c.SetCookie(controller.Config.CSRFCookieName, state, int(time.Hour.Seconds()), "/", fmt.Sprintf(".%s", controller.Config.Domain), controller.Config.SecureCookie, true) + c.SetCookie(controller.Config.CSRFCookieName, state, int(time.Hour.Seconds()), "/", controller.Config.Domain, controller.Config.SecureCookie, true) @@ - c.SetCookie(controller.Config.RedirectCookieName, redirectURI, int(time.Hour.Seconds()), "/", fmt.Sprintf(".%s", controller.Config.Domain), controller.Config.SecureCookie, true) + c.SetCookie(controller.Config.RedirectCookieName, redirectURI, int(time.Hour.Seconds()), "/", controller.Config.Domain, controller.Config.SecureCookie, true) @@ - c.SetCookie(controller.Config.CSRFCookieName, "", -1, "/", fmt.Sprintf(".%s", controller.Config.Domain), controller.Config.SecureCookie, true) + c.SetCookie(controller.Config.CSRFCookieName, "", -1, "/", controller.Config.Domain, controller.Config.SecureCookie, true) @@ - c.SetCookie(controller.Config.RedirectCookieName, "", -1, "/", fmt.Sprintf(".%s", controller.Config.Domain), controller.Config.SecureCookie, true) + c.SetCookie(controller.Config.RedirectCookieName, "", -1, "/", controller.Config.Domain, controller.Config.SecureCookie, true)Also applies to: 112-116, 198-199
internal/service/github_oauth_service.go (3)
61-68: Shorten state size and keep padding-free encoding; simplify fallback128 random bytes produce a very long state (~172 URL chars). 32 bytes are plenty. Also, fmt.Appendf is unusual here; Sprintf is simpler.
func (github *GithubOAuthService) GenerateState() string { - b := make([]byte, 128) + b := make([]byte, 32) _, err := rand.Read(b) if err != nil { - return base64.RawURLEncoding.EncodeToString(fmt.Appendf(nil, "state-%d", time.Now().UnixNano())) + return base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf("state-%d", time.Now().UnixNano()))) } state := base64.RawURLEncoding.EncodeToString(b) return state }
50-57: Add an HTTP client timeout to avoid indefinite hangsThe default http.Client has no timeout; network stalls can hang your OAuth flow forever. Set a sane timeout (e.g., 15s).
Apply:
func (github *GithubOAuthService) Init() error { - httpClient := &http.Client{} + httpClient := &http.Client{Timeout: 15 * time.Second} ctx := context.Background() ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient) verifier := oauth2.GenerateVerifier()
86-93: Guard against missing/invalid token before calling GitHub APIsIf VerifyCode wasn’t called or failed, github.Token can be nil/empty. Fail fast with a clear error.
func (github *GithubOAuthService) Userinfo() (config.Claims, error) { var user config.Claims + if github.Token == nil || github.Token.AccessToken == "" { + return user, errors.New("github oauth: token not present; call VerifyCode first") + } + client := github.Config.Client(github.Context, github.Token)internal/config/config.go (1)
71-74: Add BC alias for previously exported misspelling PassowrdLabelsOlder code (including third-party users) may still refer to PassowrdLabels. Provide a type alias for backward compatibility.
type PasswordLabels struct { Plain string File string } + +// Backward-compatibility alias for the previous misspelling. +// Deprecated: use PasswordLabels. +type PassowrdLabels = PasswordLabelsinternal/service/auth_service.go (3)
55-63: Cookie options: fix Domain handling, add SameSite, and validate key lengths
- fmt.Sprintf(".%s", domain) yields a bare "." when domain is empty; breaks cookies on localhost.
- Add SameSite=Lax for CSRF hardening (reasonable default for session cookies).
- Fail fast on invalid key lengths for securecookie (block key must be 16/24/32 bytes); also assert HMAC secret is set.
Apply:
func (auth *AuthService) Init() error { - store := sessions.NewCookieStore([]byte(auth.Config.HMACSecret), []byte(auth.Config.EncryptionSecret)) + // Validate secrets early + if l := len(auth.Config.EncryptionSecret); l != 0 && l != 16 && l != 24 && l != 32 { + return fmt.Errorf("invalid EncryptionSecret length: got %d, want 16/24/32", l) + } + if len(auth.Config.HMACSecret) == 0 { + return fmt.Errorf("HMACSecret must be set") + } + store := sessions.NewCookieStore([]byte(auth.Config.HMACSecret), []byte(auth.Config.EncryptionSecret)) store.Options = &sessions.Options{ Path: "/", MaxAge: auth.Config.SessionExpiry, Secure: auth.Config.SecureCookie, HttpOnly: true, - Domain: fmt.Sprintf(".%s", auth.Config.Domain), + // Only set Domain when configured; host-only cookie is fine for localhost. + Domain: auth.Config.Domain, + SameSite: http.SameSiteLaxMode, }Add missing import:
import ( "fmt" + "net/http" "regexp" "strings" "sync" "time" "tinyauth/internal/config" "tinyauth/internal/utils"
74-77: Clear-cookie Domain has the same leading-dot issueAvoid emitting a bare "." domain when clearing the cookie.
- c.SetCookie(auth.Config.SessionCookieName, "", -1, "/", fmt.Sprintf(".%s", auth.Config.Domain), auth.Config.SecureCookie, true) + c.SetCookie(auth.Config.SessionCookieName, "", -1, "/", auth.Config.Domain, auth.Config.SecureCookie, true)
269-271: Same fix when deleting the session cookieEnsure the clear-cookie path also uses the corrected Domain handling.
- c.SetCookie(auth.Config.SessionCookieName, "", -1, "/", fmt.Sprintf(".%s", auth.Config.Domain), auth.Config.SecureCookie, true) + c.SetCookie(auth.Config.SessionCookieName, "", -1, "/", auth.Config.Domain, auth.Config.SecureCookie, true)
🧹 Nitpick comments (55)
frontend/src/context/app-context.tsx (1)
18-19: Proxy configuration validated; endpoint path correctAll calls to
/api/context/appare correctly routed by the dev‐server proxy invite.config.ts(the/apirule rewrites/api/context/app → /context/appand targetshttp://tinyauth-backend:3000/api). No instances of the old/api/appremain in the frontend code.Optional performance tweak (since the app context is mostly static):
const { isFetching, data, error } = useSuspenseQuery({ queryKey: ["app"], queryFn: () => axios.get("/api/context/app").then((res) => res.data), + staleTime: 5 * 60 * 1000, // keep fresh for 5 minutes + cacheTime: 30 * 60 * 1000, // retain cache for 30 minutes + retry: 1, // one retry on failure });– No further action required for proxy wiring.
– Consider adding the caching knobs above to reduce unnecessary refetches.frontend/src/context/user-context.tsx (2)
41-44: Grammar nit: “a UserContextProvider”Minor copy tweak in the thrown message.
- "useUserContext must be used within an UserContextProvider", + "useUserContext must be used within a UserContextProvider",
18-19: Migration complete: add caching & retry to user context query
- Verified no remaining references to the old
/api/userendpoint for context retrieval—onlyfrontend/src/context/user-context.tsx:18now points to/api/context/user.- As with the app context, you can optionally mirror caching and retry settings to prevent auth-edge thrash:
const { isFetching, data, error } = useSuspenseQuery({ queryKey: ["user"], queryFn: () => axios.get("/api/context/user").then((res) => res.data), + staleTime: 30 * 1000, // cache valid for 30 seconds + gcTime: 10 * 60 * 1000, // garbage-collect after 10 minutes + retry: (failureCount, error) => failureCount < 1, // single retry on failure });frontend/src/pages/logout-page.tsx (1)
29-45: Endpoint update to /api/user/logout looks right; consider clearing cached contexts on successApproving the path change. Optionally clear the "user" and "app" queries before redirect to prevent any brief stale UI if the redirect is delayed.
const logoutMutation = useMutation({ - mutationFn: () => axios.post("/api/user/logout"), + mutationFn: () => axios.post("/api/user/logout"), mutationKey: ["logout"], onSuccess: () => { + // Clear cached context before redirect to avoid stale flashes + qc.removeQueries({ queryKey: ["user"], exact: true }); + qc.removeQueries({ queryKey: ["app"], exact: true }); toast.success(t("logoutSuccessTitle"), { description: t("logoutSuccessSubtitle"), }); setTimeout(async () => { window.location.replace("/login"); }, 500); },Additional change outside this hunk:
// Add import import { useQueryClient } from "@tanstack/react-query"; // Create client near other hooks const qc = useQueryClient();If the backend enforces CSRF, confirm axios is configured with the appropriate xsrfCookieName/xsrfHeaderName so this POST succeeds without manual headers.
.gitignore (1)
28-29: Scope the data ignore to repository root to avoid unintended matchesPlain “data” will ignore any directory named data anywhere in the tree. If you only intend to ignore the root-level data dir, anchor it.
-# data directory -data +# data directory +/datafrontend/src/pages/totp-page.tsx (1)
35-35: Include credentials with TOTP POST to send session cookiesWe didn’t detect any global Axios configuration for
withCredentials, so this call will not send cookies by default. To ensure the TOTP-pending session (tracked via an HTTP-only cookie) is recognized server-side, add thewithCredentials: trueoption or configure a shared Axios instance accordingly.• File:
frontend/src/pages/totp-page.tsx(around line 35)- mutationFn: (values: TotpSchema) => axios.post("/api/user/totp", values), + mutationFn: (values: TotpSchema) => + axios.post("/api/user/totp", values, { withCredentials: true }),• Alternatively, create or update a centralized Axios client (e.g. in
src/utils/api.ts):export const apiClient = axios.create({ baseURL: import.meta.env.VITE_API_BASE_URL, withCredentials: true, });and use
apiClient.post(...)instead ofaxios.post(...)..env.example (1)
23-31: Reorder .env.example and surface allowed log levels in docs
- Moved
LOG_LEVEL=debugaboveOAUTH_WHITELISTto satisfy dotenv-linter’s natural key ordering.- Verified that
GetLogLevel(level string)in internal/utils/app_utils.go handles case-insensitive string levels (trace,debug,info,warn,error,fatal,panic).- Confirmed the
Configstruct in internal/config/config.go uses avalidate:"oneof=trace debug info warn error fatal panic"tag to enforce allowed values.- Noted that the CLI flag in cmd/root.go (around line 107) defaults to
"info"with a description of"Log level."—consider expanding that help text (and any README.md docs) to enumerate the permitted levels.Apply this local reordering:
-OAUTH_WHITELIST= -GENERIC_NAME=My OAuth -SESSION_EXPIRY=7200 -LOGIN_TIMEOUT=300 -LOGIN_MAX_RETRIES=5 -LOG_LEVEL=debug +LOG_LEVEL=debug +OAUTH_WHITELIST= +GENERIC_NAME=My OAuth +SESSION_EXPIRY=7200 +LOGIN_TIMEOUT=300 +LOGIN_MAX_RETRIES=5Optional refactors recommended:
- ✏️ Update the CLI help string in cmd/root.go to list allowed log levels.
- 📚 Extend any user-facing documentation (e.g. README.md) to include the full set of valid log-level values.
internal/middleware/zerolog_middleware.go (3)
11-36: Tighten skip matching: HasPrefix can over-skip (e.g., GET /api/healthz). Use exact matches via a set.Current HasPrefix on the "METHOD path" string will also skip endpoints that merely start with the listed prefixes. Switch to exact matching to avoid unintended debug-only logging.
Apply this diff:
@@ -var ( - loggerSkipPathsPrefix = []string{ - "GET /api/health", - "HEAD /api/health", - "GET /favicon.ico", - } -) +var ( + loggerSkipPaths = map[string]struct{}{ + "GET /api/health": {}, + "HEAD /api/health": {}, + "GET /favicon.ico": {}, + } +) @@ -func (m *ZerologMiddleware) logPath(path string) bool { - for _, prefix := range loggerSkipPathsPrefix { - if strings.HasPrefix(path, prefix) { - return false - } - } - return true -} +func (m *ZerologMiddleware) logPath(methodPath string) bool { + _, skip := loggerSkipPaths[methodPath] + return !skip +}
50-64: Use structured duration and conventional levels: <400=Info, 4xx=Warn, 5xx=Error.This improves queryability (numeric duration) and aligns severity with typical HTTP semantics.
- latency := time.Since(tStart).String() + latency := time.Since(tStart) @@ - if m.logPath(method + " " + path) { - switch { - case code >= 200 && code < 300: - log.Info().Str("method", method).Str("path", path).Str("address", address).Str("clientIp", clientIP).Int("status", code).Str("latency", latency).Msg("Request") - case code >= 300 && code < 400: - log.Warn().Str("method", method).Str("path", path).Str("address", address).Str("clientIp", clientIP).Int("status", code).Str("latency", latency).Msg("Request") - case code >= 400: - log.Error().Str("method", method).Str("path", path).Str("address", address).Str("clientIp", clientIP).Int("status", code).Str("latency", latency).Msg("Request") - } - } else { - log.Debug().Str("method", method).Str("path", path).Str("address", address).Int("status", code).Str("latency", latency).Msg("Request") + if m.logPath(method + " " + path) { + switch { + case code < 400: + log.Info().Str("method", method).Str("path", path).Str("address", address).Str("clientIp", clientIP).Int("status", code).Dur("latency", latency).Msg("Request") + case code < 500: + log.Warn().Str("method", method).Str("path", path).Str("address", address).Str("clientIp", clientIP).Int("status", code).Dur("latency", latency).Msg("Request") + default: + log.Error().Str("method", method).Str("path", path).Str("address", address).Str("clientIp", clientIP).Int("status", code).Dur("latency", latency).Msg("Request") + } + } else { + log.Debug().Str("method", method).Str("path", path).Str("address", address).Int("status", code).Dur("latency", latency).Msg("Request") }
44-49: Consider adding route pattern and errors for better observability.Add c.FullPath() (route) to reduce log cardinality and attach c.Errors when present.
Example:
- method := c.Request.Method - path := c.Request.URL.Path + method := c.Request.Method + path := c.Request.URL.Path + route := c.FullPath() // may be empty for unmatched routesThen extend logs with
.Str("route", route)and, if you want error details:// pseudo-pattern for each event evt := log.Info() /* or Warn/Error */ if len(c.Errors) > 0 { evt = evt.Strs("errors", c.Errors.Errors()) } evt.Msg("Request")internal/service/ldap_service.go (1)
40-53: Graceful shutdown and ticker leak: prefer NewTicker + stop on context.time.Tick leaks and you have no way to stop the goroutine. Consider accepting a context in Init() or wiring one from bootstrap, then use time.NewTicker and stop it on ctx.Done().
I can draft a context-aware Init(ctx) and a shutdown hook in bootstrap if helpful.
internal/utils/fs_utils.go (1)
5-17: Remove redundant Stat() to avoid TOCTOU and extra syscall.os.ReadFile already returns a precise error; the pre-check adds a race window and overhead.
func ReadFile(file string) (string, error) { - _, err := os.Stat(file) - if err != nil { - return "", err - } - - data, err := os.ReadFile(file) - if err != nil { - return "", err - } - - return string(data), nil + data, err := os.ReadFile(file) + if err != nil { + return "", err + } + return string(data), nil }main.go (1)
13-13: Good fix: removed hardcoded Fatal level; consider configurable level and optional Caller toggle.Nice improvement dropping the Fatal filter and adding Caller(). For ops flexibility, allow overriding the global log level via an env (e.g., LOG_LEVEL). Optionally disable Caller() in production to reduce noise/overhead.
Apply this diff:
func main() { - log.Logger = log.Output(zerolog.ConsoleWriter{Out: os.Stderr, TimeFormat: time.RFC3339}).With().Timestamp().Caller().Logger() + // Build console logger + log.Logger = log.Output(zerolog.ConsoleWriter{Out: os.Stderr, TimeFormat: time.RFC3339}).With().Timestamp().Caller().Logger() + + // Optional: make log level configurable via LOG_LEVEL (defaults to Info) + if lvlStr := os.Getenv("LOG_LEVEL"); lvlStr != "" { + if lvl, err := zerolog.ParseLevel(lvlStr); err == nil { + zerolog.SetGlobalLevel(lvl) + } else { + // keep default if parsing fails + } + } cmd.Execute() }internal/utils/string_utils.go (2)
3-5: Import utf8 if optimizing Capitalize.The proposed Capitalize refactor below needs utf8 decoding.
import ( - "strings" + "strings" + "unicode/utf8" )
7-12: Avoid double []rune allocation; use utf8.DecodeRune to uppercase first glyph.Current code constructs []rune twice and slices it, which is unnecessary. The change below preserves existing semantics (uses strings.ToUpper on the first rune, so ß -> SS) while reducing allocations.
func Capitalize(str string) string { if len(str) == 0 { return "" } - return strings.ToUpper(string([]rune(str)[0])) + string([]rune(str)[1:]) + // Uppercase only the first Unicode rune; leave the remainder untouched. + r, size := utf8.DecodeRuneInString(str) + return strings.ToUpper(str[:size]) + str[size:] }internal/controller/health_controller.go (2)
3-3: Import net/http and use http.Status constants.*Small cleanup and correctness for HEAD semantics in the handler below.
-import "github.com/gin-gonic/gin" +import ( + "net/http" + "github.com/gin-gonic/gin" +)
20-25: Return empty body for HEAD; avoid writing JSON on HEAD requests.HEAD responses should not include a message body. Short-circuit HEAD with StatusOK; send JSON only for GET.
-func (controller *HealthController) healthHandler(c *gin.Context) { - c.JSON(200, gin.H{ - "status": "ok", - "message": "Healthy", - }) -} +func (controller *HealthController) healthHandler(c *gin.Context) { + if c.Request.Method == http.MethodHead { + c.Status(http.StatusOK) + return + } + c.JSON(http.StatusOK, gin.H{ + "status": "ok", + "message": "Healthy", + }) +}internal/middleware/ui_middleware.go (2)
3-11: Prepare for path sanitation if applied below.If you adopt the path.Clean tweak in Middleware(), add the import.
import ( "io/fs" "net/http" "os" "strings" "tinyauth/internal/assets" + "path" "github.com/gin-gonic/gin" )
35-56: Refactor path handling to avoid indexing and sanitize the clean pathI verified that in app_bootstrap.go you’re always calling
Init()on every middleware (including UIMiddleware) before registering it with Gin, som.UIFSandm.UIFileServerare guaranteed to be non-nil in production. Guarding against uninitialized middleware is therefore unnecessary here (though you can still add a nil-check if you plan to use this handler standalone or in tests).However, improving how you detect API/resource prefixes and sanitize incoming paths will make the middleware more robust and clear. I recommend an optional refactor along these lines:
• Replace
strings.Split(c.Request.URL.Path, "/")[1]with explicitHasPrefix/equality checks for/apiand/resources.
• Usepath.Clean(after trimming the leading slash) to normalize the asset path (and handle"."→"").
• Continue falling back to/for the SPA index whenfs.Statreturns a not-exists error.Example diff snippet (omitting the nil-guard check since bootstrap covers initialization):
func (m *UIMiddleware) Middleware() gin.HandlerFunc { - return func(c *gin.Context) { - switch strings.Split(c.Request.URL.Path, "/")[1] { - case "api", "resources": - c.Next() - return - default: - _, err := fs.Stat(m.UIFS, strings.TrimPrefix(c.Request.URL.Path, "/")) - if os.IsNotExist(err) { - c.Request.URL.Path = "/" - } - m.UIFileServer.ServeHTTP(c.Writer, c.Request) - c.Abort() - return - } - } + return func(c *gin.Context) { + p := c.Request.URL.Path + if p == "/api" || strings.HasPrefix(p, "/api/") || + p == "/resources" || strings.HasPrefix(p, "/resources/") { + c.Next() + return + } + + // Clean and prepare the relative path for asset lookup + rel := strings.TrimPrefix(p, "/") + rel = path.Clean(rel) + if rel == "." { + rel = "" + } + + // If asset missing, serve SPA index + if _, err := fs.Stat(m.UIFS, rel); os.IsNotExist(err) { + c.Request.URL.Path = "/" + } + m.UIFileServer.ServeHTTP(c.Writer, c.Request) + c.Abort() + return + }Feel free to adopt or adapt this optional refactor to improve readability and safety without changing existing behavior.
internal/controller/context_controller.go (2)
64-89: Confirm API contract: HTTP 200 on unauthorized with payload status 401.If clients expect a real 401, return it at the HTTP layer. Otherwise, keep 200 for legacy reasons. Also, avoid naming the variable “context” for readability.
Option A — return 401:
-func (controller *ContextController) userContextHandler(c *gin.Context) { - context, err := utils.GetContext(c) +func (controller *ContextController) userContextHandler(c *gin.Context) { + userCtx, err := utils.GetContext(c) userContext := UserContextResponse{ - Status: 200, + Status: 200, Message: "Success", - IsLoggedIn: context.IsLoggedIn, - Username: context.Username, - Name: context.Name, - Email: context.Email, - Provider: context.Provider, - Oauth: context.OAuth, - TotpPending: context.TotpPending, + IsLoggedIn: userCtx.IsLoggedIn, + Username: userCtx.Username, + Name: userCtx.Name, + Email: userCtx.Email, + Provider: userCtx.Provider, + Oauth: userCtx.OAuth, + TotpPending: userCtx.TotpPending, } if err != nil { log.Debug().Err(err).Msg("No user context found in request") userContext.Status = 401 userContext.Message = "Unauthorized" userContext.IsLoggedIn = false - c.JSON(200, userContext) + c.JSON(401, userContext) return } - c.JSON(200, userContext) + c.JSON(200, userContext) }If keeping 200 is intentional, please confirm and consider adding a comment to document the contract.
91-104: Minor: prefer http.StatusOK over magic 200.Not critical, but consistent use of http constants improves readability.
- c.JSON(200, AppContextResponse{ - Status: 200, + c.JSON(http.StatusOK, AppContextResponse{ + Status: http.StatusOK, Message: "Success", ConfiguredProviders: controller.Config.ConfiguredProviders, DisableContinue: controller.Config.DisableContinue, Title: controller.Config.Title, GenericName: controller.Config.GenericName, Domain: controller.Config.Domain, ForgotPasswordMessage: controller.Config.ForgotPasswordMessage, BackgroundImage: controller.Config.BackgroundImage, OAuthAutoRedirect: controller.Config.OAuthAutoRedirect, })Add import:
import ( "tinyauth/internal/utils" "github.com/gin-gonic/gin" "github.com/rs/zerolog/log" + "net/http" )internal/utils/user_utils.go (1)
20-22: Remove unreachable length check after Split.After trimming and the prior empty-string guard, strings.Split(...) always yields len >= 1. This check never fires.
Apply this diff to simplify:
- if len(userList) == 0 { - return []config.User{}, errors.New("invalid user format") - }internal/controller/resources_controller.go (1)
33-42: Serve using the cached server with the computed prefix and standard status code.Avoid returning JSON for static file 404s unless you intend JSON for all clients.
Apply this diff:
func (controller *ResourcesController) resourcesHandler(c *gin.Context) { if controller.Config.ResourcesDir == "" { - c.JSON(404, gin.H{ - "status": 404, - "message": "Resources not found", - }) + c.AbortWithStatus(http.StatusNotFound) return } - controller.FileServer.ServeHTTP(c.Writer, c.Request) + http.StripPrefix(controller.prefix, controller.FileServer).ServeHTTP(c.Writer, c.Request) }Optional hardening: if directory listings are undesirable, wrap the filesystem to reject directory requests.
internal/service/oauth_broker_service.go (5)
3-8: Import fmt/sort/strings for better errors, determinism, and provider key handling.Apply this diff:
import ( + "fmt" "errors" "tinyauth/internal/config" + "sort" + "strings" "github.com/rs/zerolog/log" )
30-43: Normalize provider keys; warn on unknown instead of silently defaulting.Lowercasing avoids accidental duplicates (“Google” vs “google”); logging on unknown keys helps detect typos.
Apply this diff:
func (broker *OAuthBrokerService) Init() error { for name, cfg := range broker.Configs { - switch name { + key := strings.ToLower(strings.TrimSpace(name)) + switch key { case "github": service := NewGithubOAuthService(cfg) - broker.Services[name] = service + broker.Services[key] = service case "google": service := NewGoogleOAuthService(cfg) - broker.Services[name] = service + broker.Services[key] = service default: + log.Warn().Msgf("Unknown OAuth provider %q, using generic", name) service := NewGenericOAuthService(cfg) - broker.Services[name] = service + broker.Services[key] = service } }Note: If external callers use original-cased names, keep a mapping alias or document the normalized key requirement.
45-55: Don’t bail on first init error; initialize all and aggregate errors.This prevents one bad provider from blocking others.
Example approach:
- for name, service := range broker.Services { - err := service.Init() - if err != nil { - log.Error().Err(err).Msgf("Failed to initialize OAuth service: %s", name) - return err - } - log.Info().Msgf("Initialized OAuth service: %s", name) - } + var initErr error + for name, service := range broker.Services { + if err := service.Init(); err != nil { + log.Error().Err(err).Msgf("Failed to initialize OAuth service: %s", name) + initErr = errors.Join(initErr, fmt.Errorf("%s: %w", name, err)) + continue + } + log.Info().Msgf("Initialized OAuth service: %s", name) + } + if initErr != nil { + return initErr + }
57-63: Return a deterministic list of configured services.Sorting helps with tests and predictable UIs.
Apply this diff:
func (broker *OAuthBrokerService) GetConfiguredServices() []string { services := make([]string, 0, len(broker.Services)) for name := range broker.Services { services = append(services, name) } + sort.Strings(services) return services }
70-76: Include provider name in not-found errors.Improves debuggability; also ensure Init was called before use.
Apply this diff:
- return config.Claims{}, errors.New("oauth service not found") + return config.Claims{}, fmt.Errorf("oauth service %q not found", service)If this map is accessed concurrently, a read/write lock around broker.Services is advised.
Would you like a follow-up patch adding a RWMutex if the broker is used across goroutines post-initialization?
internal/utils/label_utils.go (1)
29-35: Harden header key validation against forbidden separators.Beyond spaces/tabs, RFC token disallows these separators: ()<>@,;:"/[]?={}. Reject them explicitly.
Apply this diff:
- key := SanitizeHeader(strings.TrimSpace(split[0])) - if strings.ContainsAny(key, " \t") { + key := SanitizeHeader(strings.TrimSpace(split[0])) + // Reject whitespace or separators in header field-name + if strings.ContainsAny(key, " \t()<>@,;:\\\"/[]?={}") { continue }Optional: instead of sanitizing keys, consider rejecting keys that change after sanitization to avoid silent mutation.
internal/utils/security_utils.go (1)
120-124: Avoid shadowing the uuid package name.Rename the local variable for readability.
Apply this diff:
func GenerateIdentifier(str string) string { - uuid := uuid.NewSHA1(uuid.NameSpaceURL, []byte(str)) - uuidString := uuid.String() - return strings.Split(uuidString, "-")[0] + id := uuid.NewSHA1(uuid.NameSpaceURL, []byte(str)) + idString := id.String() + return strings.Split(idString, "-")[0] }internal/controller/user_controller.go (2)
115-131: Nit: Avoid err shadowing; use inline short-if to constrain scope.Current block declares a new
errthat shadows the outer one. Inline keeps scope tight and avoids confusion.- err := controller.Auth.CreateSessionCookie(c, &config.SessionCookie{ + if err := controller.Auth.CreateSessionCookie(c, &config.SessionCookie{ Username: user.Username, Name: utils.Capitalize(req.Username), Email: fmt.Sprintf("%s@%s", strings.ToLower(req.Username), controller.Config.Domain), Provider: "username", TotpPending: true, - }) - - if err != nil { + }); err != nil { log.Error().Err(err).Msg("Failed to create session cookie") c.JSON(500, gin.H{ "status": 500, "message": "Internal Server Error", }) return - } + }
221-224: Message clarity: distinguish TOTP vs login rate limits.This branch is in the TOTP handler; the message still says “login attempts”. Minor UX papercut.
- "message": fmt.Sprintf("Too many failed login attempts. Try again in %d seconds", remainingTime), + "message": fmt.Sprintf("Too many failed TOTP attempts. Try again in %d seconds", remainingTime),internal/utils/app_utils.go (1)
76-102: Optional: Enforce HTTPS in IsRedirectSafe.If you want to prevent downgrades, also require
parsedURL.Scheme == "https".if !parsedURL.IsAbs() { return false } + if strings.ToLower(parsedURL.Scheme) != "https" { + return false + }internal/service/google_oauth_service.go (3)
45-53: Set HTTP client timeout to avoid hangs.The default client has no timeout, risking stuck goroutines on network stalls.
func (google *GoogleOAuthService) Init() error { - httpClient := &http.Client{} + httpClient := &http.Client{ + Timeout: 10 * time.Second, + } ctx := context.Background() ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient) verifier := oauth2.GenerateVerifier()
56-64: Fallback state generation: use Sprintf for broader Go version compatibility.
fmt.Appendfrequires newer Go;fmt.Sprintfis clearer.- return base64.RawURLEncoding.EncodeToString(fmt.Appendf(nil, "state-%d", time.Now().UnixNano())) + return base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf("state-%d", time.Now().UnixNano())))
33-43: Avoid import shadowing and future-proof scopesVerified that
OAuthServiceConfig(internal/config/config.go:95) includes aScopes []stringfield. To prevent shadowing the package name and allow callers to override the default Google scopes, please:• In internal/service/google_oauth_service.go, rename the parameter from
configtocfg
• Initializeoauth2.Config.Scopesby preferringcfg.Scopeswhen non-empty, otherwise falling back toGoogleOAuthScopesSuggested diff:
func NewGoogleOAuthService(cfg config.OAuthServiceConfig) *GoogleOAuthService { // prefer caller-provided scopes if available + scopes := GoogleOAuthScopes + if len(cfg.Scopes) > 0 { + scopes = cfg.Scopes + } return &GoogleOAuthService{ Config: oauth2.Config{ ClientID: cfg.ClientID, ClientSecret: cfg.ClientSecret, RedirectURL: cfg.RedirectURL, - Scopes: GoogleOAuthScopes, + Scopes: scopes, Endpoint: endpoints.Google, }, } }cmd/root.go (1)
44-46: Nit: Redundant cast; GetLogLevel already returns zerolog.Level.No behavior change; small cleanup.
- log.Logger = log.Level(zerolog.Level(utils.GetLogLevel(conf.LogLevel))) + log.Logger = log.Level(utils.GetLogLevel(conf.LogLevel))internal/middleware/context_middleware.go (1)
45-56: Optional: Replace goto with early returns for readability.The label pattern works but increases cognitive load. Early returns would simplify control flow.
If you’re open to it, I can provide a refactor patch converting the
basic:label to a helper function and early returns.Also applies to: 105-157
internal/controller/proxy_controller.go (3)
102-105: Avoid double GetSecret() reads and unintended Authorization overrideCurrently you (a) read the secret twice and (b) always overwrite Authorization with Basic, even if an Authorization header was already propagated. Cache the secret and only set Basic when no Authorization is already present (or make it explicit that you intend to override).
Apply to 102–105:
- if labels.Basic.Username != "" && utils.GetSecret(labels.Basic.Password.Plain, labels.Basic.Password.File) != "" { - log.Debug().Str("username", labels.Basic.Username).Msg("Setting basic auth header") - c.Header("Authorization", fmt.Sprintf("Basic %s", utils.GetBasicAuth(labels.Basic.Username, utils.GetSecret(labels.Basic.Password.Plain, labels.Basic.Password.File)))) - } + pwd := utils.GetSecret(labels.Basic.Password.Plain, labels.Basic.Password.File) + if labels.Basic.Username != "" && pwd != "" { + if c.Writer.Header().Get("Authorization") == "" { + log.Debug().Str("username", labels.Basic.Username).Msg("Setting basic auth header") + c.Header("Authorization", fmt.Sprintf("Basic %s", utils.GetBasicAuth(labels.Basic.Username, pwd))) + } else { + log.Debug().Msg("Authorization already set; skipping Basic override") + } + }Repeat analogous changes at 167–170 and 280–283.
Also applies to: 167-170, 280-283
95-101: DRY: factor header application into a helper to reduce duplicationParsing and applying label headers repeats three times. Extract into a small helper to keep logic consistent.
Example helper (outside this file, e.g., internal/utils/header.go):
func ApplyLabelHeaders(c *gin.Context, labels config.Labels) { headers := utils.ParseHeaders(labels.Headers) for k, v := range headers { log.Debug().Str("header", k).Msg("Setting header") c.Header(k, v) } }Then replace the repeated blocks with:
utils.ApplyLabelHeaders(c, labels)Also applies to: 160-166, 273-278
66-73: Be defensive if X-Forwarded-Host/Uri are missingWhen proxies misconfigure, host/uri can be empty, producing id=="" and odd redirects. Add a sanity fallback or 400 guard.
uri := c.Request.Header.Get("X-Forwarded-Uri") proto := c.Request.Header.Get("X-Forwarded-Proto") host := c.Request.Header.Get("X-Forwarded-Host") +if host == "" || proto == "" || uri == "" { + log.Warn().Msg("Missing X-Forwarded headers") + c.JSON(http.StatusBadRequest, gin.H{"status": 400, "message": "Bad Request"}) + return +}internal/bootstrap/app_bootstrap.go (4)
211-223: Rename local slice from “controller” to “controllers” to avoid shadowing the imported package namePrevents confusion and improves readability.
- controller := []Controller{ + controllers := []Controller{ contextController, oauthController, proxyController, userController, healthController, resourcesController, } - for _, ctrl := range controller { + for _, ctrl := range controllers { log.Debug().Msgf("Setting up %T controller", ctrl) ctrl.SetupRoutes() }
145-151: Add gin.Recovery() to guard against panics in handlersEnsures a 500 instead of crashing the process in production.
engine := gin.New() if config.Version != "development" { gin.SetMode(gin.ReleaseMode) } +// Recover from panics to avoid process crashes. +engine.Use(gin.Recovery())
163-170: Avoid shadowing the middleware package with the loop variableMinor naming cleanup.
-for _, middleware := range middlewares { - log.Debug().Str("middleware", fmt.Sprintf("%T", middleware)).Msg("Initializing middleware") - err := middleware.Init() +for _, m := range middlewares { + log.Debug().Str("middleware", fmt.Sprintf("%T", m)).Msg("Initializing middleware") + err := m.Init() if err != nil { return fmt.Errorf("failed to initialize middleware %T: %w", middleware, err) } - engine.Use(middleware.Middleware()) + engine.Use(m.Middleware()) }
248-257: Normalize Generic OAuth scopes to avoid empty/whitespace-only entriesstrings.Split can yield [""] when GenericScopes is empty, causing some providers to reject requests.
- Scopes: strings.Split(app.Config.GenericScopes, ","), + Scopes: strings.FieldsFunc(app.Config.GenericScopes, func(r rune) bool { return r == ',' || r == ' ' || r == '\t' }),internal/controller/oauth_controller.go (1)
109-114: Clear CSRF cookie on validation failure to avoid sticky stateOn mismatch, drop the cookie before redirecting.
if err != nil || state != csrfCookie { log.Warn().Err(err).Msg("CSRF token mismatch or cookie missing") + c.SetCookie(controller.Config.CSRFCookieName, "", -1, "/", controller.Config.Domain, controller.Config.SecureCookie, true) c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.Config.AppURL)) return }internal/service/generic_oauth_service.go (1)
52-54: Set an HTTP client timeout to avoid hangsAdd a sane Timeout to the OAuth HTTP client.
httpClient := &http.Client{ Transport: transport, + Timeout: 15 * time.Second, }internal/service/docker_service.go (2)
25-37: Nit: avoid shadowing the imported client package; enable API version negotiation in constructorUse cli var name, and consider WithAPIVersionNegotiation to simplify.
func (docker *DockerService) Init() error { - client, err := client.NewClientWithOpts(client.FromEnv) + cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) if err != nil { return err } ctx := context.Background() - client.NegotiateAPIVersion(ctx) - - docker.Client = client + docker.Client = cli docker.Context = ctx return nil }
73-95: Variable shadowing with import name; minor rename improves clarityRename loop variable to avoid clashing with the removed container import and improve readability.
- for _, container := range containers { - inspect, err := docker.InspectContainer(container.ID) + for _, ctr := range containers { + inspect, err := docker.InspectContainer(ctr.ID) if err != nil { - log.Warn().Str("id", container.ID).Err(err).Msg("Error inspecting container, skipping") + log.Warn().Str("id", ctr.ID).Err(err).Msg("Error inspecting container, skipping") continue } labels, err := utils.GetLabels(inspect.Config.Labels) if err != nil { - log.Warn().Str("id", container.ID).Err(err).Msg("Error getting container labels, skipping") + log.Warn().Str("id", ctr.ID).Err(err).Msg("Error getting container labels, skipping") continue } @@ - if strings.TrimPrefix(inspect.Name, "/") == app { + if strings.TrimPrefix(inspect.Name, "/") == app { log.Debug().Str("id", inspect.ID).Msg("Found matching container by name") return labels, nil } }internal/service/github_oauth_service.go (3)
96-96: Set GitHub API version header to avoid defaults and warningsGitHub recommends sending X-GitHub-Api-Version. Add it to both requests.
req.Header.Set("Accept", "application/vnd.github+json") +req.Header.Set("X-GitHub-Api-Version", "2022-11-28") @@ req.Header.Set("Accept", "application/vnd.github+json") +req.Header.Set("X-GitHub-Api-Version", "2022-11-28")Also applies to: 125-125
104-106: Make error messages endpoint-specificSmall DX polish: include the endpoint for faster triage.
- if res.StatusCode < 200 || res.StatusCode >= 300 { - return user, fmt.Errorf("request failed with status: %s", res.Status) - } + if res.StatusCode < 200 || res.StatusCode >= 300 { + return user, fmt.Errorf("GET /user failed: %s", res.Status) + }
133-135: Same as above for /user/emails- if res.StatusCode < 200 || res.StatusCode >= 300 { - return user, fmt.Errorf("request failed with status: %s", res.Status) - } + if res.StatusCode < 200 || res.StatusCode >= 300 { + return user, fmt.Errorf("GET /user/emails failed: %s", res.Status) + }internal/config/config.go (2)
3-8: Consider tightening Claims.Groups typeGroups as
anymakes downstream handling error-prone. If feasible, prefer[]string(common case) orjson.RawMessageto carry opaque structures.
14-16: Consider constants for cookie namesIf these are not intended to be mutated at runtime,
constcommunicates intent and prevents accidental changes.internal/service/auth_service.go (1)
183-217: Potential unbounded growth of LoginAttempts mapConsider expiring old entries to prevent unbounded memory growth under attack or long-running processes. A periodic cleanup or TTL-based eviction on read/write would suffice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (57)
.env.example(2 hunks).gitignore(1 hunks)air.toml(1 hunks)cmd/root.go(3 hunks)cmd/version.go(2 hunks)frontend/src/context/app-context.tsx(1 hunks)frontend/src/context/user-context.tsx(1 hunks)frontend/src/pages/login-page.tsx(1 hunks)frontend/src/pages/logout-page.tsx(1 hunks)frontend/src/pages/totp-page.tsx(1 hunks)frontend/vite.config.ts(1 hunks)go.mod(1 hunks)internal/assets/assets.go(1 hunks)internal/auth/auth_test.go(0 hunks)internal/bootstrap/app_bootstrap.go(1 hunks)internal/config/config.go(5 hunks)internal/constants/constants.go(0 hunks)internal/controller/context_controller.go(1 hunks)internal/controller/health_controller.go(1 hunks)internal/controller/oauth_controller.go(1 hunks)internal/controller/proxy_controller.go(1 hunks)internal/controller/resources_controller.go(1 hunks)internal/controller/user_controller.go(1 hunks)internal/handlers/context.go(0 hunks)internal/handlers/handlers.go(0 hunks)internal/handlers/handlers_test.go(0 hunks)internal/handlers/oauth.go(0 hunks)internal/handlers/proxy.go(0 hunks)internal/handlers/user.go(0 hunks)internal/hooks/hooks.go(0 hunks)internal/middleware/context_middleware.go(1 hunks)internal/middleware/ui_middleware.go(1 hunks)internal/middleware/zerolog_middleware.go(1 hunks)internal/oauth/oauth.go(0 hunks)internal/providers/generic.go(0 hunks)internal/providers/github.go(0 hunks)internal/providers/google.go(0 hunks)internal/providers/providers.go(0 hunks)internal/server/server.go(0 hunks)internal/service/auth_service.go(14 hunks)internal/service/docker_service.go(3 hunks)internal/service/generic_oauth_service.go(1 hunks)internal/service/github_oauth_service.go(1 hunks)internal/service/google_oauth_service.go(1 hunks)internal/service/ldap_service.go(5 hunks)internal/service/oauth_broker_service.go(1 hunks)internal/types/api.go(0 hunks)internal/types/types.go(0 hunks)internal/utils/app_utils.go(1 hunks)internal/utils/fs_utils.go(1 hunks)internal/utils/label_utils.go(1 hunks)internal/utils/security_utils.go(1 hunks)internal/utils/string_utils.go(1 hunks)internal/utils/user_utils.go(1 hunks)internal/utils/utils.go(0 hunks)internal/utils/utils_test.go(0 hunks)main.go(1 hunks)
💤 Files with no reviewable changes (19)
- internal/auth/auth_test.go
- internal/constants/constants.go
- internal/server/server.go
- internal/oauth/oauth.go
- internal/providers/google.go
- internal/handlers/handlers.go
- internal/providers/providers.go
- internal/providers/generic.go
- internal/handlers/context.go
- internal/providers/github.go
- internal/handlers/proxy.go
- internal/handlers/user.go
- internal/hooks/hooks.go
- internal/handlers/oauth.go
- internal/handlers/handlers_test.go
- internal/types/api.go
- internal/types/types.go
- internal/utils/utils.go
- internal/utils/utils_test.go
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 8-8: [UnorderedKey] The SECURE_COOKIE key should go before the USERS key
(UnorderedKey)
[warning] 28-28: [UnorderedKey] The LOG_LEVEL key should go before the OAUTH_WHITELIST key
(UnorderedKey)
2 participants
Summary by CodeRabbit
New Features
Changes
Chores