security: Add CSRF protection to login and authentication routes #446
Conversation
- Configure CSRF middleware with custom settings - Add CSRF token generation and rendering in login page - Expose CSRF token in login template - Modify security functions to support local subnet detection - Improve security middleware configuration
WalkthroughThe changes implement CSRF protection in the authentication flow. The login page rendering now injects a CSRF token into the template data, and a corresponding hidden input field has been added to the login form. A new middleware is introduced to validate CSRF tokens, with configurable cookie settings and a skip mechanism for certain routes. Additionally, the helper function for checking local subnets has been renamed and exported for external use. New methods have been added to the OAuth2Server to manage user authentication, authorization code generation, exchange, and access token validation. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Middleware
participant A as Auth Route
participant V as View Renderer
U->>M: Sends login page request
M->>A: Forwards request after CSRF check
A->>V: Renders login page with CSRF token data
V-->>U: Returns HTML form with hidden CSRF token field
sequenceDiagram
participant U as User
participant O as OAuth2Server
U->>O: Requests authentication (Auth Code)
O->>O: Checks if user is authenticated (includes local subnet check)
O->>O: Generates Auth Code with CSRF protection
U->>O: Exchanges Auth Code for Access Token
O->>O: Validates Auth Code and issues Access Token
O-->>U: Returns Access Token
Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/security/oauth.go (1)
94-99:🧩 Analysis chain
Review local subnet authentication bypass.
The automatic authentication for local subnet clients might pose a security risk if the network is not properly secured. Consider adding additional authentication factors or making this behavior configurable.
🏁 Script executed:
#!/bin/bash # Description: Check for other instances of local subnet authentication bypass # Test: Look for similar patterns of local subnet checks rg -A 5 'IsInLocalSubnet|isInLocalSubnet'Length of output: 1697
Local Subnet Bypass Found – Revisit Authentication Logic
- OAuth Check (internal/security/oauth.go): The code immediately authenticates any client whose IP is recognized as belonging to the local subnet.
- Similar Patterns: Comparable logic is used in both internal/basic.go (for cookie handling and basic auth) and internal/httpcontroller/middleware.go.
- Security Implication: Automatically trusting local subnet addresses can be risky in environments where network segmentation isn’t strict. This could allow an attacker, who manages to spoof or gain access to a local address, to bypass proper authentication. Consider either adding additional authentication factors for local subnet clients or making this behavior configurable.
🧹 Nitpick comments (2)
internal/httpcontroller/auth_routes.go (1)
95-95: Add error handling for CSRF token retrieval.While adding the CSRF token to the template data is correct, consider handling the case where the token might be nil.
- "CSRFToken": c.Get("csrf"), + "CSRFToken": c.Get("csrf"), // Token is automatically generated by the CSRF middleware + "Error": nil, // Add error handling if token is nilinternal/security/oauth.go (1)
140-156: Enhance auth code generation security.The auth code generation looks secure with proper cryptographic randomness. However, consider:
- Using a constant-time comparison for code validation
- Adding rate limiting for code generation
func (s *OAuth2Server) GenerateAuthCode() (string, error) { + // Add rate limiting + if s.isRateLimited() { + return "", errors.New("rate limit exceeded") + } + code := make([]byte, 32) _, err := rand.Read(code)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/httpcontroller/auth_routes.go(1 hunks)internal/httpcontroller/middleware.go(1 hunks)internal/security/basic.go(2 hunks)internal/security/oauth.go(4 hunks)views/elements/login.html(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit-tests
- GitHub Check: test-docker-image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/httpcontroller/middleware.go (1)
21-35: Review CSRF middleware configuration.The CSRF configuration looks good overall, with appropriate cookie settings and token lookup. However, consider the following security aspects:
- The skipper function might be too permissive. While it's common to skip CSRF for static assets, ensure that these directories only serve static files and cannot be used for file uploads or dynamic content.
- The token lifetime of 1 hour is reasonable but consider making it configurable through settings.
Run the following script to verify that the skipped paths only contain static files:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for potential security risks in assets and media directories # Test: Look for potentially dangerous file types in assets and media directories rg -g '!*.{jpg,jpeg,png,gif,ico,css,js,woff,woff2,ttf,eot,svg}' -l '/assets/|/media/' # Test: Look for file upload endpoints or dynamic content handlers in these paths ast-grep --pattern 'func $_($_) { $$$ c.FormFile($_) $$$ }'Length of output: 537
CSRF middleware configuration verified.
- The CSRF setup (token lookup, cookie options, and conditional CookieSecure) is correctly configured.
- The skipper function correctly bypasses CSRF for requests matching static assets, and our search did not reveal any file upload handlers (e.g., calls to c.FormFile) in those paths.
- Note that the grep results include several files (like routes.go and middleware.go) because they contain text references to “/assets/” or “/media/”, which is expected. Ensure that the actual assets and media directories serve only static content and that no dynamic endpoints are inadvertently exposed.
views/elements/login.html (1)
5-5: CSRF Token Hidden Input InjectionThe newly added hidden input field correctly injects a CSRF token into the login form. This aligns well with the PR objective of strengthening CSRF protection. Please ensure that the context data always provides a valid CSRF token (i.e.
{{.CSRFToken}}) and that the corresponding middleware consistently validates the token upon form submission.
| // IsInLocalSubnet checks if the given IP is in the same subnet as any local network interface | ||
| func IsInLocalSubnet(clientIP net.IP) bool { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document security implications of IsInLocalSubnet.
Since this function is now exported and used for security-critical decisions, add comprehensive documentation about its security implications and proper usage.
-// IsInLocalSubnet checks if the given IP is in the same subnet as any local network interface
+// IsInLocalSubnet checks if the given IP is in the same subnet as any local network interface.
+//
+// Security Implications:
+// 1. This function is used for authentication bypass in local networks
+// 2. Should only be used in development or in secure, isolated networks
+// 3. Network configuration must ensure proper segmentation
+//
+// Usage:
+// - Ensure proper network security before relying on this for authentication
+// - Consider additional authentication factors in production📝 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.
| // IsInLocalSubnet checks if the given IP is in the same subnet as any local network interface | |
| func IsInLocalSubnet(clientIP net.IP) bool { | |
| // IsInLocalSubnet checks if the given IP is in the same subnet as any local network interface. | |
| // | |
| // Security Implications: | |
| // 1. This function is used for authentication bypass in local networks | |
| // 2. Should only be used in development or in secure, isolated networks | |
| // 3. Network configuration must ensure proper segmentation | |
| // | |
| // Usage: | |
| // - Ensure proper network security before relying on this for authentication | |
| // - Consider additional authentication factors in production | |
| func IsInLocalSubnet(clientIP net.IP) bool { |
|
@petterip can you please review this? |
Proposed solution to fix #314