fix: resolve logical errors, bugs, and security issues across codebase

- Fix is_admin() method shadowing is_admin database column in User model
- Fix check_password() crash when password_hash is None (OAuth-only users)
- Fix SystemSetting.all_settings() formatting error (missing newline)
- Fix MAIL_PORT returning string instead of int in config
- Fix AUTOMATION_TOKEN config formatting (missing newline before comment)
- Fix path traversal vulnerability in serve_user_audio using realpath validation
- Fix weak auth in process.py, replace session check with @login_required
- Fix int() crash on non-numeric priority in import_songs.py
- Add timeout to SMTP connection in email_helper.py
- Add timeouts to external API requests in metadata.py and spotify_helper.py
- Fix security tests to properly reload config module
- Fix metadata test mock data key mismatch (preview_url -> spotify_preview_url)
- Add skip decorator to integration test requiring live API credentials

Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>

musicround/config.py

@@ -62,7 +62,7 @@ class Config:
     DROPBOX_REDIRECT_URI = os.getenv("DROPBOX_REDIRECT_URI", "http://localhost:5000/users/dropbox/callback")
     
     MAIL_HOST = os.getenv("MAIL_HOST", "localhost")
-    MAIL_PORT = os.getenv("MAIL_PORT", 25)
+    MAIL_PORT = int(os.getenv("MAIL_PORT", "25"))
     MAIL_USE_TLS = os.getenv("MAIL_USE_TLS", "False") == "True"
     MAIL_USE_SSL = os.getenv("MAIL_USE_SSL", "False") == "True"
     MAIL_USERNAME = os.getenv("MAIL_USERNAME", "")
@@ -73,7 +73,9 @@ class Config:
     # Automation settings
     AUTOMATION_TOKEN = os.getenv("AUTOMATION_TOKEN")
     if not AUTOMATION_TOKEN:
-        raise ValueError("AUTOMATION_TOKEN environment variable must be set. Generate a secure token with: python -c 'import secrets; print(secrets.token_urlsafe(32))'")    # Reverse proxy settings
+        raise ValueError("AUTOMATION_TOKEN environment variable must be set. Generate a secure token with: python -c 'import secrets; print(secrets.token_urlsafe(32))'")
+
+    # Reverse proxy settings
     USE_HTTPS = os.getenv("USE_HTTPS", "False") == "True"  # Force HTTPS URL generation
     PREFERRED_URL_SCHEME = os.getenv("PREFERRED_URL_SCHEME", 'https' if USE_HTTPS else 'http')
     

musicround/helpers/email_helper.py

@@ -82,7 +82,7 @@ def send_email(recipient, subject, body_text, attachments=None):
 
     try:
         current_app.logger.info(f"Attempting to send email to {recipient} via {mail_host}:{mail_port}")
-        with smtplib.SMTP(mail_host, mail_port) as server:
+        with smtplib.SMTP(mail_host, mail_port, timeout=30) as server:
             server.starttls()
             current_app.logger.debug("STARTTLS established")
             server.login(mail_username, mail_password)

musicround/helpers/metadata.py

@@ -504,7 +504,7 @@ def get_deezer_data(isrc, app=None):
         
         if not deezer_client:
             # If no client in app context, make direct API call
-            response = requests.get(f"https://api.deezer.com/track/isrc:{isrc}")
+            response = requests.get(f"https://api.deezer.com/track/isrc:{isrc}", timeout=10)
             if response.status_code == 200:
                 track = response.json()
             else:
@@ -539,7 +539,7 @@ def get_deezer_data(isrc, app=None):
                 if deezer_client:
                     album = deezer_client.get_album(album_id)
                 else:
-                    album_response = requests.get(f"https://api.deezer.com/album/{album_id}")
+                    album_response = requests.get(f"https://api.deezer.com/album/{album_id}", timeout=10)
                     album = album_response.json() if album_response.status_code == 200 else None
                 
                 if album and not album.get('error'):
@@ -591,7 +591,7 @@ def get_lastfm_data(artist_name, track_title, app=None):
             'format': 'json'
         }
         
-        response = requests.get(url=url, params=params)
+        response = requests.get(url=url, params=params, timeout=10)
         if response.status_code == 200:
             data = response.json()
             
@@ -753,7 +753,7 @@ def get_acrcloud_data(isrc, app=None):
             'include_works': 1  # Include additional work metadata
         }
         
-        response = requests.get(url, headers=headers, params=params)
+        response = requests.get(url, headers=headers, params=params, timeout=10)
         if response.status_code != 200:
             app.logger.warning(f"ACRCloud API error: {response.status_code} - {response.text}")
             return result

musicround/helpers/spotify_helper.py

@@ -26,7 +26,7 @@ def refresh_spotify_token(refresh_token):
         'client_secret': client_secret
     }
     
-    response = requests.post('https://accounts.spotify.com/api/token', data=data)
+    response = requests.post('https://accounts.spotify.com/api/token', data=data, timeout=10)
     
     if response.status_code == 200:
         return response.json()
@@ -181,7 +181,7 @@ def get_spotify_user_info(access_token):
     }
     
     try:
-        response = requests.get('https://api.spotify.com/v1/me', headers=headers)
+        response = requests.get('https://api.spotify.com/v1/me', headers=headers, timeout=10)
         
         if response.status_code == 200:
             return response.json()

musicround/models.py

@@ -100,6 +100,8 @@ class User(db.Model, UserMixin):
 
     def check_password(self, password):
         """Check if provided password matches the hash"""
+        if not self.password_hash:
+            return False
         return check_password_hash(self.password_hash, password)
         
     def set_token(self):
@@ -111,8 +113,8 @@ class User(db.Model, UserMixin):
         """Check if user has a specific role"""
         return any(role.name == role_name for role in self.roles)
 
-    def is_admin(self):
+    def is_admin_by_role(self):
-        """Check if user is an admin"""
+        """Check if user is an admin via role assignment"""
         return self.has_role('admin')
         
     def __repr__(self):
@@ -323,7 +325,9 @@ class SystemSetting(db.Model):
             db.session.add(setting)
         else:
             setting.value = value
-        db.session.commit()    @staticmethod
+        db.session.commit()
+
+    @staticmethod
     def all_settings():
         return {s.key: s.value for s in SystemSetting.query.all()}
 

musicround/routes/core.py

@@ -302,7 +302,10 @@ def serve_user_audio(filepath):
     """
     Serve user custom audio files from the data directory
     """
-    if '..' in filepath:
+    # Resolve the real path to prevent path traversal attacks
+    base_dir = os.path.realpath('/data')
+    requested_path = os.path.realpath(os.path.join('/data', filepath))
+    if not requested_path.startswith(base_dir + os.sep) and requested_path != base_dir:
         abort(404)
         
     if 'custommp3/' in filepath:

musicround/routes/db_admin.py

@@ -18,7 +18,7 @@ def admin_required(view_func):
         if not current_user.is_authenticated:
             return redirect(url_for('users.login'))
         
-        if not current_user.is_admin():
+        if not current_user.is_admin:
             flash('Admin access required.', 'danger')
             return redirect(url_for('core.index'))
             
@@ -37,7 +37,7 @@ def raw_db_access():
 # Base model view with authentication
 class AuthModelView(ModelView):
     def is_accessible(self):
-        return current_user.is_authenticated and current_user.is_admin()
+        return current_user.is_authenticated and current_user.is_admin
     
     def inaccessible_callback(self, name, **kwargs):
         if not current_user.is_authenticated:

musicround/routes/import_routes.py

@@ -750,7 +750,7 @@ def queue_status():
     Display real-time status of the import queue for administrators
     """
     # Check if user is an admin
-    if not current_user.is_admin():
+    if not current_user.is_admin:
         flash('Admin access required for Import Queue view.', 'danger')
         return redirect(url_for('core.index'))
         

musicround/routes/import_songs.py

@@ -85,7 +85,10 @@ def import_playlist():
             flash("No playlist ID provided for import.", "danger")
             return redirect(request.referrer or url_for('core.search'))
 
-        priority = int(request.form.get('priority', 10))
+        try:
+            priority = int(request.form.get('priority', 10))
+        except (ValueError, TypeError):
+            priority = 10
         queue = current_app.config.get('import_queue')
         if not queue:
             flash("Import queue not initialized.", "danger")

musicround/routes/process.py

@@ -1,16 +1,15 @@
 from flask import Blueprint, session, redirect, url_for, jsonify, request, current_app
+from flask_login import login_required
 import base64
 
 process_bp = Blueprint('process', __name__, url_prefix='/process')
 
 @process_bp.route('/base64', methods=['POST'])
+@login_required
 def base64_encode_data():
     """
     Return base64-encoded string from data provided in request body.
     """
-    if 'access_token' not in session:
-        return redirect(url_for('users.login'))  # Assuming 'users.login' is the correct endpoint
-    
     # Get binary data from request
     data = request.get_data()
     if not data:

musicround/routes/users.py

@@ -61,7 +61,7 @@ def admin_required(f):
     from functools import wraps
     @wraps(f)
     def decorated_function(*args, **kwargs):
-        if not current_user.is_authenticated or not current_user.is_admin():
+        if not current_user.is_authenticated or not current_user.is_admin:
             flash('Admin access required.', 'danger')
             return redirect(url_for('users.profile'))
         return f(*args, **kwargs)
@@ -882,7 +882,7 @@ def use_refresh_token():
 @login_required
 def setup():
     """One-time setup route to promote the current user to admin"""
-    if current_user.is_admin():
+    if current_user.is_admin:
         flash('You are already an administrator.', 'info')
         return redirect(url_for('users.profile'))
     
@@ -1264,7 +1264,7 @@ def create_backup():
     if automation_token == current_app.config.get('AUTOMATION_TOKEN'):
         # Allow the request without authentication for automation
         pass
-    elif not current_user.is_authenticated or not current_user.is_admin():
+    elif not current_user.is_authenticated or not current_user.is_admin:
         return jsonify({"status": "error", "message": "Unauthorized"}), 401
     
     # Get custom backup name if provided

musicround/templates/base.html

@@ -145,7 +145,7 @@
                                 <li>
                                     <a class="text-white hover:text-teal-500" href="{{ url_for('rounds.rounds_list') }}">View Rounds</a>
                                 </li>
-                                {% if current_user.is_admin() %}
+                                {% if current_user.is_admin %}
                                 <li class="group relative">
                                     <button class="peer flex items-center text-white hover:text-teal-500">
                                         <i class="fas fa-shield-alt mr-2"></i>Admin <i class="fas fa-chevron-down ml-1"></i>

musicround/templates/users/profile.html

@@ -38,7 +38,7 @@
                     <div>
                         <p class="text-sm text-gray-600">Admin Status</p>
                         <p class="font-medium">
-                            {% if current_user.is_admin() %}
+                            {% if current_user.is_admin %}
                                 <span class="text-green-600">Administrator</span>
                                 <a href="{{ url_for('admin.index') }}" class="ml-2 text-xs text-blue-600 hover:underline">
                                     Admin Dashboard
@@ -73,7 +73,7 @@
             </div>
             
             <!-- Spotify Connection Debug - Only for Admins -->
-            {% if current_user.is_admin() %}
+            {% if current_user.is_admin %}
             <div class="mt-6 p-4 border border-gray-300 rounded-lg bg-gray-50">
                 <div class="flex items-center justify-between mb-3">
                     <h3 class="text-lg font-semibold text-navy-700">Spotify Connection Debug</h3>
@@ -144,7 +144,7 @@
             <h3 class="text-lg font-semibold mb-4 text-navy-700">Connected Services</h3>
             
             <!-- Spotify Connection - Only shown for admins -->
-            {% if current_user.is_admin() %}
+            {% if current_user.is_admin %}
             <div class="bg-gray-50 p-4 rounded-lg mb-4 shadow-sm">
                 <div class="flex items-center mb-3">
                     <i class="fab fa-spotify text-[#1DB954] text-2xl mr-3"></i>
@@ -269,7 +269,7 @@
 </div>
 
 <!-- Spotify Debug Modal - Only for Admins -->
-{% if current_user.is_admin() %}
+{% if current_user.is_admin %}
 <div id="spotifyDebugModal" class="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50 hidden">
     <div class="bg-white rounded-lg shadow-lg w-full max-w-2xl max-h-[80vh] overflow-y-auto">
         <div class="p-6">
@@ -760,7 +760,7 @@
 {% endblock %}
 
 {% block scripts %}
-{% if current_user.is_admin() %}
+{% if current_user.is_admin %}
 <script>
     document.addEventListener('DOMContentLoaded', function() {
         const modal = document.getElementById('spotifyDebugModal');

tests/test_metadata.py

@@ -3,6 +3,7 @@ import sys
 import os
 import logging
 import dotenv
+import pytest
 from unittest.mock import patch, MagicMock
 
 # Add the project root to Python path for imports
@@ -91,7 +92,7 @@ class TestMetadataHelper(unittest.TestCase):
             "genre": ["Rock", "Hard Rock"],
             "spotify_id": "2zYzyRzz6pRmhPzyfMEC8s",
             "deezer_id": "89077521",
-            "preview_url": "https://audio-ssl.spotify.com/preview/track1234.mp3"
+            "spotify_preview_url": "https://audio-ssl.spotify.com/preview/track1234.mp3"
         }
         
         # Call function with test ISRC
@@ -203,6 +204,7 @@ class TestMetadataHelper(unittest.TestCase):
             self.assertEqual(metadata["year"], "1979")
             self.assertEqual(metadata["genre"], "hard rock")  # First tag from tag-list
 
+    @pytest.mark.skip(reason="Integration test requiring live API credentials. Run manually when needed.")
     def test_real_api_calls(self):
         """
         Test actual API calls with AC/DC's Highway to Hell.

tests/test_security.py

@@ -2,6 +2,7 @@
 import pytest
 import os
 import re
+import importlib
 
 
 class TestSecurityConfiguration:
@@ -14,14 +15,17 @@ class TestSecurityConfiguration:
         if secret_key:
             del os.environ['SECRET_KEY']
         
-        # Import should fail if SECRET_KEY not set
+        try:
-        with pytest.raises(ValueError, match="SECRET_KEY environment variable must be set"):
+            # Force module reload to re-evaluate class-level checks
-            from musicround.config import Config
+            import musicround.config
-            _ = Config.SECRET_KEY
+            with pytest.raises(ValueError, match="SECRET_KEY environment variable must be set"):
-        
+                importlib.reload(musicround.config)
-        # Restore environment
+        finally:
-        if secret_key:
+            # Restore environment
-            os.environ['SECRET_KEY'] = secret_key
+            if secret_key:
+                os.environ['SECRET_KEY'] = secret_key
+                # Reload with correct env so other tests work
+                importlib.reload(musicround.config)
     
     def test_automation_token_required(self):
         """Test that AUTOMATION_TOKEN must be set."""
@@ -30,14 +34,17 @@ class TestSecurityConfiguration:
         if token:
             del os.environ['AUTOMATION_TOKEN']
         
-        # Import should fail if AUTOMATION_TOKEN not set
+        try:
-        with pytest.raises(ValueError, match="AUTOMATION_TOKEN environment variable must be set"):
+            # Force module reload to re-evaluate class-level checks
-            from musicround.config import Config
+            import musicround.config
-            _ = Config.AUTOMATION_TOKEN
+            with pytest.raises(ValueError, match="AUTOMATION_TOKEN environment variable must be set"):
-        
+                importlib.reload(musicround.config)
-        # Restore environment
+        finally:
-        if token:
+            # Restore environment
-            os.environ['AUTOMATION_TOKEN'] = token
+            if token:
+                os.environ['AUTOMATION_TOKEN'] = token
+                # Reload with correct env so other tests work
+                importlib.reload(musicround.config)
     
     def test_no_credentials_in_code(self):
         """Test that no credentials are hardcoded in Python files."""