-
Notifications
You must be signed in to change notification settings - Fork 0
Security improvements #8
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements security improvements by enhancing session cookie configuration, refining authentication logic, and adding new user management functions. Key changes include:
- Adding SESSION_COOKIE_SAMESITE for CSRF protection.
- Introducing new database operations for user creation, removal, and cleanup.
- Updating the authentication endpoint and client-side registration logic.
Reviewed Changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/settings.py | Added secure session cookie settings to improve CSRF protection |
| app/database/user.py | Added user management functions with new debug logging and error handling |
| app/authenticator/authenticator.py | Implemented a registration method using secure password hashing |
| app/authentication/static/registration.js | Updated client-side registration, though use of GET with sensitive data is risky |
| app/authentication/authentication.py | Added a registration endpoint that currently uses GET for passing credentials |
| app/init.py | Updated session configuration to include the new secure cookie setting |
Files not reviewed (2)
- app/authentication/static/registration.css: Language not supported
- app/authentication/templates/authentication/registration.html: Language not supported
| fetch(`/auth/registration/${username}/${password}`, { | ||
| method: 'GET', | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| } |
Copilot
AI
May 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending sensitive credentials via a GET request exposes them in the URL. Consider switching to a POST request with the credentials in the request body.
| fetch(`/auth/registration/${username}/${password}`, { | |
| method: 'GET', | |
| headers: { | |
| 'Content-Type': 'application/json' | |
| } | |
| fetch('/auth/registration', { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json' | |
| }, | |
| body: JSON.stringify({ username: username, password: password }) |
| @authentication_bp.route('/registration/<username>/<password>', methods=['GET']) | ||
| def register(username, password): | ||
| """ | ||
| route: /auth/registration/<username>/<password> | ||
| """ |
Copilot
AI
May 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using GET for user registration with credentials in the URL is insecure. It is recommended to change this endpoint to use POST to better protect sensitive data.
| @authentication_bp.route('/registration/<username>/<password>', methods=['GET']) | |
| def register(username, password): | |
| """ | |
| route: /auth/registration/<username>/<password> | |
| """ | |
| @authentication_bp.route('/registration', methods=['POST']) | |
| def register(): | |
| """ | |
| route: /auth/registration | |
| """ | |
| from flask import request | |
| data = request.get_json() | |
| username = data.get('username') | |
| password = data.get('password') |
| query = f"INSERT INTO {self.table_user} (userid, username, password) VALUES (?, ?, ?);" | ||
|
|
||
| if DEBUG_MODE: | ||
| print(f"[DEBUG] {FILE_NAME}: create_user: {query} with username: {username} and password: {password}") |
Copilot
AI
May 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug logging of sensitive information such as a plaintext password can lead to security risks. Avoid including passwords in logs even in debug mode.
| print(f"[DEBUG] {FILE_NAME}: create_user: {query} with username: {username} and password: {password}") | |
| print(f"[DEBUG] {FILE_NAME}: create_user: {query} with username: {username} and password: [REDACTED]") |
…in-template into improvements#1 update: latest improvements#1
…thon-login-template into securityImprovements#2
add: debug. All is fine. No functional changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR strengthens security by enforcing stricter session cookies, upgrading password handling to bcrypt, and hardening client/server authentication flows.
- Added
SameSite=Strictto session cookies for CSRF protection. - Replaced SHA-256 password hashing with bcrypt and added verification methods.
- Wrapped JS listeners in
DOMContentLoadedand added debug logging for authentication flows.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| app/settings.py | Added SESSION_COOKIE_SAMESITE = 'Strict'. |
| app/home/static/home-private.js | Wrapped sign-out binding in DOMContentLoaded. |
| app/authenticator/authenticator.py | Removed SHA-256, introduced bcrypt hashing & verify. |
| app/authentication/static/signin.js | Wrapped sign-in handler, added debug console.log. |
| app/authentication/authentication.py | Imported DEBUG_MODE, added debug print in route. |
| app/init.py | Configured SESSION_COOKIE_SAMESITE in app. |
Comments suppressed due to low confidence (3)
app/authentication/authentication.py:31
- Calling
login_userbefore verifying credentials allows unauthorized users to be logged in. Move this call to afterauthenticator.authenticatereturns true.
login_user(username)
app/authentication/static/signin.js:12
- Sending credentials in a GET URL exposes sensitive data in logs and browser history. Use a POST request with credentials in the request body over HTTPS instead.
fetch(`/auth/signin/${username}/${password}`, {
app/authenticator/authenticator.py:24
- [nitpick] Consider adding unit tests for
hash_passwordandverify_passwordto validate hashing and edge-case password verification.
def hash_password(self, password):
| if DEBUG_MODE: | ||
| print(f"[DEBUG] authenticator.py: Verifying password: {checkpw(password.encode(), hashed_password.encode())}") |
Copilot
AI
May 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using print for debug output can be replaced with the standard logging module to better control log levels and formatting.
| if DEBUG_MODE: | |
| print(f"[DEBUG] authenticator.py: Verifying password: {checkpw(password.encode(), hashed_password.encode())}") | |
| logging.debug(f"authenticator.py: Verifying password: {checkpw(password.encode(), hashed_password.encode())}") |
Resolves #2
a. SQL Injection Protection
b. Password Handling
c. Session Security
d. Authentication Endpoint
e. CSRF Protection