refactor: rework user context handling throughout tinyauth#829
Open
steveiliop56 wants to merge 21 commits intomainfrom
Open
refactor: rework user context handling throughout tinyauth#829steveiliop56 wants to merge 21 commits intomainfrom
steveiliop56 wants to merge 21 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR moves many public/internal types and constants from internal/config into a new internal/model package, renames fields (e.g., Ldap→LDAP, TotpSecret→TOTPSecret), introduces typed user/context models and model.Claims/version vars, refactors session/auth to DB-backed context flows, and updates all callers (controllers, middleware, services, tests, CLI, build metadata). ChangesModel & Core Refactor (single DAG)
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Browser
participant MW as Middleware<br/>(Context)
participant Auth as Auth Service
participant DB as Database<br/>(Session)
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
Comment on lines
123
to
126
| // Dumps | ||
| tlog.App.Trace().Interface("config", app.config).Msg("Config dump") | ||
| tlog.App.Trace().Interface("users", app.context.users).Msg("Users dump") | ||
| tlog.App.Trace().Interface("users", app.context.localUsers).Msg("Users dump") | ||
| tlog.App.Trace().Interface("oauthProviders", app.context.oauthProviders).Msg("OAuth providers dump") |
Contributor
There was a problem hiding this comment.
Don't trace raw local user records.
app.context.localUsers contains credential material, so this dump will leak password hashes and TOTP secrets into logs. Please log counts or usernames only here.
Suggested change
tlog.App.Trace().Interface("config", app.config).Msg("Config dump")
- tlog.App.Trace().Interface("users", app.context.localUsers).Msg("Users dump")
+ tlog.App.Trace().Int("userCount", len(app.context.localUsers)).Msg("Loaded local users")
tlog.App.Trace().Interface("oauthProviders", app.context.oauthProviders).Msg("OAuth providers dump")📝 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.
Suggested change
| // Dumps | |
| tlog.App.Trace().Interface("config", app.config).Msg("Config dump") | |
| tlog.App.Trace().Interface("users", app.context.users).Msg("Users dump") | |
| tlog.App.Trace().Interface("users", app.context.localUsers).Msg("Users dump") | |
| tlog.App.Trace().Interface("oauthProviders", app.context.oauthProviders).Msg("OAuth providers dump") | |
| // Dumps | |
| tlog.App.Trace().Interface("config", app.config).Msg("Config dump") | |
| tlog.App.Trace().Int("userCount", len(app.context.localUsers)).Msg("Loaded local users") | |
| tlog.App.Trace().Interface("oauthProviders", app.context.oauthProviders).Msg("OAuth providers dump") |
🤖 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 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.
Member
Author
There was a problem hiding this comment.
Dump is on purpose to help debug Tinyauth.
Contributor
There was a problem hiding this comment.
@steveiliop56, understood — if it's an intentional debug aid, that's your call. Just worth keeping in mind that TRACE-level logging is sometimes accidentally enabled in production environments (e.g. via a misconfigured env var), which would expose password hashes and TOTP secrets in log output. If you'd like a middle-ground in the future, logging usernames only (no credentials) would still give plenty of debugging signal while reducing that risk. No changes needed from your side!
Comment on lines
101
to
104
| userController := controller.NewUserController(controller.UserControllerConfig{ | ||
| CookieDomain: app.context.cookieDomain, | ||
| CookieDomain: app.context.cookieDomain, | ||
| SessionCookieName: app.context.sessionCookieName, | ||
| }, apiRouter, app.services.authService) |
Contributor
There was a problem hiding this comment.
SessionCookieName is incorrectly used as a redirect URL in UserController.logoutHandler.
The context snippet from internal/controller/user_controller.go:229-235 shows:
if err != nil {
c.Redirect(http.StatusFound, controller.config.SessionCookieName)
return
}controller.config.SessionCookieName holds a cookie identifier string (e.g., "tinyauth_session"), not a URL. Passing it as gin's redirect location sets the Location header to that literal string, producing a broken redirect on every unauthenticated logout request.
It should redirect to a real path (e.g., "/" or "/login"):
- c.Redirect(http.StatusFound, controller.config.SessionCookieName)
+ c.Redirect(http.StatusFound, "/")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/bootstrap/router_bootstrap.go` around lines 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.
Comment on lines
+79
to
+101
| context, err := new(model.UserContext).NewFromGin(c) | ||
|
|
||
| if err != nil { | ||
| tlog.App.Debug().Err(err).Msg("No user context found in request") | ||
| userContext.Status = 401 | ||
| userContext.Message = "Unauthorized" | ||
| userContext.IsLoggedIn = false | ||
| c.JSON(200, userContext) | ||
| c.JSON(200, UserContextResponse{ | ||
| Status: 401, | ||
| Message: "Unauthorized", | ||
| IsLoggedIn: false, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| userContext := UserContextResponse{ | ||
| Status: 200, | ||
| Message: "Success", | ||
| IsLoggedIn: context.Authenticated, | ||
| Username: context.GetUsername(), | ||
| Name: context.GetName(), | ||
| Email: context.GetEmail(), | ||
| Provider: context.ProviderName(), | ||
| OAuth: context.IsOAuth(), | ||
| TOTPPending: context.TOTPPending(), | ||
| OAuthName: context.OAuthName(), |
Contributor
There was a problem hiding this comment.
Validate the provider-specific subcontext before calling these accessors.
NewFromGin() only proves that "context" exists and is a *model.UserContext. The new accessor calls then dereference Local/LDAP/OAuth based on Provider, so a partially populated context will panic this endpoint instead of returning unauthorized.
Suggested fix
context, err := new(model.UserContext).NewFromGin(c)
if err != nil {
tlog.App.Debug().Err(err).Msg("No user context found in request")
c.JSON(200, UserContextResponse{
Status: 401,
Message: "Unauthorized",
IsLoggedIn: false,
})
return
}
+
+ switch context.Provider {
+ case model.ProviderLocal, model.ProviderBasicAuth:
+ if context.Local == nil {
+ c.JSON(200, UserContextResponse{Status: 401, Message: "Unauthorized", IsLoggedIn: false})
+ return
+ }
+ case model.ProviderLDAP:
+ if context.LDAP == nil {
+ c.JSON(200, UserContextResponse{Status: 401, Message: "Unauthorized", IsLoggedIn: false})
+ return
+ }
+ case model.ProviderOAuth:
+ if context.OAuth == nil {
+ c.JSON(200, UserContextResponse{Status: 401, Message: "Unauthorized", IsLoggedIn: false})
+ return
+ }
+ }📝 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.
Suggested change
| context, err := new(model.UserContext).NewFromGin(c) | |
| if err != nil { | |
| tlog.App.Debug().Err(err).Msg("No user context found in request") | |
| userContext.Status = 401 | |
| userContext.Message = "Unauthorized" | |
| userContext.IsLoggedIn = false | |
| c.JSON(200, userContext) | |
| c.JSON(200, UserContextResponse{ | |
| Status: 401, | |
| Message: "Unauthorized", | |
| IsLoggedIn: false, | |
| }) | |
| return | |
| } | |
| userContext := UserContextResponse{ | |
| Status: 200, | |
| Message: "Success", | |
| IsLoggedIn: context.Authenticated, | |
| Username: context.GetUsername(), | |
| Name: context.GetName(), | |
| Email: context.GetEmail(), | |
| Provider: context.ProviderName(), | |
| OAuth: context.IsOAuth(), | |
| TOTPPending: context.TOTPPending(), | |
| OAuthName: context.OAuthName(), | |
| context, err := new(model.UserContext).NewFromGin(c) | |
| if err != nil { | |
| tlog.App.Debug().Err(err).Msg("No user context found in request") | |
| c.JSON(200, UserContextResponse{ | |
| Status: 401, | |
| Message: "Unauthorized", | |
| IsLoggedIn: false, | |
| }) | |
| return | |
| } | |
| switch context.Provider { | |
| case model.ProviderLocal, model.ProviderBasicAuth: | |
| if context.Local == nil { | |
| c.JSON(200, UserContextResponse{Status: 401, Message: "Unauthorized", IsLoggedIn: false}) | |
| return | |
| } | |
| case model.ProviderLDAP: | |
| if context.LDAP == nil { | |
| c.JSON(200, UserContextResponse{Status: 401, Message: "Unauthorized", IsLoggedIn: false}) | |
| return | |
| } | |
| case model.ProviderOAuth: | |
| if context.OAuth == nil { | |
| c.JSON(200, UserContextResponse{Status: 401, Message: "Unauthorized", IsLoggedIn: false}) | |
| return | |
| } | |
| } | |
| userContext := UserContextResponse{ | |
| Status: 200, | |
| Message: "Success", | |
| IsLoggedIn: context.Authenticated, | |
| Username: context.GetUsername(), | |
| Name: context.GetName(), | |
| Email: context.GetEmail(), | |
| Provider: context.ProviderName(), | |
| OAuth: context.IsOAuth(), | |
| TOTPPending: context.TOTPPending(), | |
| OAuthName: context.OAuthName(), |
🤖 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 - 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.
Comment on lines
+28
to
+50
| LocalUsers: []model.LocalUser{ | ||
| { | ||
| Username: "testuser", | ||
| Password: "$2a$10$ZwVYQH07JX2zq7Fjkt3gU.BjwvvwPeli4OqOno04RQIv0P7usBrXa", // password | ||
| }, | ||
| { | ||
| Username: "totpuser", | ||
| Password: "$2a$10$ZwVYQH07JX2zq7Fjkt3gU.BjwvvwPeli4OqOno04RQIv0P7usBrXa", // password | ||
| TotpSecret: "JPIEBDKJH6UGWJMX66RR3S55UFP2SGKK", | ||
| TOTPSecret: "JPIEBDKJH6UGWJMX66RR3S55UFP2SGKK", | ||
| }, | ||
| { | ||
| Username: "attruser", | ||
| Password: "$2a$10$ZwVYQH07JX2zq7Fjkt3gU.BjwvvwPeli4OqOno04RQIv0P7usBrXa", // password | ||
| Attributes: config.UserAttributes{ | ||
| Attributes: model.UserAttributes{ | ||
| Name: "Alice Smith", | ||
| Email: "alice@example.com", | ||
| }, | ||
| }, | ||
| { | ||
| Username: "attrtotpuser", | ||
| Password: "$2a$10$ZwVYQH07JX2zq7Fjkt3gU.BjwvvwPeli4OqOno04RQIv0P7usBrXa", // password | ||
| TotpSecret: "JPIEBDKJH6UGWJMX66RR3S55UFP2SGKK", | ||
| Attributes: config.UserAttributes{ | ||
| TOTPSecret: "JPIEBDKJH6UGWJMX66RR3S55UFP2SGKK", | ||
| Attributes: model.UserAttributes{ |
Contributor
There was a problem hiding this comment.
Avoid secret-shaped TOTP fixtures here too.
These inline Base32 secrets are test-only, but secret scanning is already flagging them. Please generate them in-test or explicitly allowlist the fixture so CI stays clean.
🧰 Tools
🪛 Betterleaks (1.1.2)
[high] 36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 49-49: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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 - 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.
Comment on lines
+68
to
+98
| totpCtx := func(c *gin.Context) { | ||
| c.Set("context", &model.UserContext{ | ||
| Authenticated: true, | ||
| Provider: model.ProviderLocal, | ||
| Local: &model.LocalContext{ | ||
| BaseContext: model.BaseContext{ | ||
| Username: "totpuser", | ||
| Name: "Totpuser", | ||
| Email: "totpuser@example.com", | ||
| }, | ||
| TOTPPending: true, | ||
| TOTPEnabled: true, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| totpAttrCtx := func(c *gin.Context) { | ||
| c.Set("context", &model.UserContext{ | ||
| Authenticated: true, | ||
| Provider: model.ProviderLocal, | ||
| Local: &model.LocalContext{ | ||
| BaseContext: model.BaseContext{ | ||
| Username: "attrtotpuser", | ||
| Name: "Bob Jones", | ||
| Email: "bob@example.com", | ||
| }, | ||
| TOTPPending: true, | ||
| TOTPEnabled: true, | ||
| }, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Make the TOTP-pending fixtures unauthenticated.
These helpers set Authenticated: true, but the middleware now represents TOTP-pending sessions as unauthenticated with TOTPPending: true. Keeping the fixtures “fully authenticated” can hide authorization bugs in the /api/user/totp flow.
Suggested fix
totpCtx := func(c *gin.Context) {
c.Set("context", &model.UserContext{
- Authenticated: true,
+ Authenticated: false,
Provider: model.ProviderLocal,
Local: &model.LocalContext{
BaseContext: model.BaseContext{
Username: "totpuser",
@@
totpAttrCtx := func(c *gin.Context) {
c.Set("context", &model.UserContext{
- Authenticated: true,
+ Authenticated: false,
Provider: model.ProviderLocal,
Local: &model.LocalContext{
BaseContext: model.BaseContext{
Username: "attrtotpuser",📝 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.
Suggested change
| totpCtx := func(c *gin.Context) { | |
| c.Set("context", &model.UserContext{ | |
| Authenticated: true, | |
| Provider: model.ProviderLocal, | |
| Local: &model.LocalContext{ | |
| BaseContext: model.BaseContext{ | |
| Username: "totpuser", | |
| Name: "Totpuser", | |
| Email: "totpuser@example.com", | |
| }, | |
| TOTPPending: true, | |
| TOTPEnabled: true, | |
| }, | |
| }) | |
| } | |
| totpAttrCtx := func(c *gin.Context) { | |
| c.Set("context", &model.UserContext{ | |
| Authenticated: true, | |
| Provider: model.ProviderLocal, | |
| Local: &model.LocalContext{ | |
| BaseContext: model.BaseContext{ | |
| Username: "attrtotpuser", | |
| Name: "Bob Jones", | |
| Email: "bob@example.com", | |
| }, | |
| TOTPPending: true, | |
| TOTPEnabled: true, | |
| }, | |
| }) | |
| } | |
| totpCtx := func(c *gin.Context) { | |
| c.Set("context", &model.UserContext{ | |
| Authenticated: false, | |
| Provider: model.ProviderLocal, | |
| Local: &model.LocalContext{ | |
| BaseContext: model.BaseContext{ | |
| Username: "totpuser", | |
| Name: "Totpuser", | |
| Email: "totpuser@example.com", | |
| }, | |
| TOTPPending: true, | |
| TOTPEnabled: true, | |
| }, | |
| }) | |
| } | |
| totpAttrCtx := func(c *gin.Context) { | |
| c.Set("context", &model.UserContext{ | |
| Authenticated: false, | |
| Provider: model.ProviderLocal, | |
| Local: &model.LocalContext{ | |
| BaseContext: model.BaseContext{ | |
| Username: "attrtotpuser", | |
| Name: "Bob Jones", | |
| Email: "bob@example.com", | |
| }, | |
| TOTPPending: true, | |
| TOTPEnabled: true, | |
| }, | |
| }) | |
| } |
🤖 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 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.
Comment on lines
+116
to
+123
| c.OAuth = &OAuthContext{ | ||
| BaseContext: BaseContext{ | ||
| Username: session.Username, | ||
| Name: session.Name, | ||
| Email: session.Email, | ||
| }, | ||
| Groups: strings.Split(session.OAuthGroups, ","), | ||
| Sub: session.OAuthSub, |
Contributor
There was a problem hiding this comment.
Don't materialize []string{""} for users with no OAuth groups.
strings.Split(session.OAuthGroups, ",") turns an empty column into one empty group. That changes "no groups" into a non-empty slice and can skew downstream ACL checks.
Proposed fix
- Groups: strings.Split(session.OAuthGroups, ","),
+ Groups: func() []string {
+ if session.OAuthGroups == "" {
+ return nil
+ }
+ return strings.Split(session.OAuthGroups, ",")
+ }(),📝 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.
Suggested change
| c.OAuth = &OAuthContext{ | |
| BaseContext: BaseContext{ | |
| Username: session.Username, | |
| Name: session.Name, | |
| Email: session.Email, | |
| }, | |
| Groups: strings.Split(session.OAuthGroups, ","), | |
| Sub: session.OAuthSub, | |
| c.OAuth = &OAuthContext{ | |
| BaseContext: BaseContext{ | |
| Username: session.Username, | |
| Name: session.Name, | |
| Email: session.Email, | |
| }, | |
| Groups: func() []string { | |
| if session.OAuthGroups == "" { | |
| return nil | |
| } | |
| return strings.Split(session.OAuthGroups, ",") | |
| }(), | |
| Sub: session.OAuthSub, |
🤖 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/context.go` around lines 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.
Comment on lines
+122
to
128
| func (auth *AuthService) SearchUser(username string) (*model.UserSearch, error) { | ||
| if auth.GetLocalUser(username).Username != "" { | ||
| return config.UserSearch{ | ||
| return &model.UserSearch{ | ||
| Username: username, | ||
| Type: "local", | ||
| } | ||
| Type: model.UserLocal, | ||
| }, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Nil pointer dereference when local user not found.
GetLocalUser returns nil when no matching user exists (line 177). Accessing .Username on the nil result will cause a runtime panic.
🐛 Proposed fix
func (auth *AuthService) SearchUser(username string) (*model.UserSearch, error) {
- if auth.GetLocalUser(username).Username != "" {
+ if localUser := auth.GetLocalUser(username); localUser != nil {
return &model.UserSearch{
Username: username,
Type: model.UserLocal,
}, nil
}📝 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.
Suggested change
| func (auth *AuthService) SearchUser(username string) (*model.UserSearch, error) { | |
| if auth.GetLocalUser(username).Username != "" { | |
| return config.UserSearch{ | |
| return &model.UserSearch{ | |
| Username: username, | |
| Type: "local", | |
| } | |
| Type: model.UserLocal, | |
| }, nil | |
| } | |
| func (auth *AuthService) SearchUser(username string) (*model.UserSearch, error) { | |
| if localUser := auth.GetLocalUser(username); localUser != nil { | |
| return &model.UserSearch{ | |
| Username: username, | |
| Type: model.UserLocal, | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/auth_service.go` around lines 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.
Comment on lines
+371
to
+381
| return &http.Cookie{ | ||
| Name: auth.config.SessionCookieName, | ||
| Value: session.UUID, | ||
| Path: "/", | ||
| Domain: fmt.Sprintf(".%s", auth.config.CookieDomain), | ||
| Expires: time.Now().Add(time.Duration(newExpiry-currentTime) * time.Second), | ||
| MaxAge: auth.config.SessionExpiry, | ||
| Secure: auth.config.SecureCookie, | ||
| HttpOnly: true, | ||
| SameSite: http.SameSiteLaxMode, | ||
| }, nil |
Contributor
There was a problem hiding this comment.
Cookie Expires and MaxAge are inconsistent.
Expires uses the calculated remaining time (newExpiry - currentTime) while MaxAge uses the full SessionExpiry. These can differ significantly, causing inconsistent cookie lifetime behavior across browsers (some honor Expires, others prefer MaxAge).
🐛 Proposed fix
+ cookieMaxAge := int(newExpiry - currentTime)
+
return &http.Cookie{
Name: auth.config.SessionCookieName,
Value: session.UUID,
Path: "/",
Domain: fmt.Sprintf(".%s", auth.config.CookieDomain),
- Expires: time.Now().Add(time.Duration(newExpiry-currentTime) * time.Second),
- MaxAge: auth.config.SessionExpiry,
+ Expires: time.Now().Add(time.Duration(cookieMaxAge) * time.Second),
+ MaxAge: cookieMaxAge,
Secure: auth.config.SecureCookie,
HttpOnly: true,
SameSite: http.SameSiteLaxMode,
}, nil📝 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.
Suggested change
| return &http.Cookie{ | |
| Name: auth.config.SessionCookieName, | |
| Value: session.UUID, | |
| Path: "/", | |
| Domain: fmt.Sprintf(".%s", auth.config.CookieDomain), | |
| Expires: time.Now().Add(time.Duration(newExpiry-currentTime) * time.Second), | |
| MaxAge: auth.config.SessionExpiry, | |
| Secure: auth.config.SecureCookie, | |
| HttpOnly: true, | |
| SameSite: http.SameSiteLaxMode, | |
| }, nil | |
| cookieMaxAge := int(newExpiry - currentTime) | |
| return &http.Cookie{ | |
| Name: auth.config.SessionCookieName, | |
| Value: session.UUID, | |
| Path: "/", | |
| Domain: fmt.Sprintf(".%s", auth.config.CookieDomain), | |
| Expires: time.Now().Add(time.Duration(cookieMaxAge) * time.Second), | |
| MaxAge: cookieMaxAge, | |
| Secure: auth.config.SecureCookie, | |
| HttpOnly: true, | |
| SameSite: http.SameSiteLaxMode, | |
| }, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/auth_service.go` around lines 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.
Comment on lines
16
to
24
| file, err := os.Create("/tmp/tinyauth_users_test.txt") | ||
| assert.NilError(t, err) | ||
| assert.NoError(t, err) | ||
|
|
||
| _, err = file.WriteString(" user1:" + hash + " \n user2:" + hash + " ") // Spacing is on purpose | ||
| assert.NilError(t, err) | ||
| assert.NoError(t, err) | ||
|
|
||
| err = file.Close() | ||
| assert.NilError(t, err) | ||
| assert.NoError(t, err) | ||
| defer os.Remove("/tmp/tinyauth_users_test.txt") |
Contributor
There was a problem hiding this comment.
Use t.TempDir() instead of hard-coded /tmp paths.
These tests are Unix-specific today, and the shared absolute path is unnecessary. Building both the fixture file and the missing-file case from t.TempDir() keeps the test portable and isolated.
Proposed fix
import (
"os"
+ "path/filepath"
"testing"
@@
- file, err := os.Create("/tmp/tinyauth_users_test.txt")
+ dir := t.TempDir()
+ usersPath := filepath.Join(dir, "tinyauth_users_test.txt")
+ file, err := os.Create(usersPath)
@@
- defer os.Remove("/tmp/tinyauth_users_test.txt")
@@
- users, err := utils.GetUsers([]string{}, "/tmp/tinyauth_users_test.txt", noAttrs)
+ users, err := utils.GetUsers([]string{}, usersPath, noAttrs)
@@
- users, err = utils.GetUsers([]string{"user5:" + hash}, "/tmp/tinyauth_users_test.txt", noAttrs)
+ users, err = utils.GetUsers([]string{"user5:" + hash}, usersPath, noAttrs)
@@
- users, err = utils.GetUsers([]string{}, "/tmp/tinyauth_users_test.txt", attrs)
+ users, err = utils.GetUsers([]string{}, usersPath, attrs)
@@
- users, err = utils.GetUsers([]string{}, "/tmp/non_existent_file.txt", noAttrs)
+ users, err = utils.GetUsers([]string{}, filepath.Join(dir, "missing.txt"), noAttrs)Also applies to: 29-30, 50-50, 68-68, 90-90
🤖 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_test.go` around lines 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).
Comment on lines
+17
to
+23
| assert.NoError(t, err) | ||
|
|
||
| _, err = file.WriteString(" user1:" + hash + " \n user2:" + hash + " ") // Spacing is on purpose | ||
| assert.NilError(t, err) | ||
| assert.NoError(t, err) | ||
|
|
||
| err = file.Close() | ||
| assert.NilError(t, err) | ||
| assert.NoError(t, err) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "user_utils_test.go" -type fRepository: tinyauthapp/tinyauth
Length of output: 100
🏁 Script executed:
cat -n ./internal/utils/user_utils_test.goRepository: tinyauthapp/tinyauth
Length of output: 5481
Use require for setup and nil-sensitive assertions.
These checks gate later dereferences. With assert, the test continues into file.WriteString, *users, and user.Username, which can panic and mask the real failure. Add the require import and replace assert.NoError and assert.NotNil with their require equivalents at:
- Lines 17, 20, 23 (file operations)
- Lines 31, 32 (GetUsers result validation before dereferencing on lines 33, 35)
- Line 102 (ParseUser result validation before dereferencing on line 104)
🤖 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_test.go` around lines 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.
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
Comment on lines
+136
to
+149
| func (c *UserContext) GetUsername() string { | ||
| switch c.Provider { | ||
| case ProviderLocal: | ||
| return c.Local.Username | ||
| case ProviderLDAP: | ||
| return c.LDAP.Username | ||
| case ProviderBasicAuth: | ||
| return c.Local.Username | ||
| case ProviderOAuth: | ||
| return c.OAuth.Username | ||
| default: | ||
| return "" | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Nil-pointer dereference risk in accessor methods.
The getter methods (GetUsername, GetEmail, GetName) assume the provider-specific context pointer is non-nil. If Provider is set to ProviderLocal but Local is nil, accessing c.Local.Username will panic. Consider adding nil guards or documenting that callers must ensure the context is properly initialized.
🐛 Proposed fix for GetUsername (apply similar pattern to GetEmail, GetName)
func (c *UserContext) GetUsername() string {
switch c.Provider {
case ProviderLocal:
+ if c.Local == nil {
+ return ""
+ }
return c.Local.Username
case ProviderLDAP:
+ if c.LDAP == nil {
+ return ""
+ }
return c.LDAP.Username
case ProviderBasicAuth:
+ if c.Local == nil {
+ return ""
+ }
return c.Local.Username
case ProviderOAuth:
+ if c.OAuth == nil {
+ return ""
+ }
return c.OAuth.Username
default:
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/model/context.go` around lines 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.
Comment on lines
+30
to
43
| func (acls *AccessControlsService) lookupStaticACLs(domain string) *model.App { | ||
| for app, config := range acls.static { | ||
| if config.Config.Domain == domain { | ||
| tlog.App.Debug().Str("name", app).Msg("Found matching container by domain") | ||
| return config, nil | ||
| return &config | ||
| } | ||
|
|
||
| if strings.SplitN(domain, ".", 2)[0] == app { | ||
| tlog.App.Debug().Str("name", app).Msg("Found matching container by app name") | ||
| return config, nil | ||
| return &config | ||
| } | ||
| } | ||
| return config.App{}, errors.New("no results") | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Returning pointer to loop variable causes stale data.
In for app, config := range acls.static, config is a copy of each map value. Returning &config on lines 34 and 39 returns a pointer to this copy, which in Go 1.21 and earlier is reused across iterations (causing data races), and in Go 1.22+ is a new variable per iteration but still a local copy that may cause confusion.
Consider storing pointers in the map or making an explicit copy:
🐛 Proposed fix - return a copy
func (acls *AccessControlsService) lookupStaticACLs(domain string) *model.App {
for app, config := range acls.static {
if config.Config.Domain == domain {
tlog.App.Debug().Str("name", app).Msg("Found matching container by domain")
- return &config
+ result := config // explicit copy
+ return &result
}
if strings.SplitN(domain, ".", 2)[0] == app {
tlog.App.Debug().Str("name", app).Msg("Found matching container by app name")
- return &config
+ result := config // explicit copy
+ return &result
}
}
return nil
}📝 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.
Suggested change
| func (acls *AccessControlsService) lookupStaticACLs(domain string) *model.App { | |
| for app, config := range acls.static { | |
| if config.Config.Domain == domain { | |
| tlog.App.Debug().Str("name", app).Msg("Found matching container by domain") | |
| return config, nil | |
| return &config | |
| } | |
| if strings.SplitN(domain, ".", 2)[0] == app { | |
| tlog.App.Debug().Str("name", app).Msg("Found matching container by app name") | |
| return config, nil | |
| return &config | |
| } | |
| } | |
| return config.App{}, errors.New("no results") | |
| return nil | |
| } | |
| func (acls *AccessControlsService) lookupStaticACLs(domain string) *model.App { | |
| for app, config := range acls.static { | |
| if config.Config.Domain == domain { | |
| tlog.App.Debug().Str("name", app).Msg("Found matching container by domain") | |
| result := config // explicit copy | |
| return &result | |
| } | |
| if strings.SplitN(domain, ".", 2)[0] == app { | |
| tlog.App.Debug().Str("name", app).Msg("Found matching container by app name") | |
| result := config // explicit copy | |
| return &result | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/access_controls_service.go` around lines 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.
Comment on lines
+146
to
+150
| func (auth *AuthService) CheckUserPassword(search model.UserSearch, password string) error { | ||
| switch search.Type { | ||
| case "local": | ||
| case model.UserLocal: | ||
| user := auth.GetLocalUser(search.Username) | ||
| return auth.CheckPassword(user, password) | ||
| case "ldap": | ||
| return bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(password)) |
Contributor
There was a problem hiding this comment.
Nil pointer dereference in CheckUserPassword for local users.
When search.Type is model.UserLocal, Line 149 calls auth.GetLocalUser(search.Username) which can return nil if the user doesn't exist. Line 150 then accesses user.Password without a nil check, causing a panic.
🐛 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))📝 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.
Suggested change
| func (auth *AuthService) CheckUserPassword(search model.UserSearch, password string) error { | |
| switch search.Type { | |
| case "local": | |
| case model.UserLocal: | |
| user := auth.GetLocalUser(search.Username) | |
| return auth.CheckPassword(user, password) | |
| case "ldap": | |
| return bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(password)) | |
| 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 - 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.
Comment on lines
62
to
65
| if !docker.isConnected { | ||
| tlog.App.Debug().Msg("Docker not connected, returning empty labels") | ||
| return config.App{}, nil | ||
| return nil, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Stale log messages: "returning empty labels" should reflect the nil return.
Both log messages were written when the function returned a zero-value struct (config.App{}). Now that the return is nil, nil, the phrase "returning empty labels" is misleading to anyone grepping logs.
📝 Proposed fix
- tlog.App.Debug().Msg("Docker not connected, returning empty labels")
+ tlog.App.Debug().Msg("Docker not connected, returning nil")- tlog.App.Debug().Msg("No matching container found, returning empty labels")
+ tlog.App.Debug().Msg("No matching container found, returning nil")Also applies to: 95-98
🤖 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/docker_service.go` around lines 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.
Comment on lines
96
to
106
| if appKey, ok := k.domainIndex[domain]; ok { | ||
| if apps, ok := k.ingressApps[appKey.ingressKey]; ok { | ||
| for _, app := range apps { | ||
| if app.domain == domain && app.appName == appKey.appName { | ||
| return app.app, true | ||
| return &app.app | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return config.App{}, false | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Go version to understand loop variable semantics
# Go 1.22+ has different loop variable behavior, but returning address of local is still problematic
# Check if there's a go.mod with Go version
cat go.mod 2>/dev/null | grep -E "^go [0-9]"Repository: tinyauthapp/tinyauth
Length of output: 74
🏁 Script executed:
#!/bin/bash
# Read the full function and surrounding context
sed -n '85,130p' internal/service/kubernetes_service.goRepository: tinyauthapp/tinyauth
Length of output: 1103
🏁 Script executed:
#!/bin/bash
# Find type definitions for ingressApp and related types
rg -A 5 "type ingressApp struct"Repository: tinyauthapp/tinyauth
Length of output: 377
🏁 Script executed:
#!/bin/bash
# Check the map definition to understand how apps are stored
rg -B 5 -A 2 "ingressApps.*map"Repository: tinyauthapp/tinyauth
Length of output: 1756
🏁 Script executed:
#!/bin/bash
# Check getByAppName function (mentioned as having similar pattern)
sed -n '108,130p' internal/service/kubernetes_service.goRepository: tinyauthapp/tinyauth
Length of output: 634
🏁 Script executed:
#!/bin/bash
# Look for tests of getByDomain and getByAppName
rg -A 10 "getByDomain|getByAppName" internal/service/kubernetes_service_test.goRepository: tinyauthapp/tinyauth
Length of output: 2827
🏁 Script executed:
#!/bin/bash
# Search for callers of getByDomain and getByAppName
rg "getByDomain|getByAppName" --type goRepository: tinyauthapp/tinyauth
Length of output: 1302
🏁 Script executed:
#!/bin/bash
# Check if the returned pointers are stored or immediately used
rg -B 3 -A 5 "GetByDomain|GetByAppName" internal/service/kubernetes_service.goRepository: tinyauthapp/tinyauth
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Find where getByDomain and getByAppName are called
rg -B 3 -A 8 "= k\.getByDomain|= k\.getByAppName" internal/service/kubernetes_service.goRepository: tinyauthapp/tinyauth
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Check the model.App type definition
rg -A 10 "type App struct"Repository: tinyauthapp/tinyauth
Length of output: 917
🏁 Script executed:
#!/bin/bash
# Check if the pointer is stored or immediately used
sed -n '270,290p' internal/service/kubernetes_service.goRepository: tinyauthapp/tinyauth
Length of output: 666
🏁 Script executed:
#!/bin/bash
# Let's verify the exact nature of the pointer issue by checking how it's used after return
sed -n '265,295p' internal/service/kubernetes_service.goRepository: tinyauthapp/tinyauth
Length of output: 1025
🏁 Script executed:
#!/bin/bash
# Check if there are any concurrent access patterns or if memory could be reused
rg -A 15 "func (k \*KubernetesService) GetLabels"Repository: tinyauthapp/tinyauth
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if the returned pointer is ever stored or used after the function call
rg -B 5 -A 10 "GetLabels" internal/service/kubernetes_service.goRepository: tinyauthapp/tinyauth
Length of output: 524
🏁 Script executed:
#!/bin/bash
# Verify the model.App structure is just a regular struct (not unsafe or containing pointers)
cat internal/model/config.go | head -50Repository: tinyauthapp/tinyauth
Length of output: 1210
🏁 Script executed:
#!/bin/bash
# Final verification: Check if model.App has any pointer fields that could be corrupted
rg -A 20 "type App struct" internal/model/config.go | head -30Repository: tinyauthapp/tinyauth
Length of output: 1023
Fix unsafe pointer return from loop variable.
The code returns &app.app on lines 100 and 116, where app is a loop variable that is a copy of each slice element. Taking the address of a field in this copy creates a pointer to a temporary on the stack, which is undefined behavior per the Go spec. While the pointer is used immediately in practice (and thus usually works), this should be fixed to point to the actual slice element instead.
🐛 Proposed fix
func (k *KubernetesService) getByDomain(domain string) *model.App {
k.mu.RLock()
defer k.mu.RUnlock()
if appKey, ok := k.domainIndex[domain]; ok {
if apps, ok := k.ingressApps[appKey.ingressKey]; ok {
- for _, app := range apps {
+ for i := range apps {
+ app := &apps[i]
if app.domain == domain && app.appName == appKey.appName {
return &app.app
}
}
}
}
return nil
}Apply the same fix to getByAppName on lines 108–122.
📝 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.
Suggested change
| if appKey, ok := k.domainIndex[domain]; ok { | |
| if apps, ok := k.ingressApps[appKey.ingressKey]; ok { | |
| for _, app := range apps { | |
| if app.domain == domain && app.appName == appKey.appName { | |
| return app.app, true | |
| return &app.app | |
| } | |
| } | |
| } | |
| } | |
| return config.App{}, false | |
| return nil | |
| } | |
| if appKey, ok := k.domainIndex[domain]; ok { | |
| if apps, ok := k.ingressApps[appKey.ingressKey]; ok { | |
| for i := range apps { | |
| app := &apps[i] | |
| if app.domain == domain && app.appName == appKey.appName { | |
| return &app.app | |
| } | |
| } | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/kubernetes_service.go` around lines 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.
Comment on lines
+108
to
122
| func (k *KubernetesService) getByAppName(appName string) *model.App { | ||
| k.mu.RLock() | ||
| defer k.mu.RUnlock() | ||
|
|
||
| if appKey, ok := k.appNameIndex[appName]; ok { | ||
| if apps, ok := k.ingressApps[appKey.ingressKey]; ok { | ||
| for _, app := range apps { | ||
| if app.appName == appName { | ||
| return app.app, true | ||
| return &app.app | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return config.App{}, false | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Same loop variable address issue in getByAppName.
This function has the same problem as getByDomain - returning &app.app from a loop variable.
🐛 Proposed fix
func (k *KubernetesService) getByAppName(appName string) *model.App {
k.mu.RLock()
defer k.mu.RUnlock()
if appKey, ok := k.appNameIndex[appName]; ok {
if apps, ok := k.ingressApps[appKey.ingressKey]; ok {
- for _, app := range apps {
+ for i := range apps {
+ app := &apps[i]
if app.appName == appName {
return &app.app
}
}
}
}
return nil
}📝 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.
Suggested change
| func (k *KubernetesService) getByAppName(appName string) *model.App { | |
| k.mu.RLock() | |
| defer k.mu.RUnlock() | |
| if appKey, ok := k.appNameIndex[appName]; ok { | |
| if apps, ok := k.ingressApps[appKey.ingressKey]; ok { | |
| for _, app := range apps { | |
| if app.appName == appName { | |
| return app.app, true | |
| return &app.app | |
| } | |
| } | |
| } | |
| } | |
| return config.App{}, false | |
| return nil | |
| } | |
| func (k *KubernetesService) getByAppName(appName string) *model.App { | |
| k.mu.RLock() | |
| defer k.mu.RUnlock() | |
| if appKey, ok := k.appNameIndex[appName]; ok { | |
| if apps, ok := k.ingressApps[appKey.ingressKey]; ok { | |
| for i := range apps { | |
| app := &apps[i] | |
| if app.appName == appName { | |
| return &app.app | |
| } | |
| } | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/kubernetes_service.go` around lines 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.
Comment on lines
+370
to
406
| func (service *OIDCService) StoreUserinfo(c *gin.Context, sub string, userContext model.UserContext, req AuthorizeRequest) error { | ||
| userInfoParams := repository.CreateOidcUserInfoParams{ | ||
| Sub: sub, | ||
| Name: userContext.Name, | ||
| Email: userContext.Email, | ||
| PreferredUsername: userContext.Username, | ||
| Name: userContext.GetName(), | ||
| Email: userContext.GetEmail(), | ||
| PreferredUsername: userContext.GetUsername(), | ||
| UpdatedAt: time.Now().Unix(), | ||
| GivenName: userContext.Attributes.GivenName, | ||
| FamilyName: userContext.Attributes.FamilyName, | ||
| MiddleName: userContext.Attributes.MiddleName, | ||
| Nickname: userContext.Attributes.Nickname, | ||
| Profile: userContext.Attributes.Profile, | ||
| Picture: userContext.Attributes.Picture, | ||
| Website: userContext.Attributes.Website, | ||
| Gender: userContext.Attributes.Gender, | ||
| Birthdate: userContext.Attributes.Birthdate, | ||
| Zoneinfo: userContext.Attributes.Zoneinfo, | ||
| Locale: userContext.Attributes.Locale, | ||
| PhoneNumber: userContext.Attributes.PhoneNumber, | ||
| Address: string(addressJSON), | ||
| } | ||
|
|
||
| if userContext.IsLocal() { | ||
| addressJSON, err := json.Marshal(userContext.Local.Attributes.Address) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| userInfoParams.GivenName = userContext.Local.Attributes.GivenName | ||
| userInfoParams.FamilyName = userContext.Local.Attributes.FamilyName | ||
| userInfoParams.MiddleName = userContext.Local.Attributes.MiddleName | ||
| userInfoParams.Nickname = userContext.Local.Attributes.Nickname | ||
| userInfoParams.Profile = userContext.Local.Attributes.Profile | ||
| userInfoParams.Picture = userContext.Local.Attributes.Picture | ||
| userInfoParams.Website = userContext.Local.Attributes.Website | ||
| userInfoParams.Gender = userContext.Local.Attributes.Gender | ||
| userInfoParams.Birthdate = userContext.Local.Attributes.Birthdate | ||
| userInfoParams.Zoneinfo = userContext.Local.Attributes.Zoneinfo | ||
| userInfoParams.Locale = userContext.Local.Attributes.Locale | ||
| userInfoParams.PhoneNumber = userContext.Local.Attributes.PhoneNumber | ||
| userInfoParams.Address = string(addressJSON) | ||
| } | ||
|
|
||
| // Tinyauth will pass through the groups it got from an LDAP or an OIDC server | ||
| if userContext.Provider == "ldap" { | ||
| userInfoParams.Groups = userContext.LdapGroups | ||
| if userContext.IsLDAP() { | ||
| userInfoParams.Groups = strings.Join(userContext.LDAP.Groups, ",") | ||
| } | ||
|
|
||
| if userContext.OAuth && len(userContext.OAuthGroups) > 0 { | ||
| userInfoParams.Groups = userContext.OAuthGroups | ||
| if userContext.IsOAuth() { | ||
| userInfoParams.Groups = strings.Join(userContext.OAuth.Groups, ",") | ||
| } |
Contributor
There was a problem hiding this comment.
Potential nil-pointer dereference when accessing provider context fields.
StoreUserinfo directly accesses userContext.Local.Attributes (Line 380) and userContext.LDAP.Groups/userContext.OAuth.Groups (Lines 401, 405) without nil guards. If the corresponding provider context is nil while the provider type check passes, this will panic.
This is a downstream effect of the nil-guard issue in internal/model/context.go. Once that is fixed, consider whether this code needs its own defensive checks or can rely on the context being properly initialized.
🤖 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/oidc_service.go` around lines 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.
Comment on lines
+36
to
41
| func GetUsers(usersCfg []string, usersPath string, userAttributes map[string]model.UserAttributes) (*[]model.LocalUser, error) { | ||
| var usersStr []string | ||
|
|
||
| if len(usersCfg) == 0 && usersPath == "" { | ||
| return []config.User{}, nil | ||
| return nil, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how callers handle the nil return from GetUsers
rg -n -A5 'GetUsers\(' --type=goRepository: tinyauthapp/tinyauth
Length of output: 3577
🏁 Script executed:
cat -n internal/bootstrap/app_bootstrap.go | sed -n '60,85p'Repository: tinyauthapp/tinyauth
Length of output: 1073
Fix nil-pointer dereference when GetUsers returns nil.
Line 72 in app_bootstrap.go unconditionally dereferences users with *users without checking if it's nil. Since GetUsers returns (nil, nil) when both usersCfg and usersPath are empty (as confirmed by the test at line 87 of user_utils_test.go), this causes a panic. Add a nil check before the dereference:
if users != nil {
app.context.localUsers = *users
}🤖 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 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.
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