Add support for SSO / OpenID Connect (OIDC) #996
Conversation
WalkthroughAdds OpenID Connect (OIDC) SSO: provider implementation, controller integration and status exposure, new OIDC login/callback handlers and routes, service-layer OIDC login and user bootstrap, local-login guards, tests updated for password pointer, and go-oidc/oauth2 dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User (Browser)
participant API as API v1
participant OIDC as OIDC Provider
participant SVC as UserService
participant DB as Repo/DB
rect rgb(235,245,255)
note over API: OIDC enabled — initiation
U->>API: GET /api/v1/users/login/oidc
API->>API: generate state, nonce, PKCE\nset cookies (HttpOnly, Lax)
API-->>OIDC: 302 -> Authorization URL
end
U->>OIDC: Authenticate & Consent
OIDC-->>U: Redirect with code & state
rect rgb(240,255,240)
U->>API: GET /api/v1/users/login/oidc/callback?code=...
API->>API: validate state/nonce/PKCE\nclear cookies
API->>OIDC: exchange code for tokens
OIDC-->>API: ID token + access token
API->>API: verify ID token, parse claims
API->>SVC: LoginOIDC(email, name)
SVC->>DB: find user by email
alt Not found
SVC->>DB: create user (password=nil), create Home group & defaults
end
SVC-->>API: session token (4 weeks)
API-->>U: set auth cookies, 302 /home
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Suggested reviewers
Security recommendations
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (18)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/app/api/handlers/v1/v1_ctrl_auth.go (3)
121-125: Do not return raw auth errors to clients (user-enumeration risk).Returning err.Error() leaks reasons like “user not found” or “bad password.” Return a generic 401 instead.
- log.Err(err).Msg("failed to authenticate") - return server.JSON(w, http.StatusInternalServerError, err.Error()) + log.Warn().Err(err).Msg("authentication failed") + return validate.NewUnauthorizedError()
190-222: Cookies: set SameSite and avoid Host header–derived Domain.
- Add SameSite=Lax to mitigate CSRF.
- Avoid using r.Host-derived Domain to prevent host-header–based cookie scoping. Prefer host-only cookies (omit Domain) or a trusted configured domain.
- Domain: domain, + // Prefer host-only cookie; set a trusted domain only if explicitly configured. + // Domain: ctrl.config.Web.CookieDomain, + SameSite: http.SameSiteLaxMode,Apply to all three Set-Cookie calls in setCookies.
Security: This reduces cross-site request risks and cookie injection via manipulated Host headers.
224-255: Unset cookies: mirror SameSite and Domain strategy.Mirror the SameSite and Domain choices used when setting cookies to ensure proper deletion.
- Domain: domain, + // See setCookies note re: Domain + SameSite: http.SameSiteLaxMode,Apply to all three delete cookies.
backend/internal/core/services/service_user.go (1)
81-89: Don't ignore password hashing errors during local registration.If hashing fails, you’ll silently store an empty hash (breaking local login and weakening invariants). Handle the error and fail fast.
Apply:
- hashed, _ := hasher.HashPassword(data.Password) + hashed, err := hasher.HashPassword(data.Password) + if err != nil { + log.Err(err).Msg("Failed to hash password") + return repo.UserOut{}, err + }
🧹 Nitpick comments (16)
backend/app/api/routes.go (1)
77-78: Add OIDC routes conditionally to avoid needless 403s when disabled.Mount only when OIDC is enabled to reduce attack surface and noise.
- r.Get("/users/login/oidc", chain.ToHandlerFunc(v1Ctrl.HandleOIDCLogin())) - r.Get("/users/login/oidc/callback", chain.ToHandlerFunc(v1Ctrl.HandleOIDCCallback())) + if a.conf.OIDC.Enabled { + r.Get("/users/login/oidc", chain.ToHandlerFunc(v1Ctrl.HandleOIDCLogin())) + r.Get("/users/login/oidc/callback", chain.ToHandlerFunc(v1Ctrl.HandleOIDCCallback())) + }Security note: keep these endpoints unauthenticated, but ensure upstream proxies only allow expected methods.
backend/app/api/handlers/v1/v1_ctrl_user.go (1)
26-29: Good guard; also document 403 in swagger.The pre-check is correct. Consider documenting the 403 response in the swagger block so clients can anticipate it.
Example swagger addition (outside this hunk):
// @Failure 403 {string} string "Local login is not enabled"backend/internal/core/services/main_test.go (1)
41-41: Pointer password aligns with passwordless OIDC users; add a nil-password test.This change looks good. Please add a test path that creates an OIDC-style user with Password: nil to ensure repo/service constraints permit it.
Also applies to: 45-45
backend/app/api/handlers/v1/v1_ctrl_auth.go (3)
104-107: Only block when provider=local; move the check after parsing provider.Current guard forbids all providers if local login is disabled. Make it conditional so future providers can reuse this handler.
- // Forbidden if local login is not enabled - if !ctrl.config.Options.AllowLocalLogin { - return validate.NewRequestError(fmt.Errorf("Local login is not enabled"), http.StatusForbidden) - } + // Extract provider query + provider := r.URL.Query().Get("provider") + if provider == "" { + provider = "local" + } + // Block local only when disabled + if provider == "local" && !ctrl.config.Options.AllowLocalLogin { + return validate.NewRequestError(fmt.Errorf("Local login is not enabled"), http.StatusForbidden) + } - // Extract provider query - provider := r.URL.Query().Get("provider") - if provider == "" { - provider = "local" - }
264-281: OIDC init: looks fine; ensure state/nonce protection lives in provider.Assuming InitiateOIDCFlow issues a cryptographically strong state and binds it (cookie/server-side) with short TTL. If not, we should add it.
Do you want me to review providers/oidc.go for state binding (cookie flags, TTL), PKCE usage, and nonce validation?
291-317: Callback: set cookies OK; consider 303 and “remember” semantics.
- 302 is fine for GET; 303 See Other is slightly clearer for redirects after auth.
- “remember” is hardcoded true; if there’s a setting or user choice, wire it instead.
- ctrl.setCookies(w, noPort(r.Host), newToken.Raw, newToken.ExpiresAt, true) - http.Redirect(w, r, "/home", http.StatusFound) + ctrl.setCookies(w, noPort(r.Host), newToken.Raw, newToken.ExpiresAt, true /* or from config/user */) + http.Redirect(w, r, "/home", http.StatusSeeOther)Security: ensure the provider validates
state, verifies ID token signature/alg, checksaud,iss,exp, optionallynonce, and enforces allowedhd/groups if configured.backend/internal/core/services/service_user.go (3)
193-200: Good guard to block passwordless (OIDC) users from local login; minor timing nit.The dummy hash compare is fine, but if the hasher is disabled it degenerates to an equality check. Consider a fixed-cost dummy hash path to keep timing uniform.
239-241: Comment mismatch: extended session is 4 weeks, not 7 days.createSessionToken(true) sets 4×oneWeek. Fix the comment to avoid confusion.
- // Create session token with extended session (7 days) + // Create session token with extended session (4 weeks)
243-285: Reduce PII in logs when creating OIDC users.You log raw email at info level multiple times. Prefer debug level or anonymize.
Example:
- log.Info().Str("email", email).Msg("OIDC user not found, creating new user") + log.Debug().Str("user", email).Msg("OIDC user not found, creating new user")Repeat similarly for the “created successfully” log.
backend/app/api/providers/oidc.go (7)
46-89: Constructor validations and verifier wiring — solid; drop unused fields.oauth2.Config in struct is unused; DiscoveryDocument type is unused.
type OIDCProvider struct { @@ - oauth2 oauth2.Config endpoint oauth2.Endpoint } @@ -type DiscoveryDocument struct { - AuthorizationEndpoint string `json:"authorization_endpoint"` - TokenEndpoint string `json:"token_endpoint"` - UserinfoEndpoint string `json:"userinfo_endpoint"` - JwksURI string `json:"jwks_uri"` - Issuer string `json:"issuer"` -} +// (DiscoveryDocument removed if not used)
152-163: Group allow-list: consider case-insensitive matching (providers differ).AD/Azure/OIDC providers can vary in case. Optionally normalize both sides.
- if !p.hasAllowedGroup(claims.Groups, allowedGroups) { + if !p.hasAllowedGroup(stringsliceToLower(claims.Groups), stringsliceToLower(allowedGroups)) {Outside this hunk:
func stringsliceToLower(in []string) []string { out := make([]string, len(in)) for i, v := range in { out[i] = strings.ToLower(strings.TrimSpace(v)) } return out }
244-247: Optional: avoid requesting offline access if you don’t use refresh tokens.If you’re not persisting refresh tokens, drop AccessTypeOffline to reduce provider prompts.
-return oauth2Config.AuthCodeURL(state, oauth2.AccessTypeOffline) +return oauth2Config.AuthCodeURL(state)
358-373: TrustProxy: consider X-Forwarded-Host for base URL reconstruction.Some proxies rewrite Host; honoring X-Forwarded-Host when TrustProxy is true improves correctness.
- host := r.Host + host := r.Host + if p.options.TrustProxy { + if xfHost := r.Header.Get("X-Forwarded-Host"); xfHost != "" { + host = xfHost + } + }
181-224: Claims parsing is robust; add defaults fallback.If claim names aren’t set, fall back to standard "email" and "name".
- if emailValue, exists := rawClaims[p.config.EmailClaim]; exists { + key := p.config.EmailClaim + if key == "" { key = "email" } + if emailValue, exists := rawClaims[key]; exists { @@ - if nameValue, exists := rawClaims[p.config.NameClaim]; exists { + key := p.config.NameClaim + if key == "" { key = "name" } + if nameValue, exists := rawClaims[key]; exists {
171-179: Security hardening (optional): nonce and email_verified.Consider adding a nonce in AuthCodeURL and verifying it; optionally enforce email_verified==true for providers that emit it.
249-264: Enforce ‘openid’ scope in getOAuth2Config. Defaults includeopenid(see backend/internal/sys/config/conf.go:85), but customscopevalues may omit it; validate thatopenidis present or prepend it top.config.Scopebefore splitting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (70)
backend/go.sumis excluded by!**/*.sumbackend/internal/data/ent/attachment.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/client.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/ent.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/has_id.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/migrate/schema.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/mutation.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/runtime.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/runtime/runtime.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_update.gois excluded by!backend/internal/data/ent/**
📒 Files selected for processing (8)
backend/app/api/handlers/v1/controller.go(5 hunks)backend/app/api/handlers/v1/v1_ctrl_auth.go(3 hunks)backend/app/api/handlers/v1/v1_ctrl_user.go(1 hunks)backend/app/api/providers/oidc.go(1 hunks)backend/app/api/routes.go(1 hunks)backend/go.mod(2 hunks)backend/internal/core/services/main_test.go(1 hunks)backend/internal/core/services/service_user.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/api/handlers/v1/v1_ctrl_auth.go (1)
backend/app/api/handlers/v1/controller.go (1)
V1Controller(68-79)
backend/internal/core/services/service_user.go (1)
backend/pkgs/hasher/password.go (1)
CheckPasswordHash(71-90)
⏰ 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). (18)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: Backend Server Tests / Go
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
backend/go.mod (1)
11-11: Run local vulnerability audit for OIDC and OAuth2 modules
CI sandbox couldn’t install or execute govulncheck—please run locally:cd backend go install golang.org/x/vuln/cmd/govulncheck@latest govulncheck github.com/coreos/go-oidc/[email protected] golang.org/x/[email protected]Verify no CVEs are reported against v3.15.0 and v0.30.0, then commit the findings. Use Dependabot (or similar) for ongoing automated checks.
backend/app/api/handlers/v1/controller.go (3)
78-79: Controller holds OIDC provider dependency — looks good.Field wiring is straightforward.
100-110: API summary exposes OIDC status cleanly.Fits UI needs (enabled, text, force, allowLocal).
159-165: Status plumbing for OIDC flags — LGTM.Values mirror config; no further action.
| ctrl.initOIDCProvider() | ||
|
|
||
| return ctrl | ||
| } | ||
|
|
||
| func (ctrl *V1Controller) initOIDCProvider() { | ||
| if ctrl.config.OIDC.Enabled { | ||
| oidcProvider, err := providers.NewOIDCProvider(ctrl.svc.User, &ctrl.config.OIDC, &ctrl.config.Options, ctrl.cookieSecure) | ||
| if err != nil { | ||
| log.Err(err).Msg("failed to initialize OIDC provider at startup") | ||
| } else { | ||
| ctrl.oidcProvider = oidcProvider | ||
| log.Info().Msg("OIDC provider initialized successfully at startup") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Startup initialization is fine; ensure handlers guard nil provider.
If provider init fails, ctrl.oidcProvider remains nil. Verify login/callback handlers short-circuit with a 503-style error instead of panicking.
Run:
🏁 Script executed:
#!/bin/bash
# Check handlers gate on nil provider
rg -n -C2 'HandleOIDC(Login|Callback)' backend/app/api/handlers/v1 | catLength of output: 1555
Guard against nil OIDC provider in handlers
In HandleOIDCLogin (v1_ctrl_auth.go:264) and HandleOIDCCallback (v1_ctrl_auth.go:291), add a check for ctrl.oidcProvider != nil and return a 503 Service Unavailable if it’s nil to prevent panics when initialization fails.
🤖 Prompt for AI Agents
In backend/app/api/handlers/v1/controller.go around lines 124 to 139 and update
the handlers in v1_ctrl_auth.go at lines ~264 and ~291: both HandleOIDCLogin and
HandleOIDCCallback must check if ctrl.oidcProvider == nil at the start and
immediately return a 503 Service Unavailable response when it is nil to avoid
nil-pointer panics; add the nil-check, log or trace the condition as needed, and
write a 503 response (consistent JSON/error format used by other handlers)
instead of proceeding when the provider is not initialized.
JeffResc
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/app/api/providers/oidc.go (4)
111-119: Use request context for token exchange.Prevents wasted work when the client disconnects.
- // Exchange code for token with timeout - ctx, cancel := context.WithTimeout(context.Background(), p.config.RequestTimeout) + // Exchange code for token with timeout, tied to request lifecycle + ctx, cancel := context.WithTimeout(r.Context(), p.config.RequestTimeout) defer cancel() token, err := oauth2Config.Exchange(ctx, code)
127-135: Also use request context for ID token verification.Same rationale as above.
- // Parse and validate the ID token using the library's verifier with timeout - verifyCtx, verifyCancel := context.WithTimeout(context.Background(), p.config.RequestTimeout) + // Parse and validate the ID token using the library's verifier with timeout + verifyCtx, verifyCancel := context.WithTimeout(r.Context(), p.config.RequestTimeout) defer verifyCancel() idTokenStruct, err := p.verifier.Verify(verifyCtx, idToken)
265-287: State cookie domain may mismatch behind proxies; add SameSite.When Options.Hostname overrides Host, the cookie set on r.Host won’t be sent on callback. Also add SameSite to harden CSRF.
// Get base URL from request baseURL := p.getBaseURL(r) // Store state in session cookie for validation - http.SetCookie(w, &http.Cookie{ + u, _ := url.Parse(baseURL) + domain := u.Hostname() + if domain == "" { + domain = noPort(r.Host) + } + http.SetCookie(w, &http.Cookie{ Name: "oidc_state", Value: state, Expires: time.Now().Add(p.config.StateExpiry), - Domain: noPort(r.Host), + Domain: domain, Secure: p.isSecure(r), HttpOnly: true, Path: "/", + SameSite: http.SameSiteLaxMode, })Security note: Lax works with top-level GET redirects typical for OIDC.
323-333: Reliably clear the state cookie (domain + MaxAge + SameSite).Ensures deletion across browsers and matches the set attributes.
- // Clear state cookie - http.SetCookie(w, &http.Cookie{ + // Clear state cookie + u, _ := url.Parse(p.getBaseURL(r)) + domain := u.Hostname() + if domain == "" { + domain = noPort(r.Host) + } + http.SetCookie(w, &http.Cookie{ Name: "oidc_state", Value: "", Expires: time.Unix(0, 0), - Domain: noPort(r.Host), + Domain: domain, + MaxAge: -1, Secure: p.isSecure(r), HttpOnly: true, Path: "/", + SameSite: http.SameSiteLaxMode, })
🧹 Nitpick comments (8)
backend/app/api/providers/oidc.go (8)
61-67: Add timeout to provider discovery.Avoids hanging on slow/unreachable issuers.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), config.RequestTimeout) + defer cancel() provider, err := oidc.NewProvider(ctx, config.IssuerURL)
243-246: Least-privilege: don't request offline access unless you actually use refresh tokens.You aren’t storing/using refresh tokens; requesting them increases scope.
-return oauth2Config.AuthCodeURL(state, oauth2.AccessTypeOffline) +return oauth2Config.AuthCodeURL(state)If offline access can be optionally required, gate it behind config and add oauth2.AccessTypeOffline conditionally.
248-263: Split scopes robustly.Use strings.Fields to handle multiple spaces and avoid empty scopes.
- Scopes: strings.Split(p.config.Scope, " "), + Scopes: strings.Fields(p.config.Scope),
224-241: Normalize user groups before comparison.Prevents false negatives due to stray whitespace/case.
allowedGroupsMap := make(map[string]bool) for _, group := range allowedGroups { - allowedGroupsMap[strings.TrimSpace(group)] = true + allowedGroupsMap[strings.ToLower(strings.TrimSpace(group))] = true } for _, userGroup := range userGroups { - if allowedGroupsMap[userGroup] { + if allowedGroupsMap[strings.ToLower(strings.TrimSpace(userGroup))] { return true } }
180-222: Optional: support nested group claims and robust parsing.Some IdPs (e.g., Keycloak) expose roles under nested keys (realm_access.roles). Consider supporting dotted paths or JSON pointers in GroupClaim.
341-351: Add nonce to the OIDC flow (defense-in-depth).Include a cryptographic nonce in AuthCodeURL and verify it in the ID token (nonce claim). Store alongside state (httpOnly cookie).
Security win against token replay/mix-up even in pure code flow.
353-356: IPv6-safe host handling.noPort() splits on “:”, which breaks on IPv6. If you keep it, switch to url.Hostname() wherever possible (as in the cookie changes), or replace noPort with net.SplitHostPort-aware logic.
-func noPort(host string) string { - return strings.Split(host, ":")[0] -} +// Prefer url.Hostname() on a parsed URL; if you must use a raw host:port, use net.SplitHostPort with fallbacks.
357-371: Optional: honor X-Forwarded-Host when TrustProxy is true.You already respect X-Forwarded-Proto; considering XFH improves correctness behind proxies without setting Options.Hostname.
host := r.Host if p.options.Hostname != "" { host = p.options.Hostname + } else if p.options.TrustProxy { + if xfHost := r.Header.Get("X-Forwarded-Host"); xfHost != "" { + host = xfHost + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/handlers/v1/v1_ctrl_auth.go(3 hunks)backend/app/api/handlers/v1/v1_ctrl_user.go(1 hunks)backend/app/api/providers/oidc.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/api/handlers/v1/v1_ctrl_auth.go
- backend/app/api/handlers/v1/v1_ctrl_user.go
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/api/providers/oidc.go (2)
backend/internal/core/services/service_user.go (2)
UserService(22-24)UserAuthTokenDetail(33-37)backend/internal/sys/config/conf.go (3)
OIDCConf(80-94)Options(36-45)Config(19-34)
…ks CSRF check). Add SameSite. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/app/api/providers/oidc.go (2)
355-389: Host-only cookies for localhost/IP; don’t set Domain attribute.Browsers reject Domain=localhost and often for raw IPs, breaking the flow in local/dev and IP-based deployments. Set Domain only for registrable names; keep host-only cookies otherwise. Also applies to nonce and PKCE cookies. Add net import.
@@ - // Store state in session cookie for validation - http.SetCookie(w, &http.Cookie{ + // Store state in session cookie for validation + hostOnly := domain == "localhost" || net.ParseIP(domain) != nil + stateCookie := &http.Cookie{ Name: "oidc_state", Value: state, Expires: time.Now().Add(p.config.StateExpiry), - Domain: domain, Secure: p.isSecure(r), HttpOnly: true, Path: "/", SameSite: http.SameSiteLaxMode, - }) + } + if !hostOnly { + stateCookie.Domain = domain + } + http.SetCookie(w, stateCookie) @@ - // Store nonce in session cookie for validation - http.SetCookie(w, &http.Cookie{ + // Store nonce in session cookie for validation + nonceCookie := &http.Cookie{ Name: "oidc_nonce", Value: nonce, Expires: time.Now().Add(p.config.StateExpiry), - Domain: domain, Secure: p.isSecure(r), HttpOnly: true, Path: "/", SameSite: http.SameSiteLaxMode, - }) + } + if !hostOnly { + nonceCookie.Domain = domain + } + http.SetCookie(w, nonceCookie) @@ - // Store PKCE verifier in session cookie for token exchange - http.SetCookie(w, &http.Cookie{ + // Store PKCE verifier in session cookie for token exchange + pkceCookie := &http.Cookie{ Name: "oidc_pkce_verifier", Value: pkceVerifier, Expires: time.Now().Add(p.config.StateExpiry), - Domain: domain, Secure: p.isSecure(r), HttpOnly: true, Path: "/", SameSite: http.SameSiteLaxMode, - }) + } + if !hostOnly { + pkceCookie.Domain = domain + } + http.SetCookie(w, pkceCookie)Additional import required:
import ( "context" "crypto/rand" "crypto/sha256" "encoding/base64" "fmt" + "net" "net/http" "net/url"
409-441: Clear cookies with the same host-only logic (and avoid duplication).Deletion must mirror creation; otherwise some browsers will keep stale cookies when Domain mismatches. Use hostOnly and set Domain only for registrable names.
- clearCookies := func() { - http.SetCookie(w, &http.Cookie{ - Name: "oidc_state", - Value: "", - Expires: time.Unix(0, 0), - Domain: domain, - MaxAge: -1, - Secure: p.isSecure(r), - HttpOnly: true, - Path: "/", - SameSite: http.SameSiteLaxMode, - }) - http.SetCookie(w, &http.Cookie{ - Name: "oidc_nonce", - Value: "", - Expires: time.Unix(0, 0), - Domain: domain, - MaxAge: -1, - Secure: p.isSecure(r), - HttpOnly: true, - Path: "/", - SameSite: http.SameSiteLaxMode, - }) - http.SetCookie(w, &http.Cookie{ - Name: "oidc_pkce_verifier", - Value: "", - Expires: time.Unix(0, 0), - Domain: domain, - MaxAge: -1, - Secure: p.isSecure(r), - HttpOnly: true, - Path: "/", - SameSite: http.SameSiteLaxMode, - }) - } + hostOnly := domain == "localhost" || net.ParseIP(domain) != nil + clearCookies := func() { + clear := func(name string) { + c := &http.Cookie{ + Name: name, + Value: "", + Expires: time.Unix(0, 0), + MaxAge: -1, + Secure: p.isSecure(r), + HttpOnly: true, + Path: "/", + SameSite: http.SameSiteLaxMode, + } + if !hostOnly { + c.Domain = domain + } + http.SetCookie(w, c) + } + clear("oidc_state") + clear("oidc_nonce") + clear("oidc_pkce_verifier") + }
🧹 Nitpick comments (2)
backend/app/api/providers/oidc.go (2)
284-291: Normalize user groups before lookup to avoid false negatives.Allowed groups are trimmed, but userGroups aren’t. Whitespace or casing differences from the IdP can cause mismatches.
[security_recommendation]
- allowedGroupsMap := make(map[string]bool) + allowedGroupsMap := make(map[string]bool) for _, group := range allowedGroups { - allowedGroupsMap[strings.TrimSpace(group)] = true + allowedGroupsMap[strings.ToLower(strings.TrimSpace(group))] = true } @@ - for _, userGroup := range userGroups { - if allowedGroupsMap[userGroup] { + for _, userGroup := range userGroups { + if allowedGroupsMap[strings.ToLower(strings.TrimSpace(userGroup))] { return true } }
165-174: Security hardening: prefer rejecting when email_verified is explicitly false; log issuer/subject.Current behavior is fine. For defense-in-depth, when VerifyEmail is enabled, include issuer (iss) and subject (sub) in the warning/error logs to aid forensic traces, and keep rejecting if email_verified is absent or false.
[security_recommendation]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/api/providers/oidc.go(1 hunks)backend/internal/core/services/service_user.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/internal/core/services/service_user.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JeffResc
PR: sysadminsmedia/homebox#996
File: backend/app/api/providers/oidc.go:156-170
Timestamp: 2025-09-07T07:51:13.505Z
Learning: The HomeBox authentication system uses email as the primary user identifier with a unique database constraint. Email-based OIDC identity binding was chosen over issuer+subject pairs to avoid major architectural refactoring, despite some theoretical security trade-offs.
📚 Learning: 2025-09-07T07:51:13.505Z
Learnt from: JeffResc
PR: sysadminsmedia/homebox#996
File: backend/app/api/providers/oidc.go:156-170
Timestamp: 2025-09-07T07:51:13.505Z
Learning: The HomeBox authentication system uses email as the primary user identifier with a unique database constraint. Email-based OIDC identity binding was chosen over issuer+subject pairs to avoid major architectural refactoring, despite some theoretical security trade-offs.
Applied to files:
backend/app/api/providers/oidc.go
⏰ 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). (13)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
| return oauth2.Config{ | ||
| ClientID: p.config.ClientID, | ||
| ClientSecret: p.config.ClientSecret, | ||
| RedirectURL: redirectURL, | ||
| Endpoint: p.endpoint, | ||
| Scopes: strings.Fields(p.config.Scope), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure “openid” scope is present to guarantee an ID token.
If config.Scope omits “openid”, many providers will not return id_token (line 118), breaking the flow. Force-include it in getOAuth2Config.
- return oauth2.Config{
+ // Ensure "openid" is included
+ scopes := strings.Fields(p.config.Scope)
+ hasOpenID := false
+ for _, s := range scopes {
+ if s == "openid" {
+ hasOpenID = true
+ break
+ }
+ }
+ if !hasOpenID {
+ scopes = append([]string{"openid"}, scopes...)
+ }
+ return oauth2.Config{
ClientID: p.config.ClientID,
ClientSecret: p.config.ClientSecret,
RedirectURL: redirectURL,
Endpoint: p.endpoint,
- Scopes: strings.Fields(p.config.Scope),
+ Scopes: scopes,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return oauth2.Config{ | |
| ClientID: p.config.ClientID, | |
| ClientSecret: p.config.ClientSecret, | |
| RedirectURL: redirectURL, | |
| Endpoint: p.endpoint, | |
| Scopes: strings.Fields(p.config.Scope), | |
| } | |
| // Ensure "openid" is included | |
| scopes := strings.Fields(p.config.Scope) | |
| hasOpenID := false | |
| for _, s := range scopes { | |
| if s == "openid" { | |
| hasOpenID = true | |
| break | |
| } | |
| } | |
| if !hasOpenID { | |
| scopes = append([]string{"openid"}, scopes...) | |
| } | |
| return oauth2.Config{ | |
| ClientID: p.config.ClientID, | |
| ClientSecret: p.config.ClientSecret, | |
| RedirectURL: redirectURL, | |
| Endpoint: p.endpoint, | |
| Scopes: scopes, | |
| } |
🤖 Prompt for AI Agents
In backend/app/api/providers/oidc.go around lines 315 to 321, the oauth2 config
builds Scopes from strings.Fields(p.config.Scope) but does not guarantee the
"openid" scope is present, which can prevent providers from returning an
id_token; modify the code that constructs Scopes to parse p.config.Scope into a
slice, check for "openid" (case-sensitive per spec) and add it if missing
(ensuring no duplicate entries), then pass that resulting slice into
oauth2.Config so "openid" is always included.
| u, _ := url.Parse(baseURL) | ||
| domain := u.Hostname() | ||
| if domain == "" { | ||
| domain = noPort(r.Host) | ||
| } |
There was a problem hiding this comment.
Avoid nil deref: handle url.Parse errors before using u.Hostname().
If baseURL is malformed (e.g., misconfigured Options.Hostname), url.Parse can fail and u will be nil, causing a panic on u.Hostname(). Guard the error and fall back to r.Host.
- u, _ := url.Parse(baseURL)
- domain := u.Hostname()
- if domain == "" {
- domain = noPort(r.Host)
- }
+ u, err := url.Parse(baseURL)
+ domain := ""
+ if err == nil && u != nil {
+ domain = u.Hostname()
+ }
+ if domain == "" {
+ domain = noPort(r.Host)
+ }Also applies to: 403-407
🤖 Prompt for AI Agents
In backend/app/api/providers/oidc.go around lines 349 to 353 (and similarly at
403 to 407), url.Parse(baseURL) is used without checking its error, so u may be
nil and u.Hostname() can panic; change the code to check the parse error and
only call u.Hostname() when err == nil (and u != nil), otherwise fall back to
using noPort(r.Host) (or r.Host directly) as the domain; apply the same guarded
pattern to the other occurrence at lines 403-407.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/api/handlers/v1/v1_ctrl_user.go (1)
136-139: Fix Swagger route path for change passwordDocs say
/v1/users/change-password, but the route is/v1/users/self/change-password. Update to prevent broken API docs.Apply:
-// @Router /v1/users/change-password [PUT] +// @Router /v1/users/self/change-password [PUT]backend/internal/core/services/service_user.go (1)
83-89: Handle HashPassword error instead of ignoring it.Hashing can fail; return a proper error to avoid creating users with an unintended empty/invalid hash.
- hashed, _ := hasher.HashPassword(data.Password) + hashed, err := hasher.HashPassword(data.Password) + if err != nil { + log.Err(err).Msg("Failed to hash password") + return repo.UserOut{}, err + } usrCreate := repo.UserCreate{ Name: data.Name, Email: data.Email, - Password: &hashed, + Password: &hashed, IsSuperuser: false,
♻️ Duplicate comments (7)
backend/app/api/handlers/v1/v1_ctrl_auth.go (1)
312-315: Set cookie domain using Hostname/TrustProxy to avoid lost sessions behind proxiesUse configured hostname or X-Forwarded-Host when trusted; repeat for login, refresh, and logout.
Apply:
- ctrl.setCookies(w, noPort(r.Host), newToken.Raw, newToken.ExpiresAt, true) + // Respect Options.Hostname and reverse proxies + domain := noPort(r.Host) + if h := strings.TrimSpace(ctrl.config.Options.Hostname); h != "" { + domain = strings.Split(h, ":")[0] + } else if ctrl.config.Options.TrustProxy { + if xf := r.Header.Get("X-Forwarded-Host"); xf != "" { + xfh := strings.TrimSpace(strings.Split(xf, ",")[0]) + domain = strings.Split(xfh, ":")[0] + } + } + ctrl.setCookies(w, domain, newToken.Raw, newToken.ExpiresAt, true)Repeat analogous domain derivation in:
- Line 127 (HandleAuthLogin)
- Line 181 (HandleAuthRefresh)
- Line 155 (HandleAuthLogout uses unsetCookies)
backend/internal/core/services/service_user.go (1)
235-243: Decouple service from ent: use repo sentinel for not-found.Prefer a repository-level sentinel to avoid leaking ent into the service layer.
- if !ent.IsNotFound(err) { + if !errors.Is(err, repo.ErrNotFound) { log.Err(err).Str("email", email).Msg("failed to lookup user by email") return UserAuthTokenDetail{}, err }backend/app/api/providers/oidc.go (5)
307-321: Ensure “openid” scope is always included.Without it, many providers won’t return an ID token, breaking the flow.
func (p *OIDCProvider) getOAuth2Config(baseURL string) oauth2.Config { @@ - return oauth2.Config{ + // Ensure "openid" is included + scopes := strings.Fields(p.config.Scope) + hasOpenID := false + for _, s := range scopes { + if s == "openid" { + hasOpenID = true + break + } + } + if !hasOpenID { + scopes = append([]string{"openid"}, scopes...) + } + return oauth2.Config{ ClientID: p.config.ClientID, ClientSecret: p.config.ClientSecret, RedirectURL: redirectURL, Endpoint: p.endpoint, - Scopes: strings.Fields(p.config.Scope), + Scopes: scopes, } }
347-353: Avoid nil deref on url.Parse(baseURL).Guard the parse error before calling Hostname().
- u, _ := url.Parse(baseURL) - domain := u.Hostname() + u, err := url.Parse(baseURL) + domain := "" + if err == nil && u != nil { + domain = u.Hostname() + } if domain == "" { domain = noPort(r.Host) }
355-389: Cookie Domain handling: use host-only cookies for localhost/IP to prevent callback breakage. Add hostOnly check.Browsers reject Domain=localhost or IP. Set Domain only for registrable domains.
- // Store state in session cookie for validation - http.SetCookie(w, &http.Cookie{ + hostOnly := domain == "localhost" || net.ParseIP(domain) != nil + // Store state in session cookie for validation + stateCookie := &http.Cookie{ Name: "oidc_state", Value: state, Expires: time.Now().Add(p.config.StateExpiry), - Domain: domain, Secure: p.isSecure(r), HttpOnly: true, Path: "/", SameSite: http.SameSiteLaxMode, - }) + } + if !hostOnly { + stateCookie.Domain = domain + } + http.SetCookie(w, stateCookie) // Store nonce in session cookie for validation - http.SetCookie(w, &http.Cookie{ + nonceCookie := &http.Cookie{ Name: "oidc_nonce", Value: nonce, Expires: time.Now().Add(p.config.StateExpiry), - Domain: domain, Secure: p.isSecure(r), HttpOnly: true, Path: "/", SameSite: http.SameSiteLaxMode, - }) + } + if !hostOnly { + nonceCookie.Domain = domain + } + http.SetCookie(w, nonceCookie) // Store PKCE verifier in session cookie for token exchange - http.SetCookie(w, &http.Cookie{ + pkceCookie := &http.Cookie{ Name: "oidc_pkce_verifier", Value: pkceVerifier, Expires: time.Now().Add(p.config.StateExpiry), - Domain: domain, Secure: p.isSecure(r), HttpOnly: true, Path: "/", SameSite: http.SameSiteLaxMode, - }) + } + if !hostOnly { + pkceCookie.Domain = domain + } + http.SetCookie(w, pkceCookie)Add import:
- "net/url" + "net/url" + "net"
401-407: Same url.Parse guard here to prevent panic.- u, _ := url.Parse(baseURL) - domain := u.Hostname() + u, err := url.Parse(baseURL) + domain := "" + if err == nil && u != nil { + domain = u.Hostname() + } if domain == "" { domain = noPort(r.Host) }
408-442: Clear cookies with host-only logic, too.Mirror the hostOnly Domain logic when clearing so browsers actually delete them.
- clearCookies := func() { - http.SetCookie(w, &http.Cookie{ + clearCookies := func() { + hostOnly := domain == "localhost" || net.ParseIP(domain) != nil + c := func(name string) *http.Cookie { + c := &http.Cookie{ + Name: name, + Value: "", + Expires: time.Unix(0, 0), + MaxAge: -1, + Secure: p.isSecure(r), + HttpOnly: true, + Path: "/", + SameSite: http.SameSiteLaxMode, + } + if !hostOnly { + c.Domain = domain + } + return c + } + http.SetCookie(w, c("oidc_state")) + http.SetCookie(w, c("oidc_nonce")) + http.SetCookie(w, c("oidc_pkce_verifier")) - }) - http.SetCookie(w, &http.Cookie{ - Name: "oidc_nonce", - Value: "", - Expires: time.Unix(0, 0), - Domain: domain, - MaxAge: -1, - Secure: p.isSecure(r), - HttpOnly: true, - Path: "/", - SameSite: http.SameSiteLaxMode, - }) - http.SetCookie(w, &http.Cookie{ - Name: "oidc_pkce_verifier", - Value: "", - Expires: time.Unix(0, 0), - Domain: domain, - MaxAge: -1, - Secure: p.isSecure(r), - HttpOnly: true, - Path: "/", - SameSite: http.SameSiteLaxMode, - }) }
🧹 Nitpick comments (12)
backend/internal/core/services/main_test.go (1)
41-46: Password pointer change is fine; add a nil-password path test for OIDC usersTo guard regressions, add a case that creates a user with
Password: nil(simulating OIDC-provisioned accounts) and ensures auth flows behave as expected.backend/app/api/handlers/v1/v1_ctrl_auth.go (5)
110-114: Optional: return 404 instead of 403 for disabled local loginAvoids exposing auth configuration details to clients. Safe either way.
121-133: Add no-store to login/refresh responses to prevent token cachingEven with HttpOnly cookies, you also return the token in JSON. Set
Cache-Control: no-storeon these responses.Apply:
newToken, err := p.Authenticate(w, r) if err != nil { log.Warn().Err(err).Msg("authentication failed") return validate.NewUnauthorizedError() } + // Prevent caching of auth response + w.Header().Set("Cache-Control", "no-store") ctrl.setCookies(w, noPort(r.Host), newToken.Raw, newToken.ExpiresAt, true) return server.JSON(w, http.StatusOK, TokenResponse{newToken, err := ctrl.svc.User.RenewToken(r.Context(), requestToken) if err != nil { return validate.NewUnauthorizedError() } + // Prevent caching of auth response + w.Header().Set("Cache-Control", "no-store") ctrl.setCookies(w, noPort(r.Host), newToken.Raw, newToken.ExpiresAt, false) return server.JSON(w, http.StatusOK, newToken)Also applies to: 176-183
265-276: Prefer 503 when OIDC provider isn’t initializedUse Service Unavailable to reflect a transient startup/init issue instead of 500.
Apply:
- log.Error().Msg("OIDC provider not initialized") - return validate.NewRequestError(errors.New("OIDC provider not available"), http.StatusInternalServerError) + log.Error().Msg("OIDC provider not initialized") + return validate.NewRequestError(errors.New("OIDC provider not available"), http.StatusServiceUnavailable)
291-302: Mirror 503 for nil OIDC provider in callbackSame rationale as login path.
Apply:
- log.Error().Msg("OIDC provider not initialized") - return validate.NewRequestError(errors.New("OIDC provider not available"), http.StatusInternalServerError) + log.Error().Msg("OIDC provider not initialized") + return validate.NewRequestError(errors.New("OIDC provider not available"), http.StatusServiceUnavailable)
190-222: Add SameSite=Lax to auth cookies to mitigate CSRF; mirror in unsetExplicitly set SameSite for token/remember/session cookies. This is a low-effort security win and compatible with OIDC redirects.
Apply:
http.SetCookie(w, &http.Cookie{ Name: cookieNameRemember, Value: strconv.FormatBool(remember), Expires: expires, Domain: domain, Secure: ctrl.cookieSecure, HttpOnly: true, + SameSite: http.SameSiteLaxMode, Path: "/", }) // Set HTTP only cookie http.SetCookie(w, &http.Cookie{ Name: cookieNameToken, Value: token, Expires: expires, Domain: domain, Secure: ctrl.cookieSecure, HttpOnly: true, + SameSite: http.SameSiteLaxMode, Path: "/", }) // Set Fake Session cookie http.SetCookie(w, &http.Cookie{ Name: cookieNameSession, Value: "true", Expires: expires, Domain: domain, Secure: ctrl.cookieSecure, HttpOnly: false, + SameSite: http.SameSiteLaxMode, Path: "/", })And in
unsetCookies:http.SetCookie(w, &http.Cookie{ Name: cookieNameToken, Value: "", Expires: time.Unix(0, 0), Domain: domain, Secure: ctrl.cookieSecure, HttpOnly: true, + SameSite: http.SameSiteLaxMode, Path: "/", }) http.SetCookie(w, &http.Cookie{ Name: cookieNameRemember, Value: "false", Expires: time.Unix(0, 0), Domain: domain, Secure: ctrl.cookieSecure, HttpOnly: true, + SameSite: http.SameSiteLaxMode, Path: "/", }) // Set Fake Session cookie http.SetCookie(w, &http.Cookie{ Name: cookieNameSession, Value: "false", Expires: time.Unix(0, 0), Domain: domain, Secure: ctrl.cookieSecure, HttpOnly: false, + SameSite: http.SameSiteLaxMode, Path: "/", })[Security]
Also applies to: 224-255
backend/app/api/handlers/v1/controller.go (3)
124-139: Startup init is fine; consider surfacing readinessIf init fails,
config.OIDC.Enabledmay be true while the provider is nil. Consider tracking readiness for UI.
159-165: Report provider readiness in status to prevent a broken OIDC buttonOnly show OIDC as enabled when initialized.
Apply:
- OIDC: OIDCStatus{ - Enabled: ctrl.config.OIDC.Enabled, + OIDC: OIDCStatus{ + Enabled: ctrl.config.OIDC.Enabled && ctrl.oidcProvider != nil, ButtonText: ctrl.config.OIDC.ButtonText, Force: ctrl.config.OIDC.Force, AllowLocal: ctrl.config.Options.AllowLocalLogin, },
129-133: Proxy/hostname awareness for OIDC redirect URLEnsure
providers.NewOIDCProviderbuilds its redirect URL usingOptions.HostnameandTrustProxy(X-Forwarded-Proto/Host) to avoid mismatches behind reverse proxies.backend/internal/core/services/service_user.go (3)
195-201: Constant-time guard: do real work for timing equalization.Comparing two plain strings via CheckPasswordHash may short-circuit quickly. Do a real hash+compare to normalize timing.
- // SECURITY: Perform hash to ensure response times are the same - hasher.CheckPasswordHash("not-a-real-password", "not-a-real-password") + // SECURITY: Do equivalent work to avoid timing leaks + if dummyHash, derr := hasher.HashPassword("not-a-real-password"); derr == nil { + hasher.CheckPasswordHash("not-a-real-password", dummyHash) + }Also apply the same pattern to the earlier path at Line 191 for consistency.
294-309: Bootstrap error policy differs from RegisterUser. Decide and document.Here label/location creation errors are logged and ignored; in RegisterUser (Lines 100-116) they are fatal. Align behavior or document why OIDC differs.
236-265: Minor formatting: fix indentation for readability.Mixed spaces/tabs around this block; run gofmt/goimports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (70)
backend/go.sumis excluded by!**/*.sumbackend/internal/data/ent/attachment.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/client.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/ent.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/has_id.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/migrate/schema.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/mutation.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/runtime.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/runtime/runtime.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_update.gois excluded by!backend/internal/data/ent/**
📒 Files selected for processing (8)
backend/app/api/handlers/v1/controller.go(5 hunks)backend/app/api/handlers/v1/v1_ctrl_auth.go(4 hunks)backend/app/api/handlers/v1/v1_ctrl_user.go(1 hunks)backend/app/api/providers/oidc.go(1 hunks)backend/app/api/routes.go(1 hunks)backend/go.mod(2 hunks)backend/internal/core/services/main_test.go(1 hunks)backend/internal/core/services/service_user.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-07T07:51:13.505Z
Learnt from: JeffResc
PR: sysadminsmedia/homebox#996
File: backend/app/api/providers/oidc.go:156-170
Timestamp: 2025-09-07T07:51:13.505Z
Learning: The HomeBox authentication system uses email as the primary user identifier with a unique database constraint. Email-based OIDC identity binding was chosen over issuer+subject pairs to avoid major architectural refactoring, despite some theoretical security trade-offs.
Applied to files:
backend/app/api/providers/oidc.go
⏰ 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). (6)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
🔇 Additional comments (9)
backend/go.mod (2)
11-46: Directly requiring go-oidc and oauth2 looks rightNo issues spotted with the pins; aligns with the new OIDC flow.
11-46: Verified secure versions of go-oidc and oauth2
go-oidc v3.15.0 and golang.org/x/oauth2 v0.30.0 are the latest stable releases as of September 7, 2025, and include fixes for CVE-2025-27144 and CVE-2025-22868, respectively—no version bumps required. Recommend scheduling periodic dependency audits to catch future advisories.backend/app/api/routes.go (1)
78-81: Good: OIDC routes gated behind configEndpoints are only mounted when OIDC is enabled. Ensure the exact callback path is whitelisted in the IdP’s Redirect URIs.
backend/app/api/handlers/v1/v1_ctrl_user.go (2)
23-24: Swagger update for 403 is correctDocumented 403 matches the new guard.
27-31: Confirm desired behavior: block group-token registrations when local login is disabledThe guard runs before decoding payload, so invite/group-token registrations are also forbidden. If invites should still work, move this check after decode and conditionally allow when
GroupTokenis present.backend/app/api/handlers/v1/v1_ctrl_auth.go (3)
5-5: Import addition is fine
123-125: Better UX: unauthorized on auth failureSwitch to 401 and warn-level logging is appropriate.
283-317: OIDC security measures correctly implementedState and nonce tokens are generated with crypto/rand and stored in secure, HttpOnly cookies; PKCE verifier and challenge use S256 with a 43-char verifier; redirect URI is built via url.JoinPath for exact matching; and all provider calls use context.WithTimeout for request deadlines. No further changes needed.
backend/app/api/handlers/v1/controller.go (1)
78-79: Controller field for OIDC provider looks goodNo issues.
The attachment test was failing intermittently in CI due to a race condition between attachment creation and retrieval. Adding a small 100ms delay after attachment creation ensures the file system and database operations complete before the test attempts to verify the attachment exists.
…ondition" This reverts commit 4aa8b2a.
|
I'm unable to determine the cause for the failing frontend integration test: If anyone has any ideas, please let me know, but to my understanding, this code shouldn't modify the ability to create items and add attachments |
|
I was able to successfully complete tests with a few providers, but open to feedback about what other providers people are interested in verifying compatibility with. AuthentikSpecial notes:
"HBOX_OIDC_ENABLED": "true",
"HBOX_OIDC_ISSUER_URL": "https://authentik.example.com/application/o/<application-slug>/",
"HBOX_OIDC_CLIENT_ID": "<client-id>",
"HBOX_OIDC_CLIENT_SECRET": "<client-secret>",
"HBOX_OIDC_VERIFY_EMAIL": "true",KeycloakSpecial notes:
"HBOX_OIDC_ENABLED": "true",
"HBOX_OIDC_ISSUER_URL": "https://keycloak.example.com/realms/<realm>",
"HBOX_OIDC_CLIENT_ID": "<client-id>",
"HBOX_OIDC_CLIENT_SECRET": "<client-secret>",
"HBOX_OIDC_VERIFY_EMAIL": "true","HBOX_OIDC_ENABLED": "true",
"HBOX_OIDC_ISSUER_URL": "https://accounts.google.com",
"HBOX_OIDC_CLIENT_ID": "<client-id>",
"HBOX_OIDC_CLIENT_SECRET": "<client-secret>",
"HBOX_OIDC_VERIFY_EMAIL": "true", |
|
The failing SQLite integration test is a known thing, it's sporadic at best, so for the most part if it's specifically that error we ignore it. I will personally likely test with Zitadel, and maybe Azure if I get a chance, and I'll review the back end code when I get a chance here in the next day or two. I do want to note, there is a bug in boss.dev making it impossible to verify accounts to receive rewards, we've informed the devs, but have no timeline for a fix unfortunately. ( @chrishiestand if it's at all possible to get any information on that) |
Hi can you please test the compatibility with PocketID? |
Thanks for confirming this, I'll ignore that test for now.
Ah, thanks for the heads up. Hopefully they can get that resolved, but not a huge deal on my end. |
Hi! I was able to verify compatibility with PocketID using a PocketID Demo Instance. A few special notes:
"HBOX_OIDC_ENABLED": "true",
"HBOX_OIDC_ISSUER_URL": "https://<demo-instance-id>.demo.pocket-id.org",
"HBOX_OIDC_CLIENT_ID": "<client-id>",
"HBOX_OIDC_CLIENT_SECRET": "<client-secret>",
"HBOX_OIDC_VERIFY_EMAIL": "true", |
|
Simply exquisite. Thank you so much for your hard work on this one @JeffResc ! |
|
Won’t merge until @tonyaellie has had a chance to weigh in 🙂 |
| @@ -165,6 +201,10 @@ | |||
| loading.value = false; | |||
| } | |||
|
|
|||
| function loginWithOIDC() { | |||
| window.location.href = "/api/v1/users/login/oidc"; | |||
There was a problem hiding this comment.
Why not use:
const router = useRouter();
router.push({ path: ... })
There was a problem hiding this comment.
Tried doing this, but it gives a 404. Seemingly, refreshing the page that has the 404 works and does the auth flow...
There was a problem hiding this comment.
I guess that kind of makes sense, having looked at it again is there a reason not to use an a tag?
| class="w-full" | ||
| @click="loginWithOIDC" | ||
| > | ||
| {{ status.oidc.buttonText || "Sign in with OIDC" }} |
There was a problem hiding this comment.
Might be nice to make the button default if status?.oidc?.allowLocal !== false?
There was a problem hiding this comment.
Right now, allowLocal and oidc.enabled are not mutually exclusive. So even if allowLocal == true, OIDC could be disabled. I think it would make sense to have a check in the backend on initialization to ensure that either localLogin or OIDC are enabled
There was a problem hiding this comment.
I think a backend check is definitely needed and then have something like :variant="status?.oidc?.allowLocal !== false ? 'default' : 'outline'" on this button
|
@tonyaellie Thanks for the helpful review. I think everything has been addressed now. For the ones I resolved, feel free to review and mark as unresolved if unsatisfactory. For the two left unresolved, please let me know how you'd like to proceed. |
|
@JeffResc Looks great, left replies on the comments :) |
There was a problem hiding this comment.
A user on our discord correctly pointed out (and I originally missed it) that this PR is associating the user by email.
While many applications do this it is actually the incorrect way to do it, and a user should actually be identified by the sub claim (a unique string) and iss (issuer). This would require storing the sub ID and iss with the user table.
This makes it so that things like a user changing their email/identity doesn't break SSO and result in a new separate account getting created. While that's unlikely to happen with Google for example, it is something that can happen for Github, Azure Entra ID, etc. the exact details of the sub claim can be read at https://openid.net/specs/openid-connect-core-1_0.html#IDToken
Also of note, currently all of the user information must come from the ID Token claims, which not all providers do by default (or at all), switching to use the userinfo endpoint (https://github.com/coreos/go-oidc/blob/v3/example/userinfo/app.go#L88-92) would likely be a good move from a usability standpoint. The userinfo endpoint is required by the OIDC standard, so we should be able to rely on it existing at minimum, there might be some additional scopes we need to add by default (per https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims)
Sorry to be a PITA at the last minute
|
Thanks for the PR, this is a much needed feature and very appreciated. To follow up with what @tankerkiller125 said as the "user from the discord": Note that this implementation is not OIDC spec compliant and does not follow best practice IMO. It is very close though. The spec compliance issue is that the current implementation relies on the
A common pattern is to get a token then retrieve an email claim (from the id token if available, otherwise from the UserInfo endpoint as I'll discuss down below) to first match with an existing user then associating the combined I would argue that omitting the ability to query the UserInfo endpoint for claims is not best practice. Yes, many idps implement the ability to request that email and group scopes be returned in the id token, but it is not required for OIDC spec compliance as they are "treated by Authorization Servers as Voluntary Claims" and at least one major self hosting idp player calls out using the id token for anything but identity derived from the iss and sub claims as an anti-pattern. Additionally, the ability to query the UserInfo endpoint is a required test for basic OIDC Certification so this would be a nonconforming implementation, meeting none of the certification profiles. In short, this client is not compatible with all spec compliant idps. Being able to use the claims returned in the id token but falling back to the UserInfo endpoint if required scopes are not returned brings this into compliance. I was able to get this implementation working with Authelia and it does work great once I got it configured, but while figuring out the config that would work with it I ran into the big red "You should not configure things this way" note that they left on the claims_policies.id_token config item. |
|
Might be worth doing the autoredirect within https://github.com/sysadminsmedia/homebox/blob/main/frontend/middleware/auth.ts so only 1 redirect instead of 2, though as the redirect is already client side i dont know if it would really make much diff |
6 participants
What type of PR is this?
What this PR does / why we need it:
This PR introduces comprehensive OpenID Connect (OIDC) authentication support to HomeBox, enabling integration with external identity providers like Keycloak, Authentik, Google, Microsoft, and other OIDC-compliant providers.
Implementation Security Posture:
New configuration variables:
Demo
Which issue(s) this PR fixes:
Closes #6
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Documentation