docs: Add comprehensive settings page analysis and improvements
- Add detailed settings page analysis report (settings.md) - Document identified security vulnerabilities and code quality issues - Provide prioritized improvement recommendations - Document permission and access control issues - Add testing checklist for validation - Track modifications to settings.py, routes.py, and settings.html templates
This commit is contained in:
Quality App System
437
documentation/analysis/settings.md
Normal file
437
documentation/analysis/settings.md
Normal file
@@ -0,0 +1,437 @@
|
||||
# SETTINGS PAGE - COMPREHENSIVE ANALYSIS REPORT
|
||||
|
||||
## 1. PAGE OVERVIEW
|
||||
**Location:** `/settings` route
|
||||
**Route Handler:** `routes.py` (line 319) → `settings.py` (line 199 `settings_handler()`)
|
||||
**Template:** `templates/settings.html` (2852 lines)
|
||||
**Purpose:** Admin/Superadmin configuration hub for user management, database settings, backups, and system maintenance
|
||||
|
||||
---
|
||||
|
||||
## 2. FUNCTIONALITY ANALYSIS
|
||||
|
||||
### Backend Logic (`settings.py` lines 199-250):
|
||||
```
|
||||
Function: settings_handler()
|
||||
- Checks if user is superadmin (only superadmin allowed)
|
||||
- Fetches all users from external MariaDB database
|
||||
- Loads external database configuration from external_server.conf
|
||||
- Converts user data to dictionaries for template rendering
|
||||
```
|
||||
|
||||
### What It Does:
|
||||
The settings page provides 6 major functional areas:
|
||||
|
||||
1. **User Management (Legacy)**
|
||||
- Lists all users from database
|
||||
- Edit/Delete users
|
||||
- Create new users
|
||||
- Shows username, role, email
|
||||
|
||||
2. **Simplified User Management**
|
||||
- Modern 4-tier system (Superadmin → Admin → Manager → Worker)
|
||||
- Module-based permissions (Quality, Warehouse, Labels)
|
||||
- Links to `/user_management_simple` route
|
||||
|
||||
3. **External Server Settings**
|
||||
- Configure database connection details
|
||||
- Server domain/IP, port, database name, username, password
|
||||
- Saves to `external_server.conf`
|
||||
|
||||
4. **Print Extension Management** (Superadmin only)
|
||||
- Manage QZ Tray printer pairing keys
|
||||
- Control direct printing functionality
|
||||
|
||||
5. **Maintenance & Cleanup** (Admin+ only)
|
||||
- **Log File Management**: Auto-delete old log files (7-365 days configurable)
|
||||
- **System Storage Info**: Shows usage for logs, database, backups
|
||||
- **Database Table Management**: Clear/truncate individual tables with caution warnings
|
||||
|
||||
6. **Database Backup Management** (Admin+ only)
|
||||
- **Quick Actions**: Full backup, Data-only backup, Refresh
|
||||
- **Backup Schedules**: Create automated backup schedules (daily/weekly/monthly)
|
||||
- **Per-Table Backup/Restore**: Backup and restore individual tables
|
||||
- **Full Database Restore**: Restore entire database from backup (Superadmin only)
|
||||
|
||||
---
|
||||
|
||||
## 3. FRONTEND STRUCTURE
|
||||
|
||||
### Template Layout (`settings.html`):
|
||||
- **Card-based layout** with multiple collapsible sections
|
||||
- **6 main cards**: User Management, External Server, User & Permissions, Print Extension, Maintenance & Cleanup, Database Backups
|
||||
- **Responsive grid layout** for backup management sections
|
||||
- **Status indicators** showing active/inactive features
|
||||
|
||||
### CSS Styling:
|
||||
- Uses inline CSS styles (heavy reliance on style attributes)
|
||||
- **Color coding**: Green (#4caf50) for safe actions, Orange (#ff9800) for caution, Red (#ff5722) for dangerous operations
|
||||
- **Dark mode support** with CSS variables
|
||||
- **Responsive grid** for desktop and mobile
|
||||
- **Storage stat cards** with gradient backgrounds
|
||||
|
||||
### Features:
|
||||
✅ Toggle-able sections (collapsible backup management)
|
||||
✅ Live storage information display
|
||||
✅ Status messages with color-coded backgrounds
|
||||
✅ Confirmation dialogs for dangerous operations
|
||||
✅ Progress indicators for long-running tasks
|
||||
✅ Caution warnings for data-destructive operations
|
||||
|
||||
---
|
||||
|
||||
## 4. ISSUES & BUGS FOUND
|
||||
|
||||
### 🔴 CRITICAL ISSUES:
|
||||
|
||||
1. **Weak Authorization Check**
|
||||
- **Problem**: `settings_handler()` checks only if `session['role'] == 'superadmin'`
|
||||
- **Line**: `settings.py:200`
|
||||
- **Impact**: Admin users cannot access settings even though some features should be admin-accessible
|
||||
- **Severity**: CRITICAL
|
||||
|
||||
2. **Password Visible in Template**
|
||||
- **Problem**: Password field in External Server Settings is plain text
|
||||
- **Line**: `settings.html:35 <input type="password">`
|
||||
- **Impact**: Password is visible in browser history, cached, logged
|
||||
- **Severity**: HIGH (Security Issue)
|
||||
|
||||
3. **Missing SQL Injection Protection**
|
||||
- **Problem**: Database table names in truncate/backup operations might not be validated
|
||||
- **Impact**: Potential SQL injection if table names come from user input
|
||||
- **Severity**: HIGH
|
||||
|
||||
4. **No CSRF Token Visible**
|
||||
- **Problem**: Form submissions don't show CSRF token verification
|
||||
- **Line**: `settings.html:22 <form method="POST"...>`
|
||||
- **Impact**: Forms vulnerable to CSRF attacks
|
||||
- **Severity**: HIGH
|
||||
|
||||
### 🟡 MODERATE ISSUES:
|
||||
|
||||
5. **Hardcoded Role Check in Template**
|
||||
- **Problem**: Template checks `session.role == 'superadmin'` directly instead of using decorator
|
||||
- **Line**: `settings.html:82, 191, etc.`
|
||||
- **Impact**: Permission logic scattered in template instead of centralized in backend
|
||||
- **Severity**: MEDIUM
|
||||
|
||||
6. **Missing Error Handling in settings_handler()**
|
||||
- **Problem**: No try-catch around entire function, only for database operations
|
||||
- **Impact**: Template errors will crash the page
|
||||
- **Severity**: MEDIUM
|
||||
|
||||
7. **Connection Not Properly Closed**
|
||||
- **Problem**: `conn.close()` called after cursor operations but exceptions might leak connections
|
||||
- **Line**: `settings.py:243`
|
||||
- **Impact**: Database connection pool exhaustion over time
|
||||
- **Severity**: MEDIUM
|
||||
|
||||
8. **Inline CSS Over-usage**
|
||||
- **Problem**: 2852 line template with 90% inline styles
|
||||
- **Impact**: Hard to maintain, slow to load, inconsistent styling, large file size
|
||||
- **Severity**: MEDIUM
|
||||
|
||||
9. **No Input Validation in Form**
|
||||
- **Problem**: External server settings form doesn't validate port number format or server connectivity before saving
|
||||
- **Impact**: Bad configuration saved, app breaks on next restart
|
||||
- **Severity**: MEDIUM
|
||||
|
||||
### 🟢 MINOR ISSUES:
|
||||
|
||||
10. **Inconsistent Column Names**
|
||||
- **Problem**: Some user queries select 'modules' column but it might not exist on all user rows
|
||||
- **Line**: `settings.py:224`
|
||||
- **Impact**: None if column exists, but code assumes it does
|
||||
- **Severity**: LOW
|
||||
|
||||
11. **Magic Strings**
|
||||
- **Problem**: Database table names, role names, module names hardcoded throughout
|
||||
- **Impact**: Hard to refactor, duplicate code
|
||||
- **Severity**: LOW
|
||||
|
||||
12. **Dead Code in Deprecated Function**
|
||||
- **Problem**: `get_external_db_connection()` marked deprecated but still used
|
||||
- **Line**: `settings.py:254`
|
||||
- **Impact**: Confusing for maintainers
|
||||
- **Severity**: LOW
|
||||
|
||||
13. **Print Statement Logging**
|
||||
- **Problem**: Uses `print()` instead of proper logger
|
||||
- **Impact**: Not captured in logging system
|
||||
- **Severity**: LOW
|
||||
|
||||
14. **No Loading States**
|
||||
- **Problem**: Long operations (backups, restores) might appear frozen
|
||||
- **Impact**: Users might click buttons multiple times
|
||||
- **Severity**: LOW
|
||||
|
||||
---
|
||||
|
||||
## 5. CODE QUALITY ASSESSMENT
|
||||
|
||||
### Strengths:
|
||||
✅ Comprehensive feature set
|
||||
✅ Good UI/UX with status indicators
|
||||
✅ Caution warnings for dangerous operations
|
||||
✅ Separate "Legacy" vs "Simplified" user management
|
||||
✅ Supports dark mode
|
||||
✅ Responsive design
|
||||
✅ Detailed backup management capabilities
|
||||
|
||||
### Weaknesses:
|
||||
❌ Critical authorization issues
|
||||
❌ Security vulnerabilities (CSRF, SQL injection risks)
|
||||
❌ Massive template file with inline styles
|
||||
❌ Weak error handling
|
||||
❌ Mixed permissions logic (template + backend)
|
||||
❌ Poor code organization
|
||||
❌ Connection pool management issues
|
||||
❌ No input validation
|
||||
|
||||
---
|
||||
|
||||
## 6. PERMISSIONS & ACCESS CONTROL
|
||||
|
||||
### Current Implementation:
|
||||
```
|
||||
settings_handler() → superadmin only → shows ALL features
|
||||
template → checks session['role'] == 'superadmin' for some sections
|
||||
```
|
||||
|
||||
### Issues:
|
||||
- **Admin users cannot access** even though some features are admin-appropriate
|
||||
- **Backup management** should be available to admins
|
||||
- **Log cleanup** should be available to admins
|
||||
- **User management** should be restricted to admin+ (currently superadmin only)
|
||||
|
||||
### Recommended Roles:
|
||||
- **Superadmin**: Full access (everything)
|
||||
- **Admin**: User management, settings updates, backups, cleanup (everything except pairing keys)
|
||||
- **Manager/Worker**: No access
|
||||
|
||||
---
|
||||
|
||||
## 7. DATABASE OPERATIONS ANALYSIS
|
||||
|
||||
### Tables Accessed:
|
||||
1. `users` - Read/write (fetch all users, create, edit, delete)
|
||||
2. `roles` - Possibly read (in user management)
|
||||
3. Application tables (in truncate operations) - Write (truncate/clear)
|
||||
4. Any table in database (backup/restore) - Read/Write
|
||||
|
||||
### Potential Risks:
|
||||
⚠️ Truncating tables without proper backup check
|
||||
⚠️ Restoring database without current backup
|
||||
⚠️ No transaction handling for backup/restore operations
|
||||
⚠️ No verification of backup integrity before restore
|
||||
|
||||
---
|
||||
|
||||
## 8. SECURITY ASSESSMENT
|
||||
|
||||
### VULNERABILITIES FOUND:
|
||||
|
||||
**Critical (Fix Immediately):**
|
||||
1. CSRF Token missing on forms
|
||||
2. Password field plain text in form (visible in browser)
|
||||
3. Authorization only checks superadmin, not generic admin
|
||||
|
||||
**High (Fix Soon):**
|
||||
1. SQL injection risk on table operations
|
||||
2. No input validation on external server settings
|
||||
3. Weak connection handling
|
||||
|
||||
**Medium (Fix Later):**
|
||||
1. Permissions scattered in template
|
||||
2. No rate limiting on dangerous operations (truncate, restore)
|
||||
3. No audit logging for admin actions
|
||||
|
||||
---
|
||||
|
||||
## 9. JAVASCRIPT FUNCTIONALITY CHECK
|
||||
|
||||
The template has heavy JavaScript for:
|
||||
- Backup creation (AJAX call to `/backup_now_btn`)
|
||||
- Log cleanup (AJAX call to `/cleanup_logs_now_btn`)
|
||||
- Table truncation (AJAX call to load and truncate tables)
|
||||
- Storage info refresh
|
||||
- Schedule management (create, edit, delete schedules)
|
||||
- Backup restore operations
|
||||
|
||||
**Concerns:**
|
||||
⚠️ No timeout on long operations
|
||||
⚠️ No progress bars for backups (might appear frozen)
|
||||
⚠️ No confirmation dialogs for dangerous operations (truncate table)
|
||||
⚠️ AJAX calls don't validate authorization client-side
|
||||
|
||||
---
|
||||
|
||||
## 10. FORM SUBMISSIONS
|
||||
|
||||
### Forms Found:
|
||||
1. **External Server Settings** - POST to `/save_external_db`
|
||||
- No CSRF token visible
|
||||
- No input validation
|
||||
- No test connection button
|
||||
|
||||
2. **User Management** (JavaScript-based, not traditional form)
|
||||
3. **Backup Management** (JavaScript/AJAX)
|
||||
4. **Log Cleanup** (AJAX button)
|
||||
|
||||
---
|
||||
|
||||
## SUMMARY TABLE
|
||||
|
||||
| Aspect | Status | Risk Level | Notes |
|
||||
|--------|--------|------------|-------|
|
||||
| Authentication | ✅ Working | Low | Session checks present |
|
||||
| Authorization | ❌ Broken | CRITICAL | Only superadmin allowed |
|
||||
| Error Handling | ⚠️ Partial | Medium | Missing in places |
|
||||
| Input Validation | ❌ Missing | High | No validation on forms |
|
||||
| CSRF Protection | ❌ Missing | High | No tokens visible |
|
||||
| SQL Injection Risk | ⚠️ Possible | High | Table names not validated |
|
||||
| Code Organization | ❌ Poor | Medium | Massive template, inline CSS |
|
||||
| Performance | ⚠️ Okay | Low | Might be slow on backups |
|
||||
| Security | ❌ Weak | CRITICAL | Multiple vulnerabilities |
|
||||
| Maintainability | ❌ Poor | Medium | Hard to modify |
|
||||
|
||||
---
|
||||
|
||||
## 11. SUGGESTED IMPROVEMENTS
|
||||
|
||||
### Priority 1 (CRITICAL - Fix immediately):
|
||||
|
||||
1. **Add CSRF Token to Forms**
|
||||
```html
|
||||
<form method="POST" action="...">
|
||||
{{ csrf_token() }}
|
||||
<!-- form fields -->
|
||||
</form>
|
||||
```
|
||||
|
||||
2. **Fix Authorization Logic**
|
||||
```python
|
||||
@admin_plus # Use decorator instead
|
||||
def settings_handler():
|
||||
# Remove manual superadmin check
|
||||
```
|
||||
|
||||
3. **Validate All Inputs**
|
||||
```python
|
||||
# Validate table names against whitelist
|
||||
ALLOWED_TABLES = ['scan1_orders', 'scanfg_orders', ...]
|
||||
if table_name not in ALLOWED_TABLES:
|
||||
return error("Invalid table")
|
||||
```
|
||||
|
||||
4. **Hash/Obscure Password Field**
|
||||
- Store encrypted in config file
|
||||
- Show masked dots in form
|
||||
- Add "show/hide" toggle
|
||||
|
||||
### Priority 2 (HIGH - Fix soon):
|
||||
|
||||
5. **Refactor to use Decorators**
|
||||
```python
|
||||
@bp.route('/settings')
|
||||
@admin_plus
|
||||
def settings():
|
||||
# All admin checks in decorator
|
||||
```
|
||||
|
||||
6. **Extract CSS to Separate File**
|
||||
- Create `css/settings.css`
|
||||
- Remove all inline styles
|
||||
- Reduce template to ~500 lines
|
||||
|
||||
7. **Add Input Validation**
|
||||
- Validate port is integer (1-65535)
|
||||
- Validate server domain format
|
||||
- Test connection before saving
|
||||
|
||||
8. **Fix Connection Pool**
|
||||
```python
|
||||
try:
|
||||
conn = get_external_db_connection()
|
||||
# operations
|
||||
finally:
|
||||
conn.close() # Ensure closes even on error
|
||||
```
|
||||
|
||||
9. **Add Confirmation Dialogs**
|
||||
- Truncate table warning
|
||||
- Restore database warning
|
||||
- Log cleanup confirmation
|
||||
|
||||
10. **Use Logger Instead of Print**
|
||||
```python
|
||||
logger = get_logger('settings')
|
||||
logger.error(f"Error: {e}")
|
||||
```
|
||||
|
||||
### Priority 3 (MEDIUM - Improve):
|
||||
|
||||
11. **Add Progress Indicators** for long operations
|
||||
12. **Add Operation Timeouts** (prevent infinite hangs)
|
||||
13. **Add Audit Logging** for all admin actions
|
||||
14. **Add Rate Limiting** on dangerous operations
|
||||
15. **Split Template** into multiple files (one per feature)
|
||||
16. **Add Database Connection Test** button
|
||||
17. **Show Last Backup Date/Size** in UI
|
||||
18. **Add Backup Integrity Check** before restore
|
||||
19. **Add Auto-Recovery** for failed backups
|
||||
20. **Implement Admin-Only Pages** (not just superadmin)
|
||||
|
||||
---
|
||||
|
||||
## TESTING CHECKLIST
|
||||
|
||||
Before using this page:
|
||||
|
||||
1. **Security Tests:**
|
||||
- [ ] Try accessing as non-superadmin user (should be denied)
|
||||
- [ ] Check if CSRF token is present in network requests
|
||||
- [ ] Try SQL injection in table name field
|
||||
- [ ] Verify password field is masked
|
||||
|
||||
2. **Functionality Tests:**
|
||||
- [ ] Create new user and verify in database
|
||||
- [ ] Edit user and verify changes saved
|
||||
- [ ] Delete user and verify removed
|
||||
- [ ] Save external server settings and verify file created
|
||||
- [ ] Create backup and verify file exists
|
||||
- [ ] Restore backup and verify data restored
|
||||
- [ ] Truncate table and verify data cleared
|
||||
|
||||
3. **Error Handling Tests:**
|
||||
- [ ] Break database connection, try to load settings
|
||||
- [ ] Provide invalid port number
|
||||
- [ ] Try backup with no disk space
|
||||
- [ ] Truncate table while backup running
|
||||
|
||||
4. **Performance Tests:**
|
||||
- [ ] Load settings with 1000 users
|
||||
- [ ] Create backup with large database (>1GB)
|
||||
- [ ] Check browser memory usage over time
|
||||
|
||||
5. **UI/UX Tests:**
|
||||
- [ ] Test on mobile (responsive)
|
||||
- [ ] Test dark mode toggle
|
||||
- [ ] Test all buttons are clickable
|
||||
- [ ] Verify all status messages appear
|
||||
|
||||
---
|
||||
|
||||
## NEXT STEPS FOR USER REVIEW
|
||||
|
||||
1. **Critical**: Address authorization bug (line 200)
|
||||
2. **Critical**: Add CSRF token to forms
|
||||
3. **High**: Fix password visibility issue
|
||||
4. **High**: Add input validation
|
||||
5. **Medium**: Refactor template structure
|
||||
6. **Medium**: Improve error handling
|
||||
7. **Low**: Migrate to proper logger
|
||||
8. **Low**: Add nice-to-have features
|
||||
|
||||
---
|
||||
|
||||
Reference in New Issue
Block a user