root
303 lines
14 KiB
Markdown
303 lines
14 KiB
Markdown
# Security Fix Report - school_api
|
|
|
|
## Executive Summary
|
|
|
|
- **Assessment Date:** 2026-06-04
|
|
- **Reviewer:** OpenAI GPT-5.5 Thinking
|
|
- **Application:** school_api
|
|
- **Framework Observed:** Laravel 12 (not CodeIgniter 4)
|
|
- **Total Findings:** 12
|
|
- **Fixed Findings:** 9
|
|
- **Open Findings:** 3
|
|
- **Overall Risk Level:** Medium after static fixes; High until real secrets are rotated and dynamic testing/dependency audit are completed
|
|
|
|
The uploaded verification plan was applied by category, but the codebase is Laravel 12 rather than CodeIgniter 4. Fixed items are documented with before/after evidence and verification notes. Dynamic testing and dependency advisory scanning remain open because the sandbox lacks Composer and the PHP mbstring extension required to boot Artisan.
|
|
|
|
## Vulnerability Inventory
|
|
|
|
| ID | Severity | Category | Location | Status |
|
|
|---|---:|---|---|---|
|
|
| SEC-001 | Critical | Secrets / Environment | .env; test.sql | Fixed |
|
|
| SEC-002 | High | CSRF / Session State Change | routes/web.php logout route | Fixed |
|
|
| SEC-003 | High | Brute Force / Abuse Controls | routes/api.php public auth, registration, invite, contact routes | Fixed |
|
|
| SEC-004 | High | File Upload Security | app/Services/Events/EventManagementService.php | Fixed |
|
|
| SEC-005 | High | File Upload Security | app/Services/Expenses/ExpenseReceiptService.php | Fixed |
|
|
| SEC-006 | Medium | File Upload Security | app/Services/BroadcastEmail/BroadcastEmailImageService.php | Fixed |
|
|
| SEC-007 | Medium | File Upload Security | PrintRequestsController; PrintRequestsPortalService | Fixed |
|
|
| SEC-008 | Medium | HTTP Security Headers / Content Sniffing | bootstrap/app.php; app/Http/Middleware/SecurityHeaders.php; FileServeService.php | Fixed |
|
|
| SEC-009 | Medium | SQL Injection Hardening | app/Models/User.php | Fixed |
|
|
| SEC-010 | Medium | Dependency Audit | composer.lock / environment | Open |
|
|
| SEC-011 | Medium | Dynamic Security Testing | Running application / OWASP ZAP | Open |
|
|
| SEC-012 | Low | Framework Scope Accuracy | security-verification-plan.md | Open |
|
|
|
|
## Detailed Remediation Record
|
|
|
|
### SEC-001 - Critical - Secrets / Environment
|
|
|
|
**Finding:** The archive shipped active-looking application, JWT, SMTP, database and PayPal credentials, plus production debug/local settings.
|
|
|
|
**Risk:** Credential exposure can enable account takeover, forged JWTs, SMTP abuse, payment-provider abuse, and easier exploitation through debug traces.
|
|
|
|
**Location:** .env; test.sql
|
|
|
|
**Evidence:** Static secret scan found APP_KEY, JWT_SECRET, Gmail app password, DB root/root, and PayPal client/secret values.
|
|
|
|
**Remediation:** Redacted shipped secrets to placeholders, changed production posture, enabled encrypted/secure session settings, and redacted PayPal credentials in test.sql.
|
|
|
|
**Before:** APP_DEBUG=true; MAIL_DEFAULT_PASS contained a Gmail app password; JWT_SECRET contained a live-looking HMAC secret; test.sql included PayPal secrets.
|
|
|
|
**After:** APP_DEBUG=false; secrets use CHANGE_ME placeholders; SESSION_ENCRYPT=true; SESSION_SECURE_COOKIE=true; PayPal values redacted.
|
|
|
|
**Verification:** Repeated static secret scan. Remaining matches are placeholders or framework config keys; no original Gmail/JWT/PayPal strings remained.
|
|
|
|
**Result:** Fixed in source archive. Real production credentials must still be rotated outside this codebase.
|
|
|
|
### SEC-002 - High - CSRF / Session State Change
|
|
|
|
**Finding:** Browser-session logout used GET, a state-changing action vulnerable to CSRF-style forced logout and link/preload triggering.
|
|
|
|
**Risk:** An attacker could cause authenticated users to lose sessions through an image/link/preload request. Not catastrophic, but still sloppy. The web needed one less foot-gun.
|
|
|
|
**Location:** routes/web.php logout route
|
|
|
|
**Evidence:** Route::get('logout', ...) was registered under the web middleware group.
|
|
|
|
**Remediation:** Changed logout to POST so Laravel web CSRF protection applies.
|
|
|
|
**Before:** Route::get('logout', [AuthSessionController::class, 'logout'])
|
|
|
|
**After:** Route::post('logout', [AuthSessionController::class, 'logout'])
|
|
|
|
**Verification:** PHP lint passed for routes/web.php; diff confirms GET was removed for the logout route.
|
|
|
|
**Result:** Fixed. Frontend logout calls must use POST.
|
|
|
|
### SEC-003 - High - Brute Force / Abuse Controls
|
|
|
|
**Finding:** Several public mutation endpoints had no route-level throttle: login/register aliases, captcha, contact, and authorized-user password setup.
|
|
|
|
**Risk:** Attackers could automate credential stuffing, registration abuse, contact spam, and invite/password token guessing at higher volume than necessary.
|
|
|
|
**Location:** routes/api.php public auth, registration, invite, contact routes
|
|
|
|
**Evidence:** Public POST/GET routes were registered without throttle middleware.
|
|
|
|
**Remediation:** Added route-level throttles: login 10/min, register 5/min, captcha 30/min, invite GET 20/min, password setup POST 10/min, contact 5/min.
|
|
|
|
**Before:** Route::post('login', ...); Route::post('register', ...); Route::post('contact', ...);
|
|
|
|
**After:** Routes now include middleware('throttle:...') on public auth/contact/invite mutation paths.
|
|
|
|
**Verification:** PHP lint passed for routes/api.php; route snippets confirm throttle middleware is attached.
|
|
|
|
**Result:** Fixed statically. Live rate-limit behavior still needs HTTP retesting after deployment.
|
|
|
|
### SEC-004 - High - File Upload Security
|
|
|
|
**Finding:** Event flyer upload moved files into public storage without explicit MIME allowlist or file-size enforcement in the service.
|
|
|
|
**Risk:** Untrusted files could be stored in a web-accessible directory. Extension games are ancient, stupid, and still work when services trust uploads too much.
|
|
|
|
**Location:** app/Services/Events/EventManagementService.php
|
|
|
|
**Evidence:** storeFlyer() used hashName() and move() without validating MIME or size inside the service.
|
|
|
|
**Remediation:** Added service-level MIME allowlist for JPEG, PNG, WEBP, PDF; max 5MB; cryptographically random server-side filename using the validated MIME extension.
|
|
|
|
**Before:** $name = $file->hashName(); $file->move($dir, $name);
|
|
|
|
**After:** MIME map validation, size check, random_bytes filename, controlled extension.
|
|
|
|
**Verification:** PHP lint passed for EventManagementService.php; static diff confirms validation was added before move().
|
|
|
|
**Result:** Fixed.
|
|
|
|
### SEC-005 - High - File Upload Security
|
|
|
|
**Finding:** Receipt upload stored files without service-level validation of validity, MIME type, or size.
|
|
|
|
**Risk:** Malformed or unexpected files could be accepted and later served/downloaded, increasing malware and content-sniffing risk.
|
|
|
|
**Location:** app/Services/Expenses/ExpenseReceiptService.php
|
|
|
|
**Evidence:** storeReceipt() directly called $file->store('receipts') and returned basename.
|
|
|
|
**Remediation:** Added isValid(), MIME allowlist, max 5MB, random server-side filename, and controlled extension.
|
|
|
|
**Before:** $stored = $file->store('receipts');
|
|
|
|
**After:** Validated MIME and size, then storeAs() with bin2hex(random_bytes(16)).
|
|
|
|
**Verification:** PHP lint passed for ExpenseReceiptService.php; static diff confirms checks and randomized naming.
|
|
|
|
**Result:** Fixed.
|
|
|
|
### SEC-006 - Medium - File Upload Security
|
|
|
|
**Finding:** Broadcast email image upload used uniqid() for public filenames and lacked an explicit isValid() check.
|
|
|
|
**Risk:** uniqid() is predictable enough to make public file discovery easier. Tiny risk, but still a classic example of software pretending timestamps are secrets.
|
|
|
|
**Location:** app/Services/BroadcastEmail/BroadcastEmailImageService.php
|
|
|
|
**Evidence:** $newName = uniqid('em_', true) . '.' . $ext; no isValid() guard.
|
|
|
|
**Remediation:** Added isValid() handling and replaced uniqid() with bin2hex(random_bytes(16)).
|
|
|
|
**Before:** uniqid('em_', true)
|
|
|
|
**After:** 'em_' . bin2hex(random_bytes(16))
|
|
|
|
**Verification:** PHP lint passed for BroadcastEmailImageService.php; diff confirms change.
|
|
|
|
**Result:** Fixed.
|
|
|
|
### SEC-007 - Medium - File Upload Security
|
|
|
|
**Finding:** Print request uploads had controller extension/MIME validation but service naming still trusted the client original extension.
|
|
|
|
**Risk:** Client-controlled extensions can create misleading stored files and weaken downstream serving assumptions.
|
|
|
|
**Location:** PrintRequestsController; PrintRequestsPortalService
|
|
|
|
**Evidence:** Portal service used getClientOriginalExtension() to build stored filenames.
|
|
|
|
**Remediation:** Added safeExtensionForUpload() that maps detected MIME to controlled extensions; added mimetypes rules to controller validation.
|
|
|
|
**Before:** $ext = $file->getClientOriginalExtension();
|
|
|
|
**After:** $ext = $this->safeExtensionForUpload($file); plus mimetypes validation.
|
|
|
|
**Verification:** PHP lint passed for controller and service; diff confirms service no longer trusts client extensions.
|
|
|
|
**Result:** Fixed.
|
|
|
|
### SEC-008 - Medium - HTTP Security Headers / Content Sniffing
|
|
|
|
**Finding:** The app lacked a global security header middleware and some uploaded/private file responses were cacheable as public unless nosniff was manually requested.
|
|
|
|
**Risk:** Missing headers increases clickjacking/content-sniffing exposure. Public caching of private uploaded files can leak sensitive school documents through shared caches.
|
|
|
|
**Location:** bootstrap/app.php; app/Http/Middleware/SecurityHeaders.php; FileServeService.php
|
|
|
|
**Evidence:** No global header middleware; FileServeService used Cache-Control public, max-age=86400 and conditional nosniff.
|
|
|
|
**Remediation:** Added global SecurityHeaders middleware; added nosniff, frame denial, referrer policy, permissions policy, HSTS over HTTPS; changed file serving to private max-age=300 and nosniff always.
|
|
|
|
**Before:** Cache-Control: public, max-age=86400; conditional X-Content-Type-Options.
|
|
|
|
**After:** Cache-Control: private, max-age=300; X-Content-Type-Options: nosniff; global headers middleware.
|
|
|
|
**Verification:** PHP lint passed for middleware, bootstrap, and FileServeService; static diff confirms middleware registration.
|
|
|
|
**Result:** Fixed statically. Header behavior should be confirmed by HTTP response tests in deployed environment.
|
|
|
|
### SEC-009 - Medium - SQL Injection Hardening
|
|
|
|
**Finding:** A school_year value was manually quoted and interpolated into a raw EXISTS SQL string.
|
|
|
|
**Risk:** Manual quoting is fragile. It was not obviously exploitable because PDO quote was used, but parameterized query builder logic is safer and auditable.
|
|
|
|
**Location:** app/Models/User.php
|
|
|
|
**Evidence:** $escYear = DB::getPdo()->quote($schoolYear); ... tc.school_year = {$escYear}
|
|
|
|
**Remediation:** Replaced raw interpolation with whereExists(), whereColumn(), and bound where('tc.school_year', $schoolYear).
|
|
|
|
**Before:** tc.school_year = {$escYear}
|
|
|
|
**After:** ->where('tc.school_year', $schoolYear)
|
|
|
|
**Verification:** PHP lint passed for User.php; static diff confirms interpolation removed.
|
|
|
|
**Result:** Fixed.
|
|
|
|
### SEC-010 - Medium - Dependency Audit
|
|
|
|
**Finding:** Dependency audit could not be completed in this sandbox because composer is not installed.
|
|
|
|
**Risk:** Known vulnerable packages may remain. Laravel, JWT, PDF, and mail libraries should be audited before deployment because humans keep shipping libraries faster than they patch them.
|
|
|
|
**Location:** composer.lock / environment
|
|
|
|
**Evidence:** composer audit returned: bash: composer: command not found.
|
|
|
|
**Remediation:** Not fixed here. Run composer audit in CI and fail builds on high/critical advisories.
|
|
|
|
**Before:** No verified dependency advisory baseline from this run.
|
|
|
|
**After:** Open remediation item documented.
|
|
|
|
**Verification:** Attempted composer audit; blocked by missing composer binary.
|
|
|
|
**Result:** Open.
|
|
|
|
### SEC-011 - Medium - Dynamic Security Testing
|
|
|
|
**Finding:** OWASP ZAP and manual HTTP abuse testing were not executed because no running target with required PHP extensions/database was available in the sandbox.
|
|
|
|
**Risk:** Static review can miss runtime authorization, CORS, CSRF token, redirect, and response-header issues.
|
|
|
|
**Location:** Running application / OWASP ZAP
|
|
|
|
**Evidence:** php artisan route:list failed because the PHP runtime lacks mbstring: undefined function mb_split().
|
|
|
|
**Remediation:** Install required PHP extensions, configure a test database, boot the app, then run authenticated and unauthenticated ZAP baseline plus manual endpoint abuse tests.
|
|
|
|
**Before:** No dynamic scan baseline.
|
|
|
|
**After:** Open remediation item documented with blocker.
|
|
|
|
**Verification:** Artisan route listing attempted; blocked by missing mbstring extension.
|
|
|
|
**Result:** Open.
|
|
|
|
### SEC-012 - Low - Framework Scope Accuracy
|
|
|
|
**Finding:** The uploaded plan says CodeIgniter 4, but the application is Laravel 12.
|
|
|
|
**Risk:** Wrong-framework checklists miss framework-specific controls. This is how audits become theater with better fonts.
|
|
|
|
**Location:** security-verification-plan.md
|
|
|
|
**Evidence:** composer.json name/type and dependencies identify laravel/framework ^12.0; routes, middleware, FormRequests, and artisan confirm Laravel structure.
|
|
|
|
**Remediation:** Documented scope mismatch. Future audits should use a Laravel-specific checklist.
|
|
|
|
**Before:** CodeIgniter 4 plan wording.
|
|
|
|
**After:** This report applies the plan categories to Laravel controls.
|
|
|
|
**Verification:** Static framework identification completed from composer.json and project structure.
|
|
|
|
**Result:** Open documentation correction.
|
|
|
|
## Verification Evidence
|
|
|
|
- php -l changed PHP files: passed
|
|
- composer audit: blocked - composer command not found
|
|
- php artisan route:list: blocked - missing PHP mbstring extension (mb_split undefined)
|
|
- static secret scan: original app/JWT/SMTP/PayPal secrets removed from edited .env/test.sql
|
|
|
|
## Changed Source Files
|
|
|
|
- .env
|
|
- test.sql
|
|
- routes/web.php
|
|
- routes/api.php
|
|
- bootstrap/app.php
|
|
- app/Http/Middleware/SecurityHeaders.php
|
|
- app/Services/Files/FileServeService.php
|
|
- app/Services/Events/EventManagementService.php
|
|
- app/Services/Expenses/ExpenseReceiptService.php
|
|
- app/Services/BroadcastEmail/BroadcastEmailImageService.php
|
|
- app/Services/PrintRequests/PrintRequestsPortalService.php
|
|
- app/Http/Controllers/Api/PrintRequests/PrintRequestsController.php
|
|
- app/Models/User.php
|
|
|
|
## Deployment Notes
|
|
|
|
- Rotate the exposed Gmail app password, JWT secret, Laravel APP_KEY, database password, and PayPal credentials. Redaction in the repository does not invalidate credentials already leaked in the original ZIP. That part requires action with the providers, not wishful thinking.
|
|
- Regenerate APP_KEY and JWT_SECRET in each environment. Do not commit .env files with real values.
|
|
- Run composer audit in CI and install required PHP extensions, including mbstring, before dynamic tests.
|
|
- Re-test all changed public auth/contact/invite routes for expected 429 behavior and frontend compatibility. |