fix: seeder idempotent with firstOrCreate
Use firstOrCreate instead of create so db:seed can run safely on container restart without duplicate key violation.
This commit is contained in:
root
312
AUDIT_REPORT.md
Normal file
312
AUDIT_REPORT.md
Normal file
@@ -0,0 +1,312 @@
|
||||
# AUDIT REPORT — SIDES (tckdev)
|
||||
|
||||
**Date**: 2026-05-20
|
||||
**Auditor**: Automated Code Review
|
||||
**Scope**: Full brownfield codebase audit
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
SIDES is a flood early warning system built on Laravel 12 + PostgreSQL + Docker. The application is **functional for its purpose** but has significant security vulnerabilities, code quality issues, and architectural concerns that should be addressed before any further development or production deployment.
|
||||
|
||||
**Risk Level: HIGH**
|
||||
|
||||
---
|
||||
|
||||
## 1. CRITICAL — Security Vulnerabilities
|
||||
|
||||
### 1.1 SQL Injection (HIGH)
|
||||
|
||||
**Multiple controllers** build raw SQL queries with string interpolation instead of parameterized queries.
|
||||
|
||||
**`RainfallController::index()` (line 46):**
|
||||
```php
|
||||
$stationCondition = " AND s.stationid = '{$stationFilter}'";
|
||||
```
|
||||
|
||||
**`WaterLevelController::index()` (lines 30, 37):**
|
||||
```php
|
||||
$stationCondition = " WHERE s.stationid = '{$stationFilter}' ";
|
||||
$dateCondition = " AND w.datetime = '{$sqlDate}' ";
|
||||
```
|
||||
|
||||
**`RainfallController::index()` (lines 55-106):**
|
||||
```php
|
||||
$rainfallData = collect(DB::select("
|
||||
SELECT ... CAST('$displayDate' AS timestamp) ...
|
||||
... CAST('$dateFilter' as date) - INTERVAL '6 days' ...
|
||||
$stationCondition
|
||||
"));
|
||||
```
|
||||
|
||||
User input (`$stationFilter`, `$dateFilter`, `$displayDate`, `$sqlDate`) is interpolated directly into SQL strings. An attacker could inject SQL through these parameters.
|
||||
|
||||
**Impact**: Full database compromise (read, modify, delete data)
|
||||
**Files affected**: `RainfallController.php`, `WaterLevelController.php`
|
||||
**Fix**: Use parameterized queries with `?` placeholders or named bindings (`:param`) consistently.
|
||||
|
||||
### 1.2 Hardcoded Credentials in Source Code (HIGH)
|
||||
|
||||
**`autoscript/sidesdecode.py` (lines 22-31):**
|
||||
```python
|
||||
ftp_server = "myvscada.com"
|
||||
ftp_username = "tck"
|
||||
ftp_password = "tck6789"
|
||||
pg_host = "192.168.0.211"
|
||||
pg_database = "sides_db"
|
||||
pg_user = "tck"
|
||||
pg_password = "projectdev##1"
|
||||
```
|
||||
|
||||
**`src/.env` (committed to git):**
|
||||
```
|
||||
MAIL_PASSWORD="ipmu zifw bpmf fsyp"
|
||||
POSTGRES_PASSWORD="projectdev##1"
|
||||
PGADMIN_PASSWORD="projectdev##1"
|
||||
```
|
||||
|
||||
**`src/storage/app/firebase/sides-b4abb-3604a7cf7584.json`** — Firebase service account key is committed to the repository.
|
||||
|
||||
**Impact**: Anyone with repository access has all credentials
|
||||
**Fix**: Move to environment variables, add `.env` to `.gitignore`, rotate all exposed credentials
|
||||
|
||||
### 1.3 Default Admin Credentials (HIGH)
|
||||
|
||||
**`DatabaseSeeder.php` and migration `2025_12_11_124201`:**
|
||||
```php
|
||||
'name' => 'admin',
|
||||
'password' => Hash::make('password123'),
|
||||
```
|
||||
|
||||
**Impact**: Anyone who discovers this gets admin access
|
||||
**Fix**: Remove hardcoded credentials, require admin setup on first run, use strong passwords
|
||||
|
||||
### 1.4 API Endpoints Unauthenticated (MEDIUM)
|
||||
|
||||
All API endpoints except `/api/login` have **no authentication**:
|
||||
```php
|
||||
Route::get('/station/current', [StationController::class, 'getCurrentData']);
|
||||
Route::get('/station/rainfall', [StationController::class, 'getRainfallData']);
|
||||
// ... all publicly accessible
|
||||
```
|
||||
|
||||
**`/api/alert`** is publicly accessible — anyone can trigger push notifications:
|
||||
```php
|
||||
Route::post('/alert',[AlertController::class,'send']);
|
||||
```
|
||||
|
||||
**Impact**: Data exfiltration, unauthorized push notification spam
|
||||
**Fix**: Add API authentication (sanctum/passport tokens) to all endpoints
|
||||
|
||||
### 1.5 API Login Returns Password Hash (LOW)
|
||||
|
||||
**`Api\AuthController::login()` (line 23):**
|
||||
```php
|
||||
$user = DB::select("SELECT id, name, email, access_level, password FROM users ...");
|
||||
```
|
||||
|
||||
While the full `$user` object isn't returned, the query fetches the password hash unnecessarily.
|
||||
|
||||
**Fix**: Remove `password` from the SELECT query.
|
||||
|
||||
---
|
||||
|
||||
## 2. HIGH — Code Quality Issues
|
||||
|
||||
### 2.1 No Eloquent ORM Usage
|
||||
|
||||
The entire application uses `DB::table()` and `DB::select()` (query builder / raw SQL) instead of Eloquent models. Only the `User` model exists.
|
||||
|
||||
**Impact**:
|
||||
- No type safety, no attribute casting, no relationship definitions
|
||||
- Difficulty maintaining and extending the codebase
|
||||
- No model-level validation or events
|
||||
|
||||
**Fix**: Create Eloquent models for `Station`, `Rainfall`, `WaterLevel`, `Siren`, `Notification` with proper relationships.
|
||||
|
||||
### 2.2 No Database Foreign Keys
|
||||
|
||||
All table relationships (`stationid` references) are maintained at the application level with no database constraints:
|
||||
|
||||
```php
|
||||
// Migration: no foreign key
|
||||
$table->string('stationid');
|
||||
// vs what it should be:
|
||||
$table->foreign('stationid')->references('stationid')->on('station');
|
||||
```
|
||||
|
||||
**Impact**: Data integrity cannot be guaranteed at the database level. Orphaned records possible.
|
||||
|
||||
### 2.3 Missing Database Indexes
|
||||
|
||||
No indexes exist on frequently queried columns:
|
||||
- `rainfall.stationid` — used in every rainfall query
|
||||
- `rainfall.timestamp` — used for date filtering and max() queries
|
||||
- `waterlevel.stationid` — used in every water level query
|
||||
- `waterlevel.datetime` — used for date filtering
|
||||
- `siren.stationid` — used in every siren query
|
||||
- `siren.active_time` — used for max() and ordering
|
||||
- `notification.stationid` — used in every notification query
|
||||
- `notification.timestamp` — used for date filtering
|
||||
|
||||
**Impact**: Query performance degrades significantly as data grows
|
||||
**Fix**: Add composite indexes on `(stationid, timestamp)` for data tables
|
||||
|
||||
### 2.4 Duplicate Admin User Creation
|
||||
|
||||
Admin user is created in **both** the DatabaseSeeder AND a migration:
|
||||
|
||||
- `DatabaseSeeder.php` line 22: `User::create(['name' => 'Admin', ...])`
|
||||
- Migration `2025_12_11_124201`: `DB::table('users')->insert(['name' => 'admin', ...])`
|
||||
|
||||
**Impact**: Running `migrate:fresh --seed` will create two admin users with different names ("Admin" and "admin")
|
||||
**Fix**: Remove one — keep only the seeder.
|
||||
|
||||
### 2.5 Inconsistent Naming Conventions
|
||||
|
||||
| Issue | Example |
|
||||
|-------|---------|
|
||||
| Case inconsistency | `cctvController` (lowercase), `SirenHistory` (PascalCase) |
|
||||
| Mixed route naming | `sirenhistory` vs `historicalrf` vs `historicalwl` |
|
||||
| Mixed table naming | `waterlevel.datetime` vs `rainfall.timestamp` (same concept, different column names) |
|
||||
| Variable typos | `$dateTImeFilter` (capital I), `$dateFilter` |
|
||||
|
||||
### 2.6 No Input Validation on Some Endpoints
|
||||
|
||||
`MapController::getStations()` and `MapController::getCurrentData()` accept no parameters but other controllers accept GET/POST parameters with inconsistent validation.
|
||||
|
||||
### 2.7 Commented-Out Code Blocks
|
||||
|
||||
Multiple large blocks of commented-out code throughout:
|
||||
- `RainfallController.php` lines 250-263 (alternative graph query)
|
||||
- `WaterLevelController.php` date conditions
|
||||
- `sidesdecode.py` file management functions
|
||||
|
||||
---
|
||||
|
||||
## 3. MEDIUM — Architectural Concerns
|
||||
|
||||
### 3.1 N+1 Query Pattern in MapController
|
||||
|
||||
`MapController::getCurrentData()` uses subqueries in JOINs for latest data:
|
||||
|
||||
```php
|
||||
->whereRaw('r.timestamp = (SELECT MAX(timestamp) FROM rainfall WHERE stationid = s.stationid)')
|
||||
```
|
||||
|
||||
This executes a subquery for every station for every table join (3 subqueries per station). Should use a CTE or window function.
|
||||
|
||||
### 3.2 No Caching
|
||||
|
||||
Real-time data queries (dashboard, map, station listings) run complex SQL on every page load with no caching layer despite data only updating when the Python script runs.
|
||||
|
||||
### 3.3 No Form Request Classes
|
||||
|
||||
Validation is done inline in controllers despite Laravel's Form Request feature. Only `LoginRequest` and `ProfileUpdateRequest` exist from Breeze scaffolding.
|
||||
|
||||
### 3.4 No API Token Authentication
|
||||
|
||||
The API login endpoint validates credentials but returns no token. This makes the API unusable for stateless/mobile clients that need persistent authentication.
|
||||
|
||||
### 3.5 FCM Topic Fixed
|
||||
|
||||
`AlertController::send()` always sends to `FCM_TOPIC_RAINFALL_WARNING` regardless of alert type:
|
||||
|
||||
```php
|
||||
$topic = env('FCM_TOPIC_RAINFALL_WARNING');
|
||||
```
|
||||
|
||||
Water level and siren alerts go to the rainfall topic, not their respective topics.
|
||||
|
||||
### 3.6 Water Level Alert Bug
|
||||
|
||||
**`sidesdecode.py` line 378:**
|
||||
```python
|
||||
send_alert_to_laravel(station_id, level, 1) # stationtype=1 (rainfall) instead of 2 (water level)
|
||||
```
|
||||
|
||||
Water level threshold alerts are sent with `stationtype=1` instead of `stationtype=2`, causing them to be classified as rainfall alerts.
|
||||
|
||||
### 3.7 `is_blocked` Not Enforced
|
||||
|
||||
The `is_blocked` column exists on users but no middleware checks it. Blocked users can still log in and use the system.
|
||||
|
||||
### 3.8 No Rate Limiting
|
||||
|
||||
No rate limiting on login, API endpoints, or form submissions.
|
||||
|
||||
---
|
||||
|
||||
## 4. LOW — Minor Issues
|
||||
|
||||
### 4.1 `APP_DEBUG=true` in `.env`
|
||||
|
||||
Debug mode should be `false` in any non-local environment.
|
||||
|
||||
### 4.2 `database.db` and `database.sqlite` Committed
|
||||
|
||||
SQLite database files are in the repository root and `src/database/`.
|
||||
|
||||
### 4.3 Unused Packages
|
||||
|
||||
- `laravel/sail` — installed but Docker Compose is used directly
|
||||
- `laravel/pail` — installed but no log streaming is used
|
||||
- `nunomaduro/collision` — dev dependency, standard
|
||||
|
||||
### 4.4 `.env` Committed
|
||||
|
||||
Both `.env` files (root and `src/`) are committed to the repository. They should be in `.gitignore`.
|
||||
|
||||
### 4.5 `.editorconfig` Duplication
|
||||
|
||||
`.editorconfig` exists in both root and `src/` directories.
|
||||
|
||||
### 4.6 No `.gitignore` for Autoscript Logs
|
||||
|
||||
`autoscript/sidesdecode.log` and `sidesdecode_error.log` are tracked in git.
|
||||
|
||||
### 4.7 Mixed CSS Frameworks
|
||||
|
||||
Both Tailwind CSS and Bootstrap 5 are loaded on pages, creating potential style conflicts and unnecessary bundle size.
|
||||
|
||||
### 4.8 No Tests for Domain Logic
|
||||
|
||||
Only Breeze default tests exist (`AuthenticationTest`, `EmailVerificationTest`, etc.). No tests for domain controllers, data pipeline, or export logic.
|
||||
|
||||
---
|
||||
|
||||
## 5. Summary Table
|
||||
|
||||
| # | Issue | Severity | File(s) | Effort |
|
||||
|---|-------|----------|---------|--------|
|
||||
| 1.1 | SQL Injection | CRITICAL | RainfallController, WaterLevelController | Medium |
|
||||
| 1.2 | Hardcoded credentials | CRITICAL | sidesdecode.py, .env | Low |
|
||||
| 1.3 | Default admin password | CRITICAL | DatabaseSeeder, migration | Low |
|
||||
| 1.4 | Unauthenticated API | HIGH | routes/api.php | Medium |
|
||||
| 1.5 | Password hash in query | LOW | AuthController | Low |
|
||||
| 2.1 | No Eloquent models | HIGH | All controllers | High |
|
||||
| 2.2 | No foreign keys | HIGH | All migrations | Medium |
|
||||
| 2.3 | Missing indexes | HIGH | All data tables | Medium |
|
||||
| 2.4 | Duplicate admin creation | MEDIUM | Seeder + migration | Low |
|
||||
| 2.5 | Inconsistent naming | LOW | Throughout | Medium |
|
||||
| 3.1 | N+1 query pattern | MEDIUM | MapController | Medium |
|
||||
| 3.2 | No caching | MEDIUM | Throughout | Medium |
|
||||
| 3.5 | FCM topic hardcoded | MEDIUM | AlertController | Low |
|
||||
| 3.6 | Wrong stationtype for WL | HIGH | sidesdecode.py:378 | Low |
|
||||
| 3.7 | Blocked users not enforced | MEDIUM | Auth flow | Low |
|
||||
| 3.8 | No rate limiting | MEDIUM | Throughout | Low |
|
||||
| 4.1 | Debug mode on | LOW | .env | Low |
|
||||
| 4.2 | DB files committed | LOW | Root | Low |
|
||||
| 4.4 | .env committed | LOW | Root | Low |
|
||||
| 4.7 | Mixed CSS frameworks | LOW | Blade templates | Medium |
|
||||
| 4.8 | No domain tests | LOW | tests/ | High |
|
||||
|
||||
---
|
||||
|
||||
## 6. Recommended Priority
|
||||
|
||||
1. **Immediate** (before any further changes): Fix SQL injection, rotate credentials, remove `.env` from git
|
||||
2. **Short-term**: Fix water level stationtype bug, add API auth, add database indexes, fix FCM topic routing
|
||||
3. **Medium-term**: Create Eloquent models, add foreign keys, implement caching, add input validation
|
||||
4. **Long-term**: Full test coverage, clean up naming conventions, choose single CSS framework
|
||||
Reference in New Issue
Block a user