# Security review — Phase 9 A review of every boundary where TaxPilot AI meets something it does not control: the CMS, WhatsApp, a document, an operator, a log file. **Five findings. Four fixed, one accepted with reasoning.** Regression tests in `tests/test_security_regressions.py`. The boundaries that were reviewed and found sound are listed at the end — briefly, because "we looked and it was fine" is worth recording but not worth dwelling on. --- ## S-01 · High · Secrets appeared in dataclass reprs — **fixed** `Settings` and `MetaConfig` are dataclasses. Their generated `__repr__` included `agent_api_secret`, `access_token` and `app_secret` **in full**. ```python >>> repr(settings) Settings(cms_base_url='https://my.example.com', agent_api_key='tpa_...', agent_api_secret='sk-live-THE-ACTUAL-SECRET', verify_tls=True) ``` Any of these writes that to disk: - `logger.exception("could not start: %s", settings)` - a traceback from a tool that prints local variables - a crash reporter - somebody debugging with `print(container.settings)` Logs are shipped, aggregated and kept far longer than anyone intends, and a leaked agent secret is a working credential for the whole client base. **Fixed** with explicit masked reprs. Enough survives to tell two credentials apart — "the wrong key is configured" is a real diagnostic need — and not enough to use one. Secrets of eight characters or fewer are masked entirely, since showing the last four of eight gives away half. --- ## S-02 · Medium · Unbounded request bodies — **fixed** The HTTP server read `Content-Length` bytes with no limit: ```python body = self.rfile.read(length) # length came from the caller ``` `read(n)` allocates `n`. An attacker never has to *send* what they claim — a `Content-Length: 10000000000` header and a few bytes of body is enough to exhaust memory. The webhook endpoint faces the internet. **Fixed.** Bodies over 2 MB are refused with `413` **before a byte is read**, and a malformed length is a `400`. A real WhatsApp webhook carries a media *id*, not the media, so a legitimate body is kilobytes — the limit is generous by three orders of magnitude. --- ## S-03 · Medium · No bound on OCR input, and a setting that lied — **fixed** Two problems, one of them worse than it looks. **No size limit.** Any file reaching `PaddleOcrEngine.read()` was decoded. A compressed image expands to width × height × channels in memory, so a few megabytes on disk becomes gigabytes resident — the classic decompression bomb, against input that arrives from outside over WhatsApp. **`max_side` was configured and never used.** It documented a bound on image dimensions, had an environment variable, and was read into the config object — and nothing ever passed it to the detector. Dead config is worse than absent config. Anyone reading `OcrConfig` concluded the protection was in place; a reviewer auditing it would have ticked the box. **Fixed.** A file-size bound (20 MB, matching the CMS's own upload limit) is checked on `stat()` before anything decodes, and `max_side` is now passed to the detector as `text_det_limit_side_len`. A regression test reads the source of `_build_reader` to assert it is still wired — because the failure mode here was silence, and only an explicit check catches silence. --- ## S-05 · Medium · The fix for S-03 initially made things worse — **fixed** Found while measuring, not while reading, and worth recording because the first attempt was actively harmful. Wiring `max_side` to `text_det_limit_side_len` did the **opposite** of what its docstring claimed. PaddleOCR defaults `text_det_limit_type` to `"min"`, which scales the *shortest* side **up** to the limit — so setting a side length without also setting the type makes images larger, not smaller. Paddle said so plainly in a log line nobody was reading: ``` Resized image size (4800x1600) exceeds max_side_limit of 4000. Resizing to fit. ``` A 2400 × 800 document was being upscaled to 4800 × 1600 — more memory and more time, by a parameter added to reduce both. Measured on one document: | | | | --- | --- | | `limit_type="min"` (the default) | **96.9s** | | `limit_type="max"` (a real cap) | **19.3s** | Identical output — three text blocks, CNIC extracted — at a fifth of the cost. **Fixed** by setting `text_det_limit_type: "max"` explicitly, and the regression test asserts that line is present rather than just the side length. The full test suite went from 338s back to 62s, faster than before the audit began. The lesson is narrow and worth keeping: a bound that has never been measured is a guess, and this one was a guess in the wrong direction. --- ## S-04 · Low · An update reopens the replay window — **accepted** Nonces live in Laravel's cache, and `CACHE_STORE=file`. `UpdateInstaller` calls `optimize:clear` during an over-the-air update, which clears it. For the length of the signature TTL (300s) after an update, a previously-used nonce is accepted again. **Exposure is genuinely narrow.** An attacker needs to have captured a signed request — which needs TLS interception, already a high bar — *and* replay it within five minutes, *and* have an update land in that window. What a replay would then achieve: | Replayed | Effect | | --- | --- | | Any `GET` | Nothing. Idempotent reads | | `POST proposals` | Nothing. The idempotency key returns the same proposal | | `POST documents` | **One duplicate document** — the only real impact | **Accepted rather than fixed**, for two reasons. `POST documents` is ADR-0004's promoted path, which no agent holds a grant for; and the alternatives — a database-backed nonce table, or a cache store excluded from `optimize:clear` — add a moving part to the authentication path to close a window that requires TLS interception to reach. **Revisit if** any agent is granted `documents.write`. At that point the endpoint should take an idempotency key like `proposals` does, which closes this properly and is useful for its own sake. --- ## Reviewed and sound **Signing.** HMAC-SHA256 over method, path and raw body. `hash_equals` on the CMS, `hmac.compare_digest` in Python and for Meta webhooks — no timing-variable comparison anywhere. The signature covers the raw body and the CMS never re-encodes before verifying, which the cross-language fixtures prove on bytes that Python and PHP serialise differently. **Replay protection.** Single-use nonce via `Cache::add`, which is atomic, so two concurrent replays cannot both win. Nonce TTL equals signature TTL — they cannot disagree, so a nonce cannot expire while its signature is still valid. See S-04 for the one gap. **Permissions.** Explicit grants only; a newly issued agent holds none. `proposals.submit` and `documents.write` are separate, so the ordinary path cannot file anything without a human. Every CMS query runs through `Client::scopeVisibleTo()`, and `AiProposalPolicy` mirrors the queue's own scope so a filtered-out proposal cannot be reached by URL. **Webhooks.** Signature verified before the body is parsed. Verification handshake checks the token before echoing the challenge. An unconfigured app secret refuses everything rather than accepting anything. **Uploads.** `basename()` on filenames, size limits, MIME detected from bytes rather than taken from the caller, unpredictable storage paths (`bin/hex(16)`), and the private disk with an authorizing stream route. The reviewer approves a specific SHA-256, re-checked before filing. **Audit logging.** `AgentRequestLog` records path, method, outcome, status, IP and duration — **not request bodies**, which would have put Level 3 data into a log file. Proposal events are append-only and carry an actor label that survives the account being deleted. **Data policy.** Evidence may hold Level 3 and is hidden from serialisation, excluded from the agent poll response by an explicit field list, and absent from notification bodies. Memory content is redacted before storage. Health and metrics responses carry no hostname, DSN, driver message or unmasked number. --- ## One operational note `/health/*` and `/metrics` bind to loopback by default. A deployment that sets `TAXPILOT_HTTP_HOST=0.0.0.0` — which the webhook needs behind a reverse proxy — publishes them too. They carry no secrets, but they do describe infrastructure. **Restrict `/health/*` and `/metrics` at the proxy** and expose only `/webhook/whatsapp`. This is a deployment step, not a code change, and belongs in the VPS runbook.