feat: BHV prototype with auth, uploads, and UI #65
Conversation
Summary of ChangesHello @Sahil-u07, 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 delivers a comprehensive, functional prototype for the Behavioral Health Vault (BHV) application. It establishes a robust backend API using Flask for secure user authentication, image uploads with descriptive narratives, and administrative controls. Concurrently, a modern and responsive frontend built with React, Vite, and Tailwind CSS provides a seamless user experience, allowing users to manage their entries and administrators to oversee the system. The prototype aims to demonstrate the core architectural flow and key features of the BHV system, emphasizing a minimal yet effective design. 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 introduces an impressive full-stack prototype for the BHV application, covering core features like authentication, image uploads, and a functional UI. The structure is well-organized. My review focuses on several critical security vulnerabilities in the backend concerning authentication, authorization, and data privacy that should be addressed. I've also provided feedback on potential bugs, such as orphaned file handling, and suggestions to improve code maintainability and correctness in both the frontend and backend.
| @images_bp.route('/file/<filename>', methods=['GET']) | ||
| def get_image_file(filename): | ||
| return send_from_directory(current_app.config['UPLOAD_FOLDER'], filename) |
There was a problem hiding this comment.
The /file/<filename> endpoint, which serves image files, lacks any authentication or authorization checks. This means that anyone with a link to an image can view it. Given the sensitive nature of a behavioral health application, this is a critical privacy and security vulnerability. This endpoint must be protected with @token_required and should verify that the current user has permission to view the requested image.
| @images_bp.route('/file/<filename>', methods=['GET']) | |
| def get_image_file(filename): | |
| return send_from_directory(current_app.config['UPLOAD_FOLDER'], filename) | |
| @images_bp.route('/file/<filename>', methods=['GET']) | |
| @token_required | |
| def get_image_file(current_user, filename): | |
| image = Image.query.filter_by(filename=filename).first_or_404() | |
| if image.user_id != current_user.id and not current_user.is_admin: | |
| return jsonify({'message': 'Permission denied'}), 403 | |
| return send_from_directory(current_app.config['UPLOAD_FOLDER'], filename) | |
| data = request.get_json() | ||
| email = data.get('email') | ||
| password = data.get('password') | ||
| is_admin = data.get('email') == 'admin@bhv.com' |
There was a problem hiding this comment.
Hardcoding admin user creation based on a specific email address is a critical security vulnerability. Anyone who knows this email (admin@bhv.com) can sign up and gain administrative privileges. Admin users should be created through a secure, out-of-band mechanism, such as a CLI command.
| is_admin = data.get('email') == 'admin@bhv.com' | |
| is_admin = False | |
| import os | ||
|
|
||
| class Config: | ||
| SECRET_KEY = os.environ.get('SECRET_KEY') or 'dev-secret-key-change-in-prod' |
There was a problem hiding this comment.
The SECRET_KEY has a weak, hardcoded default value committed to the repository. This is a critical security vulnerability if the application is deployed to production with this key, as it would allow attackers to forge authentication tokens. Secret keys should never be stored in version control.
| SECRET_KEY = os.environ.get('SECRET_KEY') or 'dev-secret-key-change-in-prod' | |
| SECRET_KEY = os.environ.get('SECRET_KEY') | |
| if user_id: | ||
| if user_id == 'me': | ||
| query = query.filter_by(user_id=current_user.id) | ||
| else: | ||
| query = query.filter_by(user_id=user_id) | ||
|
|
There was a problem hiding this comment.
This logic allows any authenticated user to view images of any other user by passing a user_id. For a behavioral health application, exposing all user content by default is a significant privacy risk. Access should be restricted by default.
I suggest modifying the logic so that regular users can only see their own images, while admins can view any user's images. If no user_id is provided, it should default to showing only the current user's images unless they are an admin.
| if user_id: | |
| if user_id == 'me': | |
| query = query.filter_by(user_id=current_user.id) | |
| else: | |
| query = query.filter_by(user_id=user_id) | |
| if user_id: | |
| if user_id == 'me' or int(user_id) == current_user.id: | |
| query = query.filter_by(user_id=current_user.id) | |
| elif current_user.is_admin: | |
| query = query.filter_by(user_id=user_id) | |
| else: | |
| return jsonify({'message': 'Permission denied to view this user\'s images'}), 403 | |
| elif not current_user.is_admin: | |
| # Default to current user's images if not an admin | |
| query = query.filter_by(user_id=current_user.id) | |
| except Exception as e: | ||
| return jsonify({'message': 'Token is invalid', 'error': str(e)}), 401 |
There was a problem hiding this comment.
Catching a generic Exception is too broad. It can mask underlying issues with token validation, such as bugs in your code or specific JWT errors (like an expired token). This makes debugging difficult and could hide security-related problems.
| except Exception as e: | |
| return jsonify({'message': 'Token is invalid', 'error': str(e)}), 401 | |
| except jwt.ExpiredSignatureError: | |
| return jsonify({'message': 'Token has expired'}), 401 | |
| except jwt.InvalidTokenError: | |
| return jsonify({'message': 'Token is invalid'}), 401 | |
| def migrate(): | ||
| conn = sqlite3.connect(db_path) | ||
| cursor = conn.cursor() | ||
|
|
||
| try: | ||
| cursor.execute("ALTER TABLE user ADD COLUMN is_admin BOOLEAN DEFAULT 0") | ||
| print("Added is_admin column to user table") | ||
| except sqlite3.OperationalError as e: | ||
| if "duplicate column name" in str(e): | ||
| print("Column is_admin already exists") | ||
| else: | ||
| print(f"Error: {e}") | ||
|
|
||
| conn.commit() | ||
| conn.close() |
There was a problem hiding this comment.
This file implements a manual database migration by executing raw SQL. This approach is brittle and not easily scalable. For instance, it relies on string matching in an error message to check if a column exists, which can break with different database versions or drivers.
For a more robust and maintainable solution, consider using a database migration tool like Alembic, which integrates well with Flask and SQLAlchemy via the Flask-Migrate extension. It allows you to manage schema changes with version-controlled migration scripts.
| <Route | ||
| path="/dashboard" | ||
| element={ | ||
| <ProtectedRoute> | ||
| <Dashboard /> | ||
| </ProtectedRoute> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/upload" | ||
| element={ | ||
| <ProtectedRoute> | ||
| <Upload /> | ||
| </ProtectedRoute> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/gallery" | ||
| element={ | ||
| <ProtectedRoute> | ||
| <Gallery /> | ||
| </ProtectedRoute> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/profile" | ||
| element={ | ||
| <ProtectedRoute> | ||
| <Profile /> | ||
| </ProtectedRoute> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/admin" | ||
| element={ | ||
| <ProtectedRoute> | ||
| <AdminDashboard /> | ||
| </ProtectedRoute> | ||
| } | ||
| /> |
There was a problem hiding this comment.
The ProtectedRoute component is wrapped around each protected route individually. This is repetitive and can be simplified using a layout route, which is a feature of react-router-dom v6. This will make your routing configuration cleaner and easier to maintain.
| <Route | |
| path="/dashboard" | |
| element={ | |
| <ProtectedRoute> | |
| <Dashboard /> | |
| </ProtectedRoute> | |
| } | |
| /> | |
| <Route | |
| path="/upload" | |
| element={ | |
| <ProtectedRoute> | |
| <Upload /> | |
| </ProtectedRoute> | |
| } | |
| /> | |
| <Route | |
| path="/gallery" | |
| element={ | |
| <ProtectedRoute> | |
| <Gallery /> | |
| </ProtectedRoute> | |
| } | |
| /> | |
| <Route | |
| path="/profile" | |
| element={ | |
| <ProtectedRoute> | |
| <Profile /> | |
| </ProtectedRoute> | |
| } | |
| /> | |
| <Route | |
| path="/admin" | |
| element={ | |
| <ProtectedRoute> | |
| <AdminDashboard /> | |
| </ProtectedRoute> | |
| } | |
| /> | |
| <Route element={<ProtectedRoute />}> | |
| <Route path="/dashboard" element={<Dashboard />} /> | |
| <Route path="/upload" element={<Upload />} /> | |
| <Route path="/gallery" element={<Gallery />} /> | |
| <Route path="/profile" element={<Profile />} /> | |
| <Route path="/admin" element={<AdminDashboard />} /> | |
| </Route> | |
| import axios from 'axios'; | ||
|
|
||
| const api = axios.create({ | ||
| baseURL: 'http://localhost:5000', |
There was a problem hiding this comment.
The baseURL for the API is hardcoded. This makes it difficult to switch between development and production environments. It's better to use environment variables for this configuration. In a Vite project, you can create a .env file and access variables prefixed with VITE_.
baseURL: import.meta.env.VITE_API_BASE_URL || 'http://localhost:5000',| const selectedFile = e.target.files[0]; | ||
| if (selectedFile) { | ||
| setFile(selectedFile); | ||
| setPreview(URL.createObjectURL(selectedFile)); |
There was a problem hiding this comment.
URL.createObjectURL() creates a reference to a file that stays in memory until the document is unloaded. In a single-page application, this can lead to memory leaks if not handled properly. You should revoke the object URL when the component unmounts or when a new file is selected to free up memory. You can do this using URL.revokeObjectURL() in a useEffect cleanup function.
| await api.post('/images/upload', formData, { | ||
| headers: { | ||
| 'Content-Type': 'multipart/form-data', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
You are manually setting the Content-Type header to multipart/form-data. This is unnecessary and can cause issues, because the browser needs to add the boundary parameter to this header, which your manual setting prevents. When you pass a FormData object to Axios, it correctly sets the Content-Type header automatically.
await api.post('/images/upload', formData);There was a problem hiding this comment.
Pull request overview
This PR introduces a complete BHV (Behavioral Health Vault) prototype implementing a mental wellness journal application with image uploads, user authentication, and admin capabilities.
Changes:
- Complete React frontend with authentication, image upload, gallery, and admin dashboard
- Flask REST API backend with JWT authentication, SQLite database, and file storage
- User management system with admin privileges and role-based access control
Reviewed changes
Copilot reviewed 36 out of 40 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/app.py | Flask application initialization with CORS and blueprint registration |
| backend/models.py | SQLAlchemy models for User and Image entities |
| backend/routes/auth.py | Authentication endpoints for signup/login with JWT tokens |
| backend/routes/images.py | Image upload and retrieval endpoints |
| backend/routes/admin.py | Admin-only endpoints for user/image management |
| backend/middleware.py | JWT token validation decorator |
| backend/config.py | Application configuration with database and upload settings |
| frontend/src/pages/* | React pages for landing, auth, dashboard, upload, gallery, profile, admin |
| frontend/src/components/* | Reusable Button, Input, Navbar, and ProtectedRoute components |
| frontend/src/api/client.js | Axios HTTP client with JWT token interceptor |
| frontend/package.json | Frontend dependencies including React 19, Tailwind CSS, Vite |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const api = axios.create({ | ||
| baseURL: 'http://localhost:5000', |
There was a problem hiding this comment.
Hardcoded localhost URL will break in production. The base URL should be configurable through environment variables (e.g., import.meta.env.VITE_API_URL).
| const api = axios.create({ | |
| baseURL: 'http://localhost:5000', | |
| const API_BASE_URL = | |
| typeof import.meta !== 'undefined' && | |
| import.meta.env && | |
| import.meta.env.VITE_API_URL | |
| ? import.meta.env.VITE_API_URL | |
| : 'http://localhost:5000'; | |
| const api = axios.create({ | |
| baseURL: API_BASE_URL, |
| <div className="flex-shrink-0 relative"> | ||
| <img | ||
| className="h-48 w-full object-cover" | ||
| src={`http://localhost:5000/images/file/${image.filename}`} |
There was a problem hiding this comment.
Hardcoded localhost URLs in image sources will fail in production. These should use the same configurable API base URL as the client.
| <div className="flex-shrink-0 relative"> | ||
| <img | ||
| className="h-64 w-full object-cover" | ||
| src={`http://localhost:5000/images/file/${image.filename}`} |
There was a problem hiding this comment.
Hardcoded localhost URL in image source. This should use a configurable API base URL.
| class Config: | ||
| SECRET_KEY = os.environ.get('SECRET_KEY') or 'dev-secret-key-change-in-prod' |
There was a problem hiding this comment.
Weak default secret key is a critical security vulnerability. In production, this default should never be used. Consider failing fast if SECRET_KEY is not explicitly set in production environments.
| class Config: | |
| SECRET_KEY = os.environ.get('SECRET_KEY') or 'dev-secret-key-change-in-prod' | |
| _SECRET_KEY = os.environ.get('SECRET_KEY') | |
| if not _SECRET_KEY: | |
| # In production, SECRET_KEY must be explicitly set and must not fall back to a default value. | |
| if os.environ.get('FLASK_ENV') == 'production' or os.environ.get('ENV') == 'production': | |
| raise RuntimeError("SECRET_KEY environment variable must be set in production") | |
| _SECRET_KEY = 'dev-secret-key-change-in-prod' | |
| class Config: | |
| SECRET_KEY = _SECRET_KEY |
| @admin_bp.route('/images/<int:image_id>', methods=['DELETE']) | ||
| @admin_required | ||
| def delete_image(current_user, image_id): | ||
| image = Image.query.get(image_id) | ||
| if not image: | ||
| return jsonify({'message': 'Image not found'}), 404 | ||
|
|
||
| try: | ||
| db.session.delete(image) | ||
| db.session.commit() | ||
| return jsonify({'message': 'Image deleted successfully'}), 200 | ||
| except Exception as e: | ||
| db.session.rollback() | ||
| return jsonify({'message': 'Error deleting image', 'error': str(e)}), 500 |
There was a problem hiding this comment.
Deleted image files are not removed from the filesystem. When an image is deleted from the database, the corresponding file in the upload folder should also be deleted to prevent orphaned files from accumulating.
| import { useAuth } from '../context/AuthContext'; | ||
| import { Button } from '../components/Button'; | ||
| import { Input } from '../components/Input'; | ||
| import { Trash2, Shield, Users, Image as ImageIcon, Search } from 'lucide-react'; |
There was a problem hiding this comment.
Unused import Shield.
| const [stats, setStats] = useState({ user_count: 0, image_count: 0 }); | ||
| const [users, setUsers] = useState([]); | ||
| const [activeTab, setActiveTab] = useState('stats'); | ||
| const [loading, setLoading] = useState(true); |
There was a problem hiding this comment.
Unused variable loading.
| import { Link } from 'react-router-dom'; | ||
| import { Button } from '../components/Button'; | ||
| import { Input } from '../components/Input'; | ||
| import { Plus, Calendar, Clock, Search, Trash2 } from 'lucide-react'; |
There was a problem hiding this comment.
Unused import Clock.
| import { useState, useRef } from 'react'; | ||
| import { useNavigate } from 'react-router-dom'; | ||
| import { Button } from '../components/Button'; | ||
| import { Upload as UploadIcon, X, Image as ImageIcon } from 'lucide-react'; |
There was a problem hiding this comment.
Unused import UploadIcon.
| const [preview, setPreview] = useState(null); | ||
| const [description, setDescription] = useState(''); | ||
| const [loading, setLoading] = useState(false); | ||
| const [error, setError] = useState(''); |
There was a problem hiding this comment.
Unused variable error.
2 participants
This PR introduces a complete, working BHV prototype to demonstrate the intended system flow and architecture.
What this prototype includes
This prototype demonstrates the core BHV experience:
The system is intentionally kept minimal to reflect the “simple and stupid architecture that works” philosophy discussed in the BHV threads.
Feedback welcome
I’d appreciate feedback on:
This is a proof-of-concept prototype created for architectural exploration.