-
Notifications
You must be signed in to change notification settings - Fork 20
Fix configuration persistence and dashboard authentication issues #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Added remote WebSocket settings (useRemoteWebSocket, websocketHost) to config schema - Fixed middleware Edge Runtime compatibility by removing Node.js imports - Implemented client-side authentication check with AuthCheck component - Fixed config merging to properly handle nested server configuration - Updated defaults to include all server configuration fields - Fixed useWebSocketUrl hook to correctly access config data structure - Improved authentication flow with password-required cookie mechanism Co-authored-by: Crypto Gnome <[email protected]>
|
|
||
| // Set cookie to indicate if password is required | ||
| if (data.passwordRequired) { | ||
| document.cookie = 'password-required=true; path=/; max-age=86400'; // 24 hours |
Check warning
Code scanning / CodeQL
Clear text transmission of sensitive cookie Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we need to ensure that any sensitive cookies set in the code, like password-required, include the secure attribute so they are only sent over HTTPS connections. Specifically, in src/components/AuthCheck.tsx, two places set or update the password-required cookie (lines 18 and 31). We should add the string ; secure to the cookie value in both cases. This change can be safely made by appending ; secure to the cookie string in both assignments to document.cookie. No new imports or methods are needed.
-
Copy modified line R18 -
Copy modified line R31
| @@ -15,7 +15,7 @@ | ||
|
|
||
| // Set cookie to indicate if password is required | ||
| if (data.passwordRequired) { | ||
| document.cookie = 'password-required=true; path=/; max-age=86400'; // 24 hours | ||
| document.cookie = 'password-required=true; path=/; max-age=86400; secure'; // 24 hours | ||
|
|
||
| // Check if we have a valid auth token | ||
| const authToken = document.cookie | ||
| @@ -28,7 +28,7 @@ | ||
| } | ||
| } else { | ||
| // No password required, clear the cookie | ||
| document.cookie = 'password-required=false; path=/; max-age=86400'; | ||
| document.cookie = 'password-required=false; path=/; max-age=86400; secure'; | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to check auth status:', error); |
| } | ||
| } else { | ||
| // No password required, clear the cookie | ||
| document.cookie = 'password-required=false; path=/; max-age=86400'; |
Check warning
Code scanning / CodeQL
Clear text transmission of sensitive cookie Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we should append Secure to the cookie string whenever it is set, ensuring that browsers only send this cookie over HTTPS connections. In the context of the file src/components/AuthCheck.tsx, this involves updating line 31 where the cookie is being set, so that 'password-required=false; path=/; max-age=86400' becomes 'password-required=false; path=/; max-age=86400; Secure'. No new imports or methods are needed, as this is a simple string change and document.cookie supports this attribute natively.
-
Copy modified line R31
| @@ -28,7 +28,7 @@ | ||
| } | ||
| } else { | ||
| // No password required, clear the cookie | ||
| document.cookie = 'password-required=false; path=/; max-age=86400'; | ||
| document.cookie = 'password-required=false; path=/; max-age=86400; Secure'; | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to check auth status:', error); |
Code Review for PR #13Security Review (CRITICAL) - PASSED
Critical Issue FoundAuthentication Token Weakness (src/app/api/auth/login/route.ts:36) Medium Priority Issues
Positive Aspects
Recommendations
The PR addresses the reported issues effectively but needs the auth token security issue resolved before merging. |
…50928-1133 Fix configuration persistence and dashboard authentication issues
Fixes #5
Summary
Changes
Authentication System:
Configuration Schema:
Config Persistence:
Testing
🤖 Generated with Claude Code