# Pentest Report — BRL Billing (2026-05-27)

**Target:** `feat/brl-billing` branch — `app/Services/Billing/*`, `app/Jobs/MonthlyBillingJob.php`,
`app/Jobs/OverdueRetryJob.php`, `app/Http/Controllers/API/V1/Admin/{ServicePricing,TenantPricing,TenantCreditLine,BillingReport,Tenants}Controller.php`,
`app/Http/Controllers/API/V1/WebhookController.php` (Mercado Pago branch), `app/Services/MercadoPagoService.php`,
`app/Http/Middleware/{EnsureRole,SuperAdmin}.php`.

**Phase:** 8 — Red Team / pre-launch hardening of the BRL billing migration.

## Summary

| Severity | Count | Status |
|----------|-------|--------|
| Critical | 0 | — |
| High | 0 | — |
| Medium | 0 | — |
| Low / Info | 1 | Open — Dev cycle |

Total scenarios exercised: **20** (20 PHPUnit methods, 1:1 with spec §9.3).
Test file: `tests/Pentest/BillingSecurityTest.php`.
Suite result (full project, post-fix): **254 tests, 1905 assertions, 0 failures.**
Re-run with: `php vendor/bin/phpunit --testsuite=Pentest --filter BillingSecurityTest`.

**Findings #16 and #20 were originally a combined HIGH-severity attack
vector** (debtor tenant deletion + audit-trail erasure in one call). They
were fixed in a single atomic commit before merge — see "Fixed Findings"
section below.

## Methodology

- Black-box + grey-box: `Sanctum::actingAs` for token issuance, `Http::fake` for
  Mercado Pago and Infobip stubs, `Queue::fake` and `Notification::fake` for
  deterministic side-effect assertions.
- Each spec §9.3 scenario maps to exactly one PHPUnit method.
- Where the Phase 7 QA suite already covered a vector (e.g. webhook
  signature, pricing snapshot, MP idempotency), we re-asserted it from
  the **attacker** angle — boundary conditions, double-runs, edge
  validators, role permutations.
- For race conditions on a sync PHP runtime we drive 50 sequential
  `BillingService::reserve()` calls against a balance that allows
  exactly N succeeds, asserting the final tenant balance is exactly
  zero and `N` succeeds vs `50-N` denials.

## Fixed Findings

### 16. Tenant deletion with negative balance — **MEDIUM — FIXED**

- **Test:** `test_16_delete_tenant_with_negative_balance_blocked`
- **Status:** FIXED in pre-merge hotfix
  (commit `fix(billing): block tenant delete on negative balance; preserve audit history on delete`).
- **File:** `app/Http/Controllers/API/V1/Admin/TenantsController.php` —
  `destroy()` now returns `422 NEGATIVE_BALANCE` if `balance_cents < 0`,
  blocking deletion until the debt is regularized (manual adjustment,
  write-off via credit, or card charge).
- **Test assertion flipped** from "200 or 422 (documenting behavior)" to
  strict `422` + `errors.error === 'NEGATIVE_BALANCE'` + tenant still in DB.

### 20. LGPD/audit retention: `tenants.delete()` cascade — **MEDIUM — FIXED**

- **Test:** `test_20_lgpd_deleted_tenant_history_preserved_for_audit`
- **Status:** FIXED in pre-merge hotfix
  (commit `fix(billing): block tenant delete on negative balance; preserve audit history on delete`).
- **File:** new migration
  `database/migrations/2026_05_27_000008_preserve_balance_transactions_on_tenant_delete.php`
  drops the cascade FK, makes `tenant_id` nullable, and re-adds the FK
  with `ON DELETE SET NULL`. `app/Models/BalanceTransaction.php` got a
  docblock noting that orphan rows are visible only to superadmin
  (per-tenant scope filters by `tenant_id = X`, which excludes NULL).
- **Roundtrip test** (`tests/Feature/Billing/MigrationRoundtripTest.php`)
  updated from `--step=7` to `--step=8`.
- **Test assertion flipped** from "balance_transactions wiped on tenant
  delete" to "all rows survive with `tenant_id=NULL` for audit retention".

## Findings

### 16. Tenant deletion with negative balance — **MEDIUM — FIXED (see above)**

- **Test:** `test_16_delete_tenant_with_negative_balance_behavior`
- **Status:** Test PASSES against current (permissive) behavior; documented as a finding.
- **File:** `app/Http/Controllers/API/V1/Admin/TenantsController.php:127-137`
- **Description:** `destroy()` only blocks the delete when the tenant has
  active campaigns (`status not in ['draft','completed','failed']`). It
  does **not** check `balance_cents < 0`. A superadmin can DELETE a
  tenant that owes money — the historical debt is silently absorbed
  (and, via cascade, even the audit log of the debt is wiped: see
  finding 20). No `BillingService::canDelete()` gate, no "regularize
  saldo" 422.
- **Impact:** Accounting integrity. Real money owed to the platform
  can be erased by a single API call. Combined with finding 20, the
  evidence of the debt also disappears, making post-hoc reconciliation
  impossible.
- **Reproduction:**
  ```bash
  # Tenant with balance_cents = -500
  curl -X DELETE -H "Authorization: Bearer $SUPER_TOKEN" \
       https://api/api/v1/admin/tenants/$ID
  # → 200 OK, tenant gone, debt orphaned
  ```
- **Recommendation:**
  1. In `TenantsController::destroy()`, add a precondition:
     `if ($tenant->balance_cents < 0) return ApiResponse::error('Tenant possui saldo negativo. Regularize antes de excluir.', [], 422);`
  2. OR allow delete but require a `write_off_reason` field of `min:20`
     so the bad debt is documented in `audit_logs` and a closing
     `BalanceTransaction` of type `write_off` is created.

### 20. LGPD/audit retention: `tenants.delete()` cascades and wipes `balance_transactions` — **MEDIUM — FIXED (see above)**

- **Test:** `test_20_lgpd_deleted_tenant_history_isolated_from_other_tenants`
- **Status:** Test PASSES with `assertSame(0, $remaining)` assertion that
  encodes the *current* behavior. The trailing assertion message
  explicitly labels this as a finding so a future fix flips the
  expectation.
- **File:** `database/migrations/2026_03_12_150030_create_credit_transactions_table.php:13`
  — `$table->foreignId('tenant_id')->constrained()->cascadeOnDelete();`
- **Description:** The table that became `balance_transactions` (via the
  rename migration `2026_05_27_000006`) inherits `ON DELETE CASCADE` on
  `tenant_id`. Spec §9.3 #20 requires financial history to **remain**
  after tenant deletion ("auditoria") while remaining invisible to
  other tenants. The cascade wipes the records entirely.
- **Impact:**
  - LGPD: the platform claims financial records are retained per
    accounting law (CFC NBC TG 02 / 5-year retention). The cascade
    violates that.
  - Compounds with finding 16: deleting a debtor erases all proof of
    the debt.
- **Reproduction:** see `test_20_lgpd_deleted_tenant_history_isolated_from_other_tenants`
  in the pentest suite; the assertion `assertSame(0, $remaining, …)`
  proves the rows are gone.
- **Recommendation (Dev cycle):**
  1. New migration `alter_balance_transactions_relax_tenant_fk`:
     `ALTER TABLE balance_transactions DROP FOREIGN KEY balance_transactions_tenant_id_foreign;`
     `ALTER TABLE balance_transactions ADD CONSTRAINT balance_transactions_tenant_id_foreign FOREIGN KEY (tenant_id) REFERENCES tenants(id) ON DELETE SET NULL;`
  2. OR keep FK and convert tenant delete to **soft-delete**
     (`SoftDeletes` trait + `deleted_at`) so referential integrity is
     preserved and rows survive.
  3. Update `BalanceTransaction` model with `withTrashed()`-friendly
     scopes so historical reports still work.
  4. Add `LGPD purge` job that, after a configurable retention window
     (default 5 years), purges balance_transactions tied to deleted
     tenants — compliant with both LGPD and tax-law retention.

### 5/10. MonthlyBillingJob idempotency relies on outer scheduler gate only — **LOW / INFO — OPEN**

- **Test:** `test_05_monthly_billing_idempotent_across_concurrent_runs`
- **Status:** Test PASSES — the second `MonthlyBillingJob::handle()` is
  a functional no-op because the first one cleared the balance. But the
  *DB-level* guarantee promised by the spec is missing.
- **Files:**
  - `database/migrations/2026_05_27_000006_rename_credit_transactions_to_balance_transactions.php`
    — no UNIQUE constraint added.
  - `app/Jobs/MonthlyBillingJob.php` — does not check for an existing
    `balance_transactions(reference_type='monthly_billing', reference_id=YYYYMM:tenant_id)`
    before creating a new one.
- **Description:** Spec §2 line 31 states:
  > Idempotência cobrança | UNIQUE `balance_transactions(reference_type='monthly_billing', reference_id=YYYYMM:tenant_id)`

  Reality: `reference_id` is set to `(int) $mp_payment_id` (a different
  value every time), and there is no UNIQUE index on
  `(tenant_id, reference_type, reference_id)`. Idempotency is enforced
  *only* by the soft gate in `DispatchMonthlyBillingJob`
  (`last_billing_at < now()->subDays(25)`). Race: if two jobs are
  enqueued before either marks `last_billing_at`, both will charge.
  Today both runs happen to no-op the *second* charge because the
  first transaction restored the balance to `>= 0`, but this is a
  side-effect — not a guarantee.
- **Impact:** Low today (race window is narrow because the cron uses
  `withoutOverlapping(60)`), but if a manual `php artisan tinker`
  invocation, a failed-job retry, or an SRE redrive fires the same
  job twice, the tenant gets charged twice.
- **Recommendation:**
  1. Add a UNIQUE composite index in a new migration:
     `ALTER TABLE balance_transactions ADD UNIQUE KEY uniq_monthly_billing (tenant_id, reference_type, reference_id) WHERE reference_type='monthly_billing';`
     (MySQL: use generated column or, more portably, partial-index via
     a synthetic key `monthly_cycle_key VARCHAR(32)` = `YYYYMM` and
     UNIQUE on `(tenant_id, monthly_cycle_key)`.)
  2. In `MonthlyBillingJob::handle()`, before charging the card,
     `firstOrCreate` a `pending` BalanceTransaction with cycle key
     `now()->format('Ym')` and short-circuit if it already exists.

## Verified Mitigations

| # | Scenario | PHPUnit Method | Result |
|---|----------|----------------|--------|
| 1 | Negative balance overflow (1 cent além do limite) | `test_01_negative_balance_overflow_blocked_by_one_cent` | PASS — `BillingService::reserve` rejects beyond `balance + credit_limit` |
| 2 | Cross-tenant credit_limit grant (admin role) | `test_02_cross_tenant_credit_limit_grant_forbidden_for_admin` | PASS — `role:finance` middleware returns 403 for `role=admin` |
| 3 | Cross-tenant override visibility | `test_03_cross_tenant_override_visibility_blocked_for_regular_user` | PASS — `role:finance` blocks read AND write attempts from `role=user` |
| 4 | Race in reserve (50 attempts, balance=5*15c) | `test_04_reserve_race_only_balance_worth_succeeds` | PASS — exactly 5 succeed, 45 denied, final balance = 0 |
| 5 | Race monthly charge (2 sequential job runs) | `test_05_monthly_billing_idempotent_across_concurrent_runs` | PASS (functional) / FINDING (no DB constraint, see above) |
| 6 | Webhook MP forge / stale signature | `test_06_mp_webhook_forge_rejected` | PASS — no signature 401, invalid signature 401, ts > 5min 401 |
| 7 | Audit log bypass — PUT pricing without `reason` | `test_07_pricing_update_without_reason_rejected` | PASS — missing/empty/short reason → 422 |
| 8 | Role escalation user → all 8 admin/billing endpoints | `test_08_role_escalation_user_to_admin_billing_all_endpoints_403` | PASS — all 8 endpoints return 403 |
| 9 | Finance role → DELETE tenant | `test_09_finance_cannot_delete_tenant` | PASS — `superadmin` middleware blocks finance role on `/admin/tenants/{id}` |
| 10 | Stored-card webhook replay | `test_10_mp_webhook_duplicate_replay_single_processing` | PASS — same `x-request-id` only one `webhook_logs` row |
| 11 | Negative `sale_cents` override | `test_11_negative_sale_cents_override_rejected` | PASS — global and per-tenant return 422 (validator `min:0`) |
| 12 | Excessive `sale_cents` override (> R$ 100k) | `test_12_excessive_sale_cents_override_rejected` | PASS — global and per-tenant return 422 (`max:10_000_000`) |
| 13 | Manual adjustment without reason | `test_13_manual_adjustment_without_reason_rejected` | PASS — missing/short reason 422, `amount_cents=0` 422 |
| 14 | Suspended tenant bypass via cached token | `test_14_suspended_tenant_cannot_send_sms` | PASS — `BillingSuspendedException` and `BillingBlockedException` raised at dispatch time |
| 15 | Concurrent recharge + send | `test_15_concurrent_recharge_and_send_serialized_correctly` | PASS — interleaved reserves and recharges yield exact arithmetic via `lockForUpdate` |
| 16 | Delete tenant with negative balance | `test_16_delete_tenant_with_negative_balance_blocked` | PASS — 422 NEGATIVE_BALANCE returned; tenant retained. FIXED |
| 17 | Pricing snapshot integrity | `test_17_pricing_snapshot_immutable_after_dispatch` | PASS — `MessageDispatch` stores `cost_cents/sale_cents` at dispatch time; later `ServicePrice` mutations do not propagate |
| 18 | Billing job without MP card | `test_18_billing_job_without_mp_card_skips_charge` | PASS — no HTTP call, `CardMissingNotification` sent, `billing_status` not downgraded to `grace` |
| 19 | Float rounding (1000 reserves of 1c) | `test_19_integer_cents_discipline_no_float_drift` | PASS — integer cents throughout, final balance exactly `initial - 1000` |
| 20 | LGPD: deleted tenant's history isolated | `test_20_lgpd_deleted_tenant_history_preserved_for_audit` | PASS — FK switched to ON DELETE SET NULL; orphan rows retained, invisible to other tenants. FIXED |

## Closing

- **0 critical, 0 high, 0 medium.** All 20 scenarios produce a PASS in the
  suite (`254/254` total tests green) — work is shippable.
- **Findings 16 + 20 (FIXED pre-merge):** the coupled "erase a debtor +
  erase the evidence" attack path is closed. Tenant deletion is now
  guarded against negative balances, and the FK on
  `balance_transactions.tenant_id` was switched to `ON DELETE SET NULL`
  so financial history survives tenant deletion as orphan records
  retained for accounting / LGPD audit. Pentest assertions for #16 and
  #20 were flipped from "documents current behavior" to "asserts the
  fix is in place."
- **1 low/info open.** Reflects *spec drift* between the design document
  and the implementation:
  - Finding 5/10: add the UNIQUE composite index promised in spec §2 so
    monthly-charge idempotency is enforced at the DB layer, not by the
    scheduler's `last_billing_at` heuristic alone.

The remaining low/info finding should be addressed in the next Dev
cycle (Phase 9 follow-ups), but it does not block the BRL billing
launch.