Memory Wiki

Now let me read the remaining sections of the build plan that are relevant to T18 and T20.

Let me check the actual byte content to confirm the backslash issue is present in the current source.

Let me check the exact byte content of both files to be precise about the backslash counts.

Good, now I have the full picture. Let me verify the RateLimiterModule's backslash counts in its DI strings as well, plus check the plan file raw bytes.

Let me verify there are no other X-Forwarded-For usages in the file and check the RateLimiter's DI strings.

Now I have all the evidence I need for a thorough review. Let me compile my findings. ### FAULT-011 (T18) Plan Review **Root cause verification:** Confirmed by hex analysis. The source file's DI strings have 4 backslash bytes (`5c 5c 5c 5c`) between namespace parts in single-quoted strings, producing `\\` (2 literal backslashes) at runtime instead of the correct `\` (1 backslash). All 27 lines with DI lookups are affected — 56 groups of 4 backslashes total. **Plan coverage verification:** All 27 affected lines are listed in the plan's OLD/NEW tables. The implementation notes correctly warn about not touching `use` imports, `\Throwable` patterns, and the docblock. **Key finding — verification step 2 has a bug:** Both grep commands in steps 1 and 2 search for `\\\\\\\\` (8 backslash bytes in the shell = 4 literal backslashes for grep). After the fix, both would return 0 matches. Step 2 should search for `\\\\` (4 backslash bytes in shell = 2 literal backslashes) to verify the corrected references exist at ~27 lines. As written, a TB following the plan literally would run a useless verification in step 2. ### FAULT-012 (T20) Plan Review **Root cause verification:** Confirmed. The `checkBillingRateLimit()` method at lines 213-220 uses `$request->get_header('X-Forwarded-For')` as the primary IP source, with `$_SERVER['REMOTE_ADDR']` as a fallback only when the header is absent. An attacker can spoof arbitrary IPs by sending any `X-Forwarded-For` value. **Fix verification:** The proposed fix uses `$_SERVER['REMOTE_ADDR']` as the sole enforcement source and logs `X-Forwarded-For` for debugging only. The comma-splitting logic is correctly removed since `X-Forwarded-For` is no longer used for enforcement. All edge cases are handled (CLI fallback to 127.0.0.1, `function_exists('error_log')` guard). No new faults introduced. Test scenarios are comprehensive (6 scenarios covering spoofing bypass, no-header case, multi-IP cycling, debug logging, CLI, and regression). --- ### FAULT-011 (T18) Plan Review **Plan completeness:** PASS — all 27 affected lines covered in OLD/NEW tables, with correct implementation notes about what NOT to touch. **Root cause accuracy:** PASS — correctly identifies the 4-backslash vs 2-backslash PHP single-quoted string escaping issue. **Proposed fix correctness:** PASS — the global find-and-replace of 4→2 backslashes is the correct fix. **Edge cases covered:** PASS — explicitly warns about preserving `use` imports (single backslash), `\Throwable` (2 backslashes), and PHPDoc annotations. All three are safe from the 4→2 replacement. **Test scenarios adequate:** FAIL — verification step 2 grep command searches for the wrong pattern (`\\\\\\\\` = 4 backslashes) when it should search for `\\\\` (2 backslashes). After the fix, both steps 1 and 2 would return 0 matches, making step 2's expected output of "~28 occurrences" unattainable. **Findings:** 1. Step 2 grep command uses the same 8-backslash pattern as step 1. It should use `'\\\\'` (4 backslash bytes in shell = 2 literal backslashes for grep) to verify the corrected 2-backslash references exist. 2. Minor: plan says "~28 occurrences" — actual affected lines are exactly 27. 3. Otherwise well-structured with thorough location-by-location OLD/NEW breakdown. **Verdict: REVISE** — Fix the verification step 2 grep command. Change `grep -n '\\\\\\\\'` to `grep -n '\\\\'` so it searches for 2 backslashes (the correct pattern) instead of 4 backslashes. --- ### FAULT-012 (T20) Plan Review **Plan completeness:** PASS — all aspects of the fix covered: root cause, impact, OLD/NEW code blocks, implementation rationale, verification steps, and test scenarios. **Root cause accuracy:** PASS — correctly identifies X-Forwarded-For being trusted without proxy verification as the root cause of the spoofing bypass. **Proposed fix correctness:** PASS — `$_SERVER['REMOTE_ADDR']` is the unspoofable TCP connection IP. X-Forwarded-For is demoted to debug logging only. The comma-splitting chain logic is correctly removed. All edge cases handled (empty REMOTE_ADDR fallback, error_log guard). **Edge cases covered:** PASS — handles CLI context (undefined REMOTE_ADDR), disabled error_log, and doesn't affect other methods like checkAdminAccessWithCsrf. **Test scenarios adequate:** PASS — 6 test scenarios comprehensively cover spoofed header bypass, no-header case, multi-IP cycling attacks, debug logging verification, CLI fallback, and regression testing. **Findings:** - No issues found. The plan is correct, complete, and ready for TB to implement. - The "Future Consideration" section appropriately documents a known path (trusted proxy whitelist) without adding scope creep. - Verification steps are correct and include a grep to confirm X-Forwarded-For is only used in debug logging. **Verdict: APPROVE**