# 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.