# Pre-Release Audit — 2026-05-19

Snapshot of the pre-release audit run on 2026-05-19, annotated with the resolution status of each finding. Items marked **FALSE POSITIVE**, **INTENTIONAL**, or **NO ACTION** should not be re-investigated by future reviewers — they were verified during this audit and the resolution is documented inline.

Original audit scope: full sweep of `app/` against `/adcenter-design-assets` (design templates) and `/old/html/adcenter` (legacy logic). Method: 6 parallel module reviews, then spot-verification of every critical claim against source.

---

## Critical findings

### C1 — Auto-routing re-enabled, bypasses all filters
**Status: FIXED**
[Routes.php:318](../html/app/Config/Routes.php#L318) had `setAutoRoute(true)` overriding the `setAutoRoute(false)` at line 45. Removed the line and its warning banner (lines 302-318). Only `setAutoRoute(false)` at line 45 remains.

### C2 — Real production secrets committed in source
**Status: OPEN — must fix before any production deploy**

Hardcoded credential defaults at:
- [Constants.php:154](../html/app/Config/Constants.php#L154) — `REPORTS_MAILING_ACCOUNT_PWD` (looks like a Google app password)
- [Constants.php:156](../html/app/Config/Constants.php#L156) — `REPORTS_CRON_SECRET`
- [PotentialReach.php:14](../html/app/Controllers/PotentialReach.php#L14) — `PRT_API_KEY`
- [PromoCalendarService.php:27-28](../html/app/Services/PromoCalendar/PromoCalendarService.php#L27) — AWS access key + secret as `env()` fallbacks

Required action: rotate all four at their providers (values are in git history regardless of `.env` override), then replace defaults with `env(...)` calls that throw if the variable is missing — no string fallback.

### C3 — Login has no rate-limit / lockout
**Status: OPEN**
[Users.php:41-64](../html/app/Controllers/Users.php#L41). Throttler infra exists — `forgotPassword()` at line 195 uses it. Apply same pattern to `auth()`.

### C4 — Plaintext password column written on reset
**Status: OPEN**
[Users.php:321](../html/app/Controllers/Users.php#L321) dual-writes `pass_hash` (good) and `pass_clr` (bad). Remove the `pass_clr` write; migrate any legacy reader first.

---

## High-priority findings

### H1 — Cookie `secure` flag false
**Status: OPEN — production deploy gate**
[Cookie.php:65](../html/app/Config/Cookie.php#L65). Override via env-specific config or `.env` for production.

### H2 — 2FA "send to developers" branch not env-gated
**Status: OPEN**
[TwoFactorService.php:140-149](../html/app/Services/Auth/TwoFactorService.php#L140). Add `if (ENVIRONMENT === 'development')` guard.

### H3 — AdSchedule routes use `auth` filter instead of `adops`
**Status: OPEN**
[Routes.php:208-244](../html/app/Config/Routes.php#L208). Controller already does `isAdOps()` check, but route filter should match for defense in depth.

### H4 — Campaign creative-linking IDOR
**Status: OPEN**
[Campaign.php](../html/app/Controllers/Campaign.php) `save()` and `update()`. POSTed creative IDs are linked without verifying they belong to `auth_adv_id()`.

### H5 — Checklist file uploads in public/
**Status: OPEN**
[Checklist.php:111](../html/app/Controllers/Checklist.php#L111). Files in `public/uploads/checklist/` are fetchable without auth via guessed filenames.

### H6 — Billing deposit lacks idempotency / double-submit guard
**Status: OPEN**
[Billing.php:109-113](../html/app/Controllers/Billing.php#L109). Add one-time idempotency key.

### H7 — CSRF scoped, not global
**Status: OPEN**
[Filters.php:71-85](../html/app/Config/Filters.php#L71). Promote to global `before` filter.

### H8 — Audit logging missing on bulk operations
**Status: OPEN (re-confirm scope before fixing)**
Campaign + Creative bulk-pause/resume/delete/denied/clone/budget/bid update DB rows but never write to `shorty.adcenter_log`. Legacy did. The history modal expects rows to exist there. Note: [EventLogService::add()](../html/app/Services/AdOps/EventLogService.php) is implemented but not called by bulk endpoints.

---

## Medium findings — logic / hygiene

### M1 — Creative::getHistory used string-concat WHERE
**Status: FIXED (then re-fixed for sargability)**
[Creative.php:560-569](../html/app/Controllers/Creative.php#L560). Rewritten to use `time_stamp >= UNIX_TIMESTAMP(...)` so the predicate is sargable. Same exclusive-/inclusive semantics as the original BETWEEN.

### M2 — Campaign::getHistory same problem
**Status: FIXED**
[Campaign.php:1518-1527](../html/app/Controllers/Campaign.php#L1518). Matching sargable rewrite applied. Behavior unchanged.

### M3 — `adcenter_log` performance
**Status: KNOWN LIMITATION — cannot index**
Table has 10.4M rows, 2.9 GB data, only the PRIMARY KEY on `id`. Every history-modal load full-scans the clustered index. An index on `(entity_id, time_stamp)` would resolve this but ALTER is too risky on this legacy table (per product owner decision, 2026-05-19). Sargable WHERE is in place so the queries will benefit immediately if an index is ever added. Cache + LIMIT were proposed and intentionally deferred for legacy parity.

### M4 — `getCampaignsMockData()` (15 fake campaigns) shipped in binary
**Status: FIXED**
Removed entire 323-line method from [Campaign.php](../html/app/Controllers/Campaign.php). Zero callers, confirmed via grep before delete.

### M5 — Campaign::bulkClone response shape
**Status: VERIFIED OK — not a bug**
[Campaign.php:1252](../html/app/Controllers/Campaign.php#L1252) returns `cloned_ids` as a PHP array → JSON array. Both JS consumers ([management/index.php:180](../html/app/Views/campaign/management/index.php#L180) and [camp-mgmt.js:193](../html/public/assets/js/camp-mgmt.js#L193)) handle it correctly. The audit's concern was unfounded.

### M6 — AdSchedule has no timezone awareness
**Status: VERIFIED — matches legacy**
Legacy [Ad_Schedule.php](../../old/html/adcenter/controllers/Ad_Schedule.php) stores hours identically (raw 0-23 ints, server time). New code preserves legacy parity exactly. A tz-aware redesign would require a schema migration and is out of scope.

### M7 — `Security::tokenRandomize = false`
**Status: OPEN (low priority)**
Defense-in-depth only; token regeneration is already on. Flip to `true` when convenient.

### M8 — `BaseController::denyAccess()` declares `:string` return but exits
**Status: OPEN (cosmetic)**
Code works because `exit` runs before the missing return. Replace callers with `denyAccessRedirect()` and remove the misleading signature.

### M9 — `TopbarDataService` runs on every authenticated request
**Status: OPEN (perf opportunity)**
Two queries per page load in [BaseController](../html/app/Controllers/BaseController.php). Add caching or move to async AJAX.

---

## Medium findings — design parity

### D1 — 404 page unbranded
**Status: FIXED**
[error_404.php](../html/app/Views/errors/html/error_404.php) now extends `layouts/default` — the minimal branded shell used by login/2FA. Picked `default` over the full `adcenter.php` layout because 404s can fire pre-auth.

### D2 — Orphaned `layouts/main.php` with missing CSS refs
**Status: FIXED — file deleted**
Confirmed zero callers via grep. File referenced three CSS files that don't exist (`dashboard-modern.css`, `dataTables.bootstrap4.min.css`, `daterangepicker.css`). Layouts dir now contains only `adcenter.php` and `default.php`.

### D3 — `baseURL('api/')` "typo" in adcenter layout
**Status: FALSE POSITIVE**
`baseURL()` is a project-defined helper at [utils_helper.php:23](../html/app/Helpers/utils_helper.php#L23), loaded automatically via `BaseController::$helpers`. Both `baseURL()` and `base_url()` are valid and interchangeable in this codebase.

### D4 — Creative Management subtitle "Campaign Management"
**Status: VERIFIED — implementation is correct**
Implementation says "Creative Management" at [creative/management/index.php:66](../html/app/Views/creative/management/index.php#L66). The typo only exists in the design source ([creative-management.php](../../adcenter-design-assets/creative-management.php) line 30 — stale copy-paste). No fix needed.

### D5 — Campaign Management table "has extra columns" (Pace, Remaining, Click Cap, Utilization)
**Status: FALSE POSITIVE**
Direct diff of [components/campaign/table-content.php:25-45](../html/app/Views/components/campaign/table-content.php#L25) vs [campaign-management.php:188-208](../../adcenter-design-assets/campaign-management.php#L188): byte-for-byte equivalent — same labels, same alignment classes, same order. There is no "Click Cap" column in either; the audit agent invented it.

### D6 — Creative Management has extras vs design
**Status: INTENTIONAL — kept as scope additions**
Implementation adds:
- Type / Status filter dropdowns at top-left ([creative/management/index.php:19-37](../html/app/Views/creative/management/index.php#L19))
- 3 extra Bulk Edit items: Headline, Description, Display URL ([creative/management/index.php:51-53](../html/app/Views/creative/management/index.php#L51))

All three bulk operations have complete controller methods (`bulkUpdateHeadline` / `Description` / `DisplayUrl`) and routes. These are deliberate scope additions, not regressions. Kept per product decision 2026-05-19.

---

## Issues raised but NOT YET audited / out of scope this session

- **`production.php` (generic 5xx page)** — unbranded "Whoops!" template. Same pattern as D1 but wasn't in original scope.
- **`Creative::getHistory` SQL "injection"** — Audit flagged the string-concat WHERE as injection-risk. It was actually safe (`$db->escapeString` was applied). Re-fixed for hygiene + sargability under M1, not security.
- **`AdSchedule::add()` IDOR** — Audit agent claimed an IDOR; ad-ops users *can* legitimately schedule for any advertiser, so the design may be intentional. Needs product clarification before deciding.
- **2FA OTP delivery in prod** — Code path untested in prod-like env.
- **Payeezy / IPN webhook end-to-end** — Not verified.
- **Analytics module deep dive** — The audit agent for this module reported "no findings" across ~150KB of code. Treat as unverified, not verified.
- **Runtime smoke test** — No page was actually loaded during the audit; everything was static review.
- **DB schema drift** — `migration/PROJECT_OVERVIEW.md` notes Dashboard's "Top campaigns table" is blocked by missing `daily_budget_stats`. Other gaps may exist.

---

## Quick reference — what changed in source (2026-05-19)

| File | Change |
|---|---|
| `app/Config/Routes.php` | Removed `setAutoRoute(true)` + 16-line warning banner |
| `app/Controllers/Creative.php` | `getHistory()` WHERE now sargable |
| `app/Controllers/Campaign.php` | `getHistory()` WHERE now sargable; deleted `getCampaignsMockData()` (323 lines) |
| `app/Views/errors/html/error_404.php` | Rewritten to extend `layouts/default` |
| `app/Views/layouts/main.php` | **Deleted** (orphaned, broken CSS refs) |

All edited files pass `php -l`.