refactor: rework user context handling throughout tinyauth#829
Open
steveiliop56 wants to merge 24 commits intomainfrom
Open
refactor: rework user context handling throughout tinyauth#829steveiliop56 wants to merge 24 commits intomainfrom
steveiliop56 wants to merge 24 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR migrates many types/constants from internal/config into a new internal/model package, renames fields (e.g., Ldap→LDAP, TotpSecret→TOTPSecret), adds typed user/context models and claims/version vars, converts session/auth to DB-backed cookie flows, and updates callers (controllers, middleware, services, tests, CLI, and build/linker flags). ChangesModel & Core Refactor (single DAG)
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Browser
participant MW as Middleware (Context)
participant Auth as Auth Service
participant DB as Database (sessions)
participant CTL as Controller
Client->>MW: HTTP request (cookie or basic auth)
alt session cookie present
MW->>Auth: GetSession(ctx, uuid)
Auth->>DB: Query session row
DB-->>Auth: session record
Auth->>MW: *model.UserContext (NewFromSession)
MW->>Client: optionally Set-Cookie (refreshed)
MW->>CTL: Forward request with model.UserContext
else no session cookie
MW->>Auth: basic auth validate (username/password)
Auth->>Auth: CheckUserPassword / LDAP bind
Auth-->>MW: *model.UserContext on success
MW->>CTL: Forward request with model.UserContext
end
CTL->>Auth: On login: CreateSession(ctx, sessionData)
Auth->>DB: Insert session row
DB-->>Auth: inserted id
Auth->>CTL: *http.Cookie
CTL->>Client: Set-Cookie header
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
dosubot
Bot
added
the
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/oauth_extractors.go (1)
46-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly fall back to a verified GitHub email.
This new fallback accepts the first returned email when no primary email exists. That broadens the login surface from “reject when no usable email is present” to “trust whatever comes first”, and that value is later used for whitelist decisions. Please filter for a verified email and fail closed if none is available.
Suggested fix
type GithubEmailResponse []struct { Email string `json:"email"` Primary bool `json:"primary"` + Verified bool `json:"verified"` } - for _, email := range *userEmails { - if email.Primary { + for _, email := range *userEmails { + if email.Primary && email.Verified { user.Email = email.Email break } } - // Use first available email if no primary email was found + // Fall back to the first verified email only if user.Email == "" { - user.Email = (*userEmails)[0].Email + for _, email := range *userEmails { + if email.Verified { + user.Email = email.Email + break + } + } + } + + if user.Email == "" { + return nil, errors.New("no verified emails found") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/oauth_extractors.go` around lines 46 - 60, The current fallback assigns the first returned email to user.Email when no primary exists; instead, require a verified GitHub email. In the block that inspects *userEmails (variables userEmails, user.Email, loop variable email), first pick email where email.Primary && email.Verified; if none, iterate again to find the first email with email.Verified and set user.Email to that; if still none, return an error (e.g., return nil, errors.New("no verified email found")). Ensure you update the same function where the primary/first-email logic lives (references: userEmails, user.Email, email.Primary, email.Verified).
🧹 Nitpick comments (5)
go.mod (1)
3-3: 💤 Low value
go 1.26.0is valid, but consider tracking the latest patch.Go 1.26 was released on February 10, 2026. Go 1.26.2 (released 2026-04-07) includes security fixes to the go command, compiler, and several standard library packages. While the
godirective only sets the minimum language/library version and doesn't pin the toolchain, bumping it togo 1.26.2signals the minimum safe version to consumers.♻️ Proposed change
-go 1.26.0 +go 1.26.2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` at line 3, Update the go directive in go.mod from "go 1.26.0" to "go 1.26.2" to declare the minimum required Go version that includes the relevant security/bugfix patches; edit the go.mod file and change the version string in the existing go directive so the module tracks the latest patch release.internal/model/version.go (1)
3-5: 💤 Low valueConsider grouping version variables into a single
varblock.Three adjacent top-level
vardeclarations are conventionally grouped in Go.♻️ Proposed refactor
-var Version = "development" -var CommitHash = "development" -var BuildTimestamp = "0000-00-00T00:00:00Z" +var ( + Version = "development" + CommitHash = "development" + BuildTimestamp = "0000-00-00T00:00:00Z" +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/model/version.go` around lines 3 - 5, Group the three top-level variables Version, CommitHash, and BuildTimestamp into a single var block instead of separate var declarations; update the declaration of Version, CommitHash and BuildTimestamp to a single grouped var (...) statement so they are declared together while preserving their current default values.cmd/tinyauth/verify_user.go (1)
98-110: 💤 Low valueTOTP control flow is correct; minor UX edge case when code is omitted.
When
user.TOTPSecretis set buttCfg.Totpis empty (user runsverifywithout providing--totp),totp.Validate("", secret)returnsfalse, surfacing"TOTP code incorrect"instead of a more descriptive"TOTP code required". Not a bug — the user is correctly rejected — but the message can mislead.♻️ Proposed improvement
+ if tCfg.Totp == "" { + return fmt.Errorf("TOTP code required") + } + ok := totp.Validate(tCfg.Totp, user.TOTPSecret)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/tinyauth/verify_user.go` around lines 98 - 110, The code currently calls totp.Validate(tCfg.Totp, user.TOTPSecret) and returns "TOTP code incorrect" when tCfg.Totp is empty; before invoking totp.Validate (in the block that checks user.TOTPSecret and tCfg.Totp), add an explicit check for an empty tCfg.Totp when user.TOTPSecret != "" and return a clearer error (e.g., "TOTP code required") or log a user-facing message indicating the TOTP was omitted; this change should be made immediately before the call to totp.Validate so the existing totp.Validate and error flow remain unchanged for non-empty codes.internal/controller/controller.go (1)
3-8: ⚡ Quick win
GroupErr bool(and empty string fields) will always appear in serialized URLs withoutomitempty.With
github.com/google/go-querystring, zero-value fields are included by default.groupErr=falsewill appear in every unauthorized redirect URL even when there is no group error, and empty string fields will serialize asusername=&resource=&ip=. At minimum,GroupErrshould useomitemptyto suppress the noise; the string fields should follow the same pattern unless the receiving handler relies on key presence.♻️ Proposed fix
type UnauthorizedQuery struct { - Username string `url:"username"` - Resource string `url:"resource"` - GroupErr bool `url:"groupErr"` - IP string `url:"ip"` + Username string `url:"username,omitempty"` + Resource string `url:"resource,omitempty"` + GroupErr bool `url:"groupErr,omitempty"` + IP string `url:"ip,omitempty"` } type RedirectQuery struct { - RedirectURI string `url:"redirect_uri"` + RedirectURI string `url:"redirect_uri,omitempty"` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/controller.go` around lines 3 - 8, The UnauthorizedQuery struct currently serializes zero-value fields (empty strings and false booleans) into the query string; update the struct tags on UnauthorizedQuery (fields Username, Resource, IP, and GroupErr) to include `omitempty` so go-querystring will omit empty values (unless the receiving handler explicitly depends on the presence of those keys), e.g., change the `url:"username"`/`resource`/`ip`/`groupErr` tags to use `url:"...,omitempty"` for each field to suppress noise in generated URLs.internal/service/auth_service.go (1)
146-151: ⚡ Quick winAdd defensive nil check for robustness.
While the typical flow ensures
SearchUservalidates the user first,GetLocalUsercan still returnnil. A defensive check prevents potential panics if this method is called directly or if there's a race condition.🛡️ Proposed fix
func (auth *AuthService) CheckUserPassword(search model.UserSearch, password string) error { switch search.Type { case model.UserLocal: user := auth.GetLocalUser(search.Username) + if user == nil { + return ErrUserNotFound + } return bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(password))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/auth_service.go` around lines 146 - 151, In CheckUserPassword, after calling GetLocalUser(search.Username) add a defensive nil check: if the returned user is nil return a descriptive error (use the project's existing ErrUserNotFound or, if none, return errors.New("user not found")) before calling bcrypt.CompareHashAndPassword; this prevents a nil dereference when accessing user.Password and keeps behavior consistent with SearchUser validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/bootstrap/app_bootstrap.go`:
- Around line 123-126: The trace call currently logging raw user records via
tlog.App.Trace().Interface("users", app.context.localUsers) leaks credential
material; change the dump to avoid sensitive fields by logging only safe
metadata (e.g., user count and a list of usernames or IDs) instead of the full
app.context.localUsers structure — replace the Interface call with one that
computes and logs something like len(app.context.localUsers) and a slice/map of
usernames (or IDs) extracted from each user record, ensuring no password hashes,
TOTP secrets, or other credentials are included.
In `@internal/bootstrap/router_bootstrap.go`:
- Around line 101-104: The logoutHandler in UserController is redirecting to
controller.config.SessionCookieName (a cookie name) instead of a URL; update
UserController.logoutHandler to use a real path such as "/" or "/login" for
c.Redirect, and keep SessionCookieName only for cookie operations. Locate the
UserController.logoutHandler (referenced by UserController and
controller.config.SessionCookieName) and replace the redirect target with the
chosen route (e.g., "/" or "/login") while leaving cookie clearing logic
unchanged; ensure any tests or callers expecting the previous behavior are
updated accordingly.
In `@internal/controller/context_controller.go`:
- Around line 79-101: The code builds a UserContextResponse from the result of
new(model.UserContext).NewFromGin(c) but immediately calls accessors
(GetUsername, GetName, GetEmail, ProviderName, IsOAuth, TOTPPending, OAuthName)
that dereference provider-specific subcontexts and can panic if the
Local/LDAP/OAuth subcontext is nil; before calling these accessors, validate
that the provider-specific subcontext exists (e.g., switch on
context.ProviderName() or context.Provider and check context.Local != nil for
local, context.LDAP != nil for ldap, context.OAuth != nil for oauth) and if the
expected subcontext is missing return the 401 UserContextResponse
(IsLoggedIn:false) instead of proceeding; update the block that constructs
UserContextResponse to only call the safe accessors after this check.
In `@internal/controller/user_controller_test.go`:
- Around line 28-50: The test embeds secret-shaped Base32 TOTP fixtures
(TOTPSecret fields in the model.LocalUser entries) which trigger secret
scanning; replace those hardcoded values by generating TOTP secrets at test
runtime instead: update the LocalUsers fixture construction in
user_controller_test.go to call a small helper (e.g., generateTOTPSecret or
makeTOTPSecret) when setting TOTPSecret on model.LocalUser entries (and add that
helper in the test file) using crypto/rand + base32.StdEncoding so each test
creates a non-committed secret; alternatively, if you must keep a static value,
add an explicit allowlist comment/token for the scanner next to the TOTPSecret
field, but prefer runtime generation for model.LocalUser and any tests that
reference TOTPSecret.
- Around line 68-98: The TOTP test fixtures totpCtx and totpAttrCtx create
UserContext objects with TOTPPending: true but currently set Authenticated:
true; update both helpers so Authenticated: false to mirror the middleware's
representation of TOTP-pending sessions (leave Provider, Local, BaseContext,
TOTPEnabled/TOTPPending fields intact) so the /api/user/totp flow tests exercise
unauthenticated TOTP-pending behavior.
In `@internal/controller/user_controller.go`:
- Around line 309-311: The code dereferences user.TOTPSecret without checking
that controller.auth.GetLocalUser(context.GetUsername()) returned a non-nil
user, which can panic if the account was removed; update the flow in the block
using controller.auth.GetLocalUser to check if user == nil and return a clean
rejection (e.g., HTTP 401/400 or the existing error path) before calling
totp.Validate(req.Code, user.TOTPSecret); ensure references to req.Code and
totp.Validate remain but only run after the nil check so the handler fails
gracefully instead of panicking.
- Around line 229-253: The logout handler currently aborts and returns 500 if
new(model.UserContext).NewFromGin(c) fails, which prevents clearing the session
cookie; change the flow so you always call controller.auth.DeleteSession(c,
uuid) and http.SetCookie(c.Writer, cookie) regardless of NewFromGin error,
treating tlog.AuditLogout(c, context.GetUsername(), context.ProviderName()) as
best-effort: attempt to build context (new(model.UserContext).NewFromGin), and
if it fails log the error but continue to call controller.auth.DeleteSession and
still set the cookie; only handle and return on errors from DeleteSession, and
call tlog.AuditLogout only when context was successfully built (use
context.GetUsername()/ProviderName() only if context != nil).
In `@internal/middleware/context_middleware_test.go`:
- Around line 27-44: The TOTP secret is hardcoded in the test fixture
(authServiceCfg -> LocalUsers[].TOTPSecret) which triggers secret scanning;
replace the literal Base32 string by generating a test TOTP secret at runtime or
using a clearly labeled non-secret placeholder and/or an explicit test-only
allowlist. Update the test to set LocalUsers[1].TOTPSecret via a helper (e.g.,
generateTestTOTPSecret()) or assign a constant like "TEST_TOTP_SECRET_NOT_PROD"
and document it as test-only so service.AuthServiceConfig and model.LocalUser
usage no longer include secret-shaped literals.
In `@internal/middleware/context_middleware.go`:
- Around line 68-77: The current logic returns early when cookieAuth fails,
preventing the Basic auth path from running; change the flow so that after
retrieving the cookie (c.Cookie using m.config.SessionCookieName) you call
m.cookieAuth and if it succeeds set the user context/session and return, but if
m.cookieAuth returns an error simply log it (tlog.App.Error().Msgf(...)) and do
NOT return — allow execution to proceed to the Basic auth branch; only
short-circuit/return when cookieAuth succeeds.
In `@internal/model/constants.go`:
- Around line 15-18: Restore the dropped provider override by adding the
"microsoft" entry back into the OverrideProviders map in constants.go so the
display-name override is preserved; specifically update the OverrideProviders
variable (map[string]string) to include "microsoft": "Microsoft" alongside the
existing "google"/"Google" and "github"/"GitHub" entries.
In `@internal/model/context_test.go`:
- Around line 62-103: The test is discarding errors from NewFromSession which
can lead to nil-pointer panics; update each run closure that calls
NewFromSession (the four sites using "got, _ := c.NewFromSession(...)") to check
the returned error and fail the test immediately with a clear message (e.g.,
call t.Fatalf / require.NoError / panic with the error) if err != nil before
dereferencing got, so any unexpected failure surfaces as a test failure instead
of a panic.
In `@internal/model/context.go`:
- Around line 81-88: The code does a type assertion to (*UserContext) which can
succeed even for a typed nil; after the assertion in the Get/FromContext
function (the variables userContext and type UserContext and receiver c), add a
nil check: if userContext == nil { return nil, errors.New("nil user context") }
before dereferencing *userContext and assigning *c = *userContext, so you avoid
panics when Gin stored a typed-nil pointer.
- Around line 116-123: The current construction of OAuthContext sets Groups
using strings.Split(session.OAuthGroups, ",") which turns an empty string into
[]string{""}; change this so that if session.OAuthGroups is empty (or only
whitespace) you assign nil (or an empty zero-length slice) to
OAuthContext.Groups, otherwise assign strings.Split(session.OAuthGroups, ",")
(optionally trimming/filtering empty entries). Update the code that builds
OAuthContext (the OAuthContext struct initializer where Groups is set) to
perform this check so downstream ACL checks see no groups rather than a single
empty-string group.
- Around line 92-131: UserContext.NewFromSession currently mutates the receiver
without clearing prior state, leaving stale pointers (Local, LDAP, OAuth) and
only ever setting Authenticated true; update NewFromSession to first reset the
receiver's provider-related fields (set c.Local=nil, c.LDAP=nil, c.OAuth=nil),
reset c.Provider to a zero/unknown value and set c.Authenticated=false, then
rebuild the appropriate provider branch (Local, LDAP, OAuth) from the session as
before and finally set c.Authenticated = !session.TotpPending (so it can be
false as well); reference the NewFromSession method and fields
UserContext.Authenticated, UserContext.Local, UserContext.LDAP,
UserContext.OAuth and TotpPending when making the change.
In `@internal/service/auth_service.go`:
- Around line 122-128: In AuthService.SearchUser, avoid dereferencing a nil
return from GetLocalUser by first assigning the result (e.g., local :=
auth.GetLocalUser(username)) and checking if local != nil before accessing
local.Username; if nil, proceed with the existing non-local lookup/return path,
otherwise construct and return the model.UserSearch with Type model.UserLocal.
Ensure you reference AuthService.SearchUser and GetLocalUser when making the
change.
- Around line 371-381: The cookie Expires and MaxAge are inconsistent: Expires
is set to the remaining time (newExpiry - currentTime) while MaxAge uses
auth.config.SessionExpiry; update the code that builds the cookie (the return
creating &http.Cookie with Name auth.config.SessionCookieName and Value
session.UUID) to use the same remaining-seconds value for both Expires and
MaxAge (e.g., compute remaining := int(newExpiry - currentTime) and use
time.Now().Add(time.Duration(remaining) * time.Second) for Expires and remaining
for MaxAge) so both attributes match across browsers.
In `@internal/utils/user_utils_test.go`:
- Around line 16-24: Tests in internal/utils/user_utils_test.go use a hard-coded
/tmp path; replace that with a per-test temporary directory from t.TempDir() and
create fixture files with filepath.Join(tempDir, "tinyauth_users_test.txt")
(adjust the os.Create, os.Remove calls and any missing-file test to use the same
tempDir). Update the file variable uses and any assertions that reference the
path so both the fixture and missing-file case are built from t.TempDir() to
make tests isolated and portable; apply the same change for the other
occurrences mentioned (lines around 29-30, 50, 68, 90).
- Around line 17-23: Replace test assertions that must stop execution on failure
with require checks: add the require import and change assert.NoError calls
around file operations (the file.WriteString and file.Close sections) to
require.NoError, and change the assert checks that validate GetUsers and
ParseUser results (the GetUsers result validation before using *users and
user.Username, and the ParseUser result validation before dereferencing) to
require.NoError/require.NotNil so the test aborts immediately on setup/
nil-sensitive failures.
---
Outside diff comments:
In `@internal/service/oauth_extractors.go`:
- Around line 46-60: The current fallback assigns the first returned email to
user.Email when no primary exists; instead, require a verified GitHub email. In
the block that inspects *userEmails (variables userEmails, user.Email, loop
variable email), first pick email where email.Primary && email.Verified; if
none, iterate again to find the first email with email.Verified and set
user.Email to that; if still none, return an error (e.g., return nil,
errors.New("no verified email found")). Ensure you update the same function
where the primary/first-email logic lives (references: userEmails, user.Email,
email.Primary, email.Verified).
---
Nitpick comments:
In `@cmd/tinyauth/verify_user.go`:
- Around line 98-110: The code currently calls totp.Validate(tCfg.Totp,
user.TOTPSecret) and returns "TOTP code incorrect" when tCfg.Totp is empty;
before invoking totp.Validate (in the block that checks user.TOTPSecret and
tCfg.Totp), add an explicit check for an empty tCfg.Totp when user.TOTPSecret !=
"" and return a clearer error (e.g., "TOTP code required") or log a user-facing
message indicating the TOTP was omitted; this change should be made immediately
before the call to totp.Validate so the existing totp.Validate and error flow
remain unchanged for non-empty codes.
In `@go.mod`:
- Line 3: Update the go directive in go.mod from "go 1.26.0" to "go 1.26.2" to
declare the minimum required Go version that includes the relevant
security/bugfix patches; edit the go.mod file and change the version string in
the existing go directive so the module tracks the latest patch release.
In `@internal/controller/controller.go`:
- Around line 3-8: The UnauthorizedQuery struct currently serializes zero-value
fields (empty strings and false booleans) into the query string; update the
struct tags on UnauthorizedQuery (fields Username, Resource, IP, and GroupErr)
to include `omitempty` so go-querystring will omit empty values (unless the
receiving handler explicitly depends on the presence of those keys), e.g.,
change the `url:"username"`/`resource`/`ip`/`groupErr` tags to use
`url:"...,omitempty"` for each field to suppress noise in generated URLs.
In `@internal/model/version.go`:
- Around line 3-5: Group the three top-level variables Version, CommitHash, and
BuildTimestamp into a single var block instead of separate var declarations;
update the declaration of Version, CommitHash and BuildTimestamp to a single
grouped var (...) statement so they are declared together while preserving their
current default values.
In `@internal/service/auth_service.go`:
- Around line 146-151: In CheckUserPassword, after calling
GetLocalUser(search.Username) add a defensive nil check: if the returned user is
nil return a descriptive error (use the project's existing ErrUserNotFound or,
if none, return errors.New("user not found")) before calling
bcrypt.CompareHashAndPassword; this prevents a nil dereference when accessing
user.Password and keeps behavior consistent with SearchUser validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1caf3c53-9013-4a35-8f35-caee20b26c2e
⛔ Files ignored due to path filters (2)
gen/gen_env.gois excluded by!**/gen/**gen/gen_md.gois excluded by!**/gen/**
📒 Files selected for processing (51)
cmd/tinyauth/generate_totp.gocmd/tinyauth/tinyauth.gocmd/tinyauth/verify_user.gocmd/tinyauth/version.gogo.modinternal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/controller/context_controller.gointernal/controller/context_controller_test.gointernal/controller/controller.gointernal/controller/oauth_controller.gointernal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware.gointernal/middleware/context_middleware_test.gointernal/model/config.gointernal/model/constants.gointernal/model/context.gointernal/model/context_test.gointernal/model/users.gointernal/model/version.gointernal/service/access_controls_service.gointernal/service/auth_service.gointernal/service/docker_service.gointernal/service/kubernetes_service.gointernal/service/kubernetes_service_test.gointernal/service/oauth_broker_service.gointernal/service/oauth_extractors.gointernal/service/oauth_presets.gointernal/service/oauth_service.gointernal/service/oidc_service.gointernal/service/oidc_service_test.gointernal/utils/app_utils.gointernal/utils/app_utils_test.gointernal/utils/decoders/label_decoder_test.gointernal/utils/fs_utils_test.gointernal/utils/label_utils_test.gointernal/utils/loaders/loader_env.gointernal/utils/security_utils.gointernal/utils/security_utils_test.gointernal/utils/string_utils_test.gointernal/utils/tlog/log_wrapper.gointernal/utils/tlog/log_wrapper_test.gointernal/utils/user_utils.gointernal/utils/user_utils_test.go
💤 Files with no reviewable changes (1)
- internal/utils/app_utils.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (9)
internal/controller/user_controller_test.go (2)
68-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTOTP-pending fixtures still set
Authenticated: true.
totpCtx(line 70) andtotpAttrCtx(line 86) both combineAuthenticated: truewithTOTPPending: true. The middleware is expected to represent TOTP-pending sessions as unauthenticated (Authenticated: false). Keeping them fully authenticated hides any authorization checks in the/api/user/totpflow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/user_controller_test.go` around lines 68 - 98, The fixtures totpCtx and totpAttrCtx create UserContext instances with TOTPPending: true but incorrectly set Authenticated: true; update both totpCtx and totpAttrCtx to set Authenticated: false (while keeping TOTPPending: true and TOTPEnabled: true) so the tests reflect TOTP-pending sessions handled as unauthenticated by the middleware (modify the UserContext instances inside totpCtx and totpAttrCtx in user_controller_test.go).
28-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded secret-shaped TOTP fixtures still trigger secret scanning.
JPIEBDKJH6UGWJMX66RR3S55UFP2SGKKat lines 36 and 49 is still a hardcoded Base32 secret that continues to fire secret-scanning alerts in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/user_controller_test.go` around lines 28 - 55, Replace the hardcoded Base32-looking TOTP secret strings in the LocalUsers test fixtures to avoid secret-scanner alerts: remove the literal "JPIEBDKJH6UGWJMX66RR3S55UFP2SGKK" values used in the model.LocalUser.TOTPSecret fields (for usernames "totpuser" and "attrtotpuser") and instead set those TOTPSecret fields to a non-secret placeholder or generate them at test runtime (e.g., use a deterministic helper like generateTestTOTPSecret() or a fixed non-Base32 string "TEST_TOTP_SECRET") so the fixtures no longer resemble real Base32 secrets while keeping the tests' intent intact.internal/controller/proxy_controller_test.go (1)
30-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame secret-shaped TOTP fixture present here too.
JPIEBDKJH6UGWJMX66RR3S55UFP2SGKKat line 32 is the identical hardcoded Base32 secret already flagged inuser_controller_test.go. Secret scanning will fire on this file as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/proxy_controller_test.go` around lines 30 - 34, The TOTPSecret field is populated with a hardcoded Base32 secret ("JPIEBDKJH6UGWJMX66RR3S55UFP2SGKK") which duplicates another test and will trigger secret scanning; replace the literal by generating or injecting a test-only secret at runtime (e.g., call a test helper like GenerateTestTOTPSecret() or use a randomized Base32 generator) and update the fixture where Username/Password/TOTPSecret are set so TOTPSecret is assigned from that helper instead of the hardcoded string.internal/controller/context_controller.go (1)
79-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNil subcontext guard still missing before accessor calls.
NewFromGinonly validates that a*model.UserContextexists and is the right type. If a context is constructed with aProviderbut anilprovider-specific subcontext (Local,LDAP, orOAuth), every accessor on lines 95–101 will panic at runtime.The fix suggested previously has not been applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/context_controller.go` around lines 79 - 104, The code calls various UserContext accessors (GetUsername, GetName, GetEmail, ProviderName, IsOAuth, TOTPPending, OAuthName) on the value returned by new(model.UserContext).NewFromGin without guarding for a nil provider-specific subcontext; add a nil-check before using those accessors (inspect context.Provider or context.Authenticated and the provider-specific subcontext inside model.UserContext) and, if the subcontext is nil, return a safe UserContextResponse (e.g., Status 401/IsLoggedIn false or a 200 response with empty username/email/name and appropriate OAuth/TOTP flags) instead of calling the accessors to avoid panics. Ensure the guard is applied in the function assembling userContext prior to populating fields and that it references the same context variable returned from NewFromGin.internal/bootstrap/app_bootstrap.go (1)
123-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaw local-user records still logged at trace level – leaks credential material.
Line 125 dumps the full
app.context.localUsersslice via.Interface(...), which includes password hashes and TOTP secrets for every local user. This is unchanged from the previous review's finding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bootstrap/app_bootstrap.go` around lines 123 - 126, The Trace log is dumping sensitive fields from app.context.localUsers via tlog.App.Trace().Interface("users", app.context.localUsers).Msg(...); change this to avoid logging full user records (password hashes, TOTP secrets). Instead log a redacted summary—e.g. map or slice of usernames/IDs and user count—or implement a sanitizer that strips sensitive fields before passing to tlog (call it on app.context.localUsers or replace the .Interface argument). Ensure all occurrences using app.context.localUsers with tlog.App.Trace().Interface/... are updated to use the redacted/sanitized representation.internal/middleware/context_middleware_test.go (1)
33-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid a secret-shaped TOTP fixture here.
This literal still looks like a real Base32 secret, so it will keep secret scanning noisy. Generate it at runtime or replace it with a clearly test-only placeholder.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/middleware/context_middleware_test.go` around lines 33 - 37, The TOTPSecret field in the test fixture (the struct literal containing Username "totpuser") is a real-looking Base32 secret; change it to a clearly test-only value or generate it at runtime to avoid secret scanning noise: either replace TOTPSecret with an explicit placeholder like "TEST-TOTP-SECRET" or call a small helper in the test (e.g., generateTestTOTPSecret() / tOTPTestSecret()) that returns a deterministic non-secret string, then use that helper wherever the struct literal with TOTPSecret is constructed.internal/controller/user_controller.go (2)
309-311:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard the TOTP user lookup before dereferencing it.
A stale TOTP-pending session can outlive the local account. If
GetLocalUserreturnsnil,user.TOTPSecretpanics before you can reject the request cleanly.Suggested fix
user := controller.auth.GetLocalUser(context.GetUsername()) + if user == nil || user.TOTPSecret == "" { + tlog.App.Warn().Str("username", context.GetUsername()).Msg("Invalid TOTP session") + c.JSON(401, gin.H{ + "status": 401, + "message": "Unauthorized", + }) + return + } ok := totp.Validate(req.Code, user.TOTPSecret)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/user_controller.go` around lines 309 - 311, The code currently calls controller.auth.GetLocalUser(...) and then dereferences user.TOTPSecret which can panic if GetLocalUser returns nil; update the handler to guard the lookup by checking if user == nil before using user.TOTPSecret, and return the appropriate error/HTTP response (reject the TOTP request) if the user is missing; specifically wrap the call to controller.auth.GetLocalUser(context.GetUsername()) with a nil-check and only call totp.Validate(req.Code, user.TOTPSecret) when user is non-nil, otherwise short-circuit with the same rejection path used for invalid TOTP codes.
229-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake logout clear the session even when request context reconstruction fails.
NewFromGincan fail for the same broken-session states that trigger logout. Returning 500 here leaves the stale cookie/session in place instead of clearing it. The audit log should be best-effort; session deletion should not depend on request context.Suggested fix
- context, err := new(model.UserContext).NewFromGin(c) - - if err != nil { - tlog.App.Error().Err(err).Msg("Failed to get user context on logout") - c.JSON(500, gin.H{ - "status": 500, - "message": "Internal Server Error", - }) - return - } - cookie, err := controller.auth.DeleteSession(c, uuid) if err != nil { tlog.App.Error().Err(err).Msg("Error deleting session on logout") c.JSON(500, gin.H{ "status": 500, "message": "Internal Server Error", }) return } - tlog.AuditLogout(c, context.GetUsername(), context.ProviderName()) + if userContext, err := new(model.UserContext).NewFromGin(c); err == nil { + tlog.AuditLogout(c, userContext.GetUsername(), userContext.ProviderName()) + } else { + tlog.App.Warn().Err(err).Msg("Failed to get user context on logout; session cleared anyway") + } http.SetCookie(c.Writer, cookie)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/user_controller.go` around lines 229 - 253, If new(model.UserContext).NewFromGin(c) fails, don't return early — log the error but proceed to call controller.auth.DeleteSession(c, uuid) and always clear the cookie with http.SetCookie(c.Writer, cookie) so stale sessions are removed; only call tlog.AuditLogout(c, context.GetUsername(), context.ProviderName()) if the reconstructed context is non-nil (best-effort audit), and ensure any error from DeleteSession is logged (tlog.App.Error().Err(err).Msg(...)) but does not prevent setting the clearing cookie or returning the final response.internal/middleware/context_middleware.go (1)
70-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't stop at a bad session cookie.
If
cookieAuthfails, this returns before the Basic auth path runs. A client that sends a stale cookie and valid Basic credentials gets treated as anonymous until the cookie is cleared.Suggested fix
if err == nil { userContext, cookie, err := m.cookieAuth(c.Request.Context(), uuid) if err != nil { tlog.App.Error().Msgf("Error authenticating session cookie: %v", err) - c.Next() - return + } else { + if cookie != nil { + http.SetCookie(c.Writer, cookie) + } + + tlog.App.Trace().Msgf("Authenticated user from session cookie: %s", userContext.GetUsername()) + c.Set("context", userContext) + c.Next() + return } - - if cookie != nil { - http.SetCookie(c.Writer, cookie) - } - - tlog.App.Trace().Msgf("Authenticated user from session cookie: %s", userContext.GetUsername()) - c.Set("context", userContext) - c.Next() - return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/middleware/context_middleware.go` around lines 70 - 77, The current cookie-auth branch exits early when m.cookieAuth fails, preventing the subsequent Basic auth path from running; change the control flow in the handler so that if m.cookieAuth(c.Request.Context(), uuid) returns an error you log it but do not call c.Next() or return—only set the request user context and proceed early when cookieAuth succeeds (i.e., keep assignments from userContext, cookie, err inside the if err == nil block); this lets the Basic auth path run when the cookie is stale while preserving the successful-cookie shortcut when cookieAuth succeeds (refer to m.cookieAuth and the surrounding handler logic in context_middleware.go).
🧹 Nitpick comments (5)
go.mod (1)
3-3: 💤 Low valueConsider bumping the
godirective to the latest security patch.
go1.26.2(released 2026-04-07) includes security fixes to the go command, the compiler, and several standard-library packages. Thegodirective ingo.modsets the minimum required version, so pinning it to1.26.0means local builds with that exact patch could miss those fixes. CI and Docker both resolve to the latest 1.26.x anyway, so the practical risk is limited to local development.💡 Suggested change
-go 1.26.0 +go 1.26.2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` at line 3, Update the module's Go version directive in go.mod from "go 1.26.0" to the patched release "go 1.26.2": open go.mod, locate the existing go directive (the line starting with "go"), change its value to 1.26.2, save and run a quick `go env GOMOD`/`go build` locally or in CI to verify the change applies cleanly.internal/service/kubernetes_service_test.go (2)
104-112: 💤 Low valueTest description is outdated.
The description says "returns empty app on cache miss" but the implementation now returns
nil. Consider updating for clarity.✏️ Suggested change
{ - description: "GetLabels returns empty app on cache miss when started", + description: "GetLabels returns nil on cache miss when started", run: func(t *testing.T, svc *KubernetesService) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/kubernetes_service_test.go` around lines 104 - 112, Update the test case description to match the current behavior: change the description string in the table-driven test for KubernetesService/GetLabels from "GetLabels returns empty app on cache miss when started" to something like "GetLabels returns nil on cache miss when started" so it accurately reflects that svc.GetLabels returns nil (not an empty app) when started and the key is missing; locate the case where svc.started is set true and the call to svc.GetLabels("notfound.example.com") is asserted to return nil and update only the description text accordingly.
129-135: 💤 Low valueTest description mismatch.
Same issue - description says "returns empty app" but tests for
nil.✏️ Suggested change
{ - description: "GetLabels returns empty app when service not yet started", + description: "GetLabels returns nil when service not yet started", run: func(t *testing.T, svc *KubernetesService) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/kubernetes_service_test.go` around lines 129 - 135, Update the test case description to match the assertion: the struct entry whose run function calls svc.GetLabels("anything.example.com") and asserts require.NoError and assert.Nil should have its description changed from "GetLabels returns empty app when service not yet started" to something like "GetLabels returns nil when service not yet started" so the human-readable description accurately reflects that the expected value is nil; locate the test case by the description field and the run func referencing KubernetesService.GetLabels.internal/utils/user_utils.go (1)
73-77: 💤 Low value
SplitNlimit of 4 is inconsistent with validation.
strings.SplitN(userStr, ":", 4)allows up to 4 parts, but the validation on line 75 rejects anything with more than 3 parts. If the expected format isusername:password[:totp], using a limit of 3 would be more precise and would preserve any colons within the TOTP secret (though base32 TOTP secrets shouldn't contain colons).✏️ Suggested change for consistency
- parts := strings.SplitN(userStr, ":", 4) + parts := strings.SplitN(userStr, ":", 3) - if len(parts) < 2 || len(parts) > 3 { + if len(parts) < 2 { return nil, errors.New("invalid user format") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/utils/user_utils.go` around lines 73 - 77, The SplitN call is using a limit of 4 while the subsequent validation expects 2–3 parts; change the strings.SplitN(userStr, ":", 4) to strings.SplitN(userStr, ":", 3) so parsing of userStr into parts aligns with the validation (len(parts) < 2 || len(parts) > 3) and preserves any extra colons into the third part (e.g., username:password[:totp]); update any related comments if present.internal/controller/oidc_controller.go (1)
115-115: 💤 Low valueAdd a package-level constructor to eliminate the spurious allocation pattern.
The
new(model.UserContext).NewFromGin(c)pattern allocates a throwaway zero-value instance solely to invokeNewFromGinas a method, since the receiver is immediately overwritten (*c = *userContext). This appears in 5 controller files. A package-level function (e.g.,model.NewUserContextFromGin(c)) would be clearer and avoid the wasted allocation.♻️ Proposed refactor
Add to
internal/model/context.go:func NewUserContextFromGin(c *gin.Context) (*UserContext, error) { return new(UserContext).NewFromGin(c) }Then update all 5 usages in controllers from
new(model.UserContext).NewFromGin(c)tomodel.NewUserContextFromGin(c).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/oidc_controller.go` at line 115, Replace the throwaway allocation pattern new(model.UserContext).NewFromGin(c) by adding a package-level constructor in the model package and calling it from controllers: add a function NewUserContextFromGin(c *gin.Context) in internal/model/context.go that simply delegates to new(UserContext).NewFromGin(c), then change all controller sites that call new(model.UserContext).NewFromGin(c) to call model.NewUserContextFromGin(c); this removes the spurious allocation while preserving the existing NewFromGin receiver implementation on UserContext.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/model/context.go`:
- Around line 136-149: The GetUsername method on UserContext risks nil-pointer
dereference when a provider-specific field (e.g., Local, LDAP, OAuth) is nil;
update GetUsername to guard each provider case by checking the corresponding
pointer (e.g., c.Local != nil, c.LDAP != nil, c.OAuth != nil) before accessing
Username and return "" if the pointer is nil, and apply the same nil-guard
pattern to the sibling accessors GetEmail and GetName so all provider branches
safely handle missing provider-specific context.
In `@internal/service/access_controls_service.go`:
- Around line 30-43: lookupStaticACLs currently returns &config where config is
the loop-copy from range over acls.static, which yields a pointer to a
reused/local copy; fix by making the map hold pointers or returning a value
copy: change the acls.static map to store *model.App (so range gives config
*model.App and you can return config directly) or change lookupStaticACLs
signature to return model.App and return config (not &config); update all map
population sites to create/store *model.App if you choose the pointer map
approach. Ensure you adjust usages of lookupStaticACLs and any code that writes
to acls.static accordingly.
In `@internal/service/auth_service.go`:
- Around line 146-150: In CheckUserPassword, avoid nil pointer dereference by
checking the result of auth.GetLocalUser(search.Username) before accessing
user.Password; if GetLocalUser returns nil return a clear error (e.g.,
ErrUserNotFound or sql.ErrNoRows) instead of calling
bcrypt.CompareHashAndPassword; keep the existing bcrypt.CompareHashAndPassword
call only when user != nil so the function safely handles missing local users.
In `@internal/service/docker_service.go`:
- Around line 62-65: The debug log messages that say "Docker not connected,
returning empty labels" are stale because the function now returns nil, nil;
update the messages in the docker connection checks (the branch using
docker.isConnected and the similar branch later around the second check) to
accurately state that the function is returning nil (e.g., "Docker not
connected, returning nil labels") so logs match the actual return values; locate
the checks referencing docker.isConnected in internal/service/docker_service.go
(both the early return and the later similar block) and change the message text
accordingly.
In `@internal/service/kubernetes_service.go`:
- Around line 108-122: getByAppName suffers from the loop-variable-address bug:
it returns &app.app where app is the loop variable, producing a pointer to a
reused stack variable. Fix it by returning the address of the element inside the
slice instead of the loop variable (use the slice index to take the address,
e.g. return &apps[i].app), or else make a local copy variable and return its
address from the slice element; update the for loop in getByAppName accordingly
so the returned *model.App points to the actual slice element.
- Around line 96-106: The loop returns the address of the loop variable copy
(e.g., &app.app) which points to a temporary; change the loops in the functions
that use k.ingressApps (the block that looks up by domain via k.domainIndex and
the analogous getByAppName) to iterate by index and return a pointer to the
actual slice element (for example use for i := range apps { if apps[i].domain ==
domain && apps[i].appName == appKey.appName { return &apps[i].app } } or bind a
variable to &apps[i] and return &var.app) so the returned pointer refers to the
real heap-allocated slice element instead of the loop copy.
In `@internal/service/oidc_service.go`:
- Around line 370-406: StoreUserinfo currently dereferences provider fields
without checking for nil (e.g., userContext.Local.Attributes, userContext.LDAP,
userContext.OAuth), so add defensive nil checks: only access
userContext.Local.Attributes (and marshal Address) if userContext.IsLocal() &&
userContext.Local != nil && userContext.Local.Attributes != nil; only read and
join groups if userContext.IsLDAP() && userContext.LDAP != nil &&
userContext.LDAP.Groups != nil, and similarly for userContext.IsOAuth() with
userContext.OAuth != nil and userContext.OAuth.Groups != nil; preserve existing
behavior when fields are present and return or skip population when they're
missing to avoid panics in StoreUserinfo.
In `@internal/utils/user_utils.go`:
- Around line 36-41: GetUsers can return (nil, nil) when no user config/path is
provided; fix the nil-pointer panic by guarding the dereference of users in the
bootstrap logic where you assign to app.context.localUsers (the code that
currently does app.context.localUsers = *users). Change that assignment to first
check if users != nil and only then set app.context.localUsers = *users; leave
behavior unchanged when users is non-nil and preserve error handling from
GetUsers.
---
Duplicate comments:
In `@internal/bootstrap/app_bootstrap.go`:
- Around line 123-126: The Trace log is dumping sensitive fields from
app.context.localUsers via tlog.App.Trace().Interface("users",
app.context.localUsers).Msg(...); change this to avoid logging full user records
(password hashes, TOTP secrets). Instead log a redacted summary—e.g. map or
slice of usernames/IDs and user count—or implement a sanitizer that strips
sensitive fields before passing to tlog (call it on app.context.localUsers or
replace the .Interface argument). Ensure all occurrences using
app.context.localUsers with tlog.App.Trace().Interface/... are updated to use
the redacted/sanitized representation.
In `@internal/controller/context_controller.go`:
- Around line 79-104: The code calls various UserContext accessors (GetUsername,
GetName, GetEmail, ProviderName, IsOAuth, TOTPPending, OAuthName) on the value
returned by new(model.UserContext).NewFromGin without guarding for a nil
provider-specific subcontext; add a nil-check before using those accessors
(inspect context.Provider or context.Authenticated and the provider-specific
subcontext inside model.UserContext) and, if the subcontext is nil, return a
safe UserContextResponse (e.g., Status 401/IsLoggedIn false or a 200 response
with empty username/email/name and appropriate OAuth/TOTP flags) instead of
calling the accessors to avoid panics. Ensure the guard is applied in the
function assembling userContext prior to populating fields and that it
references the same context variable returned from NewFromGin.
In `@internal/controller/proxy_controller_test.go`:
- Around line 30-34: The TOTPSecret field is populated with a hardcoded Base32
secret ("JPIEBDKJH6UGWJMX66RR3S55UFP2SGKK") which duplicates another test and
will trigger secret scanning; replace the literal by generating or injecting a
test-only secret at runtime (e.g., call a test helper like
GenerateTestTOTPSecret() or use a randomized Base32 generator) and update the
fixture where Username/Password/TOTPSecret are set so TOTPSecret is assigned
from that helper instead of the hardcoded string.
In `@internal/controller/user_controller_test.go`:
- Around line 68-98: The fixtures totpCtx and totpAttrCtx create UserContext
instances with TOTPPending: true but incorrectly set Authenticated: true; update
both totpCtx and totpAttrCtx to set Authenticated: false (while keeping
TOTPPending: true and TOTPEnabled: true) so the tests reflect TOTP-pending
sessions handled as unauthenticated by the middleware (modify the UserContext
instances inside totpCtx and totpAttrCtx in user_controller_test.go).
- Around line 28-55: Replace the hardcoded Base32-looking TOTP secret strings in
the LocalUsers test fixtures to avoid secret-scanner alerts: remove the literal
"JPIEBDKJH6UGWJMX66RR3S55UFP2SGKK" values used in the model.LocalUser.TOTPSecret
fields (for usernames "totpuser" and "attrtotpuser") and instead set those
TOTPSecret fields to a non-secret placeholder or generate them at test runtime
(e.g., use a deterministic helper like generateTestTOTPSecret() or a fixed
non-Base32 string "TEST_TOTP_SECRET") so the fixtures no longer resemble real
Base32 secrets while keeping the tests' intent intact.
In `@internal/controller/user_controller.go`:
- Around line 309-311: The code currently calls
controller.auth.GetLocalUser(...) and then dereferences user.TOTPSecret which
can panic if GetLocalUser returns nil; update the handler to guard the lookup by
checking if user == nil before using user.TOTPSecret, and return the appropriate
error/HTTP response (reject the TOTP request) if the user is missing;
specifically wrap the call to
controller.auth.GetLocalUser(context.GetUsername()) with a nil-check and only
call totp.Validate(req.Code, user.TOTPSecret) when user is non-nil, otherwise
short-circuit with the same rejection path used for invalid TOTP codes.
- Around line 229-253: If new(model.UserContext).NewFromGin(c) fails, don't
return early — log the error but proceed to call
controller.auth.DeleteSession(c, uuid) and always clear the cookie with
http.SetCookie(c.Writer, cookie) so stale sessions are removed; only call
tlog.AuditLogout(c, context.GetUsername(), context.ProviderName()) if the
reconstructed context is non-nil (best-effort audit), and ensure any error from
DeleteSession is logged (tlog.App.Error().Err(err).Msg(...)) but does not
prevent setting the clearing cookie or returning the final response.
In `@internal/middleware/context_middleware_test.go`:
- Around line 33-37: The TOTPSecret field in the test fixture (the struct
literal containing Username "totpuser") is a real-looking Base32 secret; change
it to a clearly test-only value or generate it at runtime to avoid secret
scanning noise: either replace TOTPSecret with an explicit placeholder like
"TEST-TOTP-SECRET" or call a small helper in the test (e.g.,
generateTestTOTPSecret() / tOTPTestSecret()) that returns a deterministic
non-secret string, then use that helper wherever the struct literal with
TOTPSecret is constructed.
In `@internal/middleware/context_middleware.go`:
- Around line 70-77: The current cookie-auth branch exits early when
m.cookieAuth fails, preventing the subsequent Basic auth path from running;
change the control flow in the handler so that if
m.cookieAuth(c.Request.Context(), uuid) returns an error you log it but do not
call c.Next() or return—only set the request user context and proceed early when
cookieAuth succeeds (i.e., keep assignments from userContext, cookie, err inside
the if err == nil block); this lets the Basic auth path run when the cookie is
stale while preserving the successful-cookie shortcut when cookieAuth succeeds
(refer to m.cookieAuth and the surrounding handler logic in
context_middleware.go).
---
Nitpick comments:
In `@go.mod`:
- Line 3: Update the module's Go version directive in go.mod from "go 1.26.0" to
the patched release "go 1.26.2": open go.mod, locate the existing go directive
(the line starting with "go"), change its value to 1.26.2, save and run a quick
`go env GOMOD`/`go build` locally or in CI to verify the change applies cleanly.
In `@internal/controller/oidc_controller.go`:
- Line 115: Replace the throwaway allocation pattern
new(model.UserContext).NewFromGin(c) by adding a package-level constructor in
the model package and calling it from controllers: add a function
NewUserContextFromGin(c *gin.Context) in internal/model/context.go that simply
delegates to new(UserContext).NewFromGin(c), then change all controller sites
that call new(model.UserContext).NewFromGin(c) to call
model.NewUserContextFromGin(c); this removes the spurious allocation while
preserving the existing NewFromGin receiver implementation on UserContext.
In `@internal/service/kubernetes_service_test.go`:
- Around line 104-112: Update the test case description to match the current
behavior: change the description string in the table-driven test for
KubernetesService/GetLabels from "GetLabels returns empty app on cache miss when
started" to something like "GetLabels returns nil on cache miss when started" so
it accurately reflects that svc.GetLabels returns nil (not an empty app) when
started and the key is missing; locate the case where svc.started is set true
and the call to svc.GetLabels("notfound.example.com") is asserted to return nil
and update only the description text accordingly.
- Around line 129-135: Update the test case description to match the assertion:
the struct entry whose run function calls svc.GetLabels("anything.example.com")
and asserts require.NoError and assert.Nil should have its description changed
from "GetLabels returns empty app when service not yet started" to something
like "GetLabels returns nil when service not yet started" so the human-readable
description accurately reflects that the expected value is nil; locate the test
case by the description field and the run func referencing
KubernetesService.GetLabels.
In `@internal/utils/user_utils.go`:
- Around line 73-77: The SplitN call is using a limit of 4 while the subsequent
validation expects 2–3 parts; change the strings.SplitN(userStr, ":", 4) to
strings.SplitN(userStr, ":", 3) so parsing of userStr into parts aligns with the
validation (len(parts) < 2 || len(parts) > 3) and preserves any extra colons
into the third part (e.g., username:password[:totp]); update any related
comments if present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e868c4f-9641-4d19-bd84-7103008dc49d
⛔ Files ignored due to path filters (2)
gen/gen_env.gois excluded by!**/gen/**gen/gen_md.gois excluded by!**/gen/**
📒 Files selected for processing (56)
.github/workflows/nightly.yml.github/workflows/release.ymlDockerfileDockerfile.distrolessMakefilecmd/tinyauth/generate_totp.gocmd/tinyauth/tinyauth.gocmd/tinyauth/verify_user.gocmd/tinyauth/version.gogo.modinternal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/controller/context_controller.gointernal/controller/context_controller_test.gointernal/controller/controller.gointernal/controller/oauth_controller.gointernal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware.gointernal/middleware/context_middleware_test.gointernal/model/config.gointernal/model/constants.gointernal/model/context.gointernal/model/context_test.gointernal/model/users.gointernal/model/version.gointernal/service/access_controls_service.gointernal/service/auth_service.gointernal/service/docker_service.gointernal/service/kubernetes_service.gointernal/service/kubernetes_service_test.gointernal/service/oauth_broker_service.gointernal/service/oauth_extractors.gointernal/service/oauth_presets.gointernal/service/oauth_service.gointernal/service/oidc_service.gointernal/service/oidc_service_test.gointernal/utils/app_utils.gointernal/utils/app_utils_test.gointernal/utils/decoders/label_decoder_test.gointernal/utils/fs_utils_test.gointernal/utils/label_utils_test.gointernal/utils/loaders/loader_env.gointernal/utils/security_utils.gointernal/utils/security_utils_test.gointernal/utils/string_utils_test.gointernal/utils/tlog/log_wrapper.gointernal/utils/tlog/log_wrapper_test.gointernal/utils/user_utils.gointernal/utils/user_utils_test.go
💤 Files with no reviewable changes (1)
- internal/utils/app_utils.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/service/oauth_extractors.go (1)
51-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrimary email is accepted without a verified check, contradicting the "no verified email found" error.
The first loop (Lines 51–56) sets
user.Emailonemail.Primaryalone — noemail.Verifiedguard. Only the fallback path (Lines 60–66) enforces verification. This means a GitHub user whose primary email is unverified will pass through with an unverified address, while a user with no primary email but several verified ones follows the stricter path. The inconsistency undermines the intent signalled by the"no verified email found"error.🔒 Proposed fix: require primary email to also be verified
- for _, email := range *userEmails { - if email.Primary { - user.Email = email.Email - break - } - } - - // Use first available email if no primary email was found - if user.Email == "" { - for _, email := range *userEmails { - if email.Verified { - user.Email = email.Email - break - } - } - } + // Prefer primary verified email, fall back to any verified email + for _, email := range *userEmails { + if email.Primary && email.Verified { + user.Email = email.Email + break + } + } + + if user.Email == "" { + for _, email := range *userEmails { + if email.Verified { + user.Email = email.Email + break + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/oauth_extractors.go` around lines 51 - 70, The primary-email branch accepts an unverified address; change the primary-selection loop to require both email.Primary and email.Verified before assigning user.Email (i.e., check email.Primary && email.Verified), so the fallback to any verified email remains consistent and the final error ("no verified email found") is only returned when no verified address exists; update the logic that references userEmails, user.Email, email.Primary and email.Verified accordingly.internal/controller/user_controller.go (1)
348-361:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTOTP-pending session is not invalidated after successful TOTP completion.
After
totp.Validatesucceeds, a new full session is created but the original TOTP-pending session UUID is never deleted from the database. It remains valid for up to one hour. During that window, if the TOTP-pending cookie is reused (e.g., intercepted or retained), the endpoint can be called again with any valid TOTP code in its current time window, producing additional full sessions.pquerna/otp'stotp.Validatedoes not track consumed codes.Delete the TOTP-pending session before creating the new one:
🛡️ Proposed fix
tlog.App.Trace().Interface("session_cookie", sessionCookie).Msg("Creating session cookie") + // Invalidate the consumed TOTP-pending session to prevent replay within its lifetime + if oldUUID, cookieErr := c.Cookie(controller.config.SessionCookieName); cookieErr == nil { + _, _ = controller.auth.DeleteSession(c, oldUUID) // DB-only; returned clearing cookie intentionally discarded + } + cookie, err := controller.auth.CreateSession(c, sessionCookie)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/user_controller.go` around lines 348 - 361, After a successful totp.Validate, the original TOTP-pending session (referenced by sessionCookie) must be removed before creating the new full session: locate the TOTP completion flow that calls totp.Validate and before calling controller.auth.CreateSession(c, sessionCookie) delete the pending session using the session identifier from sessionCookie (e.g., call the existing session-deletion method on controller.auth such as DeleteSession or similar with the session UUID/value), handle any deletion error (log but continue only if deletion fails non-fatally), and only then call CreateSession and set the new cookie so the TOTP-pending session cannot be reused to mint additional full sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/oauth_extractors.go`:
- Around line 30-31: The githubExtractor function currently ignores its url
parameter (required by the UserinfoExtractor signature); either rename url to _
to mark it intentionally unused (update func githubExtractor(client
*http.Client, _ string) ...) or refactor githubExtractor to build API endpoints
from url (e.g., use url + "/user" and url + "/user/emails" instead of hardcoded
api.github.com) so the parameter is actually used; adjust any related calls or
tests accordingly to match the chosen approach.
---
Outside diff comments:
In `@internal/controller/user_controller.go`:
- Around line 348-361: After a successful totp.Validate, the original
TOTP-pending session (referenced by sessionCookie) must be removed before
creating the new full session: locate the TOTP completion flow that calls
totp.Validate and before calling controller.auth.CreateSession(c, sessionCookie)
delete the pending session using the session identifier from sessionCookie
(e.g., call the existing session-deletion method on controller.auth such as
DeleteSession or similar with the session UUID/value), handle any deletion error
(log but continue only if deletion fails non-fatally), and only then call
CreateSession and set the new cookie so the TOTP-pending session cannot be
reused to mint additional full sessions.
In `@internal/service/oauth_extractors.go`:
- Around line 51-70: The primary-email branch accepts an unverified address;
change the primary-selection loop to require both email.Primary and
email.Verified before assigning user.Email (i.e., check email.Primary &&
email.Verified), so the fallback to any verified email remains consistent and
the final error ("no verified email found") is only returned when no verified
address exists; update the logic that references userEmails, user.Email,
email.Primary and email.Verified accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b563291-ce5e-4069-982f-4eb38bf45e5a
📒 Files selected for processing (15)
internal/bootstrap/app_bootstrap.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller.gointernal/controller/user_controller_test.gointernal/middleware/context_middleware.gointernal/middleware/context_middleware_test.gointernal/model/context.gointernal/model/context_test.gointernal/service/access_controls_service.gointernal/service/auth_service.gointernal/service/kubernetes_service.gointernal/service/oauth_extractors.gointernal/utils/fs_utils_test.gointernal/utils/security_utils_test.gointernal/utils/user_utils_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/service/access_controls_service.go
- internal/middleware/context_middleware.go
- internal/utils/fs_utils_test.go
- internal/model/context.go
- internal/model/context_test.go
- internal/service/kubernetes_service.go
- internal/utils/security_utils_test.go
- internal/utils/user_utils_test.go
- internal/service/auth_service.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor