fix: address findings from 10 independent code reviews

[CybotTM]

Summary

Addresses critical bugs, security issues, and code quality findings from 10 independent code reviews of the per-user LDAP credentials feature (#430).

Bug Fixes

• Slice mutation during iteration in OnRemoveUserFromGroup — added missing break after removal
• Cache pointer escape — FindByDN/FindBySAMAccountName/Find now return value copies instead of pointers into cache internals
• CSRF token cached in HTML — detail pages (user/group/computer) excluded from template cache, rendered directly
• TemplateCache.Stop() double-close panic — guarded with sync.Once
• RenderWithCache ignoring request context — switched from context.Background() to c.UserContext()
• handle500 recursion risk — rewritten to handle all Fiber error codes, prevent template render recursion, hide internal errors from users
• Rate limiter status code — fixed middleware response chain

Security

• Username validation — added LDAP filter metacharacters (*, (, ), null byte) to rejection list
• Error message leakage — handle500 now shows generic message instead of internal error details

Code Quality

• Dead code removal — OptionalAuth, GetCachedManifest, manifestCache, manifestMutex, App.logger
• createCSRFConfig — returns fiber.Handler directly instead of *fiber.Handler
• Shutdown ordering — drain Fiber first, then stop caches, then close connections
• Filter pre-allocation — pre-allocates result slice capacity
• Mock thread safety — added sync.Mutex to test helper call count tracking

Test plan

• All existing tests pass
• go vet clean
• gofumpt formatting applied
• Race detector clean (-race on ldap_cache package)
• Prettier check passes
• CI pipeline passes

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request delivers a comprehensive set of fixes and improvements based on extensive code reviews of the per-user LDAP credentials feature. The changes significantly enhance the application's robustness, security posture, and overall code quality. Key areas addressed include preventing common programming pitfalls, safeguarding against security vulnerabilities, and streamlining internal processes for better maintainability.

Highlights

• Bug Fixes: Corrected slice mutation during iteration in OnRemoveUserFromGroup by adding a break statement after removal.
• Bug Fixes: Resolved cache pointer escape by returning value copies instead of pointers from cache lookup methods (Find, FindByDN, FindBySAMAccountName).
• Bug Fixes: Prevented CSRF token caching in HTML by explicitly excluding detail pages (user, group, computer) from the template cache.
• Bug Fixes: Guarded TemplateCache.Stop() with sync.Once to prevent double-close panics during application shutdown.
• Bug Fixes: Switched RenderWithCache to use c.UserContext() instead of context.Background() to ensure request context is respected.
• Bug Fixes: Rewrote handle500 to properly handle all Fiber error codes, prevent template render recursion, and hide internal error details from users.
• Bug Fixes: Fixed the rate limiter middleware response chain to correctly set the HTTP status code before sending the response.
• Security: Enhanced username validation to reject LDAP filter metacharacters (*, (, ), and null byte) to prevent potential LDAP injection.
• Security: Modified handle500 to display a generic error message, preventing the leakage of internal error details to end-users.
• Code Quality: Removed dead code, including the OptionalAuth middleware, GetCachedManifest, manifestCache, manifestMutex, and the App.logger field.
• Code Quality: Updated createCSRFConfig to return fiber.Handler directly, simplifying its usage.
• Code Quality: Adjusted the application shutdown ordering to first drain Fiber requests, then stop caches, and finally close LDAP connections.
• Code Quality: Pre-allocated slice capacity in the Filter method for improved performance.
• Code Quality: Added sync.Mutex to mock test helper call count tracking to ensure thread safety during concurrent tests.

Changelog

• internal/ldap_cache/cache.go
  • Returned copies of cached items instead of pointers to prevent pointer escape.
  • Pre-allocated slice capacity in Filter method for performance.
• internal/ldap_cache/cache_errors_test.go
  • Updated TestCache_FilterNoMatches to assert an empty slice instead of nil.
• internal/ldap_cache/manager.go
  • Added break statements after removing items from slices in OnRemoveUserFromGroup to prevent slice mutation issues.
  • Standardized refreshInterval, warmupComplete, and retryConfig declarations.
• internal/ldap_cache/test_helpers.go
  • Introduced sync.Mutex to mockLDAPClient to ensure thread-safe tracking of call counts.
• internal/web/assets.go
  • Removed manifestCache and manifestMutex variables.
  • Removed GetCachedManifest function.
  • Removed mutex locking from LoadAssetManifest.
• internal/web/assets_test.go
  • Renamed TestGetCachedManifest to TestLoadAssetManifestIdempotent.
  • Modified test to verify idempotent loading instead of caching behavior.
• internal/web/auth.go
  • Expanded username validation in authenticateViaUPNBind to include LDAP filter metacharacters and null bytes.
• internal/web/computers.go
  • Disabled template caching for computer detail pages to prevent CSRF token issues.
• internal/web/cookie_security_test.go
  • Adjusted the usage of createCSRFConfig to directly use the returned fiber.Handler.
• internal/web/doc.go
  • Removed OptionalAuth middleware from the package documentation.
• internal/web/groups.go
  • Disabled template caching for group detail pages to prevent CSRF token issues.
• internal/web/middleware.go
  • Removed the OptionalAuth middleware function.
• internal/web/middleware_test.go
  • Removed all tests related to the OptionalAuth middleware.
• internal/web/ratelimit.go
  • Corrected the order of setting the status code and sending the response in the rate limiter middleware.
• internal/web/server.go
  • Removed the logger field from the App struct.
  • Modified createCSRFConfig to return fiber.Handler directly, removing pointer usage.
  • Adjusted setupRoutes to explicitly exclude detail pages (users, groups, computers) from template caching.
  • Reordered shutdown sequence to first drain Fiber, then stop background goroutines, then close LDAP connections.
  • Refactored handle500 to handle various Fiber error codes, use a generic error message, and include a fallback for template rendering failures.
• internal/web/template_cache.go
  • Added sync.Once to the Stop method to ensure it's called only once.
  • Changed RenderWithCache to use c.UserContext() instead of context.Background().
• internal/web/templates/flash.go
  • Modified Flash.BorderColor to return a default info color instead of panicking for unknown flash types.
• internal/web/templates/flash_test.go
  • Updated TestFlashBorderColor to test the new fallback behavior for unknown flash types.
• internal/web/users.go
  • Disabled template caching for user detail pages to prevent CSRF token issues.

Activity

• Addressed findings from 10 independent code reviews.
• Implemented bug fixes, security enhancements, and code quality improvements.
• Ensured all existing tests pass.
• Verified go vet and gofumpt cleanliness.
• Confirmed race detector cleanliness for the ldap_cache package.
• Checked Prettier formatting.
• Awaiting CI pipeline pass.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a comprehensive list of findings from previous code reviews, encompassing critical bugs, security vulnerabilities, and code quality enhancements. The changes are substantial and well-executed. However, a critical security vulnerability remains: the newly added username validation for LDAP injection prevention is inconsistently applied. It is currently only present in the authenticateViaUPNBind function, leaving the service-account-based authentication path in authenticateUser potentially vulnerable. Moving this validation to the top-level authenticateUser function will ensure consistent protection across both authentication methods. Additionally, I have one suggestion for improvement regarding error handling in the flash.go template helper to enhance observability.