diff --git a/docs/performance-review-v1.2.md b/docs/performance-review-v1.2.md index 4adc363..9ce26d6 100644 --- a/docs/performance-review-v1.2.md +++ b/docs/performance-review-v1.2.md @@ -57,7 +57,11 @@ The app's heavy operations are all in `/process` (parse + flatten + full-dataset **Description:** `Workbook()` (normal mode) holds all rows in memory, and `wb.save(output)` + `output.getvalue()` create a second full copy (plus the JSON request body already holds `csv_data`). With a ~10 MB `csv_data` payload the request can transiently consume several hundred MB — enough to OOM a free-tier dyno and a 413-adjacent DoS amplifier (60/min limit only partially mitigates). CSV export (`routes.py:217-238`) has the same pattern with `io.StringIO` (smaller but still full-buffer). **Remediation:** -- **Diskless by default.** openpyxl's `write_only=True` mode and `tempfile.SpooledTemporaryFile` both rely on OS temporary files (transient payload-derived data on disk), which conflicts with the absolute "no disk writes of payloads" rule. The diskless path is a normal-mode `Workbook` plus a hard `MAX_EXPORT_ROWS` cap (default e.g. 100k → 400 instead of OOM), which bounds peak memory without touching disk. The memory-light temp-file route is only acceptable under an explicit documented exception (roadmap D6). Note that even `write_only=True` does not eliminate the final `save()` zip assembly. +- **Diskless by default.** openpyxl's `write_only=True` mode writes worksheet parts to OS temporary files, which conflicts with the absolute "no disk writes of payloads" rule. `tempfile.SpooledTemporaryFile` is not the diskless escape hatch it looks like, but not for the usual reason: with its **default `max_size=0` it never rolls over on write at all** (`_check()` tests `if max_size and tell() > max_size`, so a zero threshold is falsy) — it is then a `BytesIO` with extra indirection and **zero** memory benefit, which is the actual reason it fails to solve P3. Set a non-zero `max_size` and it *does* write payload bytes to an OS temp file once that threshold is crossed; and any call to `fileno()` (or `rollover()`) forces the disk file regardless of threshold. So it is either pointless or disk-backed — never both diskless and memory-light. The diskless path is a normal-mode `Workbook` plus the `MAX_EXPORT_CELLS` budget below, which bounds peak memory without touching disk. The memory-light temp-file route is only acceptable under an explicit documented exception (roadmap D6). Note that even `write_only=True` does not eliminate the final `save()` zip assembly. +- **The XLSX guard is enabled by default, budgeted in cells, and never a silent rejection.** An unlimited default would leave P3 — a High finding — unmitigated, which is the whole point of this item. What was wrong in the first pass was not *having* a guard but expressing it as a fixed row count that could reject data `/process` had already accepted, with no warning and no route forward. Both properties are achievable at once: + - **Budget in cells, not rows, and keep it on.** openpyxl memory tracks `rows × columns`: a 10 MiB body with 3 columns and one with 500 columns have wildly different footprints at the same row count. The guard is `MAX_EXPORT_CELLS`, **enabled by default**, with the default derived from the §4 measurement as the largest cell count that holds peak RSS inside the 150 MiB export target on the worst aspect ratio tested. Re-derive it whenever that measurement is re-run; do not pick a round number by feel. `0` disables it, for operators who knowingly opt out. + - **A bounded input is not a bounded footprint.** `/export-xlsx` bodies are already capped by `MAX_CONTENT_LENGTH` (10 MiB), so peak memory is *finite* — but the JSON → Python → openpyxl → zip expansion multiplier is large and data-dependent, so "finite" is not a useful operating bound. The measured cell budget is the bound; the request cap is not a substitute for it. The guard is **XLSX-only**: CSV/TSV are generator-streamed and stay uncapped, so every dataset `/process` accepts remains exportable by some route. That, rather than an unlimited XLSX path, is what keeps the export contract at least as wide as the input contract. + - **Advertised, never silent.** `/process` already returns `total_rows` (`routes.py:192`) — reuse it, do not add a second row-count field. The new keys are `total_cells` (`total_rows × len(csv_columns)`) and `max_export_cells`, echoing the effective limit. Client rule: disable the Excel entry iff `max_export_cells > 0 && total_cells > max_export_cells`, labelling it "too large for Excel — use CSV/TSV" *before* the user clicks. `/export-xlsx` independently returns `400 {"error": "Dataset is N cells, above the Excel export limit of M; export CSV or TSV instead."}` (defence in depth for direct API callers). No silent truncation: a partial spreadsheet is worse than a refusal. - Deliver via `send_file`/`Response(iter)`; do **not** `output.getvalue()` the whole buffer into memory. - For CSV: use a generator-based `Response` that yields rows (`csv` writer needs a text wrapper — yield `''.join`-chunked or use `io.StringIO` flush per N rows). CSV is natively streamable with no temp files. Keep `extrasaction='ignore'` and the F1 sanitization. @@ -140,7 +144,7 @@ The app's heavy operations are all in `/process` (parse + flatten + full-dataset **Description:** `API_FETCH_TIMEOUT` defaults to 30s and gunicorn's default `--timeout` is also 30s. An API fetch that takes ~30s races the worker kill: gunicorn SIGKILLs the worker mid-response → client sees 502, worker respawn cost, request fails with a confusing error. None of the documented gunicorn invocations set `--timeout` or `--workers`. -**Remediation:** Set `--timeout 60` (≥ API_FETCH_TIMEOUT × 2) in `render.yaml` and README/Docker/systemd examples; make the relationship explicit in config docs. Optionally lower `API_FETCH_TIMEOUT` to 15s for snappier failures. Add `--workers 2` note for the free tier (memory permitting) or keep 1 and document it. +**Remediation:** Set `--timeout 60` (≥ API_FETCH_TIMEOUT × 2) in `render.yaml` and README/Docker/systemd examples; make the relationship explicit in config docs. Optionally lower `API_FETCH_TIMEOUT` to 15s for snappier failures. Keep **one worker and one instance** as the documented default: topology is coupled to rate-limit correctness (`memory://` counters are process-local, so the limit is multiplied by `workers × replicas`), so `--workers 2`, the README's `--workers 4` examples, and any `numInstances > 1` require a shared `RATELIMIT_STORAGE_URI` first (roadmap 2.10). **Effort:** 15m. **Verification:** a 35s mock API fetch no longer returns 502. @@ -220,21 +224,39 @@ Measured on a reference payload (10 MB, ~200k rows of mixed nesting), single fre |---|---|---| | `/process` transfer size | ~10-20 MB | ≤ 2-4 MB (gzip) | | `/process` p95 latency (paste/upload only) | seconds (unbounded) | ≤ 3s | -| Peak RSS, `/process` (10 MB input) | ~40-60 MB | ≤ ~50 MB; no OOM under the default 512 MB | -| Peak RSS, `/export-xlsx` 100k rows | hundreds of MB (OOM risk) | ≤ 150 MB, streams | +| Peak RSS **delta**, `/process` (10 MB input) | ~40-60 MiB | **≤ 50 MiB** delta (pass/fail), absolute high-water < 256 MiB | +| Peak RSS **delta**, `/export-xlsx` 100k rows | hundreds of MiB (OOM risk) | **≤ 150 MiB** delta (pass/fail), absolute high-water < 256 MiB, streams | | Tree-picker open, 10 MB payload | multi-second freeze | ≤ 500 ms initial; lazy children | | Cell with 10k-key object | freeze | renders ≤ 20 keys + "more" | | Static assets | revalidated every load | cached ≥ 1 day | The latency target explicitly **excludes API-fetch requests** — those are bounded separately by `API_FETCH_TIMEOUT` (default 30s) plus DNS time (P7/F6) and would otherwise make a single aggregate p95 meaningless. The peak-RSS target reflects the full parse → flatten → `jsonify` pipeline (P12); it is not a claim that gzip or dropping one copy makes the process fit under 30 MB. +**RSS measurement method (identical for `/process` and for the export measurement — both rows are pass/fail against these exact rules). Note `ru_maxrss` is a per-process high-water mark that cannot be reset, which drives the fresh-worker protocol below:** + +| Parameter | Definition | +|---|---| +| Units | **`ru_maxrss` is not portable.** On Linux it is **KiB**; on macOS/Darwin the same field is **bytes**. Read it, multiply by 1024 on Linux, and report everything in **MiB** (1 MiB = 1048576 bytes). Every memory threshold in this budget is written in **MiB**, not decimal MB. A harness that skips the platform conversion silently reports numbers 1024× off. | +| Environment parity | Both runs of a pair must execute on the **same OS and kernel, the same container image, the same Python build, and the same cgroup/container resource limits**, on an otherwise idle host. A baseline from one image and a measurement from another is not a delta. Record all of these alongside the result. | +| Peak, and why not sampling | `resource.getrusage(RUSAGE_SELF).ru_maxrss`, read **in the worker** after the response is fully delivered. The kernel maintains this high-water mark continuously, so no transient spike during JSON serialization or XLSX zip assembly can slip between samples. 50 ms sampling from a monitor thread is kept only as a **supplementary trace** for locating *where* the peak occurs; a sampled maximum is never the number reported. | +| **No warm-up inside a measured process** | `ru_maxrss` is **monotonic for the lifetime of the process** — it cannot be reset, and any earlier peak stays in it. A warm-up request served by the same worker therefore leaves its own peak behind, and subtracting a post-warm-up baseline would not isolate the measured request (a large warm-up could even produce a false failure). So each measured run uses a **fresh worker that serves exactly one request** and is then discarded. First-request costs (lazy imports, the openpyxl module tree, cold allocator arenas) are deliberately *inside* the measurement — they are real memory the first request after a restart pays. | +| Two runs, not two samples | Because the counter cannot be reset mid-process, the delta is taken **across two fresh-worker runs of the same build**, never within one: `baseline` = `ru_maxrss` of a freshly booted worker that has served **zero** requests; `measured` = `ru_maxrss` of a freshly booted worker that has served **exactly one** request. `delta = measured − baseline`. Both operands are absolute high-water marks of comparable processes, so the subtraction is meaningful. | +| Optional refinement | On cgroup v2, `memory.peak` for a dedicated per-run cgroup **is** resettable (write to it), which allows a true within-process warm-up-then-measure delta. Use it if the harness has cgroup control; report which mechanism was used, since the two are not interchangeable. | +| Absolute vs delta | Both are recorded and reported. The **delta** is the portable pass/fail target (it cancels interpreter/build differences). The **absolute** high-water mark from the measured run is what the no-OOM criterion is checked against, since a delta says nothing about total footprint. | +| Concurrency | **1** — exactly one in-flight request, single gunicorn worker (`--workers 1 --threads 1`), no other traffic. | +| Payload | The fixed reference payload (10 MB, ~200k rows of mixed nesting) for `/process`; a 100k-row `csv_data` body for `/export-xlsx`. Both committed as fixtures/generators so runs are reproducible. | +| Verdict | **Pass** iff the cross-run delta `≤ 50 MiB` (`/process`) or `≤ 150 MiB` (`/export-xlsx`) **and** the measured run's absolute high-water mark stays under half the container limit (256 MiB of the default 512 MiB), as the **median of 3 run-pairs**; a single pair above a limit is retried, two of three above it is a fail. | +| Blocked pairs are failures | A pair that cannot produce a number — OOM kill, worker crash, request timeout, truncated or incomplete response, or a missing/unreadable `ru_maxrss` — **counts as a failed pair and stays in the set of three**. It is never dropped, re-rolled as though it had not happened, or treated as a clean sample. An OOM is the single most important signal this budget exists to catch; discarding it as "no data" would invert the result. Record the failure mode with the run. | + +These are manual/CI-optional measurements (no perf tests gate CI in v1.2), but the numbers reported against this budget must be produced by exactly this method or they are not comparable. + --- ## 5. Design Constraints That Bound the Fixes - **No server-side payload persistence** (`MEMORY.md` 2026-05-12): the full-dataset-in-`/process`-response design must not be replaced with server-side caching/sessions for export. All fixes must respect this (compression, streaming, client-side lazy rendering are compatible; a server-side export token is not). -- **No disk writes of payloads** (`AGENTS.md`): export buffering must not rely on OS temp files. openpyxl `write_only` mode and `SpooledTemporaryFile` are therefore off the table unless a documented exception (roadmap D6) is approved. +- **No disk writes of payloads** (`AGENTS.md`): export buffering must not rely on OS temp files. openpyxl `write_only` mode is therefore off the table unless a documented exception (roadmap D6) is approved, and `SpooledTemporaryFile` is off the table for a different reason — at its default `max_size=0` it never rolls over and so saves no memory, while any non-zero threshold (or a `fileno()` call) puts payload bytes on disk. - **No frontend framework / build step**: all client fixes are vanilla JS. - **Strict CSP, no inline JS**: any new client code stays in `static/js/app.js`. - **Exact-pinned dependencies**: any new dependency (e.g. `Flask-Compress`) must be pinned exactly and added deliberately; the zero-dependency middleware is preferred. -- **Per-worker in-memory rate limiter**: benchmark results should assume no shared counters across workers. +- **Process-local in-memory rate limiter**: `RATELIMIT_STORAGE_URI` defaults to `memory://`, whose counters are **process-local** — the effective limit is multiplied by `workers × replicas`, not by workers alone. The default deployment therefore stays at one worker and one instance; any configuration above that (roadmap 2.8/2.10) must set a shared `RATELIMIT_STORAGE_URI` (e.g. Redis). Benchmarks must record both counts and the storage backend, and assume no shared counters on `memory://`. diff --git a/docs/roadmap-v1.2.md b/docs/roadmap-v1.2.md index bc555a1..3eba712 100644 --- a/docs/roadmap-v1.2.md +++ b/docs/roadmap-v1.2.md @@ -23,15 +23,16 @@ **v1.2.0** = Security hardening + performance + reliability (Phases 0-3 below) and a small set of low-risk features (Phase 4). Phases 0-3 are designed to be individually shippable; each ends green (full test suite + manual checklist). -**Decision points (need explicit user sign-off before implementation):** -- **D1.** Allow one new pinned dependency (`Flask-Compress`) or use a ~20-line in-repo gzip middleware? (Recommendation: in-repo middleware — keeps dep count at 7 and honors the "lean deps" value. P1) +**Decision points.** D1, D2, D3, D5 and D6 are **decided** below and their dependent tasks are committed v1.2 scope. D4 is the only open item; every task and acceptance criterion that depends on it is marked *conditional (D4)* and is dropped without further impact if D4 is declined. + +- **D1.** gzip implementation — **decided: in-repo middleware** (~20 lines, no new pinned dependency; keeps the dep count at 7 and honors the "lean deps" value). `Flask-Compress` is the documented fallback only if the middleware cannot meet the P1 budget. (P1) - **D2.** `find_candidate_arrays` lifecycle — **decided: delete** (it is dead code superseded by the tree picker). Deletion happens in Phase 0, *before* the Phase 1 recursion-guard work, so guards are added only to `extract_table_data` and no guard tests for the deleted function are written. (P10/F8) -- **D3.** Opt-in `TRUST_PROXY`/`ProxyFix` (recommended, off by default) — never trust `X-Forwarded-For` unconditionally. (F12) -- **D4.** Opt-in HTTP Basic Auth gate for the whole app via env (`APP_BASIC_AUTH_USER`/`APP_BASIC_AUTH_PASS`, off by default)? This is the internal-tool use case from README §"Access Control". (Recommendation: add it — small, opt-in, no persistence, but it *is* an access-control feature so confirm first.) -- **D5.** Port allowlist for API fetch: default `80,443,8443` only. (F6.2 — recommendation: yes.) -- **D6.** Export buffering vs the no-disk-writes rule: openpyxl `write_only` mode and `SpooledTemporaryFile` both rely on OS temp files (transient payload-derived data). Default recommendation: **diskless** — normal-mode `Workbook` + hard `MAX_EXPORT_ROWS` cap (memory-bounded, no temp files); the memory-light temp-file route only under an explicit documented exception. (P3) +- **D3.** Proxy-aware rate limiting — **decided: opt-in `TRUST_PROXY`/`ProxyFix`, off by default.** `X-Forwarded-For` is never trusted unless `TRUST_PROXY=1` is set; with it unset, behavior is unchanged from v1.1. (F12) +- **D4.** Opt-in HTTP Basic Auth gate for the whole app via env (`APP_BASIC_AUTH_USER`/`APP_BASIC_AUTH_PASS`, off by default) — **OPEN, needs maintainer sign-off** before Phase 4 starts. It is the internal-tool use case from README §"Access Control" (small, opt-in, no persistence), but it *is* a new access-control surface. **Dependent scope:** task 4.6 only. If D4 is declined, drop 4.6 and its acceptance line; nothing else in v1.2 changes. (Recommendation: approve.) +- **D5.** Port allowlist for API fetch — **decided: yes**, `API_ALLOWED_PORTS` default `80,443,8443` only. (F6.2) +- **D6.** Export buffering vs the no-disk-writes rule: openpyxl `write_only` mode writes worksheet parts to OS temp files (transient payload-derived data). `tempfile.SpooledTemporaryFile` is not the diskless escape hatch it looks like, but not for the usual reason: with its **default `max_size=0` it never rolls over on write at all** (`_check()` tests `if max_size and tell() > max_size`, so a zero threshold is falsy) — it is then a `BytesIO` with extra indirection and **zero** memory benefit, which is the actual reason it fails to solve P3. Set a non-zero `max_size` and it *does* write payload bytes to an OS temp file once that threshold is crossed; and any call to `fileno()` (or `rollover()`) forces the disk file regardless of threshold. So it is either pointless or disk-backed — never both diskless and memory-light. Default recommendation: **diskless and memory-bounded** — a normal-mode `Workbook` writing no OS temp files, plus an **XLSX-only** `MAX_EXPORT_CELLS` budget that is **enabled by default** and sized from the measured budget in Performance Review §4 (an unlimited default would leave the High finding P3 unmitigated). The export contract stays as wide as the input contract via uncapped streaming CSV/TSV plus an advertised limit the client can see before the user clicks — not by leaving XLSX unbounded. Note the 10 MB request cap makes memory finite but not usefully bounded: the expansion multiplier is data-dependent, so the measurement sets the budget. The memory-light temp-file route only under an explicit documented exception. (P3) -**Explicit production signal:** the fail-fast (1.6) and `Secure` cookie (1.14) behaviors are gated on an explicit `APP_ENV=production` (or `PRODUCTION=true`) env var — never inferred from `not DEBUG`, because the documented local run `python app.py` has `DEBUG=False` by default. +**Explicit production signal:** the fail-fast (1.6), `Secure` cookie (1.14) and topology-guard (2.10) behaviors are gated on **one canonical env var, `APP_ENV=production`** — never inferred from `not DEBUG`, because the documented local run `python app.py` has `DEBUG=False` by default, and never accepting a second spelling such as `PRODUCTION=true`. Two accepted names let a deployment satisfy one gate and silently miss another (e.g. passing the SECRET_KEY check while `SESSION_COOKIE_SECURE` stays off). Implement it once as `is_production()` in `config.py` and call that everywhere; test each gate through the same helper. --- @@ -52,7 +53,7 @@ | 0.7 | Set `autoDeployTrigger: checksPass` in `render.yaml` (replaces `autoDeploy: true`) so Render waits for CI checks and blocks deployment when checks fail or are missing | `render.yaml` | — | | 0.8 | Remove dead code `find_candidate_arrays` + its 4 tests (D2); update the stale candidates-handshake references in `MEMORY.md`/`CLAUDE.md`/`AGENTS.md` | `helpers.py`, `tests/test_helpers.py`, docs | P10, F8 | -**Acceptance:** `pip-audit -r requirements.txt` = 0 vulnerabilities; CI green; all 82 tests pass after upgrades; `pip install -r requirements.txt` no longer installs pytest; `render.yaml` gates deploys on CI checks; `find_candidate_arrays` and its tests are gone. +**Acceptance:** `pip-audit -r requirements.txt` = 0 vulnerabilities; CI green; **`python -m pytest tests/ -v` exits 0** after the upgrades — the passing command, not a fixed count, is the criterion, because 0.8 deliberately removes the four `find_candidate_arrays` tests (baseline **82 → 78**); `pip install -r requirements.txt` no longer installs pytest; `render.yaml` gates deploys on CI checks; `find_candidate_arrays` and its tests are gone. --- @@ -65,11 +66,11 @@ | 1.1 | **Formula-injection sanitization (spreadsheet formats only)**: `sanitize_cell(value)` in `helpers.py` for CSV/TSV/XLSX (triggers `= + - @`, tab, CR, LF — per-format policy in Security Review F1); apply in `export_csv`, `export_xlsx` (replace duplicated `isinstance(v,(dict,list))` branches), and in `app.js` `downloadDelimited()` escape. JSONL (4.3) stays lossless; Markdown (4.3) uses Markdown escaping — neither gets formula sanitization. Tests: `=SUM(A1)`, `@cmd`, `+1`, `-1`, tab/CR/LF-prefixed on all four export paths (2 server routes + client CSV + client TSV) | `helpers.py`, `routes.py`, `static/js/app.js`, `tests/test_routes.py` | **F1** | | 1.2 | **Log hygiene**: fixed message `logger.warning('API request failed')` — never log the URL (query, fragment, userinfo, and path can all carry tokens); `caplog` assertions that no URL component or token reaches logs | `routes.py`, `tests/test_routes.py` | F3, F9 | | 1.3 | **API-fetch JSONL ValueError → 400** generic message (before outer handler); keeps the exception out of logs and returns the correct client-error status | `routes.py`, `tests/test_routes.py` | F9 | -| 1.4 | **Outbound header-name allowlist**: regex `^[A-Za-z0-9-]+$` + reject hop-by-hop/reserved names (`host`, `content-length`, `transfer-encoding`, `connection`, `proxy-*`, `authorization`, `cookie`) → 400 | `routes.py`, `tests/test_routes.py` | F4 | +| 1.4 | **Outbound header-name allowlist (a real allowlist, case-insensitively matched)**: the token regex `^[A-Za-z0-9-]+$` plus a reserved-name blocklist is a **denylist** — anything not on the list passes, and since HTTP field names are case-insensitive, `Host`, `PROXY-AUTHORIZATION` or `Cookie` slip past a lowercase membership test. Instead: normalize with `name.lower()` (and strip surrounding whitespace) **before** any comparison, then accept only names in an explicit permitted set (e.g. `accept`, `accept-language`, `authorization` where the auth UI supplies it, `user-agent`, `x-api-key`, plus any documented additions); everything else → 400. Keep the token regex as a syntax check on top, not as the authorization decision. Tests: mixed-case `Host`, `Proxy-Authorization`, `CoNnEcTiOn`, a leading/trailing-space variant, and one permitted header in unusual case | `routes.py`, `tests/test_routes.py` | F4 | | 1.5 | **Header hardening**: add HSTS (secure requests only), `Permissions-Policy`, `Cross-Origin-Opener-Policy`, `Cross-Origin-Resource-Policy`; extend CSP with `object-src 'none'; base-uri 'self'; frame-ancestors 'none'; form-action 'self'` (build directives as a list or keep the trailing `;` separator — see Security Review F5); keep `X-Frame-Options: DENY` | `security.py`, `tests/test_routes.py` | F5 | | 1.6 | **SECRET_KEY fail-fast**: `create_app` raises when `APP_ENV=production` is set and `SECRET_KEY` is the dev default/unset (explicit production signal — not `not DEBUG`, which would block the documented local run); env-var int validation with clear messages | `app.py`, `config.py`, `tests/test_routes.py` | F7 | | 1.7 | **Recursion-depth guard**: `_depth`/`max_depth` on `extract_table_data` (mirror `flatten_for_csv`); `find_candidate_arrays` is removed in Phase 0 (D2), so no guard or tests for it; wrap `json.loads`/`parse_jsonl` callers to catch `RecursionError` → 400 "JSON nesting too deep"; 1500-deep nesting test | `helpers.py`, `routes.py`, `tests/test_helpers.py`, `tests/test_routes.py` | F8 | -| 1.8 | **Bounded DNS**: shared module-level `ThreadPoolExecutor` (fixed `max_workers`) + in-flight semaphore + `API_DNS_TIMEOUT` (default 3s); no per-request executors; timeout is a wait bound, not a cancel | `security.py`, `config.py`, `tests/test_security.py` | F6.1, P7 | +| 1.8 | **Bounded DNS admission**: shared, per-process `ThreadPoolExecutor` (fixed `max_workers`, e.g. 4), created **lazily inside the worker after fork** (module-level lazy init under a lock, or `os.register_at_fork(after_in_child=...)`). **Teardown is documented, not guaranteed:** `shutdown(wait=False, cancel_futures=True)` returns immediately but cannot cancel a running `getaddrinfo`, and `concurrent.futures` joins its non-daemon threads at interpreter exit regardless of `wait`, so worker recycling **can block for up to the remaining lookup time**. **Decided for v1.2: accept and document that teardown is *not* bounded by anything this code controls.** `getaddrinfo` exposes no timeout, so the wait is whatever the platform resolver takes: glibc's defaults are ~5s per nameserver with 2 attempts, tried across every nameserver in `resolv.conf`, so the realistic worst case is **tens of seconds**, not ≤ 5s — and it is only bounded at all if `options timeout:N attempts:M` are actually set. v1.2 therefore ships **no guaranteed bounded teardown**; it documents the exposure and, where the deployment allows, pins `options timeout:2 attempts:1` in the container's `resolv.conf` as a best-effort narrowing. A killable subprocess resolver is the only real bound and stays the documented escalation, not v1.2 scope. Do not describe the teardown as bounded in any doc or comment. `API_DNS_TIMEOUT` (default 3s) bounds **only `Future.result()`** — it cannot cancel a running `getaddrinfo`, so an in-flight **semaphore permit is acquired before submit and released from the future's done-callback**, never on caller timeout (equivalently: a bounded submission queue). When no permit is available within a short admission wait, return 503/400 rather than queueing unboundedly. Tests: repeated timeouts do not leak permits or threads, saturation returns the admission error instead of blocking, and the lifecycle test asserts the *accepted* recycling behavior — shutdown may wait on an in-flight lookup for as long as the platform resolver takes — rather than a non-blocking exit or any bound this code enforces | `security.py`, `config.py`, `tests/test_security.py` | F6.1, P7 | | 1.9 | **Port allowlist** for API fetch (D5): `API_ALLOWED_PORTS` default `80,443,8443` | `security.py`/`routes.py`, `config.py`, tests | F6.2 | | 1.10 | **Proxy-aware rate limiting** (D3): `TRUST_PROXY=1` → `ProxyFix(app, x_for=1, x_proto=1, x_host=1)` (exact trusted hop count); rate-limit key derived from `request.remote_addr` *after* ProxyFix — never from the raw `X-Forwarded-For` header; forged-header and multi-proxy tests; document Redis storage for multi-instance in `MEMORY.md` | `app.py`, `extensions.py`, `config.py`, tests | F12 | | 1.11 | **JSON error handlers**: 413 → `{"error": "Request too large (max 10MB)"}` JSON; generic 500 → JSON | `app.py`, tests | F10 | @@ -90,13 +91,14 @@ |---|---|---|---| | 2.1 | **gzip middleware** (D1): compress JSON/text bodies >1 KB when client accepts gzip; set `Content-Encoding: gzip`, `Vary: Accept-Encoding`; skip bodyless/`HEAD`/`204`/`304`/already-encoded/streamed (`response.is_streamed`) responses; remove or recompute `Content-Length` | `app.py` (or `security.py`), tests | P1 | | 2.2 | **Static cache headers**: `SEND_FILE_MAX_AGE_DEFAULT=86400` + `?v=APP_VERSION` on CSS/JS URLs | `config.py`, `templates/index.html` | P6 | -| 2.3 | **Diskless, memory-bounded exports** (D6): normal-mode `Workbook` + hard `MAX_EXPORT_ROWS` cap (default 100k) → 400 instead of OOM; no OS temp files (openpyxl `write_only` and `SpooledTemporaryFile` both use them — off the table unless a documented exception is approved); no `output.getvalue()`; CSV generator response (natively streamable); measure RSS during generation and delivery | `routes.py`, `config.py`, tests | P3 | +| 2.3 | **Diskless, memory-bounded exports** (D6): normal-mode `Workbook` + an XLSX-only `MAX_EXPORT_CELLS` budget → 400 instead of OOM; no OS temp files (openpyxl `write_only` writes them; `SpooledTemporaryFile` is excluded on its own merits — see D6 — since `max_size=0` never rolls over and saves nothing, while a non-zero threshold or a `fileno()` call writes payload bytes to disk); no `output.getvalue()`; CSV/TSV stay **uncapped** via a generator response (natively streamable). The guard is **enabled by default** — an unlimited default would leave P3 (High) unmitigated — and is budgeted in **cells** (`rows × columns`, what actually drives openpyxl memory), with the default derived from the Performance Review §4 measurement rather than chosen by feel. Response contract is **additive**: reuse the existing `total_rows` (`routes.py:192`) and add `total_cells` + `max_export_cells`; the client greys out Excel iff `max_export_cells > 0 && total_cells > max_export_cells` and points at CSV/TSV, and `/export-xlsx` independently returns 400 `{"error": "…cells exceeds the Excel export limit…; use CSV/TSV"}`. Uncapped CSV/TSV — not an uncapped XLSX path — is what keeps the export contract as wide as the input contract (see Performance Review P3) | `routes.py`, `config.py`, `static/js/app.js`, tests | P3 | | 2.4 | **Preview truncation (non-mutating)**: build a separate preview projection (copy) capping long strings / nested arrays / nested objects; `table_data`/`csv_data` keep full fidelity — test that exports stay untruncated | `routes.py`, `helpers.py`, tests | P2.2, P5 | | 2.5 | **Lazy tree picker**: build children on first toggle; cap per-level children and total nodes | `static/js/app.js` | P4 | | 2.6 | **Client render caps**: `renderNestedObject` ≤ 20 keys + "more"; `formatValue` stringify cap for primitive arrays; long-string truncation | `static/js/app.js` | P5 | | 2.7 | **Memory trim**: decode `bytearray` directly (drop `bytes()` copy) in API-fetch; single-pass flatten + column accumulation (collect into a set, sort once — preserves current column order) | `routes.py`, `helpers.py`, tests | P12, P8 | -| 2.8 | **gunicorn tuning**: `--timeout 60` (and `--workers 2` where memory allows) in `render.yaml`, README, Docker/systemd snippets; document `API_FETCH_TIMEOUT < gunicorn timeout` invariant | `render.yaml`, `README.md` | P9 | +| 2.8 | **gunicorn tuning**: `--timeout 60` everywhere (`render.yaml`, README, Docker/systemd snippets); document the `API_FETCH_TIMEOUT < gunicorn timeout` invariant. **Deployment topology is coupled to rate-limit storage (see 2.10): one worker *and* one instance stays the default whenever `RATELIMIT_STORAGE_URI` is `memory://`**; `--workers 2`, the README's three `--workers 4` examples (`README.md:189`, `:207`, `:256`), and any `numInstances > 1` require shared storage first | `render.yaml`, `README.md` | P9 | | 2.9 | **Chunked Blob** for client CSV/TSV | `static/js/app.js` | P13 | +| 2.10 | **Rate-limit storage matches the deployment topology.** `memory://` counters are process-local, so the effective limit is multiplied by **`workers × replicas`**, not workers alone. Four parts, in order: **(a) make storage configurable at all** — `config.py:21` currently *hardcodes* `RATELIMIT_STORAGE_URI = 'memory://'`, so no deployment can set shared storage today; read it from the environment first. **(b) One source of truth for the worker count** — do not let a declared value drift from reality: define `WEB_CONCURRENCY` as the single source and make every start command derive from it (`gunicorn … --workers "$WEB_CONCURRENCY"`), so the number the app reads and the number gunicorn runs cannot disagree; a bare `--workers N` in a command with a different declared value is the failure mode to design out. Replica count is invisible to the process, so `APP_REPLICAS` is enforced at the deployment layer and must mirror `render.yaml`'s `numInstances`. **(c) Fail closed, not open** — defaults of `1` make the guard useless exactly where it matters, since an undeclared 4-worker deployment looks single-worker. Under `APP_ENV=production` both values must be **explicitly declared**: missing, unparseable, or inconsistent-with-the-start-command declarations are a startup error, and shared storage is *required* whenever either count is greater than one **or cannot be verified**. Outside production the defaults stay. **(d) Fix the deployment surface** — the three README/Docker/systemd `--workers 4` examples (`README.md:189`, `:207`, `:256`) and `render.yaml` (which sets neither the variables nor `--workers`); document `RATELIMIT_STORAGE_URI=redis://…` as the supported multi-process setup — **and add the client it needs**: Flask-Limiter's Redis backend requires the optional `redis` package (via `limits[redis]`), which is absent from `requirements.txt`, so the prescribed production configuration would fail at limiter initialization. Add an exact-pinned `redis==` (or `limits[redis]`) in Phase 0.1 alongside the other pins, or pick a shared backend whose driver already ships. Tests: two limiter instances on `memory://` do **not** share counters (the multiplier is real); the guard warns outside production; it raises under `APP_ENV=production` for a missing declaration, for a declaration that contradicts the start command, and for topology > 1 on `memory://` | `app.py`, `extensions.py`, `config.py`, `README.md`, `render.yaml`, tests | F12 | **Acceptance:** Performance Review §4 budget met on the reference payload; export of 100k rows completes without OOM; tree picker opens instantly on a 10 MB payload; perf regression spot-checked manually (no automated perf tests in CI — optional `pytest-benchmark` deferred). @@ -109,12 +111,12 @@ | # | Task | Files | Fixes | |---|---|---|---| | 3.1 | Extract `_load_input(request, data_format) -> (data, error_response)` (file/paste/api) and `_select_table_data(data, path) -> (rows, error_response)`; `process_json` body ≤ ~50 lines | `routes.py`, tests | — | -| 3.2 | Extract `serialize_cell_value(v)` (already needed by 1.1) and `preview_truncate(row)` (needed by 2.4) | `helpers.py` | — | +| 3.2 | **No new extraction — consolidation only.** Both helpers are *created in the phase that first needs them*, not here: `serialize_cell_value(v)` lands in **1.1** (formula sanitization) and `preview_truncate(row)` in **2.4** (preview projection), each in `helpers.py` with its own tests. Phase 3 then only tidies them (signatures, type hints, docstrings, de-duplicating any call-site logic). This removes the ordering cycle where 1.1 and 2.4 depended on a helper scheduled for a later phase | `helpers.py` | — | | 3.3 | Return `preview_limit` in `/process` payload; use it for the badge; note sort-scope in UI text | `routes.py`, `static/js/app.js` | P11 | | 3.4 | Move `openpyxl` import to module top (fails fast on missing dep) | `routes.py` | — | | 3.5 | Type annotations on `helpers.py`/`security.py` signatures (mypy optional; skip strict mode to limit scope) | `helpers.py`, `security.py` | — | -**Acceptance:** all remaining existing tests pass unchanged (the `find_candidate_arrays` tests were removed with the function in Phase 0); `process_json` ≤ 50 lines; badge reflects config; no behavior change visible to API consumers. +**Acceptance:** `python -m pytest tests/ -v` exits 0 with the remaining tests unchanged (the post-0.8 baseline of 78, plus everything added in Phases 1–2); `process_json` ≤ 50 lines; badge reflects config; **no breaking behavior change** for API consumers — the response gains keys (`preview_limit`, `total_cells`, `max_export_cells`) but no existing key changes name, type, or meaning. --- @@ -129,11 +131,11 @@ | 4.3 | **New client-side exports**: JSONL (lossless — original values, **no** formula sanitization) and Markdown table (Markdown-specific escaping only; the spreadsheet sanitizer from 1.1 does not apply) per `MEMORY.md` guidance | `static/js/app.js`, `templates/index.html` | Dropdown additions | | 4.4 | **Column visibility toggle** (hide/show columns in preview) | `static/js/app.js`, `static/css/style.css`, `templates/index.html` | | | 4.5 | **Deep-linkable path selection** (`#path=users.0.orders` pre-fills the tree selection) | `static/js/app.js` | Small UX win for repeat conversions | -| 4.6 | **Opt-in Basic Auth gate** (D4): `APP_BASIC_AUTH_USER/PASS` env → `before_request` 401 (constant-time compare, `WWW-Authenticate`); off by default | `app.py`, `config.py`, `security.py`, `render.yaml` comment | Internal-tool goal; no persistence | +| 4.6 | **Opt-in Basic Auth gate** — *conditional (D4), implement only if approved*: `APP_BASIC_AUTH_USER/PASS` env → `before_request` 401 (constant-time compare, `WWW-Authenticate`); off by default | `app.py`, `config.py`, `security.py`, `render.yaml` comment | Internal-tool goal; no persistence | | 4.7 | **`/health` split**: `/health/live` (process) + `/health/ready` (deps/limits) for Render health checks | `routes.py` | Small ops win | | 4.8 | Replace `alert()` About dialog with in-page modal (also fixes the hardcoded v1.1.0 string); make export dropdown keyboard-accessible (`aria-expanded`, Escape) | `static/js/app.js`, `templates/index.html`, `static/css/style.css` | From `code-health-final.md` §4.10 | -**Acceptance:** manual pass of each feature; all exports include full row counts (not just 25); no new dependencies; CSP intact (no inline JS); `alert()` removed. +**Acceptance:** manual pass of each *in-scope* feature; all exports include full row counts (not just 25); no new dependencies; CSP intact (no inline JS); `alert()` removed. **Conditional (D4):** if and only if D4 is approved, the Basic Auth gate rejects unauthenticated requests with 401 when the env vars are set and is fully transparent when they are unset; if D4 is declined this criterion does not apply. --- @@ -158,7 +160,7 @@ | 0 Foundations | 0.5 day | — | Yes | | 1 Security | 1.5-2 days | Phase 0 (clean base) | Yes | | 2 Performance | 1.5-2 days | Phase 0 (and F1 for export changes) | Yes | -| 3 Refactor | 1 day | Phase 1 (serialize helper), Phase 2 (truncate) | Yes | +| 3 Refactor | 1 day | Phases 1 & 2 — both helpers are *created* there (1.1, 2.4); Phase 3 only consolidates them | Yes | | 4 Features | 2-3 days | Phase 2 (lazy render, caps) | Yes | | 5 Docs/DX | 0.5 day | all (doc accuracy) | Yes | @@ -171,7 +173,7 @@ ## 5. Explicitly Out of Scope (do not implement without a decision) - **Server-side payload caching/session storage for export** — violates the no-persistence hard requirement (`MEMORY.md` 2026-05-12). Perf is solved via compression + streaming + client-side rendering instead. -- **Redis rate-limit storage by default** — only if multi-instance becomes real; document in `MEMORY.md` and switch via `RATELIMIT_STORAGE_URI`. +- **Redis rate-limit storage by default** — the default stays `memory://` with a **single worker and a single instance** (see 2.10). Redis (or any shared `RATELIMIT_STORAGE_URI`) becomes *required*, not optional, as soon as more than one worker or instance is run; v1.2 ships the guard-rail and the docs, not a bundled Redis dependency. - **Frontend framework / build step** — forbidden by project conventions. - **CSP relaxation** (e.g. `unsafe-inline`) — never; Phase 1 only *tightens* CSP. - **Payload-level logging / telemetry** — never; Phases 1.2/1.3 are about *removing* such leaks. @@ -182,8 +184,9 @@ ## 6. Definition of Done (v1.2.0) -- [ ] `python -m pytest tests/ -v` green (existing 82 + new tests). +- [ ] `python -m pytest tests/ -v` exits 0 (the command is the criterion, not a count). Baseline for v1.2 is the **78** tests remaining after Phase 0.8 removes the four `find_candidate_arrays` tests (82 − 4), plus every test added in Phases 1–5. - [ ] `pip-audit -r requirements.txt` → 0 vulnerabilities; CI (Phase 0.3) green on push/PR. +- [ ] Rate limiting is coherent with the shipped topology: the default deployment runs one worker and one instance on `memory://`, every multi-worker or multi-instance example in the docs sets a shared `RATELIMIT_STORAGE_URI`, and the declared-topology guard fires (Phase 2.10). - [ ] `ruff check .` and `ruff format --check .` exit 0. - [ ] Security Review §4 checklist passes (formula injection inert, SSRF battery, log hygiene via caplog, headers, fail-fast SECRET_KEY, depth guards, JSON error handlers, no-store, proxy-aware limiting). - [ ] Performance Review §4 budget met on the reference payload; 100k-row XLSX export does not OOM. diff --git a/docs/security-review-v1.2.md b/docs/security-review-v1.2.md index eb0843c..7a12951 100644 --- a/docs/security-review-v1.2.md +++ b/docs/security-review-v1.2.md @@ -124,7 +124,7 @@ Do not log the URL at all — query strings, fragments, userinfo, *and paths* ca | `Cross-Origin-Opener-Policy: same-origin` | Mitigates cross-origin window-opener attacks | | `Cross-Origin-Resource-Policy: same-origin` | Prevents other origins from embedding/reading our responses | -Also consider `upgrade-insecure-requests` in CSP for HTTPS-only deploys. +`upgrade-insecure-requests` is **required**, not optional: F14's completion criterion and the checklist both demand it, so the remediation here states it as mandatory for any deployment served over HTTPS (it is inert on plain-HTTP deployments, which are already out of policy per F14). **Remediation:** @@ -155,11 +155,14 @@ Build the CSP as a list of directives (or keep the trailing `;` in the base stri **What I verified as solid (do not regress):** decimal/hex IP forms (`2130706433`, `0x7f000001`) resolve via `getaddrinfo` to `127.0.0.1` and are rejected; IPv4-mapped IPv6 (`[::ffff:7f00:1]`) has `is_global=False` and is rejected; userinfo tricks (`http://evil@127.0.0.1`) yield hostname `127.0.0.1` and are rejected; `nip.io`-style wildcard domains resolve to real IPs and are checked; `allow_redirects=False` + `raise_for_status()` means 3xx responses never re-connect. **Remediation (item 1, High-ish priority):** -- Resolve DNS in a bounded way: run `getaddrinfo` in a **shared, module-level** `ThreadPoolExecutor` (fixed `max_workers`, e.g. 4) with an in-flight semaphore, and wait with `future.result(timeout=API_DNS_TIMEOUT)` (default ~3s). Do **not** create a per-request executor: `Future.result(timeout=...)` only bounds the caller's wait and cannot cancel a running `getaddrinfo`, so per-request executors leak blocked threads and a shared unbounded pool can saturate. A *hard* execution bound requires process isolation or a cancellable resolver — document the timeout as a wait bound and gate concurrency explicitly. +- Resolve DNS in a bounded way: run `getaddrinfo` in a **shared, per-process** `ThreadPoolExecutor` (fixed `max_workers`, e.g. 4) with an in-flight semaphore, and wait with `future.result(timeout=API_DNS_TIMEOUT)` (default ~3s). Do **not** create a per-request executor: `Future.result(timeout=...)` only bounds the caller's wait and cannot cancel a running `getaddrinfo`, so per-request executors leak blocked threads and a shared unbounded pool can saturate. A *hard* execution bound requires process isolation or a cancellable resolver — document the timeout as a wait bound and gate concurrency explicitly. Three details are load-bearing, because a timed-out lookup keeps running: + - **Permit ownership.** Acquire the semaphore permit *before* `submit()` and release it from the future's **done-callback** (`future.add_done_callback`), never in the caller's `finally` after a timeout. Releasing on caller timeout would re-admit work while the blocked `getaddrinfo` thread is still occupying the pool, which is exactly how the pool saturates under repeated slow-DNS requests. An equivalent formulation is a bounded submission queue whose capacity is the pool size plus a small backlog. When no permit is available within a short admission wait, reject fast (503 / 400 "DNS resolver busy") rather than queueing unboundedly. + - **Executor lifecycle across fork.** gunicorn forks workers; an executor created at import time in the master leaves its threads behind in the parent and is not inherited usefully by children. Create it **lazily inside the worker** on first use (module-level singleton behind a lock) or re-create it via `os.register_at_fork(after_in_child=...)`. Do **not** treat `shutdown(wait=False, cancel_futures=True)` as a non-blocking exit guarantee: `cancel_futures` only drops *queued* work, a running `getaddrinfo` cannot be cancelled, and `concurrent.futures` joins its non-daemon threads at interpreter exit whatever `wait` says. **Decided for v1.2: accept and document that there is no bounded teardown.** `security.py:24` calls `socket.getaddrinfo(hostname, None)` with no timeout — the call exposes none — and nothing in the repository defines `API_DNS_TIMEOUT`, an executor, or a resolver bound today. Adding `future.result(timeout=…)` bounds the *caller* only. The teardown wait is therefore whatever the platform resolver takes: glibc defaults to ~5s per nameserver × 2 attempts × every nameserver in `resolv.conf`, so **tens of seconds** is the realistic worst case, and it is bounded at all only when `options timeout:N attempts:M` are explicitly configured. So: do **not** state or imply that v1.2 bounds teardown. Ship the admission control (it bounds *concurrency*, which is the actual worker-starvation fix), document the unbounded recycling wait, optionally pin `options timeout:2 attempts:1` in the container's `resolv.conf` as best-effort, and keep a **killable subprocess** resolver as the escalation that would provide a real bound. The lifecycle tests assert this accepted behavior, not a bound. + - **Tests.** (a) Repeated timeouts: patch `getaddrinfo` to sleep well past `API_DNS_TIMEOUT`, issue more requests than `max_workers`, and assert every caller returns within its bound, that permits are not leaked (the in-flight counter returns to zero once the sleeping lookups finish), and that no unbounded thread growth occurs. (b) Saturation: with all permits held, a new request gets the fast admission error rather than blocking. (c) Lifecycle: the executor used by a request is the one created after fork, and teardown behaves as accepted. Do **not** assert a wall-clock bound on recycling: `API_DNS_TIMEOUT` bounds `Future.result()` only, the executor thread stays blocked until the platform resolver returns, and asserting a bound the code does not enforce yields a flaky or falsely reassuring test. Assert the *caller* and *admission* timeouts instead, and have teardown stay blocked until the mocked lookup is explicitly released — that is the documented behavior, so that is what the test should pin. - Add `API_ALLOWED_PORTS` (default `80,443,8443`) — cheap and kills most non-HTTP abuse. - Keep the residual-rebinding comment; document the port decision in `MEMORY.md`. -**Effort:** 1-2h. **Tests:** mock `getaddrinfo` to raise/sleep and assert bounded wall time; port rejection tests. +**Effort:** 1-2h. **Tests:** mock `getaddrinfo` to raise/sleep and assert the bounded *caller* wait (`Future.result`) and the admission error under saturation — never a bound on teardown, which nothing here enforces; port rejection tests. --- @@ -170,10 +173,11 @@ Build the CSP as a list of directives (or keep the trailing `;` in the base stri **Description:** `SECRET_KEY = os.environ.get('SECRET_KEY', 'dev-secret-key-change-in-production')`. If an operator forgets to set it (README calls it "Required: Yes"), the app silently runs with a *publicly known* key: CSRF tokens are forgeable and the session cookie can be signed. `render.yaml` sets `generateValue: true`, but self-hosted / Docker deploys from README rely on the operator. **Remediation:** -- In `create_app()`, after config load: when an **explicit production signal** (`APP_ENV=production` or `PRODUCTION=true`) is set and `SECRET_KEY` equals the dev default (or is `None`/empty), `raise RuntimeError('SECRET_KEY must be set in production')`. Do **not** infer production from `not DEBUG` — the documented local run `python app.py` has `DEBUG=False` by default, and gating on it would block ordinary development startup. +- In `create_app()`, after config load: when the production signal is set and `SECRET_KEY` equals the dev default (or is `None`/empty), `raise RuntimeError('SECRET_KEY must be set in production')`. +- **`APP_ENV=production` is the single canonical signal.** Do not also accept `PRODUCTION=true` or any alias: two accepted spellings mean a deployment can satisfy one gate and silently miss another. Concretely, if this rule accepted `PRODUCTION=true` while the cookie rule (F16) and the verification checklist read only `APP_ENV`, a deployment setting `PRODUCTION=true` with a valid key would pass the secret-key gate **with `SESSION_COOKIE_SECURE` still off** — a live vulnerability produced purely by the inconsistency. One name, checked by one helper (`is_production()`), used by F7, F16, and the 2.10 topology guard alike. Do **not** infer production from `not DEBUG` — the documented local run `python app.py` has `DEBUG=False` by default, and gating on it would block ordinary development startup. - Also validate that config integers are actually integers (env typo like `MAX_UPLOAD_SIZE=abc` currently raises `ValueError` at import time with a confusing traceback; wrap with clear messages). -**Effort:** 30m. **Tests:** with `APP_ENV=production` and `SECRET_KEY` unset (dev default), call `create_app()` through the real factory and assert `RuntimeError`; with `APP_ENV` unset (local dev), the same default key starts successfully; a valid `SECRET_KEY` under `APP_ENV=production` also starts. Do not construct `Config(SECRET_KEY=...)` — `Config` is a class with class attributes, not a constructor; drive behavior via environment variables or `app.config` mutation after load. +**Effort:** 30m. **Tests** — cover **every** branch this finding adds, not just the unset-key one: (a) `APP_ENV=production` + `SECRET_KEY` unset (dev default) → `RuntimeError`; (b) `APP_ENV=production` + **`SECRET_KEY=""`** (empty string — a distinct branch from unset, and the one a misconfigured secrets manager actually produces) → `RuntimeError`; (c) `APP_ENV=production` + a valid key → starts; (d) `APP_ENV` unset (local dev) + the default key → starts; (e) the integer-validation branch: `MAX_UPLOAD_SIZE=abc`, and one bad value for **each** integer setting (`PREVIEW_ROW_LIMIT`, `API_FETCH_TIMEOUT`, `API_FETCH_MAX_RESPONSE`, `FLATTEN_MAX_DEPTH`, `API_DNS_TIMEOUT`, `MAX_EXPORT_CELLS`, `APP_WORKERS`, `APP_REPLICAS`) → the documented clear error, not a raw `ValueError` traceback; (f) `PRODUCTION=true` alone is **not** honored as a production signal (guards against the two-spellings hole above). Do not construct `Config(SECRET_KEY=...)` — `Config` is a class with class attributes, not a constructor; drive behavior via environment variables or `app.config` mutation after load. --- @@ -237,7 +241,7 @@ Build the CSP as a list of directives (or keep the trailing `;` in the base stri **Remediation:** - Configure `ProxyFix` (Werkzeug) in `create_app()` with `x_for=1, x_proto=1, x_host=1` **only when behind a trusted proxy** (opt-in via env var, e.g. `TRUST_PROXY=1` — do not trust client-supplied `X-Forwarded-For` unconditionally, that's spoofable). - Alternatively set a custom `key_func` that uses the rightmost non-trusted hop. -- Keep `memory://` for single-instance; document Redis (`RATELIMIT_STORAGE_URI=redis://...`) as the multi-worker/multi-instance upgrade (project decision point, see roadmap). +- Keep `memory://` only for the **single-worker, single-instance** default. Counters are process-local, so the effective limit is multiplied by **`workers × replicas`**: any deployment above 1×1 must set a shared `RATELIMIT_STORAGE_URI` (`redis://...`) or it is not enforcing the configured limit — README's three `--workers 4` examples and roadmap 2.8's `--workers 2` are all affected, as is any Render `numInstances > 1`. Note `config.py:21` **hardcodes** `RATELIMIT_STORAGE_URI = 'memory://'`, so shared storage is not even configurable today; that is the first fix. The worker count must then come from **one enforced source**, not a second declaration that can drift: derive the start command from `WEB_CONCURRENCY` (`--workers "$WEB_CONCURRENCY"`) so the value the app validates is the value gunicorn uses, and enforce `APP_REPLICAS` at the deployment layer mirroring `render.yaml`'s `numInstances`. Defaulting either to `1` fails open — an undeclared 4-worker deployment reads as single-worker — so under `APP_ENV=production` both must be explicitly declared, and shared storage is required whenever either exceeds one **or cannot be verified**. Correct the deployment examples and test the process-local semantics explicitly rather than leaving them implied (roadmap 2.10). **Effort:** 1h. **Tests:** with `TRUST_PROXY=1`, assert `request.remote_addr` is the client IP from `X-Forwarded-For`.