# Sprint 3 Architecture Review — INVEETAIRE

**Reviewer Role:** Principal Software Architect  
**Review Date:** 2026-07-06  
**Sprint Scope:** Database Foundation, TD-002, TD-009, TD-010, S3-003 WorkspaceMiddleware, WS-001 Provisioning, WS-002 Checklist, WS-003 Phase Evaluation  
**Reference Documents:** AI.md v1.1, DBP v1.0, WORKFLOW.md, SPRINT3_PLANNING.md, SPRINT3_EXECUTION_GUIDE.md

---

## 1. Executive Summary

Sprint 3 delivers the tenant boundary, workspace lifecycle, and security hardening layer of the INVEETAIRE platform. After a thorough static review of all Sprint 3 implementation files against the approved reference documentation, the implementation is architecturally sound, correctly layered, and security-compliant. No critical violations were found. Two minor findings and five forward-looking recommendations are noted below.

---

## 2. Overall Architecture Score

| Category                | Score   |
|-------------------------|---------|
| Architecture Compliance | 97/100  |
| Security                | 95/100  |
| Performance             | 92/100  |
| Code Quality            | 94/100  |
| Technical Debt          | 90/100  |
| **Overall**             | **94/100** |

---

## 3. Architecture Compliance

### 3.1 Layered Architecture — PASS

The MVC + Service + Model layer contract is correctly honored throughout Sprint 3.

| Layer      | File                       | Verdict |
|------------|----------------------------|---------|
| Controller | `WorkspaceController.php`  | Thin — captures HTTP, validates via Validator, calls Service, redirects. No SQL, no business rules. |
| Service    | `WorkspaceService.php`     | Owns all business logic, transactions, checklist evaluation, and phase evaluation. |
| Model      | `WorkspaceModel.php`       | Contains only parameterized PDO queries. No business logic or presentation logic. |
| View       | `checklist_widget.php`     | Pure presentation. No SQL, no session access, no database calls. |
| Middleware | `WorkspaceMiddleware.php`  | Stateless. Validates only. The single `Session::destroy()` on archived/purged detection is the sole mutation and is architecturally correct. |

No layer violations were detected.

The `Database::getConnection()` calls inside `WorkspaceMiddleware` private methods are an acceptable deviation — the middleware has no Model class by approved design, and these queries are scoped, minimal, and parameterized.

### 3.2 Thin Controllers — PASS

`WorkspaceController` is 89 lines. Methods `create()` and `store()` do exactly what controllers are authorised to do: read HTTP input, validate via `WorkspaceValidator`, delegate to `WorkspaceService`, and respond. No SQL, no business rules, no direct session writes beyond flashing credentials.

### 3.3 PSR-4 Structure — PASS

All Sprint 3 classes are in the correct namespace hierarchy:

- `App\Middleware\WorkspaceMiddleware`
- `App\Modules\Workspace\{WorkspaceController, WorkspaceService, WorkspaceModel, WorkspaceValidator}`

No namespace mismatches found.

### 3.4 Module Boundaries — PASS

The Workspace module does not import from the Auth module. Auth does not import from Workspace. Cross-module references flow only through shared `App\Core\*` infrastructure, which is the approved pattern.

---

## 4. Frozen Kernel Verification — PASS

Confirmed via git history analysis. Router, MiddlewarePipeline, BaseController, and Database were last modified during Sprint 0 INIT commits (2026-06-30) and the CORE-001 commit (2026-07-01). No modifications were made to any of these files during Sprint 3.

| File                             | Last Modified    | Result                    |
|----------------------------------|------------------|---------------------------|
| `app/core/Router.php`            | 2026-06-30       | Unchanged in Sprint 3 ✅  |
| `app/core/MiddlewarePipeline.php`| 2026-07-01       | Unchanged in Sprint 3 ✅  |
| `app/core/BaseController.php`    | 2026-06-30       | Unchanged in Sprint 3 ✅  |
| `app/core/Database.php`          | 2026-06-30       | Unchanged in Sprint 3 ✅  |

> **Note:** `app/core/Session.php` was modified in the AUTH-004 commit (2026-07-05). AUTH-004 is a Sprint 2 finalization task — not a Sprint 3 kernel violation. The kernel remains frozen for Sprint 3.

---

## 5. Workspace Isolation — PASS

### 5.1 Tenant Boundary

`WorkspaceMiddleware` correctly enforces workspace isolation on every request entering `/app/couple/*` and `/app/crew/*`:

- Session `workspace_id` is extracted, validated against the `workspaces` table.
- Authenticated user's `users.workspace_id` is cross-referenced against session `workspace_id`.
- A mismatch yields an immediate 403.
- `workspace_status = 'archived'` or `workspace_status = 'purged'` (or `purged_at IS NOT NULL`) yields session destroy + 403.

### 5.2 Super Admin Isolation

Super Admins may only access workspace-specific routes under a verified and active `support_sessions` database record (`is_active=1, closed_at IS NULL`). This guard is database-level, not session-level only. No Super Admin workspace bypass exists outside this approved flow.

### 5.3 Query Isolation

Every query in `WorkspaceModel` is parameterized with `:workspace_id`. No query touches cross-workspace data. No query lacks a workspace boundary predicate where one is applicable.

---

## 6. WorkspaceMiddleware Review — PASS

| Requirement                                                    | Result |
|----------------------------------------------------------------|--------|
| Stateless                                                      | ✅ No instance state, no caching |
| No business logic                                              | ✅ Only boundary validation |
| No provisioning logic                                          | ✅ Confirmed absent |
| No checklist logic                                             | ✅ Confirmed absent |
| No phase evaluation logic                                      | ✅ Confirmed absent |
| No workspace mutation (except session destroy on termination)  | ✅ Correct |
| Middleware ordering: Auth → Role → Workspace                   | ✅ Verified in `couple.php` and `crew.php` |
| Correct 403 handling via `ErrorHandler::renderForbidden()`    | ✅ Used consistently |
| Database error isolation (DB failure → null → 403)            | ✅ Conservative and correct |

---

## 7. Workspace Provisioning Review — PASS

| Requirement                              | Result |
|------------------------------------------|--------|
| Transaction ownership in Service layer   | ✅ `WorkspaceService::provision()` owns `beginTransaction()` / `commit()` / `rollBack()` |
| No nested transactions                   | ✅ `WorkspaceModel` contains no `beginTransaction` call |
| SQL only in Model layer                  | ✅ Confirmed — Service contains no PDO calls |
| Rollback on failure                      | ✅ `catch(\Throwable)` checks `inTransaction()` before rollback |
| Atomic provisioning                      | ✅ 9 child inserts within one transaction |
| Slug uniqueness validation               | ✅ Pre-transaction `slugExists()` check; `RuntimeException` on collision |
| Password handling                        | ✅ `bin2hex(random_bytes(6))` — CSPRNG source; bcrypt-hashed before insert; never logged |
| Audit logging                            | ✅ Emitted inside transaction scope; excludes credentials |

**Minor Finding #1 — Slug pre-transaction race condition (low risk):**

The `slugExists()` check occurs before the transaction opens. Under extreme concurrent provisioning load, two simultaneous requests could both pass the pre-check. In that scenario, the database UNIQUE constraint on `workspaces.slug` will reject the second insert and the rollback will fire — no data corruption occurs. The error message surfaced to the second admin will be an unhandled database exception rather than the clean "slug already taken" message.

*Risk Level: Low. No action required for Sprint 3 approval.*

---

## 8. Workspace Checklist Review — PASS

| Requirement                           | Result |
|---------------------------------------|--------|
| Service performs evaluation           | ✅ `WorkspaceService::getChecklist()` evaluates all 7 steps |
| Model retrieves data                  | ✅ Dedicated single-purpose query methods |
| View only renders                     | ✅ `checklist_widget.php` contains no SQL, no session access, no logic |
| Progress calculation                  | ✅ `(completedCount / 7) * 100` rounded to integer |
| Ready-To-Send gate                    | ✅ All steps 1–6 must be `true` before step 7 unlocks |
| No presentation logic outside views  | ✅ Service returns data arrays; view applies class and markup logic |
| XSS protection                        | ✅ All dynamic output wrapped in `escape_html()` |

**Minor Finding #2 — `SELECT *` in `getPrinterConfig()` (low risk):**

`WorkspaceModel::getPrinterConfig()` uses `SELECT *`. This fetches all columns including any future additions, making the query contract implicit. This is not exploitable but is a code quality concern.

*Risk Level: Low. Forward-looking improvement.*

---

## 9. Workspace Phase Evaluation Review — PASS

| Requirement                          | Result |
|--------------------------------------|--------|
| Correct lifecycle transitions        | ✅ past → `post_event`, today → `live`, future+activated → `active`, future+not_activated+configured → `setup`, else → `provisioned` |
| Terminal states remain terminal      | ✅ `archived` and `purged` return immediately — no DB write, no audit |
| No unnecessary updates               | ✅ `if ($targetPhase !== $currentStatus)` guards all persistence |
| Audit only on transition             | ✅ `insertAuditLog()` is inside the conditional persistence block |
| No transition loops                  | ✅ Terminal state guard prevents any re-evaluation cycle |

---

## 10. Security Review

### 10.1 Content Security Policy (TD-002) — PASS WITH NOTE

`public/.htaccess` sets the following headers:

- `Content-Security-Policy` with `default-src 'self'` — correct safe default
- `frame-ancestors 'none'` — clickjacking prevention
- `X-Frame-Options: SAMEORIGIN` — redundant secondary protection (harmless)
- `X-Content-Type-Options: nosniff`
- `X-XSS-Protection: 1; mode=block`
- `Referrer-Policy: strict-origin-when-cross-origin`

**Finding:** `script-src 'self' 'unsafe-inline' 'unsafe-eval'` — `'unsafe-eval'` is present. Unless a runtime dependency (e.g. a template engine using `eval()`) specifically requires it, this weakens the CSP against XSS attacks that rely on JavaScript `eval()`. This was flagged in the S3-002 review and remains unresolved.

*See Recommendation #1.*

### 10.2 Forgot Password Rate Limiting (TD-009) — PASS

- Dual-axis limiting by IP AND email hash — correctly blocks both vectors independently.
- Limits are configurable via `config/auth.php` (`max_attempts=3`, `window=900` seconds).
- Email is MD5-hashed in all log output — never plaintext.
- Cache files use `LOCK_EX` — no race condition on write.
- Invalid JSON cache returns empty array (safe fallback).
- Malformed email rejected by `FILTER_VALIDATE_EMAIL` before any rate-limit file lookup — bypass via malformed email is not possible.

### 10.3 Reset Token Cleanup (TD-010) — PASS

- Probabilistic cleanup on `requestPasswordReset()` — no dedicated process required.
- `glob()` returning `false` is handled with an early return.
- Corrupted JSON files (non-array `json_decode` result) are safely deleted.
- `@unlink()` silences permission errors without interrupting the auth flow.
- Token file paths are derived via `ctype_xdigit()` validation — path traversal is not possible.

### 10.4 SQL Injection Protection — PASS

All queries in `WorkspaceModel` and `WorkspaceMiddleware` use PDO prepared statements with named parameters. Zero string interpolation in SQL. The one instance of dynamic SQL concatenation in `updateWorkspaceStatus()` concatenates only fixed column name literals selected by an internal `if/elseif` block — not user input — and is safe.

### 10.5 Session Integrity — PASS

All session access in Sprint 3 code uses `Session::has()` / `Session::get()` / `Session::set()`. No direct `$_SESSION` access was found in any Sprint 3 file.

### 10.6 Secret Leakage — PASS

- `plain_password` is not present in `audit_logs` payload (confirmed).
- `plain_password` is not passed to `Logger` at any point (confirmed).
- `plain_password` surfaces only in `Session::set('flash_info', ...)` for one-time display to the Super Admin — intentional by design.
- Reset log entries write `email=***` — email is masked.
- Rate limiting logs record `md5($email)` — not plaintext email.

---

## 11. Performance Review

| Area | Finding | Verdict |
|------|---------|---------|
| WorkspaceMiddleware query count | 2 queries per request (workspace + user), plus 1 optional for Super Admin support sessions | Acceptable |
| Checklist query count | 5 individual queries per checklist load | Acceptable at MVP scale |
| Phase evaluation query count | 1–3 queries depending on branch taken | Acceptable |
| Provisioning query count | 9 inserts inside one transaction | Correct |
| N+1 patterns | None detected | Pass |
| Transaction scope | Correctly opened and closed inside Service | Pass |
| Middleware efficiency | No in-memory state accumulation | Pass |

**Performance Note — Checklist (5 queries):** `getChecklist()` issues 5 separate queries. This is appropriate for MVP scale and avoids premature optimisation. A future improvement could batch-fetch all data in a single query or a DB view. Not a blocker.

---

## 12. Code Quality Review

| Criterion                  | Verdict |
|----------------------------|---------|
| Separation of concerns     | Excellent — layers are clearly delineated with no bleed |
| Naming consistency         | `snake_case` for variables/parameters, `PascalCase` for classes — consistent |
| Dependency direction       | Controller → Service → Model — no reverse dependencies |
| Maintainability            | All methods are short, single-purpose, and well-commented |
| Readability                | Inline comments explain the intent at each step in `provision()` |
| Future extensibility       | Phase evaluation expressed as a clean conditional tree — adding phases is straightforward |
| `declare(strict_types=1)`  | Present in all Sprint 3 PHP files |
| Type declarations          | Method signatures include return types and parameter types throughout |
| Docblocks                  | Present on all public methods |

---

## 13. Technical Debt Review

| ID        | Item                                                                 | Priority |
|-----------|----------------------------------------------------------------------|----------|
| TD-S3-01  | `'unsafe-eval'` in CSP `script-src`                                 | Medium   |
| TD-S3-02  | `SELECT *` in `WorkspaceModel::getPrinterConfig()`                  | Low      |
| TD-S3-03  | Slug uniqueness pre-transaction check — race condition on collision  | Low      |
| TD-S3-04  | `plain_password` in session flash — consider a one-time display page | Low      |
| TD-S3-05  | `evaluatePhase()` is passive — no caller wired yet                  | Low      |

Only genuine findings are listed. No speculative debt is included.

---

## 14. Architectural Risks

| Risk | Likelihood | Impact | Mitigation Status |
|------|-----------|--------|-------------------|
| Future Couple/Crew routes registered outside a guarded group | Medium | High | Current route groups in `couple.php` and `crew.php` are correct. Convention must be maintained in future sprints. |
| `evaluatePhase()` is a passive method with no caller | Low | Medium | Method is production-ready. The Invitation Send workflow (future sprint) must wire it in as a trigger or a periodic evaluation hook. |
| `activated_at` is never set by any Sprint 3 code | Low | Low | Expected — the `active` phase transition is intentionally deferred to the Invitation Send sprint. The evaluator handles `null` correctly and falls back to the `provisioned`/`setup` evaluation tree. |

---

## 15. DRP Assessment

**No DRP Required.**

Sprint 3 implemented features that were fully specified in the approved `SPRINT3_PLANNING.md` and `SPRINT3_EXECUTION_GUIDE.md`. No new architectural patterns were introduced that deviate from the established MVC + Service + Model contract:

1. `WorkspaceMiddleware` follows the same `MiddlewareInterface` contract defined in CORE-001.
2. The Workspace module follows the same module structure established in Sprints 1–2.
3. Phase evaluation uses the existing `workspaces.workspace_status` column — no schema alterations.
4. Rate limiting uses the established filesystem-based JSON cache pattern from Sprint 2.

No new frameworks, infrastructure components, or architectural layers were introduced. All decisions are consistent with the pre-approved architecture documented in `AI.md v1.1`.

---

## 16. Recommendations

| # | Recommendation | Priority |
|---|---------------|----------|
| 1 | Remove `'unsafe-eval'` from the CSP `script-src` directive unless a specific runtime dependency is documented as requiring it | Medium |
| 2 | Replace `SELECT *` in `WorkspaceModel::getPrinterConfig()` with an explicit column list | Low |
| 3 | Document the expected caller for `evaluatePhase()` in Sprint 4 planning — triggered on couple dashboard load or via a scheduled cron job | Medium |
| 4 | Consider a dedicated one-time credentials acknowledgement page (POST–redirect–GET pattern) instead of a session flash for `plain_password` | Low |
| 5 | Add a docblock note to `WorkspaceModel::insertWorkspace()` documenting that the database UNIQUE constraint on `slug` is the definitive collision guard | Low |

---

## 17. Sprint 3 Verdict

## ✅ APPROVED WITH MINOR RECOMMENDATIONS

**Sprint 3 is officially closed.**

All Sprint 3 deliverables have been implemented correctly against the approved specification:

| Task | Deliverable | Result |
|------|-------------|--------|
| Database Foundation | schema.sql, seed.sql, README.md | ✅ |
| TD-002 | Content Security Policy in `public/.htaccess` | ✅ |
| TD-009 | Forgot Password Rate Limiting (IP + email, dual-axis) | ✅ |
| TD-010 | Reset Token Cleanup (probabilistic, safe) | ✅ |
| S3-003 | WorkspaceMiddleware (stateless, correct 403) | ✅ |
| WS-001 | Workspace Provisioning (atomic transaction, no credential leakage) | ✅ |
| WS-002 | Workspace Setup Checklist (correctly layered, XSS-safe) | ✅ |
| WS-003 | Workspace Phase Evaluation (terminal states respected, no-op guard) | ✅ |

No blocking issues were found. The five recommendations above are recorded for Sprint 4 backlog consideration. No DRP is required.

The architecture remains clean, the kernel is frozen and untouched, tenant isolation is enforced at both middleware and query levels, and no security violations were detected.

---

*Sprint 3 Architecture Review — INVEETAIRE*  
*Produced: 2026-07-06*