Conversation
- Use constant-time comparison for PSK authentication (timing attack) - Add SameSite=Lax to all auth cookies and ClearCookies - Remove session tokens from log messages (credential leak) - Use json.Marshal for webhook payload instead of string concatenation - Return success on password reset for non-existent accounts (enumeration) - Add guarded type assertion in logout handler Signed-off-by: Maciek <tomczukmaciej@gmail.com>
- Validate profile name (required, min=1, max=100) - Validate email format on login - Add array bounds (min=1, max=100, dive) to blocklist and service updates - Register device_id custom validator using libs/deviceid package - Cap search query length (max=256) in query logs - Require action field with oneof constraint on custom rules - Pre-compile regex patterns in validator (was per-call) Signed-off-by: Maciek <tomczukmaciej@gmail.com>
- Guard type assertions in sessions and auth handlers - Fix wrong error constant in updateAccount validation (was ErrFailedToCreateCustomRule) - Fix wrong error message in disable2FA (was ErrFailedToRegisterAccount) - Return error from NewWebAuthn instead of nil on failure - Fix dead code in config: reorder profileIDMinLen bounds check - Add security warnings for empty PSK/BasicAuth in config - Extract envOrDefault helper to reduce cyclomatic complexity Signed-off-by: Maciek <tomczukmaciej@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR implements multiple security hardening measures across the API codebase, addressing vulnerabilities related to account enumeration, timing attacks, input validation, session security, and error logging.
Changes:
- Prevented account enumeration by returning success for non-existent accounts in password reset flows
- Implemented constant-time comparison for PSK authentication to prevent timing attacks
- Added comprehensive input validation including length limits, format constraints, and device ID validation
- Enhanced session security with SameSite cookie attributes and safer type assertions
- Removed sensitive data (tokens, emails) from error logs
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| api/service/account/account.go | Prevents account enumeration in password reset by returning nil for non-existent accounts |
| api/service/account/service_test.go | Updates test expectations to reflect account enumeration prevention |
| api/model/profile.go | Adds max length validation (50 chars) to profile name field |
| api/main.go | Adds error handling for WebAuthn initialization |
| api/internal/validator/validator.go | Optimizes password validation with pre-compiled regexes and adds device ID validation |
| api/internal/middleware/psk.go | Implements constant-time comparison for PSK to prevent timing attacks |
| api/internal/middleware/auth.go | Improves WebAuthn initialization with proper error handling |
| api/internal/client/http.go | Replaces manual JSON construction with proper marshaling |
| api/db/mongodb/session.go | Removes sensitive token data from error logs |
| api/config/config.go | Refactors config parsing with helper function and adds security warnings |
| api/api/webauthn_test.go | Updates test to handle WebAuthn initialization errors |
| api/api/webauthn.go | Adds SameSite attribute to WebAuthn session cookies |
| api/api/totp.go | Fixes error message for 2FA disable endpoint |
| api/api/sessions.go | Adds safe type assertions for session management |
| api/api/services.go | Adds validation for service IDs array (min=1, max=100) |
| api/api/requests/query_logs.go | Adds max length validation (256 chars) to search query |
| api/api/requests/profiles.go | Adds validation for profile custom rule actions |
| api/api/requests/mobileconfig.go | Adds device ID validation to mobile config requests |
| api/api/requests/auth.go | Adds email format validation to login request |
| api/api/profiles.go | Adds validation for profile name field |
| api/api/blocklists.go | Adds validation for blocklist IDs array (min=1, max=100) |
| api/api/auth.go | Adds safe type assertions and SameSite attributes to auth cookies |
| api/api/accounts.go | Fixes error message for account update validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
PR type
What kind of change does this PR introduce?