# Pentest Report — Messaging APIs (2026-05-26)

**Target:** `feat/messaging-apis` branch — `app/Http/Controllers/API/V1/Messaging/*`,
`app/Services/Messaging/*`, `app/Http/Controllers/API/V1/InboundWebhookController.php`,
`app/Http/Controllers/API/V1/WebhookController.php`.

**Phase:** 4 — Red Team / pre-launch hardening.

## Summary

| Severity   | Count | Status      |
|------------|-------|-------------|
| Critical   | 0     | —           |
| **High**   | 2     | **FIXED**   |
| Medium     | 0     | —           |
| Low / Info | 1     | by design   |

- Total scenarios exercised: **20** (22 PHPUnit methods).
- Test file: `tests/Pentest/MessagingSecurityTest.php`.
- Suite result (full project, after fixes): **174 tests, 591 assertions, 0 failures.**
- Both HIGH findings were patched in commits `24b98ca6` (PhoneNormalizer) and `db7b5753` (EmailController::sanitizeHtml).
- Re-run with `./vendor/bin/phpunit --testsuite=Pentest`.

## Methodology

- Black-box + grey-box: Sanctum acting-as for token issuance, `Http::fake`
  for provider stubs, `Queue::fake` to keep behaviour deterministic.
- Each spec section-8 item maps to one (or two) PHPUnit method(s) in
  `tests/Pentest/MessagingSecurityTest.php`. Scenarios where Phase-3 QA
  already covered the happy path are exercised here on the **edges**
  (case/whitespace bypass, unquoted attribute XSS, cross-token
  rate-limit sharing, etc.).
- Static review (regex over source) for constant-time secret comparison
  (`hash_equals`) — scenario 20.

## Findings

### 6b. Opt-out bypass via missing "+" prefix on SMS — **HIGH — FIXED (commit `24b98ca6`)**

- **Test:** `test_06b_optout_bypass_sms_formats_rejected`
- **Status:** PASS (after fix)
- **Description:** `App\Services\Messaging\PhoneNormalizer::e164()` decides
  whether the input is "already international" by checking for a literal
  leading `+` or `00`. An opt-out stored as `+5521999998888` is **not**
  matched by a subsequent send to `5521999998888` (no `+`), because the
  normalizer treats the latter as a national-format number and produces
  `+555521999998888` (prepends the default country code 55 onto the
  already-present 55). Hash lookup misses → opt-out bypassed.
- **Impact:** LGPD violation, regulatory exposure (CONAR / Anatel SMS
  rules), reputational damage. Any caller — internal or external — can
  send to an opted-out destination by simply omitting the `+`.
- **Reproduction:** `tests/Pentest/MessagingSecurityTest.php::test_06b_optout_bypass_sms_formats_rejected`
  — variants `'5521999998888'`, `'+5521 99999-8888'`, `'55 21 9 9999 8888'`,
  `'+55(21)99999-8888'`. The first variant returns 202 (bypass);
  the others (with `+` already present) are correctly rejected.
- **Recommendation:**
  - Detect 12–13 digit BR numbers (starting with `55`) and treat as
    already international even without the `+`.
  - Better: defer to `giggsey/libphonenumber-for-php` (already widely used
    by Laravel apps) and call `parse(input, 'BR')` so any BR-local /
    BR-international representation normalises to the same E.164.
  - Add a unit test in `tests/Unit/Messaging/PhoneNormalizerTest.php`
    asserting `e164('5521999998888') === '+5521999998888'`.

### 8. XSS sanitiser leaves unquoted event-handler attributes — **HIGH — FIXED (commit `db7b5753`)**

- **Test:** `test_08_xss_email_content_sanitized`
- **Status:** PASS (after fix). HTMLPurifier still recommended for production hardening.
- **Description:** `App\Http\Controllers\API\V1\Messaging\EmailController::sanitizeHtml()`
  uses two regexes for inline event handlers:
  ```php
  preg_replace('#\son\w+\s*=\s*"[^"]*"#i', '', $clean); // double-quoted
  preg_replace('#\son\w+\s*=\s*\'[^\']*\'#i', '', $clean); // single-quoted
  ```
  Neither matches **unquoted** values, so `<img src=x onerror=alert(1)>`
  passes through untouched and is persisted verbatim in
  `message_dispatches.content`. The render-side delivery (Infobip Email
  / Laravel Mail) will render this as live HTML.
- **Impact:** Stored XSS in any inbox that renders HTML email (most
  webmail clients sandbox <script>, but `onerror` on `<img>` is still
  executed by older clients and by previews in custom integrations).
- **Reproduction:** `tests/Pentest/MessagingSecurityTest.php::test_08_xss_email_content_sanitized`
  vector `<img src=x onerror=alert(1)>` — passes through sanitiser; the
  `onerror=alert(1)` substring survives.
- **Recommendation:**
  - Replace the homegrown regex with **HTMLPurifier** (`ezyang/htmlpurifier`)
    or **DOMPurify-server** for HTML email content. The comment in
    `sanitizeHtml()` already acknowledges this ("consider HTMLPurifier
    for production").
  - As a quick stop-gap, add a third regex for unquoted values:
    `'#\son\w+\s*=\s*[^\s>]+#i'` and a generic
    `'#\sjavascript\s*:#i'` — but a proper parser is strongly preferred.

### 11b. Unsubscribe token works cross-tenant — **INFO / by design**

- **Test:** `test_11b_unsubscribe_token_works_cross_tenant_by_design`
- **Status:** PASS (documents intended behavior)
- **Description:** `UnsubscribeController` looks up `MessageDispatch` by
  `unsubscribe_token` **without** scoping by tenant. This is by design —
  the URL is the capability; whoever holds it can unsubscribe. Tokens
  are 64-char `Str::random()` (~256 bits of entropy) and `unique` in DB.
  Verified the random brute test (`test_11`) returns 404 as expected.
- **Recommendation:** Keep current behavior. Documenting only — if the
  policy ever changes (e.g., require auth to unsubscribe from owner
  tenant), update `test_11b` accordingly.

## Verified Mitigations (passing)

| # | Scenario | Test |
|---|----------|------|
| 1 | Authn bypass — 401 without token | `test_01_authn_bypass_returns_401_without_token` |
| 2 | IDOR — uniform 404 across tenants | `test_02_idor_cross_tenant_dispatch_returns_uniform_404` |
| 3 | Token scope — `messaging:sms` can't post voice | `test_03_token_scope_bypass_voice_denied_for_sms_token` |
| 4 | Idempotency replay / conflict | `test_04_idempotency_abuse_same_key_different_payload_returns_409` |
| 5 | Credit race — exactly N succeed, balance 0 | `test_05_credit_race_limits_dispatches_to_balance` |
| 6 | Opt-out email — case/whitespace bypass blocked | `test_06_optout_bypass_email_case_and_whitespace_rejected` |
| 7 | SSRF — internal IP / non-https / file:// blocked | `test_07_ssrf_audio_url_blocks_internal_and_non_https` |
| 9 | Header injection — CR / LF / CRLF stripped from subject | `test_09_header_injection_email_subject_stripped` |
| 10 | Webhook secret — no header / wrong / empty Bearer → 401 | `test_10_webhook_secret_forgery_returns_401` |
| 11 | Unsubscribe — random 64-char hex → 404 | `test_11_unsubscribe_random_token_returns_404` |
| 12 | Rate-limit shared bucket across tokens in same tenant | `test_12_rate_limit_shared_across_tokens_same_tenant` |
| 13 | Quiet hours + past `scheduled_for` rejected | `test_13_quiet_hours_and_past_schedule_rejected` |
| 14 | Opt-out probing — 50 attempts, 0 credits debited | `test_14_optout_probing_does_not_leak_credits` |
| 15 | Token never appears in any captured log entry | `test_15_request_authorization_token_not_logged` |
| 16 | LGPD — peer in same tenant sees masked content | `test_16_peer_in_same_tenant_sees_masked_content` |
| 17 | Stateless `auth:sanctum` — no CSRF token required | `test_17_stateless_bearer_auth_requires_no_csrf_token` |
| 18 | Phone validation — null byte / SQL / CRLF normalised or rejected, raw never persisted | `test_18_phone_validation_rejects_dangerous_input` |
| 19 | Mass-assignment — extra fields ignored (tenant_id, status, etc.) | `test_19_mass_assignment_extra_fields_ignored` |
| 20 | `hash_equals` used in both webhook controllers (static review) | `test_20_webhook_secret_comparison_uses_constant_time` |

## Known limits (documented, no regression)

- **Premium-number block (#18):** no `^+55090` / similar deny-list yet.
  Acceptable for v1 (handled at provider level by Infobip pricing); add
  a tenant-level "destination prefix deny-list" in Phase 5.
- **Quiet-hours override header:** `X-Quiet-Hours-Strategy` is accepted
  from the request, which lets any token holder convert reject→schedule.
  Mitigated by per-tenant config in the spec but worth re-validating in
  Phase 5 (token ability gate for the override).

## Closing

Both HIGH findings were patched in the same Phase 4 cycle:

1. **PhoneNormalizer** now detects 12–13 digit numbers starting with the
   default country code (e.g. `55…`) and treats them as already
   international, eliminating the opt-out bypass. Commit `24b98ca6`.
2. **EmailController::sanitizeHtml** now strips unquoted event handlers
   (added pattern `#\son\w+\s*=\s*[^\s>]+#i`) and neutralises
   `javascript:` URIs in `href`/`src` (quoted + unquoted). HTMLPurifier
   is still the recommended production hardening — listed in follow-ups.
   Commit `db7b5753`.

All 20 scenarios are now GREEN. Total: **174 tests / 591 assertions / 0
failures.** The pentest suite remains in the repository as a permanent
regression gate.