Memory Wiki

I'll start by running the verification commands and exploring the directory structure of all three modules simultaneously.

All syntax checks pass. Now let me read all source files and test files in detail.

Now let me read the remaining source files and all tests.

Let me read all test files and the exception/repository source files.

Now let me read the remaining test files, exception files, and fixture files.

Let me check the fixtures and search for specific security patterns.

Let me read the fixture files and the SubscriptionRepository.

Let me check one more thing — the StripeService's test for a non-existent private method.

Now I have a complete picture. Let me compile the adversarial review. ## GS CODE REVIEW — Adversarial Sweep Starting verification commands first, then deep-dive into all 3 modules. --- ### VERIFICATION RESULTS ``` php -l: All 12 PHP files pass syntax check (0 errors) grep '\\\\\\\\': 0 occurrences in all modules (FAULT-011 clean) grep 'eval|assert|exec|system|passthru|shell_exec|unserialize': 0 occurrences ``` --- ### T52: Google Ads Conversion — REVISE **What TA approved:** PHP lint ✓, no quadruple-backslash ✓, 17 tests ✓, DI wiring ✓ **What TA missed:** **1. SECURITY: Missing currency code validation (minor)** `buildClickConversion()` passes `strtoupper($currencyCode)` directly to the SDK without validating it's a real ISO 4217 code. Google Ads API will reject bad values, but this creates a different error path than expected. Not exploitable — the SDK is the consumer — but should validate upfront. **2. EDGE CASE: No test for `enhanced_conversions` = false** `buildClickConversion()` checks `$this->config->getBool('google_ads.enhanced_conversions', true)`. When disabled, `user_identifiers` is NOT included even if `$hashedEmail` is provided. **Zero tests cover this config path.** A production site that disables enhanced conversions would silently skip hashed email inclusion — and there's no test proving this works as intended. **3. TEST QUALITY: Weak assertion pattern** `testUploadConversionAcceptsHashedEmail()` and `testUploadConversionAcceptsCustomConversionActionId()` — both verify only that the return type is string. They **never verify what data was actually passed to the (stub) API**. The values asserted are implicit at best. **4. TEST QUALITY: Stub method, not a real integration test** `uploadClickConversion()` on line 183 is a simulation returning a `sprintf()` resource name. The real SDK integration path is commented out. Tests exercise `md5()` + string concatenation, not actual Google Ads API behavior. This is a coverage gap TA should have flagged. **5. ERROR HANDLING: `uploadConversion()` re-catches `GoogleAdsConversionException` then re-throws it unchanged (line 62-63)** This is dead code — the catch does nothing except re-throw. Minor but unnecessary. ### T17: WebhookHandler — REVISE **What TA approved:** Routes correct ✓, idempotency ✓, conversion upload flow ✓, CSRF exemption ✓ **What TA missed:** **1. SECURITY: Unknown `Throwable` silently returns 200 with `success: true` (line 213-214)** `handleStripeWebhook()` catches generic `Throwable` and returns `['success' => true]` with HTTP 200. The comment says this is to "avoid Stripe retry spam" — but this **masks real bugs**. If the DI container fails, a dependency throws, or PHP runs out of memory, Stripe gets a 200 OK and assumes everything worked. This should at minimum log the error and return 500. Current behavior is a monitoring blind spot. **2. EDGE CASE: Non-`GoogleAdsConversionException` failures skip retry queue** In `uploadConversionToGoogleAds()`, the `catch (GoogleAdsConversionException $e)` block (line 360) enqueues a retry. But `catch (Throwable $e)` (line 372) **only logs** — it does NOT call `enqueueConversionRetry()`. This means if `uploadConversion()` throws a `RuntimeException`, `TypeError`, or any non-typed exception, the conversion data is lost permanently with no retry mechanism. **Data loss path.** **3. EDGE CASE: `handleInvoicePaid` bypasses `StripeServiceInterface` abstraction** Line 208: `\Stripe\Subscription::retrieve($invoice->subscription)` — this calls the Stripe SDK directly instead of going through `$this->stripeService`. This breaks the abstraction guarantee (HC-01: "This is the ONLY module that interacts with Stripe"). Every other method delegates through the service layer. **4. TEST QUALITY: `handleSubscriptionUpdated` registered but untested** The route `customer.subscription.updated` → `handleSubscriptionUpdated()` is wired up. There is **zero tests** for this handler. The update handler calls `stripeService->syncSubscription()` and emits `subscription_updated` — no test verifies this. **5. TEST QUALITY: `testConversionUploadFailureEnqueuesRetry` uses `credentialsNotConfigured` exception** The test (line 384) throws `GoogleAdsConversionException::credentialsNotConfigured()` which has code=500. The real failure path for API errors would use `GoogleAdsConversionException::fromApiError()` (also code=500). The catch block catches `GoogleAdsConversionException` generally, so the test passes — but it's testing a credential error scenario rather than an API error scenario. Subtly wrong scenario testing. ### T15: StripeService + CheckoutService — REVISE **What TA approved:** CSRF on POST endpoints ✓, backwards compatibility (hosted default) ✓, GCLID cookie reading ✓, ui_mode validation ✓ **What TA missed:** **1. TEST QUALITY (CRITICAL): False-positive tests for non-existent private methods** Three tests in `StripeServiceTest` invoke private methods via reflection that **do not exist** in the current code: - `testIsLiveModeReturnsTrueForLiveKey()` → calls private `isLiveMode()` — **method removed/renamed**. Actual code uses `$config->getEnvironment() === 'production'`. - `testGetPriceIdReturnsLite()` and `testGetPriceIdReturnsStandard()` → call private `getPriceId()` — **method removed/renamed**. Actual code uses `resolveLevelFromPrice()`. - `testGetPriceIdThrowsForInvalidTier()` → same issue. These tests pass because they catch all `Throwable` in their try/catch blocks. **They are false positives** — they do not test anything meaningful and would crash if not wrapped in try/catch-all. **2. TEST QUALITY (CRITICAL): Try/catch-all pattern masks real failures** Multiple tests (e.g., `testCreateCustomerStoresAndReturnsCustomerId`, `testCreateCheckoutSessionReturnsSessionData`, `testCreateBillingPortalReturnsUrl`, `testCreateCheckoutSessionHostedDefaultDoesNotIncludeEmbeddedParams`, etc.) use this pattern: ```php try { ... } catch (Throwable $e) { $this->assertNotEmpty($e->getMessage()); } ``` This means the test **passes even if the method throws an unexpected exception** — it only checks that the exception message is non-empty. These tests provide zero confidence that the code path works correctly. They are **assertion-free pass-throughs**. **3. TEST QUALITY: `testCreateCustomerStoresAndReturnsCustomerId` passes a stdClass not a ProfeUser** Line 141-145 creates a `new stdClass()` with properties `uuid`, `email`, `display_name`, `user_name`. The actual method signature expects `ProfeUser`. The test calls via reflection so PHP type-hints are bypassed. However, `stdClass->email` is an untyped dynamic property — if this were a real call with strict_types, it would fail. The test works by accident, not by design. **4. SECURITY: `$_GET['gclid']` fallback lacks regex validation** In `CheckoutService::getGclidFromCookie()`: - Cookie path (line 173-184): validates with `preg_match('/^[a-zA-Z0-9_-]+$/', $gclid)` ✓ - GET parameter fallback (line 189-191): **only `trim()` — no regex validation** ✗ While the GCLID flows through Stripe's API (not SQL/shell), this inconsistency means users can inject arbitrary characters into Stripe session metadata via the URL. Not actively exploitable, but violates the principle of consistent input validation. **5. FAULT-002: Duplicated `getProfeUser()` in both modules** Both `StripeServiceModule` (line 385) and `CheckoutServiceModule` (line 261) define an identical `getProfeUser()` method with a direct `profe_users` query. While documented, this is a code smell — a shared helper should exist. TA noted this but didn't flag the code duplication as a maintenance risk. **6. API CONSISTENCY: `getWebhookSecret()` not in interface** `StripeService` exposes `public function getWebhookSecret(): string` (line 452) that is **not declared in `StripeServiceInterface`**. This is a leaky abstraction — consumers depending on the concrete class instead of the interface would break if this method is refactored. **7. EDGE CASE: `assignLevel(0, ...)` passes userId=0** `updateMembershipFromSubscription()` (line 538) calls `$this->memData->assignLevel(0, $levelId, null)` with userId=0, explicitly documented as a placeholder. This means the primary membership update path silently fails (throws caught by `catch (Throwable)`) and falls through to the direct `profe_users` query workaround. The fallback works, but the primary path is dead code. --- ### SUMMARY ``` ### T52: REVISE — Weak test assertions, missing enhanced_conversions=off test, stub-only integration ### T17: REVISE — Silent 200 on Throwable, non-GoogleAdsConversionException data loss, direct Stripe SDK call in handleInvoicePaid, untested subscription.updated handler ### T15: REVISE — 3 false-positive tests testing non-existent methods, try/catch-all pattern in 5+ tests, GET gclid path lacks regex validation, duplicated getProfeUser() ``` **Security concerns:** 1. Webhook handler swallows unknown `Throwable` with 200 + `success:true` — monitoring blind spot 2. `$_GET['gclid']` fallback in CheckoutService lacks regex validation unlike the cookie path 3. `handleInvoicePaid` bypasses StripeServiceInterface abstraction with direct SDK call **Edge cases missed:** 1. Non-`GoogleAdsConversionException` during conversion upload → data lost (no retry enqueued) 2. `enhanced_conversions` config = false → no test coverage 3. `subscription.updated` handler → no test coverage 4. `getAllTiers()` raw `SELECT *` — potential perf issue on large tier sets **Test quality issues:** 1. **3 tests call private methods that don't exist** — false positives masked by try/catch-all 2. **5+ tests use try/catch-all pattern** — effectively assertion-free pass-throughs 3. Test creates stdClass instead of ProfeUser, bypassing type hints via reflection 4. Integration test uses wrong exception type for scenario **Overall: REVISE** — All three modules need test corrections and the critical edge cases addressed before approval. The code logic is sound; the test suite and error handling gaps are the issues.