Refactor: Fix brittle sys.path imports in test_validators #54
Conversation
Summary of ChangesHello @Goldyeti2006, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a foundational Flask-based web application designed for managing images. It provides essential features such as user management, a secure image upload mechanism that includes metadata capture, and a browsable image gallery. The changes also introduce a set of specialized utility functions for robust file validation and image processing, which are critical for maintaining data integrity and security. Furthermore, a comprehensive suite of unit tests has been added to verify the correctness and resilience of these newly implemented validation components. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a substantial addition, introducing a new Flask application, rather than just fixing an import path as the title suggests. While the test import fix is correct, the new application code has several critical security vulnerabilities and maintainability issues that need to be addressed. My review focuses on these key areas:
- Security: There are critical vulnerabilities related to a hardcoded
SECRET_KEYand the automatic creation of a default user with a weak, hardcoded password. - Code Structure: There is significant code duplication between
app.py,models.py, andforms.py, which will make maintenance difficult. - Maintainability: The templates make extensive use of inline styles, which should be moved to external CSS files. Also, error logging should use the application logger instead of
print().
| static_url_path='/static') | ||
|
|
||
| # SECURITY FIX: Load SECRET_KEY from environment variable | ||
| app.config['SECRET_KEY'] = os.environ.get('SECRET_KEY') or 'dev-secret-key-change-in-production' |
There was a problem hiding this comment.
Using a hardcoded, predictable fallback for SECRET_KEY is a significant security risk. If the environment variable is not set in production, this weak key will be used, making the application vulnerable to session hijacking and other attacks. The application should fail on startup if SECRET_KEY is not set in a production environment. Removing the fallback will cause the app to fail if the key isn't set, which is a safer default.
| app.config['SECRET_KEY'] = os.environ.get('SECRET_KEY') or 'dev-secret-key-change-in-production' | |
| app.config['SECRET_KEY'] = os.environ.get('SECRET_KEY') |
| if User.query.count() == 0: | ||
| default_user = User(username='default', email='default@bhv.org') | ||
| default_user.set_password('changeme123') | ||
| db.session.add(default_user) | ||
| db.session.commit() |
There was a problem hiding this comment.
Creating a default user with a hardcoded password (changeme123) automatically on application startup is a major security vulnerability. This user account could be exploited by attackers. Initial database seeding should be handled by a separate, explicit administrative script or command (e.g., a Flask CLI command), not automatically when the application runs.
| # Database setup | ||
| db = SQLAlchemy() | ||
|
|
||
| # Models | ||
| class User(db.Model): | ||
| __tablename__ = 'users' | ||
| id = db.Column(db.Integer, primary_key=True) | ||
| username = db.Column(db.String(80), unique=True, nullable=False, index=True) | ||
| email = db.Column(db.String(120), unique=True, nullable=False, index=True) | ||
| password_hash = db.Column(db.String(255), nullable=False) | ||
| is_admin = db.Column(db.Boolean, default=False) | ||
| created_at = db.Column(db.DateTime, default=datetime.utcnow) | ||
| images = db.relationship('Image', backref='owner', lazy='dynamic', cascade='all, delete-orphan') | ||
|
|
||
| def set_password(self, password): | ||
| self.password_hash = generate_password_hash(password) | ||
|
|
||
| def check_password(self, password): | ||
| return check_password_hash(self.password_hash, password) | ||
|
|
||
| class Image(db.Model): | ||
| __tablename__ = 'images' | ||
| id = db.Column(db.Integer, primary_key=True) | ||
| filename = db.Column(db.String(255), nullable=False) | ||
| original_filename = db.Column(db.String(255), nullable=False) | ||
| title = db.Column(db.String(200)) | ||
| description = db.Column(db.Text) | ||
| file_size = db.Column(db.Integer) | ||
| mime_type = db.Column(db.String(50)) | ||
| width = db.Column(db.Integer) | ||
| height = db.Column(db.Integer) | ||
| user_id = db.Column(db.Integer, db.ForeignKey('users.id'), nullable=False, index=True) | ||
| uploaded_at = db.Column(db.DateTime, default=datetime.utcnow, index=True) | ||
|
|
||
| # Forms | ||
| class ImageUploadForm(FlaskForm): | ||
| image = FileField('Image', validators=[ | ||
| FileRequired(message='Please select an image'), | ||
| FileAllowed(['jpg', 'jpeg', 'png', 'gif'], 'Images only!') | ||
| ]) | ||
| title = StringField('Title', validators=[ | ||
| DataRequired(message='Title is required'), | ||
| Length(min=3, max=200, message='Title must be between 3 and 200 characters') | ||
| ]) | ||
| description = TextAreaField('Description (Your Story)', validators=[ | ||
| Optional(), | ||
| Length(max=5000, message='Description must be less than 5000 characters') | ||
| ]) | ||
| submit = SubmitField('Upload Image') |
There was a problem hiding this comment.
The db object, User and Image models, and the ImageUploadForm are all defined here, but they are also defined in bhv/models.py and bhv/forms.py which are also part of this PR. This duplication will cause maintenance problems. You should remove this entire block and instead import the necessary objects from the other modules.
For example, at the top of the file, you would add:
from bhv.models import db, User, Image
from bhv.forms import ImageUploadFormThis will also allow you to remove the now-unused imports for SQLAlchemy and WTForms from this file.
| mime_type=file.mimetype, | ||
| width=0, | ||
| height=0, | ||
| user_id=1 |
There was a problem hiding this comment.
The user_id is hardcoded to 1. This is incorrect and will associate all uploaded images with the same user. This should be replaced with the ID of the currently authenticated user. Once user authentication is implemented (e.g., with flask-login), you should use current_user.id here.
| user_id=1 | |
| user_id=1 # TODO: Replace with current_user.id after implementing authentication |
| def validate_image_content(file_path): | ||
| """Simple validation to check if the file exists on disk.""" | ||
| return 'jpeg' if os.path.exists(file_path) else None |
There was a problem hiding this comment.
The function validate_image_content is unused and has several issues:
- The name is misleading. It only checks for file existence; it doesn't validate image content.
- It incorrectly returns the hardcoded string
'jpeg'if the file exists, regardless of the actual file type.
Since this function is not used and is incorrect, it should be removed.
| <div style="text-align: center; padding: 50px 20px;"> | ||
| <h2 style="font-size: 2.5rem; margin-bottom: 20px;">Welcome to BHV</h2> | ||
| <p style="font-size: 1.2rem; color: #666; max-width: 600px; margin: 0 auto 30px;"> | ||
| A secure platform for storing and sharing your behavioral health journey through images and narratives. | ||
| </p> | ||
| <div style="margin-top: 40px;"> | ||
| <a href="{{ url_for('upload') }}" style="display: inline-block; background: #3498db; color: white; padding: 15px 40px; text-decoration: none; border-radius: 5px; font-size: 1.1rem; margin: 10px;">Upload Your Story</a> | ||
| <a href="{{ url_for('gallery') }}" style="display: inline-block; background: #2ecc71; color: white; padding: 15px 40px; text-decoration: none; border-radius: 5px; font-size: 1.1rem; margin: 10px;">View Gallery</a> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
This template uses inline style attributes for styling. This makes the code harder to read and maintain, and mixes concerns (structure and presentation). It's a best practice to move all styling to external CSS files and use classes to apply them.
| <div style="max-width: 600px; margin: 0 auto; background: white; padding: 30px; border-radius: 10px; box-shadow: 0 2px 10px rgba(0,0,0,0.1);"> | ||
| <h2 style="margin-bottom: 20px; color: #2c3e50;">Upload Your Image</h2> | ||
| <form method="POST" enctype="multipart/form-data"> | ||
| {{ form.hidden_tag() }} | ||
| <div style="margin-bottom: 20px;"> | ||
| {{ form.image.label(style="display: block; margin-bottom: 5px; font-weight: bold;") }} | ||
| {{ form.image(style="width: 100%; padding: 10px;") }} | ||
| {% if form.image.errors %} | ||
| {% for error in form.image.errors %}<p style="color: #e74c3c;">{{ error }}</p>{% endfor %} | ||
| {% endif %} | ||
| <small style="color: #666;">Allowed: JPG, PNG, GIF (Max 5MB)</small> | ||
| </div> | ||
| <div style="margin-bottom: 20px;"> | ||
| {{ form.title.label(style="display: block; margin-bottom: 5px; font-weight: bold;") }} | ||
| {{ form.title(style="width: 100%; padding: 10px; border: 1px solid #ddd; border-radius: 5px;") }} | ||
| {% if form.title.errors %} | ||
| {% for error in form.title.errors %}<p style="color: #e74c3c;">{{ error }}</p>{% endfor %} | ||
| {% endif %} | ||
| </div> | ||
| <div style="margin-bottom: 20px;"> | ||
| {{ form.description.label(style="display: block; margin-bottom: 5px; font-weight: bold;") }} | ||
| {{ form.description(style="width: 100%; padding: 10px; border: 1px solid #ddd; border-radius: 5px; min-height: 150px;") }} | ||
| {% if form.description.errors %} | ||
| {% for error in form.description.errors %}<p style="color: #e74c3c;">{{ error }}</p>{% endfor %} | ||
| {% endif %} | ||
| </div> | ||
| <div> | ||
| {{ form.submit(style="width: 100%; padding: 15px; background: #3498db; color: white; border: none; border-radius: 5px; font-size: 1.1rem; cursor: pointer;") }} | ||
| </div> | ||
| </form> | ||
| </div> |
There was a problem hiding this comment.
Similar to index.html, this template heavily uses inline style attributes. For better maintainability and separation of concerns, these styles should be extracted into a separate CSS file and applied using classes.
| } | ||
| # SECURITY & CODE QUALITY FIX: Catching specific exceptions | ||
| except (IOError, UnidentifiedImageError) as e: | ||
| print(f"Error processing image metadata for {file_path}: {e}") |
There was a problem hiding this comment.
Using print() for logging errors is not recommended in a web application, as the output might get lost or not be properly managed. You should use Flask's application logger instead (e.g., current_app.logger.error(...)).
| def sanitize_filename(filename): | ||
| """Removes unsafe characters and limits filename length.""" | ||
| filename = secure_filename(filename) | ||
| filename = filename.replace('/', '').replace('\\', '') |
There was a problem hiding this comment.
The werkzeug.utils.secure_filename function already handles path separators like / and \. This line is redundant and can be removed.
| BASE_DIR = Path(__file__).parent | ||
|
|
||
| class Config: | ||
| SECRET_KEY = os.environ.get('SECRET_KEY') or 'dev-secret-key-change-in-production' |
There was a problem hiding this comment.
Similar to app.py, this configuration uses a hardcoded fallback secret key. While this is for tests and less critical, it's a good practice to avoid this pattern. For test configurations, it's better to use a simple, static key directly.
| SECRET_KEY = os.environ.get('SECRET_KEY') or 'dev-secret-key-change-in-production' | |
| SECRET_KEY = 'test-secret-key' |
4 participants
Summary
Removes manual
sys.path.appendmodification intest_validators.py.Changes
sysandosimports used for path manipulation.python -m unittest discover tests.Verification
"Note: This fix is built on top of PR #50."