Implement OAuth2 authentication (for issue #301) #313
Conversation
WalkthroughThe changes in this pull request encompass the addition of a new script for resetting authentication settings, enhancements to the Dockerfile, and updates across various files including README, CSS, JavaScript, and HTML templates. Key modifications include the introduction of new security features, OAuth2 authentication routes, and improvements to UI components for better responsiveness and user experience. Documentation has also been expanded to include security practices, while several utility functions and validation mechanisms have been added or refined. Changes
Possibly related issues
Poem
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (92)
assets/util.js (1)
6-12: 🛠️ Refactor suggestion
Add input validation to prevent invalid dates.
The date manipulation logic has been improved, but it lacks input validation which could lead to issues with invalid dates.
function moveDatePicker(days) { const picker = document.getElementById('datePicker'); + if (!picker?.value?.match(/^\d{4}-\d{2}-\d{2}$/)) { + console.error('Invalid date format'); + return; + } const [yy, mm, dd] = picker.value.split('-'); + if (!yy || !mm || !dd) { + console.error('Invalid date components'); + return; + } const date = new Date(yy, mm - 1, dd); + if (isNaN(date.getTime())) { + console.error('Invalid date'); + return; + } date.setDate(date.getDate() + days); picker.value = date.toLocaleDateString('sv'); - picker.dispatchEvent(new Event('change')); + try { + picker.dispatchEvent(new Event('change')); + } catch (e) { + console.error('Failed to dispatch change event:', e); + } }📝 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.const picker = document.getElementById('datePicker'); if (!picker?.value?.match(/^\d{4}-\d{2}-\d{2}$/)) { console.error('Invalid date format'); return; } const [yy, mm, dd] = picker.value.split('-'); if (!yy || !mm || !dd) { console.error('Invalid date components'); return; } const date = new Date(yy, mm - 1, dd); if (isNaN(date.getTime())) { console.error('Invalid date'); return; } date.setDate(date.getDate() + days); picker.value = date.toLocaleDateString('sv'); try { picker.dispatchEvent(new Event('change')); } catch (e) { console.error('Failed to dispatch change event:', e); }views/settings/templates/checkbox.html (2)
1-4:
⚠️ Potential issueAdd input validation and XSS prevention.
The template parameters should be validated to prevent potential security issues:
- The Alpine.js initialization should handle potential JavaScript injection in the parameters
- The dynamic class attribute needs proper HTML escaping
Apply this diff to add proper escaping:
-<div class="form-control relative {{.class}}" x-data="checkbox('{{.id}}', '{{.label}}', '{{.model}}', '{{.name}}')"> +<div class="form-control relative {{template "escapeHTML" .class}}" + x-data="checkbox({{template "escapeJS" .id}}, {{template "escapeJS" .label}}, {{template "escapeJS" .model}}, {{template "escapeJS" .name}})">Committable suggestion skipped: line range outside the PR's diff.
15-23: 🛠️ Refactor suggestion
Improve tooltip accessibility and mobile support.
The tooltip implementation needs improvements for keyboard navigation and touch devices:
Apply these accessibility enhancements:
-<span class="ml-2 text-sm text-gray-500 cursor-help" @mouseenter="showTooltip = '{{.id}}'" - @mouseleave="showTooltip = null">ⓘ</span> +<button type="button" + class="ml-2 text-sm text-gray-500 cursor-help" + @mouseenter="showTooltip = '{{.id}}'" + @mouseleave="showTooltip = null" + @focus="showTooltip = '{{.id}}'" + @blur="showTooltip = null" + @touchstart.prevent="showTooltip = showTooltip === '{{.id}}' ? null : '{{.id}}'" + aria-label="More information">ⓘ</button> -<div x-show="showTooltip === '{{.id}}'" x-cloak +<div x-show="showTooltip === '{{.id}}'" x-cloak + :id="'tooltip-{{.id}}'" + role="tooltip" class="absolute left-0 bottom-full mb-2 p-2 bg-gray-100 text-sm rounded shadow-md z-50">📝 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.<button type="button" class="ml-2 text-sm text-gray-500 cursor-help" @mouseenter="showTooltip = '{{.id}}'" @mouseleave="showTooltip = null" @focus="showTooltip = '{{.id}}'" @blur="showTooltip = null" @touchstart.prevent="showTooltip = showTooltip === '{{.id}}' ? null : '{{.id}}'" aria-label="More information">ⓘ</button> </label> <!-- Tooltip --> <div x-show="showTooltip === '{{.id}}'" x-cloak :id="'tooltip-{{.id}}'" role="tooltip" class="absolute left-0 bottom-full mb-2 p-2 bg-gray-100 text-sm rounded shadow-md z-50"> {{.tooltip}} </div>internal/security/oauth_test.go (1)
16-53: 🛠️ Refactor suggestion
Enhance test robustness and coverage.
While the basic test flow is good, consider the following improvements:
- Add cleanup to prevent test pollution
- Add more test cases including expired tokens
- Convert to table-driven tests for better coverage
- Enhance error messages
- Add session storage verification
Here's a suggested implementation:
+func TestIsUserAuthenticated(t *testing.T) { + // Restore original gothic.Store after test + originalStore := gothic.Store + defer func() { + gothic.Store = originalStore + }() + + tests := []struct { + name string + token string + expires time.Duration + want bool + }{ + { + name: "valid token", + token: "valid_token", + expires: time.Hour, + want: true, + }, + { + name: "expired token", + token: "expired_token", + expires: -time.Hour, + want: false, + }, + { + name: "invalid token", + token: "invalid_token", + expires: time.Hour, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + settings := &conf.Settings{ + Security: conf.Security{ + SessionSecret: "test-secret", + }, + } + + s := NewOAuth2Server(settings) + + gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) + gothic.SetState = func(req *http.Request) string { + return "" + } + + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + // Store token using gothic's method + gothic.StoreInSession("access_token", tt.token, req, rec) + + // Verify token is stored in session + session, err := gothic.Store.Get(req, gothic.SessionName) + if err != nil { + t.Fatalf("Failed to get session: %v", err) + } + if token, ok := session.Values["access_token"]; !ok || token != tt.token { + t.Errorf("Token not stored in session correctly, got %v", token) + } + + // Add cookie to request + req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) + + if tt.token != "invalid_token" { + // Add token to OAuth2Server's valid tokens + s.accessTokens[tt.token] = AccessToken{ + Token: tt.token, + ExpiresAt: time.Now().Add(tt.expires), + } + } + + got := s.IsUserAuthenticated(c) + if got != tt.want { + t.Errorf("IsUserAuthenticated() = %v, want %v", got, tt.want) + } + }) + } +}📝 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.func TestIsUserAuthenticated(t *testing.T) { // Restore original gothic.Store after test originalStore := gothic.Store defer func() { gothic.Store = originalStore }() tests := []struct { name string token string expires time.Duration want bool }{ { name: "valid token", token: "valid_token", expires: time.Hour, want: true, }, { name: "expired token", token: "expired_token", expires: -time.Hour, want: false, }, { name: "invalid token", token: "invalid_token", expires: time.Hour, want: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { settings := &conf.Settings{ Security: conf.Security{ SessionSecret: "test-secret", }, } s := NewOAuth2Server(settings) gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) gothic.SetState = func(req *http.Request) string { return "" } e := echo.New() req := httptest.NewRequest(http.MethodGet, "/", nil) rec := httptest.NewRecorder() c := e.NewContext(req, rec) // Store token using gothic's method gothic.StoreInSession("access_token", tt.token, req, rec) // Verify token is stored in session session, err := gothic.Store.Get(req, gothic.SessionName) if err != nil { t.Fatalf("Failed to get session: %v", err) } if token, ok := session.Values["access_token"]; !ok || token != tt.token { t.Errorf("Token not stored in session correctly, got %v", token) } // Add cookie to request req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) if tt.token != "invalid_token" { // Add token to OAuth2Server's valid tokens s.accessTokens[tt.token] = AccessToken{ Token: tt.token, ExpiresAt: time.Now().Add(tt.expires), } } got := s.IsUserAuthenticated(c) if got != tt.want { t.Errorf("IsUserAuthenticated() = %v, want %v", got, tt.want) } }) } }reset_auth.sh (5)
1-8: 🛠️ Refactor suggestion
Add bash safety flags for robust error handling.
Add these essential bash safety flags at the start of the script to prevent silent failures and undefined variable usage:
#!/bin/bash +set -euo pipefail +IFS=$'\n\t'These flags will:
-e: Exit immediately if a command exits with non-zero status-u: Error on undefined variables-o pipefail: Return value of a pipeline is the status of the last command to exit with non-zero status📝 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.#!/bin/bash set -euo pipefail IFS=$'\n\t' # Text styling BOLD='\033[1m' GREEN='\033[0;32m' BLUE='\033[0;34m' NC='\033[0m'
9-15:
⚠️ Potential issueAdd path validation for security.
The script should validate the command-line argument to prevent path traversal or injection attacks. Consider adding validation before using the path:
+validate_path() { + local path="$1" + # Check for path traversal attempts + if [[ "$path" == *".."* ]]; then + echo "Error: Path contains invalid components" >&2 + exit 1 + fi + # Ensure path is absolute or relative to current directory + if [[ ! "$path" =~ ^(/|\./).+ ]]; then + echo "Error: Path must be absolute or relative to current directory" >&2 + exit 1 + fi +} CONFIG_PATHS=( - "$1" # Command line parameter takes precedence + "$([ -n "$1" ] && validate_path "$1" && echo "$1")" # Validated command line parameter "./config.yaml" "$HOME/.config/birdnet-go/config.yaml" "/etc/birdnet-go/config.yaml" )📝 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.validate_path() { local path="$1" # Check for path traversal attempts if [[ "$path" == *".."* ]]; then echo "Error: Path contains invalid components" >&2 exit 1 fi # Ensure path is absolute or relative to current directory if [[ ! "$path" =~ ^(/|\./).+ ]]; then echo "Error: Path must be absolute or relative to current directory" >&2 exit 1 fi } # Standard config locations CONFIG_PATHS=( "$([ -n "$1" ] && validate_path "$1" && echo "$1")" # Validated command line parameter "./config.yaml" "$HOME/.config/birdnet-go/config.yaml" "/etc/birdnet-go/config.yaml" )
17-21: 🛠️ Refactor suggestion
Add confirmation prompt and warning message.
The script performs irreversible changes to the configuration. Add a warning and confirmation prompt:
echo -e "${BOLD}BirdNET-Go Authentication Reset Tool${NC}\n" +echo -e "${BOLD}WARNING:${NC} This will reset all authentication settings to their defaults." +echo -e "A backup will be created, but the changes to the active config will be immediate.\n" +read -p "Are you sure you want to continue? (y/N) " -n 1 -r +echo +if [[ ! $REPLY =~ ^[Yy]$ ]]; then + echo "Operation cancelled." + exit 1 +fi + if [ "$1" ]; then echo -e "${BLUE}Using provided config path:${NC} $1" fi📝 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.echo -e "${BOLD}BirdNET-Go Authentication Reset Tool${NC}\n" echo -e "${BOLD}WARNING:${NC} This will reset all authentication settings to their defaults." echo -e "A backup will be created, but the changes to the active config will be immediate.\n" read -p "Are you sure you want to continue? (y/N) " -n 1 -r echo if [[ ! $REPLY =~ ^[Yy]$ ]]; then echo "Operation cancelled." exit 1 fi if [ "$1" ]; then echo -e "${BLUE}Using provided config path:${NC} $1" fi
23-49: 🛠️ Refactor suggestion
⚠️ Potential issueImprove robustness of configuration modification.
Several critical improvements are needed:
- Check write permissions before modification
- Use a more robust YAML parser instead of sed
- Handle backup file conflicts
for CONFIG_PATH in "${CONFIG_PATHS[@]}"; do [ -z "$CONFIG_PATH" ] && continue # Skip empty paths if [ -f "$CONFIG_PATH" ]; then + # Check write permissions + if [ ! -w "$CONFIG_PATH" ]; then + echo -e "${BOLD}Error:${NC} No write permission for $CONFIG_PATH" + continue + } + echo -e "${BLUE}Found config at:${NC} $CONFIG_PATH" # Create timestamped backup BACKUP="${CONFIG_PATH}.$(date +%Y%m%d_%H%M%S).bak" + # Handle existing backup + while [ -f "$BACKUP" ]; do + BACKUP="${CONFIG_PATH}.$(date +%Y%m%d_%H%M%S)_$RANDOM.bak" + done cp "$CONFIG_PATH" "$BACKUP"Consider using
yqinstead ofsedfor more reliable YAML manipulation:- # Reset auth settings - sed -i ' - /^security:/,/^[^ ]/ { - s/\(host:\).*/\1 ""/ - s/\(autotls:\).*/\1 false/ - s/\(redirecttohttps:\).*/\1 false/ - s/\(googleauth.enabled:\).*/\1 false/ - s/\(githubauth.enabled:\).*/\1 false/ - s/\(basicauth.enabled:\).*/\1 false/ - } - ' "$CONFIG_PATH" + # Check if yq is available + if ! command -v yq &> /dev/null; then + echo "Error: yq is required but not installed." + exit 1 + fi + + # Reset auth settings using yq + yq eval -i ' + .security.host = "" | + .security.autotls = false | + .security.redirecttohttps = false | + .security.googleauth.enabled = false | + .security.githubauth.enabled = false | + .security.basicauth.enabled = false + ' "$CONFIG_PATH"Committable suggestion skipped: line range outside the PR's diff.
1-53: 💡 Codebase verification
Script permissions are too permissive and need to be restricted
The script has world-readable and executable permissions (-rwxr-xr-x), which is concerning for a security-critical tool. Based on the verification:
- The script is located in the repository root and has overly permissive permissions
- There are no direct web endpoints or programmatic executions of this script
- The script is not part of any installation process or build system
Recommendations:
- Change permissions to be more restrictive (e.g., 700 or -rwx------)
- Move the script to a secure admin/tools directory
- Document that this script should only be accessible to system administrators
🔗 Analysis chain
Verify script installation location and permissions.
Since this script modifies security settings, it's crucial to ensure:
- The script is installed with restricted permissions
- Only authorized users can execute it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check script permissions in the Dockerfile rg -A 5 "reset_auth.sh" --type dockerfile # Check if there are any references to this script in web-accessible paths rg -g "*.go" -g "*.html" "reset_auth.sh"Length of output: 117
Script:
#!/bin/bash # Check if script is in any installation/build scripts rg -g "*.sh" -g "Makefile" -g "*.mk" "reset_auth.sh" # Check file location and any references in docs fd "reset_auth.sh" # Look for any API endpoints or handlers that might expose this functionality ast-grep --pattern 'func $_($$$) { $$$ exec($$$"reset_auth.sh"$$$) $$$ }'Length of output: 165
Script:
#!/bin/bash # Check current permissions of the script ls -l reset_auth.sh # Check if script is referenced in any Go files that might handle system commands ast-grep --pattern 'exec.Command($$$"reset_auth"$$$)' # Check for any web routes or API endpoints rg -g "*.go" "(HandleFunc|Handle)\(.*\)" -A 2Length of output: 1262
views/settings/templates/textField.html (3)
7-9: 🛠️ Refactor suggestion
Add error handling for validity check.
The
validInput()method should handle cases where the DOM reference is not yet available to prevent runtime errors.validInput() { - return this.$refs.{{.id}}.validity.valid; + return this.$refs.{{.id}} ? this.$refs.{{.id}}.validity.valid : true; },📝 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.validInput() { return this.$refs.{{.id}} ? this.$refs.{{.id}}.validity.valid : true; },
15-19: 🛠️ Refactor suggestion
Enhance accessibility for the tooltip trigger.
The tooltip trigger needs proper accessibility attributes and keyboard support.
- <span class="ml-2 text-sm text-gray-500 cursor-help" @mouseenter="showTooltip = '{{.id}}'" - @mouseleave="showTooltip = null">ⓘ</span> + <button type="button" + class="ml-2 text-sm text-gray-500 cursor-help" + aria-label="More information about {{.label}}" + @mouseenter="showTooltip = '{{.id}}'" + @mouseleave="showTooltip = null" + @focus="showTooltip = '{{.id}}'" + @blur="showTooltip = null">ⓘ</button>📝 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.<label class="label justify-start" for="{{.id}}"> <span class="label-text capitalize">{{.label}}</span> <button type="button" class="ml-2 text-sm text-gray-500 cursor-help" aria-label="More information about {{.label}}" @mouseenter="showTooltip = '{{.id}}'" @mouseleave="showTooltip = null" @focus="showTooltip = '{{.id}}'" @blur="showTooltip = null">ⓘ</button> </label>
23-35:
⚠️ Potential issueAdd input constraints and sanitize pattern attribute.
Since this component is used in security settings, we should:
- Add maxlength constraint to prevent buffer overflow
- Make the required attribute configurable
- Ensure pattern attribute is properly escaped
<input type="text" id="{{.id}}" :name="name" x-model="{{.model}}" :placeholder="placeholder" class="input input-sm input-bordered {{.class}}" :class="{ 'invalid': touched && !validInput() }" - x-ref="{{.id}}" + x-ref="{{.id}}" + maxlength="255" {{if .disabled}} x-bind:disabled="{{.disabled}}" {{end}} {{if .pattern}} - pattern="{{.pattern}}" + pattern="{{html .pattern}}" @blur="touched = true" @invalid="touched = true" @input="touched = false" - required + {{if .required}}required{{end}} {{end}}>📝 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.<input type="text" id="{{.id}}" :name="name" x-model="{{.model}}" :placeholder="placeholder" class="input input-sm input-bordered {{.class}}" :class="{ 'invalid': touched && !validInput() }" x-ref="{{.id}}" maxlength="255" {{if .disabled}} x-bind:disabled="{{.disabled}}" {{end}} {{if .pattern}} pattern="{{html .pattern}}" @blur="touched = true" @invalid="touched = true" @input="touched = false" {{if .required}}required{{end}} {{end}}>views/settings/templates/hostField.html (2)
11-15:
⚠️ Potential issueImprove host validation logic.
The current host validation has potential issues:
- It only strips http/https protocols but doesn't handle trailing slashes
- The comparison logic is inverted (returns true when hosts don't match)
validHost() { - const configHost = security.host.replace(/^https?:\/\//, ''); - const currentHost = location.host; - return configHost !== currentHost; + const configHost = security.host.replace(/^https?:\/\//, '').replace(/\/$/, ''); + const currentHost = location.host; + return configHost === currentHost; }📝 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.validHost() { const configHost = security.host.replace(/^https?:\/\//, '').replace(/\/$/, ''); const currentHost = location.host; return configHost === currentHost; },
29-35: 🛠️ Refactor suggestion
Enhance input field security and accessibility.
- The current regex pattern allows potentially unsafe host patterns
- Missing ARIA attributes for better accessibility
<input type="text" id="{{.id}}" :name="name" x-model="{{.model}}" :placeholder="placeholder" class="input input-sm input-bordered {{.class}}" :class="{ 'invalid': touched && !validInput() }" x-ref="{{.id}}" @blur="touched = true" {{if .disabled}} x-bind:disabled="{{.disabled}}" {{end}} - pattern="^(https?:\/\/)?(localhost|([a-zA-Z0-9](-*[a-zA-Z0-9])*\.)+[a-zA-Z]{2,}|(\d{1,3}\.){3}\d{1,3})(:\d{1,5})?$" + pattern="^(https?:\/\/)?(localhost|([a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}|(\d{1,3}\.){3}\d{1,3})(:\d{1,5})?$" @invalid="touched = true;" @input="touched = false;" - required> + required + aria-required="true" + aria-invalid="touched && !validInput()" + aria-describedby="{{.id}}-error">📝 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.<input type="text" id="{{.id}}" :name="name" x-model="{{.model}}" :placeholder="placeholder" class="input input-sm input-bordered {{.class}}" :class="{ 'invalid': touched && !validInput() }" x-ref="{{.id}}" @blur="touched = true" {{if .disabled}} x-bind:disabled="{{.disabled}}" {{end}} pattern="^(https?:\/\/)?(localhost|([a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}|(\d{1,3}\.){3}\d{1,3})(:\d{1,5})?$" @invalid="touched = true;" @input="touched = false;" required aria-required="true" aria-invalid="touched && !validInput()" aria-describedby="{{.id}}-error">Dockerfile (1)
67-69: 🛠️ Refactor suggestion
Consider more restrictive permissions for reset_auth.sh
The script has executable permissions for all users (via chmod +x). Since this is a security-sensitive script that can reset authentication settings, consider restricting permissions to root only.
Apply this diff to make the permissions more restrictive:
-RUN chmod +x /usr/bin/reset_auth.sh +RUN chmod 700 /usr/bin/reset_auth.sh📝 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.# Include reset_auth tool from build stage COPY --from=build /home/dev-user/src/BirdNET-Go/reset_auth.sh /usr/bin/ RUN chmod 700 /usr/bin/reset_auth.shviews/elements/callback.html (4)
19-41:
⚠️ Potential issueCritical: Remove client secret from frontend code.
Exposing client secrets in frontend code is a severe security vulnerability. The token exchange should be handled server-side.
Refactor the token exchange to use a server-side endpoint:
-function exchangeToken() { - const clientId = '{{.ClientID}}'; - const clientSecret = '{{.Secret}}'; - const redirectUri = window.location.origin + '{{.RedirectURL}}'; - - fetch('/oauth2/token', { - method: 'POST', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - 'Authorization': 'Basic ' + btoa(clientId + ':' + clientSecret) - }, - body: `grant_type=authorization_code&code={{.Code}}&redirect_uri=${encodeURIComponent(redirectUri)}` - }) +function exchangeToken() { + fetch('/oauth2/exchange', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'X-CSRF-Token': '{{.CSRFToken}}' + }, + body: JSON.stringify({ + code: '{{.Code}}', + state: '{{.State}}' + }) + })Committable suggestion skipped: line range outside the PR's diff.
1-10:
⚠️ Potential issueAdd security-related meta tags and CSP headers.
For OAuth callback pages, it's crucial to implement proper security headers to prevent XSS and other security vulnerabilities.
Add the following security-related meta tags:
<head> <meta charset="UTF-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0"> + <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self';"> + <meta http-equiv="X-Frame-Options" content="DENY"> + <meta http-equiv="X-Content-Type-Options" content="nosniff"> <title>BirdNET-Go</title>📝 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.{{define "callback"}} <!DOCTYPE html> <html lang="en"> <head> <meta charset="UTF-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0"> <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self';"> <meta http-equiv="X-Frame-Options" content="DENY"> <meta http-equiv="X-Content-Type-Options" content="nosniff"> <title>BirdNET-Go</title> <link href="/assets/tailwind.css" rel="stylesheet" /> <link href="/assets/custom.css" rel="stylesheet" />
32-41: 🛠️ Refactor suggestion
Improve error handling and user feedback.
The current error handling is basic and might not cover all edge cases.
Enhance error handling:
.then(response => response.json()) .then(data => { if (data.access_token) { window.location.href = redirectUri; } else { - showError(data.error); + showError(data.error_description || data.error || 'Authentication failed'); } }) - .catch(error => showError(error.message)); + .catch(error => { + console.error('Authentication error:', error); + showError('Network error occurred. Please try again.'); + });📝 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..then(response => response.json()) .then(data => { if (data.access_token) { window.location.href = redirectUri; } else { showError(data.error_description || data.error || 'Authentication failed'); } }) .catch(error => { console.error('Authentication error:', error); showError('Network error occurred. Please try again.'); }); }
24-31: 💡 Codebase verification
⚠️ Potential issueBased on the code analysis, I can now generate the final response:
CSRF and state parameter protection missing in OAuth flow
The OAuth2 implementation is missing critical security controls:
- No CSRF token validation in the token exchange endpoint
- No state parameter generation and validation in the authorization flow
- The callback endpoint (
/callback) accepts and passes the authorization code without any state validationKey vulnerable endpoints:
/oauth2/authorize- No state parameter generation/callback- No state parameter validation/oauth2/token- No CSRF protection🔗 Analysis chain
Add CSRF protection and state parameter validation.
The token exchange endpoint needs protection against CSRF attacks and must validate the state parameter to prevent CSRF attacks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if CSRF protection is implemented in the auth routes rg -l "csrf.Protect|csrf.Token" internal/httpcontroller/Length of output: 58
Script:
#!/bin/bash # Let's check for CSRF protection and state parameter validation in auth-related files rg -i "csrf|state.*param|antiforgery" --type html --type go -A 5 # Also check the OAuth2 callback route handler implementation rg -i "func.*callback|oauth.*callback" --type go -A 10Length of output: 2618
Script:
#!/bin/bash # Let's check the OAuth2 authorization endpoint implementation for state parameter rg -i "authorize|oauth.*auth" --type go -A 10 # Also check if there's any state parameter handling in the templates rg -i "state.*=|state.*param" --type htmlLength of output: 23866
views/settings/templates/passwordField.html (1)
17-45: 🛠️ Refactor suggestion
⚠️ Potential issueEnhance security and accessibility for password input.
The implementation needs some security and accessibility improvements:
- Update autocomplete attribute for better security:
- @blur="touched = true" autocomplete="off" required> + @blur="touched = true" autocomplete="new-password" required>
- Add proper ARIA labels to SVG icons:
- <svg x-show="!showPassword" xmlns="http://www.w3.org/2000/svg" class="h-5 w-5 text-gray-400" + <svg x-show="!showPassword" xmlns="http://www.w3.org/2000/svg" class="h-5 w-5 text-gray-400" + role="img" aria-label="Show password"- <svg x-show="showPassword" xmlns="http://www.w3.org/2000/svg" class="h-5 w-5 text-gray-400" + <svg x-show="showPassword" xmlns="http://www.w3.org/2000/svg" class="h-5 w-5 text-gray-400" + role="img" aria-label="Hide 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.<div class="relative w-full"> <!-- Input field --> <input class="input input-sm input-bordered w-full pr-10" :class="{ 'invalid': touched && !{{.model}} }" :type="showPassword ? 'text' : 'password'" :name="name" :placeholder="placeholder" id="{{.id}}" x-model="{{.model}}" x-ref="{{.id}}" @blur="touched = true" autocomplete="new-password" required> <!-- Toggle button --> <button type="button" @click="showPassword = !showPassword" aria-label="Toggle password visibility" :aria-pressed="showPassword" class="absolute inset-y-0 right-0 pr-3 flex items-center"> <svg x-show="!showPassword" xmlns="http://www.w3.org/2000/svg" class="h-5 w-5 text-gray-400" role="img" aria-label="Show password" viewBox="0 0 20 20" fill="currentColor"> <path d="M10 12a2 2 0 100-4 2 2 0 000 4z" /> <path fill-rule="evenodd" d="M.458 10C1.732 5.943 5.522 3 10 3s8.268 2.943 9.542 7c-1.274 4.057-5.064 7-9.542 7S1.732 14.057.458 10zM14 10a4 4 0 11-8 0 4 4 0 018 0z" clip-rule="evenodd" /> </svg> <svg x-show="showPassword" xmlns="http://www.w3.org/2000/svg" class="h-5 w-5 text-gray-400" role="img" aria-label="Hide password" viewBox="0 0 20 20" fill="currentColor"> <path fill-rule="evenodd" d="M3.707 2.293a1 1 0 00-1.414 1.414l14 14a1 1 0 001.414-1.414l-1.473-1.473A10.014 10.014 0 0019.542 10C18.268 5.943 14.478 3 10 3a9.958 9.958 0 00-4.512 1.074l-1.78-1.781zm4.261 4.26l1.514 1.515a2.003 2.003 0 012.45 2.45l1.514 1.514a4 4 0 00-5.478-5.478z" clip-rule="evenodd" /> <path d="M12.454 16.697L9.75 13.992a4 4 0 01-3.742-3.741L2.335 6.578A9.98 9.98 0 00.458 10c1.274 4.057 5.065 7 9.542 7 .847 0 1.669-.105 2.454-.303z" /> </svg> </button> </div>views/index.html (1)
9-10:
⚠️ Potential issueConsider accessibility implications of viewport restrictions.
The viewport meta tag includes
user-scalable=nowhich prevents users from zooming the page. This can create accessibility issues for users who rely on zoom functionality, particularly those with visual impairments.Consider removing these restrictions:
- content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=no, viewport-fit=cover"> + content="width=device-width, initial-scale=1, viewport-fit=cover">📝 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.<meta name="viewport" content="width=device-width, initial-scale=1, viewport-fit=cover">doc/security.md (4)
75-87: 🛠️ Refactor suggestion
Enhance authentication recovery documentation
The Authentication Recovery section should include:
- Access control for the reset script
- Audit logging recommendations
- Steps to re-enable authentication
Add the following content:
The script automatically creates a timestamped backup of your current configuration before disabling the authentication. + +> **Important:** +> - Restrict access to the reset script to authorized administrators only +> - Monitor and log all authentication resets +> - After using the reset script: +> 1. Login to the web interface immediately +> 2. Configure new authentication settings +> 3. Test the new configuration +> 4. Review security logs📝 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.## Authentication Recovery If you end up locking yourself out, authentication can be turned off with the following command: ```bash # For host system installations ./reset_auth.sh [path/to/config.yaml] # For Docker deployments docker exec $(docker ps | grep birdnet-go | awk '{print $1}') reset_auth.sh ``` The script automatically creates a timestamped backup of your current configuration before disabling the authentication. > **Important:** > - Restrict access to the reset script to authorized administrators only > - Monitor and log all authentication resets > - After using the reset script: > 1. Login to the web interface immediately > 2. Configure new authentication settings > 3. Test the new configuration > 4. Review security logs
7-20: 🛠️ Refactor suggestion
Enhance security guidance for Basic Authentication
The Basic Authentication section should include important security considerations:
- Password requirements (minimum length, complexity)
- HTTPS requirement for secure transmission
- Best practices for storing sensitive configuration
Add the following content before the YAML example:
Basic password authentication follows the OAuth2.0 specification. It uses merely a password to authenticate a user. If no client id or secret is provided, they will be created automatically. + +> **Security Notice:** +> - Use a strong password (minimum 12 characters, mix of letters, numbers, symbols) +> - Always use HTTPS in production to protect credentials +> - Avoid storing sensitive credentials in version control +> - Consider using environment variables for sensitive values📝 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.### Basic Password Authentication Basic password authentication follows the OAuth2.0 specification. It uses merely a password to authenticate a user. If no client id or secret is provided, they will be created automatically. > **Security Notice:** > - Use a strong password (minimum 12 characters, mix of letters, numbers, symbols) > - Always use HTTPS in production to protect credentials > - Avoid storing sensitive credentials in version control > - Consider using environment variables for sensitive values ```yaml security: host: "https://your.domain.com" basicauth: enabled: true password: "your-password" redirecturi: "https://your.domain.com" clientid: "your-client-id" clientsecret: "your-client-secret" ```
22-46: 🛠️ Refactor suggestion
Enhance OAuth2 security documentation
The Social Authentication section should include additional security guidance:
- Required OAuth scopes
- Security implications of different scopes
- Alternative authorization strategies
Add the following content before the YAML examples:
BirdNET-Go supports OAuth authentication through Google and GitHub identity providers. To implement either provider, you'll need to generate the corresponding client ID and secret, then configure them through the Security settings or in the configuration file. Remember to set the Redirect URI parameter in your Google or GitHub developer console to match the value configured in `redirecturi`. The `userid` is a list of accepted authenticated user emails. + +> **Important Security Considerations:** +> - Request minimal OAuth scopes needed for your use case +> - For Google OAuth: typically only `email` and `profile` scopes are needed +> - For GitHub OAuth: `read:user` and `user:email` scopes are recommended +> - Consider implementing organization-based restrictions for GitHub +> - Store OAuth credentials securely using environment variables📝 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.### Social Authentication BirdNET-Go supports OAuth authentication through Google and GitHub identity providers. To implement either provider, you'll need to generate the corresponding client ID and secret, then configure them through the Security settings or in the configuration file. Remember to set the Redirect URI parameter in your Google or GitHub developer console to match the value configured in `redirecturi`. The `userid` is a list of accepted authenticated user emails. > **Important Security Considerations:** > - Request minimal OAuth scopes needed for your use case > - For Google OAuth: typically only `email` and `profile` scopes are needed > - For GitHub OAuth: `read:user` and `user:email` scopes are recommended > - Consider implementing organization-based restrictions for GitHub > - Store OAuth credentials securely using environment variables ```yaml security: googleauth: enabled: true clientid: "your-google-client-id" clientsecret: "your-google-client-secret" userid: "allowed@gmail.com,another@gmail.com" redirecturi: "https://your.domain.com/auth/google/callback" ``` Similarly, GitHub authentication can be enabled: ```yaml security: githubauth: enabled: true clientid: "your-github-client-id" clientsecret: "your-github-client-secret" userid: "user@example.com" redirecturi: "https://your.domain.com/auth/github/callback" ```
48-74:
⚠️ Potential issueAdd security warnings for authentication bypass features
The Authentication Bypass section should emphasize security implications and best practices:
- Risks of subnet-based bypass
- Proper Cloudflare Access configuration
- JWT token validation
Add security warnings:
If you are running BirdNET-Go on a trusted network, you can bypass authentication either by configuring a Cloudflare Tunnel with Cloudflare Access enabled, or by specifying a trusted subnet. Both options will allow access to the application without any authentication. + +> **⚠️ Security Warning:** +> Authentication bypass features should be used with extreme caution: +> - Subnet bypass should only be enabled on secure, isolated networks +> - Regular audits of allowed subnets are recommended +> - Consider using VPN instead of subnet bypass for remote access +> - When using Cloudflare Access: +> - Configure allowed users/groups in Cloudflare dashboard +> - Enable Multi-Factor Authentication (MFA) +> - Regularly rotate access credentials📝 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.## Authentication Bypass If you are running BirdNET-Go on a trusted network, you can bypass authentication either by configuring a Cloudflare Tunnel with Cloudflare Access enabled, or by specifying a trusted subnet. Both options will allow access to the application without any authentication. > **⚠️ Security Warning:** > Authentication bypass features should be used with extreme caution: > - Subnet bypass should only be enabled on secure, isolated networks > - Regular audits of allowed subnets are recommended > - Consider using VPN instead of subnet bypass for remote access > - When using Cloudflare Access: > - Configure allowed users/groups in Cloudflare dashboard > - Enable Multi-Factor Authentication (MFA) > - Regularly rotate access credentials Both options can be configured through the web interface or in the `config.yaml` file: ```yaml security: allowcftunnelbypass: true allowsubnetbypass: enabled: true subnet: "192.168.1.0/24,10.0.0.0/8" ``` ### Cloudflare Access Authentication Bypass Cloudflare Access provides an authentication layer that uses your existing identity providers, such as Google or GitHub accounts, to control access to your applications. When using Cloudflare Access for authentication, you can configure BirdNET-Go to trust traffic coming through the Cloudflare tunnel. The system authenticates requests by validating the `Cf-Access-Jwt-Assertion` header containing a JWT token from Cloudflare. - [Cloudflare tunnels](https://developers.cloudflare.com/cloudflare-one/connections/connect-apps/) - [Create a remotely-managed tunnel](https://developers.cloudflare.com/cloudflare-one/connections/connect-networks/get-started/create-remote-tunnel/) - [Self-hosted applications](https://developers.cloudflare.com/cloudflare-one/applications/configure-apps/self-hosted-apps/) ### Subnet-based Authentication Bypass When enabled, BirdNET-Go will allow access to the application without any authentication if the client's IP address is within the specified subnet. Home routers typically use `192.168.1.0/24`, `192.168.0.0/24` or `172.16.0.0/24`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~68-~68: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: .../connections/connect-apps/) - [Create a remotely-managed tunnel](https://developers.cloudflare.c...(HYPHENATED_LY_ADVERB_ADJECTIVE)
views/elements/login.html (5)
51-54: 🛠️ Refactor suggestion
Enhance social login security and accessibility.
The social login buttons need security improvements and accessibility enhancements.
- <a href="/auth/google" class="btn btn-primary grow pr-10" onclick="showSpinner('googleSpinner')"> + <a href="/auth/google" class="btn btn-primary grow pr-10" + onclick="showSpinner('googleSpinner')" + aria-label="Login with Google" + data-state="{{.StateToken}}"> <span id="googleSpinner" class="invisible loading loading-spinner"></span> Login with Google </a>Similar changes should be applied to the GitHub login button.
Also applies to: 57-60
26-26: 🛠️ Refactor suggestion
Improve error message accessibility and styling.
The error message div needs accessibility improvements and proper styling.
- <div id="loginError" class="text-red-700 relative hidden" role="alert"></div> + <div id="loginError" class="text-red-700 relative hidden" role="alert" aria-live="polite"></div>📝 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.<div id="loginError" class="text-red-700 relative hidden" role="alert" aria-live="polite"></div>
3-4:
⚠️ Potential issueAdd CSRF protection to the login form.
The form submission lacks CSRF protection which is critical for login endpoints.
Add CSRF token to the form:
- <form id="loginForm" hx-post="/login" hx-on:submit="showError(false)"> + <form id="loginForm" hx-post="/login" hx-on:submit="showError(false)"> + <input type="hidden" name="csrf_token" value="{{.CSRFToken}}">Committable suggestion skipped: line range outside the PR's diff.
103-111: 🛠️ Refactor suggestion
Improve error handling in HTMX event listener.
The event listener needs better error handling and race condition prevention.
document.body.addEventListener('htmx:afterSettle', function (event) { if (event.detail.elt.id === 'loginResult' && event.detail.xhr.status === 200) { - setTimeout(closeLoginModal, 2000) + const modalTimeout = setTimeout(closeLoginModal, 2000); + // Clear timeout if modal is closed manually + document.getElementById('loginModal').addEventListener('close', () => { + clearTimeout(modalTimeout); + }, { once: true }); } + // Handle error cases + if (event.detail.xhr.status !== 200) { + showError('Login failed. Please try again.'); + } if (event.detail.target.id === 'loginModal') { document.getElementById('loginPassword')?.focus(); } })Committable suggestion skipped: line range outside the PR's diff.
92-95:
⚠️ Potential issueFix error message visibility toggle.
The error message visibility toggle uses incorrect style property.
- errorBanner.style.display = 'hidden'; + errorBanner.classList.add('hidden');Committable suggestion skipped: line range outside the PR's diff.
internal/httpcontroller/template_renderers.go (1)
92-96: 🛠️ Refactor suggestion
Enhance authentication route handling.
The current implementation has several limitations:
- Hard-coded "/login" path could be a maintenance issue
- Doesn't handle other authentication routes (OAuth callbacks, logout, etc.)
Consider this improvement:
- // Is a login route, set isLoginRoute to true - isLoginRoute := path == "/login" - - if !isPageRoute && !isFragment && !isLoginRoute { + // Check if this is an authentication-related route + var authRoutes = map[string]bool{ + "/login": true, + "/logout": true, + "/auth/google": true, + "/auth/github": true, + "/auth/callback": true, + } + + isAuthRoute := authRoutes[path] + + if !isPageRoute && !isFragment && !isAuthRoute {This approach:
- Makes auth routes maintainable in one place
- Covers all authentication endpoints
- Makes it easier to add new auth providers
📝 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.// Check if this is an authentication-related route var authRoutes = map[string]bool{ "/login": true, "/logout": true, "/auth/google": true, "/auth/github": true, "/auth/callback": true, } isAuthRoute := authRoutes[path] if !isPageRoute && !isFragment && !isAuthRoute {internal/conf/config.yaml (2)
134-141:
⚠️ Potential issueSecurity consideration: Document password hashing and secret management.
For the basic auth section:
- Add a comment specifying the expected password hash format
- Document how the auto-generated client secret is stored securely
- Consider adding a minimum length requirement for the client secret
130-133: 💡 Codebase verification
⚠️ Potential issueAdd CIDR format hint to subnet configuration comment
The codebase already includes subnet validation in
internal/conf/validate.gothat enforces CIDR notation, but this isn't clear from the configuration file. Update the comment to specify:allowsubnetbypass: enabled: false # true to disable OAuth in subnet subnet: "" # comma-separated list of CIDR ranges (e.g., "192.168.1.0/24,10.0.0.0/8")🔗 Analysis chain
Security consideration: Document subnet bypass format.
The subnet bypass feature could pose a security risk if misconfigured. Consider adding a comment specifying the expected subnet format (e.g., CIDR notation "192.168.1.0/24").
Let's check if there's any existing documentation about this:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for subnet format documentation rg -i "subnet.*format|cidr|network.*mask" README.md docs/Length of output: 105
Script:
#!/bin/bash # Let's search for subnet-related code and documentation more broadly # First, find all relevant files fd -t f -e md -e txt # Then search for subnet validation or parsing code rg -i "subnet|cidr" --type go -A 5 # Also check for any configuration validation code ast-grep --pattern 'func $_(config $_ Config) $_'Length of output: 5228
views/settings/settingsBase.html (2)
159-163: 🛠️ Refactor suggestion
Enhance notification accessibility.
The notification alerts lack proper ARIA attributes for screen readers.
Add appropriate ARIA attributes:
-<div x-show="!notification.removing" :class="{ +<div x-show="!notification.removing" + role="alert" + aria-live="polite" + :class="{ 'alert-success': notification.type === 'success', 'alert-error': notification.type === 'error', 'alert-info': notification.type === 'info' }" class="alert"> <div class="flex items-start"> <svg x-show="notification.type === 'success'" xmlns="http://www.w3.org/2000/svg" + aria-label="Success" class="stroke-current flex-shrink-0 h-6 w-6 mr-2" fill="none" viewBox="0 0 24 24"> <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M9 12l2 2 4-4m6 2a9 9 0 11-18 0 9 9 0 0118 0z" /> </svg> <svg x-show="notification.type === 'error'" xmlns="http://www.w3.org/2000/svg" + aria-label="Error" class="stroke-current flex-shrink-0 h-6 w-6 mr-2" fill="none" viewBox="0 0 24 24"> <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M10 14l2-2m0 0l2-2m-2 2l-2-2m2 2l2 2m7-2a9 9 0 11-18 0 9 9 0 0118 0z" /> </svg> <svg x-show="notification.type === 'info'" xmlns="http://www.w3.org/2000/svg" fill="none" + aria-label="Information" viewBox="0 0 24 24" class="stroke-current flex-shrink-0 w-6 h-6 mr-2"> <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M13 16h-1v-4h-1m1-4h.01M21 12a9 9 0 11-18 0 9 9 0 0118 0z"></path> </svg>Also applies to: 165-179
85-102: 🛠️ Refactor suggestion
Consider using Alpine.js refs instead of direct DOM queries.
The form validation logic directly manipulates DOM elements, which could be better handled using Alpine.js refs or
$refs.Here's a more Alpine.js-centric approach:
isFormValid(form) { - const visibleFieldsValid = Array.from(form.elements) - .filter(element => element.offsetParent !== null) - .every(element => element.checkValidity()); + const visibleFieldsValid = Array.from(this.$refs.form.elements) + .filter(element => element.offsetParent !== null) + .every(element => element.checkValidity()); - const inputSelector = 'input[type=\'password\'], input[type=\'text\']'; - if(!visibleFieldsValid) { - form.querySelectorAll(inputSelector).forEach(input => { - if (input.offsetParent === null) return; + if (!visibleFieldsValid) { + this.$refs.form.querySelectorAll('input').forEach(input => { + if (input.offsetParent === null || !['password', 'text'].includes(input.type)) return; input.checkValidity(); if (!input.validity.valid) input.dispatchEvent(new Event('blur')); }); } return visibleFieldsValid; }Committable suggestion skipped: line range outside the PR's diff.
internal/conf/defaults.go (1)
196-203: 🛠️ Refactor suggestion
Improve error handling and documentation in GenerateRandomSecret.
While the implementation is secure, consider:
- Return an error instead of panicking, as this is a configuration utility.
- Add documentation about the output format and length.
-// Generate a secure random string for the client secret. +// GenerateRandomSecret generates a URL-safe base64 encoded random string +// suitable for use as a client secret. The output is 43 characters long, +// providing 256 bits of entropy. func GenerateRandomSecret() string { - // Use crypto/rand for secure random generation bytes := make([]byte, 32) if _, err := rand.Read(bytes); err != nil { - panic(err) + // Log the error and return a safe fallback or empty string + log.Printf("Failed to generate random secret: %v", err) + return "" } return base64.RawURLEncoding.EncodeToString(bytes) }📝 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.// GenerateRandomSecret generates a URL-safe base64 encoded random string // suitable for use as a client secret. The output is 43 characters long, // providing 256 bits of entropy. func GenerateRandomSecret() string { bytes := make([]byte, 32) if _, err := rand.Read(bytes); err != nil { // Log the error and return a safe fallback or empty string log.Printf("Failed to generate random secret: %v", err) return "" } return base64.RawURLEncoding.EncodeToString(bytes) }internal/conf/validate.go (1)
150-163: 🛠️ Refactor suggestion
Enhance port validation for security.
While the basic validation is present, consider adding more comprehensive port validation:
- Validate port number range (0-65535)
- Check for privileged ports (0-1023)
- Validate port string format
func validateWebServerSettings(settings *struct { Enabled bool Port string Log LogConfig }) error { if settings.Enabled { if settings.Port == "" { return errors.New("WebServer port is required when enabled") } + // Validate port number + portNum, err := strconv.Atoi(settings.Port) + if err != nil { + return fmt.Errorf("invalid port format: %w", err) + } + if portNum < 0 || portNum > 65535 { + return fmt.Errorf("port number must be between 0 and 65535") + } + if portNum < 1024 { + log.Printf("Warning: Using privileged port %d", portNum) + } } 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.Enabled bool Port string Log LogConfig }) error { if settings.Enabled { // Check if port is provided when enabled if settings.Port == "" { return errors.New("WebServer port is required when enabled") } // Validate port number portNum, err := strconv.Atoi(settings.Port) if err != nil { return fmt.Errorf("invalid port format: %w", err) } if portNum < 0 || portNum > 65535 { return fmt.Errorf("port number must be between 0 and 65535") } if portNum < 1024 { log.Printf("Warning: Using privileged port %d", portNum) } } return nil }internal/security/basic_test.go (7)
252-300: 🛠️ Refactor suggestion
Remove redundant test case.
TestHandleBasicAuthTokenValidAuthorizationCodeAndRedirectURIis nearly identical toTestHandleBasicAuthTokenSuccess. Consider removing this test and enhancingTestHandleBasicAuthTokenSuccesswith any additional validations needed.
209-250: 🛠️ Refactor suggestion
Fix inconsistent request setup and enhance error testing.
The test has two issues:
- Uses
SetParamNames/Valuesinstead of form data (inconsistent with other tests)- Should test each missing field individually
-func TestHandleBasicAuthTokenMissingFields(t *testing.T) { +func TestHandleBasicAuthTokenValidation(t *testing.T) { + tests := []struct { + name string + grantType string + code string + redirectURI string + wantError string + }{ + { + name: "missing grant_type", + code: "validCode", + redirectURI: "http://example.com/callback", + wantError: "invalid_request", + }, + { + name: "missing code", + grantType: "authorization_code", + redirectURI: "http://example.com/callback", + wantError: "invalid_request", + }, + { + name: "missing redirect_uri", + grantType: "authorization_code", + code: "validCode", + wantError: "invalid_request", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + e := echo.New() + formData := url.Values{} + if tt.grantType != "" { + formData.Set("grant_type", tt.grantType) + } + if tt.code != "" { + formData.Set("code", tt.code) + } + if tt.redirectURI != "" { + formData.Set("redirect_uri", tt.redirectURI) + } + + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(formData.Encode())) + req.Header.Set(echo.HeaderContentType, "application/x-www-form-urlencoded") + req.Header.Set(echo.HeaderAuthorization, "Basic "+base64.StdEncoding.EncodeToString([]byte("validClientID:validClientSecret"))) + + // ... rest of the test implementation + }) + } +}Committable suggestion skipped: line range outside the PR's diff.
159-207: 🛠️ Refactor suggestion
Enhance token response validation per OAuth2 spec.
The test should validate the token response according to OAuth2 specification (RFC 6749):
- Verify token format and expiry
- Validate token_type (should be "Bearer")
- Consider adding refresh_token support
func TestHandleBasicAuthTokenSuccess(t *testing.T) { // ... existing setup ... - var response map[string]string + var response struct { + AccessToken string `json:"access_token"` + TokenType string `json:"token_type"` + ExpiresIn int `json:"expires_in"` + RefreshToken string `json:"refresh_token,omitempty"` + } if err := json.Unmarshal(rec.Body.Bytes(), &response); err != nil { t.Fatalf("failed to unmarshal response: %v", err) } - if response["access_token"] == "" { + if response.AccessToken == "" { t.Error("expected access token in response") } + + if response.TokenType != "Bearer" { + t.Errorf("expected token_type 'Bearer', got %s", response.TokenType) + } + + if response.ExpiresIn <= 0 { + t.Error("expected positive expires_in value") + } + + // Verify token is stored with correct expiry + token, exists := s.accessTokens[response.AccessToken] + if !exists { + t.Error("access token not found in server storage") + } + + expectedExpiry := time.Now().Add(time.Duration(response.ExpiresIn) * time.Second) + if !token.ExpiresAt.Round(time.Second).Equal(expectedExpiry.Round(time.Second)) { + t.Error("token expiry time mismatch") + } }Committable suggestion skipped: line range outside the PR's diff.
51-87: 🛠️ Refactor suggestion
Enhance error handling test coverage per OAuth2 spec.
The test should validate error responses according to OAuth2 specification (RFC 6749):
- Add test case for invalid redirect_uri
- Verify error response format includes
erroranderror_descriptionfieldsfunc TestHandleBasicAuthorizeInvalidClientID(t *testing.T) { // ... existing setup ... - expectedBody := "Invalid client_id" - if rec.Body.String() != expectedBody { - t.Errorf("unexpected response body: got %s, want %s", rec.Body.String(), expectedBody) + var response struct { + Error string `json:"error"` + ErrorDescription string `json:"error_description"` + } + if err := json.NewDecoder(rec.Body).Decode(&response); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + + if response.Error != "invalid_client" { + t.Errorf("expected error 'invalid_client', got %s", response.Error) } } + +func TestHandleBasicAuthorizeInvalidRedirectURI(t *testing.T) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/?client_id=validClientID&redirect_uri=http://invalid.redirect", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + server := &OAuth2Server{ + Settings: &conf.Settings{ + Security: conf.Security{ + BasicAuth: conf.BasicAuth{ + ClientID: "validClientID", + RedirectURI: "http://valid.redirect", + }, + }, + }, + } + + server.HandleBasicAuthorize(c) + + if rec.Code != http.StatusBadRequest { + t.Errorf("expected status %d, got %d", http.StatusBadRequest, rec.Code) + } + + var response struct { + Error string `json:"error"` + ErrorDescription string `json:"error_description"` + } + if err := json.NewDecoder(rec.Body).Decode(&response); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + + if response.Error != "invalid_request" { + t.Errorf("expected error 'invalid_request', got %s", response.Error) + } +}Committable suggestion skipped: line range outside the PR's diff.
1-300: 🛠️ Refactor suggestion
Add missing test cases for comprehensive OAuth2 coverage.
Consider adding these critical test cases:
- Expired authorization code handling
- Prevention of authorization code reuse
- Invalid client credentials in token request
- Invalid grant types
- Rate limiting (if implemented)
+func TestHandleBasicAuthTokenExpiredCode(t *testing.T) { + // Test with expired auth code +} + +func TestHandleBasicAuthTokenCodeReuse(t *testing.T) { + // Test prevention of auth code reuse +} + +func TestHandleBasicAuthTokenInvalidClientCredentials(t *testing.T) { + // Test invalid client authentication +} + +func TestHandleBasicAuthTokenInvalidGrantType(t *testing.T) { + // Test unsupported grant types +}Committable suggestion skipped: line range outside the PR's diff.
89-157: 🛠️ Refactor suggestion
Consolidate redundant test cases.
TestHandleBasicAuthorizeAuthCodeGenerationandTestHandleBasicAuthorizeValidParametersare nearly identical toTestHandleBasicAuthorizeSuccess. Consider consolidating these tests into a single comprehensive test case with table-driven tests.-func TestHandleBasicAuthorizeAuthCodeGeneration(t *testing.T) {...} -func TestHandleBasicAuthorizeValidParameters(t *testing.T) {...} +func TestHandleBasicAuthorize(t *testing.T) { + tests := []struct { + name string + clientID string + redirectURI string + state string + expectedCode int + expectedError string + validateCode bool + }{ + { + name: "success", + clientID: "validClientID", + redirectURI: "http://valid.redirect", + state: "randomState123", + expectedCode: http.StatusFound, + validateCode: true, + }, + { + name: "invalid client_id", + clientID: "invalidClientID", + redirectURI: "http://valid.redirect", + expectedCode: http.StatusBadRequest, + expectedError: "invalid_client", + }, + // Add more test cases + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test implementation + }) + } +}Committable suggestion skipped: line range outside the PR's diff.
16-49: 🛠️ Refactor suggestion
Enhance test coverage for authorization flow.
While the basic flow is tested, consider adding these important validations:
- Verify the format and length of the generated auth code
- Confirm the auth code is properly stored in server's authCodes map
- Add tests for state parameter handling (CSRF protection)
func TestHandleBasicAuthorizeSuccess(t *testing.T) { e := echo.New() - req := httptest.NewRequest(http.MethodGet, "/?client_id=validClientID&redirect_uri=http://valid.redirect", nil) + state := "randomState123" + req := httptest.NewRequest(http.MethodGet, "/?client_id=validClientID&redirect_uri=http://valid.redirect&state="+state, nil) rec := httptest.NewRecorder() c := e.NewContext(req, rec) server := &OAuth2Server{ // ... existing setup ... } err := server.HandleBasicAuthorize(c) if err != nil { t.Fatalf("HandleBasicAuthorize returned an error: %v", err) } location := rec.Header().Get("Location") if !strings.HasPrefix(location, "http://valid.redirect?code=") { t.Errorf("unexpected redirect location: %s", location) } + + // Extract auth code from location + code := strings.TrimPrefix(location, "http://valid.redirect?code=") + code = strings.Split(code, "&")[0] + + // Verify auth code format and length + if len(code) < 32 { + t.Errorf("auth code length should be at least 32 characters, got %d", len(code)) + } + + // Verify auth code is stored + if _, exists := server.authCodes[code]; !exists { + t.Error("auth code not found in server storage") + } + + // Verify state parameter is preserved + if !strings.Contains(location, "state="+state) { + t.Error("state parameter not preserved in redirect") + } }📝 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.// Correct client_id and redirect_uri result in redirection with auth code func TestHandleBasicAuthorizeSuccess(t *testing.T) { e := echo.New() state := "randomState123" req := httptest.NewRequest(http.MethodGet, "/?client_id=validClientID&redirect_uri=http://valid.redirect&state="+state, nil) rec := httptest.NewRecorder() c := e.NewContext(req, rec) server := &OAuth2Server{ Settings: &conf.Settings{ Security: conf.Security{ BasicAuth: conf.BasicAuth{ ClientID: "validClientID", RedirectURI: "http://valid.redirect", }, }, }, authCodes: make(map[string]AuthCode), accessTokens: make(map[string]AccessToken), } err := server.HandleBasicAuthorize(c) if err != nil { t.Fatalf("HandleBasicAuthorize returned an error: %v", err) } if rec.Code != http.StatusFound { t.Errorf("expected status %d, got %d", http.StatusFound, rec.Code) } location := rec.Header().Get("Location") if !strings.HasPrefix(location, "http://valid.redirect?code=") { t.Errorf("unexpected redirect location: %s", location) } // Extract auth code from location code := strings.TrimPrefix(location, "http://valid.redirect?code=") code = strings.Split(code, "&")[0] // Verify auth code format and length if len(code) < 32 { t.Errorf("auth code length should be at least 32 characters, got %d", len(code)) } // Verify auth code is stored if _, exists := server.authCodes[code]; !exists { t.Error("auth code not found in server storage") } // Verify state parameter is preserved if !strings.Contains(location, "state="+state) { t.Error("state parameter not preserved in redirect") } }views/settings/integrationSettings.html (2)
231-234: 🛠️ Refactor suggestion
Add URL validation for the endpoint field.
Consider adding URL validation to ensure the endpoint is properly formatted and secure.
- <input type="text" id="openWeatherEndpoint" name="realtime.openweather.endpoint" - x-model="openWeather.endpoint" class="input input-bordered input-sm w-full"> + <input type="url" id="openWeatherEndpoint" name="realtime.openweather.endpoint" + x-model="openWeather.endpoint" + pattern="https?:\/\/.+" + title="Please enter a valid HTTP/HTTPS URL" + class="input input-bordered input-sm w-full">📝 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.<input type="url" id="openWeatherEndpoint" name="realtime.openweather.endpoint" x-model="openWeather.endpoint" pattern="https?:\/\/.+" title="Please enter a valid HTTP/HTTPS URL" class="input input-bordered input-sm w-full"> <div x-show="showTooltip === 'openWeatherEndpoint'" x-cloak class="absolute left-0 bottom-full mb-2 p-2 bg-gray-100 text-sm rounded shadow-md z-50">
489-493: 🛠️ Refactor suggestion
Add validation for the listen address field.
The listen address field should validate the format to ensure it's a valid network address and port combination.
- <input type="text" id="telemetryListen" name="realtime.telemetry.listen" x-model="telemetry.listen" - class="input input-bordered input-sm w-full"> + <input type="text" id="telemetryListen" name="realtime.telemetry.listen" x-model="telemetry.listen" + pattern="^(?:[0-9]{1,3}\.){3}[0-9]{1,3}:[0-9]{1,5}$" + title="Please enter a valid IP address and port (e.g., 0.0.0.0:8090)" + placeholder="0.0.0.0:8090" + class="input input-bordered input-sm w-full">📝 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.<input type="text" id="telemetryListen" name="realtime.telemetry.listen" x-model="telemetry.listen" pattern="^(?:[0-9]{1,3}\.){3}[0-9]{1,3}:[0-9]{1,5}$" title="Please enter a valid IP address and port (e.g., 0.0.0.0:8090)" placeholder="0.0.0.0:8090" class="input input-bordered input-sm w-full"> <div x-show="showTooltip === 'telemetryListen'" x-cloak class="absolute left-0 bottom-full mb-2 p-2 bg-gray-100 text-sm rounded shadow-md z-50"> The IP address and port to listen on (e.g., 0.0.0.0:8090).views/settings/mainSettings.html (4)
627-628: 🛠️ Refactor suggestion
Add validation for MySQL port.
The MySQL port input should be validated to ensure it's within the valid range (1-65535).
-<input type="text" id="mysqlPort" x-model="mysql.port" name="output.mysql.port" - class="input input-sm input-bordered" placeholder="Enter MySQL port"> +<input type="number" id="mysqlPort" x-model="mysql.port" name="output.mysql.port" + class="input input-sm input-bordered" placeholder="Enter MySQL port" + min="1" max="65535" required + x-on:input="$el.value = Math.min(65535, Math.max(1, $el.value))">📝 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.<input type="number" id="mysqlPort" x-model="mysql.port" name="output.mysql.port" class="input input-sm input-bordered" placeholder="Enter MySQL port" min="1" max="65535" required x-on:input="$el.value = Math.min(65535, Math.max(1, $el.value))">
50-51: 🛠️ Refactor suggestion
Enhance security for node name input.
The node name input should be sanitized to prevent XSS attacks and invalid characters.
-<input type="text" id="nodeName" name="main.name" x-model="main.name" - class="input input-sm input-bordered" placeholder="Enter node name"> +<input type="text" id="nodeName" name="main.name" x-model="main.name" + class="input input-sm input-bordered" placeholder="Enter node name" + pattern="[a-zA-Z0-9_-]+" required + x-on:input="$el.value = $el.value.replace(/[^a-zA-Z0-9_-]/g, '')">📝 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.<input type="text" id="nodeName" name="main.name" x-model="main.name" class="input input-sm input-bordered" placeholder="Enter node name" pattern="[a-zA-Z0-9_-]+" required x-on:input="$el.value = $el.value.replace(/[^a-zA-Z0-9_-]/g, '')">
281-282:
⚠️ Potential issueSecure model path input against path traversal.
The model path input needs validation to prevent directory traversal attacks and ensure only valid model files are loaded.
-<input type="text" id="birdnetModelPath" name="birdnet.modelpath" x-model="birdnet.modelPath" - class="input input-bordered input-sm w-full"> +<input type="text" id="birdnetModelPath" name="birdnet.modelpath" x-model="birdnet.modelPath" + class="input input-bordered input-sm w-full" + pattern="^[a-zA-Z0-9_\-./]+\.(tflite|pb)$" + x-on:input="$el.value = $el.value.replace(/[^a-zA-Z0-9_\-./]/g, '')" + title="Only .tflite or .pb files are allowed">📝 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.<input type="text" id="birdnetModelPath" name="birdnet.modelpath" x-model="birdnet.modelPath" class="input input-bordered input-sm w-full" pattern="^[a-zA-Z0-9_\-./]+\.(tflite|pb)$" x-on:input="$el.value = $el.value.replace(/[^a-zA-Z0-9_\-./]/g, '')" title="Only .tflite or .pb files are allowed">
592-593:
⚠️ Potential issueSecure SQLite database path.
The SQLite database path should be validated to prevent potential security issues.
-<input type="text" id="sqlitePath" x-model="sqlite.path" name="output.sqlite.path" - class="input input-sm input-bordered flex-grow" placeholder="Enter SQLite database path"> +<input type="text" id="sqlitePath" x-model="sqlite.path" name="output.sqlite.path" + class="input input-sm input-bordered flex-grow" placeholder="Enter SQLite database path" + pattern="^[a-zA-Z0-9_\-./]+\.db$" + x-on:input="$el.value = $el.value.replace(/[^a-zA-Z0-9_\-./]/g, '')" + title="Only .db files are allowed">📝 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.<input type="text" id="sqlitePath" x-model="sqlite.path" name="output.sqlite.path" class="input input-sm input-bordered flex-grow" placeholder="Enter SQLite database path" pattern="^[a-zA-Z0-9_\-./]+\.db$" x-on:input="$el.value = $el.value.replace(/[^a-zA-Z0-9_\-./]/g, '')" title="Only .db files are allowed">internal/httpcontroller/middleware.go (1)
68-73:
⚠️ Potential issueHandle redirection securely and correctly
When redirecting unauthenticated users, ensure that the redirect URL is properly URL-encoded to handle special characters and prevent redirection vulnerabilities.
Apply the following changes to URL-encode the redirect paths:
+import ( + "net/url" + // other imports +) ... if c.Request().Header.Get("HX-Request") == "true" { - c.Response().Header().Set("HX-Redirect", "/login?redirect="+c.Request().URL.Path) + redirectPath := url.QueryEscape(c.Request().URL.Path) + c.Response().Header().Set("HX-Redirect", "/login?redirect="+redirectPath) return c.String(http.StatusUnauthorized, "") } ... - return c.Redirect(http.StatusFound, "/login?redirect="+c.Request().URL.Path) + redirectPath := url.QueryEscape(c.Request().URL.Path) + return c.Redirect(http.StatusFound, "/login?redirect="+redirectPath)Committable suggestion skipped: line range outside the PR's diff.
internal/security/basic.go (4)
84-89:
⚠️ Potential issueAvoid exposing
ClientSecretto the client to prevent security risks.In
HandleBasicAuthCallback, theClientSecretis being sent to the client within the rendered template. Exposing theClientSecretcompromises the security of your application and can lead to unauthorized access.Apply this diff to remove the
ClientSecretfrom the response:84 return c.Render(http.StatusOK, "callback", map[string]interface{}{ 85 "Code": code, 86 "RedirectURL": redirect, 87 "ClientID": s.Settings.Security.BasicAuth.ClientID, 88- "Secret": s.Settings.Security.BasicAuth.ClientSecret, 89 })📝 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.return c.Render(http.StatusOK, "callback", map[string]interface{}{ "Code": code, "RedirectURL": redirect, "ClientID": s.Settings.Security.BasicAuth.ClientID, })
55-57:
⚠️ Potential issueEnsure strict validation of
redirectURIto prevent open redirect vulnerabilities.Using
strings.Containsto validateredirectURIis insufficient, as an attacker could craft a malicious URL containing the expected host. This could lead to security breaches.Replace the current validation with exact host matching:
54 // Verify redirect URI 55- if !strings.Contains(redirectURI, s.Settings.Security.Host) { 56+ parsedURI, err := url.Parse(redirectURI) 57+ if err != nil || parsedURI.Host != s.Settings.Security.Host { 58 return c.JSON(http.StatusBadRequest, map[string]string{"error": "Invalid redirect URI"}) 59 }Import the
net/urlpackage if not already imported:3 import ( 4 "net/http" 5 "strings" + "net/url" 6 7 "github.com/labstack/echo/v4" 8 "github.com/markbates/goth/gothic" 9 )📝 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.parsedURI, err := url.Parse(redirectURI) if err != nil || parsedURI.Host != s.Settings.Security.Host { return c.JSON(http.StatusBadRequest, map[string]string{"error": "Invalid redirect URI"}) }
77-89:
⚠️ Potential issueValidate the
redirectparameter to prevent open redirect and XSS vulnerabilities.The
redirectparameter from the query is used directly in the template rendering without validation. This could be exploited to perform open redirects or inject malicious content.Consider validating the
redirectparameter against a whitelist of allowed URLs:77 code := c.QueryParam("code") 78 redirect := c.QueryParam("redirect") + // Validate redirect URL + if !isValidRedirectURL(redirect, s.Settings.Security.BasicAuth.AllowedRedirectURLs) { + return c.String(http.StatusBadRequest, "Invalid redirect URL") + }Implement an
isValidRedirectURLfunction to ensure theredirectparameter is safe to use.Committable suggestion skipped: line range outside the PR's diff.
66-66:
⚠️ Potential issueCheck the error return value of
gothic.StoreInSessionto handle potential errors.The error returned by
gothic.StoreInSessionon line 66 is not being checked. Ignoring this error could lead to silent failures if the session storage encounters an issue. It's important to handle this error to ensure robust session management.Apply this diff to fix the issue:
66 // Store the access token in Gothic session - gothic.StoreInSession("access_token", accessToken, c.Request(), c.Response()) + err = gothic.StoreInSession("access_token", accessToken, c.Request(), c.Response()) + if err != nil { + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "Failed to store session"}) + }📝 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.err = gothic.StoreInSession("access_token", accessToken, c.Request(), c.Response()) if err != nil { return c.JSON(http.StatusInternalServerError, map[string]string{"error": "Failed to store session"}) }🧰 Tools
🪛 GitHub Check: golangci / lint
[failure] 66-66:
Error return value ofgothic.StoreInSessionis not checked (errcheck)🪛 GitHub Check: lint
[failure] 66-66:
Error return value ofgothic.StoreInSessionis not checked (errcheck)internal/httpcontroller/auth_routes.go (6)
92-97:
⚠️ Potential issueStore Hashed Passwords Instead of Plaintext
Storing plaintext passwords is a significant security risk. It's recommended to store and compare hashed passwords using a secure hashing algorithm like bcrypt.
Would you like assistance in implementing secure password hashing?
66-71:
⚠️ Potential issueValidate Redirect Parameters to Prevent Open Redirects
The
redirectparameter is used directly from user input without validation, which could lead to open redirect vulnerabilities. Ensure that the redirect URL is validated to allow only safe, internal paths.Apply this diff to validate the
redirectparameter:if c.Request().Header.Get("HX-Request") == "true" { redirect := c.QueryParam("redirect") + // Validate the redirect parameter + if !isValidRedirect(redirect) { + redirect = "/settings/main" + } // If no redirect is provided, redirect to the main settings page if redirect == "" { redirect = "/settings/main" }And add a helper function to validate redirects:
// isValidRedirect ensures the redirect path is safe and internal func isValidRedirect(redirect string) bool { // Allow only relative paths starting with '/' return strings.HasPrefix(redirect, "/") && !strings.Contains(redirect, "//") }
53-53: 🛠️ Refactor suggestion
Avoid Exposing Internal Error Messages
Returning internal error details to clients may reveal sensitive information. Replace
err.Error()with a generic error message.Apply this diff to improve error handling:
if err != nil { - return c.String(http.StatusBadRequest, err.Error()) + return c.String(http.StatusBadRequest, "Authentication failed") }📝 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.return c.String(http.StatusBadRequest, "Authentication failed")
56-57:
⚠️ Potential issueHandle Errors from
gothic.StoreInSessionCallsThe return values of
gothic.StoreInSessionare not checked. Failing to handle these errors might lead to unexpected behavior or security issues if the session data isn't stored properly.Apply this diff to handle potential errors:
func handleGothCallback(c echo.Context) error { // ... - gothic.StoreInSession(c.Param("provider"), user.UserID, c.Request(), c.Response()) - gothic.StoreInSession("userId", user.Email, c.Request(), c.Response()) + if err := gothic.StoreInSession(c.Param("provider"), user.UserID, c.Request(), c.Response()); err != nil { + return c.String(http.StatusInternalServerError, "Error storing session data") + } + if err := gothic.StoreInSession("userId", user.Email, c.Request(), c.Response()); err != nil { + return c.String(http.StatusInternalServerError, "Error storing session data") + } // ... }📝 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.if err := gothic.StoreInSession(c.Param("provider"), user.UserID, c.Request(), c.Response()); err != nil { return c.String(http.StatusInternalServerError, "Error storing session data") } if err := gothic.StoreInSession("userId", user.Email, c.Request(), c.Response()); err != nil { return c.String(http.StatusInternalServerError, "Error storing session data") }🧰 Tools
🪛 GitHub Check: golangci / lint
[failure] 56-56:
Error return value ofgothic.StoreInSessionis not checked (errcheck)
[failure] 57-57:
Error return value ofgothic.StoreInSessionis not checked (errcheck)🪛 GitHub Check: lint
[failure] 56-56:
Error return value ofgothic.StoreInSessionis not checked (errcheck)
[failure] 57-57:
Error return value ofgothic.StoreInSessionis not checked (errcheck)
111-112:
⚠️ Potential issueHandle Errors from
gothic.StoreInSessionCalls During LogoutThe return values of
gothic.StoreInSessionare not checked when clearing session data. Ignoring potential errors here could leave stale session information.Apply this diff to handle potential errors:
func (s *Server) handleLogout(c echo.Context) error { // Logout from all providers - gothic.StoreInSession("userId", "", c.Request(), c.Response()) - gothic.StoreInSession("access_token", "", c.Request(), c.Response()) + if err := gothic.StoreInSession("userId", "", c.Request(), c.Response()); err != nil { + return echo.NewHTTPError(http.StatusInternalServerError, "Error clearing session data") + } + if err := gothic.StoreInSession("access_token", "", c.Request(), c.Response()); err != nil { + return echo.NewHTTPError(http.StatusInternalServerError, "Error clearing session data") + } // ... }📝 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.if err := gothic.StoreInSession("userId", "", c.Request(), c.Response()); err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "Error clearing session data") } if err := gothic.StoreInSession("access_token", "", c.Request(), c.Response()); err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "Error clearing session data") }🧰 Tools
🪛 GitHub Check: golangci / lint
[failure] 111-111:
Error return value ofgothic.StoreInSessionis not checked (errcheck)
[failure] 112-112:
Error return value ofgothic.StoreInSessionis not checked (errcheck)🪛 GitHub Check: lint
[failure] 111-111:
Error return value ofgothic.StoreInSessionis not checked (errcheck)
[failure] 112-112:
Error return value ofgothic.StoreInSessionis not checked (errcheck)
95-97:
⚠️ Potential issueUse Constant-Time Comparison for Passwords
Comparing passwords using
!=can make the application vulnerable to timing attacks. Usecrypto/subtle.ConstantTimeComparefor password comparison to mitigate this risk.Apply this diff to enhance security:
+import ( + // ... + "crypto/subtle" +) func (s *Server) handleBasicAuthLogin(c echo.Context) error { password := c.FormValue("password") storedPassword := s.Settings.Security.BasicAuth.Password - if password != storedPassword { + if subtle.ConstantTimeCompare([]byte(password), []byte(storedPassword)) != 1 { return c.HTML(http.StatusUnauthorized, "<div class='text-red-500'>Invalid password</div>") } // ... }Committable suggestion skipped: line range outside the PR's diff.
internal/httpcontroller/server.go (1)
121-123:
⚠️ Potential issueFix IP parsing in
RealIPmethod for compliance with standardsThe
RealIPfunction splits theX-Forwarded-Forheader using", ", which may not handle all cases correctly. According to the standard, theX-Forwarded-Forheader is a comma-separated list of IP addresses without guaranteed spaces. To ensure accurate extraction of the client's IP address, split by comma and trim any whitespace.Apply this diff to fix the issue:
if forwardedFor := c.Request().Header.Get("X-Forwarded-For"); forwardedFor != "" { - ip = strings.Split(forwardedFor, ", ")[0] + ip = strings.Split(forwardedFor, ",")[0] + ip = strings.TrimSpace(ip) } else { ip = strings.Split(c.Request().RemoteAddr, ":")[0] }📝 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.if forwardedFor := c.Request().Header.Get("X-Forwarded-For"); forwardedFor != "" { ip = strings.Split(forwardedFor, ",")[0] ip = strings.TrimSpace(ip) } else {internal/security/oauth.go (5)
126-126:
⚠️ Potential issueAvoid logging sensitive user information
Logging user IDs can expose personally identifiable information (PII) in logs. Consider removing or anonymizing the user ID in log messages to protect user privacy.
Apply this diff to remove sensitive information from the log:
-log.Printf("User with userId is not allowed to login: %s", providedId) +log.Printf("User is not allowed to login")📝 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.log.Printf("User is not allowed to login")
179-179:
⚠️ Potential issueAvoid logging access tokens
Including access tokens in log messages when they are not found can expose them unnecessarily. Remove the token from the log statement to enhance security.
Apply this diff to remove the token from the log:
-log.Printf("Access token not found: %s", token) +log.Printf("Access token not found")📝 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.log.Printf("Access token not found")
172-172:
⚠️ Potential issueAvoid logging access tokens
Access tokens should be treated as confidential. Logging them may lead to security risks if the logs are accessed by unauthorized individuals.
Apply this diff to avoid logging the token:
-log.Printf("Validating access token: %s", token) +log.Printf("Validating access token")📝 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.log.Printf("Validating access token")
93-93:
⚠️ Potential issueAvoid logging access tokens
Access tokens are sensitive credentials and should not be logged. Logging them can lead to security vulnerabilities if unauthorized access to logs occurs.
Apply this diff to prevent logging the token:
-log.Printf("User is authenticated with token: %s", token) +log.Printf("User is authenticated")📝 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.log.Printf("User is authenticated")
91-95:
⚠️ Potential issueHandle errors from session retrieval functions
The error returned by
gothic.GetFromSessionis being ignored. Proper error handling ensures robustness and helps identify issues during session retrieval.Apply this diff to handle the error:
-if token, _ := gothic.GetFromSession("access_token", c.Request()); +if token, err := gothic.GetFromSession("access_token", c.Request()); err == nil && token != "" && s.ValidateAccessToken(token) {📝 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.if token, err := gothic.GetFromSession("access_token", c.Request()); err == nil && token != "" && s.ValidateAccessToken(token) { log.Printf("User is authenticated with token: %s", token) return true }internal/httpcontroller/routes.go (4)
149-149: 🛠️ Refactor suggestion
Add Nil Check for
s.Settings.Securityto Prevent PanicAccessing
s.Settings.Security.AllowCloudflareBypasswithout checking ifs.Settings.Securityis nil could lead to a nil pointer dereference ifSecurityis not initialized.Add a nil check before accessing
s.Settings.Security:- isCloudflare := s.Settings.Security.AllowCloudflareBypass && s.CloudflareAccess.IsEnabled(c) + var isCloudflare bool + if s.Settings.Security != nil { + isCloudflare = s.Settings.Security.AllowCloudflareBypass && s.CloudflareAccess.IsEnabled(c) + }This ensures that the application does not panic if
s.Settings.Securityis 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.var isCloudflare bool if s.Settings.Security != nil { isCloudflare = s.Settings.Security.AllowCloudflareBypass && s.CloudflareAccess.IsEnabled(c) }
176-176:
⚠️ Potential issuePotential XSS Vulnerability from Unsanitized
RequestURIinPreloadFragmentAssigning
data.PreloadFragment = c.Request().RequestURIwithout proper sanitization could lead to Cross-Site Scripting (XSS) attacks ifPreloadFragmentis rendered in templates without escaping. SinceRequestURIcan contain user-controlled input, it's important to sanitize or escape it before use.Escape the
RequestURIwhen assigning it toPreloadFragment:import ( "fmt" "html/template" + "html" "net/http" "github.com/labstack/echo/v4" ) ... - data.PreloadFragment = c.Request().RequestURI + data.PreloadFragment = html.EscapeString(c.Request().RequestURI)Ensure that the templates render
PreloadFragmentsafely, using the escaping mechanisms provided by the templating engine.Committable suggestion skipped: line range outside the PR's diff.
59-59: 🛠️ Refactor suggestion
Handle Possible Errors from
initAuthRoutesFunctionThe call to
s.initAuthRoutes()does not check for errors. If this function can return an error, it should be handled to prevent unexpected behavior during route initialization.Modify the code to handle potential errors:
- // Initialize OAuth2 routes - s.initAuthRoutes() + // Initialize OAuth2 routes + if err := s.initAuthRoutes(); err != nil { + s.Echo.Logger.Fatal("Failed to initialize auth routes:", err) + }Ensure that the
initAuthRoutesfunction is updated to return an error if necessary.Committable suggestion skipped: line range outside the PR's diff.
78-83:
⚠️ Potential issueMissing Authentication on Partial Routes May Expose Sensitive Data
While full page routes are protected based on the
Authorizedfield, partial routes ins.partialRoutesdo not have similar authentication checks. This could allow unauthorized users to access sensitive partial content by directly invoking these endpoints.Consider adding an
Authorizedfield toPartialRouteConfigand applying theAuthMiddlewareto those routes that require authentication.Apply this diff to update the
PartialRouteConfigand route initialization:type PartialRouteConfig struct { Path string TemplateName string Title string Handler echo.HandlerFunc + Authorized bool // Whether the route requires authentication } // Set up partial routes for _, route := range s.partialRoutes { + if route.Authorized { + s.Echo.GET(route.Path, route.Handler, s.AuthMiddleware) + } else { s.Echo.GET(route.Path, route.Handler) + } }Then, update the partial routes that should be protected:
s.partialRoutes = map[string]PartialRouteConfig{ "/detections": { Path: "/detections", TemplateName: "", Title: "", Handler: h.WithErrorHandling(h.Detections), + Authorized: true, }, "/detections/recent": { Path: "/detections/recent", TemplateName: "recentDetections", Title: "Recent Detections", Handler: h.WithErrorHandling(h.RecentDetections), + Authorized: true, }, "/detections/details": { Path: "/detections/details", TemplateName: "detectionDetails", Title: "Detection Details", Handler: h.WithErrorHandling(h.DetectionDetails), + Authorized: true, }, "/top-birds": { Path: "/top-birds", TemplateName: "birdsTableHTML", Title: "Top Birds", Handler: h.WithErrorHandling(h.TopBirds), + Authorized: true, }, "/notes": { Path: "/notes", TemplateName: "notes", Title: "All Notes", Handler: h.WithErrorHandling(h.GetAllNotes), + Authorized: true, }, "/media/spectrogram": { Path: "/media/spectrogram", TemplateName: "", Title: "", Handler: h.WithErrorHandling(h.ServeSpectrogram), }, "/login": { Path: "/login", TemplateName: "login", Title: "Login", Handler: h.WithErrorHandling(s.handleLoginPage), Authorized: false, }, }📝 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.if route.Authorized { s.Echo.GET(route.Path, h.WithErrorHandling(s.handlePageRequest), s.AuthMiddleware) } else { s.Echo.GET(route.Path, h.WithErrorHandling(s.handlePageRequest)) } type PartialRouteConfig struct { Path string TemplateName string Title string Handler echo.HandlerFunc Authorized bool // Whether the route requires authentication } // Set up partial routes for _, route := range s.partialRoutes { if route.Authorized { s.Echo.GET(route.Path, route.Handler, s.AuthMiddleware) } else { s.Echo.GET(route.Path, route.Handler) } } s.partialRoutes = map[string]PartialRouteConfig{ "/detections": { Path: "/detections", TemplateName: "", Title: "", Handler: h.WithErrorHandling(h.Detections), Authorized: true, }, "/detections/recent": { Path: "/detections/recent", TemplateName: "recentDetections", Title: "Recent Detections", Handler: h.WithErrorHandling(h.RecentDetections), Authorized: true, }, "/detections/details": { Path: "/detections/details", TemplateName: "detectionDetails", Title: "Detection Details", Handler: h.WithErrorHandling(h.DetectionDetails), Authorized: true, }, "/top-birds": { Path: "/top-birds", TemplateName: "birdsTableHTML", Title: "Top Birds", Handler: h.WithErrorHandling(h.TopBirds), Authorized: true, }, "/notes": { Path: "/notes", TemplateName: "notes", Title: "All Notes", Handler: h.WithErrorHandling(h.GetAllNotes), Authorized: true, }, "/media/spectrogram": { Path: "/media/spectrogram", TemplateName: "", Title: "", Handler: h.WithErrorHandling(h.ServeSpectrogram), }, "/login": { Path: "/login", TemplateName: "login", Title: "Login", Handler: h.WithErrorHandling(s.handleLoginPage), Authorized: false, }, }internal/httpcontroller/template_functions.go (3)
66-67:
⚠️ Potential issueHandle Division by Zero in
divFuncandmodFuncThe
divFuncandmodFuncfunctions do not handle cases where the divisorbis zero. This can lead to a runtime panic due to division or modulo by zero.Consider adding input validation to prevent division or modulo by zero. Here's how you could modify the functions:
-func divFunc(a, b int) int { return a / b } -func modFunc(a, b int) int { return a % b } +func divFunc(a, b int) (int, error) { + if b == 0 { + return 0, errors.New("division by zero") + } + return a / b, nil +} + +func modFunc(a, b int) (int, error) { + if b == 0 { + return 0, errors.New("modulo by zero") + } + return a % b, nil +}Please note that changing the return type will require updates to any templates or functions that call
divFuncandmodFuncto handle the error.Would you like assistance in updating the templates to accommodate these changes?
📝 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.func divFunc(a, b int) (int, error) { if b == 0 { return 0, errors.New("division by zero") } return a / b, nil } func modFunc(a, b int) (int, error) { if b == 0 { return 0, errors.New("modulo by zero") } return a % b, nil }
183-196:
⚠️ Potential issueEnsure
seqFuncHandlesstartGreater ThanendThe
seqFuncfunction may panic ifstartis greater thanendbecause it attempts to create a slice with a negative length.Add input validation to handle cases where
startis greater thanend:func seqFunc(start, end int) []int { + if start > end { + return []int{} + } seq := make([]int, end-start+1) for i := range seq { seq[i] = start + i } return seq }Alternatively, decide if the sequence should be generated in reverse order when
start > end.📝 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./** * seqFunc generates a sequence of integers starting from 'start' to 'end' (inclusive). * * @param start The starting integer of the sequence * @param end The ending integer of the sequence * @return []int The generated sequence of integers */ func seqFunc(start, end int) []int { if start > end { return []int{} } seq := make([]int, end-start+1) for i := range seq { seq[i] = start + i } return seq }
222-228: 🛠️ Refactor suggestion
Add Input Validation in
sumHourlyCountsRangeThe
sumHourlyCountsRangefunction lacks validation forstartandlengthparameters. Negative values or excessively large sums could lead to incorrect calculations.Implement input checks to ensure
startandlengthare within valid ranges:func sumHourlyCountsRange(counts [24]int, start, length int) int { + if start < 0 || start >= len(counts) { + return 0 + } + if length < 0 { + return 0 + } sum := 0 for i := start; i < start+length; i++ { sum += counts[i%24] } return sum }📝 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.func sumHourlyCountsRange(counts [24]int, start, length int) int { if start < 0 || start >= len(counts) { return 0 } if length < 0 { return 0 } sum := 0 for i := start; i < start+length; i++ { sum += counts[i%24] } return sum }internal/security/cloudflare.go (5)
212-214:
⚠️ Potential issueReturn Audience in
GetAudience()MethodCurrently, the
GetAudience()method returnsnil, which may cause issues during token validation where the audience is expected to be verified.Modify the method to return the actual audience:
-func (c *CloudflareAccessClaims) GetAudience() (jwt.ClaimStrings, error) { - return nil, nil +func (c *CloudflareAccessClaims) GetAudience() (jwt.ClaimStrings, error) { + return jwt.ClaimStrings(c.Audience), nil }This ensures that audience claims are correctly provided for validation.
📝 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.func (c *CloudflareAccessClaims) GetAudience() (jwt.ClaimStrings, error) { return jwt.ClaimStrings(c.Audience), nil }
216-218:
⚠️ Potential issueImplement Proper Claims Validation in
Valid()MethodThe
Valid()method ofCloudflareAccessClaimscurrently returnsnil, effectively skipping standard JWT claims validation. This could allow invalid or expired tokens to be accepted.Implement validation logic in the
Valid()method:-func (c *CloudflareAccessClaims) Valid() error { - return nil +func (c *CloudflareAccessClaims) Valid() error { + now := time.Now().Unix() + if c.ExpiresAt < now { + return fmt.Errorf("token expired") + } + if c.NotBefore > now { + return fmt.Errorf("token not yet valid") + } + return nil }This ensures that the token's expiration and not-before times are properly validated.
📝 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.func (c *CloudflareAccessClaims) Valid() error { now := time.Now().Unix() if c.ExpiresAt < now { return fmt.Errorf("token expired") } if c.NotBefore > now { return fmt.Errorf("token not yet valid") } return nil }
77-81:
⚠️ Potential issueCheck HTTP Response Status Code Before Parsing
After fetching the certificates from Cloudflare, the response status code is not checked. If the response is not
200 OK, attempting to decode the response body could lead to errors or panics.Apply this diff to handle non-200 responses:
resp, err := http.Get(certsURL) if err != nil { return fmt.Errorf("failed to fetch Cloudflare certs: %w", err) +} +if resp.StatusCode != http.StatusOK { + return fmt.Errorf("failed to fetch Cloudflare certs: received status code %d", resp.StatusCode) } defer resp.Body.Close()This ensures that only successful responses are processed.
📝 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.resp, err := http.Get(certsURL) if err != nil { return fmt.Errorf("failed to fetch Cloudflare certs: %w", err) } if resp.StatusCode != http.StatusOK { return fmt.Errorf("failed to fetch Cloudflare certs: received status code %d", resp.StatusCode) } defer resp.Body.Close()
147-153:
⚠️ Potential issueHandle Missing Certificates Safely and Ensure Thread-Safe Access
When accessing
ca.certs[kid], there's no check to verify if the certificate exists for the givenkid. Additionally, accessingca.certswithout proper synchronization may lead to race conditions.Apply this diff to handle missing certificates and ensure thread-safe access:
+ca.certCache.mutex.RLock() cert, ok := ca.certs[kid] +ca.certCache.mutex.RUnlock() +if !ok { + log.Printf("No certificate found for key ID: %s", kid) + return nil, fmt.Errorf("no certificate found for key ID: %s", kid) +}This adds a read lock around the access to
ca.certsand checks if the certificate exists, preventing potential panics and race conditions.Committable suggestion skipped: line range outside the PR's diff.
129-133:
⚠️ Potential issueCorrect
teamDomainExtraction from Issuer URLExtracting
teamDomainby splitting the issuer URL on dots may not reliably produce the correct domain, especially if subdomains are present.Use URL parsing for accurate extraction:
-if claims.Issuer != "" { - parts := strings.Split(claims.Issuer, ".") - if len(parts) > 0 { - ca.teamDomain = strings.TrimPrefix(parts[0], "https://") - } +if claims.Issuer != "" { + parsedIssuer, err := url.Parse(claims.Issuer) + if err != nil { + log.Printf("Invalid issuer URL: %v", err) + return nil, fmt.Errorf("invalid issuer URL: %w", err) + } + ca.teamDomain = parsedIssuer.Hostname() }This approach accurately parses the issuer URL to extract the hostname.
📝 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.if claims.Issuer != "" { parsedIssuer, err := url.Parse(claims.Issuer) if err != nil { log.Printf("Invalid issuer URL: %v", err) return nil, fmt.Errorf("invalid issuer URL: %w", err) } ca.teamDomain = parsedIssuer.Hostname() }internal/security/cloudflare_test.go (4)
164-181: 🛠️ Refactor suggestion
Duplicate tests for non-200 status code handling
Both
TestFetchCertsFailure(lines 164-181) andTestFetchCertsNon200Response(lines 222-235) test the behavior offetchCertswhen the server returns a non-200 status code. This results in redundant code. Consolidating these tests will improve maintainability.Consider merging these tests into a single parameterized test function:
func TestFetchCertsNon200Response(t *testing.T) { testCases := []struct { statusCode int description string }{ {http.StatusInternalServerError, "Internal Server Error"}, {http.StatusNotFound, "Not Found"}, } for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(tc.statusCode) })) defer server.Close() ca := &CloudflareAccess{certs: make(map[string]string)} err := ca.fetchCerts(server.URL) if err == nil { t.Fatalf("Expected an error for status code %d, but got nil", tc.statusCode) } if len(ca.certs) != 0 { t.Fatalf("Expected 0 certificates for status code %d, got %d", tc.statusCode, len(ca.certs)) } }) } }This approach reduces duplication and enhances the clarity of test cases.
Also applies to: 222-235
163-164:
⚠️ Potential issueMismatch between function comment and function name
There's a discrepancy between the comment and the function name. The comment refers to
TestFetchCertsNon200Response, but the function is namedTestFetchCertsFailure. Please align the function name and comment for consistency.Consider one of the following fixes:
Option 1: Rename the function to match the comment.
-// TestFetchCertsNon200Response tests the behavior of fetchCerts when the server returns a non-200 status code -func TestFetchCertsFailure(t *testing.T) { +// TestFetchCertsNon200Response tests the behavior of fetchCerts when the server returns a non-200 status code +func TestFetchCertsNon200Response(t *testing.T) {Option 2: Update the comment to match the function name.
-// TestFetchCertsNon200Response tests the behavior of fetchCerts when the server returns a non-200 status code +// TestFetchCertsFailure tests the behavior of fetchCerts under failure conditions📝 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.// TestFetchCertsNon200Response tests the behavior of fetchCerts when the server returns a non-200 status code func TestFetchCertsNon200Response(t *testing.T) {
60-60:
⚠️ Potential issueCorrect the function comment to match the test function
The comment describes
TestFetchCertsFailure, but the function is namedTestFetchCertsNetworkFailure. Please update the comment to accurately reflect the function's purpose for better understanding.Apply this diff:
-// TestFetchCertsFailure tests the behavior of fetchCerts when the server returns a non-200 status code +// TestFetchCertsNetworkFailure tests the behavior of fetchCerts when there is a network failure📝 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.// TestFetchCertsNetworkFailure tests the behavior of fetchCerts when there is a network failure
205-212:
⚠️ Potential issueAvoid calling
t.Errorfinside goroutinesUsing
t.Errorfwithin a goroutine is unsafe because the testing framework's methods are not goroutine-safe. This can lead to race conditions or unexpected behavior.Collect errors from goroutines via a channel and report them in the main goroutine. Here's how you can modify the code:
func TestConcurrentAccessToCertsMap(t *testing.T) { // Setup test server // ... var wg sync.WaitGroup numRoutines := 10 wg.Add(numRoutines) + errCh := make(chan error, numRoutines) for i := 0; i < numRoutines; i++ { go func() { defer wg.Done() err := ca.fetchCerts(server.URL) if err != nil { - t.Errorf("Error fetching certs: %v", err) + errCh <- err + } else { + errCh <- nil } }() } wg.Wait() + close(errCh) + for err := range errCh { + if err != nil { + t.Errorf("Error fetching certs: %v", err) + } + } if len(ca.certs) != 1 { t.Errorf("Expected 1 certificate, got %d", len(ca.certs)) } }This change ensures thread safety by handling errors in the main goroutine.
Committable suggestion skipped: line range outside the PR's diff.
internal/httpcontroller/handlers/settings.go (1)
111-114:
⚠️ Potential issueValidate
Hostto prevent malformed redirect URIsThe
Hostvalue fromsettings.Security.Hostis used without validation to construct redirect URIs. This could lead to malformed or insecure URIs ifHostis improperly set. Consider validatingHostto ensure it is a well-formed URL.Apply this diff to add host validation:
+import "net/url" func (h *Handlers) updateAuthenticationSettings(settings *conf.Settings) { protocol := "http" if settings.Security.RedirectToHTTPS { protocol = "https" } host := strings.TrimRight(settings.Security.Host, "/") if !strings.HasPrefix(host, "http") { host = fmt.Sprintf("%s://%s", protocol, host) } + // Validate the host URL + parsedURL, err := url.Parse(host) + if err != nil || parsedURL.Host == "" { + log.Println("Invalid host URL:", host) + // Handle error appropriately, such as returning an error + return + } settings.Security.BasicAuth.RedirectURI = host settings.Security.GoogleAuth.RedirectURI = fmt.Sprintf("%s/auth/google/callback", host) settings.Security.GithubAuth.RedirectURI = fmt.Sprintf("%s/auth/github/callback", host)Committable suggestion skipped: line range outside the PR's diff.
views/settings/securitySettings.html (3)
10-10:
⚠️ Potential issuePotential Issue with Default Host Assignment
On line 10, the
hostis assigned using the expression:host: '{{.Settings.Security.Host}}' || (location.protocol + '//' + location.host),If
{{.Settings.Security.Host}}evaluates to an empty string, JavaScript treats it as a truthy value, and the fallback tolocation.protocol + '//' + location.hostwill not occur. This may result inhostbeing set to an empty string, which could cause issues in the application.Apply this diff to ensure the fallback works correctly:
-host: '{{.Settings.Security.Host}}' || (location.protocol + '//' + location.host), +host: '{{.Settings.Security.Host}}' ? '{{.Settings.Security.Host}}' : (location.protocol + '//' + location.host),
275-276: 🛠️ Refactor suggestion
Inconsistent Use of
hasChangesState ManagementIn lines 275-276, you're updating
Alpine.store('security').hasChanges, buthasChangesis also defined within the component's localx-data.For consistency and to avoid confusion, consider managing
hasChangeseither entirely within the local component state or entirely within the Alpine.js store. Here's how you might adjust it to use the local state:$watch('security', () => { - Alpine.store('security').hasChanges = true; + hasChanges = true; }, { deep: true });Committable suggestion skipped: line range outside the PR's diff.
138-139:
⚠️ Potential issueIncorrect Host Parsing in
getRedirectUriFunctionIn lines 138-139, the function
getRedirectUristrips the protocol fromthis.security.hostbut does not handle cases where the host may already be without a protocol. Additionally, ifsecurity.hostis an empty string, this may result in incorrect URIs.Consider modifying the function to ensure it consistently constructs the redirect URI:
-getRedirectUri(provider) { - const cleanHost = (this.security.host || location.host).replace(/^https?:\/\//, ''); - return `${location.protocol}//${cleanHost}/auth/${provider}/callback`; +getRedirectUri(provider) { + const host = this.security.host ? this.security.host.replace(/^https?:\/\//, '') : location.host; + return `${location.protocol}//${host}/auth/${provider}/callback`;📝 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.const host = this.security.host ? this.security.host.replace(/^https?:\/\//, '') : location.host; return `${location.protocol}//${host}/auth/${provider}/callback`;internal/conf/config.go (3)
210-210:
⚠️ Potential issueAvoid storing plain text passwords in configuration
Storing passwords in plain text within configuration files can lead to security vulnerabilities. It's recommended to handle passwords securely, such as using environment variables, secure storage mechanisms, or prompts at runtime.
402-403:
⚠️ Potential issueInconsistent configuration key used for BasicAuth client secret
In the
createDefaultConfigfunction, the code checks forsecurity.basicauth.secretbut setssecurity.basicauth.clientsecret. This inconsistency may lead to theClientSecretnot being properly initialized.Apply this diff to correct the configuration key:
- // If the basicauth secret is not set, generate a random one - if viper.GetString("security.basicauth.secret") == "" { + // If the basicauth client secret is not set, generate a random one + if viper.GetString("security.basicauth.clientsecret") == "" { viper.Set("security.basicauth.clientsecret", GenerateRandomSecret()) }📝 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.// If the basicauth client secret is not set, generate a random one if viper.GetString("security.basicauth.clientsecret") == "" { viper.Set("security.basicauth.clientsecret", GenerateRandomSecret())
231-250: 🛠️ Refactor suggestion
Ensure secure handling of session secrets
The
SessionSecretis critical for securing session cookies and should be securely generated if not provided. Consider generating a randomSessionSecretwhen it's not set to enhance security.Apply this diff to generate a random
SessionSecretif not provided:+ // If the session secret is not set, generate a random one + if viper.GetString("security.sessionsecret") == "" { + viper.Set("security.sessionsecret", GenerateRandomSecret()) + }Add this code in the
createDefaultConfigfunction after generating theClientSecretforBasicAuth.Committable suggestion skipped: line range outside the PR's diff.
assets/tailwind.css (2)
2639-2644:
⚠️ Potential issueConflicting
background-colordeclarations in.inputclassIn the
.inputclass, there are twobackground-colorproperties declared consecutively, which may lead to unintended behavior. Specifically,background-coloris first set tovar(--fallback-p,oklch(var(--p)/var(--tw-bg-opacity)));and immediately after set totransparent;.Consider removing the redundant
background-colordeclaration or ensure the intended color is correctly applied. Apply this diff to fix the issue:.input input { --tw-bg-opacity: 1; - background-color: var(--fallback-p,oklch(var(--p)/var(--tw-bg-opacity))); background-color: transparent; }📝 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..\!input input { --tw-bg-opacity: 1 !important; background-color: transparent !important; }
2651-2654: 🛠️ Refactor suggestion
Reconsider focus styles for accessibility
Currently, the focus outline is set to be transparent, which might hinder keyboard navigation visibility for users.
To improve accessibility, consider applying a visible focus outline. Here's a suggested change:
.input input:focus { - outline: 2px solid transparent; + outline: 2px solid var(--fallback-bc,oklch(var(--bc)/1)); outline-offset: 2px; }Committable suggestion skipped: line range outside the PR's diff.
petterip
force-pushed
the
main
branch
3 times, most recently
from
5d69e47
to
85044ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 50
🧹 Outside diff range and nitpick comments (43)
internal/httpcontroller/middleware.go (1)
79-81: Consider making protected routes configurableThe current implementation hardcodes "/settings/" as the only protected path prefix. Consider:
- Making protected paths configurable via settings
- Adding support for multiple protected path prefixes
- Including exact path matches in addition to prefixes
+var defaultProtectedPaths = []string{ + "/settings/", + "/admin/", + "/api/private/", +} + -func isProtectedRoute(path string) bool { - return strings.HasPrefix(path, "/settings/") +func (s *Server) isProtectedRoute(path string) bool { + protectedPaths := s.Settings.Security.ProtectedPaths + if len(protectedPaths) == 0 { + protectedPaths = defaultProtectedPaths + } + for _, prefix := range protectedPaths { + if strings.HasPrefix(path, prefix) { + return true + } + } + return false +}internal/security/oauth_test.go (1)
1-111: Consider adding integration tests with mock OAuth2 providers.While the current unit tests cover the token validation logic, consider adding integration tests that simulate the complete OAuth2 flow with mock providers (Google, GitHub). This would help ensure the entire authentication pipeline works correctly.
Would you like me to provide an example of how to implement mock OAuth2 provider tests using a library like
go-oauth2/oauth2or similar?views/settings/templates/passwordField.html (3)
14-18: Add keyboard interaction support for tooltip.The tooltip trigger is only accessible via mouse hover. Consider adding keyboard support for better accessibility.
<span class="ml-2 text-sm text-gray-500 cursor-help" @mouseenter="showTooltip = '{{.id}}'" - @mouseleave="showTooltip = null">ⓘ</span> + @mouseleave="showTooltip = null" + @focus="showTooltip = '{{.id}}'" + @blur="showTooltip = null" + tabindex="0" + role="button">ⓘ</span>
42-62: Add security warning when password is visible.Consider adding a warning message when the password is visible to enhance security awareness.
<button type="button" @click="showPassword = !showPassword" aria-label="Toggle password visibility" :aria-pressed="showPassword" - class="absolute inset-y-0 right-0 pr-3 flex items-center"> + class="absolute inset-y-0 right-0 pr-3 flex items-center" + @click="$dispatch('password-visible', showPassword)">Add this warning message below the input:
<div x-show="showPassword" class="text-sm text-yellow-600 mt-1" role="alert"> Warning: Your password is currently visible </div>
66-83: Enhance validation error messages.The current error message "Please enter a valid {{.label}}" is generic. Consider providing more specific feedback based on the validation rules.
- Please enter a valid {{.label}} + <template x-if="$refs.{{.id}}.validity.valueMissing"> + Please enter your {{.label}} + </template> + <template x-if="$refs.{{.id}}.validity.patternMismatch"> + Password must be at least 8 characters long and contain both letters and numbers + </template>views/index.html (3)
15-16: Review and potentially strengthen the Content Security Policy.While the CSP is a good start, consider these security improvements:
- Replace 'unsafe-inline' with nonces or hashes for scripts and styles
- Add missing directives like connect-src for AJAX calls
- Consider adding frame-ancestors to prevent clickjacking
9-10: Consider accessibility implications of viewport settings.The
user-scalable=nosetting may create accessibility issues for users who need to zoom the page.- content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=no, viewport-fit=cover"> + content="width=device-width, initial-scale=1, viewport-fit=cover">
57-66: Consider simplifying the date picker initialization.The
setTimeoutmight not be necessary since the script is loaded withdefer.- setTimeout(function () { var datePicker = document.getElementById('datePicker'); if (datePicker) { var dateInHash = window.location.hash.substring(1); var date = dateInHash ? dateInHash : new Date().toLocaleDateString('sv'); datePicker.value = date; } - }, 0); // ensure the DOM is fully loaded + document.addEventListener('DOMContentLoaded', function() { + var datePicker = document.getElementById('datePicker'); + if (datePicker) { + var dateInHash = window.location.hash.substring(1); + var date = dateInHash ? dateInHash : new Date().toLocaleDateString('sv'); + datePicker.value = date; + } + });doc/security.md (2)
24-46: Enhance OAuth provider configuration documentation.Please consider the following improvements:
- Explain the difference in redirect URI formats between providers (
/auth/google/callbackvs/auth/github/callback)- Document required OAuth scopes for each provider
- Add a note about environment variables as a secure alternative for storing secrets
- Clarify the format for multiple
useridentries (comma-separated without spaces)
68-68: Style: Remove unnecessary hyphenation.Change "remotely-managed" to "remotely managed" as per standard style guidelines for 'ly' adverbs.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~68-~68: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: .../connections/connect-apps/) - [Create a remotely-managed tunnel](https://developers.cloudflare.c...(HYPHENATED_LY_ADVERB_ADJECTIVE)
views/elements/login.html (2)
19-29: Enhance password field security and accessibility.Consider the following improvements:
- Add
minlengthattribute to enforce minimum password length- Add password visibility toggle for better UX
- Include password requirements/constraints using
aria-description<input type="password" id="loginPassword" name="password" class="input input-bordered" required autocomplete="current-password" aria-required="true" aria-labelledby="passwordLabel" - aria-describedby="loginError"> + aria-describedby="loginError passwordHint" minlength="8"> +<div id="passwordHint" class="text-sm text-gray-600"> + Password must be at least 8 characters long +</div>
46-67: Consider enhancing OAuth provider section.While the basic implementation is good, consider these improvements:
- Add provider-specific error handling
- Visually distinguish between providers (e.g., provider logos)
- Add provider-specific loading states in the UI
<a href="/auth/google" class="btn btn-primary grow pr-10" onclick="showSpinner('googleSpinner')" role="button" aria-label="Login with Google"> <span id="googleSpinner" class="invisible loading loading-spinner" aria-hidden="true"></span> + <img src="/static/img/google-logo.svg" alt="" class="w-5 h-5 mr-2" aria-hidden="true"> Login with Google </a>assets/custom.css (4)
25-33: Consider adding transition for smoother loading statesThe HTMX indicator implementation is correct, but could benefit from a smooth transition between states.
Consider adding this CSS for smoother transitions:
.htmx-indicator { display: none; + opacity: 0; + transition: opacity 200ms ease-in-out; } .htmx-request .htmx-indicator { display: inline-block; + opacity: 1; }
35-37: Enhance invalid input accessibilityWhile the error state is visually clear, it could benefit from additional accessibility improvements.
Consider adding these enhancements:
input.invalid { border-color: #dc2626; + outline-color: #dc2626; + box-shadow: 0 0 0 1px #dc2626; } +input.invalid:focus { + outline-color: #dc2626; + box-shadow: 0 0 0 2px rgba(220, 38, 38, 0.4); +}
65-79: Improve maintainability of responsive breakpointsThe responsive rules are well-structured but could be more maintainable.
Consider using CSS custom properties for breakpoints:
+:root { + --breakpoint-mobile: 479px; + --breakpoint-tablet: 767px; +} .hour-header, .hour-data { display: table-cell; } .hourly-count { display: table-cell; } .bi-hourly-count, .six-hourly-count { display: none; } -@media (max-width: 767px) { +@media (max-width: var(--breakpoint-tablet)) { /* ... existing rules ... */ } -@media (max-width: 479px) { +@media (max-width: var(--breakpoint-mobile)) { /* ... existing rules ... */ }
126-126: Consider min-max constraints for thumbnail sizingWhile using
vwunits is good for responsiveness, consider adding constraints for very large screens.- max-width: 16vw; + max-width: min(16vw, 300px);internal/httpcontroller/server.go (2)
108-116: Replace colored log with structured loggingThe current implementation uses ANSI color codes directly in log.Printf. Consider using the server's logger instance with appropriate log levels instead.
- log.Printf("\033[1;35m*** IsAccessAllowed: Cloudflare Access token valid") + s.Logger.Info("auth", "Cloudflare Access token valid")
108-116: Consider rate limiting for authentication checksGiven that IsAccessAllowed is likely called frequently, consider implementing rate limiting for authentication checks to prevent potential DoS attacks. This is especially important for the Cloudflare Access token validation which might involve cryptographic operations.
Consider using a token cache with appropriate TTL and implementing rate limiting middleware for authentication endpoints.
internal/httpcontroller/routes.go (2)
118-119: Consider adding request validation for settings endpoints.While the endpoints are properly protected with authentication, consider adding request validation to ensure data integrity.
-s.Echo.POST("/settings/save", h.WithErrorHandling(h.SaveSettings), s.AuthMiddleware) +s.Echo.POST("/settings/save", h.WithErrorHandling(h.ValidateSettings), h.WithErrorHandling(h.SaveSettings), s.AuthMiddleware)
161-177: Security context is well-implemented, consider adding request origin validation.The security context implementation is solid, with proper handling of authentication state and Cloudflare integration. Consider adding validation for the
PreloadFragmentrequest URI to prevent potential open redirect vulnerabilities.-data.PreloadFragment = html.EscapeString(c.Request().RequestURI) +data.PreloadFragment = html.EscapeString(validateLocalRedirect(c.Request().RequestURI))internal/conf/config.yaml (1)
54-57: Document retention policy configuration.The retention policy configuration looks good, but could benefit from additional documentation:
- Specify the format for
maxage(e.g., supported units: d=days, h=hours)- Clarify if
maxusageapplies to the disk partition or a specific directory- Document how
minclipsinteracts with other retention policies🧰 Tools
🪛 yamllint
[error] 57-57: trailing spaces
(trailing-spaces)
views/settings/settingsBase.html (5)
26-29: Consider improving password visibility state tracking.The current implementation might miss dynamically added password fields and could target unrelated password fields outside the form.
Consider this approach:
-document.querySelectorAll('input[type="password"]').forEach(field => { +document.getElementById('settingsForm').querySelectorAll('input[type="password"]').forEach(field => { const id = field.id; Alpine.store('security').showPasswords[id] = false; });Also consider using a MutationObserver to handle dynamically added fields:
const observer = new MutationObserver(mutations => { mutations.forEach(mutation => { mutation.addedNodes.forEach(node => { if (node.querySelectorAll) { node.querySelectorAll('input[type="password"]').forEach(field => { const id = field.id; if (!Alpine.store('security').showPasswords[id]) { Alpine.store('security').showPasswords[id] = false; } }); } }); }); }); observer.observe(document.getElementById('settingsForm'), { childList: true, subtree: true });
61-64: Use standard CSS comment format.The inline comments use JavaScript-style comments which, while functional, don't follow CSS conventions.
- margin-top: 0.125rem; - /* mt-0.5 */ - height: 1rem; - /* h-4 */ + margin-top: 0.125rem; /* mt-0.5 */ + height: 1rem; /* h-4 */
85-102: Enhance form validation with ARIA attributes.While the validation logic is solid, it could be more accessible to screen readers.
isFormValid(form) { const visibleFieldsValid = Array.from(form.elements) .filter(element => element.offsetParent !== null) .every(element => element.checkValidity()); const inputSelector = 'input[type=\'password\'], input[type=\'text\']'; if(!visibleFieldsValid) { form.querySelectorAll(inputSelector).forEach(input => { if (input.offsetParent === null) return; input.checkValidity(); - if (!input.validity.valid) + if (!input.validity.valid) { // Trigger validation message - input.dispatchEvent(new Event('blur')); + input.setAttribute('aria-invalid', 'true'); + input.dispatchEvent(new Event('blur')); + } else { + input.setAttribute('aria-invalid', 'false'); + } }); } return visibleFieldsValid; }
Line range hint
112-131: Consider debouncing the save operation.The save operation could benefit from debouncing to prevent multiple rapid submissions.
Add a debounce utility:
function debounce(func, wait) { let timeout; return function executedFunction(...args) { const later = () => { clearTimeout(timeout); func(...args); }; clearTimeout(timeout); timeout = setTimeout(later, wait); }; }Then modify the save operation:
-saveSettings() { +saveSettings: debounce(function() { const form = document.getElementById('settingsForm'); const formData = new FormData(form); if(!this.isFormValid(form)) { this.addNotification('Please fill out all required fields.', 'error'); return; } this.saving = true; // ... rest of the save logic ... -} +}, 300)
159-163: Enhance notification accessibility and animations.The notification system could benefit from improved accessibility and smooth transitions.
- <div x-show="!notification.removing" :class="{ + <div x-show="!notification.removing" + x-transition:enter="transition ease-out duration-300" + x-transition:enter-start="opacity-0 transform translate-x-8" + x-transition:enter-end="opacity-100 transform translate-x-0" + x-transition:leave="transition ease-in duration-200" + x-transition:leave-start="opacity-100 transform translate-x-0" + x-transition:leave-end="opacity-0 transform translate-x-8" + role="alert" + :aria-live="notification.type === 'error' ? 'assertive' : 'polite'" + :class="{ 'alert-success': notification.type === 'success', 'alert-error': notification.type === 'error', 'alert-info': notification.type === 'info' }" class="alert">Also applies to: 182-183
views/elements/sidebar.html (2)
17-25: Add ARIA labels for better accessibility.While the navigation layout is well-structured, consider adding ARIA labels to improve accessibility:
- <nav - class="flex flex-col h-[100dvh] w-64 bg-base-100 absolute inset-y-0 sm:static sm:h-full overflow-y-auto p-4"> + <nav + class="flex flex-col h-[100dvh] w-64 bg-base-100 absolute inset-y-0 sm:static sm:h-full overflow-y-auto p-4" + aria-label="Main navigation">
Line range hint
53-61: Remove commented-out code.The commented-out logs menu item should be removed if it's no longer needed. If it's for future use, consider tracking it in an issue instead.
internal/security/basic_test.go (2)
16-49: Consider enhancing test coverage with additional validations.While the basic flow is well tested, consider adding:
- Validation of the auth code format (e.g., length, character set)
- Verification of auth code expiration
- Test data constants instead of hardcoded values
+const ( + validClientID = "validClientID" + validRedirectURI = "http://valid.redirect" +) func TestHandleBasicAuthorizeSuccess(t *testing.T) { e := echo.New() - req := httptest.NewRequest(http.MethodGet, "/?client_id=validClientID&redirect_uri=http://valid.redirect", nil) + req := httptest.NewRequest(http.MethodGet, "/?client_id=" + validClientID + "&redirect_uri=" + validRedirectURI, nil) // ... rest of the test + + // Verify auth code format + code := strings.TrimPrefix(location, validRedirectURI + "?code=") + if len(code) < 32 || !regexp.MustCompile(`^[a-zA-Z0-9]+$`).MatchString(code) { + t.Error("invalid auth code format") + } }
51-87: Add test coverage for additional error scenarios.The test covers invalid client ID well, but consider adding:
- Test for invalid redirect URI
- Verification of error response format (JSON vs plain text)
- Test for missing parameters
+func TestHandleBasicAuthorizeInvalidRedirectURI(t *testing.T) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/?client_id=validClientID&redirect_uri=invalid", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + server := &OAuth2Server{ + Settings: &conf.Settings{ + Security: conf.Security{ + BasicAuth: conf.BasicAuth{ + ClientID: "validClientID", + RedirectURI: "http://valid.redirect", + }, + }, + }, + } + + server.HandleBasicAuthorize(c) + + if rec.Code != http.StatusBadRequest { + t.Errorf("expected status %d, got %d", http.StatusBadRequest, rec.Code) + } +}internal/conf/validate.go (1)
161-163: Add security-focused port validation.Given this PR's security focus, consider enhancing port validation:
- Validate port number range (1-65535)
- Warn if using well-known ports (<1024)
- Recommend secure ports for production
Here's a suggested enhancement:
func validateWebServerSettings(settings *struct { Enabled bool Port string Log LogConfig }) error { if settings.Enabled { if settings.Port == "" { return errors.New("WebServer port is required when enabled") } + // Validate port number + port, err := strconv.Atoi(settings.Port) + if err != nil { + return fmt.Errorf("invalid port number: %w", err) + } + if port < 1 || port > 65535 { + return fmt.Errorf("port must be between 1 and 65535") + } + // Warn about well-known ports + if port < 1024 { + log.Printf("Warning: Using well-known port %d requires root/admin privileges", port) + } } return nil }internal/httpcontroller/handlers/settings.go (1)
106-123: Consider enhancing URL validation securityWhile the basic URL validation is good, consider adding these security improvements:
- Validate against internal/localhost addresses if not intended
- Add protection against URL injection patterns
- Consider validating against allowed domains list
Example enhancement:
func formatAndValidateHost(host string, useHTTPS bool) (string, error) { protocol := "http" if useHTTPS { protocol = "https" } host = strings.TrimRight(host, "/") if !strings.HasPrefix(host, "http") { host = fmt.Sprintf("%s://%s", protocol, host) } parsedHost, err := url.Parse(host) if err != nil || parsedHost.Host == "" { return "", fmt.Errorf("Invalid host address") } + // Validate against internal addresses + if strings.Contains(parsedHost.Host, "localhost") || + strings.Contains(parsedHost.Host, "127.0.0.1") { + return "", fmt.Errorf("Internal addresses not allowed") + } + + // Validate against common URL injection patterns + if strings.Contains(parsedHost.Host, "@") || + strings.Contains(parsedHost.Host, "\\") { + return "", fmt.Errorf("Invalid characters in host") + } return host, nil }assets/tailwind.css (1)
1645-1661: Consider consolidating duplicate input stylesThe input styles are duplicated between regular and important (!important) variations. Consider using Tailwind's modifier syntax or CSS custom properties to reduce duplication.
Example approach:
/* Define base input styles once */ .input-base { flex-shrink: 1; appearance: none; height: 3rem; /* ... other common styles ... */ } .input { @extend .input-base; } .\!input { @extend .input-base !important; }Also applies to: 2639-2777
internal/security/basic.go (2)
15-17: Consider allowing multiple validclient_idvaluesThe strict check
clientID != s.Settings.Security.BasicAuth.ClientIDonly allows a single validclient_id. This might limit scalability if you plan to support multiple clients or rotate client IDs in the future.Suggest modifying the code to check against a list of valid client IDs or a more flexible validation mechanism.
19-21: Inflexibleredirect_urivalidationThe exact match check
redirectURI != s.Settings.Security.BasicAuth.RedirectURImay not accommodate situations where multiple redirect URIs are needed (e.g., for different environments like development, staging, production).Recommend implementing a whitelist of allowed
redirect_uris or using a more robust validation method to enhance flexibility.internal/httpcontroller/auth_routes.go (1)
40-47: Consider Handling All Authentication ErrorsIn
handleGothProvider, ifgothic.CompleteUserAuthreturns an error, it proceeds togothic.BeginAuthHandlerwithout handling the error. Ensure that all errors are appropriately logged or returned to the user.Modify the error handling to provide feedback:
if gothUser, err := gothic.CompleteUserAuth(response, request); err == nil { return c.JSON(http.StatusOK, gothUser) +} else { + // Handle authentication error if needed + c.Logger().Errorf("Authentication error: %v", err) } gothic.BeginAuthHandler(response, request) return nilinternal/security/oauth.go (3)
203-203: Remove unnecessary debug loggingThe log statement
log.Printf("*** %s", clientIP)appears to be a leftover debug statement. Remove it to clean up the logs and avoid exposing potentially sensitive information.Apply this diff to remove the unnecessary log:
-log.Printf("*** %s", clientIP)
213-217: Handle errors when parsing CIDR notationWhen parsing the subnet CIDR strings, errors are silently ignored. If a subnet is incorrectly specified, it won't be checked, which might lead to security issues. Log a warning when a subnet fails to parse to aid in debugging configuration issues.
Apply this diff to log parsing errors:
for _, subnet := range subnets { _, ipNet, err := net.ParseCIDR(strings.TrimSpace(subnet)) - if err == nil && ipNet.Contains(clientIP) { + if err != nil { + log.Printf("Invalid subnet CIDR '%s': %v", subnet, err) + continue + } + if ipNet.Contains(clientIP) { return true } }
181-192: Review the logic for authentication bypass based on IP addressThe
IsAuthenticationEnabledfunction disables authentication if the request originates from an allowed subnet. Ensure that bypassing authentication for requests from certain IP ranges aligns with your security policies. This could pose a security risk if the allowed subnets are not properly secured.internal/security/cloudflare_test.go (4)
263-266: Remove redundantlog.SetOutput(io.Discard)The call to
log.SetOutput(io.Discard)is immediately overridden bylog.SetOutput(&logs). The initial call is unnecessary and can be removed to clean up the code.Apply this diff to remove the redundant line:
var logs logWriter - log.SetOutput(io.Discard) log.SetFlags(0) log.SetOutput(&logs)
25-35: Refactor duplicate test server setup into a helper functionThe code for setting up the test server and defining
certsJSONis duplicated across multiple test functions. Consider extracting this code into a helper function to improve maintainability and reduce redundancy.Here's how you could refactor the test server setup:
func setupTestServer(response string) *httptest.Server { return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprintln(w, response) })) }Then, in your tests, use:
server := setupTestServer(certsJSON)Also applies to: 62-72, 94-97, 130-133, 155-158, 197-199, 225-227, 258-261
34-34: Ensure consistent initialization ofCloudflareAccessinstancesIn some tests,
CloudflareAccessis initialized usingNewCloudflareAccess(), while in others it's initialized directly. For consistency and to ensure any initialization logic inNewCloudflareAccess()is executed, consider usingNewCloudflareAccess()in all tests.Replace direct initialization with the constructor function:
- ca := &CloudflareAccess{certs: make(map[string]string)} + ca := NewCloudflareAccess()Also applies to: 74-74, 99-99, 118-118, 135-135, 161-161, 201-201, 232-232, 268-268
46-46: Expand test coverage by adding more test casesThe comment "// Add more test cases here" indicates that additional scenarios can be tested. Consider adding more test cases to
TestCloudflareAccessto cover edge cases and improve test coverage.Examples of additional test cases:
- Certificate fetch with invalid URL.
- Response with duplicate
kidvalues.- Handling of network timeouts or interruptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
assets/alpinejs.min.jsis excluded by!**/*.min.jsgo.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
Dockerfile(2 hunks)README.md(1 hunks)assets/custom.css(4 hunks)assets/tailwind.css(53 hunks)assets/util.js(1 hunks)doc/security.md(1 hunks)go.mod(3 hunks)internal/conf/config.go(5 hunks)internal/conf/config.yaml(3 hunks)internal/conf/defaults.go(2 hunks)internal/conf/validate.go(3 hunks)internal/httpcontroller/auth_routes.go(1 hunks)internal/httpcontroller/handlers/handlers.go(4 hunks)internal/httpcontroller/handlers/settings.go(3 hunks)internal/httpcontroller/middleware.go(3 hunks)internal/httpcontroller/routes.go(8 hunks)internal/httpcontroller/server.go(7 hunks)internal/httpcontroller/template_functions.go(4 hunks)internal/httpcontroller/template_renderers.go(2 hunks)internal/security/basic.go(1 hunks)internal/security/basic_test.go(1 hunks)internal/security/cloudflare.go(1 hunks)internal/security/cloudflare_test.go(1 hunks)internal/security/oauth.go(1 hunks)internal/security/oauth_test.go(1 hunks)reset_auth.sh(1 hunks)tailwind.config.js(1 hunks)views/elements/callback.html(1 hunks)views/elements/login.html(1 hunks)views/elements/sidebar.html(2 hunks)views/index.html(3 hunks)views/settings/integrationSettings.html(16 hunks)views/settings/mainSettings.html(25 hunks)views/settings/securitySettings.html(1 hunks)views/settings/settingsBase.html(5 hunks)views/settings/templates/checkbox.html(1 hunks)views/settings/templates/hostField.html(1 hunks)views/settings/templates/passwordField.html(1 hunks)views/settings/templates/textField.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- Dockerfile
- README.md
- assets/util.js
- go.mod
- internal/httpcontroller/handlers/handlers.go
- internal/httpcontroller/template_renderers.go
- reset_auth.sh
- tailwind.config.js
- views/elements/callback.html
- views/settings/integrationSettings.html
- views/settings/mainSettings.html
- views/settings/securitySettings.html
- views/settings/templates/checkbox.html
- views/settings/templates/hostField.html
- views/settings/templates/textField.html
🧰 Additional context used
🪛 LanguageTool
doc/security.md
[uncategorized] ~68-~68: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: .../connections/connect-apps/) - [Create a remotely-managed tunnel](https://developers.cloudflare.c...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🔇 Additional comments (50)
internal/httpcontroller/middleware.go (3)
14-14: LGTM: Correct middleware ordering
The AuthMiddleware is appropriately positioned as one of the first middleware in the chain, ensuring authentication checks occur before other processing.
63-63: Verify RealIP implementation for proxy handling
Ensure RealIP() properly handles X-Forwarded-For and other proxy headers to prevent IP spoofing attacks.
#!/bin/bash
# Look for RealIP implementation
ast-grep --pattern 'func $$$RealIP($$) {
$$$
}'58-60: Verify Cloudflare Access implementation security
The Cloudflare bypass occurs before IP authentication. While this might be intentional, ensure that CloudflareAccess.IsEnabled() properly validates Cloudflare's JWT tokens and doesn't just check headers which could be spoofed.
✅ Verification successful
Cloudflare Access implementation is secure
The implementation properly validates Cloudflare Access JWTs with multiple security checks:
- Verifies JWT signature using Cloudflare's public certificates that are fetched and cached
- Validates all standard JWT claims (expiration, not before, issuer)
- Checks token type is "app"
- Implements proper certificate caching with mutex protection
- Uses RSA public key validation
- Includes comprehensive logging for security events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Cloudflare JWT validation implementation
rg -A 10 "IsEnabled.*Context"
Length of output: 51
Script:
#!/bin/bash
# Let's try different search patterns to find Cloudflare related implementations
rg -l "CloudflareAccess|IsEnabled" --type go
# Then search for specific implementation details
rg -A 10 "type CloudflareAccess" --type go
# Also look for any JWT validation related code
rg -A 10 "jwt.*validate|validate.*jwt" --type go -i
Length of output: 1719
Script:
#!/bin/bash
# Let's look at the IsEnabled implementation and JWT validation
rg -A 20 "func.*IsEnabled" internal/security/cloudflare.go
# Also check for any token validation logic
rg -A 20 "func.*[Vv]alidate" internal/security/cloudflare.go
# And look at the test file to understand the validation coverage
cat internal/security/cloudflare_test.go
Length of output: 8495
Script:
#!/bin/bash
# Let's see the complete implementation of VerifyAccessJWT since we only saw its beginning
cat internal/security/cloudflare.go
Length of output: 6755
internal/security/oauth_test.go (2)
1-14: LGTM! Package structure and imports are well organized.
The imports cover all necessary packages for OAuth2 testing, including HTTP testing utilities, session management, and the Echo framework.
56-111: 🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
The current table-driven test structure is good, but:
- The function name suggests testing invalid tokens, but only tests valid ones
- Missing critical test cases
- Contains duplicated setup code
Consider these improvements:
-func TestIsUserAuthenticated(t *testing.T) {
+func TestIsUserAuthenticatedScenarios(t *testing.T) {
+ // Use fixed time for deterministic testing
+ now := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
+
tests := []struct {
name string
token string
expires time.Duration
want bool
}{
{
name: "valid token",
token: "valid_token",
expires: time.Hour,
want: true,
},
+ {
+ name: "expired token",
+ token: "expired_token",
+ expires: -time.Hour, // expired 1 hour ago
+ want: false,
+ },
+ {
+ name: "empty token",
+ token: "",
+ expires: time.Hour,
+ want: false,
+ },
+ {
+ name: "about to expire token",
+ token: "expiring_token",
+ expires: time.Minute * 5, // expires in 5 minutes
+ want: true,
+ },
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
+ t.Cleanup(func() {
+ gothic.Store = nil
+ })Also consider extracting the common setup code into a helper function to reduce duplication between tests.
Let's check if there are any existing helper functions we could reuse:
#!/bin/bash
# Search for test helper functions
ast-grep --pattern 'func setup$_Test($_) $_'views/settings/templates/passwordField.html (1)
1-12: 🛠️ Refactor suggestion
Enhance password validation constraints.
While the basic validation is in place, consider strengthening the security by:
- Adding a minimum length requirement
- Implementing password complexity requirements
- Using a more restrictive pattern than ".+"
- pattern=".+"
+ pattern="^(?=.*[A-Za-z])(?=.*\d)[A-Za-z\d]{8,}$"This pattern requires at least 8 characters with at least one letter and one number.
Likely invalid or redundant comment.
views/index.html (2)
100-109: Verify HTTPS redirect implementation across the application.
The HTTPS redirect logic looks correct, but let's verify the security settings implementation.
Let's check the security settings implementation:
#!/bin/bash
# Search for RedirectToHTTPS usage in Go files
rg -t go "RedirectToHTTPS"
# Search for HTTPS redirect implementation in handlers
ast-grep --pattern 'if req.TLS == nil {
$$$
}'41-44: LGTM! Clean implementation of conditional content loading.
The HTMX integration for the login modal is well-implemented with proper event handling.
Let's verify the PreloadFragment template variable:
✅ Verification successful
PreloadFragment implementation verified and correctly used
The PreloadFragment variable is properly defined in the RenderData struct and is being used correctly in the template. The implementation shows:
- Variable is declared in
internal/httpcontroller/routes.goas part of RenderData - It's populated with the escaped request URI in routes.go
- Template usage in views/index.html follows the correct conditional pattern for both login modal and general content preloading
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PreloadFragment usage in handlers and templates
rg -t go "PreloadFragment"
Length of output: 294
Script:
#!/bin/bash
# Let's search for PreloadFragment in all file types since the previous search was limited to Go files
rg "PreloadFragment"
# Also search for template variable declarations in handlers
ast-grep --pattern 'type $_ struct {
$$$
PreloadFragment $_
$$$
}'
Length of output: 1359
doc/security.md (1)
11-20: Enhance security-related documentation for basic authentication.
While the configuration example is clear, consider adding the following security-related information:
- Add a note about keeping
clientidandclientsecretsecure and not committing them to version control - Document HTTPS requirements for OAuth2
- Include password complexity recommendations
- Add information about token expiration and session management
Let's verify if HTTPS is enforced in the codebase:
✅ Verification successful
Security configuration validation is properly implemented
The review comment's suggestions are already well-addressed in the codebase:
- The security settings validation in
internal/conf/validate.goensures proper configuration of authentication. - HTTPS is enforced through AutoTLS when enabled, as seen in
internal/httpcontroller/server.go. - OAuth2 implementation in
internal/security/oauth.gofollows security best practices including:- Secure token generation and validation
- Token expiration handling
- Session management
- HTTPS requirement (through host validation)
The security documentation should be updated to include:
security:
# Enable HTTPS with automatic certificate management
autotls: true
# Redirect all HTTP traffic to HTTPS
redirecttohttps: true
host: "your.domain.com"
basicauth:
enabled: true
# Other settings...🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for HTTPS enforcement in the codebase
rg -l "http.ListenAndServe|http.Server"
Length of output: 72
Script:
#!/bin/bash
# Let's examine the HTTPS enforcement in endpoint.go and any other security-related configurations
rg -A 5 "http.ListenAndServe|http.Server" internal/telemetry/endpoint.go
# Also search for TLS/HTTPS related configurations
rg -l "TLSConfig|tls.Config|https"
# Look for any OAuth2 related implementations
rg -l "oauth2|OAuth"
Length of output: 1661
Script:
#!/bin/bash
# Let's check the OAuth2 implementation and HTTPS enforcement
cat internal/security/oauth.go
# Check the server configuration
cat internal/httpcontroller/server.go
# Examine the configuration validation
cat internal/conf/validate.go
# Look at the default configuration
cat internal/conf/defaults.go
Length of output: 28667
views/elements/login.html (3)
33-44: LGTM! Well-implemented action buttons.
Good implementation with:
- Proper ARIA labels
- Form validation
- Loading indicators
- Clear user feedback
69-78: LGTM! Well-implemented modal close functionality.
Good implementation with proper accessibility attributes and multiple ways to close the modal.
1-4: Verify redirect URI validation and CSRF protection.
The hidden redirect URI field could be vulnerable to open redirect attacks if not properly validated server-side. Additionally, ensure CSRF protection is implemented for the form submission.
assets/custom.css (3)
21-21: LGTM: Improved mobile responsiveness
The addition of margin: auto ensures proper centering of confidence indicators on mobile screens.
81-83: LGTM: Appropriate text sizing utility
The .text-2xs utility class provides a good size option for compact displays.
90-109: LGTM: Well-implemented skeleton loader
The audio player skeleton loader is well-implemented with proper aspect ratio handling and gradient background.
internal/httpcontroller/server.go (3)
7-7: LGTM: New security-related imports and struct fields.
The additions of OAuth2Server and CloudflareAccess fields to the Server struct, along with their corresponding imports, are well-structured and align with the PR's security enhancement objectives.
Also applies to: 17-17, 27-28
78-78: LGTM: Proper AutoTLS configuration
The migration of TLS settings to the Security configuration and the updated host policy implementation are well-structured. The error handling and configuration paths are properly managed.
Also applies to: 87-87, 101-101
52-53: Verify the nil parameter passed to handlers.New()
While the OAuth2Server initialization looks good, there's a nil parameter being passed to handlers.New(). This should be documented or its purpose should be clarified.
Also applies to: 56-57, 65-65
internal/httpcontroller/routes.go (4)
25-25: Well-structured security-related additions to data structures.
The new security-related fields and structures are well-designed, providing a clean way to track authentication state and requirements across routes.
Also applies to: 36-40, 51-51
69-74: Appropriate authorization requirements for sensitive routes.
All settings-related routes are correctly marked as requiring authorization, providing proper access control for sensitive configurations.
79-84: Verify AuthMiddleware implementation and error handling.
While the conditional middleware application is correct, ensure that:
- AuthMiddleware properly handles and reports authentication failures
- Failed authentication attempts are logged
- Users are appropriately redirected to the login page
#!/bin/bash
# Search for AuthMiddleware implementation
ast-grep --pattern 'func (s *Server) AuthMiddleware(next echo.HandlerFunc) echo.HandlerFunc {
$$$
}'59-61: Verify the implementation of initAuthRoutes().
The OAuth2 routes initialization is placed correctly, but we should verify its implementation.
✅ Verification successful
Let me search for the actual initAuthRoutes implementation since the previous search didn't find it.
Implementation of initAuthRoutes() is properly defined and complete
The implementation in internal/httpcontroller/auth_routes.go correctly sets up:
- OAuth2 routes (
/oauth2/authorize,/oauth2/token,/callback) - Social authentication routes (
/auth/:provider,/auth/:provider/callback) - Basic authentication routes
The initialization is well-structured and includes all necessary authentication endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the initAuthRoutes implementation
ast-grep --pattern 'func (s *Server) initAuthRoutes() {
$$$
}'
Length of output: 1930
Script:
#!/bin/bash
# Search for files containing initAuthRoutes
rg -l "initAuthRoutes"
# Show the content of files containing initAuthRoutes
rg "initAuthRoutes" -A 10
Length of output: 2329
internal/httpcontroller/template_functions.go (4)
6-6: LGTM: Import additions are appropriate.
The new imports are properly justified by the added functionality - errors for enhanced error handling and handlers for the new hourly counts feature.
Also applies to: 15-15
24-28: LGTM: Template function map updates are well-structured.
The new template functions are properly registered and follow the established naming conventions.
Also applies to: 47-48
52-64: LGTM: Enhanced addFunc with variadic parameters.
The function has been improved to handle multiple numbers, with clear documentation and a safe implementation.
69-89: LGTM: Well-implemented dictionary function.
The dictFunc implementation includes proper error handling, type checking, and clear documentation.
internal/conf/config.yaml (4)
87-87: LGTM! Good practice to use empty string default.
Removing the placeholder "00000" value in favor of an empty string is a better practice as it makes it clear when the ID needs to be configured.
142-151: 🛠️ Refactor suggestion
Add validation for OAuth2 provider configurations.
For both Google and GitHub OAuth2:
- Validate client IDs and secrets when enabled
- Ensure user ID format matches provider requirements
- Consider adding allowed redirect URI patterns
Let's check for existing OAuth2 validation:
#!/bin/bash
# Search for OAuth2 validation logic
rg -A 5 "oauth.*validation|validateOAuth"134-141:
Security concern: Basic auth configuration needs enhancement.
Several improvements needed for basic auth:
- Add password complexity requirements
- Consider making
clientsecretrequired when basic auth is enabled - Add validation for redirect URI format
- Document the expected password hash format
Let's check for any existing password validation:
#!/bin/bash
# Search for password validation logic
rg -A 5 "password.*validation|validatePassword"126-133:
Security concern: Ensure HTTPS usage with OAuth2.
When OAuth2 is enabled, HTTPS should be mandatory. Consider:
- Automatically enabling
redirecttohttpswhen any OAuth2 provider is enabled - Adding validation to prevent OAuth2 activation without proper HTTPS configuration
Let's verify if there are any OAuth2-related routes that don't enforce HTTPS:
internal/conf/defaults.go (2)
6-7: LGTM: Import changes are appropriate.
The addition of the "time" import is correctly placed and properly utilized in the configuration.
163-170: Security configuration needs additional safeguards.
While the security configuration provides flexibility, some default values warrant careful consideration:
- An empty default host (
security.host) might allow unintended access. Consider setting a more restrictive default like "localhost". - The subnet bypass feature could potentially weaken security if not properly configured.
Let's verify the security implications:
Consider implementing the following security enhancements:
- Add validation for subnet CIDR notation
- Implement rate limiting for bypass attempts
- Add logging for security-related configuration changes
internal/security/cloudflare.go (1)
1-16: LGTM: Clean package structure and appropriate imports.
The imports are well-organized and all are actively used in the implementation.
views/elements/sidebar.html (3)
Line range hint 1-16: LGTM! Well-structured Alpine.js setup.
The implementation properly handles route tracking and HTMX navigation updates with good separation of concerns.
98-130: Add client-side validation for logout function.
The logout button calls an undefined logout() function. Ensure this function is properly defined and handles errors appropriately.
#!/bin/bash
# Description: Verify the logout function implementation
# Search for the logout function definition in JavaScript files
rg -A 5 "function logout\(\)" --type js62-93: Verify security condition implementation.
The security conditions look correct, but let's verify that .Security.Enabled and .Security.AccessAllowed are properly set in the handler.
✅ Verification successful
Based on the search results, I can now generate a final response as we have found the security-related implementations:
Security conditions are properly implemented
The security conditions in the sidebar template are correctly implemented and backed by the codebase:
- The
Securitystruct is defined ininternal/httpcontroller/routes.gowith both required fields:EnabledandAccessAllowed - Security settings are properly validated through
validateSecuritySettings()ininternal/conf/validate.go - The authentication system supports multiple providers (Basic, Google, GitHub) and includes subnet bypass options
- The security context is properly prepared and passed to templates through the HTTP controller routes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the security conditions are properly set in handlers
# Search for handler implementations setting these security flags
rg -A 5 "Security.*Enabled|Security.*AccessAllowed" --type go
# Search for the Security struct definition
ast-grep --pattern 'type Security struct {
$$$
Enabled bool
AccessAllowed bool
$$$
}'
Length of output: 373
Script:
#!/bin/bash
# Let's try different approaches to find security-related code
# Search for Security type definition
rg -l "type\s+Security\s+struct" --type go
# Search for security-related fields in structs
rg "Security\s+struct|Enabled\s+bool|AccessAllowed\s+bool" --type go
# Search for security middleware or handlers
rg "func.*Security|security.*Handler" --type go
# Search for files containing both "Security" and "Enabled"
rg -l "Security.*Enabled" --type go
# Look for template data preparation
rg "Security.*=|\.Security\." --type go
Length of output: 7477
internal/security/basic_test.go (1)
1-14: LGTM! Package structure and imports are well organized.
The imports cover all necessary packages for HTTP handler testing and OAuth2 functionality.
internal/conf/validate.go (2)
9-9: LGTM: Appropriate import for CIDR validation.
The addition of the "net" package is necessary for subnet validation in the security settings.
45-48: LGTM: Well-integrated security validation.
The security settings validation is properly integrated into the validation chain, following the established error handling pattern.
internal/httpcontroller/handlers/settings.go (3)
8-12: LGTM: SaveSettings modifications are well-placed
The new imports and the placement of updateAuthenticationSettings call ensure proper validation and configuration of authentication settings before saving.
Also applies to: 72-74
125-138: LGTM: Solid OAuth2 redirect URI configuration
The function correctly configures redirect URIs for all supported authentication providers using the validated host.
139-161: Consider enhancing authentication settings security and flexibility
While the implementation is good, consider these improvements:
- Add validation for existing secrets' strength
- Make expiration times configurable via settings
- Add validation for OAuth provider configurations (client IDs, secrets)
Let's verify the OAuth provider configurations:
Example improvements:
func (h *Handlers) updateAuthenticationSettings(settings *conf.Settings) {
// ... existing host validation ...
basicAuth := &settings.Security.BasicAuth
if basicAuth.Enabled {
+ // Validate existing secrets
+ if len(basicAuth.ClientSecret) < 32 {
+ basicAuth.ClientSecret = conf.GenerateRandomSecret()
+ }
+
+ // Use configurable expiration times
+ if basicAuth.AuthCodeExp == 0 {
+ basicAuth.AuthCodeExp = settings.Security.DefaultAuthCodeExp
+ if basicAuth.AuthCodeExp == 0 {
+ basicAuth.AuthCodeExp = 10 * time.Minute
+ }
+ }
+
+ // Validate OAuth provider configuration
+ if settings.Security.GoogleAuth.Enabled {
+ if settings.Security.GoogleAuth.ClientID == "" ||
+ settings.Security.GoogleAuth.ClientSecret == "" {
+ h.SSE.SendNotification(Notification{
+ Message: "Google OAuth configuration is incomplete",
+ Type: "error",
+ })
+ }
+ }
}
// ... rest of the function ...
}internal/conf/config.go (1)
276-276: LGTM!
The addition of the Security field to the Settings struct is clean and well-documented.
assets/tailwind.css (4)
1139-1144: LGTM: Button circle style implementation looks good
The .btn-circle class implementation provides proper circular button styling with correct dimensions and border radius.
1310-1329: LGTM: Divider component implementation is well structured
The .divider class and its pseudo-elements provide a clean implementation for creating horizontal dividers with proper spacing and styling. The use of before and after pseudo-elements for the lines is a good pattern.
Line range hint 1758-1836: LGTM: Modal implementation follows best practices
The modal implementation includes:
- Proper z-indexing and positioning
- Backdrop handling
- Animation properties
- Responsive sizing
- Accessibility considerations (pointer-events, visibility)
2820-2823: LGTM: Loading spinner implementation
The .loading-spinner class correctly implements the spinner animation using SVG and mask properties.
internal/security/basic.go (1)
65-68: Potential session management issue with gothic
Storing the access token in the session using gothic.StoreInSession may cause issues if the session store isn't properly configured or if gothic isn't intended for this use case.
Please verify that gothic is correctly set up for session management in this context.
Run the following script to search for gothic session store initialization:
✅ Verification successful
Gothic session store is properly configured and used consistently
The codebase shows proper initialization of the Gothic session store using a secure session secret:
- Session store is initialized in
internal/security/oauth.gousingsessions.NewCookieStore - The session secret is obtained from security settings
- The store is consistently used across authentication flows for storing tokens and user data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for gothic session store initialization
# Expect: Initialization of gothic session store in the codebase
rg 'gothic\.Store' -A 5
Length of output: 3791
internal/security/oauth.go (1)
61-61: Verify the strength of the session secret
Ensure that the SessionSecret used to initialize the session store is a sufficiently random and long string (recommended at least 32 characters) to provide adequate security for session cookies.
internal/security/cloudflare_test.go (1)
147-183:
Ensure thread-safe access to certs map in concurrent tests
In TestConcurrentAccessToCertsMap, multiple goroutines call fetchCerts concurrently, which accesses the certs map. If fetchCerts doesn't properly synchronize access to certs, this can lead to data races and undefined behavior. Verify that fetchCerts includes appropriate synchronization mechanisms like mutexes.
Run the following script to search for unsynchronized access to certs:
| redirectPath := url.QueryEscape(c.Request().URL.Path) | ||
| if c.Request().Header.Get("HX-Request") == "true" { | ||
| c.Response().Header().Set("HX-Redirect", "/login?redirect="+redirectPath) | ||
| return c.String(http.StatusUnauthorized, "") | ||
| } | ||
| return c.Redirect(http.StatusFound, "/login?redirect="+redirectPath) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add additional security measures for redirect handling
While the redirect path is URL encoded, consider these security enhancements:
- Implement a whitelist of allowed redirect paths
- Add CSRF protection for the authentication flow
- Consider adding rate limiting for failed authentication attempts
redirectPath := url.QueryEscape(c.Request().URL.Path)
+// Validate redirect path against whitelist
+if !isAllowedRedirectPath(redirectPath) {
+ redirectPath = "/"
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Suggestion added. Adding CSRF will be needed addition, let's consider having that in the future.
There was a problem hiding this comment.
@petterip, glad to hear that you've added the suggestion. Adding CSRF protection would indeed be a valuable enhancement. Would you like me to open a new GitHub issue to track this for future consideration?
There was a problem hiding this comment.
Why not, please do.
There was a problem hiding this comment.
@petterip, I've created the GitHub issue to track adding CSRF protection to the authentication flow: Issue #314.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to track adding CSRF protection to the authentication flow
gh issue create \
--title "Add CSRF protection to authentication flow" \
--body "As discussed in [PR #313](https://github.com/tphakala/birdnet-go/pull/313) and [this comment](https://github.com/tphakala/birdnet-go/pull/313#discussion_r1828165867), @petterip suggested adding CSRF protection to the authentication flow in the future to enhance security." \
--assignee petterip
Length of output: 421
| s.Echo.Use(s.AuthMiddleware) | ||
| s.Echo.Use(middleware.Recover()) | ||
| s.Echo.Use(middleware.GzipWithConfig(middleware.GzipConfig{ | ||
| Level: 6, | ||
| MinLength: 2048, | ||
| })) | ||
| // Apply the Cache Control Middleware | ||
| s.Echo.Use(CacheControlMiddleware()) | ||
| // Apply a middleware to set the Vary: HX-Request header for all responses | ||
| s.Echo.Use(VaryHeaderMiddleware()) |
There was a problem hiding this comment.
💡 Codebase verification
Security headers need to be implemented at middleware level
While Content-Security-Policy and X-Content-Type-Options are set via HTML meta tags in views/index.html, critical security headers should be implemented at the HTTP response level through middleware for consistent application across all responses. Required headers not currently set in middleware:
- Content-Security-Policy
- X-Frame-Options
- X-Content-Type-Options
- Strict-Transport-Security
Consider adding a new SecurityHeadersMiddleware that sets these headers for all responses. Meta tag implementations should be kept as a defense-in-depth measure.
🔗 Analysis chain
Verify security headers implementation
Important security headers should be set for all responses. Please verify if these are implemented elsewhere:
- Content-Security-Policy
- X-Frame-Options
- X-Content-Type-Options
- Strict-Transport-Security
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for security headers implementation
rg -A 5 'Content-Security-Policy|X-Frame-Options|X-Content-Type-Options|Strict-Transport-Security'
Length of output: 692
Script:
#!/bin/bash
# Search for security headers in middleware implementations
ast-grep --pattern 'func $$$HeaderMiddleware() $_ {
$$$
}'
# Also check for direct header setting in middleware.go
rg -A 5 'Header\(\)\.Set|Header\(\)\.Add' internal/httpcontroller/middleware.go
Length of output: 683
There was a problem hiding this comment.
Tried this, but broke fonts, the map and tooltips. Has to be studied more in the future.
There was a problem hiding this comment.
@petterip, understood. I've noted this information for future reviews.
✏️ Learnings added
Learnt from: petterip
PR: tphakala/birdnet-go#313
File: internal/httpcontroller/middleware.go:14-22
Timestamp: 2024-11-06T20:10:04.321Z
Learning: Implementing security headers via middleware in `internal/httpcontroller/middleware.go` caused issues with fonts, the map, and tooltips. Further investigation is needed before integrating.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
internal/security/oauth_test.go
Outdated
| // TestIsUserAuthenticatedValidAccessToken tests the IsUserAuthenticated function with a valid access token | ||
| func TestIsUserAuthenticatedValidAccessToken(t *testing.T) { | ||
| settings := &conf.Settings{ | ||
| Security: conf.Security{ | ||
| SessionSecret: "test-secret", | ||
| }, | ||
| } | ||
|
|
||
| s := NewOAuth2Server(settings) | ||
|
|
||
| // Initialize gothic exactly as in production | ||
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | ||
| gothic.SetState = func(req *http.Request) string { | ||
| return "" | ||
| } | ||
|
|
||
| e := echo.New() | ||
| req := httptest.NewRequest(http.MethodGet, "/", nil) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| // Store token using gothic's method | ||
| gothic.StoreInSession("access_token", "valid_token", req, rec) | ||
|
|
||
| // Add cookie to request | ||
| req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) | ||
|
|
||
| // Add token to OAuth2Server's valid tokens | ||
| s.accessTokens["valid_token"] = AccessToken{ | ||
| Token: "valid_token", | ||
| ExpiresAt: time.Now().Add(time.Hour), | ||
| } | ||
|
|
||
| isAuthenticated := s.IsUserAuthenticated(c) | ||
|
|
||
| if !isAuthenticated { | ||
| t.Errorf("Expected IsUserAuthenticated to return true, got false") | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Test coverage needs improvement for token authentication
The review comment is correct. The codebase only tests the happy path for token authentication:
TestIsUserAuthenticatedValidAccessTokentests a single success caseTestIsUserAuthenticatedis set up for table-driven tests but only includes one test case for valid token- No tests exist for expired tokens, invalid tokens, or edge cases around token expiration
Key improvements needed:
- Add test cases for expired tokens, invalid tokens, and edge cases
- Implement test cleanup to prevent state pollution
- Use constants for test values
- Use fixed time for deterministic testing
🔗 Analysis chain
Consider improving test coverage and reliability.
While the basic success case is covered, there are several improvements that could make this test more robust:
- Add test cleanup to prevent test pollution
- Use constants for test values
- Add edge cases for token expiration
- Use fixed time for deterministic testing
Consider applying these improvements:
+const (
+ testSessionSecret = "test-secret"
+ testValidToken = "valid_token"
+)
+
func TestIsUserAuthenticatedValidAccessToken(t *testing.T) {
+ // Use fixed time for deterministic testing
+ now := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
+
settings := &conf.Settings{
Security: conf.Security{
- SessionSecret: "test-secret",
+ SessionSecret: testSessionSecret,
},
}
s := NewOAuth2Server(settings)
+ t.Cleanup(func() {
+ // Clean up the session store
+ gothic.Store = nil
+ })
+
// Initialize gothic exactly as in production
gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret))Also consider adding test cases for:
- Expired tokens
- Tokens about to expire
- Invalid token format
Let's verify if there are any existing test cases for these scenarios:
Let's search for test files and authentication-related test functions to better understand the test coverage.
I noticed there's another test function TestIsUserAuthenticated that might contain the edge cases we're looking for. Let's examine its implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing token expiration test cases
rg -A 5 "expired|invalid.*token|expir.*token" "internal/security"
Length of output: 140
Script:
#!/bin/bash
# Search for test files in the security package
fd ".*_test.go" "internal/security"
# Then let's look for all test functions related to authentication and tokens
rg "func Test.*(?i)(token|auth)" "internal/security" -A 5
Length of output: 3868
Script:
#!/bin/bash
# Let's look at the TestIsUserAuthenticated function which might contain the edge cases
rg "func TestIsUserAuthenticated\b" internal/security/oauth_test.go -A 20
Length of output: 494
| <input | ||
| class="input input-sm input-bordered w-full pr-10" | ||
| :class="{ 'invalid': touched && !validInput() }" | ||
| :type="showPassword ? 'text' : 'password'" | ||
| :name="name" | ||
| :placeholder="placeholder" | ||
| id="{{.id}}" | ||
| x-model="{{.model}}" | ||
| x-ref="{{.id}}" | ||
| pattern=".+" | ||
| @blur="touched = true" | ||
| @invalid="touched = true;" | ||
| @input="touched = false;" | ||
| autocomplete="off" | ||
| required | ||
| maxlength="255" | ||
| {{if .disabled}}x-bind:disabled="{{.disabled}}"{{end}} | ||
| aria-required="true" | ||
| aria-invalid="touched && !{{.model}}" | ||
| aria-describedby="{{.id}}-error {{.id}}-tooltip"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider autocomplete attribute for password fields.
Setting autocomplete="off" might not be the best practice for password fields. Consider using more specific values based on the context:
- For new password fields:
autocomplete="new-password" - For current password fields:
autocomplete="current-password"
- autocomplete="off"
+ autocomplete="new-password"Committable suggestion skipped: line range outside the PR's diff.
| {{define "passwordField"}} | ||
|
|
||
| <!-- Password field with a toggle showing and hiding the content --> | ||
| <div class="form-control relative {{.class}}" x-data="{ | ||
| showPassword: false, | ||
| showTooltip: null, | ||
| touched: false, | ||
| validInput() { | ||
| return this.$refs.{{.id}}.validity.valid; | ||
| }, | ||
| ...inputField('{{.id}}', '{{.label}}', '{{.model}}', '{{.name}}', '{{.placeholder}}') | ||
| }"> | ||
| <!-- Label --> | ||
| <label class="label justify-start" for="{{.id}}"> | ||
| <span class="label-text capitalize">{{.label}}</span> | ||
| <span class="ml-2 text-sm text-gray-500 cursor-help" @mouseenter="showTooltip = '{{.id}}'" | ||
| @mouseleave="showTooltip = null">ⓘ</span> | ||
| </label> | ||
|
|
||
| <div class="relative w-full"> | ||
| <input | ||
| class="input input-sm input-bordered w-full pr-10" | ||
| :class="{ 'invalid': touched && !validInput() }" | ||
| :type="showPassword ? 'text' : 'password'" | ||
| :name="name" | ||
| :placeholder="placeholder" | ||
| id="{{.id}}" | ||
| x-model="{{.model}}" | ||
| x-ref="{{.id}}" | ||
| pattern=".+" | ||
| @blur="touched = true" | ||
| @invalid="touched = true;" | ||
| @input="touched = false;" | ||
| autocomplete="off" | ||
| required | ||
| maxlength="255" | ||
| {{if .disabled}}x-bind:disabled="{{.disabled}}"{{end}} | ||
| aria-required="true" | ||
| aria-invalid="touched && !{{.model}}" | ||
| aria-describedby="{{.id}}-error {{.id}}-tooltip"> | ||
|
|
||
| <button type="button" | ||
| @click="showPassword = !showPassword" | ||
| aria-label="Toggle password visibility" | ||
| :aria-pressed="showPassword" | ||
| class="absolute inset-y-0 right-0 pr-3 flex items-center"> | ||
| <svg x-show="!showPassword" xmlns="http://www.w3.org/2000/svg" class="h-5 w-5 text-gray-400" | ||
| viewBox="0 0 20 20" fill="currentColor" role="img" aria-label="Show password"> | ||
| <path d="M10 12a2 2 0 100-4 2 2 0 000 4z" /> | ||
| <path fill-rule="evenodd" | ||
| d="M.458 10C1.732 5.943 5.522 3 10 3s8.268 2.943 9.542 7c-1.274 4.057-5.064 7-9.542 7S1.732 14.057.458 10zM14 10a4 4 0 11-8 0 4 4 0 018 0z" | ||
| clip-rule="evenodd" /> | ||
| </svg> | ||
| <svg x-show="showPassword" xmlns="http://www.w3.org/2000/svg" class="h-5 w-5 text-gray-400" | ||
| viewBox="0 0 20 20" fill="currentColor" role="img" aria-label="Hide password"> | ||
| <path fill-rule="evenodd" | ||
| d="M3.707 2.293a1 1 0 00-1.414 1.414l14 14a1 1 0 001.414-1.414l-1.473-1.473A10.014 10.014 0 0019.542 10C18.268 5.943 14.478 3 10 3a9.958 9.958 0 00-4.512 1.074l-1.78-1.781zm4.261 4.26l1.514 1.515a2.003 2.003 0 012.45 2.45l1.514 1.514a4 4 0 00-5.478-5.478z" | ||
| clip-rule="evenodd" /> | ||
| <path | ||
| d="M12.454 16.697L9.75 13.992a4 4 0 01-3.742-3.741L2.335 6.578A9.98 9.98 0 00.458 10c1.274 4.057 5.065 7 9.542 7 .847 0 1.669-.105 2.454-.303z" /> | ||
| </svg> | ||
| </button> | ||
| </div> | ||
|
|
||
| <!-- Validation message --> | ||
| <span class="text-sm text-red-500" | ||
| id="{{.id}}-error" | ||
| role="alert" | ||
| aria-live="polite" | ||
| x-show="touched && !$refs.{{.id}}.validity.valid" | ||
| x-cloak> | ||
| Please enter a valid {{.label}} | ||
| </span> | ||
|
|
||
| <!-- Tooltip --> | ||
| <div x-show="showTooltip === '{{.id}}'" | ||
| id="{{.id}}-tooltip" | ||
| role="tooltip" | ||
| aria-hidden="true" | ||
| x-cloak | ||
| class="absolute left-0 bottom-full mb-2 p-2 bg-gray-100 text-sm rounded shadow-md w-96"> | ||
| {{.tooltip}} | ||
| </div> | ||
| </div> | ||
| {{end}} |
There was a problem hiding this comment.
💡 Codebase verification
Security inconsistencies found in password field implementations
The codebase has inconsistent password field implementations that need to be standardized using the secure passwordField template:
views/settings.html: Uses custom implementation without proper security attributes and validationviews/settings/outputSettings.html: Uses basic implementation missing visibility toggle and security featuresviews/elements/login.html: Missing password visibility toggle feature- Legacy password fields in
views/settings/integrationSettings.html(commented out but should be removed)
The new passwordField template is already being used correctly in:
- Security settings (basic auth, Google auth, GitHub auth)
- Integration settings (newer implementations)
Recommendations:
- Replace all custom password field implementations with the secure
passwordFieldtemplate - Remove commented-out legacy password field implementations
- Update login form to use the standardized password component
🔗 Analysis chain
Verify consistent password field implementation across the codebase.
Since this is a security-related component, we should ensure consistent implementation across all password fields in the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other password field implementations
echo "Searching for other password input fields..."
rg -l 'type=["'\'']password["'\'']' --type html
# Search for other Alpine.js password visibility toggle implementations
echo "Searching for other password visibility toggle implementations..."
rg 'showPassword' --type html
# Search for other autocomplete attributes on password fields
echo "Checking autocomplete attributes on password fields..."
rg 'autocomplete=["'\''][^"'\'']*["'\''].*type=["'\'']password["'\'']' --type html
Length of output: 2289
Script:
#!/bin/bash
# Let's examine the password field implementations in detail
echo "=== Password field in settings.html ==="
rg -A 10 -B 2 'type=["'\'']password["'\'']' views/settings.html
echo "=== Password field in outputSettings.html ==="
rg -A 10 -B 2 'type=["'\'']password["'\'']' views/settings/outputSettings.html
echo "=== Password field in settingsBase.html ==="
rg -A 10 -B 2 'type=["'\'']password["'\'']' views/settings/settingsBase.html
echo "=== Password field in integrationSettings.html ==="
rg -A 10 -B 2 'type=["'\'']password["'\'']' views/settings/integrationSettings.html
echo "=== Password field in login.html ==="
rg -A 10 -B 2 'type=["'\'']password["'\'']' views/elements/login.html
# Check if the passwordField template is consistently used
echo "=== Usage of passwordField template ==="
rg -B 2 'template ["'\'']passwordField["'\'']' --type html
Length of output: 7301
| func (s *OAuth2Server) GenerateAuthCode() (string, error) { | ||
| code := make([]byte, 32) | ||
| _, err := rand.Read(code) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| authCode := base64.URLEncoding.EncodeToString(code) | ||
|
|
||
| s.mutex.Lock() | ||
| defer s.mutex.Unlock() | ||
|
|
||
| s.authCodes[authCode] = AuthCode{ | ||
| Code: authCode, | ||
| ExpiresAt: time.Now().Add(s.Settings.Security.BasicAuth.AuthCodeExp), | ||
| } | ||
| return authCode, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential memory leak due to unbounded growth of authCodes map
The authCodes map stores authentication codes but does not remove expired codes unless they are exchanged. Over time, this can lead to unbounded memory growth if codes are not exchanged. Implement a cleanup mechanism to remove expired auth codes periodically.
| func (s *OAuth2Server) ValidateAccessToken(token string) bool { | ||
| s.mutex.RLock() | ||
| defer s.mutex.RUnlock() | ||
|
|
||
| accessToken, exists := s.accessTokens[token] | ||
| if !exists { | ||
| return false | ||
| } | ||
|
|
||
| return time.Now().Before(accessToken.ExpiresAt) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential memory leak due to unbounded growth of accessTokens map
The accessTokens map stores access tokens but does not remove expired tokens. This can lead to unbounded memory growth over time. Implement a mechanism to remove expired access tokens from the map, such as periodic cleanup or removal upon validation.
internal/security/oauth.go
Outdated
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | ||
|
|
||
| // Initialize Gothic providers | ||
| gothic.SetState = func(req *http.Request) string { | ||
| return "" // Gothic handles state automatically |
There was a problem hiding this comment.
Critical Security Issue: Missing CSRF protection in OAuth2 flow
Returning an empty string for the OAuth2 state parameter can make the application vulnerable to Cross-Site Request Forgery (CSRF) attacks. The state parameter should be a random, unique value to prevent CSRF attacks. Remove the custom SetState function to allow Gothic to handle the state parameter securely.
Apply this diff to fix the issue:
func InitializeGoth(settings *conf.Settings) {
// Set up the session store first
gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret))
- // Initialize Gothic providers
- gothic.SetState = func(req *http.Request) string {
- return "" // Gothic handles state automatically
- }
+ // Gothic will handle the state parameter automatically to prevent CSRF
googleProvider :=
gothGoogle.New(settings.Security.GoogleAuth.ClientID,📝 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.
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | |
| // Initialize Gothic providers | |
| gothic.SetState = func(req *http.Request) string { | |
| return "" // Gothic handles state automatically | |
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | |
| // Gothic will handle the state parameter automatically to prevent CSRF |
| for i, logMsg := range logs { | ||
| if strings.TrimSpace(logMsg) != strings.TrimSpace(expectedLogs[i]) { | ||
| t.Errorf("Log message mismatch. Expected: %s, Got: %s", expectedLogs[i], logMsg) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add length check before iterating over logs to prevent index out of range errors
In TestFetchCertsLogging, if the number of log messages does not match the number of expected logs, accessing expectedLogs[i] could cause an index out of range error. It's advisable to check the lengths before iterating.
Apply this diff to add a length check:
+ if len(logs) != len(expectedLogs) {
+ t.Fatalf("Expected %d log messages, but got %d", len(expectedLogs), len(logs))
+ }
for i, logMsg := range logs {
if strings.TrimSpace(logMsg) != strings.TrimSpace(expectedLogs[i]) {
t.Errorf("Log message mismatch. Expected: %s, Got: %s", expectedLogs[i], logMsg)
}
}📝 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.
| for i, logMsg := range logs { | |
| if strings.TrimSpace(logMsg) != strings.TrimSpace(expectedLogs[i]) { | |
| t.Errorf("Log message mismatch. Expected: %s, Got: %s", expectedLogs[i], logMsg) | |
| } | |
| } | |
| if len(logs) != len(expectedLogs) { | |
| t.Fatalf("Expected %d log messages, but got %d", len(expectedLogs), len(logs)) | |
| } | |
| for i, logMsg := range logs { | |
| if strings.TrimSpace(logMsg) != strings.TrimSpace(expectedLogs[i]) { | |
| t.Errorf("Log message mismatch. Expected: %s, Got: %s", expectedLogs[i], logMsg) | |
| } | |
| } |
internal/security/cloudflare_test.go
Outdated
| var logs logWriter | ||
| log.SetOutput(io.Discard) | ||
| log.SetFlags(0) | ||
| log.SetOutput(&logs) | ||
|
|
||
| ca := &CloudflareAccess{certs: make(map[string]string)} | ||
| err := ca.fetchCerts(server.URL) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("Expected no error, got %v", err) | ||
| } | ||
|
|
||
| expectedLogs := []string{ | ||
| fmt.Sprintf("Fetching Cloudflare certs from URL: %s/cdn-cgi/access/certs", server.URL), | ||
| "Added certificate with Kid: 1234", | ||
| "Added certificate with Kid: 5678", | ||
| } | ||
|
|
||
| for i, logMsg := range logs { | ||
| if strings.TrimSpace(logMsg) != strings.TrimSpace(expectedLogs[i]) { | ||
| t.Errorf("Log message mismatch. Expected: %s, Got: %s", expectedLogs[i], logMsg) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Restore original logger settings after the test to prevent side effects
Modifying global logger settings can affect other tests if not restored. It's important to save the original logger output and flags before changing them and restore them after the test completes.
Apply this diff to save and restore the logger settings:
var logs logWriter
+ originalOutput := log.Writer()
+ originalFlags := log.Flags()
+ defer func() {
+ log.SetOutput(originalOutput)
+ log.SetFlags(originalFlags)
+ }()
log.SetFlags(0)
log.SetOutput(&logs)📝 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.
| var logs logWriter | |
| log.SetOutput(io.Discard) | |
| log.SetFlags(0) | |
| log.SetOutput(&logs) | |
| ca := &CloudflareAccess{certs: make(map[string]string)} | |
| err := ca.fetchCerts(server.URL) | |
| if err != nil { | |
| t.Fatalf("Expected no error, got %v", err) | |
| } | |
| expectedLogs := []string{ | |
| fmt.Sprintf("Fetching Cloudflare certs from URL: %s/cdn-cgi/access/certs", server.URL), | |
| "Added certificate with Kid: 1234", | |
| "Added certificate with Kid: 5678", | |
| } | |
| for i, logMsg := range logs { | |
| if strings.TrimSpace(logMsg) != strings.TrimSpace(expectedLogs[i]) { | |
| t.Errorf("Log message mismatch. Expected: %s, Got: %s", expectedLogs[i], logMsg) | |
| } | |
| } | |
| } | |
| var logs logWriter | |
| originalOutput := log.Writer() | |
| originalFlags := log.Flags() | |
| defer func() { | |
| log.SetOutput(originalOutput) | |
| log.SetFlags(originalFlags) | |
| }() | |
| log.SetFlags(0) | |
| log.SetOutput(&logs) | |
| ca := &CloudflareAccess{certs: make(map[string]string)} | |
| err := ca.fetchCerts(server.URL) | |
| if err != nil { | |
| t.Fatalf("Expected no error, got %v", err) | |
| } | |
| expectedLogs := []string{ | |
| fmt.Sprintf("Fetching Cloudflare certs from URL: %s/cdn-cgi/access/certs", server.URL), | |
| "Added certificate with Kid: 1234", | |
| "Added certificate with Kid: 5678", | |
| } | |
| for i, logMsg := range logs { | |
| if strings.TrimSpace(logMsg) != strings.TrimSpace(expectedLogs[i]) { | |
| t.Errorf("Log message mismatch. Expected: %s, Got: %s", expectedLogs[i], logMsg) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
internal/security/oauth.go (2)
121-121: Avoid logging sensitive user informationThe log statement reveals the user's ID, which may be considered sensitive. To enhance privacy and security, consider logging less specific information.
Apply this diff to modify the log statement:
-log.Printf("User with userId is not allowed to login: %s", providedId) +log.Printf("Unauthorized login attempt detected.")
201-201: Remove unnecessary debug logging statementThe log statement appears to be a leftover from debugging and may not be necessary in production code.
Apply this diff to remove the log statement:
-log.Printf("*** %s", clientIP)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
internal/security/oauth.go(1 hunks)
🔇 Additional comments (1)
internal/security/oauth.go (1)
88-106: Ensure consistent user ID handling across providers
In the IsUserAuthenticated method, the userId retrieved from the session is used for both Google and GitHub authentication checks. User IDs may differ between providers. Verify that the userId corresponds correctly to the authenticated user for each provider to prevent authentication issues.
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (18)
internal/httpcontroller/middleware.go (2)
83-85: Enhance protected route detection.The current implementation only protects "/settings/" paths. Consider:
- Making protected paths configurable
- Adding support for multiple protected prefixes
- Including exact path matches
+var defaultProtectedPaths = []string{ + "/settings/", + "/admin/", + "/api/private/", +} + -func isProtectedRoute(path string) bool { - return strings.HasPrefix(path, "/settings/") +func isProtectedRoute(path string) bool { + path = strings.TrimSpace(path) + for _, protected := range defaultProtectedPaths { + if strings.HasPrefix(path, protected) { + return true + } + } + return false +}
14-22: Consider a phased approach for security headers.While previous attempts to add security headers caused issues with fonts, maps, and tooltips, consider:
- Implementing non-breaking headers first (X-Content-Type-Options, X-Frame-Options)
- Adding CSP in report-only mode to gather violations
- Gradually tightening CSP based on reports
Would you like assistance in creating a phased implementation plan for security headers?
doc/security.md (2)
1-6: Enhance the security overview section.Consider adding:
- Security implications of using multiple authentication methods
- Guidance on choosing the appropriate method based on use case
- Best practices for secure configuration
Add this content after line 5:
+ +When choosing an authentication method, consider: +- Basic Password Authentication: Suitable for personal deployments +- Social Authentication: Recommended for team deployments +- Authentication Bypass: Use with caution, only in secure networks + +> ⚠️ **Security Note**: When combining multiple authentication methods, ensure each method is properly configured and regularly audited. A misconfigured authentication method could compromise the security of your deployment.
74-74: Replace hard tab with spaces.For consistency with the rest of the file, replace the hard tab with spaces in the YAML indentation.
🧰 Tools
🪛 Markdownlint
74-74: Column: 4
Hard tabs(MD010, no-hard-tabs)
assets/custom.css (3)
25-27: Consider using CSS custom property for error colorThe implementation is good, but consider using a CSS custom property for better maintainability and theme support:
+:root { + --color-error: #dc2626; +} input.invalid { - border-color: #dc2626; + border-color: var(--color-error); }
71-73: Consider expanding the type scale systemWhile the
.text-2xsclass is useful, consider implementing a complete type scale system using CSS custom properties:+:root { + --text-3xs: 0.5rem; + --text-2xs: 0.6rem; + --text-xs: 0.75rem; +} .text-2xs { - font-size: 0.6rem; + font-size: var(--text-2xs); }
75-94: LGTM: Well-implemented skeleton loaderGood implementation of the skeleton loader with proper aspect ratio maintenance. Consider adding a subtle animation for better user experience:
.audio-player-container { background: linear-gradient(to bottom, rgba(128, 128, 128, 0.4), rgba(128, 128, 128, 0.1)); border-radius: 0.5rem; + animation: pulse 2s cubic-bezier(0.4, 0, 0.6, 1) infinite; } + +@keyframes pulse { + 0%, 100% { opacity: 1; } + 50% { opacity: 0.5; } +}internal/httpcontroller/routes.go (1)
36-40: Add documentation for the Security struct fields.The
Securitystruct fields would benefit from documentation explaining their specific purposes:
- What conditions set
Enabled?- What determines
AccessAllowed?- When is
IsCloudflaretrue and what are its implications?type Security struct { + // Enabled indicates whether authentication is currently active Enabled bool + // AccessAllowed indicates if the current request passes authentication checks AccessAllowed bool + // IsCloudflare indicates if the request is authenticated via Cloudflare Access IsCloudflare bool }internal/conf/config.yaml (2)
54-57: Document time duration format for maxage.The
maxagesetting uses a duration format (e.g., "30d"), but the accepted format and units should be clearly documented. Consider adding a comment specifying supported units (e.g., "h" for hours, "d" for days, "w" for weeks).retention: policy: usage # retention policy: none, age or usage - maxage: 30d # age policy: maximum age of clips to keep before starting evictions + maxage: 30d # age policy: maximum age of clips (format: ##d for days, ##h for hours, ##w for weeks) maxusage: 80% # usage policy: percentage of disk usage to trigger eviction🧰 Tools
🪛 yamllint
[error] 57-57: trailing spaces
(trailing-spaces)
145-154: Document OAuth2 provider configuration.For both Google and GitHub OAuth2 sections:
- Add links to provider documentation
- Document required OAuth2 scopes
- Explain user ID format requirements
googleauth: enabled: false # true to enable Google OAuth2 - clientid: "" # client id - clientsecret: "" # client secret - userid: "" # user id + clientid: "" # Client ID from Google Cloud Console + clientsecret: "" # Client secret from Google Cloud Console + userid: "" # Google account email or ID (format: email@domain.com) + # See: https://console.cloud.google.com/apis/credentials githubauth: enabled: false # true to enable GitHub OAuth2 - clientid: "" # client id - clientsecret: "" # client secret - userid: "" # user id + clientid: "" # Client ID from GitHub OAuth Apps + clientsecret: "" # Client secret from GitHub OAuth Apps + userid: "" # GitHub username + # See: https://github.com/settings/developersinternal/security/oauth.go (4)
24-43: Add godoc comments for exported typesThe exported types
AuthCode,AccessToken, andOAuth2Servershould have godoc comments explaining their purpose and fields.Add documentation comments like this:
+// AuthCode represents an OAuth2 authorization code with expiration type AuthCode struct { Code string ExpiresAt time.Time } +// AccessToken represents an OAuth2 access token with expiration type AccessToken struct { Token string ExpiresAt time.Time } +// OAuth2Server handles OAuth2 authentication flow and token management type OAuth2Server struct {
132-148: Add token entropy validationWhile using crypto/rand is good, additional entropy validation would enhance security.
Consider adding entropy validation:
func (s *OAuth2Server) GenerateAuthCode() (string, error) { code := make([]byte, 32) _, err := rand.Read(code) if err != nil { return "", err } + // Ensure sufficient entropy in the generated code + if !hasMinimumEntropy(code) { + return "", errors.New("insufficient entropy in generated code") + } authCode := base64.URLEncoding.EncodeToString(code)
229-256: Make cleanup interval configurableThe cleanup interval should be configurable through settings.
Consider making the interval configurable:
+ // Use configured cleanup interval or default to 1 hour + cleanupInterval := s.Settings.Security.TokenCleanupInterval + if cleanupInterval == 0 { + cleanupInterval = time.Hour + } - ticker := time.NewTicker(interval) + ticker := time.NewTicker(cleanupInterval)
1-266: Security Review SummaryThe OAuth2 implementation is generally solid but could benefit from these security enhancements:
- Rate limiting for authentication attempts
- Token entropy validation
- Secure handling of IP addresses in logs
- Configurable cleanup intervals
- Error handling for provider initialization
Additionally, please address the previously identified CSRF protection issue and memory leak concerns from the past reviews.
Consider implementing these security features in a separate security middleware layer for better separation of concerns.
internal/conf/config.go (1)
532-543: Make GenerateRandomSecret function private.Since this function is only used internally within the package, it should be unexported (private) to follow Go's encapsulation principles. Rename it from
GenerateRandomSecrettogenerateRandomSecret.-func GenerateRandomSecret() string { +func generateRandomSecret() string {internal/security/oauth_test.go (2)
17-57: Consolidate test functions to reduce code duplicationThe functions
TestIsUserAuthenticatedValidAccessTokenandTestIsUserAuthenticatedhave similar setup code and both test theIsUserAuthenticatedmethod with valid tokens. Consider merging these tests or refactoring the shared setup code into a helper function to reduce redundancy and improve maintainability.
19-19: Evaluate the necessity of callingconf.Setting()in testsIn your test functions, you call
conf.Setting()to set the settings instance. Since you are creating new settings instances within each test, this call might be redundant. Verify ifconf.Setting()is required for your tests; if not, consider removing it to simplify the test setup.Also applies to: 62-62, 121-121
internal/httpcontroller/auth_routes.go (1)
59-64: Store Non-Sensitive Identifiers in SessionStoring the user's email address in the session under the key
"userId"may have privacy implications. Consider using a unique identifier that doesn't expose personal information, such as a user ID or a hashed value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
assets/custom.css(4 hunks)doc/security.md(1 hunks)internal/conf/config.go(5 hunks)internal/conf/config.yaml(3 hunks)internal/conf/defaults.go(2 hunks)internal/conf/validate.go(3 hunks)internal/httpcontroller/auth_routes.go(1 hunks)internal/httpcontroller/middleware.go(3 hunks)internal/httpcontroller/routes.go(8 hunks)internal/httpcontroller/server.go(7 hunks)internal/security/cloudflare.go(1 hunks)internal/security/cloudflare_test.go(1 hunks)internal/security/oauth.go(1 hunks)internal/security/oauth_test.go(1 hunks)reset_auth.sh(1 hunks)views/index.html(3 hunks)views/settings/securitySettings.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/conf/validate.go
- internal/security/cloudflare.go
- reset_auth.sh
- views/index.html
- views/settings/securitySettings.html
🧰 Additional context used
📓 Learnings (1)
doc/security.md (1)
Learnt from: petterip
PR: tphakala/birdnet-go#313
File: doc/security.md:89-98
Timestamp: 2024-11-06T19:40:32.294Z
Learning: In the `doc/security.md`, when resetting authentication for Docker deployments of BirdNET-Go, use the command `docker exec $(docker ps | grep birdnet-go | awk '{print $1}') reset_auth.sh` as it works correctly. Alternative commands using `docker container ls --filter name=birdnet-go --quiet | xargs -r docker exec reset_auth.sh` do not work in this context.
🪛 LanguageTool
doc/security.md
[uncategorized] ~78-~78: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: .../connections/connect-apps/) - [Create a remotely-managed tunnel](https://developers.cloudflare.c...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 Markdownlint
doc/security.md
74-74: Column: 4
Hard tabs
(MD010, no-hard-tabs)
🪛 yamllint
internal/conf/config.yaml
[error] 133-133: trailing spaces
(trailing-spaces)
🔇 Additional comments (22)
assets/custom.css (3)
21-21: LGTM: Improved mobile responsiveness
The addition of margin: auto enhances the centering of confidence indicators on mobile devices.
55-69: LGTM: Well-structured responsive table implementation
The implementation provides a good progressive enhancement approach for different screen sizes, with clear breakpoints and logical display rules.
111-111: LGTM: Responsive thumbnail sizing
The use of viewport-relative units for max-width ensures thumbnails remain proportional across different screen sizes.
internal/httpcontroller/server.go (5)
7-7: LGTM: Clean integration of security components
The new imports and Server struct fields are well-organized and properly typed, providing a clean foundation for the authentication features.
Also applies to: 17-17, 27-28
78-78: LGTM: Well-structured TLS configuration
The AutoTLS settings are properly organized under the Security configuration section, maintaining good separation of concerns.
Also applies to: 87-87, 101-101
104-115: LGTM: Well-designed authentication flow
The authentication methods implement a clear and logical flow, checking Cloudflare Access before falling back to OAuth2. The separation of concerns and method visibility are well-thought-out.
117-126: Previous review comment about IP validation remains applicable
The current RealIP implementation still needs the enhancements suggested in the previous review regarding IP validation, spoofing protection, and IPv6 handling.
104-106: Verify authentication bypass configuration
Let's ensure the authentication bypass settings are properly validated and logged.
✅ Verification successful
Let me gather more specific information about the authentication bypass implementation.
Let me check one more aspect regarding the security settings and IP validation.
Let me check one final aspect about the subnet bypass implementation.
Authentication bypass configuration is properly implemented and validated
The implementation shows:
- Authentication state is correctly checked across all providers (Basic, Google, GitHub)
- Subnet bypass has proper validation:
- CIDR format validation for allowed subnets
- IP address parsing and validation
- Explicit enable/disable flag
- All bypass attempts are logged
- Configuration changes are validated at startup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in authentication bypass configuration
# Test: Look for bypass-related configuration validation
echo "Checking for bypass configuration validation..."
rg -l "bypass|whitelist|allowlist" --type go
# Test: Look for logging of bypass attempts
echo "Checking for bypass logging..."
rg "log|logger" -C 3 -g "**/*security*/**"
Length of output: 4942
Script:
#!/bin/bash
# Check the actual implementation of IsAuthenticationEnabled and related security checks
# Look for IsAuthenticationEnabled implementation
echo "Checking IsAuthenticationEnabled implementation..."
ast-grep --pattern 'func $_IsAuthenticationEnabled($$$) {
$$$
}'
# Check for bypass/allowlist configuration
echo "Checking bypass configuration..."
rg -A 10 "type OAuth2Server struct" internal/security/oauth.go
# Check for IP validation
echo "Checking IP validation..."
rg -A 5 "IsAuthenticationEnabled.*\(.*\)" internal/security/oauth.go
Length of output: 945
Script:
#!/bin/bash
# Check security settings validation and IP bypass configuration
# Check security settings validation
echo "Checking security settings validation..."
rg -A 10 "Security.*struct" internal/conf/config.go
# Check for IP bypass list configuration
echo "Checking IP bypass configuration..."
rg -B 5 -A 15 "func.*IsAuthenticationEnabled" internal/security/oauth.go
# Check for any IP whitelist/bypass validation
echo "Checking IP validation in config..."
rg "Security.*Bypass|Whitelist|AllowList" internal/conf/
Length of output: 1504
Script:
#!/bin/bash
# Check subnet bypass implementation and validation
# Check IsRequestFromAllowedSubnet implementation
echo "Checking subnet bypass implementation..."
rg -B 2 -A 15 "func.*IsRequestFromAllowedSubnet" internal/security/oauth.go
# Check subnet configuration validation
echo "Checking subnet configuration..."
rg -A 10 "type BasicAuth struct|type Security struct" internal/conf/config.go
# Check for subnet validation in settings
echo "Checking subnet validation..."
rg "AllowedSubnets|subnet" internal/conf/validate.go
Length of output: 2193
internal/httpcontroller/routes.go (4)
161-171: LGTM! Security context is properly initialized.
The security context is correctly set up with authentication state and Cloudflare access information.
69-74: LGTM! Settings routes are properly protected.
All settings routes are correctly marked with Authorized: true, ensuring they require authentication.
#!/bin/bash
# Verify no sensitive routes are left unprotected
rg -A 5 'TemplateName: "settings.*Authorized: false' --type go59-61: Verify OAuth2 routes initialization.
The code initializes OAuth2 routes through initAuthRoutes(). Let's verify its implementation to ensure all necessary OAuth2 endpoints are properly configured.
✅ Verification successful
Let me gather more information about the OAuth2 route initialization.
OAuth2 routes initialization is properly implemented
The implementation includes all necessary OAuth2 endpoints:
- Standard OAuth2 endpoints:
/oauth2/authorize,/oauth2/token,/callback - Social auth routes:
/auth/:provider,/auth/:provider/callback - Basic auth routes:
/login,/logout
The OAuth2Server struct is also properly configured with required components including settings, auth codes, access tokens, and provider configs for GitHub and Google.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for OAuth2 route initialization
rg -A 10 'func.*initAuthRoutes' --type go
# Look for standard OAuth2 endpoints
rg -A 5 '/oauth2|/auth|/callback' --type go
Length of output: 4909
Script:
#!/bin/bash
# Search for initAuthRoutes implementation
rg -A 15 'func \(s \*Server\) initAuthRoutes\(\)' --type go
# Look for OAuth2Server implementation
ast-grep --pattern 'type OAuth2Server struct {
$$$
}'
Length of output: 2051
118-119: Verify input validation in settings handlers.
Ensure that SaveSettings and GetAudioDevices handlers implement proper input validation before processing the requests.
✅ Verification successful
Let me gather more information about the input validation, particularly focusing on the form parameters handling in the SaveSettings handler.
Let me check the implementation of updateSettingsFromForm function as it seems to be the main validation point.
Let me check one more aspect - the type validation and conversion logic in this settings update process.
Based on the gathered information, I can now provide the final response:
Input validation is properly implemented in both handlers
The handlers implement appropriate validation:
GetAudioDevices: No input validation needed as it's a GET endpoint without parametersSaveSettings:- Validates settings initialization
- Implements proper form parameter parsing with error handling
- Performs type validation and safe conversion for all fields (string, bool, int)
- Returns appropriate HTTP status codes (400 for bad requests, 500 for internal errors)
- Includes special handling for audio equalizer settings
- Validates numeric values with proper error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for input validation in handlers
rg -A 10 'func.*(SaveSettings|GetAudioDevices)' --type go
Length of output: 2569
Script:
#!/bin/bash
# Search for the complete SaveSettings handler implementation
rg -A 30 'func \(h \*Handlers\) SaveSettings' --type go
Length of output: 2797
Script:
#!/bin/bash
# Search for updateSettingsFromForm implementation
rg -A 40 'func updateSettingsFromForm' --type go
Length of output: 3354
Script:
#!/bin/bash
# Search for the rest of the updateStructFromForm implementation and any type conversion functions
rg -A 50 'if field.Kind\(\) == reflect.Struct' --type go
Length of output: 3920
internal/conf/config.yaml (3)
87-87: LGTM: Better default value.
The change from a placeholder value ("00000") to an empty string is a better practice for optional configuration fields.
137-144: 🛠️ Refactor suggestion
Add validation for authentication timeouts.
The expiration times for authorization codes and access tokens need bounds for security:
authcodeexp: Should be short (5-15 minutes)accesstokenexp: Should be limited (1-24 hours)
clientid: "" # client id
- clientsecret: "" # if left empty, will be autogenerated
+ clientsecret: "" # Auto-generated if empty. WARNING: Store securely and rotate periodically
redirecturi: "" # redirect uri prefix
- authcodeexp: 10m # authorization code expiration
- accesstokenexp: 1h # access token expiration
+ authcodeexp: 10m # Authorization code expiration (min: 5m, max: 15m)
+ accesstokenexp: 1h # Access token expiration (min: 1h, max: 24h)Likely invalid or redundant comment.
126-129: Validate host configuration for TLS settings.
When autotls or redirecttohttps is enabled, the host field becomes critical. Consider adding validation to ensure:
- Host is not empty when TLS features are enabled
- Host format is valid (domain or IP:port)
internal/conf/defaults.go (3)
173-180: Token expiration configuration looks good.
The auth code (10m) and access token (1h) expiration times are reasonable defaults that balance security and usability.
Note: Previous review comments about basic auth security concerns are still applicable.
181-187: 🛠️ Refactor suggestion
Ensure proper construction of OAuth2 redirect URIs.
The default redirect URI ("/settings") is relative, but OAuth2 providers require absolute URLs. The application needs to combine this with the security.host value.
Let's check if there's proper URL construction logic:
#!/bin/bash
# Search for redirect URI handling
rg -A 5 'redirecturi.*settings'Consider updating the default to include a placeholder:
-viper.SetDefault("security.google.redirecturi", "/settings")
+viper.SetDefault("security.google.redirecturi", "{host}/settings")188-193: 🛠️ Refactor suggestion
Maintain consistency with redirect URI handling across providers.
The GitHub OAuth2 configuration mirrors the Google setup, which is good for consistency. However, it shares the same relative redirect URI consideration.
Apply the same redirect URI update here:
-viper.SetDefault("security.github.redirecturi", "/settings")
+viper.SetDefault("security.github.redirecturi", "{host}/settings")Let's verify the OAuth2 provider implementations are consistent:
#!/bin/bash
# Compare OAuth2 provider implementations
rg -A 10 'func.*InitializeOAuth.*Provider'internal/security/oauth.go (1)
185-197: Verify subnet bypass security implications
The subnet bypass logic could potentially be exploited if not properly configured.
Let's check for potential security implications:
✅ Verification successful
Let me gather more context about the security validation and configuration loading to better understand the security implications.
Let me try a different approach to understand the security validation and configuration loading.
Let me check one final aspect about the subnet validation implementation.
Subnet bypass implementation is secure with proper safeguards
The subnet bypass implementation includes necessary security controls:
- Explicit enable/disable flag check
- Proper IP address validation
- CIDR subnet validation
- Support for multiple subnets with comma separation
- Comprehensive logging for security events
- Configuration validation at startup
The bypass only disables authentication for specific internal networks, which is a common and valid security pattern for trusted networks. The implementation follows security best practices with input validation and proper error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for subnet bypass usage and configuration
rg -A 5 "AllowSubnetBypass|IsRequestFromAllowedSubnet" --type go
Length of output: 3021
Script:
#!/bin/bash
# Check security validation and configuration loading
rg -A 10 "func.*ValidateSecurity|func.*LoadConfig" --type go
# Check for any security-related tests
ast-grep --pattern 'func Test$_($_, $_) {
$$$
AllowSubnetBypass
$$$
}'
Length of output: 141
Script:
#!/bin/bash
# Check for security validation in conf package
rg -A 10 "ValidateSecurity" internal/conf/
# Look for configuration loading and validation
rg -A 10 "LoadConfig" internal/conf/
# Check how the subnet bypass is used in the security flow
ast-grep --pattern 'if $_ && s.IsRequestFromAllowedSubnet($_) {
$$$
}'
Length of output: 331
Script:
#!/bin/bash
# Check the complete implementation of IsRequestFromAllowedSubnet
ast-grep --pattern 'func (s *OAuth2Server) IsRequestFromAllowedSubnet($_) bool {
$$$
}'
# Check for any subnet validation in tests
rg -B 2 -A 5 "subnet.*validation|validate.*subnet" --type go
Length of output: 2275
internal/conf/config.go (2)
284-285: LGTM!
The Security field is properly integrated into the Settings struct.
220-227:
Add validation for RedirectURI to prevent open redirect vulnerabilities.
The RedirectURI field should be validated to ensure it points to a trusted domain and follows proper URL format. This helps prevent potential security issues like open redirect vulnerabilities.
internal/httpcontroller/auth_routes.go (1)
14-28: Routes Initialization is Clear and Organized
The authentication routes are neatly set up, distinguishing between OAuth2, social authentication, and basic authentication. This enhances readability and maintainability.
| @@ -9,14 +11,14 @@ import ( | |||
|
|
|||
| // configureMiddleware sets up middleware for the server. | |||
| func (s *Server) configureMiddleware() { | |||
| s.Echo.Use(s.AuthMiddleware) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adjusting middleware order for better security.
The AuthMiddleware should be positioned after the Recover() middleware to ensure proper error handling of authentication panics.
- s.Echo.Use(s.AuthMiddleware)
s.Echo.Use(middleware.Recover())
+ s.Echo.Use(s.AuthMiddleware)
s.Echo.Use(middleware.GzipWithConfig(middleware.GzipConfig{Committable suggestion skipped: line range outside the PR's diff.
| redirectPath := url.QueryEscape(c.Request().URL.Path) | ||
| // Validate redirect path against whitelist | ||
| if !isValidRedirect(redirectPath) { | ||
| redirectPath = "/" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure comprehensive redirect validation.
The redirect validation is a critical security control. Consider these additional checks:
- Absolute URL detection to prevent open redirects
- Path traversal prevention
- Length limits
- if !isValidRedirect(redirectPath) {
- redirectPath = "/"
- }
+ // Validate and sanitize redirect path
+ if !isValidRedirect(redirectPath) || len(redirectPath) > 1000 ||
+ strings.Contains(redirectPath, "://") ||
+ strings.Contains(redirectPath, "..") {
+ redirectPath = "/"
+ }📝 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.
| redirectPath := url.QueryEscape(c.Request().URL.Path) | |
| // Validate redirect path against whitelist | |
| if !isValidRedirect(redirectPath) { | |
| redirectPath = "/" | |
| } | |
| redirectPath := url.QueryEscape(c.Request().URL.Path) | |
| // Validate redirect path against whitelist | |
| // Validate and sanitize redirect path | |
| if !isValidRedirect(redirectPath) || len(redirectPath) > 1000 || | |
| strings.Contains(redirectPath, "://") || | |
| strings.Contains(redirectPath, "..") { | |
| redirectPath = "/" | |
| } |
| ### Basic Password Authentication | ||
|
|
||
| Basic password authentication follows the OAuth2.0 specification. It uses merely a password to authenticate a user. If no client id or secret is provided, they will be created automatically. | ||
|
|
||
| ```yaml | ||
| security: | ||
| host: "https://your.domain.com" | ||
| basicauth: | ||
| enabled: true | ||
| password: "your-password" | ||
| redirecturi: "https://your.domain.com" | ||
| clientid: "your-client-id" | ||
| clientsecret: "your-client-secret" | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security guidelines for basic authentication.
The basic authentication section should include:
- Password complexity requirements
- Storage location of auto-generated credentials
- HTTPS requirement for OAuth2 security
Add this content before the YAML example:
+> ⚠️ **Security Requirements**:
+> - Use strong passwords (min. 12 characters, mix of letters, numbers, symbols)
+> - Always use HTTPS in production (OAuth2 requires secure transport)
+> - Auto-generated credentials are stored in `auth.db`. Backup this file securely
+📝 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.
| ### Basic Password Authentication | |
| Basic password authentication follows the OAuth2.0 specification. It uses merely a password to authenticate a user. If no client id or secret is provided, they will be created automatically. | |
| ```yaml | |
| security: | |
| host: "https://your.domain.com" | |
| basicauth: | |
| enabled: true | |
| password: "your-password" | |
| redirecturi: "https://your.domain.com" | |
| clientid: "your-client-id" | |
| clientsecret: "your-client-secret" | |
| ``` | |
| ### Basic Password Authentication | |
| Basic password authentication follows the OAuth2.0 specification. It uses merely a password to authenticate a user. If no client id or secret is provided, they will be created automatically. | |
| > ⚠️ **Security Requirements**: | |
| > - Use strong passwords (min. 12 characters, mix of letters, numbers, symbols) | |
| > - Always use HTTPS in production (OAuth2 requires secure transport) | |
| > - Auto-generated credentials are stored in `auth.db`. Backup this file securely | |
| ```yaml | |
| security: | |
| host: "https://your.domain.com" | |
| basicauth: | |
| enabled: true | |
| password: "your-password" | |
| redirecturi: "https://your.domain.com" | |
| clientid: "your-client-id" | |
| clientsecret: "your-client-secret" | |
| ``` |
| ## Authentication Recovery | ||
|
|
||
| If you end up locking yourself out, authentication can be turned off with the following command: | ||
|
|
||
| ```bash | ||
| # For host system installations | ||
| ./reset_auth.sh [path/to/config.yaml] | ||
|
|
||
| # For Docker deployments | ||
| docker exec $(docker ps | grep birdnet-go | awk '{print $1}') reset_auth.sh | ||
|
|
||
| # For a devcontainer | ||
| docker exec $(docker ps | grep birdnet-go | awk '{print $1}') ./reset_auth.sh | ||
| ``` | ||
|
|
||
| The script automatically creates a timestamped backup of your current configuration before disabling the authentication. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add post-recovery security steps.
The recovery section should include:
- Security implications of resetting authentication
- Steps to restore security after recovery
- Backup file handling instructions
Add this content after line 100:
+
+> ⚠️ **Important Security Notes**:
+> 1. The authentication reset temporarily disables all security measures
+> 2. After regaining access:
+> - Immediately configure new authentication settings
+> - Verify the configuration
+> - Delete or securely store the backup file
+> 3. Backup files are stored in the same directory with timestamp suffix📝 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.
| ## Authentication Recovery | |
| If you end up locking yourself out, authentication can be turned off with the following command: | |
| ```bash | |
| # For host system installations | |
| ./reset_auth.sh [path/to/config.yaml] | |
| # For Docker deployments | |
| docker exec $(docker ps | grep birdnet-go | awk '{print $1}') reset_auth.sh | |
| # For a devcontainer | |
| docker exec $(docker ps | grep birdnet-go | awk '{print $1}') ./reset_auth.sh | |
| ``` | |
| The script automatically creates a timestamped backup of your current configuration before disabling the authentication. | |
| ## Authentication Recovery | |
| If you end up locking yourself out, authentication can be turned off with the following command: | |
| ```bash | |
| # For host system installations | |
| ./reset_auth.sh [path/to/config.yaml] | |
| # For Docker deployments | |
| docker exec $(docker ps | grep birdnet-go | awk '{print $1}') reset_auth.sh | |
| # For a devcontainer | |
| docker exec $(docker ps | grep birdnet-go | awk '{print $1}') ./reset_auth.sh | |
| ``` | |
| The script automatically creates a timestamped backup of your current configuration before disabling the authentication. | |
| > ⚠️ **Important Security Notes**: | |
| > 1. The authentication reset temporarily disables all security measures | |
| > 2. After regaining access: | |
| > - Immediately configure new authentication settings | |
| > - Verify the configuration | |
| > - Delete or securely store the backup file | |
| > 3. Backup files are stored in the same directory with timestamp suffix |
| ### Social Authentication | ||
|
|
||
| BirdNET-Go supports OAuth authentication through Google and GitHub identity providers. To implement either provider, you'll need to generate the corresponding client ID and secret, then configure them through the Security settings or in the configuration file. Remember to set the Redirect URI parameter in your Google or GitHub developer console to match the value configured in `redirecturi`. The `userid` is a list of accepted authenticated user emails. | ||
|
|
||
| ```yaml | ||
| security: | ||
| googleauth: | ||
| enabled: true | ||
| clientid: "your-google-client-id" | ||
| clientsecret: "your-google-client-secret" | ||
| userid: "allowed@gmail.com,another@gmail.com" | ||
| redirecturi: "https://your.domain.com/auth/google/callback" | ||
| ``` | ||
|
|
||
| Similarly, GitHub authentication can be enabled: | ||
|
|
||
| ```yaml | ||
| security: | ||
| githubauth: | ||
| enabled: true | ||
| clientid: "your-github-client-id" | ||
| clientsecret: "your-github-client-secret" | ||
| userid: "user@example.com" | ||
| redirecturi: "https://your.domain.com/auth/github/callback" | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance social authentication security guidance.
Add security considerations for OAuth implementation:
- Callback URL validation
- User access management
- Token handling
Add this content before the Google configuration example:
+> 🔒 **Security Best Practices**:
+> - Verify callback URLs match exactly to prevent OAuth hijacking
+> - Regularly audit the user access list
+> - Never commit OAuth credentials to version control
+> - Store client secrets securely and rotate them periodically
+📝 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.
| ### Social Authentication | |
| BirdNET-Go supports OAuth authentication through Google and GitHub identity providers. To implement either provider, you'll need to generate the corresponding client ID and secret, then configure them through the Security settings or in the configuration file. Remember to set the Redirect URI parameter in your Google or GitHub developer console to match the value configured in `redirecturi`. The `userid` is a list of accepted authenticated user emails. | |
| ```yaml | |
| security: | |
| googleauth: | |
| enabled: true | |
| clientid: "your-google-client-id" | |
| clientsecret: "your-google-client-secret" | |
| userid: "allowed@gmail.com,another@gmail.com" | |
| redirecturi: "https://your.domain.com/auth/google/callback" | |
| ``` | |
| Similarly, GitHub authentication can be enabled: | |
| ```yaml | |
| security: | |
| githubauth: | |
| enabled: true | |
| clientid: "your-github-client-id" | |
| clientsecret: "your-github-client-secret" | |
| userid: "user@example.com" | |
| redirecturi: "https://your.domain.com/auth/github/callback" | |
| ``` | |
| ### Social Authentication | |
| BirdNET-Go supports OAuth authentication through Google and GitHub identity providers. To implement either provider, you'll need to generate the corresponding client ID and secret, then configure them through the Security settings or in the configuration file. Remember to set the Redirect URI parameter in your Google or GitHub developer console to match the value configured in `redirecturi`. The `userid` is a list of accepted authenticated user emails. | |
| > 🔒 **Security Best Practices**: | |
| > - Verify callback URLs match exactly to prevent OAuth hijacking | |
| > - Regularly audit the user access list | |
| > - Never commit OAuth credentials to version control | |
| > - Store client secrets securely and rotate them periodically | |
| ```yaml | |
| security: | |
| googleauth: | |
| enabled: true | |
| clientid: "your-google-client-id" | |
| clientsecret: "your-google-client-secret" | |
| userid: "allowed@gmail.com,another@gmail.com" | |
| redirecturi: "https://your.domain.com/auth/google/callback" | |
| ``` | |
| Similarly, GitHub authentication can be enabled: | |
| ```yaml | |
| security: | |
| githubauth: | |
| enabled: true | |
| clientid: "your-github-client-id" | |
| clientsecret: "your-github-client-secret" | |
| userid: "user@example.com" | |
| redirecturi: "https://your.domain.com/auth/github/callback" | |
| ``` |
| // InitializeGoth initializes social authentication providers. | ||
| func InitializeGoth(settings *conf.Settings) { | ||
| // Set up the session store first | ||
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | ||
|
|
||
| // Initialize Gothic providers | ||
| googleProvider := | ||
| gothGoogle.New(settings.Security.GoogleAuth.ClientID, | ||
| settings.Security.GoogleAuth.ClientSecret, | ||
| settings.Security.GoogleAuth.RedirectURI, | ||
| "https://www.googleapis.com/auth/userinfo.email", | ||
| ) | ||
| googleProvider.SetAccessType("offline") | ||
|
|
||
| goth.UseProviders( | ||
| googleProvider, | ||
| github.New(settings.Security.GithubAuth.ClientID, | ||
| settings.Security.GithubAuth.ClientSecret, | ||
| settings.Security.GithubAuth.RedirectURI, | ||
| "user:email", | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for provider initialization
The provider initialization should handle potential configuration errors, such as missing or invalid client IDs/secrets.
Add error handling:
func InitializeGoth(settings *conf.Settings) error {
if settings.Security.SessionSecret == "" {
return errors.New("session secret is required")
}
gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret))
if settings.Security.GoogleAuth.Enabled {
if settings.Security.GoogleAuth.ClientID == "" || settings.Security.GoogleAuth.ClientSecret == "" {
return errors.New("Google OAuth2 credentials are required when enabled")
}
googleProvider := gothGoogle.New(...)
googleProvider.SetAccessType("offline")
goth.UseProviders(googleProvider)
}
if settings.Security.GithubAuth.Enabled {
if settings.Security.GithubAuth.ClientID == "" || settings.Security.GithubAuth.ClientSecret == "" {
return errors.New("GitHub OAuth2 credentials are required when enabled")
}
goth.UseProviders(
github.New(...)
)
}
return nil
}Committable suggestion skipped: line range outside the PR's diff.
internal/security/oauth.go
Outdated
| } | ||
|
|
||
| clientIP := net.ParseIP(ip) | ||
| log.Printf("*** %s", clientIP) |
There was a problem hiding this comment.
Remove sensitive IP logging
Logging IP addresses might violate privacy regulations in some jurisdictions.
Remove or mask the IP address in logs:
- log.Printf("*** %s", clientIP)
+ s.Debug("Processing request from masked IP: %s", maskIP(clientIP))Committable suggestion skipped: line range outside the PR's diff.
| func (s *OAuth2Server) IsUserAuthenticated(c echo.Context) bool { | ||
| if token, err := gothic.GetFromSession("access_token", c.Request()); err == nil && | ||
| token != "" && s.ValidateAccessToken(token) { | ||
| s.Debug("User was authenticated with valid access_token") | ||
| return true | ||
| } | ||
|
|
||
| userId, _ := gothic.GetFromSession("userId", c.Request()) | ||
| if s.Settings.Security.GoogleAuth.Enabled { | ||
| if googleUser, _ := gothic.GetFromSession("google", c.Request()); isValidUserId(s.Settings.Security.GoogleAuth.UserId, userId) && googleUser != "" { | ||
| s.Debug("User was authenticated with valid Google user") | ||
| return true | ||
| } | ||
| } | ||
| if s.Settings.Security.GithubAuth.Enabled { | ||
| if githubUser, _ := gothic.GetFromSession("github", c.Request()); isValidUserId(s.Settings.Security.GithubAuth.UserId, userId) && githubUser != "" { | ||
| s.Debug("User was authenticated with valid GitHub user") | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add rate limiting middleware to protect authentication endpoints
The codebase currently lacks rate limiting protection for authentication attempts. While it uses Echo's middleware for logging, compression, and cache control, there's no implementation of rate limiting.
- Add rate limiting middleware using Echo's built-in rate limiter specifically for:
/auth/*endpoints/loginendpoints- Session validation endpoints
Example implementation:
e.Group("/auth").Use(middleware.RateLimiter(middleware.NewRateLimiterMemoryStore(20)))🔗 Analysis chain
Add rate limiting for authentication attempts
The authentication check method should be protected against brute force attempts.
Consider implementing rate limiting using a token bucket algorithm or similar mechanism to prevent abuse.
Let's check if rate limiting is implemented elsewhere:
Let me search for more specific security-related implementations:
Let me check for any middleware or rate limiting implementations in the Echo server setup:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for rate limiting implementations
rg -i "rate.*limit|throttle|bucket" --type go
Length of output: 3021
Script:
#!/bin/bash
# Search for security middleware and rate limiting
rg -A 5 "func.*middleware|limiter|security" --type go
# Search for any HTTP middleware setup
ast-grep --pattern 'e.Use($$$)'
Length of output: 9917
Script:
#!/bin/bash
# Search for Echo middleware setup and rate limiting
rg -A 10 'e\.Use|middleware\.' --type go
# Search specifically for rate limit related imports
rg -l "rate.*limit|github.com/labstack/echo/v4/middleware" --type go
Length of output: 1970
internal/security/oauth_test.go
Outdated
| gothic.SetState = func(req *http.Request) string { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset gothic.SetState after tests to prevent side effects
You are overriding the global gothic.SetState function in your tests without resetting it afterward. This could lead to unintended side effects in other tests that rely on the original gothic.SetState behavior. Use t.Cleanup to restore the original function after each test.
Apply this diff to reset gothic.SetState after the test:
func TestIsUserAuthenticatedValidAccessToken(t *testing.T) {
// Set the settings instance
conf.Setting()
+ // Save original SetState to restore later
+ originalSetState := gothic.SetState
settings := &conf.Settings{
Security: conf.Security{
SessionSecret: "test-secret",
},
}
s := NewOAuth2Server()
// Initialize gothic exactly as in production
gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret))
gothic.SetState = func(req *http.Request) string {
return ""
}
+ // Restore SetState after test
+ t.Cleanup(func() {
+ gothic.SetState = originalSetState
+ })Apply the same pattern in TestIsUserAuthenticated:
func TestIsUserAuthenticated(t *testing.T) {
// Set the settings instance
conf.Setting()
+ // Save original SetState to restore later
+ originalSetState := gothic.SetState
tests := []struct {
name string
token string
expires time.Duration
want bool
}{
// Test cases...
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
settings := &conf.Settings{
Security: conf.Security{
SessionSecret: "test-secret",
},
}
s := NewOAuth2Server()
// Initialize gothic exactly as in production
gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret))
gothic.SetState = func(req *http.Request) string {
return ""
}
+ // Restore SetState after test
+ t.Cleanup(func() {
+ gothic.SetState = originalSetState
+ })
// Test logic...Also applies to: 90-92
| ca := &CloudflareAccess{ | ||
| certs: make(map[string]string), | ||
| } | ||
|
|
||
| var wg sync.WaitGroup | ||
| numRoutines := 10 | ||
| wg.Add(numRoutines) | ||
|
|
||
| for i := 0; i < numRoutines; i++ { | ||
| go func() { | ||
| defer wg.Done() | ||
| err := ca.fetchCerts(server.URL) | ||
| if err != nil { | ||
| t.Errorf("Error fetching certs: %v", err) | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| wg.Wait() | ||
|
|
||
| if len(ca.certs) != 1 { | ||
| t.Errorf("Expected 1 certificate, got %d", len(ca.certs)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential data race due to concurrent map writes in TestConcurrentAccessToCertsMap
In TestConcurrentAccessToCertsMap, multiple goroutines invoke ca.fetchCerts(server.URL), which accesses and modifies the ca.certs map. Go maps are not safe for concurrent writes without proper synchronization, leading to potential data races or runtime panics.
To address this issue, ensure that access to ca.certs is properly synchronized. You can add a mutex to the CloudflareAccess struct and lock it during map access:
+ // In cloudflare.go, add a mutex to the struct
+ type CloudflareAccess struct {
+ certs map[string]string
+ mu sync.Mutex
+ // ... other fields
+ }
// In the fetchCerts method, wrap map writes with the mutex
func (ca *CloudflareAccess) fetchCerts(url string) error {
// ... existing code
+ ca.mu.Lock()
for _, cert := range certs.PublicCerts {
ca.certs[cert.Kid] = cert.Cert
}
+ ca.mu.Unlock()
return nil
}
// Ensure all accesses to ca.certs are protected by the mutexCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (9)
internal/httpcontroller/middleware.go (1)
70-73: Add security headers for HTMX responses.While the HTMX-specific handling is correct, consider adding security headers specifically for HTMX responses to prevent XSS and other attacks.
if c.Request().Header.Get("HX-Request") == "true" { + c.Response().Header().Set("X-Content-Type-Options", "nosniff") + c.Response().Header().Set("X-XSS-Protection", "1; mode=block") c.Response().Header().Set("HX-Redirect", "/login?redirect="+redirectPath) return c.String(http.StatusUnauthorized, "") }assets/custom.css (4)
25-27: Consider using CSS custom property for error colorThe implementation is good, but to maintain consistency across the application, consider using a CSS custom property for the error color.
+:root { + --color-error: #dc2626; +} input.invalid { - border-color: #dc2626; + border-color: var(--color-error); }
71-73: Consider accessibility in font sizingWhile the utility class serves its purpose, consider these accessibility improvements:
- Use a relative unit (em) instead of rem for better scaling
- Ensure the resulting size meets WCAG minimum text size guidelines (minimum 9px)
.text-2xs { - font-size: 0.6rem; + font-size: 0.75em; + min-font-size: 9px; }
75-94: LGTM: Well-implemented skeleton loaderThe audio player skeleton implementation is excellent, using modern CSS techniques for aspect ratio maintenance and visual feedback. Consider extracting the gradient colors into CSS custom properties for better maintainability.
:root { + --skeleton-gradient-start: rgba(128, 128, 128, 0.4); + --skeleton-gradient-end: rgba(128, 128, 128, 0.1); } .audio-player-container { - background: linear-gradient(to bottom, rgba(128, 128, 128, 0.4), rgba(128, 128, 128, 0.1)); + background: linear-gradient(to bottom, var(--skeleton-gradient-start), var(--skeleton-gradient-end)); border-radius: 0.5rem; }
111-111: Consider min/max constraints for thumbnail sizeWhile using
vwunits is good for responsiveness, consider adding minimum and maximum pixel constraints to ensure thumbnails remain usable across all viewport sizes..thumbnail-container { position: relative; display: inline-block; - max-width: 16vw; + max-width: min(16vw, 200px); + min-width: max(16vw, 100px); }internal/httpcontroller/server.go (2)
66-66: Document the purpose of nil parameter in handlers.New().The purpose of the nil parameter in the handlers.New() call is unclear. Consider adding a comment explaining its purpose or remove it if unused.
109-116: Add logging for authentication failures.Consider adding debug logging when authentication fails to help with troubleshooting. This is particularly important in a multi-provider setup.
func (s *Server) IsAccessAllowed(c echo.Context) bool { // First check Cloudflare Access JWT if s.CloudflareAccess.IsEnabled(c) { return true } - return s.OAuth2Server.IsUserAuthenticated(c) + authenticated := s.OAuth2Server.IsUserAuthenticated(c) + if !authenticated { + s.Logger.Debug("web", "Authentication failed for IP: %s", s.RealIP(c)) + } + return authenticated }internal/security/oauth.go (2)
217-222: Log invalid subnet configurations during parsingWhen parsing the allowed subnets in the
IsRequestFromAllowedSubnetmethod, if an invalid CIDR notation is encountered, the error is silently ignored. Logging these errors can help diagnose misconfigurations in the allowed subnet list.Apply this diff to add error logging:
for _, subnet := range subnets { _, ipNet, err := net.ParseCIDR(strings.TrimSpace(subnet)) - if err == nil && ipNet.Contains(clientIP) { + if err != nil { + s.Debug("Invalid CIDR notation in allowed subnets: %s", subnet) + continue + } + if ipNet.Contains(clientIP) { s.Debug("Access allowed for IP %s", clientIP) return true } }
228-255: Handle context cancellation in the cleanup goroutineThe
StartAuthCleanupmethod starts a goroutine that runs indefinitely, cleaning up expired tokens at regular intervals. To allow graceful shutdown of the application and prevent potential goroutine leaks, consider incorporating context cancellation to stop the goroutine when the server is shutting down.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
assets/custom.css(4 hunks)internal/conf/config.go(5 hunks)internal/conf/defaults.go(2 hunks)internal/conf/validate.go(3 hunks)internal/httpcontroller/auth_routes.go(1 hunks)internal/httpcontroller/middleware.go(3 hunks)internal/httpcontroller/server.go(7 hunks)internal/security/cloudflare.go(1 hunks)internal/security/cloudflare_test.go(1 hunks)internal/security/oauth.go(1 hunks)internal/security/oauth_test.go(1 hunks)reset_auth.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/conf/validate.go
- internal/security/cloudflare_test.go
- reset_auth.sh
🔇 Additional comments (12)
assets/custom.css (2)
21-21: LGTM: Improved mobile responsiveness
The addition of margin: auto enhances the centering of confidence balls on mobile screens, aligning with the PR's responsive design objectives.
55-69: LGTM: Well-structured responsive design
The progressive disclosure pattern for table data is well-implemented, with clear breakpoints that ensure optimal data presentation across different device sizes.
internal/httpcontroller/server.go (2)
28-29: LGTM: Well-structured authentication provider integration.
The addition of OAuth2Server and CloudflareAccess fields to the Server struct provides a clean separation of concerns for different authentication methods.
Line range hint 79-88: LGTM: Improved TLS configuration with host validation.
The AutoTLS configuration now properly validates hosts using a whitelist, which is a security best practice for preventing unauthorized certificate issuance.
internal/conf/defaults.go (2)
163-172: LGTM! Security defaults follow secure-by-default principle.
The security configuration defaults are appropriately restrictive with security features disabled by default, requiring explicit opt-in. The empty default values for sensitive fields like host, subnet, teamdomain, and audience ensure these must be explicitly configured when enabling the respective features.
Note: As mentioned in past reviews, while subnet validation exists, please ensure Cloudflare bypass validation is implemented as suggested.
173-180: Token expiration configuration looks good.
The token expiration defaults are well-configured:
- 10-minute auth code expiration provides sufficient time for authentication flow
- 1-hour access token expiration balances security with user experience
Note: As mentioned in past reviews, please address the hardcoded client ID and empty password concerns.
internal/conf/config.go (6)
209-218: Separate OAuth2 configuration from basic authentication.
The BasicAuth struct mixes password authentication with OAuth2-specific settings, which violates the Single Responsibility Principle.
220-227: Follow Go naming conventions and add field validation.
The struct needs improvements:
- Rename
UserIdtoUserIDto follow Go naming conventions - Add validation tags to ensure required fields are set when the provider is enabled
242-260: Add validation for critical security settings.
The Security struct should include additional validations:
- Host validation when AutoTLS is enabled
- Session secret minimum length/entropy validation
284-285: LGTM! Clean integration of security settings.
The Security configuration is properly integrated into the Settings struct.
411-415: Fix configuration key inconsistency and add error handling.
There are two issues in the client secret generation:
- The viper key names don't match:
basicauth.secretvsbasicauth.clientsecret - Missing error handling when secret generation fails
532-543: Strengthen random secret generation.
The current implementation has several security concerns:
- Returning an empty string on error is dangerous as it might be used as a valid secret
- The secret length should be defined as a constant
- Missing entropy validation
| if s.OAuth2Server.IsAuthenticationEnabled(s.RealIP(c)) { | ||
| if !s.IsAccessAllowed(c) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing rate limiting for authentication attempts.
The IP-based authentication check could benefit from rate limiting to prevent brute force attempts. Consider adding a rate limiter middleware or integrating rate limiting into the authentication flow.
Would you like me to provide an example implementation using a token bucket or sliding window algorithm?
| if s.CloudflareAccess.IsEnabled(c) { | ||
| return next(c) | ||
| } |
There was a problem hiding this comment.
Add error handling for authentication services.
The calls to IsEnabled() and IsAuthenticationEnabled() should include error handling to prevent security bypasses if the services fail.
if s.CloudflareAccess.IsEnabled(c) {
+ if err != nil {
+ // Log the error
+ return c.String(http.StatusServiceUnavailable, "Authentication service unavailable")
+ }
return next(c)
}Also applies to: 63-64
| func isProtectedRoute(path string) bool { | ||
| return strings.HasPrefix(path, "/settings/") | ||
| } |
There was a problem hiding this comment.
Strengthen protected route validation.
The current implementation could be bypassed through:
- Case sensitivity (e.g., "/SETTINGS/")
- Path normalization (e.g., "/settings/../settings/")
- URL encoding variations
func isProtectedRoute(path string) bool {
- return strings.HasPrefix(path, "/settings/")
+ // Normalize path
+ normalized := path
+ for normalized != path {
+ path = normalized
+ normalized = strings.ToLower(path)
+ normalized = strings.ReplaceAll(normalized, "//", "/")
+ normalized = strings.ReplaceAll(normalized, "/./", "/")
+ }
+ return strings.HasPrefix(normalized, "/settings/")
}Committable suggestion skipped: line range outside the PR's diff.
| SessionSecret: "test-secret", | ||
| }, | ||
| } | ||
|
|
||
| s := NewOAuth2Server() | ||
|
|
||
| // Initialize gothic exactly as in production | ||
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | ||
|
|
||
| e := echo.New() | ||
| req := httptest.NewRequest(http.MethodGet, "/", nil) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| // Store token using gothic's method | ||
| gothic.StoreInSession("access_token", tt.token, req, rec) | ||
|
|
||
| // Add cookie to request | ||
| req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) | ||
|
|
||
| // Add token to OAuth2Server's valid tokens | ||
| s.accessTokens[tt.token] = AccessToken{ | ||
| Token: tt.token, | ||
| ExpiresAt: time.Now().Add(tt.expires), | ||
| } | ||
|
|
||
| got := s.IsUserAuthenticated(c) | ||
| if got != tt.want { | ||
| t.Errorf("IsUserAuthenticated() = %v, want %v", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestOAuth2Server(t *testing.T) { | ||
| // Set the settings instance | ||
| conf.Setting() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| test func(*testing.T, *OAuth2Server) | ||
| }{ | ||
| { | ||
| name: "generate and validate auth code", | ||
| test: func(t *testing.T, s *OAuth2Server) { | ||
| // Initialize settings | ||
| s.Settings = &conf.Settings{ | ||
| Security: conf.Security{ | ||
| BasicAuth: conf.BasicAuth{ | ||
| Enabled: true, | ||
| ClientID: "test-client", | ||
| ClientSecret: "test-secret", | ||
| AuthCodeExp: 10 * time.Minute, | ||
| AccessTokenExp: 1 * time.Hour, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Generate and immediately use the auth code | ||
| code, err := s.GenerateAuthCode() | ||
| if err != nil { | ||
| t.Fatalf("Failed to generate auth code: %v", err) | ||
| } | ||
|
|
||
| token, err := s.ExchangeAuthCode(code) | ||
| if err != nil { | ||
| t.Fatalf("Failed to exchange auth code: %v", err) | ||
| } | ||
|
|
||
| if !s.ValidateAccessToken(token) { | ||
| t.Error("Token validation failed") | ||
| } | ||
| }, | ||
| }, | ||
| { | ||
| name: "subnet bypass validation", | ||
| test: func(t *testing.T, s *OAuth2Server) { | ||
| s.Settings.Security.AllowSubnetBypass = conf.AllowSubnetBypass{ | ||
| Enabled: true, | ||
| Subnet: "192.168.1.0/24", | ||
| } | ||
|
|
||
| if !s.IsRequestFromAllowedSubnet("192.168.1.100") { | ||
| t.Error("Expected IP to be allowed") | ||
| } | ||
|
|
||
| if s.IsRequestFromAllowedSubnet("10.0.0.1") { | ||
| t.Error("Expected IP to be denied") | ||
| } | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| s := NewOAuth2Server() | ||
| tt.test(t, s) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Security tests need additional test cases and session hardening
The test suite has several security gaps that need to be addressed:
- Missing test cases for expired tokens, invalid tokens, and token revocation
- No tests for session security configurations (Secure, HttpOnly, SameSite flags)
- Limited error case coverage in auth code exchange tests
- Single test case in
TestIsUserAuthenticatedonly covers the happy path
Key locations requiring attention:
internal/security/oauth_test.go: Add negative test cases toTestIsUserAuthenticatedinternal/security/oauth_test.go: Add session configuration validation inTestIsUserAuthenticatedValidAccessTokeninternal/security/oauth_test.go: Add token expiry and revocation tests toTestOAuth2Server
The implementation shows proper use of crypto/rand for token generation and mutex protection for concurrent access, but the test coverage needs improvement to verify security boundaries.
🔗 Analysis chain
Verify security-critical functionality
Let's verify the implementation of security-critical functions being tested:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for security-critical functions to ensure proper implementation
# Check token validation implementation
echo "Checking token validation..."
ast-grep --pattern 'func $_(token string) bool {
$$$
}'
# Check for proper crypto usage
echo "Checking crypto usage..."
rg -A 5 "crypto/rand|GenerateRandomBytes"
# Check for proper session handling
echo "Checking session security..."
rg -A 5 "MaxAge|Secure|HttpOnly|SameSite"
# Check for proper error handling in auth flows
echo "Checking auth error handling..."
rg "err.*return.*nil" "internal/security"
Length of output: 4476
Script:
#!/bin/bash
# Let's verify the security implementation more thoroughly
# Check token validation and authentication logic
echo "Checking token validation implementation..."
rg -A 10 "ValidateAccessToken|IsUserAuthenticated" "internal/security/"
# Check auth code generation and exchange
echo "Checking auth code implementation..."
rg -A 10 "GenerateAuthCode|ExchangeAuthCode" "internal/security/"
# Check session configuration
echo "Checking session configuration..."
rg -A 5 "sessions.NewCookieStore" "internal/security/"
# Check error handling in auth flows
echo "Checking error handling in auth flows..."
rg -A 5 "if err != nil" "internal/security/oauth.go"
Length of output: 10105
| if err := gothic.StoreInSession("userId", user.Email, c.Request(), c.Response()); err != nil { | ||
| return echo.NewHTTPError(http.StatusInternalServerError, "Failed to store user to session") | ||
| } |
There was a problem hiding this comment.
Enhance session security and user identification
Using email as userId could cause issues if the user changes their email address. Additionally, consider implementing session security best practices:
- Use a stable unique identifier instead of email
- Implement session expiration
- Rotate session tokens after login
- if err := gothic.StoreInSession("userId", user.Email, c.Request(), c.Response()); err != nil {
+ // Generate or retrieve a stable unique identifier
+ userId := generateOrRetrieveStableUserId(user)
+ if err := gothic.StoreInSession("userId", userId, c.Request(), c.Response()); err != nil {
return echo.NewHTTPError(http.StatusInternalServerError, "Failed to store user to session")
}
+ // Set session expiration
+ if err := gothic.StoreInSession("session_expires", time.Now().Add(24*time.Hour).Unix(), c.Request(), c.Response()); err != nil {
+ return echo.NewHTTPError(http.StatusInternalServerError, "Failed to set session expiration")
+ }Committable suggestion skipped: line range outside the PR's diff.
| // GitHub OAuth2 configuration | ||
| viper.SetDefault("security.github.enabled", false) | ||
| viper.SetDefault("security.github.clientid", "") | ||
| viper.SetDefault("security.github.clientsecret", "") | ||
| viper.SetDefault("security.github.redirecturi", "/settings") | ||
| viper.SetDefault("security.github.userid", "") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency with Google OAuth2 redirect URI changes.
The GitHub OAuth2 configuration mirrors the Google OAuth2 structure, which is good for consistency. However, it shares the same relative redirect URI consideration.
Apply the same absolute URL update:
-viper.SetDefault("security.github.redirecturi", "/settings")
+viper.SetDefault("security.github.redirecturi", "") // Must be absolute URL, e.g., https://example.com/settings📝 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.
| // GitHub OAuth2 configuration | |
| viper.SetDefault("security.github.enabled", false) | |
| viper.SetDefault("security.github.clientid", "") | |
| viper.SetDefault("security.github.clientsecret", "") | |
| viper.SetDefault("security.github.redirecturi", "/settings") | |
| viper.SetDefault("security.github.userid", "") | |
| // GitHub OAuth2 configuration | |
| viper.SetDefault("security.github.enabled", false) | |
| viper.SetDefault("security.github.clientid", "") | |
| viper.SetDefault("security.github.clientsecret", "") | |
| viper.SetDefault("security.github.redirecturi", "") // Must be absolute URL, e.g., https://example.com/settings | |
| viper.SetDefault("security.github.userid", "") |
| // Google OAuth2 configuration | ||
| viper.SetDefault("security.google.enabled", false) | ||
| viper.SetDefault("security.google.clientid", "") | ||
| viper.SetDefault("security.google.clientsecret", "") | ||
| viper.SetDefault("security.google.redirecturi", "/settings") | ||
| viper.SetDefault("security.google.userid", "") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using absolute URL for OAuth2 redirect URI.
While the defaults are secure with OAuth2 disabled and empty credentials, the relative redirect URI (/settings) might cause issues with some OAuth2 providers that require absolute URLs.
Consider updating the default to include a placeholder that reminds users to set an absolute URL:
-viper.SetDefault("security.google.redirecturi", "/settings")
+viper.SetDefault("security.google.redirecturi", "") // Must be absolute URL, e.g., https://example.com/settings📝 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.
| // Google OAuth2 configuration | |
| viper.SetDefault("security.google.enabled", false) | |
| viper.SetDefault("security.google.clientid", "") | |
| viper.SetDefault("security.google.clientsecret", "") | |
| viper.SetDefault("security.google.redirecturi", "/settings") | |
| viper.SetDefault("security.google.userid", "") | |
| // Google OAuth2 configuration | |
| viper.SetDefault("security.google.enabled", false) | |
| viper.SetDefault("security.google.clientid", "") | |
| viper.SetDefault("security.google.clientsecret", "") | |
| viper.SetDefault("security.google.redirecturi", "") // Must be absolute URL, e.g., https://example.com/settings | |
| viper.SetDefault("security.google.userid", "") |
| s.Debug("Invalid IP address: %s", ip) | ||
| return false |
There was a problem hiding this comment.
Avoid logging unmasked IP addresses
The Debug messages in IsRequestFromAllowedSubnet log client IP addresses. Logging raw IP addresses can have privacy implications and may violate privacy regulations in some jurisdictions. Consider masking or anonymizing IP addresses when logging.
Apply this diff to mask IP addresses in logs:
- s.Debug("Invalid IP address: %s", ip)
+ s.Debug("Invalid IP address")
...
- s.Debug("Access allowed for IP %s", clientIP)
+ s.Debug("Access allowed for client IP")
...
- s.Debug("IP %s is not in the allowed subnet", clientIP)
+ s.Debug("Client IP is not in the allowed subnet")Also applies to: 219-220, 224-225
| if !ca.debug { | ||
| if len(v) == 0 { | ||
| log.Print(format) | ||
| } else { | ||
| log.Printf(format, v...) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix debug logging condition in Debug method.
The current implementation logs messages when ca.debug is false, which is the opposite of the expected behavior. The logging should occur when ca.debug is true.
Apply this fix:
func (ca *CloudflareAccess) Debug(format string, v ...interface{}) {
- if !ca.debug {
+ if ca.debug {
if len(v) == 0 {
log.Print(format)
} else {
log.Printf(format, v...)
}
}
}📝 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.
| if !ca.debug { | |
| if len(v) == 0 { | |
| log.Print(format) | |
| } else { | |
| log.Printf(format, v...) | |
| } | |
| } | |
| } | |
| if ca.debug { | |
| if len(v) == 0 { | |
| log.Print(format) | |
| } else { | |
| log.Printf(format, v...) | |
| } | |
| } | |
| } |
| func (ca *CloudflareAccess) fetchCerts(issuer string) error { | ||
| certsURL := fmt.Sprintf("%s/cdn-cgi/access/certs", issuer) | ||
| ca.Debug("Fetching Cloudflare certs from URL: %s", certsURL) | ||
|
|
||
| resp, err := http.Get(certsURL) | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to fetch Cloudflare certs: %w", err) | ||
| } | ||
| if resp.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("failed to fetch Cloudflare certs: received status code %d", resp.StatusCode) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| var certsResponse struct { | ||
| PublicCerts []struct { | ||
| Kid string `json:"kid"` | ||
| Cert string `json:"cert"` | ||
| } `json:"public_certs"` | ||
| } | ||
| if err := json.NewDecoder(resp.Body).Decode(&certsResponse); err != nil { | ||
| return fmt.Errorf("failed to decode certs response: %w", err) | ||
| } |
There was a problem hiding this comment.
Add security improvements to certificate fetching.
The fetchCerts method lacks validation of the issuer URL and does not set a timeout for the HTTP client, which could lead to security vulnerabilities and resource exhaustion.
Apply this fix:
func (ca *CloudflareAccess) fetchCerts(issuer string) error {
+ // Validate issuer URL
+ parsedIssuer, err := url.Parse(issuer)
+ if err != nil || !parsedIssuer.IsAbs() {
+ return fmt.Errorf("invalid issuer URL")
+ }
+
+ certsURL := fmt.Sprintf("%s/cdn-cgi/access/certs", parsedIssuer.String())
ca.Debug("Fetching Cloudflare certs from URL: %s", certsURL)
- resp, err := http.Get(certsURL)
+ client := &http.Client{
+ Timeout: 30 * time.Second,
+ }
+ resp, err := client.Get(certsURL)
if err != nil {
return fmt.Errorf("failed to fetch Cloudflare certs: %w", err)
}📝 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.
| func (ca *CloudflareAccess) fetchCerts(issuer string) error { | |
| certsURL := fmt.Sprintf("%s/cdn-cgi/access/certs", issuer) | |
| ca.Debug("Fetching Cloudflare certs from URL: %s", certsURL) | |
| resp, err := http.Get(certsURL) | |
| if err != nil { | |
| return fmt.Errorf("failed to fetch Cloudflare certs: %w", err) | |
| } | |
| if resp.StatusCode != http.StatusOK { | |
| return fmt.Errorf("failed to fetch Cloudflare certs: received status code %d", resp.StatusCode) | |
| } | |
| defer resp.Body.Close() | |
| var certsResponse struct { | |
| PublicCerts []struct { | |
| Kid string `json:"kid"` | |
| Cert string `json:"cert"` | |
| } `json:"public_certs"` | |
| } | |
| if err := json.NewDecoder(resp.Body).Decode(&certsResponse); err != nil { | |
| return fmt.Errorf("failed to decode certs response: %w", err) | |
| } | |
| func (ca *CloudflareAccess) fetchCerts(issuer string) error { | |
| // Validate issuer URL | |
| parsedIssuer, err := url.Parse(issuer) | |
| if err != nil || !parsedIssuer.IsAbs() { | |
| return fmt.Errorf("invalid issuer URL") | |
| } | |
| certsURL := fmt.Sprintf("%s/cdn-cgi/access/certs", parsedIssuer.String()) | |
| ca.Debug("Fetching Cloudflare certs from URL: %s", certsURL) | |
| client := &http.Client{ | |
| Timeout: 30 * time.Second, | |
| } | |
| resp, err := client.Get(certsURL) | |
| if err != nil { | |
| return fmt.Errorf("failed to fetch Cloudflare certs: %w", err) | |
| } | |
| if resp.StatusCode != http.StatusOK { | |
| return fmt.Errorf("failed to fetch Cloudflare certs: received status code %d", resp.StatusCode) | |
| } | |
| defer resp.Body.Close() | |
| var certsResponse struct { | |
| PublicCerts []struct { | |
| Kid string `json:"kid"` | |
| Cert string `json:"cert"` | |
| } `json:"public_certs"` | |
| } | |
| if err := json.NewDecoder(resp.Body).Decode(&certsResponse); err != nil { | |
| return fmt.Errorf("failed to decode certs response: %w", err) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (10)
assets/custom.css (4)
25-27: LGTM: Good error state stylingThe invalid input styling provides clear visual feedback for authentication errors. Consider adding a transition for smoother state changes.
input.invalid { border-color: #dc2626; + transition: border-color 0.2s ease-in-out; }
55-69: Consider using CSS custom properties for breakpointsThe responsive display rules are well-structured but could be more maintainable. Consider using CSS custom properties for breakpoints and creating a more systematic approach.
+:root { + --breakpoint-mobile: 479px; + --breakpoint-tablet: 767px; +} + .hour-header, .hour-data { display: table-cell; } .hourly-count { display: table-cell; } .bi-hourly-count, .six-hourly-count { display: none; } -@media (max-width: 767px) { +@media (max-width: var(--breakpoint-tablet)) { .hour-header:not(.bi-hourly), .hour-data:not(.bi-hourly) { display: none; } .hourly-count { display: none; } .bi-hourly-count { display: table-cell; } } -@media (max-width: 479px) { +@media (max-width: var(--breakpoint-mobile)) { .hour-header:not(.six-hourly), .hour-data:not(.six-hourly) { display: none; } .bi-hourly-count { display: none; } .six-hourly-count { display: table-cell; } }
71-73: Consider using relative units for better scalingWhile the utility class is helpful, using relative units would make it more flexible across different base font sizes.
.text-2xs { - font-size: 0.6rem; + font-size: clamp(0.6rem, 0.75em, 0.7rem); }
111-111: Consider adding a minimum width constraintWhile using viewport-relative units is good for responsiveness, consider adding a minimum width to prevent thumbnails from becoming too small on narrow screens.
- max-width: 16vw; + max-width: 16vw; + min-width: 100px;internal/httpcontroller/auth_routes.go (1)
54-72: Add structured error logging for authentication failuresThe error handling could be improved by adding structured logging to help with debugging and monitoring authentication issues.
+import "log/slog" + func handleGothCallback(c echo.Context) error { request := c.Request() response := c.Response().Writer user, err := gothic.CompleteUserAuth(response, request) if err != nil { + slog.Error("Social auth completion failed", + "provider", c.Param("provider"), + "error", err, + "request_id", c.Response().Header().Get(echo.HeaderXRequestID)) return c.String(http.StatusBadRequest, "Authentication failed") }internal/httpcontroller/server.go (1)
53-54: Consider defensive initialization of security components.While the initialization is correct, consider adding error handling for the security component initialization:
-OAuth2Server: security.NewOAuth2Server(), -CloudflareAccess: security.NewCloudflareAccess(), +OAuth2Server: security.NewOAuth2Server(settings), +CloudflareAccess: security.NewCloudflareAccess(settings),This would allow proper error propagation if the security components fail to initialize correctly.
internal/security/oauth.go (3)
24-43: Add documentation for exported typesThe exported types
AuthCode,AccessToken, andOAuth2Servershould have proper documentation comments following Go conventions.Add documentation like this:
+// AuthCode represents an authorization code with expiration time type AuthCode struct { Code string ExpiresAt time.Time } +// AccessToken represents an access token with expiration time type AccessToken struct { Token string ExpiresAt time.Time } +// OAuth2Server handles OAuth2 authentication flow and token management type OAuth2Server struct { Settings *conf.Settings authCodes map[string]AuthCode accessTokens map[string]AccessToken mutex sync.RWMutex debug bool GithubConfig *oauth2.Config GoogleConfig *oauth2.Config }
93-114: Improve error handling in authentication checksThe
IsUserAuthenticatedmethod silently ignores errors fromgothic.GetFromSession. Consider logging these errors when in debug mode to help with troubleshooting.- userId, _ := gothic.GetFromSession("userId", c.Request()) + userId, err := gothic.GetFromSession("userId", c.Request()) + if err != nil { + s.Debug("Error getting userId from session: %v", err) + } if s.Settings.Security.GoogleAuth.Enabled { - if googleUser, _ := gothic.GetFromSession("google", c.Request()); + googleUser, err := gothic.GetFromSession("google", c.Request()) + if err != nil { + s.Debug("Error getting Google user from session: %v", err) + } if isValidUserId(s.Settings.Security.GoogleAuth.UserId, userId) && googleUser != "" {
228-255: Add graceful shutdown support to cleanup routineThe cleanup goroutine runs indefinitely without a way to stop it gracefully. Consider adding a stop channel for proper shutdown.
+ // Add stop channel to OAuth2Server struct + type OAuth2Server struct { + // ... existing fields ... + cleanupStop chan struct{} + } func (s *OAuth2Server) StartAuthCleanup(interval time.Duration) { + if s.cleanupStop != nil { + close(s.cleanupStop) + } + s.cleanupStop = make(chan struct{}) go func() { ticker := time.NewTicker(interval) defer ticker.Stop() - for range ticker.C { + for { + select { + case <-ticker.C: now := time.Now() s.mutex.Lock() // ... existing cleanup code ... s.mutex.Unlock() s.Debug("Token & code cleanup completed") + case <-s.cleanupStop: + return + } } }() } + // Add method to stop cleanup + func (s *OAuth2Server) StopAuthCleanup() { + if s.cleanupStop != nil { + close(s.cleanupStop) + s.cleanupStop = nil + } + }internal/conf/config.go (1)
229-238: Document security implications of bypass features.The bypass features (subnet and Cloudflare) need clear documentation about their security implications and recommended usage patterns.
Add detailed comments explaining:
- When to use these features
- Security risks of enabling them
- Recommended security controls when using them
type AllowSubnetBypass struct { - Enabled bool // true to enable subnet bypass - Subnet string // disable OAuth2 in subnet + // Enabled allows bypassing OAuth2 authentication for specific subnets. + // SECURITY WARNING: This feature should only be used in trusted environments + // where network access is strictly controlled. Enabling this feature could + // allow unauthorized access if the subnet is compromised. + Enabled bool + + // Subnet specifies the CIDR range (e.g., "10.0.0.0/8") where OAuth2 + // authentication will be bypassed. This should typically be an internal + // network range. Ensure proper network segmentation is in place. + Subnet string }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
assets/custom.css(4 hunks)internal/conf/config.go(5 hunks)internal/conf/defaults.go(2 hunks)internal/conf/validate.go(3 hunks)internal/httpcontroller/auth_routes.go(1 hunks)internal/httpcontroller/middleware.go(3 hunks)internal/httpcontroller/server.go(7 hunks)internal/security/cloudflare.go(1 hunks)internal/security/cloudflare_test.go(1 hunks)internal/security/oauth.go(1 hunks)internal/security/oauth_test.go(1 hunks)reset_auth.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/httpcontroller/middleware.go
- internal/security/cloudflare.go
- internal/security/cloudflare_test.go
- reset_auth.sh
🔇 Additional comments (11)
assets/custom.css (2)
21-21: LGTM: Improved mobile responsiveness
The addition of margin: auto enhances the centering of confidence indicators on mobile devices.
75-94: LGTM: Well-implemented loading state
The audio player skeleton implementation is excellent, using modern CSS features and maintaining proper aspect ratio. The gradient background provides a subtle loading indication.
internal/httpcontroller/server.go (3)
7-8: LGTM: New imports and struct fields.
The additions properly support the OAuth2 and Cloudflare Access integration mentioned in the PR objectives.
Also applies to: 28-29
105-116: LGTM: Authentication methods implementation.
The authentication methods are well-structured with proper precedence:
isAuthenticationEnabledcorrectly delegates to the OAuth2 serverIsAccessAllowedproperly checks Cloudflare Access before falling back to OAuth2
Line range hint 79-88: Verify AutoTLS host configuration.
The AutoTLS host whitelist is critical for security. Please ensure that Security.Host is properly configured in your settings to prevent unauthorized certificate issuance.
✅ Verification successful
Let me generate one more verification script to check the host validation logic:
Let me try a different approach to find the host validation:
Based on the gathered information, I can now provide the final response:
AutoTLS host configuration is properly implemented with security controls
The Security.Host setting is well-integrated with appropriate validations:
- Host validation is performed via
formatAndValidateHostbefore applying settings - Host is used consistently for both AutoTLS and OAuth2 redirect URL validation
- The UI provides clear guidance that the host setting is required for AutoTLS
- AutoTLS is properly configured with host whitelist policy to prevent unauthorized certificate issuance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Security.Host configuration
# Look for Security.Host configuration and usage
rg -A 2 "Security.Host"
# Look for AutoTLS related configuration
rg -A 2 "AutoTLS"
Length of output: 4039
Script:
#!/bin/bash
# Check the host validation implementation
ast-grep --pattern 'func formatAndValidateHost'
Length of output: 49
Script:
#!/bin/bash
# Search for host validation using ripgrep
rg -A 10 "formatAndValidateHost"
Length of output: 1638
internal/conf/defaults.go (5)
167-171: LGTM! Secure defaults for bypass authentication.
The empty defaults for bypass settings ensure explicit configuration is required.
181-187: LGTM! Secure defaults for Google OAuth2.
The empty defaults for credentials ensure explicit configuration is required.
188-193: LGTM! Secure defaults for GitHub OAuth2.
The empty defaults for credentials ensure explicit configuration is required.
178-179: 🛠️ Refactor suggestion
Add validation for token expiration durations.
The default durations look reasonable (10m for auth code, 1h for access token), but should be validated to ensure they're:
- Parseable time durations
- Auth code expiration is shorter than access token expiration
- Neither duration is too long (security risk) or too short (usability issue)
#!/bin/bash
# Check if validation exists for token expiration
rg "security\.basic\.(authcodeexp|accesstokenexp)" "internal/conf/validate.go"Consider adding this validation:
func validateTokenExpirations() error {
authCodeExp, err := time.ParseDuration(viper.GetString("security.basic.authcodeexp"))
if err != nil {
return fmt.Errorf("invalid auth code expiration: %v", err)
}
accessTokenExp, err := time.ParseDuration(viper.GetString("security.basic.accesstokenexp"))
if err != nil {
return fmt.Errorf("invalid access token expiration: %v", err)
}
if authCodeExp >= accessTokenExp {
return fmt.Errorf("auth code expiration must be shorter than access token expiration")
}
if authCodeExp > 30*time.Minute {
return fmt.Errorf("auth code expiration too long (max 30m)")
}
if accessTokenExp > 24*time.Hour {
return fmt.Errorf("access token expiration too long (max 24h)")
}
return nil
}164-166: Add validation for auto TLS configuration.
When security.autotls is enabled, the security.host should be required as it's needed for obtaining SSL certificates.
Consider adding this validation to ensure proper TLS setup:
if viper.GetBool("security.autotls") && viper.GetString("security.host") == "" {
return fmt.Errorf("host must be set when auto TLS is enabled")
}internal/conf/config.go (1)
284-285: LGTM!
The Security field is correctly integrated into the Settings struct.
| func TestOAuth2Server(t *testing.T) { | ||
| // Set the settings instance | ||
| conf.Setting() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| test func(*testing.T, *OAuth2Server) | ||
| }{ | ||
| { | ||
| name: "generate and validate auth code", | ||
| test: func(t *testing.T, s *OAuth2Server) { | ||
| // Initialize settings | ||
| s.Settings = &conf.Settings{ | ||
| Security: conf.Security{ | ||
| BasicAuth: conf.BasicAuth{ | ||
| Enabled: true, | ||
| ClientID: "test-client", | ||
| ClientSecret: "test-secret", | ||
| AuthCodeExp: 10 * time.Minute, | ||
| AccessTokenExp: 1 * time.Hour, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Generate and immediately use the auth code | ||
| code, err := s.GenerateAuthCode() | ||
| if err != nil { | ||
| t.Fatalf("Failed to generate auth code: %v", err) | ||
| } | ||
|
|
||
| token, err := s.ExchangeAuthCode(code) | ||
| if err != nil { | ||
| t.Fatalf("Failed to exchange auth code: %v", err) | ||
| } | ||
|
|
||
| if !s.ValidateAccessToken(token) { | ||
| t.Error("Token validation failed") | ||
| } | ||
| }, | ||
| }, | ||
| { | ||
| name: "subnet bypass validation", | ||
| test: func(t *testing.T, s *OAuth2Server) { | ||
| s.Settings.Security.AllowSubnetBypass = conf.AllowSubnetBypass{ | ||
| Enabled: true, | ||
| Subnet: "192.168.1.0/24", | ||
| } | ||
|
|
||
| if !s.IsRequestFromAllowedSubnet("192.168.1.100") { | ||
| t.Error("Expected IP to be allowed") | ||
| } | ||
|
|
||
| if s.IsRequestFromAllowedSubnet("10.0.0.1") { | ||
| t.Error("Expected IP to be denied") | ||
| } | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| s := NewOAuth2Server() | ||
| tt.test(t, s) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for token expiration time
The test verifies token validation but doesn't check if the expiration time is set correctly. This is critical for OAuth2 security.
Add expiration time validation:
token, err := s.ExchangeAuthCode(code)
if err != nil {
t.Fatalf("Failed to exchange auth code: %v", err)
}
+// Verify token expiration time
+if accessToken, exists := s.accessTokens[token]; !exists {
+ t.Error("Token not found in access tokens map")
+} else {
+ expectedExp := time.Now().Add(s.Settings.Security.BasicAuth.AccessTokenExp)
+ if accessToken.ExpiresAt.Sub(expectedExp) > time.Second {
+ t.Errorf("Token expiration time mismatch: got %v, want %v",
+ accessToken.ExpiresAt, expectedExp)
+ }
+}
if !s.ValidateAccessToken(token) {Committable suggestion skipped: line range outside the PR's diff.
| // TestIsUserAuthenticatedValidAccessToken tests the IsUserAuthenticated function with a valid access token | ||
| func TestIsUserAuthenticatedValidAccessToken(t *testing.T) { | ||
| // Set the settings instance | ||
| conf.Setting() | ||
|
|
||
| settings := &conf.Settings{ | ||
| Security: conf.Security{ | ||
| SessionSecret: "test-secret", | ||
| }, | ||
| } | ||
|
|
||
| s := NewOAuth2Server() | ||
|
|
||
| // Initialize gothic exactly as in production | ||
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | ||
|
|
||
| e := echo.New() | ||
| req := httptest.NewRequest(http.MethodGet, "/", nil) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| // Store token using gothic's method | ||
| gothic.StoreInSession("access_token", "valid_token", req, rec) | ||
|
|
||
| // Add cookie to request | ||
| req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) | ||
|
|
||
| // Add token to OAuth2Server's valid tokens | ||
| s.accessTokens["valid_token"] = AccessToken{ | ||
| Token: "valid_token", | ||
| ExpiresAt: time.Now().Add(time.Hour), | ||
| } | ||
|
|
||
| isAuthenticated := s.IsUserAuthenticated(c) | ||
|
|
||
| if !isAuthenticated { | ||
| t.Errorf("Expected IsUserAuthenticated to return true, got false") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add session security configuration validation
The test should verify that the session cookie has proper security settings (Secure, HttpOnly, SameSite flags) as this is critical for OAuth2 security.
Add session configuration validation:
gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret))
+
+// Verify session security configuration
+store := gothic.Store.(*sessions.CookieStore)
+opts := store.Options
+if !opts.Secure || !opts.HttpOnly || opts.SameSite != http.SameSiteLaxMode {
+ t.Error("Session cookie missing security settings")
+}📝 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.
| // TestIsUserAuthenticatedValidAccessToken tests the IsUserAuthenticated function with a valid access token | |
| func TestIsUserAuthenticatedValidAccessToken(t *testing.T) { | |
| // Set the settings instance | |
| conf.Setting() | |
| settings := &conf.Settings{ | |
| Security: conf.Security{ | |
| SessionSecret: "test-secret", | |
| }, | |
| } | |
| s := NewOAuth2Server() | |
| // Initialize gothic exactly as in production | |
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | |
| e := echo.New() | |
| req := httptest.NewRequest(http.MethodGet, "/", nil) | |
| rec := httptest.NewRecorder() | |
| c := e.NewContext(req, rec) | |
| // Store token using gothic's method | |
| gothic.StoreInSession("access_token", "valid_token", req, rec) | |
| // Add cookie to request | |
| req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) | |
| // Add token to OAuth2Server's valid tokens | |
| s.accessTokens["valid_token"] = AccessToken{ | |
| Token: "valid_token", | |
| ExpiresAt: time.Now().Add(time.Hour), | |
| } | |
| isAuthenticated := s.IsUserAuthenticated(c) | |
| if !isAuthenticated { | |
| t.Errorf("Expected IsUserAuthenticated to return true, got false") | |
| } | |
| } | |
| // TestIsUserAuthenticatedValidAccessToken tests the IsUserAuthenticated function with a valid access token | |
| func TestIsUserAuthenticatedValidAccessToken(t *testing.T) { | |
| // Set the settings instance | |
| conf.Setting() | |
| settings := &conf.Settings{ | |
| Security: conf.Security{ | |
| SessionSecret: "test-secret", | |
| }, | |
| } | |
| s := NewOAuth2Server() | |
| // Initialize gothic exactly as in production | |
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | |
| // Verify session security configuration | |
| store := gothic.Store.(*sessions.CookieStore) | |
| opts := store.Options | |
| if !opts.Secure || !opts.HttpOnly || opts.SameSite != http.SameSiteLaxMode { | |
| t.Error("Session cookie missing security settings") | |
| } | |
| e := echo.New() | |
| req := httptest.NewRequest(http.MethodGet, "/", nil) | |
| rec := httptest.NewRecorder() | |
| c := e.NewContext(req, rec) | |
| // Store token using gothic's method | |
| gothic.StoreInSession("access_token", "valid_token", req, rec) | |
| // Add cookie to request | |
| req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) | |
| // Add token to OAuth2Server's valid tokens | |
| s.accessTokens["valid_token"] = AccessToken{ | |
| Token: "valid_token", | |
| ExpiresAt: time.Now().Add(time.Hour), | |
| } | |
| isAuthenticated := s.IsUserAuthenticated(c) | |
| if !isAuthenticated { | |
| t.Errorf("Expected IsUserAuthenticated to return true, got false") | |
| } | |
| } |
| // TestIsUserAuthenticatedInvalidAccessToken tests the IsUserAuthenticated function with an invalid access token | ||
| func TestIsUserAuthenticated(t *testing.T) { | ||
| // Set the settings instance | ||
| conf.Setting() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| token string | ||
| expires time.Duration | ||
| want bool | ||
| }{ | ||
| { | ||
| name: "valid token", | ||
| token: "valid_token", | ||
| expires: time.Hour, | ||
| want: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| settings := &conf.Settings{ | ||
| Security: conf.Security{ | ||
| SessionSecret: "test-secret", | ||
| }, | ||
| } | ||
|
|
||
| s := NewOAuth2Server() | ||
|
|
||
| // Initialize gothic exactly as in production | ||
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | ||
|
|
||
| e := echo.New() | ||
| req := httptest.NewRequest(http.MethodGet, "/", nil) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| // Store token using gothic's method | ||
| gothic.StoreInSession("access_token", tt.token, req, rec) | ||
|
|
||
| // Add cookie to request | ||
| req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) | ||
|
|
||
| // Add token to OAuth2Server's valid tokens | ||
| s.accessTokens[tt.token] = AccessToken{ | ||
| Token: tt.token, | ||
| ExpiresAt: time.Now().Add(tt.expires), | ||
| } | ||
|
|
||
| got := s.IsUserAuthenticated(c) | ||
| if got != tt.want { | ||
| t.Errorf("IsUserAuthenticated() = %v, want %v", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function name doesn't match implementation
The function name TestIsUserAuthenticated is too generic and doesn't reflect its purpose. The comment suggests it tests invalid tokens, but it only contains test cases for valid tokens. Consider renaming it to better reflect its purpose or expanding it to include the promised invalid token tests.
Rename the function to match its implementation:
-func TestIsUserAuthenticated(t *testing.T)
+func TestIsUserAuthenticatedWithValidToken(t *testing.T)📝 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.
| // TestIsUserAuthenticatedInvalidAccessToken tests the IsUserAuthenticated function with an invalid access token | |
| func TestIsUserAuthenticated(t *testing.T) { | |
| // Set the settings instance | |
| conf.Setting() | |
| tests := []struct { | |
| name string | |
| token string | |
| expires time.Duration | |
| want bool | |
| }{ | |
| { | |
| name: "valid token", | |
| token: "valid_token", | |
| expires: time.Hour, | |
| want: true, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| settings := &conf.Settings{ | |
| Security: conf.Security{ | |
| SessionSecret: "test-secret", | |
| }, | |
| } | |
| s := NewOAuth2Server() | |
| // Initialize gothic exactly as in production | |
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | |
| e := echo.New() | |
| req := httptest.NewRequest(http.MethodGet, "/", nil) | |
| rec := httptest.NewRecorder() | |
| c := e.NewContext(req, rec) | |
| // Store token using gothic's method | |
| gothic.StoreInSession("access_token", tt.token, req, rec) | |
| // Add cookie to request | |
| req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) | |
| // Add token to OAuth2Server's valid tokens | |
| s.accessTokens[tt.token] = AccessToken{ | |
| Token: tt.token, | |
| ExpiresAt: time.Now().Add(tt.expires), | |
| } | |
| got := s.IsUserAuthenticated(c) | |
| if got != tt.want { | |
| t.Errorf("IsUserAuthenticated() = %v, want %v", got, tt.want) | |
| } | |
| }) | |
| } | |
| } | |
| // TestIsUserAuthenticatedWithValidToken tests the IsUserAuthenticated function with an invalid access token | |
| func TestIsUserAuthenticatedWithValidToken(t *testing.T) { | |
| // Set the settings instance | |
| conf.Setting() | |
| tests := []struct { | |
| name string | |
| token string | |
| expires time.Duration | |
| want bool | |
| }{ | |
| { | |
| name: "valid token", | |
| token: "valid_token", | |
| expires: time.Hour, | |
| want: true, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| settings := &conf.Settings{ | |
| Security: conf.Security{ | |
| SessionSecret: "test-secret", | |
| }, | |
| } | |
| s := NewOAuth2Server() | |
| // Initialize gothic exactly as in production | |
| gothic.Store = sessions.NewCookieStore([]byte(settings.Security.SessionSecret)) | |
| e := echo.New() | |
| req := httptest.NewRequest(http.MethodGet, "/", nil) | |
| rec := httptest.NewRecorder() | |
| c := e.NewContext(req, rec) | |
| // Store token using gothic's method | |
| gothic.StoreInSession("access_token", tt.token, req, rec) | |
| // Add cookie to request | |
| req.Header.Set("Cookie", rec.Header().Get("Set-Cookie")) | |
| // Add token to OAuth2Server's valid tokens | |
| s.accessTokens[tt.token] = AccessToken{ | |
| Token: tt.token, | |
| ExpiresAt: time.Now().Add(tt.expires), | |
| } | |
| got := s.IsUserAuthenticated(c) | |
| if got != tt.want { | |
| t.Errorf("IsUserAuthenticated() = %v, want %v", got, tt.want) | |
| } | |
| }) | |
| } | |
| } |
| func isValidRedirect(redirectPath string) bool { | ||
| // Allow only relative paths | ||
| return strings.HasPrefix(redirectPath, "/") && | ||
| !strings.Contains(redirectPath, "//") && | ||
| !strings.Contains(redirectPath, "\\") && | ||
| !strings.Contains(redirectPath, "://") && | ||
| !strings.Contains(redirectPath, "..") && | ||
| len(redirectPath) < 512 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize paths before validation
Add path normalization to prevent bypass attempts using different path representations.
+import "path"
+
func isValidRedirect(redirectPath string) bool {
+ // Normalize the path first
+ redirectPath = path.Clean(redirectPath)
+
// Allow only relative paths
return strings.HasPrefix(redirectPath, "/") &&
!strings.Contains(redirectPath, "//") &&📝 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.
| func isValidRedirect(redirectPath string) bool { | |
| // Allow only relative paths | |
| return strings.HasPrefix(redirectPath, "/") && | |
| !strings.Contains(redirectPath, "//") && | |
| !strings.Contains(redirectPath, "\\") && | |
| !strings.Contains(redirectPath, "://") && | |
| !strings.Contains(redirectPath, "..") && | |
| len(redirectPath) < 512 | |
| } | |
| import "path" | |
| func isValidRedirect(redirectPath string) bool { | |
| // Normalize the path first | |
| redirectPath = path.Clean(redirectPath) | |
| // Allow only relative paths | |
| return strings.HasPrefix(redirectPath, "/") && | |
| !strings.Contains(redirectPath, "//") && | |
| !strings.Contains(redirectPath, "\\") && | |
| !strings.Contains(redirectPath, "://") && | |
| !strings.Contains(redirectPath, "..") && | |
| len(redirectPath) < 512 | |
| } |
| // initAuthRoutes initializes all authentication related routes | ||
| func (s *Server) initAuthRoutes() { | ||
| // Add rate limiter for auth and login routes | ||
| g := s.Echo.Group("") | ||
| g.Use(middleware.RateLimiter(middleware.NewRateLimiterMemoryStore(10))) | ||
|
|
||
| // OAuth2 routes | ||
| g.GET("/oauth2/authorize", s.Handlers.WithErrorHandling(s.OAuth2Server.HandleBasicAuthorize)) | ||
| g.POST("/oauth2/token", s.Handlers.WithErrorHandling(s.OAuth2Server.HandleBasicAuthToken)) | ||
| g.GET("/callback", s.Handlers.WithErrorHandling(s.OAuth2Server.HandleBasicAuthCallback)) | ||
|
|
||
| // Social authentication routes | ||
| g.GET("/auth/:provider", s.Handlers.WithErrorHandling(handleGothProvider)) | ||
| g.GET("/auth/:provider/callback", s.Handlers.WithErrorHandling(handleGothCallback)) | ||
|
|
||
| // Basic authentication routes | ||
| g.GET("/login", s.Handlers.WithErrorHandling(s.handleLoginPage)) | ||
| g.POST("/login", s.handleBasicAuthLogin) | ||
| g.GET("/logout", s.Handlers.WithErrorHandling(s.handleLogout)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security middleware for authentication routes
While the routes are well-organized by authentication type, consider adding essential security middleware:
- CORS protection for cross-origin requests
- Secure headers middleware
- Request ID middleware for better request tracing
func (s *Server) initAuthRoutes() {
// Add rate limiter for auth and login routes
g := s.Echo.Group("")
+ // Add security middleware
+ g.Use(middleware.RequestID())
+ g.Use(middleware.SecureWithConfig(middleware.SecureConfig{
+ XSSProtection: "1; mode=block",
+ ContentTypeNosniff: "nosniff",
+ XFrameOptions: "DENY",
+ HSTSMaxAge: 31536000,
+ ContentSecurityPolicy: "default-src 'self'",
+ }))
+ g.Use(middleware.CORSWithConfig(middleware.CORSConfig{
+ AllowOrigins: []string{"*"},
+ AllowMethods: []string{http.MethodGet, http.MethodPost},
+ AllowHeaders: []string{echo.HeaderOrigin, echo.HeaderContentType, echo.HeaderAccept},
+ }))
g.Use(middleware.RateLimiter(middleware.NewRateLimiterMemoryStore(10)))Committable suggestion skipped: line range outside the PR's diff.
| subnets := strings.Split(allowedSubnet.Subnet, ",") | ||
|
|
||
| for _, subnet := range subnets { | ||
| _, ipNet, err := net.ParseCIDR(strings.TrimSpace(subnet)) | ||
| if err == nil && ipNet.Contains(clientIP) { | ||
| s.Debug("Access allowed for IP %s", clientIP) | ||
| return true | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Improve subnet validation security
The current subnet validation has potential security issues:
- No validation of malformed CIDR ranges
- No limit on the number of subnets to check
subnets := strings.Split(allowedSubnet.Subnet, ",")
+ if len(subnets) > 100 { // Prevent DoS from too many subnets
+ s.Debug("Too many subnets configured")
+ return false
+ }
for _, subnet := range subnets {
- _, ipNet, err := net.ParseCIDR(strings.TrimSpace(subnet))
+ subnet = strings.TrimSpace(subnet)
+ if subnet == "" {
+ continue
+ }
+ _, ipNet, err := net.ParseCIDR(subnet)
if err == nil && ipNet.Contains(clientIP) {📝 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.
| subnets := strings.Split(allowedSubnet.Subnet, ",") | |
| for _, subnet := range subnets { | |
| _, ipNet, err := net.ParseCIDR(strings.TrimSpace(subnet)) | |
| if err == nil && ipNet.Contains(clientIP) { | |
| s.Debug("Access allowed for IP %s", clientIP) | |
| return true | |
| } | |
| } | |
| subnets := strings.Split(allowedSubnet.Subnet, ",") | |
| if len(subnets) > 100 { // Prevent DoS from too many subnets | |
| s.Debug("Too many subnets configured") | |
| return false | |
| } | |
| for _, subnet := range subnets { | |
| subnet = strings.TrimSpace(subnet) | |
| if subnet == "" { | |
| continue | |
| } | |
| _, ipNet, err := net.ParseCIDR(subnet) | |
| if err == nil && ipNet.Contains(clientIP) { | |
| s.Debug("Access allowed for IP %s", clientIP) | |
| return true | |
| } | |
| } |
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing WebServer validation.
The current validation only checks if the port is provided. Consider adding these essential validations:
- Port number range validation (1-65535)
- Port availability check
- Reserved port checks (e.g., warn if using well-known ports 0-1023)
if settings.Enabled {
// Check if port is provided when enabled
if settings.Port == "" {
return errors.New("WebServer port is required when enabled")
}
+ // Validate port number
+ portNum, err := strconv.Atoi(settings.Port)
+ if err != nil {
+ return fmt.Errorf("invalid port number: %w", err)
+ }
+ if portNum < 1 || portNum > 65535 {
+ return fmt.Errorf("port number must be between 1 and 65535")
+ }
+ if portNum < 1024 {
+ log.Printf("Warning: Using a well-known port (%d) may require elevated privileges", portNum)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| // validateSecuritySettings validates the security-specific settings | ||
| func validateSecuritySettings(settings *Security) error { | ||
| // Check if any OAuth provider is enabled | ||
| if (settings.BasicAuth.Enabled || settings.GoogleAuth.Enabled || settings.GithubAuth.Enabled) && settings.Host == "" { | ||
| return fmt.Errorf("security.host must be set when using authentication providers") | ||
| } | ||
|
|
||
| // Validate the subnet bypass setting against the allowed pattern | ||
| if settings.AllowSubnetBypass.Enabled { | ||
| subnets := strings.Split(settings.AllowSubnetBypass.Subnet, ",") | ||
| for _, subnet := range subnets { | ||
| _, _, err := net.ParseCIDR(strings.TrimSpace(subnet)) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid subnet format: %w", err) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance OAuth2 credentials validation.
The current validation only checks if the host is set. For better security, consider adding these essential validations for enabled OAuth2 providers:
- Client ID format and length validation
- Client secret minimum length and complexity requirements
- Redirect URI format validation
func validateSecuritySettings(settings *Security) error {
// Check if any OAuth provider is enabled
if (settings.BasicAuth.Enabled || settings.GoogleAuth.Enabled || settings.GithubAuth.Enabled) && settings.Host == "" {
return fmt.Errorf("security.host must be set when using authentication providers")
}
+
+ // Validate OAuth2 credentials
+ if settings.GoogleAuth.Enabled {
+ if err := validateOAuthCredentials("Google", settings.GoogleAuth.ClientID, settings.GoogleAuth.ClientSecret); err != nil {
+ return err
+ }
+ }
+ if settings.GithubAuth.Enabled {
+ if err := validateOAuthCredentials("GitHub", settings.GithubAuth.ClientID, settings.GithubAuth.ClientSecret); err != nil {
+ return err
+ }
+ }
// Validate the subnet bypass setting against the allowed pattern
if settings.AllowSubnetBypass.Enabled {
subnets := strings.Split(settings.AllowSubnetBypass.Subnet, ",")
for _, subnet := range subnets {
_, _, err := net.ParseCIDR(strings.TrimSpace(subnet))
if err != nil {
return fmt.Errorf("invalid subnet format: %w", err)
}
}
}
return nil
}
+// validateOAuthCredentials validates OAuth2 client credentials
+func validateOAuthCredentials(provider, clientID, clientSecret string) error {
+ if clientID == "" {
+ return fmt.Errorf("%s OAuth client ID is required when enabled", provider)
+ }
+ if clientSecret == "" {
+ return fmt.Errorf("%s OAuth client secret is required when enabled", provider)
+ }
+ if len(clientSecret) < 16 {
+ return fmt.Errorf("%s OAuth client secret should be at least 16 characters", provider)
+ }
+ 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.
| // validateSecuritySettings validates the security-specific settings | |
| func validateSecuritySettings(settings *Security) error { | |
| // Check if any OAuth provider is enabled | |
| if (settings.BasicAuth.Enabled || settings.GoogleAuth.Enabled || settings.GithubAuth.Enabled) && settings.Host == "" { | |
| return fmt.Errorf("security.host must be set when using authentication providers") | |
| } | |
| // Validate the subnet bypass setting against the allowed pattern | |
| if settings.AllowSubnetBypass.Enabled { | |
| subnets := strings.Split(settings.AllowSubnetBypass.Subnet, ",") | |
| for _, subnet := range subnets { | |
| _, _, err := net.ParseCIDR(strings.TrimSpace(subnet)) | |
| if err != nil { | |
| return fmt.Errorf("invalid subnet format: %w", err) | |
| } | |
| } | |
| } | |
| // validateSecuritySettings validates the security-specific settings | |
| func validateSecuritySettings(settings *Security) error { | |
| // Check if any OAuth provider is enabled | |
| if (settings.BasicAuth.Enabled || settings.GoogleAuth.Enabled || settings.GithubAuth.Enabled) && settings.Host == "" { | |
| return fmt.Errorf("security.host must be set when using authentication providers") | |
| } | |
| // Validate OAuth2 credentials | |
| if settings.GoogleAuth.Enabled { | |
| if err := validateOAuthCredentials("Google", settings.GoogleAuth.ClientID, settings.GoogleAuth.ClientSecret); err != nil { | |
| return err | |
| } | |
| } | |
| if settings.GithubAuth.Enabled { | |
| if err := validateOAuthCredentials("GitHub", settings.GithubAuth.ClientID, settings.GithubAuth.ClientSecret); err != nil { | |
| return err | |
| } | |
| } | |
| // Validate the subnet bypass setting against the allowed pattern | |
| if settings.AllowSubnetBypass.Enabled { | |
| subnets := strings.Split(settings.AllowSubnetBypass.Subnet, ",") | |
| for _, subnet := range subnets { | |
| _, _, err := net.ParseCIDR(strings.TrimSpace(subnet)) | |
| if err != nil { | |
| return fmt.Errorf("invalid subnet format: %w", err) | |
| } | |
| } | |
| } | |
| return nil | |
| } | |
| // validateOAuthCredentials validates OAuth2 client credentials | |
| func validateOAuthCredentials(provider, clientID, clientSecret string) error { | |
| if clientID == "" { | |
| return fmt.Errorf("%s OAuth client ID is required when enabled", provider) | |
| } | |
| if clientSecret == "" { | |
| return fmt.Errorf("%s OAuth client secret is required when enabled", provider) | |
| } | |
| if len(clientSecret) < 16 { | |
| return fmt.Errorf("%s OAuth client secret should be at least 16 characters", provider) | |
| } | |
| return nil | |
| } |
| type AllowSubnetBypass struct { | ||
| Enabled bool // true to enable subnet bypass | ||
| Subnet string // disable OAuth2 in subnet | ||
| } | ||
|
|
||
| type AllowCloudflareBypass struct { | ||
| Enabled bool // true to enable CF Access | ||
| TeamDomain string // Cloudflare team domain | ||
| Audience string // Cloudflare policy audience | ||
| } |
There was a problem hiding this comment.
Add validation for security-critical fields.
The bypass configuration structs contain security-critical fields that should be validated:
Subnetshould be a valid CIDR notationTeamDomainshould be a valid domain name
Add validation in the ValidateSettings function:
func validateSecuritySettings(settings *Security) error {
if settings.AllowSubnetBypass.Enabled {
if _, _, err := net.ParseCIDR(settings.AllowSubnetBypass.Subnet); err != nil {
return fmt.Errorf("invalid subnet CIDR: %w", err)
}
}
if settings.AllowCloudflareBypass.Enabled {
if settings.AllowCloudflareBypass.TeamDomain == "" {
return fmt.Errorf("team domain is required when Cloudflare bypass is enabled")
}
if !govalidator.IsDNSName(settings.AllowCloudflareBypass.TeamDomain) {
return fmt.Errorf("invalid team domain")
}
}
return nil
}| type Security struct { | ||
|
|
||
| // Host is the primary hostname used for TLS certificates | ||
| // and OAuth redirect URLs. Required when using AutoTLS or | ||
| // authentication providers. Used to form the redirect URIs. | ||
| Host string | ||
|
|
||
| // AutoTLS enables automatic TLS certificate management using | ||
| // Let's Encrypt. Requires Host to be set and port 80/443 access. | ||
| AutoTLS bool | ||
|
|
||
| RedirectToHTTPS bool // true to redirect to HTTPS | ||
| AllowSubnetBypass AllowSubnetBypass // subnet bypass configuration | ||
| AllowCloudflareBypass AllowCloudflareBypass // Cloudflare Access configuration | ||
| BasicAuth BasicAuth // password authentication configuration | ||
| GoogleAuth SocialProvider // Google OAuth2 configuration | ||
| GithubAuth SocialProvider // Github OAuth2 configuration | ||
| SessionSecret string // secret for session cookie | ||
| } |
There was a problem hiding this comment.
Add validation for session secret.
The session secret is a critical security field that should be validated for minimum length and entropy.
Add validation in the ValidateSettings function:
func validateSecuritySettings(settings *Security) error {
if settings.SessionSecret == "" {
return fmt.Errorf("session secret is required")
}
// Ensure minimum length of 32 characters for 256 bits of entropy
if len(settings.SessionSecret) < 32 {
return fmt.Errorf("session secret must be at least 32 characters long")
}
// Ensure the secret contains a mix of characters for better entropy
if !regexp.MustCompile(`[A-Za-z]`).MatchString(settings.SessionSecret) ||
!regexp.MustCompile(`[0-9]`).MatchString(settings.SessionSecret) {
return fmt.Errorf("session secret must contain a mix of letters and numbers")
}
return nil
}
Security & UI Enhancements
🔒 Security Features
1. Authentication
2. Access Control
3. Configuration
🎨 UI/UX Improvements
1. Settings Interface
2. Navigation
🛠 Technical Improvements
1. Code Organization
2. Frontend Enhancements