usher-manage-stack/.planning/codebase/CONCERNS.md

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