Skip to content

fix: potential nil pointer dereferences#893

Merged
steveiliop56 merged 1 commit into
mainfrom
fix/nil-pointers
May 24, 2026
Merged

fix: potential nil pointer dereferences#893
steveiliop56 merged 1 commit into
mainfrom
fix/nil-pointers

Conversation

@scottmckendry
Copy link
Copy Markdown
Member

@scottmckendry scottmckendry commented May 23, 2026

in light of the bug fixed in 2737a25, i decided to look for other potential areas where we may not be defensively checking for nil pointers.

The first case where the user is nil is extremely unlikely, but under specific race conditions, the TOTP check may try to deref a nil pointer to user. E.g. GetLocalUser returns nil after SearchUser succeeds when LocalUsers is modified concurrently. At this point accessing user.TOTPSecret on nil panics.

Second case is more obvious, ctx.ACLs isn't checked for nil for ldap groups. It's deref'd later in the function without a defensive nil check.

Summary by CodeRabbit

  • Bug Fixes
    • Improved robustness of user authentication by properly handling cases where local user accounts are missing
    • Enhanced null safety checks in access control rule evaluation to prevent potential application crashes

Review Change Stack

@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label May 23, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: febe043e-6053-4eb1-817d-672ca3f510c6

📥 Commits

Reviewing files that changed from the base of the PR and between 2737a25 and a7bc672.

📒 Files selected for processing (2)
  • internal/middleware/context_middleware.go
  • internal/service/access_controls_rules.go

📝 Walkthrough

Walkthrough

This PR adds two defensive nil-pointer checks to prevent panics: one in local user authentication when the user record is missing, and another in LDAP group rule evaluation when ACL context is absent.

Changes

Nil-guard safety improvements

Layer / File(s) Summary
Local user nil check in auth middleware
internal/middleware/context_middleware.go
basicAuth validates that the local user record from GetLocalUser is not nil before accessing fields like TOTPSecret, returning a formatted error for missing users instead of continuing with a nil pointer.
ACL context nil check in LDAP rules
internal/service/access_controls_rules.go
LDAPGroupRule.Evaluate strengthens its early-return guard to include ctx.ACLs == nil, treating missing ACL context as EffectAbstain rather than proceeding to LDAP group checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A rabbit hops through code with care,
Finding nil pointers hiding there,
With guards in place, both safe and sound,
No panics when the users aren't found! 🛡️

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: potential nil pointer dereferences' directly and accurately summarizes the main changes: two defensive nil-pointer checks are added to prevent dereferences in the context middleware and LDAP group evaluation logic.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/nil-pointers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2026

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/middleware/context_middleware.go 0.00% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label May 24, 2026
@steveiliop56 steveiliop56 merged commit e532cde into main May 24, 2026
5 checks passed
@steveiliop56 steveiliop56 deleted the fix/nil-pointers branch May 24, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants