Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements username/password authentication to secure the Settings page, addressing issue #8. The implementation adds environment variable-based authentication with credentials stored in the .env file.
- Added authentication context and protected route components for Settings page access
- Configured environment variables for admin credentials in Docker and Vite
- Updated documentation to include authentication setup instructions
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docker-compose.yml | Adds MARS_ADMIN and MARS_PASSWORD environment variables to the client service |
| client/vite.config.ts | Exposes authentication environment variables to the client build |
| client/src/utils/AuthContext.tsx | Implements authentication context with session storage and credential validation |
| client/src/main.tsx | Wraps the app with AuthProvider for authentication state management |
| client/src/components/ProtectedRoute.tsx | Creates protected route wrapper that shows login form for unauthenticated users |
| client/src/components/LoginForm.tsx | Implements login form modal with username/password inputs |
| client/src/App.tsx | Protects the Settings route with authentication requirement |
| README.md | Documents the new authentication feature and setup instructions |
| .env.example | Adds example configuration for admin credentials |
| 'process.env.GITHUB_ENTERPRISE_NAME': JSON.stringify(process.env.VITE_GITHUB_ENTERPRISE_NAME) | ||
| 'process.env.GITHUB_ENTERPRISE_NAME': JSON.stringify(process.env.VITE_GITHUB_ENTERPRISE_NAME), | ||
| 'process.env.MARS_ADMIN': JSON.stringify(process.env.VITE_MARS_ADMIN), | ||
| 'process.env.MARS_PASSWORD': JSON.stringify(process.env.VITE_MARS_PASSWORD) |
There was a problem hiding this comment.
Exposing credentials to the client-side code is a security vulnerability. These environment variables will be visible in the built JavaScript bundle to any user who inspects the source code. Consider implementing server-side authentication instead.
Note: See the diff below for a potential fix:
@@ -10,9 +10,7 @@
strictPort: true
},
define: {
- 'process.env.GITHUB_TOKEN': JSON.stringify(process.env.VITE_GITHUB_TOKEN),
- 'process.env.GITHUB_ENTERPRISE_NAME': JSON.stringify(process.env.VITE_GITHUB_ENTERPRISE_NAME),
- 'process.env.MARS_ADMIN': JSON.stringify(process.env.VITE_MARS_ADMIN),
- 'process.env.MARS_PASSWORD': JSON.stringify(process.env.VITE_MARS_PASSWORD)
+ // Do not expose sensitive credentials to client-side code.
+ // If you need to expose non-sensitive config, add it here.
}
})
| }; | ||
|
|
||
| const logout = () => { | ||
| setIsAuthenticated(false); |
There was a problem hiding this comment.
Client-side authentication is fundamentally insecure. The credentials are exposed in the built JavaScript and can be easily bypassed by anyone with browser dev tools. This provides no real security protection.
| setIsAuthenticated(false); | |
| sessionStorage.removeItem('authToken'); | |
| } | |
| }, [isAuthenticated]); | |
| const login = async (username: string, password: string): Promise<boolean> => { | |
| try { | |
| const response = await fetch('/api/login', { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ username, password }), | |
| }); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| // Assume the backend returns a JWT token | |
| sessionStorage.setItem('authToken', data.token); | |
| setIsAuthenticated(true); | |
| return true; | |
| } else { | |
| setIsAuthenticated(false); | |
| return false; | |
| } | |
| } catch (error) { | |
| setIsAuthenticated(false); | |
| return false; | |
| } | |
| }; | |
| const logout = () => { | |
| setIsAuthenticated(false); | |
| sessionStorage.removeItem('authToken'); |
| onSuccess={() => setShowLogin(false)} | ||
| onCancel={() => { | ||
| // Redirect to dashboard when cancel is clicked | ||
| window.location.href = '/'; |
There was a problem hiding this comment.
Using window.location.href for navigation bypasses React Router and can cause issues. Use the navigate hook from react-router-dom instead for proper SPA navigation.
| window.location.href = '/'; | |
| navigate('/'); |
| - Default username: `admin` (configurable via `MARS_ADMIN` in `.env`) | ||
| - Default password: `secure123` (configurable via `MARS_PASSWORD` in `.env`) | ||
|
|
||
| **Important**: Make sure to change the default credentials in production environments! |
There was a problem hiding this comment.
The warning about changing default credentials is insufficient given the fundamental security flaw. The authentication system provides no real security since credentials are client-side and easily bypassed.
| **Important**: Make sure to change the default credentials in production environments! | |
| The Settings page is protected with username/password authentication, which must be implemented and validated server-side for security: | |
| - Default username: `admin` (configurable via `MARS_ADMIN` in `.env`) | |
| - Default password: `secure123` (configurable via `MARS_PASSWORD` in `.env`) | |
| **Important**: Credentials must always be validated server-side. Never implement authentication checks on the client, as this provides no real security and can be easily bypassed. Always change default credentials in production, store secrets securely using environment variables, and use industry-standard authentication libraries (e.g., bcrypt for password hashing, Passport.js for authentication). |
This solves the #8
User/Password now will be required to access the Settings view
User and Password should be defined in the .env file, like: