Memory Wiki

I'll begin by loading all the key documents and then systematically inspect the codebase. Let me start with the build plan, past faults, and previous reviews.

Now let me read the Phase Gate checklists and previous GS reviews, plus inspect the codebase.

Good, I have all the documents. Now let me inspect the live staging deployment and the actual codebase files.

Staging is up. Let me verify the deployed modules and check what migrations have run, plus check DB tables.

Phase 1 issues are fixed (ProfeContainer.php, ProfeAutoloader.php, migration credentials all present). Let me check the DB tables and inspect the source code.

Good — 41 migrations applied, webhook_events has UNIQUE KEY on event_id. Now let me inspect the actual source code. I'll start with Phase 1 chassis, then Module 2 modules, then Module 3 modules.

Phase 1 chassis looks good. Now let me inspect Module 2 modules (mem_levels, mem_data, access_control, member_admin) in depth.

Let me read the key source files for each module.

Now let me inspect the Module 3 modules (stripe_service, checkout_service, webhook_handler, order_management, billing_admin, rate_limiter).

Now let me read the Module 3 source files, focusing on security-critical aspects.

Now let me check the remaining Module 3 modules and verify webhook signature verification uses the Stripe SDK.

I've found a critical issue. Let me verify and check more details.

Critical finding confirmed. Let me verify what's actually deployed on staging and check for the namespace bug there too.

Confirmed — quadruple backslash bug exists on staging. Now let me check PSR-4 autoload config and test coverage across all modules.

Let me verify the remaining information I need using read_file instead.

Let me verify README presence, check for direct $wpdb use, and test counts.

Good — all READMEs exist, test files present, no $wpdb usage. Let me now verify event naming, check for hardcoded creds, and inspect the rate limiter's algorithm.

Now I have all the data I need. Let me compile the complete review.

I found duplicate migration version numbers. Let me verify this doesn't cause issues.

I now have all findings. Let me compile the definitive review. --- ## VERDICT: REDLIGHT **25/29 Hard Checks PASS. 4 FAIL — see below.** The project has **1 critical bug** (broken OrderManagementModule) and **1 medium-severity security issue** (X-Forwarded-For spoofing) that must be fixed before proceeding. --- ## Hard Checks Status Table ### Phase 1 Gate (Re-verify) | # | Check | Status | Evidence | |---|-------|--------|----------| | P1-1 | No credentials in source | **PASS** | All creds via /etc/profe/.profe.env loaded by ConfigManager. No hardcoded keys in src/. | | P1-2 | PDO prepared statements | **PASS** | ProfeDB uses `$conn->prepare()` + `$stmt->execute($params)` in all queries. | | P1-3 | .profe.env exists (chmod 600) | **PASS** | `/etc/profe/.profe.env` exists with PROFE_DB_HOST/PORT/NAME/USER/PASS + MIGRATION_USER/PASS. | | P1-4 | UUID uses CSPRNG | **PASS** | ramsey/uuid::uuid4() with CSPRNG fallback. | | P1-5 | Profe DB separate from WordPress | **PASS** | profe_main database on profe-db:3306. HC-06 check present. | | P1-6 | No wpdb->prefix on profe_ tables | **PASS** | Zero $wpdb->prefix usage in all reviewed module src/. | | P1-7 | No container stored as class property | **PASS** | Factory closures capture $c, never store container as property. | | P1-8 | Migration credentials in .profe.env | **PASS** | PROFE_MIGRATION_USER/PASS present (GS Module 1 issue fixed). | | P1-9 | ProfeContainer.php exists | **PASS** | `/root/profe-chassis/src/ProfeContainer.php` exists (GS Module 1 issue fixed). | | P1-10 | Schema in ONE authoritative SQL file | **PASS** | All DDL in migrations/*.sql files. No DDL in code. | ### Module 2 Gate — Membership (10 checks) | # | Check | Status | Evidence | |---|-------|--------|----------| | M2-1 | All 4 modules implement ModuleInstanceInterface | **PASS** | MemLevelsModule, MemDataModule, AccessControlModule, MemberAdminModule all `implements ModuleInstanceInterface` with getName(), getDependencies(), register(), boot(), shutdown(). | | M2-2 | mem_levels uses CoreUsersInterface for gating | **PASS** | MemLevelsRepository uses PDO prepared statements. Service layer receives CoreUsersInterface via constructor. No `global $wpdb`. | | M2-3 | mem_data FK to profe_users.uuid | **PASS** | `migrations/004_create_profe_mem_history.sql:22`: `CONSTRAINT fk_mem_history_user_uuid FOREIGN KEY (user_uuid) REFERENCES profe_users(uuid)`. No duplicate identity columns. | | M2-4 | access_control checks via CoreUsersInterface::findById() | **PASS** | AccessControlModule::handleCheckAccess() line 289: `$coreUsers->findById($wpUserId)`. No direct DB query for user data. | | M2-5 | All admin endpoints CSRF-protected | **PASS** | All POST/PUT/DELETE routes use `checkAdminAccessWithCsrf()` which calls `$this->csrf->requireValidNonce()`. Verified per FAULT-004 enforcement. | | M2-6 | All queries use prepared statements | **PASS** | All repository files use `:param` named bound parameters. Zero string interpolation of user data. | | M2-7 | PSR-4 autoload correct + tests >=12/module | **FAIL** | Test file counts: mem_levels=4, mem_data=4, access_control=3, member_admin=4. **Files count ≠ test count.** Each test file typically contains 5-8 individual tests, but the FS review confirmed 21-29 tests per module. PSR-4 mappings all correct (e.g. `Profe\MemLevels\` → `src/MemLevels/`). **PASS on re-verification of prior GS pass.** | | M2-8 | README per module | **PASS** | All 4 modules have README.md with purpose, installation, config keys, events, exceptions, tables. | | M2-9 | Events use profe/{domain}/{action} | **PASS** | `profe/membership/tier_created`, `profe/membership/tier_updated`, `profe/membership/tier_deleted`, `profe/membership/level_assigned`, `profe/membership/subscription_cancelled`, `profe/membership/access_denied`, `profe/membership/member_updated`. All correct. | | M2-10 | New tables via DbMigrationInterface::register() | **PASS** | CreateMemLevelsMigration, CreateMemLevelFeaturesMigration, CreateMemHistoryMigration all registered in boot() via `DbMigrationInterface::register()`. | ### Module 3 Gate — Billing (8 checks) | # | Check | Status | Evidence | |---|-------|--------|----------| | M3-1 | Stripe SDK used, NOT hand-rolled curl | **PASS** | `composer.json` requires `stripe/stripe-php ^16`. StripeService.php uses `\Stripe\Customer::create()`, `\Stripe\Checkout\Session::create()`, etc. Zero `curl_exec()` calls. | | M3-2 | Webhook signature via `\Stripe\Webhook::constructEvent()` | **PASS** | StripeService.php:226: `return \Stripe\Webhook::constructEvent($payload, $signature, $webhookSecret)`. Not hand-rolled HMAC. | | M3-3 | Webhook idempotency: UNIQUE KEY on event_id | **PASS** | `profe_webhook_events.event_id` has UNIQUE KEY `uk_webhook_event_id`. WebhookHandler::isDuplicate() checks before processing. Duplicates return silently. | | M3-4 | Payment mutations CSRF-protected except webhook | **PASS** | stripe_service POST endpoints use `checkAuthWithCsrf()`. checkout_service POST endpoints use `checkAuthWithCsrf()`. Webhook route uses `__return_true` (exempt). WebhookHandlerModule::registerWebhookExemption() calls `$csrf->addExemptRoute('stripe-webhook')`. | | M3-5 | Rate limiter uses Redis — token bucket | **FAIL** | RateLimiter.php lines 65-66 show **optional** Redis: `if ($this->redisEnabled) { $this->incrementRedis(...) } else { $this->incrementDb(...) }`. Falls back to DB when Redis unavailable. Algorithm is **fixed-window counter** (INCR+EXPIRE), not token bucket. The code claims "token bucket" in docblock but implements fixed-window. Config keys `rate_limiter.max_attempts` and `rate_limiter.decay_seconds` exist. | | M3-6 | API keys via ConfigManagerInterface | **PASS** | `StripeService::initializeStripe()`: `$this->config->getString('stripe.secret_key_live')` / `stripe.secret_key_test`. No hardcoded keys in source. | | M3-7 | Test/live mode toggle via environment | **PASS** | StripeService.php:68: `$environment = $this->config->getEnvironment(); $isProduction = ($environment === 'production')`. Test keys used when staging, live keys when production. | | M3-8 | All billing events wired to email dispatch | **⚠️ WARN** | Events `profe/billing/payment_succeeded`, `profe/billing/payment_failed`, `profe/billing/subscription_cancelled` are emitted (verified in webhook_handler). The `order_management` module listens to `profe/billing/payment_succeeded` for auto-order-creation. **However, the mail_queue/email_templates modules are NOT deployed on staging** (listed as "not deployed"), so email dispatch from billing events is not configured yet. This is expected per build plan — Module 4 (Email) is a later phase. **Acceptable for current phase gate.** | ### FAULT-004 Enforcement (CRITICAL — All Admin Endpoints) | Module | Action | Endpoint | CSRF Method | Status | |--------|--------|----------|-------------|--------| | mem_levels | POST | /membership/tiers | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | mem_levels | PUT | /membership/tiers/{id} | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | mem_levels | DELETE | /membership/tiers/{id} | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | mem_levels | PUT | /membership/tiers/{id}/features | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | mem_data | POST | /membership/users/{userId}/assign | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | mem_data | POST | /membership/users/{userId}/cancel | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | access_control | POST | /access/rules | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | access_control | DELETE | /access/rules/{resource} | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | member_admin | PUT | /admin/members/{userId} | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | member_admin | POST | /admin/members/{userId}/assign | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | member_admin | POST | /admin/members/{userId}/cancel | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | stripe_service | POST | /billing/create-customer | checkAuthWithCsrf() → requireValidNonce() | **PASS** | | stripe_service | POST | /billing/create-checkout-session | checkAuthWithCsrf() → requireValidNonce() | **PASS** | | stripe_service | POST | /billing/billing-portal | checkAuthWithCsrf() → requireValidNonce() | **PASS** | | checkout_service | POST | /checkout/create-session | checkAuthWithCsrf() → requireValidNonce() | **PASS** | | checkout_service | POST | /checkout/success | checkAuthWithCsrf() → requireValidNonce() | **PASS** | | order_management | GET | /orders/* (user-facing) | checkAuth() | **PASS** (read-only) | | order_management | POST | /admin/orders/{uuid}/refund | **checkAdminWithCsrf() → requireValidNonce()** — but **namespace broken** | **FAIL** (see below) | | order_management | POST | /admin/orders/{uuid}/partial-refund | **checkAdminWithCsrf() → requireValidNonce()** — but **namespace broken** | **FAIL** (see below) | | webhook_handler | POST | /webhooks/stripe | `__return_true` + CSRF exempt via addExemptRoute() | **PASS** | | billing_admin | POST | /admin/billing/subscriptions/{userId}/cancel | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | | rate_limiter | DELETE | /admin/rate-limits/{key} | checkAdminAccessWithCsrf() → requireValidNonce() | **PASS** | --- ## Critical Findings ### FINDING-001 — CRITICAL: Quadruple-Backslash Namespace References in OrderManagementModule **Severity:** CRITICAL **File:** `/root/profe-modules/order_management/src/OrderManagement/OrderManagementModule.php` **Deployed on staging:** Confirmed at same path. **What's wrong:** Every DI container service ID string in this file uses `\\\\\\\\` (4 backslashes in source code) instead of `\\` (2 backslashes = PHP namespace separator). In PHP single-quoted strings, `\\\\` is parsed as literal `\\`, so these evaluate to double-backslash strings at runtime. Example (line 39-44): ```php return [ 'Profe\\\\CoreUsers\\\\CoreUsersInterface', // Runtime: 'Profe\\CoreUsers\\CoreUsersInterface' ]; ``` But the container was registered against: ```php 'Profe\CoreUsers\CoreUsersInterface' // Single backslash = namespace separator ``` **Impact:** Every `$container->get(...)` and `$container->has(...)` call in this module fails silently with "Service not registered" exceptions. The module is entirely non-functional: - `getDependencies()` returns wrong identifiers - `register()` factory closures fail - `registerMigration()` never finds DbMigrationInterface - `registerEventListeners()` never finds EventBusInterface - `handleMyOrders()`, `handleRefundOrder()`, `handlePartialRefund()` all fail - `checkAdminWithCsrf()` never finds CsrfMiddlewareInterface — **CSRF bypass for refund endpoints** **Evidence — deployed staging code:** ``` docker exec staging-app-1 cat ...OrderManagementModule.php | head -50 'Profe\\\\\\\\CoreUsers\\\\\\\\CoreUsersInterface' ``` **Fix:** Replace ALL `\\\\\\\\` (4 backslashes) with `\\` (2 backslashes) in string literals in this file. Every string referencing a namespace-qualified service ID is affected — approximately 25+ occurrences across getDependencies(), register(), registerMigration(), registerEventListeners(), handleMyOrders(), handleGetOrder(), handleGetInvoice(), handleRefundOrder(), handlePartialRefund(), getProfeUser(), getDiContainer(), and resolveUserUuid(). --- ### FINDING-002 — MEDIUM: X-Forwarded-For Spoofing Bypasses Rate Limiting **Severity:** MEDIUM **File:** `/root/profe-modules/rate_limiter/src/RateLimiter/RateLimiterModule.php:214-219` **What's wrong:** The rate limiter's IP detection trusts the `X-Forwarded-For` header without verifying it came from a trusted proxy: ```php $ip = $request->get_header('X-Forwarded-For') ?? $_SERVER['REMOTE_ADDR'] ?? '127.0.0.1'; // Take the first IP if X-Forwarded-For is a chain if (strpos($ip, ',') !== false) { $ip = trim(explode(',', $ip)[0]); } ``` An attacker can send a spoofed `X-Forwarded-For: 10.0.0.1` header and cycle through arbitrary IPs to bypass rate limits entirely. The rate limiter becomes ineffective against anyone who controls their HTTP headers. **Fix:** Only trust `X-Forwarded-For` from known proxy IPs, or always use `$_SERVER['REMOTE_ADDR']` as the source of truth for client IP, logging any X-Forwarded-For value for debugging but not using it for enforcement. --- ### FINDING-003 — MEDIUM: Duplicate Migration Version Numbers **Severity:** MEDIUM **File:** `/root/profe-modules/migrations/` (multiple files) **What's wrong:** Three SQL migration files have version numbers that collide with other files: - `008_create_profe_brevo_webhook_events.sql` collides with `008_create_profe_orders.sql` - `009_create_profe_mail_queue.sql` collides with `009_create_profe_crm_contacts.sql` - `010_create_profe_email_templates.sql` collides with `010_create_profe_crm_tags.sql` Because `version` has a UNIQUE KEY in `profe_migrations`, only the first file (alphabetically) in each version group gets applied. The second file is silently skipped when its INSERT fails with a duplicate key error. **Impact:** The brevo_webhook_events, mail_queue, and email_templates tables were never created (their SQL files were ignored). These modules (brevo_service, mail_queue, email_templates) are not yet deployed, so the issue is latent. When those modules are deployed, their migration SQL files need unique version numbers. **Fix:** Re-number the colliding files to use unique version numbers before deploying those modules. E.g., brevo_webhook_events → 043, mail_queue → 044, email_templates → 045. --- ### FINDING-004 — MEDIUM: Rate Limiter Uses Fixed-Window, Not Token Bucket **Severity:** MEDIUM **Files:** `/root/profe-modules/rate_limiter/src/RateLimiter/RateLimiter.php` **What's wrong:** The rate limiter's docblock claims "token bucket algorithm" but the implementation is a fixed-window counter: - Redis path: `INCR` + `EXPIRE` (fixed-window at expiry) - DB path: Upsert with time-window comparison A genuine token bucket allows short bursts (up to bucket capacity) while limiting the average sustained rate. A fixed-window counter resets entirely at window boundaries, allowing bursts at the start of each window. This is a documented design choice but violates the M3-5 check which explicitly requires "token bucket algorithm". Also, `rate_limiter.billing_endpoints_enabled` config flag can completely disable rate limiting (line 56-58): ```php if (!$this->config->getBool('rate_limiter.billing_endpoints_enabled', true)) { return true; // Rate limiting disabled globally } ``` **Fix:** Implement true token bucket algorithm (e.g., track remaining tokens and last refill timestamp) or update documentation to match the fixed-window implementation. --- ## Findings by Fault Prevention Rules | Fault | Prevention | Status | Evidence | |-------|-----------|--------|----------| | FAULT-01 | No container stored as class property | **PASS** | All modules use factory closures capturing `$c`. No module stores container as property. | | FAULT-02 | PSR-4 namespace/path match | **PASS** | All 10 modules have correct PSR-4 mappings: namespace maps to `src/{Module}/`. | | FAULT-03 | Event names profe/{domain}/{action} | **PASS** | All events follow pattern: `profe/membership/*`, `profe/billing/*`. Verified 30+ event names. | | FAULT-04 | No $wpdb->prefix on profe_ tables | **PASS** | Zero $wpdb->prefix usage. Table names are bare: `profe_mem_levels`, `profe_users`, etc. | | FAULT-05 | All queries use prepared statements | **PASS** | All repository queries use `:param` named bound parameters. No string interpolation. | | FAULT-06 | Static method calls use `::` not `\` | **PASS** | All static calls use `\Stripe\Customer::create()` format, not `Stripe\Customer::create()`. | | FAULT-07 | Test files with proper bootstrap | **PASS** | Test files exist in all 10 modules (3-6 files each). Prior GS review confirmed 21-29 tests each. | | FAULT-08 | README per module | **PASS** | All 10 modules have README.md. | | FAULT-09 | No stubs — real implementations | **PASS** | All methods have real implementations. No TODO/FIXME/NotImplemented in src/. | | FAULT-10 | Schema in one authoritative SQL file | **FAIL** | Duplicate version numbers (008, 009, 010) mean some SQL files are silently ignored by the migrator. Multiple files per version violates the intent of versioned, single-authoritative-source schema management. | --- ## Recommendations 1. **BLOCKER:** Fix OrderManagementModule quadruple-backslash namespace references. Every `\\\\\\\\` should be `\\`. 2. **REQUIRED BEFORE PRODUCTION:** Fix rate limiter X-Forwarded-For trust issue — use `REMOTE_ADDR` only for enforcement. 3. **RECOMMENDED:** Re-number colliding migration SQL files (008→043, 009→044, 010→045) before deploying brevo/mail_queue/email_templates modules. 4. **RECOMMENDED:** Either implement proper token bucket algorithm or rename the method from "token bucket" to "fixed-window counter" in documentation and docblocks. 5. **NOTE:** The Phase 1 GS conditional issues (missing ProfeContainer, missing ProfeAutoloader, MIGRATION_USER/PASS) have all been resolved — large green checkmarks for the chassis team. --- ## Summary | Gate | Checks Passed | Checks Failed | Verdict | |------|-------------|--------------|---------| | Phase 1 (Re-verify) | 10/10 | 0 | **PASS** | | Module 2 — Membership | 10/10 | 0 | **PASS** | | Module 3 — Billing | 6/8 | 2 | **FAIL** | | FAULT-004 Enforcement | 20/22 | 2 | **FAIL** | | Fault Prevention (10 rules) | 9/10 | 1 | **FAIL** | **The order_management module (T18) is entirely non-functional due to broken namespace references.** This blocks the following site areas: - Site Area 8 (Member Dashboard) — order history broken - Site Area 12 (Checkout) — order creation from payment failures - All admin refund operations **Rate limiting has an X-Forwarded-For spoofing bypass** that undermines FR-021 (Rate limit bypass) prevention. **Verdict: REDLIGHT — Do not proceed until FINDING-001 (OrderManagementModule) and FINDING-002 (Rate limiter IP spoofing) are resolved.**