From 6164eb932b3ef90d43376f129693f364de95ee4a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 19:00:12 +0000 Subject: [PATCH 1/6] docs: add a single status index for every planned point The four planning documents (roadmap, security review, performance review, code-health review) each track their own numbering, and none of them says what shipped. docs/plan-status.md indexes all of them in two sections -- done, and pending/planned -- with every "done" line checked against the code rather than against the CHANGELOG's own claims. Pending items it separates out: the open D4 Basic Auth decision, the JSONL export fidelity limitation awaiting a maintainer call, the accepted DNS teardown and rebinding exposures, and the carried-over engineering work (routes.py/config.py annotations and mypy, the unmeasured rows of the perf budget, the uncommitted measurement harness, transitive dependency pinning). Claude-Session: https://claude.ai/code/session_014hfwMqvSZ2zpvY2Mgkfxf7 --- docs/plan-status.md | 175 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 docs/plan-status.md diff --git a/docs/plan-status.md b/docs/plan-status.md new file mode 100644 index 0000000..812f480 --- /dev/null +++ b/docs/plan-status.md @@ -0,0 +1,175 @@ +# Planned Points — Consolidated Status + +**Date:** 2026-09-02 +**Shipped version:** `Config.APP_VERSION = '1.2.0'` (CHANGELOG entry dated 2026-08-21) + +Single index of every point planned across the four planning documents: + +- `docs/roadmap-v1.2.md` — Phases 0–5, decisions D1–D6 +- `docs/security-review-v1.2.md` — findings F1–F17 +- `docs/performance-review-v1.2.md` — findings P1–P13, perf budget §4 +- `docs/code-health-final.md` — 2026-05 review: top-10 issues, quick wins, larger refactors +- `docs/export-budget-v1.2.md` — how `MAX_EXPORT_CELLS` was derived + +Status is against the code in this repository, not against the CHANGELOG's own +claims: every "Done" line below was checked against `config.py`, `routes.py`, +`helpers.py`, `security.py`, `static/js/app.js`, `Makefile`, `.github/workflows/ci.yml` +and `requirements*.txt`. + +--- + +## Section 1 — Done + +### 1.1 Security findings (F1–F17) + +| ID | Point | Where it landed | +|---|---|---| +| F1 | CSV/XLSX formula injection (Critical) | `helpers.sanitize_cell` / `is_formula_trigger` for CSV/TSV/XLSX; XLSX pins `data_type='s'`; JSONL/Markdown deliberately exempt | +| F2 | 7 dependency CVEs | Flask 3.1.3, requests 2.33.0, gunicorn 22.0.0, openpyxl 3.1.5; pytest moved out of the runtime file into `requirements-dev.txt`; `pip-audit -r requirements.txt` runs in CI | +| F3 / F9 | Credential leakage into logs; API JSONL `ValueError` → 500 | Fixed log string with no interpolation; JSONL parse failure returns 400 | +| F4 | User-controlled outbound header name | `routes.is_allowed_outbound_header` — strip + lowercase, then explicit allowlist | +| F5 | Security-header gaps | HSTS on secure requests, `Permissions-Policy`, COOP, CORP; CSP gained `object-src`, `base-uri`, `frame-ancestors`, `form-action`, `upgrade-insecure-requests` | +| F6 | SSRF: blocking DNS, no port restriction | Bounded resolver pool with admission control (`API_DNS_*`); `API_ALLOWED_PORTS` default `80,443,8443` | +| F7 | Default SECRET_KEY accepted in production | `is_production()` fail-fast in the app factory; `env_int()` names the mistyped variable | +| F8 | Recursion-depth DoS | `extract_table_data` depth cap; `RecursionError` → 400 `JSON nesting too deep` | +| F10 | 413 returned HTML | JSON handlers for 413/500/404 | +| F11 | No `Cache-Control: no-store` on data responses | `security.NO_STORE_ENDPOINTS` | +| F12 | Rate limiting ineffective behind a proxy | Opt-in `TRUST_PROXY=1` → `ProxyFix` with exactly one hop; key derived after ProxyFix | +| F13 | Upload extension/content-type unchecked | `routes.validate_upload` | +| F14 | Plain-HTTP credential exposure | Docs-only remediation: HTTPS stated per deploy path; `upgrade-insecure-requests` in CSP | +| F15 | `/health` version disclosure | Kept on by default, gated by `HEALTH_REVEAL_VERSION` | +| F16 | Cookie hardening | `HttpOnly`, `SameSite=Lax`, `Secure` tied to `APP_ENV=production` | +| F17 | Documentation drift | Candidates-handshake references replaced with the tree picker; MIT → GPL-3.0 | + +### 1.2 Performance findings (P1–P13) + +| ID | Point | Where it landed | +|---|---|---| +| P1 | Uncompressed `/process` responses | In-repo gzip middleware (D1), no new dependency; honours `Accept-Encoding` q-values; skips streamed/bodyless/already-encoded responses | +| P2.2 / P5 | Preview mutation, unbounded nested rendering | `helpers.preview_truncate` returns a capped **copy**; client caps at 20 keys / 20 items / 500 chars | +| P3 | XLSX fully buffered in memory (High) | Diskless normal-mode workbook + `MAX_EXPORT_CELLS` (measured, default 250 000); CSV/TSV generator-streamed and uncapped; 400 rather than truncation | +| P4 | Eager tree-picker build | Lazy children on first toggle, per-level and total node caps | +| P6 | No static cache headers | `STATIC_MAX_AGE` (86 400) with `?v=APP_VERSION` URLs | +| P7 | Blocking DNS in the request path | Shared bounded resolver pool (shared with F6) | +| P8 / P12 | Double column scan; multi-copy memory | `flatten_rows` does one pass; API path decodes the `bytearray` without an intermediate copy | +| P9 | gunicorn timeout vs `API_FETCH_TIMEOUT` | `--timeout 60` everywhere; the invariant is now enforced at startup | +| P10 | Dead code `find_candidate_arrays` | Removed with its four tests (D2) | +| P11 | Hardcoded "25" preview badge | `/process` returns `preview_limit` | +| P13 | Client-side CSV/TSV string building | Chunked Blob builders | + +### 1.3 Roadmap phases + +- **Phase 0 — Foundations.** Dependency pins, `requirements-dev.txt`, `.github/workflows/ci.yml`, `pyproject.toml` (ruff + pytest), ruff clean, GPL-3.0 correction, `autoDeployTrigger: checksPass`, dead-code removal. +- **Phase 1 — Security.** All 15 tasks (1.1–1.15); see F-table above. +- **Phase 2 — Performance.** All 10 tasks (2.1–2.10), including the rate-limit topology guard: `RATELIMIT_STORAGE_URI` is configurable, `WEB_CONCURRENCY` is the single source of truth for workers, `APP_REPLICAS` mirrors `numInstances`, and production refuses to start on `memory://` above one worker × one replica. +- **Phase 3 — Refactor.** `_load_input` / `_select_table_data` extracted, `process_json` reduced, `openpyxl` imported at module top, `preview_limit` returned, type annotations added to `helpers.py` and `security.py`. +- **Phase 4 — Features.** 4.1 load more / load all with a 50 000-row DOM guard, 4.2 row filter, 4.3 JSONL + Markdown exports, 4.4 column visibility, 4.5 `#path=` deep links, 4.7 `/health/live` + `/health/ready`, 4.8 in-page About modal and keyboard-accessible export dropdown. (4.6 is not done — see Section 2.) +- **Phase 5 — Docs/DX.** `MEMORY.md`, `CLAUDE.md`, `AGENTS.md`, `README.md` synced; `.env.example`; `Makefile`; `CHANGELOG.md`; version bumped to 1.2.0. + +### 1.4 Decisions closed + +| ID | Decision | +|---|---| +| D1 | gzip via in-repo middleware, not `Flask-Compress` | +| D2 | `find_candidate_arrays` deleted in Phase 0, before the depth-guard work | +| D3 | Proxy trust is opt-in via `TRUST_PROXY`, off by default | +| D5 | Port allowlist adopted, default `80,443,8443` | +| D6 | Exports stay diskless and memory-bounded: normal-mode workbook + measured cell budget; `write_only` and `SpooledTemporaryFile` both rejected, each on its own grounds | + +### 1.5 Code-health review (2026-05) items now closed + +Top-10 issues 1, 2, 3, 4, 5, 7, 8, 9, 10 are done (CI, CVEs, dev/prod dependency +split, ruff, `process_json` complexity, preview badge, LICENSE, `.env.example`, +depth guard). Quick wins 1–8 are done. Larger refactors 7.1, 7.3, 7.4 and 7.5 are +done; 7.2 is partial (see Section 2). + +### 1.6 Measurement work + +`MAX_EXPORT_CELLS = 250 000` is a measured number, derived by the two-fresh-worker +`ru_maxrss` protocol recorded in `docs/export-budget-v1.2.md`, with a confirming +point re-run at the shipped value. + +--- + +## Section 2 — Pending / Planned + +### 2.1 Open decision + +- **D4 — opt-in HTTP Basic Auth gate (roadmap 4.6).** Still unresolved; needs + maintainer sign-off. Scope if approved: `APP_BASIC_AUTH_USER` / `APP_BASIC_AUTH_PASS` + → `before_request` 401 with constant-time compare and `WWW-Authenticate`, off by + default, no persistence. Nothing else depends on it. The roadmap's own + recommendation is to approve. No `BASIC_AUTH` code exists in the repository today. + +### 2.2 Known limitation awaiting a decision + +- **JSONL export is not a faithful copy of the input document.** Roadmap 4.3 called + it "lossless — original values". Shipped behaviour: values are written verbatim + (the security-relevant half), but over the server's *flattened* projection, so + `{"tags": [1,2]}` exports as `"tags": "[1, 2]"` and `{"meta": {"role": "x"}}` as + `"meta.role": "x"`. Fixing it means shipping the original rows alongside the + flattened ones, doubling the payload and client memory that P2/P12 exist to + reduce. Explicitly flagged for a maintainer decision, not resolved either way. + +### 2.3 Accepted exposures, documented rather than fixed + +- **Unbounded DNS teardown (F6.1 / P7 residual).** `getaddrinfo` exposes no timeout + and cannot be cancelled. `API_DNS_TIMEOUT` bounds only the caller's wait; a pool + thread stays occupied until the platform resolver returns, and interpreter exit + can still block on it — realistically tens of seconds with glibc defaults. A + killable subprocess resolver is the documented escalation and remains out of v1.2 + scope. Best-effort narrowing where the deployment allows: pin + `options timeout:2 attempts:1` in the container's `resolv.conf`. +- **DNS rebinding (SSRF residual).** Mitigated by `allow_redirects=False` and + documented in `routes.py`. Escalation path if the threat model changes from + internal tool to public service: a custom `HTTPAdapter` that resolves once and + passes the IP with an explicit `Host` header. + +### 2.4 Carried-over engineering work + +- **Type annotations (code-health 7.2, roadmap 3.5) — partial.** `helpers.py` and + `security.py` are annotated. `routes.py` and `config.py` have no annotated + signatures, and `mypy` is not in `requirements-dev.txt`, `pyproject.toml`, the + `Makefile`, or CI. The roadmap deliberately marked mypy optional and skipped + strict mode; the remaining work is the two unannotated modules plus a mypy + configuration if it is wanted. +- **Perf budget verification (performance review §4).** The XLSX row of the budget + was measured. The rest of the table — `/process` transfer size ≤ 2–4 MB, + `/process` p95 ≤ 3 s, `/process` peak-RSS delta ≤ 50 MiB, tree-picker open + ≤ 500 ms on a 10 MB payload — is defined but has no recorded run. No perf test + gates CI (`pytest-benchmark` was deferred by decision), so these stay manual. +- **Measurement harness is not committed.** The export-budget script is a throwaway; + re-deriving the budget means rebuilding it from the method section. Committing + fixtures/generators for the 10 MB reference payload and the 100 k-row `csv_data` + body would make the budget reproducible rather than re-derivable. +- **Transitive dependency drift (code-health §12).** Direct dependencies are + exact-pinned; transitives are not. A lock file (`pip-compile`) was noted as a + post-review iteration and has not been added. +- **Docker artifacts.** `README.md` carries Dockerfile and docker-compose templates, + but no `Dockerfile` is checked in. Phase 5 deliberately kept these docs-only; a + committed Dockerfile is a fresh decision, not pending roadmap work. + +### 2.5 Explicitly out of scope — do not implement without a new decision + +These are settled exclusions, listed so they are not mistaken for a backlog: + +- Server-side payload caching or session storage for export — violates the + no-persistence requirement. Perf is served by compression, streaming and + client-side rendering instead. +- Redis rate-limit storage as the default. It ships as an optional + `requirements-redis.txt`, required only above one worker or one replica. +- Any frontend framework or build step. +- Any CSP relaxation, `unsafe-inline` included. Phase 1 only tightened it. +- Payload-level logging or telemetry. +- Auto keep-alive pings against the Render free tier. +- Committed `docker-compose` / systemd unit files. + +--- + +## Verification checklists still requiring a manual sign-off + +Two lines in the security review's post-fix checklist are marked manual and have +no automated equivalent: **F14** (every deployment path states HTTPS is required) +and **F17** (no stale `candidates` or MIT references remain in the docs). The rest +of that checklist is covered by `tests/test_routes.py`, `tests/test_security.py` +and the Node assertions in `tests/js/`. From 05db70b0d233f0d4a29a0dd7e033c1e0713707db Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 19:05:25 +0000 Subject: [PATCH 2/6] =?UTF-8?q?docs:=20record=20D4=20as=20declined=20?= =?UTF-8?q?=E2=80=94=20no=20Basic=20Auth=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D4 was the last open decision in the v1.2 planning set. It is declined: no app-level authentication gate ships, and roadmap task 4.6 plus its acceptance line are dropped. Per the roadmap's own dependency note, nothing else in v1.2 changes. D4 moves from "open decision" in Section 2 to the closed-decisions table, and the exclusion is recorded in the out-of-scope list so it is not read back later as unfinished work. Section 2 renumbers accordingly; JSONL export fidelity is now the only item awaiting a maintainer call. Claude-Session: https://claude.ai/code/session_014hfwMqvSZ2zpvY2Mgkfxf7 --- docs/plan-status.md | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/docs/plan-status.md b/docs/plan-status.md index 812f480..50ed10b 100644 --- a/docs/plan-status.md +++ b/docs/plan-status.md @@ -5,7 +5,7 @@ Single index of every point planned across the four planning documents: -- `docs/roadmap-v1.2.md` — Phases 0–5, decisions D1–D6 +- `docs/roadmap-v1.2.md` — Phases 0–5, decisions D1–D6 (all six now closed) - `docs/security-review-v1.2.md` — findings F1–F17 - `docs/performance-review-v1.2.md` — findings P1–P13, perf budget §4 - `docs/code-health-final.md` — 2026-05 review: top-10 issues, quick wins, larger refactors @@ -63,7 +63,7 @@ and `requirements*.txt`. - **Phase 1 — Security.** All 15 tasks (1.1–1.15); see F-table above. - **Phase 2 — Performance.** All 10 tasks (2.1–2.10), including the rate-limit topology guard: `RATELIMIT_STORAGE_URI` is configurable, `WEB_CONCURRENCY` is the single source of truth for workers, `APP_REPLICAS` mirrors `numInstances`, and production refuses to start on `memory://` above one worker × one replica. - **Phase 3 — Refactor.** `_load_input` / `_select_table_data` extracted, `process_json` reduced, `openpyxl` imported at module top, `preview_limit` returned, type annotations added to `helpers.py` and `security.py`. -- **Phase 4 — Features.** 4.1 load more / load all with a 50 000-row DOM guard, 4.2 row filter, 4.3 JSONL + Markdown exports, 4.4 column visibility, 4.5 `#path=` deep links, 4.7 `/health/live` + `/health/ready`, 4.8 in-page About modal and keyboard-accessible export dropdown. (4.6 is not done — see Section 2.) +- **Phase 4 — Features.** 4.1 load more / load all with a 50 000-row DOM guard, 4.2 row filter, 4.3 JSONL + Markdown exports, 4.4 column visibility, 4.5 `#path=` deep links, 4.7 `/health/live` + `/health/ready`, 4.8 in-page About modal and keyboard-accessible export dropdown. (4.6 was dropped with D4 — see §1.4.) - **Phase 5 — Docs/DX.** `MEMORY.md`, `CLAUDE.md`, `AGENTS.md`, `README.md` synced; `.env.example`; `Makefile`; `CHANGELOG.md`; version bumped to 1.2.0. ### 1.4 Decisions closed @@ -75,6 +75,7 @@ and `requirements*.txt`. | D3 | Proxy trust is opt-in via `TRUST_PROXY`, off by default | | D5 | Port allowlist adopted, default `80,443,8443` | | D6 | Exports stay diskless and memory-bounded: normal-mode workbook + measured cell budget; `write_only` and `SpooledTemporaryFile` both rejected, each on its own grounds | +| D4 | **Declined (2026-09-02).** No Basic Auth gate. Roadmap task 4.6 and its acceptance line are dropped; per the roadmap, nothing else changes | ### 1.5 Code-health review (2026-05) items now closed @@ -93,15 +94,7 @@ point re-run at the shipped value. ## Section 2 — Pending / Planned -### 2.1 Open decision - -- **D4 — opt-in HTTP Basic Auth gate (roadmap 4.6).** Still unresolved; needs - maintainer sign-off. Scope if approved: `APP_BASIC_AUTH_USER` / `APP_BASIC_AUTH_PASS` - → `before_request` 401 with constant-time compare and `WWW-Authenticate`, off by - default, no persistence. Nothing else depends on it. The roadmap's own - recommendation is to approve. No `BASIC_AUTH` code exists in the repository today. - -### 2.2 Known limitation awaiting a decision +### 2.1 Known limitation awaiting a decision - **JSONL export is not a faithful copy of the input document.** Roadmap 4.3 called it "lossless — original values". Shipped behaviour: values are written verbatim @@ -111,7 +104,7 @@ point re-run at the shipped value. flattened ones, doubling the payload and client memory that P2/P12 exist to reduce. Explicitly flagged for a maintainer decision, not resolved either way. -### 2.3 Accepted exposures, documented rather than fixed +### 2.2 Accepted exposures, documented rather than fixed - **Unbounded DNS teardown (F6.1 / P7 residual).** `getaddrinfo` exposes no timeout and cannot be cancelled. `API_DNS_TIMEOUT` bounds only the caller's wait; a pool @@ -125,7 +118,7 @@ point re-run at the shipped value. internal tool to public service: a custom `HTTPAdapter` that resolves once and passes the IP with an explicit `Host` header. -### 2.4 Carried-over engineering work +### 2.3 Carried-over engineering work - **Type annotations (code-health 7.2, roadmap 3.5) — partial.** `helpers.py` and `security.py` are annotated. `routes.py` and `config.py` have no annotated @@ -149,7 +142,7 @@ point re-run at the shipped value. but no `Dockerfile` is checked in. Phase 5 deliberately kept these docs-only; a committed Dockerfile is a fresh decision, not pending roadmap work. -### 2.5 Explicitly out of scope — do not implement without a new decision +### 2.4 Explicitly out of scope — do not implement without a new decision These are settled exclusions, listed so they are not mistaken for a backlog: @@ -163,6 +156,9 @@ These are settled exclusions, listed so they are not mistaken for a backlog: - Payload-level logging or telemetry. - Auto keep-alive pings against the Render free tier. - Committed `docker-compose` / systemd unit files. +- **Any app-level authentication gate**, HTTP Basic Auth included (roadmap 4.6 / D4, + declined 2026-09-02). Access control stays a deployment concern — put the app + behind whatever the network or reverse proxy already enforces. --- From f2eecfb146997327485c3036b98b54929e92bd71 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 19:12:12 +0000 Subject: [PATCH 3/6] docs: consolidate the five planning documents into one docs/ held five separate files with five numbering schemes and no single answer to "what shipped". They are replaced by docs/plan-status-v1.2.md. The two things later work has to execute are carried over in full rather than summarized: Appendix A is the XLSX export-budget measurement (method, the eight-shape data table, the fit, and how to reproduce it), which CLAUDE.md's "Re-deriving the Excel export budget" task depends on; Appendix B is the perf budget with the ru_maxrss protocol and the design constraints that bound any future fix. Dropped: each finding's own location/description/effort write-up and the 2026-05 category scores -- narrative about shipped work. The full text stays in git history at 065883f. Every reference to a deleted path is repointed: config.py (2), README.md, CLAUDE.md (the tree and the re-derivation task), MEMORY.md, CHANGELOG.md. No dangling links remain. Claude-Session: https://claude.ai/code/session_014hfwMqvSZ2zpvY2Mgkfxf7 --- CHANGELOG.md | 9 +- CLAUDE.md | 16 +- MEMORY.md | 2 +- README.md | 2 +- config.py | 10 +- docs/code-health-final.md | 695 -------------------------------- docs/export-budget-v1.2.md | 113 ------ docs/performance-review-v1.2.md | 262 ------------ docs/plan-status-v1.2.md | 348 ++++++++++++++++ docs/plan-status.md | 171 -------- docs/roadmap-v1.2.md | 197 --------- docs/security-review-v1.2.md | 350 ---------------- 12 files changed, 367 insertions(+), 1808 deletions(-) delete mode 100644 docs/code-health-final.md delete mode 100644 docs/export-budget-v1.2.md delete mode 100644 docs/performance-review-v1.2.md create mode 100644 docs/plan-status-v1.2.md delete mode 100644 docs/plan-status.md delete mode 100644 docs/roadmap-v1.2.md delete mode 100644 docs/security-review-v1.2.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 64ec424..693b9d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,10 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [1.2.0] - 2026-08-21 — "Hardening & Performance" -Implements `docs/roadmap-v1.2.md`, closing every finding in -`docs/security-review-v1.2.md` (F1–F17) and `docs/performance-review-v1.2.md` -(P1–P13). +Implements the v1.2 roadmap, closing every security finding (F1–F17) and +performance finding (P1–P13). Those four planning documents were later +consolidated into `docs/plan-status-v1.2.md`; their full text remains in git +history at `065883f`. No response key changed name, type or meaning; `/process` only gained keys. @@ -68,7 +69,7 @@ No response key changed name, type or meaning; `/process` only gained keys. - **Diskless, memory-bounded exports (P3).** CSV/TSV are generator-streamed and uncapped. XLSX keeps a normal-mode workbook (no OS temp files) plus `MAX_EXPORT_CELLS`, measured rather than guessed — see - `docs/export-budget-v1.2.md`. + `docs/plan-status-v1.2.md`, Appendix A. - **Lazy tree picker (P4)** and **client render caps (P5)**: children build on first toggle; nested objects stop at 20 keys, primitive arrays at 20 items, strings at 500 characters. diff --git a/CLAUDE.md b/CLAUDE.md index 450ade2..8b3a583 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -36,11 +36,9 @@ json-table-tool/ │ ├── test_render_caps.mjs │ └── test_features.mjs ├── docs/ -│ ├── code-health-final.md -│ ├── security-review-v1.2.md -│ ├── performance-review-v1.2.md -│ ├── roadmap-v1.2.md -│ └── export-budget-v1.2.md # How MAX_EXPORT_CELLS was measured +│ └── plan-status-v1.2.md # The planning doc: what shipped, what is pending, +│ # the export-budget measurement (App. A) and the +│ # perf budget + RSS protocol (App. B) ├── .github/workflows/ci.yml # lint, format, tests, JS assertions, pip-audit ├── pyproject.toml # ruff + pytest configuration ├── requirements.txt # Runtime dependencies (exact-pinned) @@ -317,7 +315,7 @@ convention is a dedicated bump commit, not a bump ridden along with a feature. ### Re-deriving the Excel export budget `MAX_EXPORT_CELLS` is a measured number, not a chosen one. Follow the method in -`docs/export-budget-v1.2.md` (fresh process per measured request, `ru_maxrss` -converted for the platform, delta across two runs), re-fit against the narrowest -aspect ratio you care about, and re-run a confirming point at the value you -intend to ship. Record the new data in that file. +`docs/plan-status-v1.2.md` Appendix A (fresh process per measured request, +`ru_maxrss` converted for the platform, delta across two runs), re-fit against +the narrowest aspect ratio you care about, and re-run a confirming point at the +value you intend to ship. Record the new data in that appendix. diff --git a/MEMORY.md b/MEMORY.md index 8e1154c..83c6607 100644 --- a/MEMORY.md +++ b/MEMORY.md @@ -154,7 +154,7 @@ Keep entries short — if it grows past ~10 lines, it probably belongs in `READM ### 2026-08-21 — The XLSX export budget is measured, in cells, and on by default (area: performance) **What:** `MAX_EXPORT_CELLS` (default 250,000) caps Excel exports only. CSV/TSV are generator-streamed and stay uncapped. -**Why:** openpyxl memory tracks `rows × columns`, not rows — at equal cell counts a narrow, tall sheet costs *more* (84.3 MiB at 50k×3 vs 73.6 at 15k×10), so a row limit says almost nothing about the footprint. The default comes from the measurement in `docs/export-budget-v1.2.md`, not from feel. An unlimited default would leave a High finding unmitigated; uncapped CSV/TSV is what keeps the export contract as wide as the input contract. +**Why:** openpyxl memory tracks `rows × columns`, not rows — at equal cell counts a narrow, tall sheet costs *more* (84.3 MiB at 50k×3 vs 73.6 at 15k×10), so a row limit says almost nothing about the footprint. The default comes from the measurement in `docs/plan-status-v1.2.md` (Appendix A), not from feel. An unlimited default would leave a High finding unmitigated; uncapped CSV/TSV is what keeps the export contract as wide as the input contract. **How to apply:** Re-derive the number whenever the measurement is re-run — do not round it to something tidy. Never truncate an oversized export: `/process` advertises `total_cells`/`max_export_cells` so the client greys Excel out beforehand, and `/export-xlsx` returns 400 for direct callers. And keep exports diskless: openpyxl's `write_only` mode writes worksheet parts to OS temp files, and `SpooledTemporaryFile` is either pointless (its default `max_size=0` never rolls over) or disk-backed. **Owner / source:** performance review P3, decision D6. diff --git a/README.md b/README.md index eccfb76..6a9ca1d 100644 --- a/README.md +++ b/README.md @@ -559,7 +559,7 @@ need no server round trip and nothing is persisted — responses over `GZIP_MIN_SIZE` are gzipped to keep that affordable. The preview rows are a truncated *copy*, so exports keep full fidelity. Excel exports are bounded by `MAX_EXPORT_CELLS`, measured rather than guessed -(`docs/export-budget-v1.2.md`); CSV and TSV stream and stay uncapped. The JSON +(`docs/plan-status-v1.2.md`, Appendix A); CSV and TSV stream and stay uncapped. The JSON tree picker builds children only when a node is opened. --- diff --git a/config.py b/config.py index 32d3562..30002d7 100644 --- a/config.py +++ b/config.py @@ -5,8 +5,8 @@ # Publicly known, and therefore only ever acceptable outside production. DEV_SECRET_KEY = 'dev-secret-key-change-in-production' -# See MAX_EXPORT_CELLS below and docs/export-budget-v1.2.md for how this number -# was measured. +# See MAX_EXPORT_CELLS below and docs/plan-status-v1.2.md (Appendix A) for how +# this number was measured. DEFAULT_MAX_EXPORT_CELLS = 250_000 @@ -164,9 +164,9 @@ class Config: # 500 columns have wildly different footprints at the same row count. # # Enabled by default: an unlimited default would leave P3 (High) unmitigated. - # The value is derived from the Performance Review section 4 measurement (see - # docs/export-budget-v1.2.md), not chosen by feel -- re-derive it whenever that - # measurement is re-run. 0 disables the guard for operators who knowingly opt + # The value is derived from the measurement in docs/plan-status-v1.2.md + # (Appendix A), not chosen by feel -- re-derive it whenever that measurement + # is re-run. 0 disables the guard for operators who knowingly opt # out. CSV/TSV stay uncapped and streamed, so every dataset /process accepts # remains exportable by some route. MAX_EXPORT_CELLS = env_int('MAX_EXPORT_CELLS', DEFAULT_MAX_EXPORT_CELLS) diff --git a/docs/code-health-final.md b/docs/code-health-final.md deleted file mode 100644 index 6c854d0..0000000 --- a/docs/code-health-final.md +++ /dev/null @@ -1,695 +0,0 @@ -# Code-Health Review — JSON Table Converter - -**Date:** 2026-05-02 -**Reviewer:** Automated senior code-health review -**Repository:** https://github.com/Badry-Kudu/json-table-tool -**Revision reviewed:** `main` branch (HEAD at time of review) - ---- - -## 1. Initial Score Estimate - -| Category | Score | Weight | -|---|---|---| -| Maintainability | 7/10 | 10% | -| Test coverage & quality | 8/10 | 15% | -| Complexity | 7/10 | 10% | -| Duplication | 8/10 | 5% | -| Architecture consistency | 8/10 | 10% | -| Dependency / security risk | 3/10 | 15% | -| Documentation | 7/10 | 10% | -| CI/CD reliability | 0/10 | 15% | -| Input validation & error handling | 9/10 | 10% | -| Usability & developer experience | 7/10 | 10% | - -**Overall weighted score: ~68 / 100** - -**Target score: 85+ / 100** - ---- - -## 2. Project Overview (Verified) - -**Stack:** Python 3.11+, Flask 3.0.0 (app-factory pattern), Flask-WTF (CSRF), Flask-Limiter, gunicorn, requests, openpyxl. Frontend: Vanilla HTML / CSS / JS (no framework, no build step). Tests: pytest. - -**Purpose:** Lightweight stateless web tool that converts JSON and JSONL data into viewable HTML tables, with CSV, TSV, and Excel export. Supports file upload, paste, and external API fetch with auth. - -**Directory layout:** -``` -app.py Flask app factory -config.py All settings via env vars -extensions.py Shared CSRF + limiter instances -security.py SSRF validation + security headers -helpers.py Data parsing, flattening, path selection -routes.py Blueprint with /, /health, /process, /export-csv, /export-xlsx -static/ css/style.css js/app.js -templates/ index.html (structure only, no inline scripts/styles) -tests/ conftest.py, test_helpers.py, test_routes.py, test_security.py -render.yaml Render.com deployment blueprint -requirements.txt -``` - ---- - -## 3. Commands Run & Results - -### Install -```bash -pip install -r requirements.txt -# → Success. All 7 direct dependencies installed. -``` - -### Tests -```bash -python -m pytest tests/ -v -# → 82 passed, 2 warnings (openpyxl DeprecationWarning from library internals), 0 failures -``` - -### Coverage -```bash -coverage run -m pytest tests/ && coverage report -m -``` - -| File | Stmts | Miss | Cover | -|---|---|---|---| -| app.py | 16 | 1 | 94% | -| config.py | 14 | 0 | 100% | -| extensions.py | 5 | 0 | 100% | -| helpers.py | 78 | 1 | 99% | -| routes.py | 181 | 33 | 82% | -| security.py | 33 | 3 | 91% | -| tests/ | 446 | 0 | 100% | -| **TOTAL** | **773** | **38** | **95%** | - -### Dependency Audit -```bash -pip-audit -r requirements.txt -``` -**Found 7 known vulnerabilities in 4 packages:** - -| Package | Current | CVE | Fix Version | Severity | -|---|---|---|---|---| -| Flask | 3.0.0 | CVE-2026-27205 | 3.1.3 | — | -| requests | 2.31.0 | CVE-2024-35195 | 2.32.0 | Medium | -| requests | 2.31.0 | CVE-2024-47081 | 2.32.4 | Medium | -| requests | 2.31.0 | CVE-2026-25645 | 2.33.0 | — | -| gunicorn | 21.2.0 | CVE-2024-1135 | 22.0.0 | High | -| gunicorn | 21.2.0 | CVE-2024-6827 | 22.0.0 | — | -| pytest | 7.4.4 | CVE-2025-71176 | 9.0.3 | — | - -### Linting / Formatting -``` -No linting or formatting tools are configured for this project. -``` -Run (locally or in CI): -```bash -pip install ruff -ruff check . -ruff format --check . -``` - -### Type checking -``` -No type annotations present. No mypy or pyright configured. -``` - -### CI/CD Workflows -``` -No .github/workflows/ directory found. Zero CI/CD configuration. -``` - ---- - -## 4. Category-by-Category Findings - -### 4.1 Maintainability — 7/10 - -**Strengths:** -- Clean app-factory pattern (`create_app()`) with proper extension initialization. -- Well-separated modules: `config.py`, `extensions.py`, `security.py`, `helpers.py`, `routes.py`. -- Consistent docstrings on all public functions. -- Naming conventions are clear and consistent throughout. - -**Issues:** -- `process_json()` in `routes.py` is ~150 lines with three nested branches (file/paste/api), five auth sub-branches, and a post-parse block — cyclomatic complexity ≈ 15+. -- No type annotations anywhere. With Flask's dynamic nature, type hints would catch many errors at development time. -- No linter or formatter configured; code style is consistent now but will drift as contributors add code. -- `openpyxl` is imported lazily inside `export_xlsx()` — inconsistent with all other imports at module top, and hides ImportError until runtime. - -### 4.2 Test Coverage & Quality — 8/10 - -**Strengths:** -- **95% overall coverage** — exceptional for a Flask project. -- 82 tests across three well-organized files (`test_helpers`, `test_routes`, `test_security`). -- All auth paths tested via mock. SSRF blocking well-covered (16 URL validation tests). -- Edge cases covered: UTF-8 decoding failure, max-size exceeded, JSONL invalid line, path selection modal flow. - -**Issues:** -- `routes.py` is 82% covered; uncovered lines include auth header/key/bearer/query-param branches (lines 92–109) and the `ValueError` path in `parse_jsonl` when called from API fetch. -- No edge-case input tests for helpers: `null` values in JSON objects, Unicode / emoji column names, boolean-only arrays, numbers as top-level JSON, empty-object arrays `[{}, {}]`, extremely large number of columns. -- No test confirming the `PREVIEW_ROW_LIMIT` server config is reflected in the JS badge (hardcoded "25" in `app.js` line 219 and 220 is inconsistent with server config). -- No test for file upload with no `json_file` field (line 50 branch is untested). - -### 4.3 Complexity — 7/10 - -**Strengths:** -- `helpers.py` functions are small, single-purpose, and easy to reason about. -- `security.py` is clear and minimal. - -**Issues:** -- `process_json()` has high cyclomatic complexity and should be refactored: extract `_parse_input(request, data_format)` → returns `(json_data, error_response)` and `_apply_path_selection(json_data, json_path)` → returns `(table_data, error_response)`. -- `extract_table_data()` falls through multiple isinstance checks with no early return at each branch; a match/case or helper would improve clarity (Python 3.10+). -- `renderTable` / `renderTableDOM` split in `app.js` is unnecessary — `renderTable` just stores refs then calls `renderTableDOM`, creating indirection without benefit. - -### 4.4 Duplication — 8/10 - -**Strengths:** -- `escapeHtml()` is defined once and reused across all JS rendering functions — no XSS risk from duplication. -- `get_all_columns()` is reused for both preview and CSV columns. - -**Issues:** -- `export_csv` and `export_xlsx` both implement identical `dict/list → JSON string` serialization for cell values: - ```python - if isinstance(v, (dict, list)): - clean_row[k] = json.dumps(v) - ``` - This logic should live in a shared helper (e.g., `serialize_cell_value(v)`). -- `downloadDelimited` in `app.js` and `export_csv` route in `routes.py` both implement CSV serialization independently. This is intentional (client-side fallback + server fallback) but should be documented as such. - -### 4.5 Architecture Consistency — 8/10 - -**Strengths:** -- Blueprint pattern used correctly. Extensions initialized with `init_app()` pattern. -- Security headers applied globally via `after_request`. No inline styles/scripts (CSP-safe). -- Config exclusively from environment variables with documented defaults. - -**Issues:** -- `app = create_app()` at module level in `app.py` (line 28) is fine for `python app.py` dev use, but means importing `app` in tests or tools always creates an app instance with production config. Tests avoid this by calling `create_app()` directly, but it's a subtle trap. -- `RATE_LIMIT_PROCESS` / `RATE_LIMIT_EXPORT` are stored as custom config keys rather than using Flask-Limiter's standard `RATELIMIT_*` namespace, requiring the lambda wrapper on every route decorator. -- No `pyproject.toml` or `setup.cfg` — project metadata (name, version, Python constraint) exists only in `config.py` and `render.yaml`, not in a standard packaging file. - -### 4.6 Dependency / Security Risk — 3/10 - -**Strengths:** -- All direct dependencies are pinned to exact versions — no floating ranges. -- Minimal dependency footprint: 7 direct deps for a full web app is lean. -- No unused dependencies found. - -**Issues:** -- **7 CVEs across 4 packages** — all have published fixes: - - `gunicorn 21.2.0` → CVE-2024-1135 (HTTP Request Smuggling, **High**) + CVE-2024-6827. Fix: `gunicorn==22.0.0`. - - `requests 2.31.0` → 3 CVEs. Fix: `requests==2.33.0` (supersedes all three). - - `Flask 3.0.0` → CVE-2026-27205. Fix: `Flask==3.1.3`. - - `pytest 7.4.4` → CVE-2025-71176 (test-only, lower risk). Fix: `pytest==9.0.3`. -- No `requirements-dev.txt` / `requirements-test.txt` separation — `pytest` (test-only) is mixed with production dependencies, meaning it gets installed in production. -- No transitive dependency pinning (`pip-compile` or lock file). Transitive deps are resolved fresh on every `pip install`, so builds are not fully reproducible. -- `openpyxl==3.1.2` triggers `DeprecationWarning: datetime.datetime.utcnow()` (library-internal issue, tracked upstream). Fix: upgrade to `openpyxl==3.1.5`. - -### 4.7 Documentation — 7/10 - -**Strengths:** -- `README.md` is thorough: quick start, full deployment section (Render, gunicorn+systemd+nginx, Docker, Railway, Fly.io), usage guide, security notes, troubleshooting, environment variable table. -- `CLAUDE.md` provides excellent internal contributor guidance. -- All Python functions have docstrings. - -**Issues:** -- No `CONTRIBUTING.md` or `DEVELOPMENT.md` — new contributors must infer linting/testing conventions from `CLAUDE.md`. -- No `CHANGELOG.md` — no history of changes or versioning rationale. -- No `docs/` directory or architecture decision records. -- README does not mention linting, type checking, or coverage — the "Development" section only shows `python app.py` and `pytest`. -- README badge for "License" points to no `LICENSE` file (the file doesn't exist in the repo). -- Missing: JSON format edge-case examples (null values, booleans, Unicode, deeply nested, large files). -- `APP_VERSION = '1.1.0'` in `config.py` is not linked to any tag or release — version can drift silently. - -### 4.8 CI/CD Reliability — 0/10 - -**Critical gap.** There is no `.github/workflows/` directory. No CI pipeline exists. - -This means: -- No automated test runs on pull requests or pushes to `main`. -- Dependency vulnerabilities are never automatically flagged. -- Linting and formatting checks never run automatically. -- Regressions can be merged without detection. -- The `render.yaml` auto-deploys on every push — with no CI gate, broken code deploys directly to production. - -**Required:** A GitHub Actions workflow running on `push` and `pull_request` to `main` that: -1. Installs dependencies -2. Runs `pytest` with coverage -3. Runs `ruff check` and `ruff format --check` -4. Runs `pip-audit` - -### 4.9 Input Validation & Error Handling — 9/10 - -**Strengths:** -- CSRF protection on all POST routes via Flask-WTF. -- SSRF protection with DNS validation blocking private/loopback/link-local/multicast IPs. -- Rate limiting on all routes (configurable). -- `MAX_CONTENT_LENGTH` enforced (10MB default). -- API response size capped + streamed (no full-load-then-check). -- `allow_redirects=False` on API fetch. -- UTF-8 decoding error caught and returns 400. -- `UnicodeDecodeError`, `JSONDecodeError`, `ValueError`, `Timeout`, `RequestException` all handled with appropriate HTTP status codes. -- All `RequestException` detail is swallowed and a generic message returned (no internal host leakage). -- `escapeHtml()` used consistently in all JS rendering paths (no XSS risk from JSON data). - -**Issues:** -- No JSON parsing depth limit — a specially crafted JSON with 10,000 levels of nesting could exhaust the Python call stack before `flatten_for_csv`'s `max_depth` guard is reached (Python's default recursion limit is ~1000; `json.loads` itself is C-level and handles it, but `extract_table_data` is recursive Python with no depth guard). -- No file extension validation on upload (any file extension accepted; only content parsing catches non-JSON). -- No explicit `Content-Type` validation on file upload (`accept=".json,.jsonl"` is client-side only). -- The JS `formatValue()` function does not guard against extremely large arrays/objects in a cell — rendering a cell with 50,000-item nested array would freeze the browser tab. - -### 4.10 Usability & Developer Experience — 7/10 - -**Strengths:** -- Drag-and-drop file upload, dark/light theme with `localStorage` persistence, format toggle (JSON/JSONL), multi-format export dropdown. -- Keyboard-accessible tabs (they are `