294 lines
16 KiB
Markdown
294 lines
16 KiB
Markdown
# Codebase Concerns
|
|
|
|
**Analysis Date:** 2026-02-13
|
|
|
|
## Tech Debt
|
|
|
|
### FinanceDocument Model Complexity
|
|
- **Issue:** `app/Models/FinanceDocument.php` is 882 lines with 27+ status constants and multiple workflows (approval, disbursement, recording, payment). Contains both old workflow (cashier→accountant) and new workflow (secretary→chair→board) fields and methods coexisting.
|
|
- **Files:** `app/Models/FinanceDocument.php` (lines 1-882)
|
|
- **Impact:** Hard to maintain, difficult to extend, high risk of introducing bugs in approval logic. Multiple overlapping status fields make state transitions error-prone.
|
|
- **Fix approach:**
|
|
- Extract new workflow logic into separate `FinanceDocumentApprovalService` class (already partially exists in codebase)
|
|
- Remove deprecated cashier/accountant approval workflow entirely once old code migrated
|
|
- Consider using state machine package (e.g., `thiagoprz/eloquent-state-machine`) to manage state transitions
|
|
|
|
### Large Model Files
|
|
- **Issue:** Multiple models exceed 400+ lines: `Income` (467), `Document` (466), `Article` (460), `Member` (437), `Announcement` (427), `Issue` (363)
|
|
- **Files:** `app/Models/{Income,Document,Article,Member,Announcement,Issue}.php`
|
|
- **Impact:** Models are bloated with business logic; difficult to test and maintain
|
|
- **Fix approach:** Extract domain logic to service classes, keep models for data relationships only
|
|
|
|
### Backward Compatibility Overhead
|
|
- **Issue:** FinanceDocument maintains both old (cashier/accountant) and new (secretary/chair/board) approval workflows with deprecated methods marked `@deprecated`
|
|
- **Files:** `app/Models/FinanceDocument.php` (lines 551-568, 757+)
|
|
- **Impact:** Code duplication, confusing for developers, increases cognitive load
|
|
- **Fix approach:** Set migration deadline (e.g., March 2026), remove all deprecated methods and legacy status constants
|
|
|
|
## Known Bugs
|
|
|
|
### Site Revalidation Webhook Silent Failures
|
|
- **Symptoms:** Next.js static site may not update when articles/pages/documents are modified in Laravel admin
|
|
- **Files:** `app/Services/SiteRevalidationService.php` (lines 34-59)
|
|
- **Trigger:** Log warnings only at WARNING level when webhook fails; developers may not notice missed revalidations
|
|
- **Current state:** Catches exceptions but logs only warnings; webhook timeout is 5 seconds
|
|
- **Workaround:** Check Laravel logs manually for `"Site revalidation failed"` warnings
|
|
- **Fix approach:**
|
|
- Queue webhook calls with retry logic (e.g., 3 retries with exponential backoff)
|
|
- Log errors at ERROR level with alert notification
|
|
- Add monitoring for failed revalidations
|
|
|
|
### Email Notifications May Fail Silently
|
|
- **Symptoms:** Users don't receive approval/rejection notifications (e.g., `FinanceDocumentFullyApproved`, `PaymentSubmittedMail`)
|
|
- **Files:** `app/Http/Controllers/FinanceDocumentController.php` (lines 105-108, 174, 184, etc.)
|
|
- **Trigger:** Uses `Mail::queue()` without error handling or confirmation
|
|
- **Current state:** Emails queued but no verification of delivery
|
|
- **Fix approach:**
|
|
- Add event listeners to track mail failures
|
|
- Implement fallback notification channel (SMS via existing SMS service or in-app notifications)
|
|
- Log all mail queue operations with request ID for audit trail
|
|
|
|
## Security Considerations
|
|
|
|
### National ID Decryption Exception Not Fully Handled
|
|
- **Risk:** Corrupted or mismatched encryption keys could cause decryption to fail; exception logged but member object may be in inconsistent state
|
|
- **Files:** `app/Models/Member.php` (lines 177-190)
|
|
- **Current mitigation:** Try-catch block logs error to Laravel log
|
|
- **Recommendations:**
|
|
- Rotate encryption keys with migration strategy for existing encrypted data
|
|
- Add health check command to verify all encrypted data can be decrypted
|
|
- Implement decryption retry with fallback (query original source if available)
|
|
- Document key rotation procedure in SECURITY.md
|
|
|
|
### Password Generation from Phone Number
|
|
- **Risk:** Password derived from last 4 digits of phone (`substr($phone, -4)`) is not cryptographically secure for initial user setup
|
|
- **Files:** `app/Console/Commands/ImportMembersCommand.php` (lines 135-146)
|
|
- **Current mitigation:** Used only during bulk import; users should change password after first login
|
|
- **Recommendations:**
|
|
- Force password reset on first login (add `force_password_reset` flag to User model)
|
|
- Use random password generation instead of phone-derived passwords
|
|
- Send temporary password via email (encrypted) or secure channel, never via phone number
|
|
|
|
### Mail Headers Not Validated
|
|
- **Risk:** Site revalidation webhook uses unvalidated `x-revalidate-token` header; no CSRF protection
|
|
- **Files:** `app/Services/SiteRevalidationService.php` (line 54)
|
|
- **Current mitigation:** Token stored in `.env` file
|
|
- **Recommendations:**
|
|
- Document token rotation process
|
|
- Add rate limiting to revalidation endpoint
|
|
- Consider using HMAC-SHA256 signed requests instead of bearer tokens
|
|
|
|
### Public Document UUID Leakage
|
|
- **Risk:** Documents have `public_uuid` exposed in API; uuid may be guessable if predictable
|
|
- **Files:** `app/Models/Document.php` (revalidation calls public_uuid)
|
|
- **Current mitigation:** Appears to use Laravel UUIDs (non-sequential)
|
|
- **Recommendations:**
|
|
- Verify UUID generation uses `Illuminate\Support\Str::uuid()` (v4, cryptographically random)
|
|
- Audit Document access control in `app/Http/Controllers/PublicDocumentController.php`
|
|
|
|
## Performance Bottlenecks
|
|
|
|
### Large Load in FinanceDocument Show Page
|
|
- **Problem:** `FinanceDocumentController::show()` eager loads 17 relationships, many of which may be unused in view
|
|
- **Files:** `app/Http/Controllers/FinanceDocumentController.php` (lines 116-141)
|
|
- **Cause:** Over-eager loading; should load relationships based on view requirements
|
|
- **Improvement path:**
|
|
- Split load into view-specific queries
|
|
- Use `with()` conditionally based on user role
|
|
- Measure N+1 queries with Laravel Debugbar
|
|
|
|
### N+1 Queries on Finance Index Listing
|
|
- **Problem:** FinanceDocument index loads 15 results with full relationships but filters happen in PHP, not database
|
|
- **Files:** `app/Http/Controllers/FinanceDocumentController.php` (lines 20-56)
|
|
- **Cause:** Workflow stage filtering happens after query, not in WHERE clause
|
|
- **Improvement path:**
|
|
- Move workflow_stage logic into Eloquent scopes
|
|
- Use `whereRaw()` for date-based stage detection if necessary
|
|
- Index database columns: `payment_order_created_at`, `payment_executed_at`, `cashier_ledger_entry_id`, `accounting_transaction_id`
|
|
|
|
### Import Commands Process Large Files In Memory
|
|
- **Problem:** `ImportMembersCommand`, `ImportAccountingData`, `ImportHugoContent` load entire Excel files into memory with PhpSpreadsheet
|
|
- **Files:** `app/Console/Commands/{ImportMembersCommand,ImportAccountingData,ImportHugoContent}.php`
|
|
- **Cause:** Using `IOFactory::load()` without streaming
|
|
- **Improvement path:**
|
|
- Use chunked reading for large files (PhpSpreadsheet supports `ChunkReadFilter`)
|
|
- Batch insert rows in groups of 100-500
|
|
- Add progress bar using Artisan command
|
|
|
|
### Mail Queue Without Batch Processing
|
|
- **Problem:** Finance document approval sends individual emails in loop (e.g., line 105-108) without batching
|
|
- **Files:** `app/Http/Controllers/FinanceDocumentController.php` (lines 105-108, 182-185, 215-218)
|
|
- **Cause:** Loop creates separate Mail::queue() call for each cashier/chair/board member
|
|
- **Improvement path:**
|
|
- Collect all recipients and send single batch notification
|
|
- Use Mailable::withSymfonyMessage() to set BCC instead of TO:recipient loop
|
|
- Consider notification channels for role-based sending
|
|
|
|
## Fragile Areas
|
|
|
|
### Multi-Tier Approval Workflow State Machine
|
|
- **Files:** `app/Models/FinanceDocument.php`, `app/Http/Controllers/FinanceDocumentController.php`
|
|
- **Why fragile:**
|
|
- Complex conditional logic determining next approver (amount tier + status = next tier)
|
|
- No atomic state transitions; multiple `update()` calls without transactions
|
|
- Amount tier can change after approval (potential issue if amount modified)
|
|
- Self-approval prevention logic repeated in multiple methods
|
|
- **Safe modification:**
|
|
- Add database constraint: `UNIQUE(finance_documents, status, approved_by_secretary_id)` to prevent duplicate approvals
|
|
- Wrap approval logic in explicit DB::transaction() in controller
|
|
- Add test for each tier (small, medium, large) with all roles
|
|
- **Test coverage:** Good coverage exists (`tests/Unit/FinanceDocumentTest.php`) but missing edge cases
|
|
|
|
### Member Encryption/Decryption
|
|
- **Files:** `app/Models/Member.php` (lines 175-207)
|
|
- **Why fragile:**
|
|
- Decryption failure returns null without distinction from empty field
|
|
- No validation that decrypted data is valid national ID format
|
|
- Hash function (SHA256) hardcoded; no version indicator if hashing algorithm changes
|
|
- **Safe modification:**
|
|
- Create `MemberEncryption` helper class with version support
|
|
- Add `$member->national_id_decryption_failed` flag to track failures
|
|
- Validate decrypted ID format matches Taiwan ID requirements
|
|
- **Test coverage:** Missing - no tests for encryption/decryption failure scenarios
|
|
|
|
### Article Slug Generation with Chinese Characters
|
|
- **Files:** `app/Models/Article.php` (lines 84-109)
|
|
- **Why fragile:**
|
|
- Slug fallback uses `Str::ascii()` which may produce empty string for pure Chinese titles
|
|
- Final fallback `'article-'.time()` creates non-semantic slugs
|
|
- No uniqueness guarantee if two articles created in same second
|
|
- **Safe modification:**
|
|
- Use pinyin conversion library for Chinese titles
|
|
- Always append counter to ensure uniqueness (current code does this, but starting from base slug not timestamp)
|
|
- **Test coverage:** Missing - no tests for Chinese title slug generation
|
|
|
|
### Document Access Control
|
|
- **Files:** `app/Http/Controllers/PublicDocumentController.php`, `app/Http/Controllers/Api/PublicDocumentController.php`
|
|
- **Why fragile:**
|
|
- Document access depends on category and user role
|
|
- Multiple access control points (controller + model) make audit trail difficult
|
|
- No consistent logging of document access
|
|
- **Safe modification:**
|
|
- Centralize access checks in `Document::canBeViewedBy()` method
|
|
- Add audit logging to `DocumentAccessLog` table on every access
|
|
- Use policy classes for clearer authorization
|
|
- **Test coverage:** Missing - no tests for document access control by category
|
|
|
|
## Scaling Limits
|
|
|
|
### SQLite in Development, MySQL in Production
|
|
- **Current capacity:** SQLite limited by single process/file locks
|
|
- **Limit:** Development environment may not catch MySQL-specific issues (JSON functions, full text search, etc.)
|
|
- **Scaling path:**
|
|
- Add CI step running tests against MySQL
|
|
- Use Docker Compose with MySQL for local development
|
|
- Document differences in CLAUDE.md
|
|
|
|
### Audit Logging Unbounded Growth
|
|
- **Current capacity:** `AuditLog` table grows with every action (`AuditLogger::log()` called throughout codebase)
|
|
- **Limit:** No retention policy; table will grow indefinitely
|
|
- **Scaling path:**
|
|
- Add scheduled cleanup command (`php artisan audit-logs:prune --days=90`)
|
|
- Archive old logs to separate table or external storage
|
|
- Add indexes on `auditable_id`, `auditable_type`, `created_at` for queries
|
|
|
|
### File Upload Storage
|
|
- **Current capacity:** Attachments stored in `storage/app/local` and `storage/app/public` without quota
|
|
- **Limit:** Disk space will fill with finance documents, articles, disability certificates
|
|
- **Scaling path:**
|
|
- Migrate uploads to S3 or cloud storage
|
|
- Add cleanup for soft-deleted documents
|
|
- Implement virus scanning for uploaded files
|
|
|
|
## Dependencies at Risk
|
|
|
|
### barryvdh/laravel-dompdf (PDF Generation)
|
|
- **Risk:** Package maintained but based on Dompdf which has history of security issues in HTML/CSS parsing
|
|
- **Impact:** User-submitted content (article attachments, issue descriptions) rendered to PDF could be exploited
|
|
- **Migration plan:**
|
|
- Sanitize HTML before PDF generation
|
|
- Consider mPDF or TCPDF as alternatives
|
|
- Test with OWASP XSS payloads in PDF generation
|
|
|
|
### maatwebsite/excel (Excel Import/Export)
|
|
- **Risk:** PhpSpreadsheet (underlying library) may have formula injection vulnerabilities
|
|
- **Impact:** Imported Excel files with formulas could execute on user machines
|
|
- **Migration plan:**
|
|
- Strip formulas during import (`setReadDataOnly(true)`)
|
|
- Validate cell content is not formula before importing
|
|
- Document in user guide: "Do not import untrusted Excel files"
|
|
|
|
### simplesoftwareio/simple-qrcode
|
|
- **Risk:** Package has been archived; no active maintenance
|
|
- **Impact:** Potential security vulnerabilities won't be patched
|
|
- **Migration plan:**
|
|
- Consider switch to `chillerlan/php-qrcode` (actively maintained)
|
|
- Audit current QR generation code for security issues
|
|
- Test QR code generation with large payloads
|
|
|
|
### spatie/laravel-permission (Role/Permission)
|
|
- **Risk:** Complex permission system; incorrect configuration could expose restricted data
|
|
- **Impact:** Member lifecycle, finance approvals depend entirely on permission configuration
|
|
- **Migration plan:**
|
|
- Document all role/permission requirements in RBAC_SPECIFICATION.md
|
|
- Add smoke test verifying each role can/cannot access correct routes
|
|
- Audit permission checks with debugbar on each protected route
|
|
|
|
## Missing Critical Features
|
|
|
|
### No Rate Limiting on Finance Approval
|
|
- **Problem:** No rate limit prevents rapid approval/rejection spam
|
|
- **Blocks:** Audit trail integrity; potential for accidental approvals
|
|
- **Recommendation:** Add throttle middleware on `FinanceDocumentController::approve()` (1 approval per 2 seconds per user)
|
|
|
|
### No Email Delivery Confirmation
|
|
- **Problem:** System queues emails but provides no confirmation users received them
|
|
- **Blocks:** Important notifications (payment rejection, expense approval) may go unseen
|
|
- **Recommendation:** Implement email open tracking or delivery webhook with SendGrid/Mailgun
|
|
|
|
### No Database Transaction on Multi-Step Operations
|
|
- **Problem:** Finance document approval updates multiple fields in separate queries (lines 160-164)
|
|
- **Blocks:** Partial failures could leave document in inconsistent state
|
|
- **Recommendation:** Wrap all multi-step operations in explicit `DB::transaction()`
|
|
|
|
### No Async Job Retry Logic
|
|
- **Problem:** Site revalidation webhook has only catch block, no retry queue
|
|
- **Blocks:** Temporary network issues cause permanent cache inconsistency
|
|
- **Recommendation:** Use queued jobs with `ShouldBeEncrypted` and exponential backoff
|
|
|
|
## Test Coverage Gaps
|
|
|
|
### Finance Approval Multi-Tier Logic
|
|
- **What's not tested:** All tier combinations with role changes (e.g., chair approves small amount, medium amount requires board)
|
|
- **Files:** `app/Models/FinanceDocument.php` (determineAmountTier, canBeApprovedByChair, etc.)
|
|
- **Risk:** Tier logic changes could silently break approval chain
|
|
- **Priority:** High - this is financial workflow core
|
|
|
|
### Member Encryption Edge Cases
|
|
- **What's not tested:** Decryption failures, key rotation, migration from plaintext
|
|
- **Files:** `app/Models/Member.php` (getNationalIdAttribute, setNationalIdAttribute)
|
|
- **Risk:** Corrupted encryption keys could cause data loss
|
|
- **Priority:** High - financial impact
|
|
|
|
### Article Slug Generation for Chinese Titles
|
|
- **What's not tested:** Pure Chinese titles, mixed Chinese/English, special characters
|
|
- **Files:** `app/Models/Article.php` (generateUniqueSlug)
|
|
- **Risk:** Frontend routing breaks if slug generation fails
|
|
- **Priority:** Medium - affects CMS functionality
|
|
|
|
### Document Access Control by Category and Role
|
|
- **What's not tested:** Board members accessing member-only documents, public documents accessible without auth
|
|
- **Files:** `app/Http/Controllers/PublicDocumentController.php`, `app/Models/Document.php`
|
|
- **Risk:** Private documents exposed to wrong users
|
|
- **Priority:** High - security critical
|
|
|
|
### Email Notification Delivery
|
|
- **What's not tested:** Actual email sends, template rendering, missing email addresses
|
|
- **Files:** All Mail classes in `app/Mail/`
|
|
- **Risk:** Users don't know about approvals/rejections
|
|
- **Priority:** Medium - business critical but hard to test
|
|
|
|
---
|
|
|
|
*Concerns audit: 2026-02-13*
|