From 66196607364527e38dae69a7c13266c51222458c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:22:20 +0000 Subject: [PATCH 01/36] Phase 0.1/0.2: upgrade dependency pins, split out dev dependencies Task 0.1: Flask 3.0.0 -> 3.1.3, requests 2.31.0 -> 2.33.0, gunicorn 21.2.0 -> 22.0.0 (CVE-2024-1135 request smuggling), openpyxl 3.1.2 -> 3.1.5. pip-audit -r requirements.txt now reports 0 vulnerabilities (was 7). Task 0.2: pytest moves to requirements-dev.txt together with ruff, coverage and pip-audit, so a production install no longer pulls test tooling. requirements-redis.txt carries the exact-pinned redis client that Flask-Limiter needs for a shared RATELIMIT_STORAGE_URI (roadmap 2.10d). It is a separate opt-in file rather than a line in requirements.txt so the default install stays free of a Redis dependency, as roadmap section 5 requires. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- requirements-dev.txt | 7 +++++++ requirements-redis.txt | 10 ++++++++++ requirements.txt | 9 ++++----- 3 files changed, 21 insertions(+), 5 deletions(-) create mode 100644 requirements-dev.txt create mode 100644 requirements-redis.txt diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 0000000..8c74e53 --- /dev/null +++ b/requirements-dev.txt @@ -0,0 +1,7 @@ +# Development / CI dependencies. Production installs requirements.txt only. +-r requirements.txt + +pytest==9.0.3 +ruff==0.16.4 +coverage==7.15.4 +pip-audit==2.10.1 diff --git a/requirements-redis.txt b/requirements-redis.txt new file mode 100644 index 0000000..0582d07 --- /dev/null +++ b/requirements-redis.txt @@ -0,0 +1,10 @@ +# Optional: the client Flask-Limiter needs for a shared RATELIMIT_STORAGE_URI. +# +# Install this ONLY when running more than one gunicorn worker or more than one +# replica, i.e. when RATELIMIT_STORAGE_URI is set to redis://... The default +# single-worker / single-instance deployment uses memory:// and does not need it. +# +# pip install -r requirements.txt -r requirements-redis.txt +-r requirements.txt + +redis==8.1.0 diff --git a/requirements.txt b/requirements.txt index 3dc4d49..c30a028 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,7 +1,6 @@ -Flask==3.0.0 -requests==2.31.0 -gunicorn==21.2.0 +Flask==3.1.3 +requests==2.33.0 +gunicorn==22.0.0 Flask-WTF==1.2.1 Flask-Limiter==3.5.0 -pytest==7.4.4 -openpyxl==3.1.2 +openpyxl==3.1.5 From 38646b7812cbef20629823ad7a446d5796fc14aa Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:22:20 +0000 Subject: [PATCH 02/36] Phase 0.3/0.4: add CI workflow and ruff/pytest configuration Task 0.3: .github/workflows/ci.yml installs requirements-dev.txt then runs ruff check, ruff format --check, pytest and pip-audit against the runtime requirements. render.yaml previously auto-deployed every push with no checks at all. Task 0.4: pyproject.toml pins ruff to py311 / line-length 100 and holds the pytest config. ruff is restricted to *.py so the review documents and README keep their illustrative snippets byte-identical. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- .github/workflows/ci.yml | 33 +++++++++++++++++++++++++++++++++ pyproject.toml | 26 ++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 .github/workflows/ci.yml create mode 100644 pyproject.toml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..7522091 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,33 @@ +name: CI + +on: + push: + branches: ["**"] + pull_request: + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: "3.11" + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements-dev.txt + + - name: Lint + run: ruff check . + + - name: Format check + run: ruff format --check . + + - name: Tests + run: python -m pytest tests/ -v + + - name: Audit runtime dependencies + run: pip-audit -r requirements.txt diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 0000000..6e9332e --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,26 @@ +[tool.ruff] +target-version = "py311" +line-length = 100 +exclude = [".venv", "venv", "__pycache__"] +# Only lint/format Python sources; prose files carry illustrative snippets that +# must stay byte-identical to what they document. +include = ["*.py", "*.pyi"] + +[tool.ruff.lint] +select = ["E", "F", "W", "I", "UP", "B", "C4", "SIM"] +ignore = [ + # Long URLs and prose in docstrings/comments are allowed; code lines are still + # held to line-length by the formatter. + "E501", +] + +[tool.ruff.lint.per-file-ignores] +"tests/*" = ["B011"] + +[tool.ruff.format] +quote-style = "single" + +[tool.pytest.ini_options] +testpaths = ["tests"] +python_files = ["test_*.py"] +addopts = "-ra" From 2dc846d9f8c461ec1d491ca44c834c57ce389e62 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:22:20 +0000 Subject: [PATCH 03/36] Phase 0.5: apply ruff lint fixes and formatting ruff check . --fix plus manual fixes for the rules it could not fix safely: unused loop variables in extract_table_data (B007), exception chaining in parse_jsonl (B904), a redundant list() inside sorted() (C414) and two if/else blocks that ruff wanted as ternaries (SIM108). No behavior change; the full suite still passes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- app.py | 2 + extensions.py | 2 +- helpers.py | 32 +++-- routes.py | 82 ++++++------- tests/conftest.py | 1 + tests/test_helpers.py | 102 ++++++++-------- tests/test_routes.py | 262 +++++++++++++++++++---------------------- tests/test_security.py | 2 + 8 files changed, 237 insertions(+), 248 deletions(-) diff --git a/app.py b/app.py index 2befda8..fb84064 100644 --- a/app.py +++ b/app.py @@ -1,6 +1,7 @@ """JSON Table Converter - Flask application factory.""" from flask import Flask + from config import Config from extensions import csrf, limiter from security import apply_security_headers @@ -20,6 +21,7 @@ def create_app(config_class=Config): # Register routes from routes import bp + app.register_blueprint(bp) return app diff --git a/extensions.py b/extensions.py index bd1e0fa..0da628a 100644 --- a/extensions.py +++ b/extensions.py @@ -1,8 +1,8 @@ """Flask extensions (initialized without app, bound later via init_app).""" -from flask_wtf.csrf import CSRFProtect from flask_limiter import Limiter from flask_limiter.util import get_remote_address +from flask_wtf.csrf import CSRFProtect csrf = CSRFProtect() limiter = Limiter(key_func=get_remote_address) diff --git a/helpers.py b/helpers.py index cf37459..c466f31 100644 --- a/helpers.py +++ b/helpers.py @@ -17,10 +17,12 @@ def flatten_for_csv(data, parent_key='', sep='.', _depth=0, max_depth=10): items = [] if isinstance(data, dict): for k, v in data.items(): - new_key = f"{parent_key}{sep}{k}" if parent_key else k + new_key = f'{parent_key}{sep}{k}' if parent_key else k if isinstance(v, dict): items.extend( - flatten_for_csv(v, new_key, sep=sep, _depth=_depth + 1, max_depth=max_depth).items() + flatten_for_csv( + v, new_key, sep=sep, _depth=_depth + 1, max_depth=max_depth + ).items() ) elif isinstance(v, list): items.append((new_key, json.dumps(v))) @@ -41,17 +43,17 @@ def extract_table_data(json_data): if len(json_data) > 0 and isinstance(json_data[0], dict): return json_data else: - return [{"value": item} for item in json_data] + return [{'value': item} for item in json_data] if isinstance(json_data, dict): - for key, value in json_data.items(): + for value in json_data.values(): if isinstance(value, list) and len(value) > 0: if isinstance(value[0], dict): return value else: - return [{"value": item} for item in value] + return [{'value': item} for item in value] - for key, value in json_data.items(): + for value in json_data.values(): if isinstance(value, dict): result = extract_table_data(value) if result: @@ -75,7 +77,7 @@ def parse_jsonl(text): try: rows.append(json.loads(line)) except json.JSONDecodeError as e: - raise ValueError(f'Invalid JSON on line {i}: {str(e)}') + raise ValueError(f'Invalid JSON on line {i}: {e}') from e return rows @@ -90,21 +92,15 @@ def find_candidate_arrays(json_data, prefix='', candidates=None): if isinstance(json_data, list): if len(json_data) > 0 and isinstance(json_data[0], dict): sample_keys = sorted(json_data[0].keys())[:5] - candidates.append({ - 'path': prefix or '(root)', - 'length': len(json_data), - 'sample_keys': sample_keys - }) + candidates.append( + {'path': prefix or '(root)', 'length': len(json_data), 'sample_keys': sample_keys} + ) elif isinstance(json_data, dict): for key, value in json_data.items(): path = f'{prefix}.{key}' if prefix else key if isinstance(value, list) and len(value) > 0 and isinstance(value[0], dict): sample_keys = sorted(value[0].keys())[:5] - candidates.append({ - 'path': path, - 'length': len(value), - 'sample_keys': sample_keys - }) + candidates.append({'path': path, 'length': len(value), 'sample_keys': sample_keys}) elif isinstance(value, dict): find_candidate_arrays(value, path, candidates) @@ -144,4 +140,4 @@ def get_all_columns(data): for row in data: if isinstance(row, dict): columns.update(row.keys()) - return sorted(list(columns)) + return sorted(columns) diff --git a/routes.py b/routes.py index 65e685c..c68aff3 100644 --- a/routes.py +++ b/routes.py @@ -1,19 +1,23 @@ """Flask route handlers.""" -import json import csv import io +import json import logging + import requests +from flask import Blueprint, Response, current_app, jsonify, render_template, request from requests.auth import HTTPBasicAuth -from flask import Blueprint, render_template, request, jsonify, Response, current_app from extensions import limiter -from security import validate_url from helpers import ( - flatten_for_csv, extract_table_data, get_all_columns, parse_jsonl, - extract_by_path + extract_by_path, + extract_table_data, + flatten_for_csv, + get_all_columns, + parse_jsonl, ) +from security import validate_url logger = logging.getLogger(__name__) @@ -29,10 +33,7 @@ def index(): @bp.route('/health') def health(): """Health check endpoint.""" - return jsonify({ - 'status': 'ok', - 'version': current_app.config['APP_VERSION'] - }) + return jsonify({'status': 'ok', 'version': current_app.config['APP_VERSION']}) @bp.route('/process', methods=['POST']) @@ -53,10 +54,7 @@ def process_json(): return jsonify({'error': 'No file selected'}), 400 try: content = file.read().decode('utf-8') - if data_format == 'jsonl': - json_data = parse_jsonl(content) - else: - json_data = json.loads(content) + json_data = parse_jsonl(content) if data_format == 'jsonl' else json.loads(content) except UnicodeDecodeError: return jsonify({'error': 'File must be UTF-8 encoded'}), 400 except (json.JSONDecodeError, ValueError) as e: @@ -123,7 +121,7 @@ def process_json(): params=params, timeout=timeout, stream=True, - allow_redirects=False + allow_redirects=False, ) resp.raise_for_status() @@ -131,16 +129,15 @@ def process_json(): for chunk in resp.iter_content(chunk_size=8192): content.extend(chunk) if len(content) > max_size: - return jsonify({ - 'error': f'API response exceeds maximum size ' - f'({max_size // (1024 * 1024)}MB)' - }), 400 + return jsonify( + { + 'error': f'API response exceeds maximum size ' + f'({max_size // (1024 * 1024)}MB)' + } + ), 400 text = bytes(content).decode('utf-8') - if data_format == 'jsonl': - json_data = parse_jsonl(text) - else: - json_data = json.loads(text) + json_data = parse_jsonl(text) if data_format == 'jsonl' else json.loads(text) except requests.exceptions.Timeout: return jsonify({'error': 'API request timed out'}), 400 @@ -164,15 +161,12 @@ def process_json(): elif isinstance(selected, dict): table_data = [selected] else: - return jsonify({ - 'error': f'Path "{json_path}" is a primitive value; pick an object or array' - }), 400 + return jsonify( + {'error': f'Path "{json_path}" is a primitive value; pick an object or array'} + ), 400 else: # No path chosen yet — let the client render a tree picker - return jsonify({ - 'needs_selection': True, - 'raw_json': json_data - }) + return jsonify({'needs_selection': True, 'raw_json': json_data}) if not table_data: return jsonify({'error': 'Could not extract tabular data from JSON'}), 400 @@ -185,16 +179,18 @@ def process_json(): csv_data = [flatten_for_csv(row, max_depth=max_depth) for row in table_data] csv_columns = get_all_columns(csv_data) - return jsonify({ - 'success': True, - 'columns': columns, - 'preview': preview_data, - 'total_rows': len(table_data), - 'csv_data': csv_data, - 'csv_columns': csv_columns - }) + return jsonify( + { + 'success': True, + 'columns': columns, + 'preview': preview_data, + 'total_rows': len(table_data), + 'csv_data': csv_data, + 'csv_columns': csv_columns, + } + ) - except Exception as e: + except Exception: logger.exception('Unexpected error in process_json') return jsonify({'error': 'An internal error occurred'}), 500 @@ -233,11 +229,11 @@ def export_csv(): mimetype='text/csv', headers={ 'Content-Disposition': 'attachment; filename=exported_data.csv', - 'Content-Type': 'text/csv; charset=utf-8' - } + 'Content-Type': 'text/csv; charset=utf-8', + }, ) - except Exception as e: + except Exception: logger.exception('Unexpected error in export_csv') return jsonify({'error': 'Export failed'}), 500 @@ -285,9 +281,9 @@ def export_xlsx(): mimetype='application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', headers={ 'Content-Disposition': 'attachment; filename=exported_data.xlsx', - } + }, ) - except Exception as e: + except Exception: logger.exception('Unexpected error in export_xlsx') return jsonify({'error': 'Export failed'}), 500 diff --git a/tests/conftest.py b/tests/conftest.py index bad89c1..941b4cc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,7 @@ """Shared pytest fixtures.""" import pytest + from app import create_app diff --git a/tests/test_helpers.py b/tests/test_helpers.py index bd9e2b3..cec96bd 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -1,84 +1,89 @@ """Tests for data processing helpers.""" import json + from helpers import ( - flatten_for_csv, extract_table_data, get_all_columns, parse_jsonl, - find_candidate_arrays, extract_by_path + extract_by_path, + extract_table_data, + find_candidate_arrays, + flatten_for_csv, + get_all_columns, + parse_jsonl, ) class TestFlattenForCsv: def test_flat_dict(self): - result = flatten_for_csv({"a": 1, "b": "hello"}) - assert result == {"a": 1, "b": "hello"} + result = flatten_for_csv({'a': 1, 'b': 'hello'}) + assert result == {'a': 1, 'b': 'hello'} def test_nested_dict(self): - result = flatten_for_csv({"a": {"b": 1, "c": 2}}) - assert result == {"a.b": 1, "a.c": 2} + result = flatten_for_csv({'a': {'b': 1, 'c': 2}}) + assert result == {'a.b': 1, 'a.c': 2} def test_deeply_nested(self): - result = flatten_for_csv({"a": {"b": {"c": {"d": 42}}}}) - assert result == {"a.b.c.d": 42} + result = flatten_for_csv({'a': {'b': {'c': {'d': 42}}}}) + assert result == {'a.b.c.d': 42} def test_list_becomes_json_string(self): - result = flatten_for_csv({"tags": [1, 2, 3]}) - assert result == {"tags": "[1, 2, 3]"} + result = flatten_for_csv({'tags': [1, 2, 3]}) + assert result == {'tags': '[1, 2, 3]'} def test_empty_dict(self): result = flatten_for_csv({}) assert result == {} def test_max_depth_stops_recursion(self): - deep = {"a": {"b": {"c": {"d": "value"}}}} + deep = {'a': {'b': {'c': {'d': 'value'}}}} result = flatten_for_csv(deep, max_depth=2) # At depth 2, the remaining dict should be JSON-serialized - assert "a.b" in result - assert isinstance(result["a.b"], str) - parsed = json.loads(result["a.b"]) - assert parsed == {"c": {"d": "value"}} + assert 'a.b' in result + assert isinstance(result['a.b'], str) + parsed = json.loads(result['a.b']) + assert parsed == {'c': {'d': 'value'}} def test_primitive_value(self): - result = flatten_for_csv("hello", parent_key="key") - assert result == {"key": "hello"} + result = flatten_for_csv('hello', parent_key='key') + assert result == {'key': 'hello'} def test_mixed_types(self): - data = {"name": "Alice", "meta": {"age": 30}, "scores": [90, 85]} + data = {'name': 'Alice', 'meta': {'age': 30}, 'scores': [90, 85]} result = flatten_for_csv(data) - assert result["name"] == "Alice" - assert result["meta.age"] == 30 - assert result["scores"] == "[90, 85]" + assert result['name'] == 'Alice' + assert result['meta.age'] == 30 + assert result['scores'] == '[90, 85]' class TestExtractTableData: def test_array_of_objects(self): - data = [{"id": 1}, {"id": 2}] + data = [{'id': 1}, {'id': 2}] assert extract_table_data(data) == data def test_array_of_primitives(self): result = extract_table_data([1, 2, 3]) - assert result == [{"value": 1}, {"value": 2}, {"value": 3}] + assert result == [{'value': 1}, {'value': 2}, {'value': 3}] def test_dict_with_array_property(self): - data = {"results": [{"id": 1}, {"id": 2}]} - assert extract_table_data(data) == [{"id": 1}, {"id": 2}] + data = {'results': [{'id': 1}, {'id': 2}]} + assert extract_table_data(data) == [{'id': 1}, {'id': 2}] def test_dict_with_primitive_array(self): - data = {"items": ["a", "b"]} - assert extract_table_data(data) == [{"value": "a"}, {"value": "b"}] + data = {'items': ['a', 'b']} + assert extract_table_data(data) == [{'value': 'a'}, {'value': 'b'}] def test_nested_dict_with_array(self): - data = {"data": {"users": [{"name": "Alice"}]}} - assert extract_table_data(data) == [{"name": "Alice"}] + data = {'data': {'users': [{'name': 'Alice'}]}} + assert extract_table_data(data) == [{'name': 'Alice'}] def test_single_object(self): - data = {"key": "value", "num": 42} + data = {'key': 'value', 'num': 42} assert extract_table_data(data) == [data] def test_empty_list(self): - assert extract_table_data([]) == [{"value": item} for item in []] + assert extract_table_data([]) == [{'value': item} for item in []] def test_non_dict_non_list(self): - assert extract_table_data("hello") == [] + assert extract_table_data('hello') == [] class TestParseJsonl: @@ -86,7 +91,7 @@ def test_basic_jsonl(self): text = '{"id": 1}\n{"id": 2}\n{"id": 3}' result = parse_jsonl(text) assert len(result) == 3 - assert result[0] == {"id": 1} + assert result[0] == {'id': 1} def test_empty_lines_skipped(self): text = '{"a": 1}\n\n{"a": 2}\n' @@ -98,6 +103,7 @@ def test_empty_input(self): def test_invalid_line_raises(self): import pytest + with pytest.raises(ValueError, match='line 2'): parse_jsonl('{"a": 1}\n{bad json}\n{"a": 3}') @@ -109,14 +115,14 @@ def test_mixed_objects(self): class TestFindCandidateArrays: def test_single_array_at_root(self): - data = [{"id": 1}, {"id": 2}] + data = [{'id': 1}, {'id': 2}] candidates = find_candidate_arrays(data) assert len(candidates) == 1 assert candidates[0]['path'] == '(root)' assert candidates[0]['length'] == 2 def test_multiple_arrays(self): - data = {"users": [{"name": "A"}], "orders": [{"id": 1}, {"id": 2}]} + data = {'users': [{'name': 'A'}], 'orders': [{'id': 1}, {'id': 2}]} candidates = find_candidate_arrays(data) assert len(candidates) == 2 paths = [c['path'] for c in candidates] @@ -124,48 +130,48 @@ def test_multiple_arrays(self): assert 'orders' in paths def test_nested_array(self): - data = {"data": {"items": [{"x": 1}]}} + data = {'data': {'items': [{'x': 1}]}} candidates = find_candidate_arrays(data) assert len(candidates) == 1 assert candidates[0]['path'] == 'data.items' def test_no_arrays(self): - data = {"a": 1, "b": "text"} + data = {'a': 1, 'b': 'text'} assert find_candidate_arrays(data) == [] class TestExtractByPath: def test_root_path(self): - data = [{"a": 1}] + data = [{'a': 1}] assert extract_by_path(data, '(root)') == data def test_nested_path(self): - data = {"data": {"items": [1, 2, 3]}} + data = {'data': {'items': [1, 2, 3]}} assert extract_by_path(data, 'data.items') == [1, 2, 3] def test_invalid_path(self): - data = {"a": 1} + data = {'a': 1} assert extract_by_path(data, 'b.c') is None def test_array_index_in_path(self): - data = {"data": [{"orders": [{"x": 1}, {"x": 2}]}, {"orders": []}]} - assert extract_by_path(data, 'data.0.orders') == [{"x": 1}, {"x": 2}] + data = {'data': [{'orders': [{'x': 1}, {'x': 2}]}, {'orders': []}]} + assert extract_by_path(data, 'data.0.orders') == [{'x': 1}, {'x': 2}] def test_array_index_out_of_range(self): - assert extract_by_path({"a": [1, 2]}, 'a.5') is None + assert extract_by_path({'a': [1, 2]}, 'a.5') is None def test_non_numeric_index_on_array(self): - assert extract_by_path({"a": [1, 2]}, 'a.foo') is None + assert extract_by_path({'a': [1, 2]}, 'a.foo') is None class TestGetAllColumns: def test_basic(self): - data = [{"a": 1, "b": 2}, {"b": 3, "c": 4}] - assert get_all_columns(data) == ["a", "b", "c"] + data = [{'a': 1, 'b': 2}, {'b': 3, 'c': 4}] + assert get_all_columns(data) == ['a', 'b', 'c'] def test_empty(self): assert get_all_columns([]) == [] def test_non_dict_rows_ignored(self): - data = [{"a": 1}, "not a dict", {"b": 2}] - assert get_all_columns(data) == ["a", "b"] + data = [{'a': 1}, 'not a dict', {'b': 2}] + assert get_all_columns(data) == ['a', 'b'] diff --git a/tests/test_routes.py b/tests/test_routes.py index 5b22ae6..ef6e150 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -1,7 +1,7 @@ """Tests for Flask routes.""" import json -from unittest.mock import patch, MagicMock +from unittest.mock import MagicMock, patch class TestIndexRoute: @@ -29,11 +29,14 @@ def test_returns_version(self, client, app): class TestProcessRoute: def test_paste_valid_json(self, client): - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': '[{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]', - 'json_path': '(root)' - }) + response = client.post( + '/process', + data={ + 'input_method': 'paste', + 'pasted_json': '[{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]', + 'json_path': '(root)', + }, + ) data = json.loads(response.data) assert data['success'] is True assert data['total_rows'] == 2 @@ -41,54 +44,46 @@ def test_paste_valid_json(self, client): assert 'name' in data['columns'] def test_paste_invalid_json(self, client): - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': '{invalid json}' - }) + response = client.post( + '/process', data={'input_method': 'paste', 'pasted_json': '{invalid json}'} + ) assert response.status_code == 400 data = json.loads(response.data) assert 'error' in data def test_paste_empty(self, client): - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': '' - }) + response = client.post('/process', data={'input_method': 'paste', 'pasted_json': ''}) assert response.status_code == 400 def test_invalid_input_method(self, client): - response = client.post('/process', data={ - 'input_method': 'unknown' - }) + response = client.post('/process', data={'input_method': 'unknown'}) assert response.status_code == 400 def test_nested_json_object(self, client): - nested = json.dumps({"data": [{"x": 1}, {"x": 2}]}) - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': nested, - 'json_path': 'data' - }) + nested = json.dumps({'data': [{'x': 1}, {'x': 2}]}) + response = client.post( + '/process', data={'input_method': 'paste', 'pasted_json': nested, 'json_path': 'data'} + ) data = json.loads(response.data) assert data['success'] is True assert data['total_rows'] == 2 def test_no_path_returns_tree_payload(self, client): - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': '[{"id": 1}, {"id": 2}]' - }) + response = client.post( + '/process', data={'input_method': 'paste', 'pasted_json': '[{"id": 1}, {"id": 2}]'} + ) data = json.loads(response.data) assert data.get('needs_selection') is True - assert data['raw_json'] == [{"id": 1}, {"id": 2}] + assert data['raw_json'] == [{'id': 1}, {'id': 2}] def test_file_upload(self, client): import io - json_content = json.dumps([{"a": 1}]) + + json_content = json.dumps([{'a': 1}]) data = { 'input_method': 'file', 'json_path': '(root)', - 'json_file': (io.BytesIO(json_content.encode()), 'test.json') + 'json_file': (io.BytesIO(json_content.encode()), 'test.json'), } response = client.post('/process', data=data, content_type='multipart/form-data') result = json.loads(response.data) @@ -96,24 +91,28 @@ def test_file_upload(self, client): def test_jsonl_paste(self, client): jsonl_content = '{"id": 1, "name": "Alice"}\n{"id": 2, "name": "Bob"}' - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': jsonl_content, - 'data_format': 'jsonl', - 'json_path': '(root)' - }) + response = client.post( + '/process', + data={ + 'input_method': 'paste', + 'pasted_json': jsonl_content, + 'data_format': 'jsonl', + 'json_path': '(root)', + }, + ) data = json.loads(response.data) assert data['success'] is True assert data['total_rows'] == 2 def test_jsonl_file_upload(self, client): import io + jsonl_content = '{"a": 1}\n{"a": 2}\n{"a": 3}' data = { 'input_method': 'file', 'data_format': 'jsonl', 'json_path': '(root)', - 'json_file': (io.BytesIO(jsonl_content.encode()), 'test.jsonl') + 'json_file': (io.BytesIO(jsonl_content.encode()), 'test.jsonl'), } response = client.post('/process', data=data, content_type='multipart/form-data') result = json.loads(response.data) @@ -122,12 +121,10 @@ def test_jsonl_file_upload(self, client): def test_preview_limit(self, client, app): app.config['PREVIEW_ROW_LIMIT'] = 5 - rows = json.dumps([{"id": i} for i in range(20)]) - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': rows, - 'json_path': '(root)' - }) + rows = json.dumps([{'id': i} for i in range(20)]) + response = client.post( + '/process', data={'input_method': 'paste', 'pasted_json': rows, 'json_path': '(root)'} + ) data = json.loads(response.data) assert data['total_rows'] == 20 assert len(data['preview']) == 5 @@ -135,12 +132,12 @@ def test_preview_limit(self, client, app): class TestExportCsvRoute: def test_export_valid_data(self, client): - response = client.post('/export-csv', - data=json.dumps({ - 'csv_data': [{'a': 1, 'b': 2}, {'a': 3, 'b': 4}], - 'csv_columns': ['a', 'b'] - }), - content_type='application/json' + response = client.post( + '/export-csv', + data=json.dumps( + {'csv_data': [{'a': 1, 'b': 2}, {'a': 3, 'b': 4}], 'csv_columns': ['a', 'b']} + ), + content_type='application/json', ) assert response.status_code == 200 assert response.content_type.startswith('text/csv') @@ -148,99 +145,86 @@ def test_export_valid_data(self, client): assert 'a,b' in csv_text def test_export_empty_data(self, client): - response = client.post('/export-csv', - data=json.dumps({ - 'csv_data': [], - 'csv_columns': [] - }), - content_type='application/json' + response = client.post( + '/export-csv', + data=json.dumps({'csv_data': [], 'csv_columns': []}), + content_type='application/json', ) assert response.status_code == 400 class TestExportXlsxRoute: def test_export_xlsx(self, client): - response = client.post('/export-xlsx', - data=json.dumps({ - 'csv_data': [{'a': 1, 'b': 2}], - 'csv_columns': ['a', 'b'] - }), - content_type='application/json' + response = client.post( + '/export-xlsx', + data=json.dumps({'csv_data': [{'a': 1, 'b': 2}], 'csv_columns': ['a', 'b']}), + content_type='application/json', ) assert response.status_code == 200 assert 'spreadsheetml' in response.content_type def test_export_xlsx_empty(self, client): - response = client.post('/export-xlsx', + response = client.post( + '/export-xlsx', data=json.dumps({'csv_data': [], 'csv_columns': []}), - content_type='application/json' + content_type='application/json', ) assert response.status_code == 400 class TestPathSelection: def test_no_path_returns_raw_json_for_tree(self, client): - payload = {"users": [{"n": "A"}], "orders": [{"id": 1}]} - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': json.dumps(payload) - }) + payload = {'users': [{'n': 'A'}], 'orders': [{'id': 1}]} + response = client.post( + '/process', data={'input_method': 'paste', 'pasted_json': json.dumps(payload)} + ) data = json.loads(response.data) assert data.get('needs_selection') is True assert data['raw_json'] == payload def test_path_selection(self, client): - multi = json.dumps({"users": [{"n": "A"}], "orders": [{"id": 1}, {"id": 2}]}) - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': multi, - 'json_path': 'orders' - }) + multi = json.dumps({'users': [{'n': 'A'}], 'orders': [{'id': 1}, {'id': 2}]}) + response = client.post( + '/process', data={'input_method': 'paste', 'pasted_json': multi, 'json_path': 'orders'} + ) data = json.loads(response.data) assert data['success'] is True assert data['total_rows'] == 2 def test_path_to_array_index_object(self, client): - payload = json.dumps({"data": [{"id": 1, "orders": [{"x": 1}, {"x": 2}]}]}) - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': payload, - 'json_path': 'data.0.orders' - }) + payload = json.dumps({'data': [{'id': 1, 'orders': [{'x': 1}, {'x': 2}]}]}) + response = client.post( + '/process', + data={'input_method': 'paste', 'pasted_json': payload, 'json_path': 'data.0.orders'}, + ) data = json.loads(response.data) assert data['success'] is True assert data['total_rows'] == 2 def test_path_to_single_object_becomes_one_row(self, client): - payload = json.dumps({"meta": {"version": 3, "name": "x"}}) - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': payload, - 'json_path': 'meta' - }) + payload = json.dumps({'meta': {'version': 3, 'name': 'x'}}) + response = client.post( + '/process', data={'input_method': 'paste', 'pasted_json': payload, 'json_path': 'meta'} + ) data = json.loads(response.data) assert data['success'] is True assert data['total_rows'] == 1 assert 'version' in data['columns'] def test_path_to_primitive_rejected(self, client): - payload = json.dumps({"a": 1}) - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': payload, - 'json_path': 'a' - }) + payload = json.dumps({'a': 1}) + response = client.post( + '/process', data={'input_method': 'paste', 'pasted_json': payload, 'json_path': 'a'} + ) assert response.status_code == 400 data = json.loads(response.data) assert 'primitive' in data['error'] def test_invalid_path_rejected(self, client): - payload = json.dumps({"a": {"b": 1}}) - response = client.post('/process', data={ - 'input_method': 'paste', - 'pasted_json': payload, - 'json_path': 'a.c' - }) + payload = json.dumps({'a': {'b': 1}}) + response = client.post( + '/process', data={'input_method': 'paste', 'pasted_json': payload, 'json_path': 'a.c'} + ) assert response.status_code == 400 data = json.loads(response.data) assert 'not found' in data['error'] @@ -261,11 +245,14 @@ def test_api_fetch_success(self, mock_validate, mock_get, client): mock_validate.return_value = (True, None) mock_get.return_value = self._mock_response([{'id': 1}, {'id': 2}]) - response = client.post('/process', data={ - 'input_method': 'api', - 'api_url': 'https://api.example.com/data', - 'json_path': '(root)' - }) + response = client.post( + '/process', + data={ + 'input_method': 'api', + 'api_url': 'https://api.example.com/data', + 'json_path': '(root)', + }, + ) data = json.loads(response.data) assert data['success'] is True assert data['total_rows'] == 2 @@ -277,13 +264,13 @@ def test_api_fetch_success(self, mock_validate, mock_get, client): @patch('routes.validate_url') def test_api_fetch_timeout(self, mock_validate, mock_get, client): import requests as req + mock_validate.return_value = (True, None) mock_get.side_effect = req.exceptions.Timeout('timed out') - response = client.post('/process', data={ - 'input_method': 'api', - 'api_url': 'https://api.example.com/data' - }) + response = client.post( + '/process', data={'input_method': 'api', 'api_url': 'https://api.example.com/data'} + ) assert response.status_code == 400 data = json.loads(response.data) assert 'timed out' in data['error'].lower() @@ -298,10 +285,9 @@ def test_api_fetch_non_json(self, mock_validate, mock_get, client): mock_resp.raise_for_status.return_value = None mock_get.return_value = mock_resp - response = client.post('/process', data={ - 'input_method': 'api', - 'api_url': 'https://api.example.com/data' - }) + response = client.post( + '/process', data={'input_method': 'api', 'api_url': 'https://api.example.com/data'} + ) assert response.status_code == 400 data = json.loads(response.data) assert 'not valid JSON' in data['error'] @@ -317,22 +303,23 @@ def test_api_fetch_max_size_exceeded(self, mock_validate, mock_get, client, app) mock_resp.raise_for_status.return_value = None mock_get.return_value = mock_resp - response = client.post('/process', data={ - 'input_method': 'api', - 'api_url': 'https://api.example.com/data' - }) + response = client.post( + '/process', data={'input_method': 'api', 'api_url': 'https://api.example.com/data'} + ) assert response.status_code == 400 data = json.loads(response.data) assert 'exceeds maximum size' in data['error'] @patch('routes.validate_url') def test_api_fetch_ssrf_blocked(self, mock_validate, client): - mock_validate.return_value = (False, 'URLs pointing to private or internal networks are not allowed') + mock_validate.return_value = ( + False, + 'URLs pointing to private or internal networks are not allowed', + ) - response = client.post('/process', data={ - 'input_method': 'api', - 'api_url': 'http://169.254.169.254/metadata' - }) + response = client.post( + '/process', data={'input_method': 'api', 'api_url': 'http://169.254.169.254/metadata'} + ) assert response.status_code == 400 data = json.loads(response.data) assert 'private' in data['error'].lower() @@ -348,12 +335,15 @@ def test_api_fetch_jsonl(self, mock_validate, mock_get, client): mock_resp.raise_for_status.return_value = None mock_get.return_value = mock_resp - response = client.post('/process', data={ - 'input_method': 'api', - 'api_url': 'https://api.example.com/data', - 'data_format': 'jsonl', - 'json_path': '(root)' - }) + response = client.post( + '/process', + data={ + 'input_method': 'api', + 'api_url': 'https://api.example.com/data', + 'data_format': 'jsonl', + 'json_path': '(root)', + }, + ) data = json.loads(response.data) assert data['success'] is True assert data['total_rows'] == 2 @@ -362,15 +352,15 @@ def test_api_fetch_jsonl(self, mock_validate, mock_get, client): @patch('routes.validate_url') def test_api_fetch_request_error_no_leak(self, mock_validate, mock_get, client): import requests as req + mock_validate.return_value = (True, None) mock_get.side_effect = req.exceptions.ConnectionError( 'Connection to secret-internal-host:8080 refused' ) - response = client.post('/process', data={ - 'input_method': 'api', - 'api_url': 'https://api.example.com/data' - }) + response = client.post( + '/process', data={'input_method': 'api', 'api_url': 'https://api.example.com/data'} + ) assert response.status_code == 400 data = json.loads(response.data) # Should NOT leak the internal connection details @@ -381,12 +371,10 @@ def test_api_fetch_request_error_no_leak(self, mock_validate, mock_get, client): class TestFileUploadEncoding: def test_non_utf8_file_returns_400(self, client): import io + # Latin-1 encoded content with bytes invalid in UTF-8 content = b'\xff\xfe This is not valid UTF-8' - data = { - 'input_method': 'file', - 'json_file': (io.BytesIO(content), 'test.json') - } + data = {'input_method': 'file', 'json_file': (io.BytesIO(content), 'test.json')} response = client.post('/process', data=data, content_type='multipart/form-data') assert response.status_code == 400 result = json.loads(response.data) @@ -395,15 +383,13 @@ def test_non_utf8_file_returns_400(self, client): class TestExportEdgeCases: def test_export_csv_no_json_body(self, client): - response = client.post('/export-csv', data='not json', - content_type='application/json') + response = client.post('/export-csv', data='not json', content_type='application/json') assert response.status_code == 400 data = json.loads(response.data) assert 'Invalid or missing' in data['error'] def test_export_xlsx_no_json_body(self, client): - response = client.post('/export-xlsx', data='not json', - content_type='application/json') + response = client.post('/export-xlsx', data='not json', content_type='application/json') assert response.status_code == 400 data = json.loads(response.data) assert 'Invalid or missing' in data['error'] diff --git a/tests/test_security.py b/tests/test_security.py index 2cb4909..4040178 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -1,6 +1,7 @@ """Tests for security utilities.""" from unittest.mock import patch + from security import validate_url @@ -72,6 +73,7 @@ def test_blocks_multicast(self): def test_unresolvable_hostname(self): import socket + with patch('security.socket.getaddrinfo') as mock_dns: mock_dns.side_effect = socket.gaierror('Name not found') is_valid, error = validate_url('http://nonexistent.invalid') From 3fc5730dc4ee57087bfb835eb053a1260a7b6b9a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:22:21 +0000 Subject: [PATCH 04/36] Phase 0.6: correct the license references to GPL-3.0 The LICENSE file is GPL-3.0 but the README badge and license section still claimed MIT (F17). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 3a3b1ac..7db4d14 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A lightweight web tool to convert JSON data into viewable tables with CSV export ![Python](https://img.shields.io/badge/Python-3.11+-blue) ![Flask](https://img.shields.io/badge/Flask-3.0-green) -![License](https://img.shields.io/badge/License-MIT-yellow) +![License](https://img.shields.io/badge/License-GPL--3.0-yellow) ## Features @@ -433,7 +433,7 @@ python -m pytest tests/ -v ## License -MIT License - Feel free to modify and use internally. +GNU General Public License v3.0 — see [`LICENSE`](LICENSE) for the full text. --- From 9070be2149b35244c8650ee7bfa958f95307e1d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:22:21 +0000 Subject: [PATCH 05/36] Phase 0.7: gate Render auto-deploy on passing CI checks Replaces autoDeploy: true with autoDeployTrigger: checksPass so a push with failing or missing checks does not reach production. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- render.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/render.yaml b/render.yaml index 3d06f07..3c15117 100644 --- a/render.yaml +++ b/render.yaml @@ -15,5 +15,6 @@ services: # Free tier settings plan: free # No persistent disk = no data storage - # Auto-deploy on push - autoDeploy: true + # Auto-deploy on push, but only once CI checks pass. Render blocks the + # deploy when the commit has failing checks or no checks at all. + autoDeployTrigger: checksPass From 4e37551f78ed99dc6aa231c9660e4def3984e91d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:22:31 +0000 Subject: [PATCH 06/36] Phase 0.8: remove dead find_candidate_arrays and sync the stale docs D2/P10/F8: the function and its four tests served the old candidates handshake that the JSON tree picker replaced; routes.py has not imported it since. Deleting it in Phase 0 means the Phase 1 recursion guard only has to cover extract_table_data. MEMORY.md, CLAUDE.md and AGENTS.md still described the /process response as {needs_selection, candidates}; they now describe the actual {needs_selection, raw_json} tree-picker handshake (F17). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- AGENTS.md | 3 +-- CLAUDE.md | 9 ++++----- MEMORY.md | 6 +++--- helpers.py | 26 -------------------------- tests/test_helpers.py | 28 ---------------------------- 5 files changed, 8 insertions(+), 64 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e7c4914..107cfcc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,13 +66,12 @@ python -m pytest tests/ -v - `flatten_for_csv(data, parent_key='', sep='.', _depth=0, max_depth=10)` — depth-capped recursion; deep nesting is JSON-stringified instead of stack-overflowing. - `extract_table_data(json_data)` — heuristic: list-of-dicts → use directly; dict with array value → that array; nested dicts → recurse; otherwise single-row. - `parse_jsonl(text)` — line-by-line JSON; raises `ValueError` with the offending line number. - - `find_candidate_arrays(json_data, prefix='', candidates=None)` — discovers every array-of-objects with `{path, length, sample_keys}` metadata so the frontend can prompt the user. - `extract_by_path(json_data, path)` — dot-notation navigation; `'(root)'` is the sentinel for top-level lists. - `get_all_columns(data)` — sorted union of keys. - **`routes.py`** — Blueprint `bp`. Routes: - `GET /` → `templates/index.html`. - `GET /health` → `{"status": "ok", "version": APP_VERSION}`. - - `POST /process` → rate-limited (default `RATE_LIMIT_PROCESS=30/min`). Reads `input_method` (`file`/`paste`/`api`), `data_format` (`json`/`jsonl`), optional `json_path`. Returns `{success, columns, preview, total_rows, csv_data, csv_columns}` **or** `{needs_selection: true, candidates: [...]}` when multiple arrays found and no `json_path` selected. + - `POST /process` → rate-limited (default `RATE_LIMIT_PROCESS=30/min`). Reads `input_method` (`file`/`paste`/`api`), `data_format` (`json`/`jsonl`), optional `json_path`. Returns `{success, columns, preview, total_rows, csv_data, csv_columns}` **or** `{needs_selection: true, raw_json: ...}` when no `json_path` was selected, so the frontend can render the JSON tree picker. - `POST /export-csv` → rate-limited (default `RATE_LIMIT_EXPORT=60/min`). Server-side CSV fallback. - `POST /export-xlsx` → rate-limited. Server-side Excel via openpyxl. diff --git a/CLAUDE.md b/CLAUDE.md index e38ed2a..f861247 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -87,13 +87,12 @@ python app.py - `extract_table_data(json_data)` — Extracts tabular rows from various JSON shapes (top-level array, dict containing an array of objects, nested dicts, or a single object). - `get_all_columns(data)` — Returns sorted unique column names across rows. - `parse_jsonl(text)` — Parses JSON Lines (one JSON value per non-empty line), raising `ValueError` with line numbers on errors. -- `find_candidate_arrays(json_data)` — Discovers arrays of objects with their `path`, `length`, and first 5 `sample_keys` (used for the multi-array selector modal). - `extract_by_path(json_data, path)` — Navigates JSON by dot-notation path (`(root)` returns the document itself). **`routes.py`** — Flask Blueprint (`bp`): - `GET /` — Serves `index.html`. - `GET /health` — Returns `{"status": "ok", "version": APP_VERSION}` for monitoring. -- `POST /process` — Parses JSON/JSONL from file/paste/API, returns preview + full CSV-ready data. Returns `{"needs_selection": true, "candidates": [...]}` if multiple arrays found and no `json_path` was provided. Rate-limited via `RATE_LIMIT_PROCESS`. +- `POST /process` — Parses JSON/JSONL from file/paste/API, returns preview + full CSV-ready data. Returns `{"needs_selection": true, "raw_json": ...}` when no `json_path` was provided, so the client can render the JSON tree picker. Rate-limited via `RATE_LIMIT_PROCESS`. - `POST /export-csv` — Server-side CSV generation (fallback). Rate-limited via `RATE_LIMIT_EXPORT`. - `POST /export-xlsx` — Server-side Excel generation via openpyxl. Rate-limited via `RATE_LIMIT_EXPORT`. @@ -110,7 +109,7 @@ API-fetch specifics: `requests.get` is called with `stream=True`, `allow_redirec - Client-side CSV/TSV export (no server round-trip needed) - Server-side Excel export via `/export-xlsx` - Theme detection (`prefers-color-scheme`) with localStorage override -- Path selector modal when multiple candidate arrays are detected +- JSON tree picker modal for choosing which node becomes the table **`templates/index.html`** — HTML structure only. References external CSS/JS via `url_for('static', ...)`. Includes CSRF meta tag, theme toggle button, format selector, export dropdown, and path selector modal. No inline scripts or styles (CSP enforced). @@ -118,7 +117,7 @@ API-fetch specifics: `requests.get` is called with `stream=True`, `allow_redirec 1. User provides JSON/JSONL (file / paste / API URL with optional auth). 2. Server validates input (SSRF check for API URLs, UTF-8 decoding, JSON/JSONL parsing, size caps). -3. If multiple candidate arrays found and no `json_path` is supplied, server returns the candidates so the UI can prompt the user to pick one. +3. If no `json_path` is supplied, the server returns `raw_json` so the UI can render a tree picker and let the user choose a node. 4. Server returns `preview` (first `PREVIEW_ROW_LIMIT` rows) plus full `csv_data` / `csv_columns`. 5. Frontend renders the sortable preview table; nested objects render as mini tables. 6. Export: CSV/TSV generated client-side instantly; Excel via the server endpoint. @@ -161,7 +160,7 @@ python -m pytest tests/ -v ``` Test files: -- `tests/test_helpers.py` (31 tests) — `flatten_for_csv`, `extract_table_data`, `get_all_columns`, `parse_jsonl`, `find_candidate_arrays`, `extract_by_path`. +- `tests/test_helpers.py` — `flatten_for_csv`, `extract_table_data`, `get_all_columns`, `parse_jsonl`, `extract_by_path`. - `tests/test_security.py` (16 tests) — URL validation with mocked DNS, private/loopback/link-local IP blocking, scheme checks. - `tests/test_routes.py` (35 tests) — All route integration tests, security headers, JSONL, path selection, API-fetch SSRF/size/timeout/error-leak coverage, CSV/Excel export edge cases. diff --git a/MEMORY.md b/MEMORY.md index ed82583..46d27b6 100644 --- a/MEMORY.md +++ b/MEMORY.md @@ -63,10 +63,10 @@ Keep entries short — if it grows past ~10 lines, it probably belongs in `READM **Why:** All state-changing routes accept browser form posts, so CSRF is mandatory. Disabling it in tests keeps fixtures simple — production behavior is exercised manually and via the security headers test. **How to apply:** When adding a route that mutates state or returns sensitive data, it inherits CSRF protection automatically. Don't add `@csrf.exempt` without justification. When testing CSRF behavior, do so in a dedicated test that flips `WTF_CSRF_ENABLED` back on. -### 2026-05-12 — Multi-array JSON triggers a path-selector handshake (area: backend / ux) +### 2026-05-12 — Unselected JSON triggers the tree-picker handshake (area: backend / ux) -**What:** `/process` returns `{"needs_selection": true, "candidates": [...]}` (HTTP 200) when `find_candidate_arrays` reports more than one array of objects in the payload. The frontend opens a modal; the user picks; the request is re-submitted with `json_path` set to the chosen dotted path. -**Why:** The original heuristic (`extract_table_data`) silently picked the first array it found, which surfaced the wrong data for nested API responses. Returning candidates is more honest than guessing. +**What:** `/process` returns `{"needs_selection": true, "raw_json": }` (HTTP 200) whenever no `json_path` was supplied. The frontend renders a JSON **tree picker** over `raw_json`; the user clicks any array or object node; the request is re-submitted with `json_path` set to the chosen dotted path. +**Why:** The original heuristic (`extract_table_data`) silently picked the first array it found, which surfaced the wrong data for nested API responses. Handing the client the document and letting the user point at a node is more honest than guessing — and unlike the earlier `candidates` list it can reach any level, not just arrays of objects. **How to apply:** Don't "fix" the heuristic by being smarter — the selection prompt *is* the fix. The sentinel `'(root)'` is used when the top-level value is itself a list. ### 2026-05-12 — `flatten_for_csv` has a recursion-depth cap (area: backend) diff --git a/helpers.py b/helpers.py index c466f31..1a28d28 100644 --- a/helpers.py +++ b/helpers.py @@ -81,32 +81,6 @@ def parse_jsonl(text): return rows -def find_candidate_arrays(json_data, prefix='', candidates=None): - """ - Find all arrays of objects in JSON data, returning their paths and metadata. - Used when multiple arrays exist so the user can choose which to tabularize. - """ - if candidates is None: - candidates = [] - - if isinstance(json_data, list): - if len(json_data) > 0 and isinstance(json_data[0], dict): - sample_keys = sorted(json_data[0].keys())[:5] - candidates.append( - {'path': prefix or '(root)', 'length': len(json_data), 'sample_keys': sample_keys} - ) - elif isinstance(json_data, dict): - for key, value in json_data.items(): - path = f'{prefix}.{key}' if prefix else key - if isinstance(value, list) and len(value) > 0 and isinstance(value[0], dict): - sample_keys = sorted(value[0].keys())[:5] - candidates.append({'path': path, 'length': len(value), 'sample_keys': sample_keys}) - elif isinstance(value, dict): - find_candidate_arrays(value, path, candidates) - - return candidates - - def extract_by_path(json_data, path): """ Extract data from JSON using a dot-notation path. diff --git a/tests/test_helpers.py b/tests/test_helpers.py index cec96bd..945baaa 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -5,7 +5,6 @@ from helpers import ( extract_by_path, extract_table_data, - find_candidate_arrays, flatten_for_csv, get_all_columns, parse_jsonl, @@ -113,33 +112,6 @@ def test_mixed_objects(self): assert result[1]['name'] == 'Bob' -class TestFindCandidateArrays: - def test_single_array_at_root(self): - data = [{'id': 1}, {'id': 2}] - candidates = find_candidate_arrays(data) - assert len(candidates) == 1 - assert candidates[0]['path'] == '(root)' - assert candidates[0]['length'] == 2 - - def test_multiple_arrays(self): - data = {'users': [{'name': 'A'}], 'orders': [{'id': 1}, {'id': 2}]} - candidates = find_candidate_arrays(data) - assert len(candidates) == 2 - paths = [c['path'] for c in candidates] - assert 'users' in paths - assert 'orders' in paths - - def test_nested_array(self): - data = {'data': {'items': [{'x': 1}]}} - candidates = find_candidate_arrays(data) - assert len(candidates) == 1 - assert candidates[0]['path'] == 'data.items' - - def test_no_arrays(self): - data = {'a': 1, 'b': 'text'} - assert find_candidate_arrays(data) == [] - - class TestExtractByPath: def test_root_path(self): data = [{'a': 1}] From e493e2929da0e82f8e13f6af8f0ad4014c3cf639 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:26:06 +0000 Subject: [PATCH 07/36] Phase 1.1: neutralize spreadsheet formula injection on all four export paths F1 (Critical, CWE-1236). Cell values beginning with = + - @ tab CR or LF were written verbatim into CSV, TSV and XLSX, so a value from an attacker-controlled API response became a live formula when the export was opened. Per-format policy, as F1 prescribes: - CSV/TSV (server export_csv and client downloadDelimited): delimited output has no type channel, so a dangerous value is prefixed with a single quote. Sanitization happens before delimiter quoting. - XLSX: the format carries an explicit type per cell, so the value is written untouched and the cell's data_type is pinned to 's'. openpyxl otherwise infers a formula cell for any string starting with '='. Verified to survive a save/reload round trip. Column headers go through the same sanitizer -- a header is a cell too. helpers.py gains serialize_cell_value / is_formula_trigger / sanitize_cell (the helper roadmap 3.2 schedules for creation here), which also replaces the duplicated isinstance(v, (dict, list)) branches in both export routes. Tests: seven server-side tests covering every trigger, safe values, headers, container serialization and numeric preservation. The two client paths are covered by tests/js/test_export_sanitize.mjs, which loads the real static/js/app.js in a stubbed DOM and asserts both delimiters -- 37 assertions, wired into CI. No build step and no new dependency. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- .github/workflows/ci.yml | 7 +++ helpers.py | 39 ++++++++++++ routes.py | 45 +++++++------- static/js/app.js | 46 +++++++++++--- tests/js/dom_stub.mjs | 73 +++++++++++++++++++++++ tests/js/test_export_sanitize.mjs | 99 +++++++++++++++++++++++++++++++ tests/test_routes.py | 94 +++++++++++++++++++++++++++++ 7 files changed, 375 insertions(+), 28 deletions(-) create mode 100644 tests/js/dom_stub.mjs create mode 100644 tests/js/test_export_sanitize.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7522091..c814ad9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,8 +26,15 @@ jobs: - name: Format check run: ruff format --check . + - uses: actions/setup-node@v4 + with: + node-version: "22" + - name: Tests run: python -m pytest tests/ -v + - name: Client-side export assertions (F1) + run: node tests/js/test_export_sanitize.mjs + - name: Audit runtime dependencies run: pip-audit -r requirements.txt diff --git a/helpers.py b/helpers.py index 1a28d28..c1b8c13 100644 --- a/helpers.py +++ b/helpers.py @@ -33,6 +33,45 @@ def flatten_for_csv(data, parent_key='', sep='.', _depth=0, max_depth=10): return dict(items) +# Characters that make a spreadsheet treat a cell as a formula (or let it smuggle +# extra rows/fields past a delimited parser). OWASP lists all seven. +FORMULA_TRIGGERS = ('=', '+', '-', '@', '\t', '\r', '\n') + + +def serialize_cell_value(value): + """ + Reduce one cell value to the scalar an export writer can emit. + + Containers become their JSON text; everything else is passed through + unchanged so numbers stay numbers in the workbook. + """ + if isinstance(value, (dict, list)): + return json.dumps(value) + return value + + +def is_formula_trigger(value): + """True when a serialized value would be read as a formula by a spreadsheet.""" + return isinstance(value, str) and value.startswith(FORMULA_TRIGGERS) + + +def sanitize_cell(value): + """ + Serialize a cell for the delimited formats (CSV/TSV) and defuse formula + injection (CWE-1236). + + Delimited output has no type channel, so a dangerous value is prefixed with a + single quote -- the OWASP mitigation for CSV. XLSX does not use this: it has a + real string type, so `export_xlsx` writes the untouched value and pins the + cell's data_type instead (see `is_formula_trigger`). Lossless exports (JSONL) + and Markdown must not call this. + """ + serialized = serialize_cell_value(value) + if is_formula_trigger(serialized): + return "'" + serialized + return serialized + + def extract_table_data(json_data): """ Extract tabular data from JSON. diff --git a/routes.py b/routes.py index c68aff3..e417de9 100644 --- a/routes.py +++ b/routes.py @@ -15,7 +15,10 @@ extract_table_data, flatten_for_csv, get_all_columns, + is_formula_trigger, parse_jsonl, + sanitize_cell, + serialize_cell_value, ) from security import validate_url @@ -195,6 +198,23 @@ def process_json(): return jsonify({'error': 'An internal error occurred'}), 500 +def _append_xlsx_row(ws, values): + """ + Append one row to a worksheet, writing formula-triggering strings as strings. + + openpyxl serializes a str starting with '=' as a formula cell, so Excel would + evaluate an attacker-supplied value on open (F1). XLSX carries an explicit + type per cell, so the fix is to pin data_type rather than mangle the text the + way the delimited exports have to. + """ + serialized = [serialize_cell_value(value) for value in values] + ws.append(serialized) + row_index = ws.max_row + for column_index, value in enumerate(serialized, start=1): + if is_formula_trigger(value): + ws.cell(row=row_index, column=column_index).data_type = 's' + + @bp.route('/export-csv', methods=['POST']) @limiter.limit(lambda: current_app.config.get('RATE_LIMIT_EXPORT', '60/minute')) def export_csv(): @@ -211,17 +231,11 @@ def export_csv(): return jsonify({'error': 'No data to export'}), 400 output = io.StringIO() - writer = csv.DictWriter(output, fieldnames=csv_columns, extrasaction='ignore') - writer.writeheader() + writer = csv.writer(output) + writer.writerow([sanitize_cell(col) for col in csv_columns]) for row in csv_data: - clean_row = {} - for k, v in row.items(): - if isinstance(v, (dict, list)): - clean_row[k] = json.dumps(v) - else: - clean_row[k] = v - writer.writerow(clean_row) + writer.writerow([sanitize_cell(row.get(col, '')) for col in csv_columns]) output.seek(0) return Response( @@ -259,18 +273,9 @@ def export_xlsx(): ws = wb.active ws.title = 'Data' - # Header row - ws.append(xlsx_columns) - - # Data rows + _append_xlsx_row(ws, xlsx_columns) for row in xlsx_data: - row_values = [] - for col in xlsx_columns: - v = row.get(col, '') - if isinstance(v, (dict, list)): - v = json.dumps(v) - row_values.append(v) - ws.append(row_values) + _append_xlsx_row(ws, [row.get(col, '') for col in xlsx_columns]) output = io.BytesIO() wb.save(output) diff --git a/static/js/app.js b/static/js/app.js index 6eb5939..ca28c6e 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -478,8 +478,34 @@ document.querySelectorAll('.export-dropdown-item').forEach(item => { }); }); -// Client-side CSV/TSV generation -function downloadDelimited(columns, data, delimiter, filename) { +// Spreadsheet formula triggers (OWASP). Must stay in sync with +// helpers.FORMULA_TRIGGERS on the server. +function formulaTriggers() { + return ['=', '+', '-', '@', '\t', '\r', '\n']; +} + +// Reduce a cell to the scalar a writer emits. Containers become their JSON text. +function serializeCellValue(value) { + if (value === null || value === undefined) return ''; + if (typeof value === 'object') return JSON.stringify(value); + return value; +} + +// Defuse CSV/TSV formula injection (CWE-1236) by prefixing a dangerous value +// with a single quote. Delimited output carries no type channel, so this is the +// only place the value can be marked as text. JSONL and Markdown exports are +// deliberately NOT routed through here. +function sanitizeCell(value) { + const serialized = serializeCellValue(value); + if (typeof serialized === 'string' && formulaTriggers().some(t => serialized.startsWith(t))) { + return "'" + serialized; + } + return serialized; +} + +// Pure string builder, kept separate from the download so it can be asserted +// directly (tests/js/test_export_sanitize.mjs). +function buildDelimited(columns, data, delimiter) { const escape = (val) => { const str = String(val ?? ''); if (str.includes(delimiter) || str.includes('"') || str.includes('\n')) { @@ -488,15 +514,19 @@ function downloadDelimited(columns, data, delimiter, filename) { return str; }; - let output = columns.map(escape).join(delimiter) + '\n'; + let output = columns.map(col => escape(sanitizeCell(col))).join(delimiter) + '\n'; data.forEach(row => { - const line = columns.map(col => { - let v = row[col]; - if (typeof v === 'object' && v !== null) v = JSON.stringify(v); - return escape(v); - }).join(delimiter); + const line = columns + .map(col => escape(sanitizeCell(row[col]))) + .join(delimiter); output += line + '\n'; }); + return output; +} + +// Client-side CSV/TSV generation +function downloadDelimited(columns, data, delimiter, filename) { + const output = buildDelimited(columns, data, delimiter); const mimeType = delimiter === '\t' ? 'text/tab-separated-values; charset=utf-8' diff --git a/tests/js/dom_stub.mjs b/tests/js/dom_stub.mjs new file mode 100644 index 0000000..8b76a3e --- /dev/null +++ b/tests/js/dom_stub.mjs @@ -0,0 +1,73 @@ +// Minimal DOM/browser stubs so static/js/app.js can be loaded in Node for +// assertions on its pure export helpers. No build step, no dependencies: the +// script under test is the exact file the browser gets. + +function makeElement() { + const el = { + dataset: {}, + files: [], + style: {}, + textContent: '', + innerHTML: '', + value: '', + disabled: false, + classList: { + add() {}, + remove() {}, + toggle() { return false; }, + contains() { return false; }, + }, + addEventListener() {}, + removeEventListener() {}, + appendChild() {}, + remove() {}, + setAttribute() {}, + getAttribute() { return 'test-csrf-token'; }, + click() {}, + closest() { return null; }, + querySelector() { return makeElement(); }, + querySelectorAll() { return []; }, + }; + return el; +} + +export function createContext() { + const document = { + body: makeElement(), + documentElement: makeElement(), + createElement: () => makeElement(), + getElementById: () => makeElement(), + querySelector: () => makeElement(), + querySelectorAll: () => [], + addEventListener() {}, + }; + + const window = { + matchMedia: () => ({ matches: false, addEventListener() {} }), + URL: { createObjectURL: () => 'blob:stub', revokeObjectURL() {} }, + location: { hash: '' }, + }; + + const context = { + document, + window, + localStorage: { getItem: () => null, setItem() {}, removeItem() {} }, + location: window.location, + Blob: class Blob { + constructor(parts, options) { + this.parts = parts; + this.type = options && options.type; + } + }, + FormData: class FormData { + append() {} + }, + fetch: async () => ({ ok: true, json: async () => ({}) }), + alert() {}, + console, + setTimeout, + clearTimeout, + }; + context.globalThis = context; + return context; +} diff --git a/tests/js/test_export_sanitize.mjs b/tests/js/test_export_sanitize.mjs new file mode 100644 index 0000000..650dcf8 --- /dev/null +++ b/tests/js/test_export_sanitize.mjs @@ -0,0 +1,99 @@ +// F1 - client-side export paths must not emit spreadsheet formulas. +// +// Exercises the real static/js/app.js in a stubbed DOM and asserts the CSV and +// TSV builders neutralize every OWASP formula trigger, in both cell values and +// column headers. Run with: node tests/js/test_export_sanitize.mjs + +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; +import vm from 'node:vm'; +import assert from 'node:assert/strict'; +import { createContext } from './dom_stub.mjs'; + +const here = dirname(fileURLToPath(import.meta.url)); +const appJs = readFileSync(join(here, '..', '..', 'static', 'js', 'app.js'), 'utf8'); + +const context = vm.createContext(createContext()); +vm.runInContext(appJs, context, { filename: 'app.js' }); + +const { sanitizeCell, buildDelimited } = context; +assert.equal(typeof sanitizeCell, 'function', 'sanitizeCell must be reachable'); +assert.equal(typeof buildDelimited, 'function', 'buildDelimited must be reachable'); + +const DANGEROUS = ['=SUM(A1)', '@cmd', '+1', '-1', '\tlead', '\rlead', '\nlead']; +const SAFE = ['plain', 'a=b', 'user@example.com', '', '0']; + +let checks = 0; +const check = (label, fn) => { + fn(); + checks += 1; +}; + +// --- sanitizeCell itself ------------------------------------------------- +for (const value of DANGEROUS) { + check(`sanitizeCell ${JSON.stringify(value)}`, () => { + assert.equal(sanitizeCell(value), `'${value}`); + }); +} +for (const value of SAFE) { + check(`sanitizeCell passthrough ${JSON.stringify(value)}`, () => { + assert.equal(sanitizeCell(value), value); + }); +} +check('sanitizeCell serializes containers', () => { + assert.equal(sanitizeCell({ a: 1 }), '{"a":1}'); + assert.equal(sanitizeCell([1, 2]), '[1,2]'); +}); +check('sanitizeCell maps null/undefined to empty', () => { + assert.equal(sanitizeCell(null), ''); + assert.equal(sanitizeCell(undefined), ''); +}); +check('numbers are not prefixed', () => { + assert.equal(sanitizeCell(-1), -1); + assert.equal(sanitizeCell(1), 1); +}); + +// --- both delimiters, values and headers --------------------------------- +for (const delimiter of [',', '\t']) { + const name = delimiter === ',' ? 'CSV' : 'TSV'; + + for (const value of DANGEROUS) { + check(`${name} value ${JSON.stringify(value)}`, () => { + const out = buildDelimited(['col'], [{ col: value }], delimiter); + const body = out.split('\n')[1]; + assert.ok( + body.startsWith("'") || body.startsWith('"\''), + `${name} cell ${JSON.stringify(value)} not neutralized: ${JSON.stringify(body)}` + ); + assert.ok( + !body.startsWith(value[0]), + `${name} cell ${JSON.stringify(value)} still leads with a trigger` + ); + }); + } + + check(`${name} header is sanitized too`, () => { + const out = buildDelimited(['=EVIL()'], [{ '=EVIL()': 1 }], delimiter); + const header = out.split('\n')[0]; + assert.ok(header.startsWith("'="), `${name} header not neutralized: ${header}`); + }); + + check(`${name} safe values are untouched`, () => { + const out = buildDelimited(['a', 'b'], [{ a: 'plain', b: 5 }], delimiter); + assert.equal(out, `a${delimiter}b\nplain${delimiter}5\n`); + }); + + check(`${name} quoting still applies after sanitization`, () => { + const out = buildDelimited(['a'], [{ a: `x${delimiter}y` }], delimiter); + assert.equal(out.split('\n')[1], `"x${delimiter}y"`); + }); + + check(`${name} objects are serialized`, () => { + const out = buildDelimited(['a'], [{ a: { k: 'v' } }], delimiter); + // Quotes inside the JSON force delimited quoting/doubling. + assert.equal(out.split('\n')[1], '"{""k"":""v""}"'); + }); +} + +console.log(`ok - ${checks} client export assertions passed`); diff --git a/tests/test_routes.py b/tests/test_routes.py index ef6e150..c3e9814 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -1,5 +1,7 @@ """Tests for Flask routes.""" +import csv +import io import json from unittest.mock import MagicMock, patch @@ -419,3 +421,95 @@ def test_xcontent_type_header(self, client): def test_referrer_policy(self, client): response = client.get('/') assert 'strict-origin' in response.headers['Referrer-Policy'] + + +class TestFormulaInjection: + """F1 - CSV/XLSX formula injection (CWE-1236).""" + + DANGEROUS = ['=SUM(A1)', '@cmd', '+1', '-1', '\tlead', '\rlead', '\nlead'] + + def test_csv_prefixes_every_trigger(self, client): + rows = [{'v': value} for value in self.DANGEROUS] + response = client.post( + '/export-csv', + data=json.dumps({'csv_data': rows, 'csv_columns': ['v']}), + content_type='application/json', + ) + assert response.status_code == 200 + + parsed = list(csv.reader(io.StringIO(response.data.decode('utf-8')))) + emitted = [row[0] for row in parsed[1:]] + assert emitted == ["'" + value for value in self.DANGEROUS] + + def test_csv_leaves_safe_values_alone(self, client): + response = client.post( + '/export-csv', + data=json.dumps( + { + 'csv_data': [{'a': 'plain', 'b': 5, 'c': 'user@example.com'}], + 'csv_columns': ['a', 'b', 'c'], + } + ), + content_type='application/json', + ) + parsed = list(csv.reader(io.StringIO(response.data.decode('utf-8')))) + assert parsed[1] == ['plain', '5', 'user@example.com'] + + def test_csv_sanitizes_column_headers(self, client): + response = client.post( + '/export-csv', + data=json.dumps({'csv_data': [{'=EVIL()': 1}], 'csv_columns': ['=EVIL()']}), + content_type='application/json', + ) + parsed = list(csv.reader(io.StringIO(response.data.decode('utf-8')))) + assert parsed[0] == ["'=EVIL()"] + + def test_csv_serializes_containers(self, client): + response = client.post( + '/export-csv', + data=json.dumps({'csv_data': [{'a': {'k': 'v'}}], 'csv_columns': ['a']}), + content_type='application/json', + ) + parsed = list(csv.reader(io.StringIO(response.data.decode('utf-8')))) + assert parsed[1] == ['{"k": "v"}'] + + def test_xlsx_writes_triggers_as_string_cells(self, client): + from openpyxl import load_workbook + + rows = [{'v': value} for value in self.DANGEROUS] + response = client.post( + '/export-xlsx', + data=json.dumps({'csv_data': rows, 'csv_columns': ['v']}), + content_type='application/json', + ) + assert response.status_code == 200 + + ws = load_workbook(io.BytesIO(response.data)).active + for index, value in enumerate(self.DANGEROUS, start=2): + cell = ws.cell(row=index, column=1) + assert cell.data_type == 's', f'{value!r} was written as {cell.data_type}' + # XLSX carries an explicit type, so the text itself stays intact. + assert cell.value == value.replace('\r', '\n') + + def test_xlsx_sanitizes_column_headers(self, client): + from openpyxl import load_workbook + + response = client.post( + '/export-xlsx', + data=json.dumps({'csv_data': [{'=EVIL()': 1}], 'csv_columns': ['=EVIL()']}), + content_type='application/json', + ) + ws = load_workbook(io.BytesIO(response.data)).active + assert ws.cell(row=1, column=1).data_type == 's' + + def test_xlsx_keeps_numbers_numeric(self, client): + from openpyxl import load_workbook + + response = client.post( + '/export-xlsx', + data=json.dumps({'csv_data': [{'a': -1, 'b': 2.5}], 'csv_columns': ['a', 'b']}), + content_type='application/json', + ) + ws = load_workbook(io.BytesIO(response.data)).active + assert ws.cell(row=2, column=1).value == -1 + assert ws.cell(row=2, column=2).value == 2.5 From 1fd8a259d1abdc311249af719b40aad64a9dea83 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:26:46 +0000 Subject: [PATCH 08/36] Phase 1.2/1.3: stop logging API-fetch URLs; malformed API JSONL is a 400 Task 1.2 (F3/F9): logger.warning('API request failed: %s', e) wrote requests' exception text to stdout, which contains the full URL. With query_param auth the token rides in that URL, so the secret was logged. Paths, fragments and userinfo can carry tokens too, so the fix is a fixed message with no interpolation rather than a redaction helper. Task 1.3 (F9): parse_jsonl raises ValueError on a malformed line, but the API branch caught only Timeout / RequestException / JSONDecodeError. The ValueError reached the outer handler as a logged 500 even though the remote data, not the app, was at fault. It now returns 400 with a generic message and is not logged. The clause sits after the JSONDecodeError one because JSONDecodeError is itself a ValueError. Tests: caplog assertions that neither a query-string token, a path token nor the query_param auth value appears in any log record, and that a malformed JSONL response yields 400 with no error-level record. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- routes.py | 13 +++++- tests/test_routes.py | 108 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/routes.py b/routes.py index e417de9..e6d9000 100644 --- a/routes.py +++ b/routes.py @@ -144,11 +144,20 @@ def process_json(): except requests.exceptions.Timeout: return jsonify({'error': 'API request timed out'}), 400 - except requests.exceptions.RequestException as e: - logger.warning('API request failed: %s', e) + except requests.exceptions.RequestException: + # Fixed message, no interpolation: requests' exception text carries + # the full URL, and the query string, fragment, userinfo AND path + # can each hold a token (F3/F9). Redacting one component is not + # enough, so nothing user-controlled is logged at all. + logger.warning('API request failed') return jsonify({'error': 'API request failed'}), 400 except json.JSONDecodeError: return jsonify({'error': 'API response is not valid JSON'}), 400 + except ValueError: + # parse_jsonl raises ValueError on a malformed line. Without this + # it reached the outer handler as a 500 with a logged traceback, + # although it is the caller's data that is wrong (F9). + return jsonify({'error': 'API response is not valid JSONL'}), 400 else: return jsonify({'error': 'Invalid input method'}), 400 diff --git a/tests/test_routes.py b/tests/test_routes.py index c3e9814..8f9a0b3 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -3,6 +3,7 @@ import csv import io import json +import logging from unittest.mock import MagicMock, patch @@ -513,3 +514,110 @@ def test_xlsx_keeps_numbers_numeric(self, client): ws = load_workbook(io.BytesIO(response.data)).active assert ws.cell(row=2, column=1).value == -1 assert ws.cell(row=2, column=2).value == 2.5 + + +class TestApiFetchLogHygiene: + """F3/F9 - no URL component or token may reach the logs.""" + + SECRET = 'sup3rs3cr3t-token' + + def _post(self, client, url, **extra): + data = {'input_method': 'api', 'api_url': url} + data.update(extra) + return client.post('/process', data=data) + + @patch('routes.requests.get') + @patch('routes.validate_url') + def test_token_in_query_string_never_logged(self, mock_validate, mock_get, client, caplog): + import requests as req + + mock_validate.return_value = (True, None) + url = f'https://api.example.com/data?api_key={self.SECRET}' + # requests puts the whole URL in the exception message. + mock_get.side_effect = req.exceptions.ConnectionError( + f"HTTPSConnectionPool(host='api.example.com', port=443): " + f'Max retries exceeded with url: /data?api_key={self.SECRET}' + ) + + with caplog.at_level(logging.DEBUG): + response = self._post(client, url) + + assert response.status_code == 400 + assert json.loads(response.data)['error'] == 'API request failed' + + logged = '\n'.join(record.getMessage() for record in caplog.records) + assert self.SECRET not in logged + assert 'api.example.com' not in logged + assert '/data' not in logged + assert 'API request failed' in logged + + @patch('routes.requests.get') + @patch('routes.validate_url') + def test_token_in_path_never_logged(self, mock_validate, mock_get, client, caplog): + import requests as req + + mock_validate.return_value = (True, None) + url = f'https://api.example.com/v1/{self.SECRET}/data' + mock_get.side_effect = req.exceptions.ConnectionError( + f'Failed to establish a new connection to /v1/{self.SECRET}/data' + ) + + with caplog.at_level(logging.DEBUG): + response = self._post(client, url) + + assert response.status_code == 400 + logged = '\n'.join(record.getMessage() for record in caplog.records) + assert self.SECRET not in logged + assert 'v1' not in logged + + @patch('routes.requests.get') + @patch('routes.validate_url') + def test_query_param_auth_value_never_logged(self, mock_validate, mock_get, client, caplog): + import requests as req + + mock_validate.return_value = (True, None) + mock_get.side_effect = req.exceptions.ConnectionError('boom') + + with caplog.at_level(logging.DEBUG): + response = self._post( + client, + 'https://api.example.com/data', + auth_method='query_param', + query_param_name='api_key', + query_param_value=self.SECRET, + ) + + assert response.status_code == 400 + assert self.SECRET not in '\n'.join(r.getMessage() for r in caplog.records) + + +class TestApiFetchJsonlErrors: + """F9 - a malformed JSONL body from the API is a 400, not a logged 500.""" + + @patch('routes.requests.get') + @patch('routes.validate_url') + def test_malformed_jsonl_returns_400(self, mock_validate, mock_get, client, caplog): + mock_validate.return_value = (True, None) + mock_resp = MagicMock() + mock_resp.status_code = 200 + mock_resp.iter_content.return_value = [b'{"a": 1}\n{bad json}\n'] + mock_resp.raise_for_status.return_value = None + mock_get.return_value = mock_resp + + with caplog.at_level(logging.DEBUG): + response = client.post( + '/process', + data={ + 'input_method': 'api', + 'api_url': 'https://api.example.com/data', + 'data_format': 'jsonl', + }, + ) + + assert response.status_code == 400 + assert json.loads(response.data)['error'] == 'API response is not valid JSONL' + + logged = '\n'.join(record.getMessage() for record in caplog.records) + assert 'Unexpected error' not in logged + assert 'bad json' not in logged + assert not [r for r in caplog.records if r.levelno >= logging.ERROR] From 4c8a49a378f240272c5e37dbf28fcec7840628ae Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:27:37 +0000 Subject: [PATCH 09/36] Phase 1.4: replace the outbound header rule with a real allowlist F4. The client supplies the header NAME for api_key auth, so `Host`, `Transfer-Encoding`, `Connection`, `Proxy-Authorization` and `Cookie` could all be forwarded to the target. A token regex plus a list of rejected names would still be a denylist, and because HTTP field names are case-insensitive a lowercase membership test lets `Host`, `PROXY-AUTHORIZATION` and `CoNnEcTiOn` through. is_allowed_outbound_header strips and lowercases the name BEFORE any comparison and then requires membership in an explicit permitted set (accept, accept-language, authorization, user-agent, x-api-key). The [A-Za-z0-9-]+ regex stays as a syntax check, not as the authorization decision. Anything else is a 400 and no request is made. authorization is on the list because the bearer auth path already sets it server-side, so permitting it here grants no capability the UI does not already offer. Tests: 16 rejected names covering mixed case, hop-by-hop headers, and whitespace-padded variants; a permitted header in unusual case is forwarded with its original spelling; the default X-API-Key still works. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- routes.py | 37 +++++++++++++++++++- tests/test_routes.py | 80 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/routes.py b/routes.py index e6d9000..3ab93dd 100644 --- a/routes.py +++ b/routes.py @@ -4,6 +4,7 @@ import io import json import logging +import re import requests from flask import Blueprint, Response, current_app, jsonify, render_template, request @@ -27,6 +28,33 @@ bp = Blueprint('main', __name__) +# F4: outbound header names come from the client, so this is a real allowlist, +# not a token regex plus a list of names to reject. A denylist cannot work here: +# HTTP field names are case-insensitive, so `Host`, `PROXY-AUTHORIZATION` and +# `CoNnEcTiOn` all slip past a lowercase membership test, and anything simply +# absent from the list would pass. Names are stripped and lowercased before the +# membership test; the regex stays only as a syntax check on top of it. +ALLOWED_OUTBOUND_HEADERS = frozenset( + { + 'accept', + 'accept-language', + 'authorization', + 'user-agent', + 'x-api-key', + } +) + +HEADER_NAME_PATTERN = re.compile(r'^[A-Za-z0-9-]+$') + + +def is_allowed_outbound_header(name): + """True when a client-supplied outbound header name may be forwarded.""" + normalized = name.strip().lower() + if not HEADER_NAME_PATTERN.match(normalized): + return False + return normalized in ALLOWED_OUTBOUND_HEADERS + + @bp.route('/') def index(): """Render the main page.""" @@ -93,7 +121,14 @@ def process_json(): header_name = request.form.get('api_key_header', 'X-API-Key') api_key = request.form.get('api_key', '') if api_key: - headers[header_name] = api_key + if not is_allowed_outbound_header(header_name): + return jsonify( + { + 'error': 'Header name is not permitted. Allowed: ' + + ', '.join(sorted(ALLOWED_OUTBOUND_HEADERS)) + } + ), 400 + headers[header_name.strip()] = api_key elif auth_method == 'basic': username = request.form.get('basic_username', '') password = request.form.get('basic_password', '') diff --git a/tests/test_routes.py b/tests/test_routes.py index 8f9a0b3..161b93c 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -621,3 +621,83 @@ def test_malformed_jsonl_returns_400(self, mock_validate, mock_get, client, capl assert 'Unexpected error' not in logged assert 'bad json' not in logged assert not [r for r in caplog.records if r.levelno >= logging.ERROR] + + +class TestOutboundHeaderAllowlist: + """F4 - the client supplies the outbound header NAME; only an allowlist passes.""" + + REJECTED = [ + 'Host', + 'host', + 'HOST', + 'Content-Length', + 'Transfer-Encoding', + 'Connection', + 'CoNnEcTiOn', + 'Proxy-Authorization', + 'PROXY-AUTHORIZATION', + 'Cookie', + 'X-CSRF-Token', + ' Host ', + 'Host\t', + 'X Api Key', + 'X-Api-Key:', + '', + ] + + def _fetch(self, client, header_name): + return client.post( + '/process', + data={ + 'input_method': 'api', + 'api_url': 'https://api.example.com/data', + 'auth_method': 'api_key', + 'api_key_header': header_name, + 'api_key': 'secret', + 'json_path': '(root)', + }, + ) + + @patch('routes.requests.get') + @patch('routes.validate_url') + def test_reserved_and_hop_by_hop_names_rejected(self, mock_validate, mock_get, client): + mock_validate.return_value = (True, None) + + for name in self.REJECTED: + response = self._fetch(client, name) + assert response.status_code == 400, f'{name!r} was accepted' + assert 'not permitted' in json.loads(response.data)['error'] + + # Nothing was ever sent upstream. + assert mock_get.call_count == 0 + + @patch('routes.requests.get') + @patch('routes.validate_url') + def test_permitted_header_in_unusual_case_is_forwarded(self, mock_validate, mock_get, client): + mock_validate.return_value = (True, None) + mock_resp = MagicMock() + mock_resp.status_code = 200 + mock_resp.iter_content.return_value = [b'[{"id": 1}]'] + mock_resp.raise_for_status.return_value = None + mock_get.return_value = mock_resp + + response = self._fetch(client, ' x-API-kEy ') + assert json.loads(response.data)['success'] is True + + _, kwargs = mock_get.call_args + assert kwargs['headers'] == {'x-API-kEy': 'secret'} + + @patch('routes.requests.get') + @patch('routes.validate_url') + def test_default_header_still_works(self, mock_validate, mock_get, client): + mock_validate.return_value = (True, None) + mock_resp = MagicMock() + mock_resp.status_code = 200 + mock_resp.iter_content.return_value = [b'[{"id": 1}]'] + mock_resp.raise_for_status.return_value = None + mock_get.return_value = mock_resp + + response = self._fetch(client, 'X-API-Key') + assert json.loads(response.data)['success'] is True + _, kwargs = mock_get.call_args + assert kwargs['headers'] == {'X-API-Key': 'secret'} From c5291b2ad1c2abd78343fe61f022f249aab64d77 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:28:16 +0000 Subject: [PATCH 10/36] Phase 1.5: add the missing hardening headers and tighten CSP F5. Adds Permissions-Policy, Cross-Origin-Opener-Policy, Cross-Origin-Resource-Policy and HSTS, and extends CSP with object-src 'none', base-uri 'self', frame-ancestors 'none', form-action 'self' and upgrade-insecure-requests (F14's completion criterion makes the last one mandatory, not optional). The policy is built from a tuple of directives joined with '; ' rather than by string concatenation, so a future addition cannot fuse two directives into one malformed token. A test asserts every directive parses as a directive. HSTS is emitted only for request.is_secure, so a local http run is unaffected. Behind a TLS-terminating proxy that flag comes from X-Forwarded-Proto, which is only honored once ProxyFix is enabled (TRUST_PROXY=1, task 1.10). X-Frame-Options: DENY stays as the legacy fallback. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- security.py | 45 ++++++++++++++++++++++++++++++++-------- tests/test_routes.py | 49 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/security.py b/security.py index d635e63..3a49dd7 100644 --- a/security.py +++ b/security.py @@ -4,6 +4,30 @@ import socket from urllib.parse import urlparse +from flask import request + +# Built as a list of directives so appending can never fuse two tokens into one +# malformed directive (F5). Google Fonts is the only third-party origin the page +# uses; scripts stay self-only, so no inline JS is possible anywhere. +CSP_DIRECTIVES = ( + "default-src 'self'", + "script-src 'self'", + "style-src 'self' https://fonts.googleapis.com", + "font-src 'self' https://fonts.gstatic.com", + "img-src 'self' data:", + "connect-src 'self'", + # Shrink the XSS blast radius: no plugins, no hijacking, no framing, + # no cross-origin form exfiltration. + "object-src 'none'", + "base-uri 'self'", + "frame-ancestors 'none'", + "form-action 'self'", + # Inert on a plain-http deployment, mandatory on an https one (F5/F14). + 'upgrade-insecure-requests', +) + +CONTENT_SECURITY_POLICY = '; '.join(CSP_DIRECTIVES) + def validate_url(url): """ @@ -45,14 +69,19 @@ def validate_url(url): def apply_security_headers(response): """Add security headers to every response.""" response.headers['X-Content-Type-Options'] = 'nosniff' + # Legacy fallback for browsers predating CSP frame-ancestors. response.headers['X-Frame-Options'] = 'DENY' response.headers['Referrer-Policy'] = 'strict-origin-when-cross-origin' - response.headers['Content-Security-Policy'] = ( - "default-src 'self'; " - "script-src 'self'; " - "style-src 'self' https://fonts.googleapis.com; " - "font-src 'self' https://fonts.gstatic.com; " - "img-src 'self' data:; " - "connect-src 'self'" - ) + response.headers['Permissions-Policy'] = 'camera=(), microphone=(), geolocation=()' + response.headers['Cross-Origin-Opener-Policy'] = 'same-origin' + response.headers['Cross-Origin-Resource-Policy'] = 'same-origin' + response.headers['Content-Security-Policy'] = CONTENT_SECURITY_POLICY + + # HSTS only makes sense once the connection is already TLS; sending it over + # plain http would pin a local dev server to https. request.is_secure reads + # X-Forwarded-Proto only when ProxyFix is enabled (TRUST_PROXY=1), which is + # exactly the deployment where the proxy terminates TLS. + if request.is_secure: + response.headers['Strict-Transport-Security'] = 'max-age=31536000; includeSubDomains' + return response diff --git a/tests/test_routes.py b/tests/test_routes.py index 161b93c..2d0a620 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -4,6 +4,7 @@ import io import json import logging +import re from unittest.mock import MagicMock, patch @@ -423,6 +424,54 @@ def test_referrer_policy(self, client): response = client.get('/') assert 'strict-origin' in response.headers['Referrer-Policy'] + def test_permissions_policy(self, client): + response = client.get('/') + policy = response.headers['Permissions-Policy'] + assert 'camera=()' in policy + assert 'microphone=()' in policy + assert 'geolocation=()' in policy + + def test_cross_origin_headers(self, client): + response = client.get('/') + assert response.headers['Cross-Origin-Opener-Policy'] == 'same-origin' + assert response.headers['Cross-Origin-Resource-Policy'] == 'same-origin' + + def test_csp_hardening_directives(self, client): + directives = { + part.strip() for part in client.get('/').headers['Content-Security-Policy'].split(';') + } + assert "object-src 'none'" in directives + assert "base-uri 'self'" in directives + assert "frame-ancestors 'none'" in directives + assert "form-action 'self'" in directives + assert 'upgrade-insecure-requests' in directives + + def test_csp_still_allows_google_fonts_and_data_images(self, client): + csp = client.get('/').headers['Content-Security-Policy'] + assert 'https://fonts.googleapis.com' in csp + assert 'https://fonts.gstatic.com' in csp + assert 'data:' in csp + + def test_csp_has_no_malformed_directive(self, client): + """A missing separator would fuse two directives into one token.""" + csp = client.get('/').headers['Content-Security-Policy'] + parts = [part.strip() for part in csp.split(';') if part.strip()] + assert len(parts) == len(set(parts)) + for part in parts: + assert not part.startswith("'") + # Each directive starts with a bare directive name. + assert re.match(r'^[a-z-]+( |$)', part), part + + def test_no_hsts_on_plain_http(self, client): + response = client.get('/') + assert 'Strict-Transport-Security' not in response.headers + + def test_hsts_on_secure_request(self, client): + response = client.get('/', base_url='https://localhost') + assert response.headers['Strict-Transport-Security'] == ( + 'max-age=31536000; includeSubDomains' + ) + class TestFormulaInjection: """F1 - CSV/XLSX formula injection (CWE-1236).""" From 773e88638cdfd928d6f59b154d3c850ac9a71650 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:29:32 +0000 Subject: [PATCH 11/36] Phase 1.6: fail fast on the dev SECRET_KEY in production; validate int config F7. config.is_production() is the single canonical production signal, reading only APP_ENV=production. It is deliberately not inferred from `not DEBUG` -- the documented local run `python app.py` has DEBUG False, so that would block ordinary development -- and no second spelling is accepted, because two names let a deployment satisfy one gate and silently miss another (pass the SECRET_KEY check with SESSION_COOKIE_SECURE still off). F16 and the 2.10 topology guard will call the same helper. create_app now raises when APP_ENV=production and SECRET_KEY is unset, empty, or still the publicly known dev default. Empty string is a distinct branch from unset and is the one a misconfigured secrets manager actually produces. env_int() replaces bare int(os.environ.get(...)) so a typo reports "Environment variable PREVIEW_ROW_LIMIT must be an integer, got 'abc'" instead of a ValueError traceback from inside the import. Tests cover every branch F7 names, including that PRODUCTION=true alone is NOT honored as a production signal. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- app.py | 23 ++++++++++- config.py | 52 +++++++++++++++++++++--- tests/test_routes.py | 96 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+), 7 deletions(-) diff --git a/app.py b/app.py index fb84064..56c4e18 100644 --- a/app.py +++ b/app.py @@ -2,16 +2,37 @@ from flask import Flask -from config import Config +from config import DEV_SECRET_KEY, Config, is_production from extensions import csrf, limiter from security import apply_security_headers +def _assert_production_secret_key(app): + """ + Refuse to start a production deployment on the publicly known dev key (F7). + + Without this the app runs happily with a key anyone can read out of the + repository, which makes CSRF tokens forgeable and the session cookie + signable. render.yaml generates a key, but the Docker and self-hosted paths + in the README leave it to the operator. + """ + if not is_production(): + return + secret = app.config.get('SECRET_KEY') + if not secret or secret == DEV_SECRET_KEY: + raise RuntimeError( + 'SECRET_KEY must be set to a random value when APP_ENV=production; ' + 'the built-in development key is public.' + ) + + def create_app(config_class=Config): """Create and configure the Flask application.""" app = Flask(__name__) app.config.from_object(config_class) + _assert_production_secret_key(app) + # Initialize extensions csrf.init_app(app) limiter.init_app(app) diff --git a/config.py b/config.py index 38c34a1..f36fb72 100644 --- a/config.py +++ b/config.py @@ -2,20 +2,60 @@ import os +# Publicly known, and therefore only ever acceptable outside production. +DEV_SECRET_KEY = 'dev-secret-key-change-in-production' + + +def is_production(): + """ + True when this process is running as a production deployment. + + `APP_ENV=production` is the single canonical signal, checked through this one + helper by the SECRET_KEY fail-fast (F7), the Secure cookie flag (F16) and the + rate-limit topology guard (2.10). + + Two things it deliberately is not: + + - It is never inferred from `not DEBUG`. The documented local run + `python app.py` has DEBUG False by default, so that would block ordinary + development startup. + - No second spelling (`PRODUCTION=true`, `ENV=prod`, ...) is accepted. 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. + """ + return os.environ.get('APP_ENV', '').strip().lower() == 'production' + + +def env_int(name, default): + """ + Read an integer setting, failing with a message that names the variable. + + Plain int(os.environ.get(...)) raises a bare ValueError from deep inside the + import, which tells an operator nothing about which variable they mistyped + (F7). + """ + raw = os.environ.get(name) + if raw is None or raw.strip() == '': + return default + try: + return int(raw.strip()) + except ValueError: + raise RuntimeError(f'Environment variable {name} must be an integer, got {raw!r}') from None + class Config: """Flask configuration with env var overrides.""" - SECRET_KEY = os.environ.get('SECRET_KEY', 'dev-secret-key-change-in-production') + SECRET_KEY = os.environ.get('SECRET_KEY', DEV_SECRET_KEY) # Upload and payload limits - MAX_CONTENT_LENGTH = int(os.environ.get('MAX_UPLOAD_SIZE', 10 * 1024 * 1024)) + MAX_CONTENT_LENGTH = env_int('MAX_UPLOAD_SIZE', 10 * 1024 * 1024) # Preview and processing - PREVIEW_ROW_LIMIT = int(os.environ.get('PREVIEW_ROW_LIMIT', 25)) - API_FETCH_TIMEOUT = int(os.environ.get('API_FETCH_TIMEOUT', 30)) - API_FETCH_MAX_RESPONSE = int(os.environ.get('API_FETCH_MAX_RESPONSE', 10 * 1024 * 1024)) - FLATTEN_MAX_DEPTH = int(os.environ.get('FLATTEN_MAX_DEPTH', 10)) + PREVIEW_ROW_LIMIT = env_int('PREVIEW_ROW_LIMIT', 25) + API_FETCH_TIMEOUT = env_int('API_FETCH_TIMEOUT', 30) + API_FETCH_MAX_RESPONSE = env_int('API_FETCH_MAX_RESPONSE', 10 * 1024 * 1024) + FLATTEN_MAX_DEPTH = env_int('FLATTEN_MAX_DEPTH', 10) # Rate limiting (Flask-Limiter reads RATELIMIT_* keys automatically) RATELIMIT_STORAGE_URI = 'memory://' diff --git a/tests/test_routes.py b/tests/test_routes.py index 2d0a620..7b50508 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -1,12 +1,18 @@ """Tests for Flask routes.""" import csv +import importlib import io import json import logging import re from unittest.mock import MagicMock, patch +import pytest + +import config as config_module +from app import create_app + class TestIndexRoute: def test_returns_200(self, client): @@ -750,3 +756,93 @@ def test_default_header_still_works(self, mock_validate, mock_get, client): assert json.loads(response.data)['success'] is True _, kwargs = mock_get.call_args assert kwargs['headers'] == {'X-API-Key': 'secret'} + + +@pytest.fixture +def fresh_config(monkeypatch): + """ + Rebuild config.Config from the current environment. + + Config holds class attributes evaluated at import time, so a monkeypatched + env var only takes effect after a reload. F7 rules out constructing + Config(...) -- it is a class, not a constructor. + """ + + def build(**env): + for name, value in env.items(): + if value is None: + monkeypatch.delenv(name, raising=False) + else: + monkeypatch.setenv(name, value) + return importlib.reload(config_module).Config + + yield build + # Leave the module holding the pristine values for every later test. + monkeypatch.undo() + importlib.reload(config_module) + + +class TestProductionSecretKey: + """F7 - the dev SECRET_KEY must not survive into production.""" + + def test_production_with_default_key_refuses_to_start(self, fresh_config): + cfg = fresh_config(APP_ENV='production', SECRET_KEY=None) + with pytest.raises(RuntimeError, match='SECRET_KEY must be set'): + create_app(cfg) + + def test_production_with_empty_key_refuses_to_start(self, fresh_config): + # A misconfigured secrets manager produces '' rather than "unset". + cfg = fresh_config(APP_ENV='production', SECRET_KEY='') + with pytest.raises(RuntimeError, match='SECRET_KEY must be set'): + create_app(cfg) + + def test_production_with_real_key_starts(self, fresh_config): + cfg = fresh_config(APP_ENV='production', SECRET_KEY='a-real-random-value') + app = create_app(cfg) + assert app.config['SECRET_KEY'] == 'a-real-random-value' + + def test_local_run_with_default_key_starts(self, fresh_config): + # `python app.py` has DEBUG False, so gating on `not DEBUG` would have + # blocked the documented local run. + cfg = fresh_config(APP_ENV=None, SECRET_KEY=None, FLASK_DEBUG=None) + assert cfg.DEBUG is False + app = create_app(cfg) + assert app.config['SECRET_KEY'] == config_module.DEV_SECRET_KEY + + def test_second_spelling_is_not_a_production_signal(self, fresh_config): + # Accepting PRODUCTION=true as well would let a deployment pass this gate + # while SESSION_COOKIE_SECURE (F16) stayed off. + cfg = fresh_config(APP_ENV=None, PRODUCTION='true', SECRET_KEY=None) + assert config_module.is_production() is False + create_app(cfg) + + def test_app_env_is_case_and_space_insensitive(self, fresh_config): + fresh_config(APP_ENV=' Production ') + assert config_module.is_production() is True + + +class TestIntegerConfigValidation: + """F7 - a mistyped integer setting must name the variable, not raise ValueError.""" + + INT_SETTINGS = [ + 'MAX_UPLOAD_SIZE', + 'PREVIEW_ROW_LIMIT', + 'API_FETCH_TIMEOUT', + 'API_FETCH_MAX_RESPONSE', + 'FLATTEN_MAX_DEPTH', + ] + + def test_each_integer_setting_reports_a_clear_error(self, fresh_config): + for name in self.INT_SETTINGS: + with pytest.raises(RuntimeError, match=f'{name} must be an integer'): + fresh_config(**{name: 'abc'}) + # Undo before the next iteration so errors do not stack. + fresh_config(**{name: None}) + + def test_blank_value_falls_back_to_the_default(self, fresh_config): + cfg = fresh_config(PREVIEW_ROW_LIMIT=' ') + assert cfg.PREVIEW_ROW_LIMIT == 25 + + def test_valid_value_is_applied(self, fresh_config): + cfg = fresh_config(PREVIEW_ROW_LIMIT=' 7 ') + assert cfg.PREVIEW_ROW_LIMIT == 7 From b03c5a93acb4a9c73e5bf2dbcbc00acfb709f0f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:30:49 +0000 Subject: [PATCH 12/36] Phase 1.7: bound recursion depth in extract_table_data and handle RecursionError F8. flatten_for_csv had a max_depth cap but extract_table_data did not, so a valid 1500-level-deep document -- well within the 10 MB request cap -- exhausted the Python stack and returned 500. The descent into nested dicts now mirrors flatten_for_csv's cap and yields a single row at the limit; routes passes FLATTEN_MAX_DEPTH through. find_candidate_arrays needed the same guard but was deleted in Phase 0.8 (D2), so nothing here covers it. The depth guard alone is not enough: CPython's own json parser raises RecursionError before any helper runs, and the response encoder can raise it on the raw_json tree-picker payload. process_json therefore catches RecursionError ahead of the generic handler and returns 400 'JSON nesting too deep' with no traceback logged -- it is the caller's document that is malformed, not the server. Tests: 1500-deep payloads through all three input methods, plus helper tests built iteratively so the test itself cannot recurse. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- helpers.py | 12 ++++++++-- routes.py | 10 +++++++- tests/test_helpers.py | 29 +++++++++++++++++++++++ tests/test_routes.py | 54 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 3 deletions(-) diff --git a/helpers.py b/helpers.py index c1b8c13..31ab202 100644 --- a/helpers.py +++ b/helpers.py @@ -72,12 +72,20 @@ def sanitize_cell(value): return serialized -def extract_table_data(json_data): +def extract_table_data(json_data, _depth=0, max_depth=10): """ Extract tabular data from JSON. Handles arrays of objects, nested arrays, and single objects. Returns a list of row dicts. + + Mirrors flatten_for_csv's depth cap (F8): a payload nested a thousand levels + deep is valid JSON, and without the cap the descent into nested dicts blows + the Python stack and turns a client-supplied document into a 500. At the cap + the remaining structure becomes a single row rather than being explored. """ + if _depth >= max_depth: + return [json_data] if isinstance(json_data, dict) else [] + if isinstance(json_data, list): if len(json_data) > 0 and isinstance(json_data[0], dict): return json_data @@ -94,7 +102,7 @@ def extract_table_data(json_data): for value in json_data.values(): if isinstance(value, dict): - result = extract_table_data(value) + result = extract_table_data(value, _depth=_depth + 1, max_depth=max_depth) if result: return result diff --git a/routes.py b/routes.py index 3ab93dd..3157558 100644 --- a/routes.py +++ b/routes.py @@ -204,7 +204,9 @@ def process_json(): if selected is None: return jsonify({'error': f'Path "{json_path}" not found'}), 400 if isinstance(selected, list): - table_data = extract_table_data(selected) + table_data = extract_table_data( + selected, max_depth=current_app.config['FLATTEN_MAX_DEPTH'] + ) elif isinstance(selected, dict): table_data = [selected] else: @@ -237,6 +239,12 @@ def process_json(): } ) + except RecursionError: + # Valid JSON can nest deeply enough to exhaust the C stack, in json.loads + # itself, in the helpers, or in the response encoder. That is the caller's + # document, so it is a 400 -- and it is not worth an exception traceback + # in the logs (F8). + return jsonify({'error': 'JSON nesting too deep'}), 400 except Exception: logger.exception('Unexpected error in process_json') return jsonify({'error': 'An internal error occurred'}), 500 diff --git a/tests/test_helpers.py b/tests/test_helpers.py index 945baaa..51867cf 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -147,3 +147,32 @@ def test_empty(self): def test_non_dict_rows_ignored(self): data = [{'a': 1}, 'not a dict', {'b': 2}] assert get_all_columns(data) == ['a', 'b'] + + +class TestExtractTableDataDepthGuard: + """F8 - extract_table_data must not recurse without a bound.""" + + @staticmethod + def _nest(depth): + """Build a depth-N chain of dicts iteratively (no recursion in the test).""" + node = {'leaf': 'value'} + for _ in range(depth): + node = {'a': node} + return node + + def test_deep_nesting_does_not_raise(self): + result = extract_table_data(self._nest(1500)) + assert isinstance(result, list) + assert len(result) == 1 + + def test_stops_at_max_depth(self): + # With max_depth=3 the descent stops before reaching the array. + data = {'a': {'b': {'c': {'d': [{'x': 1}]}}}} + assert extract_table_data(data, max_depth=3) == [{'d': [{'x': 1}]}] + + def test_shallow_data_is_unaffected_by_the_guard(self): + data = {'data': {'users': [{'name': 'Alice'}]}} + assert extract_table_data(data) == [{'name': 'Alice'}] + + def test_non_dict_at_the_cap_yields_no_rows(self): + assert extract_table_data('scalar', _depth=99, max_depth=10) == [] diff --git a/tests/test_routes.py b/tests/test_routes.py index 7b50508..1a76b6d 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -846,3 +846,57 @@ def test_blank_value_falls_back_to_the_default(self, fresh_config): def test_valid_value_is_applied(self, fresh_config): cfg = fresh_config(PREVIEW_ROW_LIMIT=' 7 ') assert cfg.PREVIEW_ROW_LIMIT == 7 + + +class TestRecursionDepth: + """F8 - a pathologically nested document is a 400, never a 500.""" + + @staticmethod + def _deep_json(depth): + return '{"a":' * depth + '1' + '}' * depth + + def test_deeply_nested_paste_returns_400(self, client, caplog): + with caplog.at_level(logging.DEBUG): + response = client.post( + '/process', + data={'input_method': 'paste', 'pasted_json': self._deep_json(1500)}, + ) + assert response.status_code == 400 + assert json.loads(response.data)['error'] == 'JSON nesting too deep' + assert not [r for r in caplog.records if r.levelno >= logging.ERROR] + + def test_deeply_nested_upload_returns_400(self, client): + response = client.post( + '/process', + data={ + 'input_method': 'file', + 'json_file': (io.BytesIO(self._deep_json(1500).encode()), 'deep.json'), + }, + content_type='multipart/form-data', + ) + assert response.status_code == 400 + assert json.loads(response.data)['error'] == 'JSON nesting too deep' + + @patch('routes.requests.get') + @patch('routes.validate_url') + def test_deeply_nested_api_response_returns_400(self, mock_validate, mock_get, client): + mock_validate.return_value = (True, None) + mock_resp = MagicMock() + mock_resp.status_code = 200 + mock_resp.iter_content.return_value = [self._deep_json(1500).encode()] + mock_resp.raise_for_status.return_value = None + mock_get.return_value = mock_resp + + response = client.post( + '/process', + data={'input_method': 'api', 'api_url': 'https://api.example.com/data'}, + ) + assert response.status_code == 400 + + def test_moderately_nested_document_still_works(self, client): + payload = json.dumps({'rows': [{'id': 1}, {'id': 2}]}) + response = client.post( + '/process', + data={'input_method': 'paste', 'pasted_json': payload, 'json_path': 'rows'}, + ) + assert json.loads(response.data)['total_rows'] == 2 From 06516510a546e7728f6765114b69a3c0702aab38 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:33:18 +0000 Subject: [PATCH 13/36] Phase 1.8: run DNS on a shared bounded pool with admission control F6.1 / P7. socket.getaddrinfo had no timeout and cannot be cancelled, so a hostname served by a slow nameserver pinned the gunicorn worker that called it. Lookups now run on a shared, fixed-size ThreadPoolExecutor and the caller waits with API_DNS_TIMEOUT (default 3s). Three load-bearing details, because a timed-out lookup keeps running: - Permit ownership. The admission permit is taken BEFORE submit and released from the future's done-callback, never from the caller's finally. Releasing on caller timeout would re-admit work while the blocked getaddrinfo thread still occupies the pool -- exactly how the pool saturates under repeated slow-DNS requests. Capacity is the pool size plus an equal backlog; past that, callers get a fast "DNS resolver is busy" rather than being queued without limit. - Lifecycle across fork. The pool is built lazily on first use inside the worker and records its pid, so a pool inherited from the gunicorn master is replaced rather than reused. - Teardown is NOT bounded, and v1.2 does not claim otherwise. cancel_futures only drops queued work; a running getaddrinfo keeps going until the platform resolver returns (glibc: ~5s per nameserver x 2 attempts x every nameserver in resolv.conf, so tens of seconds is the realistic worst case). Pinning `options timeout:2 attempts:1` in the container's resolv.conf is a best-effort narrowing; a killable subprocess resolver is the only real bound and stays out of scope. Tests assert what the code enforces -- the caller's wait, the admission limit, that permits return to zero once the lookups finish, and that threads never exceed max_workers -- plus a lifecycle test that pins the accepted teardown behavior by showing the worker thread still alive after shutdown until the mocked lookup is explicitly released. No test asserts a wall-clock bound on teardown. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- config.py | 8 +++ security.py | 158 ++++++++++++++++++++++++++++++++++++++++- tests/test_routes.py | 3 + tests/test_security.py | 141 ++++++++++++++++++++++++++++++++++++ 4 files changed, 308 insertions(+), 2 deletions(-) diff --git a/config.py b/config.py index f36fb72..0a7265c 100644 --- a/config.py +++ b/config.py @@ -57,6 +57,14 @@ class Config: API_FETCH_MAX_RESPONSE = env_int('API_FETCH_MAX_RESPONSE', 10 * 1024 * 1024) FLATTEN_MAX_DEPTH = env_int('FLATTEN_MAX_DEPTH', 10) + # DNS admission control for API fetch (F6.1/P7). API_DNS_TIMEOUT bounds how + # long a REQUEST waits, not how long the lookup runs -- getaddrinfo exposes no + # timeout and cannot be cancelled. API_DNS_MAX_WORKERS bounds concurrency, + # which is the actual worker-starvation fix. + API_DNS_TIMEOUT = env_int('API_DNS_TIMEOUT', 3) + API_DNS_MAX_WORKERS = env_int('API_DNS_MAX_WORKERS', 4) + API_DNS_ADMISSION_TIMEOUT = env_int('API_DNS_ADMISSION_TIMEOUT', 1) + # Rate limiting (Flask-Limiter reads RATELIMIT_* keys automatically) RATELIMIT_STORAGE_URI = 'memory://' RATELIMIT_DEFAULT = os.environ.get('RATE_LIMIT_DEFAULT', '120/minute') diff --git a/security.py b/security.py index 3a49dd7..db7fcb1 100644 --- a/security.py +++ b/security.py @@ -1,10 +1,13 @@ """Security utilities: SSRF protection and response headers.""" +import concurrent.futures import ipaddress +import os import socket +import threading from urllib.parse import urlparse -from flask import request +from flask import current_app, has_app_context, request # Built as a list of directives so appending can never fuse two tokens into one # malformed directive (F5). Google Fonts is the only third-party origin the page @@ -29,6 +32,151 @@ CONTENT_SECURITY_POLICY = '; '.join(CSP_DIRECTIVES) +# --- Bounded DNS admission (F6.1 / P7) -------------------------------------- +# +# socket.getaddrinfo takes no timeout and cannot be cancelled, so a hostname +# served by a slow or unresponsive nameserver pins the gunicorn worker that +# called it for as long as the platform resolver takes. The lookup therefore runs +# on a shared, fixed-size pool and the caller waits with a timeout. +# +# What this bounds and what it does not: +# +# * Bounded: how long a REQUEST waits (Future.result timeout), and how many +# lookups may be in flight at once (the admission permits). Concurrency is +# the actual worker-starvation fix. +# * NOT bounded: the lookup itself, and therefore worker teardown. 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. The wait is whatever the platform resolver takes -- glibc +# defaults to ~5s per nameserver x 2 attempts x every nameserver in +# resolv.conf, so tens of seconds is the realistic worst case, and it is +# bounded at all only where `options timeout:N attempts:M` is configured. +# Pinning `options timeout:2 attempts:1` in the container's resolv.conf is a +# best-effort narrowing, not a guarantee. A killable subprocess resolver is +# the only real bound and is deliberately out of v1.2 scope. +# +# Do not describe teardown as bounded anywhere. + +DEFAULT_DNS_TIMEOUT = 3 +DEFAULT_DNS_MAX_WORKERS = 4 +DEFAULT_DNS_ADMISSION_TIMEOUT = 1 + + +class ResolverBusyError(Exception): + """No admission permit was free within the admission wait.""" + + +class _ResolverPool: + """A fixed-size resolver pool with an admission permit per in-flight lookup.""" + + def __init__(self, max_workers): + self.pid = os.getpid() + self.max_workers = max_workers + # Pool size plus an equal backlog: a bounded submission queue. Beyond this + # callers are rejected rather than queued without limit. + self.capacity = max_workers * 2 + self.executor = concurrent.futures.ThreadPoolExecutor( + max_workers=max_workers, thread_name_prefix='dns-resolver' + ) + self._permits = threading.Semaphore(self.capacity) + self._counter_lock = threading.Lock() + self.in_flight = 0 + + def submit(self, hostname, admission_timeout): + """ + Admit and start one lookup, or raise ResolverBusyError. + + The permit is taken BEFORE submit and released from the future's + done-callback -- never from the caller's finally. Releasing on caller + timeout would re-admit work while the blocked getaddrinfo thread still + occupies the pool, which is precisely how the pool saturates under + repeated slow-DNS requests. + """ + if not self._permits.acquire(timeout=admission_timeout): + raise ResolverBusyError + with self._counter_lock: + self.in_flight += 1 + try: + future = self.executor.submit(socket.getaddrinfo, hostname, None) + except BaseException: + self._release() + raise + future.add_done_callback(self._on_done) + return future + + def _on_done(self, _future): + self._release() + + def _release(self): + with self._counter_lock: + self.in_flight -= 1 + self._permits.release() + + +_pool_lock = threading.Lock() +_pool = None + + +def get_resolver_pool(max_workers=None): + """ + Return this process's resolver pool, creating it on first use. + + Creation is lazy so the pool belongs to the gunicorn WORKER, not the master: + an executor built at import time in the master leaves its threads behind in + the parent and is not usefully inherited. The recorded pid also makes a pool + inherited across a fork be replaced rather than reused. + """ + global _pool + if max_workers is None: + max_workers = _setting('API_DNS_MAX_WORKERS', DEFAULT_DNS_MAX_WORKERS) + pool = _pool + if pool is not None and pool.pid == os.getpid(): + return pool + with _pool_lock: + if _pool is None or _pool.pid != os.getpid(): + _pool = _ResolverPool(max_workers) + return _pool + + +def reset_resolver_pool(): + """ + Drop the current pool so the next lookup builds a fresh one. + + shutdown(wait=False, cancel_futures=True) returns immediately but does NOT + make teardown bounded: it can only drop queued work, and any thread already + inside getaddrinfo keeps running until the platform resolver returns. + """ + global _pool + with _pool_lock: + pool = _pool + _pool = None + if pool is not None: + pool.executor.shutdown(wait=False, cancel_futures=True) + return pool + + +def _setting(name, default): + """Read a config value, falling back to the module default outside a request.""" + if has_app_context(): + return current_app.config.get(name, default) + return default + + +def resolve_hostname(hostname): + """ + Resolve a hostname under admission control. + + Returns getaddrinfo's result, or raises ResolverBusyError (no permit), + TimeoutError (the caller's wait elapsed; the lookup itself keeps running) or + socket.gaierror. + """ + pool = get_resolver_pool() + admission_timeout = _setting('API_DNS_ADMISSION_TIMEOUT', DEFAULT_DNS_ADMISSION_TIMEOUT) + timeout = _setting('API_DNS_TIMEOUT', DEFAULT_DNS_TIMEOUT) + future = pool.submit(hostname, admission_timeout) + return future.result(timeout=timeout) + + def validate_url(url): """ Validate a URL for SSRF protection. @@ -45,9 +193,15 @@ def validate_url(url): return False, 'Invalid URL: no hostname' try: - addr_infos = socket.getaddrinfo(hostname, None) + addr_infos = resolve_hostname(hostname) + except ResolverBusyError: + return False, 'DNS resolver is busy; please retry' except socket.gaierror: return False, f'Could not resolve hostname: {hostname}' + except TimeoutError: + # The caller's wait elapsed. The lookup is still running on the pool and + # still holds its permit until it finishes -- that is deliberate. + return False, f'Could not resolve hostname: {hostname}' found_valid = False for addr_info in addr_infos: diff --git a/tests/test_routes.py b/tests/test_routes.py index 1a76b6d..1fb61c1 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -830,6 +830,9 @@ class TestIntegerConfigValidation: 'API_FETCH_TIMEOUT', 'API_FETCH_MAX_RESPONSE', 'FLATTEN_MAX_DEPTH', + 'API_DNS_TIMEOUT', + 'API_DNS_MAX_WORKERS', + 'API_DNS_ADMISSION_TIMEOUT', ] def test_each_integer_setting_reports_a_clear_error(self, fresh_config): diff --git a/tests/test_security.py b/tests/test_security.py index 4040178..716a226 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -1,7 +1,13 @@ """Tests for security utilities.""" +import os +import threading +import time from unittest.mock import patch +import pytest + +import security from security import validate_url @@ -109,3 +115,138 @@ def test_blocks_ipv6_loopback(self): mock_dns.return_value = [(10, 1, 6, '', ('::1', 0, 0, 0))] is_valid, error = validate_url('http://localhost6.example.com') assert not is_valid + + +class TestBoundedDnsAdmission: + """ + F6.1 / P7 - DNS runs on a shared bounded pool. + + What is asserted here is what the code actually enforces: the CALLER's wait + and the ADMISSION limit. Nothing asserts a bound on the lookup itself or on + teardown, because nothing here enforces one -- getaddrinfo exposes no timeout + and cannot be cancelled. + """ + + RESOLVED = [(2, 1, 6, '', ('93.184.216.34', 0))] + + @pytest.fixture + def blocking_dns(self, monkeypatch): + """A getaddrinfo that hangs until the test releases it.""" + release = threading.Event() + + def slow_getaddrinfo(*args, **kwargs): + if not release.wait(30): + raise AssertionError('test never released the mocked lookup') + return self.RESOLVED + + monkeypatch.setattr(security.socket, 'getaddrinfo', slow_getaddrinfo) + monkeypatch.setattr(security, 'DEFAULT_DNS_TIMEOUT', 0.2) + monkeypatch.setattr(security, 'DEFAULT_DNS_MAX_WORKERS', 2) + monkeypatch.setattr(security, 'DEFAULT_DNS_ADMISSION_TIMEOUT', 0.1) + security.reset_resolver_pool() + + yield release + + release.set() + security.reset_resolver_pool() + + @staticmethod + def _wait_for(predicate, timeout=10): + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return True + time.sleep(0.01) + return False + + def test_caller_wait_is_bounded_and_permits_are_not_leaked(self, blocking_dns): + pool = security.get_resolver_pool() + assert pool.capacity == 4 # max_workers 2 + an equal backlog + + # Fill the pool. Every caller must come back on its own timeout even + # though the lookups behind them are still running. + for _ in range(pool.capacity): + started = time.monotonic() + is_valid, error = validate_url('http://slow.example.com') + elapsed = time.monotonic() - started + assert not is_valid + assert 'resolve' in error.lower() + assert elapsed < 5, f'caller waited {elapsed:.2f}s, not the configured bound' + + # The permits stay held: the lookups are still running. Releasing on + # caller timeout would re-admit work into an already-blocked pool. + assert pool.in_flight == pool.capacity + + # No unbounded thread growth: the pool never exceeds max_workers threads. + assert len(pool.executor._threads) <= pool.max_workers + + # Once the lookups finish, every permit comes back. + blocking_dns.set() + assert self._wait_for(lambda: pool.in_flight == 0), ( + f'{pool.in_flight} permits leaked after the lookups finished' + ) + + def test_saturation_returns_the_admission_error_instead_of_blocking(self, blocking_dns): + pool = security.get_resolver_pool() + for _ in range(pool.capacity): + validate_url('http://slow.example.com') + assert pool.in_flight == pool.capacity + + started = time.monotonic() + is_valid, error = validate_url('http://another.example.com') + elapsed = time.monotonic() - started + + assert not is_valid + assert 'busy' in error.lower() + # Rejected on the short admission wait, not queued behind the lookups. + assert elapsed < 1 + + def test_repeated_timeouts_do_not_grow_the_pool(self, blocking_dns): + pool = security.get_resolver_pool() + for _ in range(12): + validate_url('http://slow.example.com') + assert len(pool.executor._threads) <= pool.max_workers + assert pool.in_flight <= pool.capacity + + def test_pool_is_rebuilt_after_a_fork(self, monkeypatch): + monkeypatch.setattr(security.socket, 'getaddrinfo', lambda *a, **k: self.RESOLVED) + security.reset_resolver_pool() + try: + parent_pool = security.get_resolver_pool() + assert security.get_resolver_pool() is parent_pool + + # Simulate the worker being forked from a process that already built + # a pool: the recorded pid no longer matches, so it is replaced. + parent_pool.pid = parent_pool.pid + 1 + child_pool = security.get_resolver_pool() + assert child_pool is not parent_pool + assert child_pool.pid == os.getpid() + + assert validate_url('https://example.com')[0] is True + finally: + security.reset_resolver_pool() + + def test_teardown_is_not_bounded_by_anything_this_code_enforces(self, blocking_dns): + """ + Pins the ACCEPTED behavior, not a bound. + + shutdown(wait=False, cancel_futures=True) returns at once but cannot + cancel a running getaddrinfo, so the worker thread stays alive until the + platform resolver returns. Asserting a wall-clock bound here would be + asserting something the code does not enforce. + """ + validate_url('http://slow.example.com') + pool = security.get_resolver_pool() + threads = list(pool.executor._threads) + assert threads + + security.reset_resolver_pool() + + # Still running well past API_DNS_TIMEOUT: that is the documented exposure. + time.sleep(0.5) + assert any(thread.is_alive() for thread in threads) + + blocking_dns.set() + for thread in threads: + thread.join(timeout=10) + assert not any(thread.is_alive() for thread in threads) From ed60332a345f84492e965686957faf1f5dd1937e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:34:28 +0000 Subject: [PATCH 14/36] Phase 1.9: restrict API fetch to an allowlist of ports F6.2 / D5. Only the resolved IP was checked, so http://public.example.com:22 or :6379 passed validation and the tool would connect to any port on any public host. API_ALLOWED_PORTS defaults to 80,443,8443; the implicit port for the scheme is used when the URL omits one. The check runs before DNS so a rejected URL costs no lookup, and a malformed port (urlparse raises rather than returning one) is a 400 instead of an unhandled ValueError. An empty allowlist disables the check for operators who need it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- config.py | 18 +++++++++++++++ security.py | 17 ++++++++++++++ tests/test_routes.py | 9 ++++++++ tests/test_security.py | 50 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+) diff --git a/config.py b/config.py index 0a7265c..0814176 100644 --- a/config.py +++ b/config.py @@ -43,6 +43,19 @@ def env_int(name, default): raise RuntimeError(f'Environment variable {name} must be an integer, got {raw!r}') from None +def env_int_set(name, default): + """Read a comma-separated integer list (e.g. an allowlist of ports).""" + raw = os.environ.get(name) + if raw is None or raw.strip() == '': + raw = default + try: + return frozenset(int(part.strip()) for part in raw.split(',') if part.strip()) + except ValueError: + raise RuntimeError( + f'Environment variable {name} must be a comma-separated list of integers, got {raw!r}' + ) from None + + class Config: """Flask configuration with env var overrides.""" @@ -65,6 +78,11 @@ class Config: API_DNS_MAX_WORKERS = env_int('API_DNS_MAX_WORKERS', 4) API_DNS_ADMISSION_TIMEOUT = env_int('API_DNS_ADMISSION_TIMEOUT', 1) + # Ports the API-fetch feature may connect to (F6.2/D5). Only the IP was + # checked before, so http://public.example.com:22 or :6379 passed. An empty + # value disables the check. + API_ALLOWED_PORTS = env_int_set('API_ALLOWED_PORTS', '80,443,8443') + # Rate limiting (Flask-Limiter reads RATELIMIT_* keys automatically) RATELIMIT_STORAGE_URI = 'memory://' RATELIMIT_DEFAULT = os.environ.get('RATE_LIMIT_DEFAULT', '120/minute') diff --git a/security.py b/security.py index db7fcb1..62f1d79 100644 --- a/security.py +++ b/security.py @@ -57,6 +57,9 @@ # # Do not describe teardown as bounded anywhere. +DEFAULT_ALLOWED_PORTS = frozenset({80, 443, 8443}) +DEFAULT_SCHEME_PORTS = {'http': 80, 'https': 443} + DEFAULT_DNS_TIMEOUT = 3 DEFAULT_DNS_MAX_WORKERS = 4 DEFAULT_DNS_ADMISSION_TIMEOUT = 1 @@ -192,6 +195,20 @@ def validate_url(url): if not hostname: return False, 'Invalid URL: no hostname' + # Port check first: it is free, and a rejected URL should not cost a lookup. + try: + port = parsed.port + except ValueError: + return False, 'Invalid URL: malformed port' + if port is None: + port = DEFAULT_SCHEME_PORTS[parsed.scheme] + allowed_ports = _setting('API_ALLOWED_PORTS', DEFAULT_ALLOWED_PORTS) + if allowed_ports and port not in allowed_ports: + return False, ( + f'Port {port} is not allowed. Allowed ports: ' + + ', '.join(str(p) for p in sorted(allowed_ports)) + ) + try: addr_infos = resolve_hostname(hostname) except ResolverBusyError: diff --git a/tests/test_routes.py b/tests/test_routes.py index 1fb61c1..e9a3761 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -850,6 +850,15 @@ def test_valid_value_is_applied(self, fresh_config): cfg = fresh_config(PREVIEW_ROW_LIMIT=' 7 ') assert cfg.PREVIEW_ROW_LIMIT == 7 + def test_port_allowlist_reports_a_clear_error(self, fresh_config): + with pytest.raises(RuntimeError, match='API_ALLOWED_PORTS must be a comma-separated'): + fresh_config(API_ALLOWED_PORTS='80,https') + fresh_config(API_ALLOWED_PORTS=None) + + def test_port_allowlist_is_parsed(self, fresh_config): + cfg = fresh_config(API_ALLOWED_PORTS=' 80 , 8443 ') + assert sorted(cfg.API_ALLOWED_PORTS) == [80, 8443] + class TestRecursionDepth: """F8 - a pathologically nested document is a 400, never a 500.""" diff --git a/tests/test_security.py b/tests/test_security.py index 716a226..69a7619 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -250,3 +250,53 @@ def test_teardown_is_not_bounded_by_anything_this_code_enforces(self, blocking_d for thread in threads: thread.join(timeout=10) assert not any(thread.is_alive() for thread in threads) + + +class TestPortAllowlist: + """F6.2 / D5 - only 80, 443 and 8443 by default.""" + + RESOLVED = [(2, 1, 6, '', ('93.184.216.34', 0))] + + def test_blocks_non_http_ports(self): + with patch('security.socket.getaddrinfo') as mock_dns: + mock_dns.return_value = self.RESOLVED + for port in (22, 25, 6379, 3306, 5432, 11211, 8080): + is_valid, error = validate_url(f'http://public.example.com:{port}/x') + assert not is_valid, f'port {port} was accepted' + assert f'Port {port} is not allowed' in error + # A rejected port must not cost a DNS lookup. + assert mock_dns.call_count == 0 + + def test_allows_the_default_ports(self): + with patch('security.socket.getaddrinfo') as mock_dns: + mock_dns.return_value = self.RESOLVED + for url in ( + 'http://public.example.com:80/x', + 'https://public.example.com:443/x', + 'https://public.example.com:8443/x', + ): + assert validate_url(url)[0], url + + def test_implicit_scheme_port_is_used(self): + with patch('security.socket.getaddrinfo') as mock_dns: + mock_dns.return_value = self.RESOLVED + assert validate_url('http://public.example.com/x')[0] + assert validate_url('https://public.example.com/x')[0] + + def test_malformed_port_rejected(self): + is_valid, error = validate_url('http://public.example.com:notaport/x') + assert not is_valid + assert 'port' in error.lower() + + def test_allowlist_is_configurable(self, app): + with app.app_context(), patch('security.socket.getaddrinfo') as mock_dns: + mock_dns.return_value = self.RESOLVED + app.config['API_ALLOWED_PORTS'] = frozenset({8080}) + assert validate_url('http://public.example.com:8080/x')[0] + assert not validate_url('https://public.example.com/x')[0] + + def test_empty_allowlist_disables_the_check(self, app): + with app.app_context(), patch('security.socket.getaddrinfo') as mock_dns: + mock_dns.return_value = self.RESOLVED + app.config['API_ALLOWED_PORTS'] = frozenset() + assert validate_url('http://public.example.com:22/x')[0] From a36b6887e9bd93470694d26b495f1ecbe0dfb893 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:35:36 +0000 Subject: [PATCH 15/36] Phase 1.10: opt-in proxy-aware rate limiting F12 / D3. Behind Render's load balancer or an Nginx reverse proxy, request.remote_addr is the proxy's IP, so every user shared one rate-limit bucket and a single client could exhaust the whole site's /process quota. TRUST_PROXY=1 installs ProxyFix with x_for=1, x_proto=1, x_host=1 -- exactly one trusted hop, so a client that prepends its own X-Forwarded-For entry cannot pick its bucket. Off by default: with the variable unset the behavior is identical to v1.1 and forwarded headers are ignored entirely, because trusting them unconditionally is spoofable. The limiter now uses an explicit client_ip_key() that reads request.remote_addr and never the raw header, so the key is whatever ProxyFix decided rather than something the client can assert. x_proto also fixes request.is_secure behind a TLS-terminating proxy, which the HSTS header (1.5) and the Secure cookie flag (1.14) depend on. The MEMORY.md note on Redis storage for multi-instance deployments that 1.10 calls for lands with task 2.10 instead: config.py still hardcodes RATELIMIT_STORAGE_URI = 'memory://', so documenting the redis:// setup now would describe something the code cannot yet do. 2.10 is the task that makes storage configurable. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- app.py | 9 +++++ config.py | 4 +++ extensions.py | 17 +++++++-- tests/test_routes.py | 85 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 2 deletions(-) diff --git a/app.py b/app.py index 56c4e18..549b7c6 100644 --- a/app.py +++ b/app.py @@ -1,6 +1,7 @@ """JSON Table Converter - Flask application factory.""" from flask import Flask +from werkzeug.middleware.proxy_fix import ProxyFix from config import DEV_SECRET_KEY, Config, is_production from extensions import csrf, limiter @@ -33,6 +34,14 @@ def create_app(config_class=Config): _assert_production_secret_key(app) + if app.config.get('TRUST_PROXY'): + # Exactly one trusted hop. Behind Render's load balancer or an Nginx + # reverse proxy every request otherwise appears to come from the proxy + # IP, so all users share one rate-limit bucket and one client can exhaust + # the site's quota (F12). x_proto also makes request.is_secure correct, + # which the HSTS header (1.5) and the Secure cookie flag (1.14) rely on. + app.wsgi_app = ProxyFix(app.wsgi_app, x_for=1, x_proto=1, x_host=1) + # Initialize extensions csrf.init_app(app) limiter.init_app(app) diff --git a/config.py b/config.py index 0814176..72dc56f 100644 --- a/config.py +++ b/config.py @@ -83,6 +83,10 @@ class Config: # value disables the check. API_ALLOWED_PORTS = env_int_set('API_ALLOWED_PORTS', '80,443,8443') + # Trust X-Forwarded-* from exactly one proxy hop (F12/D3). Off by default: + # with it unset, behavior is identical to v1.1 and forged headers are ignored. + TRUST_PROXY = os.environ.get('TRUST_PROXY', '0').strip().lower() in ('1', 'true', 'yes') + # Rate limiting (Flask-Limiter reads RATELIMIT_* keys automatically) RATELIMIT_STORAGE_URI = 'memory://' RATELIMIT_DEFAULT = os.environ.get('RATE_LIMIT_DEFAULT', '120/minute') diff --git a/extensions.py b/extensions.py index 0da628a..afa6217 100644 --- a/extensions.py +++ b/extensions.py @@ -1,8 +1,21 @@ """Flask extensions (initialized without app, bound later via init_app).""" +from flask import request from flask_limiter import Limiter -from flask_limiter.util import get_remote_address from flask_wtf.csrf import CSRFProtect + +def client_ip_key(): + """ + Rate-limit bucket key: the client's IP. + + Deliberately reads request.remote_addr rather than the X-Forwarded-For + header. remote_addr is only rewritten from that header when ProxyFix is + installed, which create_app does exclusively under TRUST_PROXY=1 (F12/D3). + Reading the raw header here would let any client forge its own bucket. + """ + return request.remote_addr or 'unknown' + + csrf = CSRFProtect() -limiter = Limiter(key_func=get_remote_address) +limiter = Limiter(key_func=client_ip_key) diff --git a/tests/test_routes.py b/tests/test_routes.py index e9a3761..ac041c2 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -9,9 +9,11 @@ from unittest.mock import MagicMock, patch import pytest +from flask import request import config as config_module from app import create_app +from extensions import client_ip_key class TestIndexRoute: @@ -912,3 +914,86 @@ def test_moderately_nested_document_still_works(self, client): data={'input_method': 'paste', 'pasted_json': payload, 'json_path': 'rows'}, ) assert json.loads(response.data)['total_rows'] == 2 + + +class TestProxyAwareRateLimiting: + """F12 / D3 - X-Forwarded-For is honored only under TRUST_PROXY=1.""" + + @staticmethod + def _app_capturing_remote_addr(cfg): + app = create_app(cfg) + app.config['TESTING'] = True + app.config['WTF_CSRF_ENABLED'] = False + seen = {} + + @app.before_request + def _capture(): + seen['remote_addr'] = request.remote_addr + seen['key'] = client_ip_key() + seen['is_secure'] = request.is_secure + + return app, seen + + def test_forwarded_for_ignored_by_default(self, fresh_config): + cfg = fresh_config(TRUST_PROXY=None) + app, seen = self._app_capturing_remote_addr(cfg) + + app.test_client().get( + '/health', + headers={'X-Forwarded-For': '9.9.9.9', 'X-Forwarded-Proto': 'https'}, + environ_base={'REMOTE_ADDR': '10.0.0.5'}, + ) + + assert seen['remote_addr'] == '10.0.0.5' + assert seen['key'] == '10.0.0.5' + assert seen['is_secure'] is False + + def test_forwarded_for_used_when_trusted(self, fresh_config): + cfg = fresh_config(TRUST_PROXY='1') + app, seen = self._app_capturing_remote_addr(cfg) + + app.test_client().get( + '/health', + headers={'X-Forwarded-For': '9.9.9.9', 'X-Forwarded-Proto': 'https'}, + environ_base={'REMOTE_ADDR': '10.0.0.5'}, + ) + + assert seen['remote_addr'] == '9.9.9.9' + assert seen['key'] == '9.9.9.9' + # x_proto=1 also makes is_secure correct behind a TLS-terminating proxy, + # which HSTS (1.5) and the Secure cookie (1.14) depend on. + assert seen['is_secure'] is True + + def test_only_one_hop_is_trusted(self, fresh_config): + """ + A client that prepends its own hop must not choose its bucket. + + With x_for=1 ProxyFix takes the LAST entry -- the hop our single trusted + proxy actually appended -- so the forged leading entry is ignored. + """ + cfg = fresh_config(TRUST_PROXY='1') + app, seen = self._app_capturing_remote_addr(cfg) + + app.test_client().get( + '/health', + headers={'X-Forwarded-For': '1.1.1.1, 2.2.2.2, 3.3.3.3'}, + environ_base={'REMOTE_ADDR': '10.0.0.5'}, + ) + + assert seen['remote_addr'] == '3.3.3.3' + + def test_different_clients_get_different_buckets(self, fresh_config): + cfg = fresh_config(TRUST_PROXY='1') + app, seen = self._app_capturing_remote_addr(cfg) + client = app.test_client() + + keys = [] + for ip in ('9.9.9.9', '8.8.8.8'): + client.get( + '/health', + headers={'X-Forwarded-For': ip}, + environ_base={'REMOTE_ADDR': '10.0.0.5'}, + ) + keys.append(seen['key']) + + assert keys == ['9.9.9.9', '8.8.8.8'] From 7614f589c74a4517a7d0961199644edb809f9976 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:37:48 +0000 Subject: [PATCH 16/36] Phase 1.11-1.15: JSON error handlers, no-store, upload checks, cookies, health gate 1.11 (F10): 413, 500 and 404 now return {"error": ...} JSON instead of Flask's HTML pages, which made app.js's response.json() throw a SyntaxError and hide the real problem. The route handlers re-raise HTTPException rather than swallowing it into a 500 -- werkzeug raises RequestEntityTooLarge lazily, the first time the oversized body is read, which is inside the route. 1.12 (F11): Cache-Control: no-store on /process, /export-csv, /export-xlsx and /health. The index page and static assets stay cacheable, which P6 depends on. 1.13 (F13): the server now enforces what the file input's accept attribute only suggested. The extension check (.json/.jsonl) is authoritative; the content-type check is deliberately lenient about application/octet-stream and a missing type, because that is what browsers send for .jsonl -- a strict list would reject real uploads while adding nothing, since the content is parsed strictly either way. 1.14 (F16): SESSION_COOKIE_HTTPONLY, SESSION_COOKIE_SAMESITE='Lax' and SESSION_COOKIE_SECURE set explicitly. Flask emits no SameSite attribute by default and never sets Secure. Secure follows is_production(), not `not DEBUG`, so a local http run still works. 1.15 (F15): /health keeps returning version by default -- the existing test, AGENTS.md and CLAUDE.md all depend on it. HEALTH_REVEAL_VERSION=0 lets an operator opt out; the default is on, so there is no behavior change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- app.py | 27 ++++++- config.py | 17 +++++ routes.py | 53 +++++++++++++- security.py | 15 ++++ tests/test_routes.py | 168 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 278 insertions(+), 2 deletions(-) diff --git a/app.py b/app.py index 549b7c6..8857e53 100644 --- a/app.py +++ b/app.py @@ -1,6 +1,6 @@ """JSON Table Converter - Flask application factory.""" -from flask import Flask +from flask import Flask, jsonify from werkzeug.middleware.proxy_fix import ProxyFix from config import DEV_SECRET_KEY, Config, is_production @@ -27,6 +27,29 @@ def _assert_production_secret_key(app): ) +def _register_error_handlers(app): + """ + Keep every error response JSON (F10). + + Flask's built-in 413 and 500 pages are HTML, so app.js's response.json() + threw a SyntaxError on the body and surfaced a parse error instead of the + real problem. Every other error path in this app returns {"error": ...}. + """ + + @app.errorhandler(413) + def _request_entity_too_large(_error): + limit = app.config.get('MAX_CONTENT_LENGTH') or 0 + return jsonify({'error': f'Request too large (max {limit // (1024 * 1024)}MB)'}), 413 + + @app.errorhandler(500) + def _internal_server_error(_error): + return jsonify({'error': 'An internal error occurred'}), 500 + + @app.errorhandler(404) + def _not_found(_error): + return jsonify({'error': 'Not found'}), 404 + + def create_app(config_class=Config): """Create and configure the Flask application.""" app = Flask(__name__) @@ -49,6 +72,8 @@ def create_app(config_class=Config): # Security headers on every response app.after_request(apply_security_headers) + _register_error_handlers(app) + # Register routes from routes import bp diff --git a/config.py b/config.py index 72dc56f..fecf6d2 100644 --- a/config.py +++ b/config.py @@ -93,8 +93,25 @@ class Config: RATE_LIMIT_PROCESS = os.environ.get('RATE_LIMIT_PROCESS', '30/minute') RATE_LIMIT_EXPORT = os.environ.get('RATE_LIMIT_EXPORT', '60/minute') + # Cookie hardening (F16). Flask's defaults are HttpOnly=True but emit no + # SameSite attribute and never set Secure. Secure is tied to the explicit + # production signal: a plain local run has DEBUG False, so gating on + # `not DEBUG` would send Secure cookies over http and break CSRF-protected + # POSTs during development. + SESSION_COOKIE_HTTPONLY = True + SESSION_COOKIE_SAMESITE = 'Lax' + SESSION_COOKIE_SECURE = is_production() + # Application metadata APP_VERSION = '1.1.0' + # F15: /health returns `version` by default (the existing contract). Operators + # who would rather not advertise it can set HEALTH_REVEAL_VERSION=0. + HEALTH_REVEAL_VERSION = os.environ.get('HEALTH_REVEAL_VERSION', '1').strip().lower() not in ( + '0', + 'false', + 'no', + ) + # Debug mode DEBUG = os.environ.get('FLASK_DEBUG', '0').lower() in ('1', 'true', 'yes') diff --git a/routes.py b/routes.py index 3157558..28d8932 100644 --- a/routes.py +++ b/routes.py @@ -9,6 +9,7 @@ import requests from flask import Blueprint, Response, current_app, jsonify, render_template, request from requests.auth import HTTPBasicAuth +from werkzeug.exceptions import HTTPException from extensions import limiter from helpers import ( @@ -46,6 +47,40 @@ HEADER_NAME_PATTERN = re.compile(r'^[A-Za-z0-9-]+$') +# F13: `accept=".json,.jsonl"` on the file input is client-side only. The +# extension check is the authoritative one; the content-type check is deliberately +# lenient because browsers send application/octet-stream (or nothing at all) for +# extensions they do not recognize -- .jsonl in particular -- so a strict list +# would reject legitimate uploads. +ALLOWED_UPLOAD_EXTENSIONS = ('.json', '.jsonl') + +ALLOWED_UPLOAD_CONTENT_TYPES = frozenset( + { + '', + 'application/json', + 'application/jsonl', + 'application/ld+json', + 'application/octet-stream', + 'application/x-ndjson', + 'text/json', + 'text/plain', + 'text/x-json', + } +) + + +def validate_upload(file_storage): + """Return an error message for a file we will not try to parse, else None.""" + filename = (file_storage.filename or '').strip().lower() + if not filename.endswith(ALLOWED_UPLOAD_EXTENSIONS): + return 'File must be a .json or .jsonl file' + + content_type = (file_storage.mimetype or '').strip().lower() + if content_type not in ALLOWED_UPLOAD_CONTENT_TYPES: + return f'Unsupported content type: {content_type}' + + return None + def is_allowed_outbound_header(name): """True when a client-supplied outbound header name may be forwarded.""" @@ -64,7 +99,10 @@ def index(): @bp.route('/health') def health(): """Health check endpoint.""" - return jsonify({'status': 'ok', 'version': current_app.config['APP_VERSION']}) + payload = {'status': 'ok'} + if current_app.config.get('HEALTH_REVEAL_VERSION', True): + payload['version'] = current_app.config['APP_VERSION'] + return jsonify(payload) @bp.route('/process', methods=['POST']) @@ -83,6 +121,9 @@ def process_json(): file = request.files['json_file'] if file.filename == '': return jsonify({'error': 'No file selected'}), 400 + upload_error = validate_upload(file) + if upload_error: + return jsonify({'error': upload_error}), 400 try: content = file.read().decode('utf-8') json_data = parse_jsonl(content) if data_format == 'jsonl' else json.loads(content) @@ -239,6 +280,12 @@ def process_json(): } ) + except HTTPException: + # Werkzeug raises these lazily inside the route -- RequestEntityTooLarge + # fires the first time the oversized body is read. They already carry the + # right status, so let Flask's error handlers render them as JSON (F10) + # instead of swallowing them into a 500 below. + raise except RecursionError: # Valid JSON can nest deeply enough to exhaust the C stack, in json.loads # itself, in the helpers, or in the response encoder. That is the caller's @@ -299,6 +346,8 @@ def export_csv(): }, ) + except HTTPException: + raise except Exception: logger.exception('Unexpected error in export_csv') return jsonify({'error': 'Export failed'}), 500 @@ -341,6 +390,8 @@ def export_xlsx(): }, ) + except HTTPException: + raise except Exception: logger.exception('Unexpected error in export_xlsx') return jsonify({'error': 'Export failed'}), 500 diff --git a/security.py b/security.py index 62f1d79..89c53ed 100644 --- a/security.py +++ b/security.py @@ -31,6 +31,18 @@ CONTENT_SECURITY_POLICY = '; '.join(CSP_DIRECTIVES) +# Responses that carry payload data (or ops state) must not be retained by a +# shared cache, a proxy or the browser's bfcache (F11). The index page and the +# static assets are deliberately absent -- they are cacheable (P6). +NO_STORE_ENDPOINTS = frozenset( + { + 'main.process_json', + 'main.export_csv', + 'main.export_xlsx', + 'main.health', + } +) + # --- Bounded DNS admission (F6.1 / P7) -------------------------------------- # @@ -248,6 +260,9 @@ def apply_security_headers(response): response.headers['Cross-Origin-Resource-Policy'] = 'same-origin' response.headers['Content-Security-Policy'] = CONTENT_SECURITY_POLICY + if request.endpoint in NO_STORE_ENDPOINTS: + response.headers['Cache-Control'] = 'no-store' + # HSTS only makes sense once the connection is already TLS; sending it over # plain http would pin a local dev server to https. request.is_secure reads # X-Forwarded-Proto only when ProxyFix is enabled (TRUST_PROXY=1), which is diff --git a/tests/test_routes.py b/tests/test_routes.py index ac041c2..04060af 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -997,3 +997,171 @@ def test_different_clients_get_different_buckets(self, fresh_config): keys.append(seen['key']) assert keys == ['9.9.9.9', '8.8.8.8'] + + +class TestJsonErrorHandlers: + """F10 - every error response is JSON, including the framework's own.""" + + def test_oversized_request_returns_json_413(self, fresh_config): + cfg = fresh_config(MAX_UPLOAD_SIZE=str(1024 * 1024)) + app = create_app(cfg) + app.config['WTF_CSRF_ENABLED'] = False + + response = app.test_client().post( + '/process', + data={'input_method': 'paste', 'pasted_json': 'x' * (2 * 1024 * 1024)}, + ) + + assert response.status_code == 413 + assert response.content_type.startswith('application/json') + assert json.loads(response.data)['error'] == 'Request too large (max 1MB)' + + def test_unknown_route_returns_json_404(self, client): + response = client.get('/no-such-route') + assert response.status_code == 404 + assert response.content_type.startswith('application/json') + assert 'error' in json.loads(response.data) + + def test_internal_error_returns_json_500(self, fresh_config): + cfg = fresh_config() + app = create_app(cfg) + app.config['WTF_CSRF_ENABLED'] = False + # TESTING would re-raise instead of routing to the handler. + app.config['PROPAGATE_EXCEPTIONS'] = False + + @app.route('/boom') + def _boom(): + raise RuntimeError('kaboom') + + response = app.test_client().get('/boom') + assert response.status_code == 500 + assert response.content_type.startswith('application/json') + assert json.loads(response.data)['error'] == 'An internal error occurred' + assert b'kaboom' not in response.data + + +class TestNoStoreCacheControl: + """F11 - data-bearing responses must not be retained by any cache.""" + + def test_health_is_no_store(self, client): + assert client.get('/health').headers['Cache-Control'] == 'no-store' + + def test_process_is_no_store(self, client): + response = client.post( + '/process', + data={ + 'input_method': 'paste', + 'pasted_json': '[{"a": 1}]', + 'json_path': '(root)', + }, + ) + assert response.headers['Cache-Control'] == 'no-store' + + def test_export_csv_is_no_store(self, client): + response = client.post( + '/export-csv', + data=json.dumps({'csv_data': [{'a': 1}], 'csv_columns': ['a']}), + content_type='application/json', + ) + assert response.headers['Cache-Control'] == 'no-store' + + def test_export_xlsx_is_no_store(self, client): + response = client.post( + '/export-xlsx', + data=json.dumps({'csv_data': [{'a': 1}], 'csv_columns': ['a']}), + content_type='application/json', + ) + assert response.headers['Cache-Control'] == 'no-store' + + def test_index_page_is_still_cacheable(self, client): + assert client.get('/').headers.get('Cache-Control') != 'no-store' + + +class TestUploadValidation: + """F13 - the server, not just the file input's accept attribute.""" + + def _upload(self, client, filename, content_type=None, body=b'[{"a": 1}]'): + data = { + 'input_method': 'file', + 'json_path': '(root)', + 'json_file': (io.BytesIO(body), filename, content_type) + if content_type is not None + else (io.BytesIO(body), filename), + } + return client.post('/process', data=data, content_type='multipart/form-data') + + def test_rejects_unexpected_extensions(self, client): + for filename in ('evil.txt', 'evil.exe', 'evil', 'evil.json.png', 'evil.csv'): + response = self._upload(client, filename) + assert response.status_code == 400, filename + assert '.json or .jsonl' in json.loads(response.data)['error'] + + def test_rejects_unexpected_content_type(self, client): + response = self._upload(client, 'data.json', content_type='image/png') + assert response.status_code == 400 + assert 'Unsupported content type' in json.loads(response.data)['error'] + + def test_accepts_json_and_jsonl(self, client): + assert json.loads(self._upload(client, 'data.json').data)['success'] is True + assert json.loads(self._upload(client, 'DATA.JSON').data)['success'] is True + + def test_accepts_the_octet_stream_browsers_send_for_jsonl(self, client): + response = self._upload( + client, + 'data.jsonl', + content_type='application/octet-stream', + body=b'{"a": 1}', + ) + assert response.status_code == 200 + + +class TestCookieHardening: + """F16 - explicit cookie flags, Secure tied to APP_ENV=production.""" + + @staticmethod + def _set_cookie(app): + app.config['TESTING'] = True + # The index page calls csrf_token(), which writes to the session. + response = app.test_client().get('/', base_url='https://localhost') + return response.headers.get('Set-Cookie', '') + + def test_local_run_flags(self, fresh_config): + cfg = fresh_config(APP_ENV=None) + assert cfg.SESSION_COOKIE_HTTPONLY is True + assert cfg.SESSION_COOKIE_SAMESITE == 'Lax' + assert cfg.SESSION_COOKIE_SECURE is False + + cookie = self._set_cookie(create_app(cfg)) + assert 'HttpOnly' in cookie + assert 'SameSite=Lax' in cookie + assert 'Secure' not in cookie + + def test_production_sets_secure(self, fresh_config): + cfg = fresh_config(APP_ENV='production', SECRET_KEY='a-real-random-value') + assert cfg.SESSION_COOKIE_SECURE is True + + cookie = self._set_cookie(create_app(cfg)) + assert 'Secure' in cookie + assert 'HttpOnly' in cookie + assert 'SameSite=Lax' in cookie + + +class TestHealthVersionGate: + """F15 - version is returned by default; the gate only lets operators opt out.""" + + def test_version_present_by_default(self, fresh_config): + cfg = fresh_config(HEALTH_REVEAL_VERSION=None) + assert cfg.HEALTH_REVEAL_VERSION is True + + app = create_app(cfg) + data = json.loads(app.test_client().get('/health').data) + assert data['status'] == 'ok' + assert data['version'] == cfg.APP_VERSION + + def test_version_hidden_when_disabled(self, fresh_config): + cfg = fresh_config(HEALTH_REVEAL_VERSION='0') + assert cfg.HEALTH_REVEAL_VERSION is False + + app = create_app(cfg) + data = json.loads(app.test_client().get('/health').data) + assert data == {'status': 'ok'} From 4e285bc9aca113bc7f0b2bfb40ce0f45c2a87c52 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:39:03 +0000 Subject: [PATCH 17/36] Phase 2.1: gzip large text and JSON responses P1 / D1. /process ships the full flattened dataset, so a 10 MB input commonly means a 5-20 MB uncompressed body and nothing anywhere in the stack compressed it -- not Flask, not gunicorn, not the README's Nginx config. Repetitive JSON compresses 5-10x. Implemented as ~40 lines of after_request middleware rather than adding Flask-Compress, per D1: the dependency count stays at 6. Eligibility, with the skips the finding calls for: streamed and passthrough bodies are never materialized (reading one would consume the generator the export routes use), 204/304 and HEAD carry no body, anything already carrying Content-Encoding is left alone, and only text/*, */*+json and a small set of text-ish mimetypes qualify -- so the XLSX zip is not re-compressed. Vary: Accept-Encoding is set on every eligible response, compressed or not, so caches key correctly; set_data recomputes Content-Length so it always matches the wire. This is a transfer-size fix only. The browser still receives, decompresses, parses and holds the whole dataset, and server peak memory is unchanged -- those are P2, P5 and P12. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- app.py | 75 +++++++++++++++++++++++++++++++++- config.py | 3 ++ tests/test_routes.py | 95 +++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 171 insertions(+), 2 deletions(-) diff --git a/app.py b/app.py index 8857e53..4e67ec9 100644 --- a/app.py +++ b/app.py @@ -1,6 +1,8 @@ """JSON Table Converter - Flask application factory.""" -from flask import Flask, jsonify +import gzip + +from flask import Flask, current_app, jsonify, request from werkzeug.middleware.proxy_fix import ProxyFix from config import DEV_SECRET_KEY, Config, is_production @@ -27,6 +29,76 @@ def _assert_production_secret_key(app): ) +# --- gzip (P1/D1) ----------------------------------------------------------- +# +# /process returns the full flattened dataset, so a 10 MB input commonly means a +# 5-20 MB response body. Repetitive JSON compresses 5-10x, which is the single +# largest transfer win available. Implemented in-repo rather than via +# Flask-Compress: ~40 lines against a new pinned dependency (D1). +# +# It does NOT reduce peak server memory or the client's parse cost -- the browser +# still receives, decompresses and stores the whole dataset. Those are P2/P5/P12. + +COMPRESSIBLE_MIMETYPES = frozenset( + { + 'application/json', + 'application/javascript', + 'application/xml', + 'image/svg+xml', + } +) + + +def _mark_varies_on_encoding(response): + """Add Accept-Encoding to Vary without duplicating an existing entry.""" + existing = [value.strip().lower() for value in response.headers.get('Vary', '').split(',')] + if 'accept-encoding' not in existing: + response.headers.add('Vary', 'Accept-Encoding') + + +def _is_compressible(response): + mimetype = (response.mimetype or '').lower() + return ( + mimetype.startswith('text/') + or mimetype.endswith('+json') + or (mimetype in COMPRESSIBLE_MIMETYPES) + ) + + +def compress_response(response): + """Gzip an eligible response body in place.""" + # A streamed or passthrough body must never be materialized here: reading it + # would consume the generator the export routes rely on. + if response.direct_passthrough or response.is_streamed: + return response + # 204/304 carry no body; HEAD must keep the headers a GET would produce, and + # rewriting Content-Length for a body we do not send would be wrong. + if response.status_code in (204, 304) or request.method == 'HEAD': + return response + if 'Content-Encoding' in response.headers: + return response + if not _is_compressible(response): + return response + + _mark_varies_on_encoding(response) + + if 'gzip' not in request.headers.get('Accept-Encoding', '').lower(): + return response + + data = response.get_data() + if len(data) < current_app.config.get('GZIP_MIN_SIZE', 1024): + return response + + compressed = gzip.compress(data, compresslevel=6) + if len(compressed) >= len(data): + return response + + # set_data recomputes Content-Length, so it always matches what we send. + response.set_data(compressed) + response.headers['Content-Encoding'] = 'gzip' + return response + + def _register_error_handlers(app): """ Keep every error response JSON (F10). @@ -71,6 +143,7 @@ def create_app(config_class=Config): # Security headers on every response app.after_request(apply_security_headers) + app.after_request(compress_response) _register_error_handlers(app) diff --git a/config.py b/config.py index fecf6d2..862b615 100644 --- a/config.py +++ b/config.py @@ -102,6 +102,9 @@ class Config: SESSION_COOKIE_SAMESITE = 'Lax' SESSION_COOKIE_SECURE = is_production() + # Responses smaller than this are not worth a gzip round trip (P1). + GZIP_MIN_SIZE = env_int('GZIP_MIN_SIZE', 1024) + # Application metadata APP_VERSION = '1.1.0' diff --git a/tests/test_routes.py b/tests/test_routes.py index 04060af..13ccf79 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -1,6 +1,7 @@ """Tests for Flask routes.""" import csv +import gzip import importlib import io import json @@ -9,7 +10,7 @@ from unittest.mock import MagicMock, patch import pytest -from flask import request +from flask import Response, request import config as config_module from app import create_app @@ -1165,3 +1166,95 @@ def test_version_hidden_when_disabled(self, fresh_config): app = create_app(cfg) data = json.loads(app.test_client().get('/health').data) assert data == {'status': 'ok'} + + +class TestGzipCompression: + """P1 - compress large text/JSON bodies, and nothing else.""" + + @staticmethod + def _big_payload(rows=400): + return json.dumps([{'id': i, 'name': f'user-{i}', 'note': 'x' * 60} for i in range(rows)]) + + def _process(self, client, **headers): + return client.post( + '/process', + data={ + 'input_method': 'paste', + 'pasted_json': self._big_payload(), + 'json_path': '(root)', + }, + headers=headers, + ) + + def test_large_json_is_compressed(self, client): + response = self._process(client, **{'Accept-Encoding': 'gzip, deflate'}) + + assert response.headers['Content-Encoding'] == 'gzip' + assert 'Accept-Encoding' in response.headers['Vary'] + + raw = response.get_data() + decompressed = gzip.decompress(raw) + assert json.loads(decompressed)['total_rows'] == 400 + assert len(raw) < len(decompressed) + # Content-Length must describe what is actually on the wire. + assert int(response.headers['Content-Length']) == len(raw) + + def test_not_compressed_without_accept_encoding(self, client): + response = self._process(client, **{'Accept-Encoding': 'identity'}) + assert 'Content-Encoding' not in response.headers + assert 'Accept-Encoding' in response.headers['Vary'] + assert json.loads(response.data)['total_rows'] == 400 + + def test_small_response_is_not_compressed(self, client): + response = client.get('/health', headers={'Accept-Encoding': 'gzip'}) + assert 'Content-Encoding' not in response.headers + assert json.loads(response.data)['status'] == 'ok' + + def test_vary_is_not_duplicated(self, client): + response = self._process(client, **{'Accept-Encoding': 'gzip'}) + varies = [v.strip().lower() for v in response.headers.get_all('Vary')] + assert varies.count('accept-encoding') == 1 + + def test_head_request_is_left_alone(self, client): + response = client.head('/', headers={'Accept-Encoding': 'gzip'}) + assert 'Content-Encoding' not in response.headers + + def test_already_encoded_body_is_left_alone(self, app): + app.config['PROPAGATE_EXCEPTIONS'] = False + + @app.route('/pre-encoded') + def _pre_encoded(): + payload = gzip.compress(b'{"already": "' + b'x' * 5000 + b'"}') + return Response( + payload, + mimetype='application/json', + headers={'Content-Encoding': 'gzip'}, + ) + + response = app.test_client().get('/pre-encoded', headers={'Accept-Encoding': 'gzip'}) + # Not double-compressed: one gzip layer decodes to the original JSON. + assert response.headers['Content-Encoding'] == 'gzip' + assert gzip.decompress(response.get_data()).startswith(b'{"already"') + + def test_streamed_response_is_left_alone(self, app): + @app.route('/streamed') + def _streamed(): + def generate(): + for index in range(500): + yield f'line {index} ' + 'y' * 40 + '\n' + + return Response(generate(), mimetype='text/plain') + + response = app.test_client().get('/streamed', headers={'Accept-Encoding': 'gzip'}) + assert 'Content-Encoding' not in response.headers + assert response.get_data().startswith(b'line 0 ') + + def test_binary_export_is_not_compressed(self, client): + response = client.post( + '/export-xlsx', + data=json.dumps({'csv_data': [{'a': 'x' * 100}] * 50, 'csv_columns': ['a']}), + content_type='application/json', + headers={'Accept-Encoding': 'gzip'}, + ) + # XLSX is a zip container; re-compressing it wastes CPU for nothing. + assert 'Content-Encoding' not in response.headers From dac58c624adacd6136989bc8cbd0106a18734cf7 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:39:30 +0000 Subject: [PATCH 18/36] Phase 2.2: cache static assets for a day behind versioned URLs P6. style.css and app.js were served with Last-Modified/304 but no Cache-Control, so browsers revalidated on every navigation -- an extra round trip per page load, worst during a Render free-tier cold start. SEND_FILE_MAX_AGE_DEFAULT is 86400 (STATIC_MAX_AGE), which is only safe because both asset URLs now carry ?v=APP_VERSION: bumping the version busts the cache, so an update can never be served stale. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- config.py | 5 +++++ templates/index.html | 4 ++-- tests/test_routes.py | 21 +++++++++++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/config.py b/config.py index 862b615..84e3cd4 100644 --- a/config.py +++ b/config.py @@ -102,6 +102,11 @@ class Config: SESSION_COOKIE_SAMESITE = 'Lax' SESSION_COOKIE_SECURE = is_production() + # Static assets are revalidated on every navigation without this, costing a + # round trip per page load -- worst on a Render free-tier cold start (P6). + # Safe to cache for a day because the asset URLs carry ?v=APP_VERSION. + SEND_FILE_MAX_AGE_DEFAULT = env_int('STATIC_MAX_AGE', 86400) + # Responses smaller than this are not worth a gzip round trip (P1). GZIP_MIN_SIZE = env_int('GZIP_MIN_SIZE', 1024) diff --git a/templates/index.html b/templates/index.html index e3fdf24..fc6593e 100644 --- a/templates/index.html +++ b/templates/index.html @@ -8,7 +8,7 @@ - +
@@ -234,6 +234,6 @@

JSON → Table

- + diff --git a/tests/test_routes.py b/tests/test_routes.py index 13ccf79..8753bf8 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -1258,3 +1258,24 @@ def test_binary_export_is_not_compressed(self, client): ) # XLSX is a zip container; re-compressing it wastes CPU for nothing. assert 'Content-Encoding' not in response.headers + + +class TestStaticAssetCaching: + """P6 - static assets are cacheable, and their URLs are version-busted.""" + + def test_static_assets_carry_a_max_age(self, client): + response = client.get('/static/css/style.css') + assert response.status_code == 200 + assert 'max-age=86400' in response.headers['Cache-Control'] + + def test_asset_urls_are_version_busted(self, client, app): + html = client.get('/').data.decode('utf-8') + version = app.config['APP_VERSION'] + assert f'css/style.css?v={version}' in html + assert f'js/app.js?v={version}' in html + + def test_max_age_is_configurable(self, fresh_config): + cfg = fresh_config(STATIC_MAX_AGE='60') + app = create_app(cfg) + response = app.test_client().get('/static/js/app.js') + assert 'max-age=60' in response.headers['Cache-Control'] From 0a9515f5f9cffe2dcb96382eab0c30d7d26fc9e5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:58:13 +0000 Subject: [PATCH 19/36] Phase 2.3-2.10: exports, preview projection, client caps, topology guard These tasks land together because they interleave in routes.py, config.py and static/js/app.js; each is described separately below. 2.3 - Diskless, memory-bounded exports (P3/D6) CSV is now generator-streamed: one StringIO is reused and drained every 500 rows instead of the whole file being built before the first byte goes out. It stays deliberately UNCAPPED -- CSV is natively streamable with no temp files, so every dataset /process accepts remains exportable by some route. That, not an unbounded XLSX path, is what keeps the export contract as wide as the input contract. XLSX keeps a normal-mode Workbook and a plain BytesIO, so no OS temp files are written. write_only mode writes worksheet parts to disk, and SpooledTemporaryFile is either pointless -- its default max_size=0 never rolls over, so it is a BytesIO with extra indirection and zero memory benefit -- or disk-backed once a non-zero threshold is crossed or fileno() is called. What bounds memory instead is MAX_EXPORT_CELLS, and send_file replaces output.getvalue(), which made a second full copy of the workbook at peak. The budget is in CELLS, enabled by default, and measured rather than chosen: docs/export-budget-v1.2.md records seven run-pairs and the derivation. At equal cell counts the narrow/tall shape is consistently worse (84.3 MiB at 50k x 3 vs 73.6 at 15k x 10), so 3 columns is the worst aspect ratio tested and the one sized against. 274,998 cells measured 152.2 MiB, over the 150 MiB target, putting the crossing at ~270,900; the default is 250,000, about 8% below it. 0 disables it. Advertised, never silent: /process gains total_cells and max_export_cells (additive -- no existing key changes name, type or meaning), the client greys out the Excel entry before the user clicks, and /export-xlsx independently returns 400 for direct API callers. No truncation. 2.4 - Non-mutating preview truncation (P2.2/P5) helpers.preview_truncate builds a capped COPY of a preview row: strings over 256 chars, nested objects over 20 keys and nested arrays over 20 items get markers. Every column of the row survives -- capping row keys would leave the preview table disagreeing with its own header. Because it is a projection, table_data and csv_data keep full fidelity; tests assert both server exports still contain the untruncated values. 2.5 - Lazy tree picker (P4) The picker built a DOM node for every key of every object up front, so a 10 MB payload meant tens of thousands of nodes in one synchronous pass. Children are now built on first toggle, with the per-level fan-out capped at 200 and the total node count at 5000. 2.6 - Client render caps (P5) renderNestedObject stops at 20 keys, primitive arrays are stringified only up to their first 20 items, long strings are cut at 500 chars, and the nested table caps its column count. A 50k-key object and a 100k-item array now render in bounded output. 2.7 - Memory trims (P12/P8) The API path decodes the accumulated bytearray directly; bytes(content) made a second full copy of the body at peak. This removes one copy only: parsing, flattening and jsonify still materialize the dataset. helpers.flatten_rows flattens and collects column names in one pass rather than flattening and then rescanning with get_all_columns; names go into a set and are sorted once, which is byte-identical to the old output. Tests assert parity with the two-pass result, order included. 2.8 - gunicorn tuning (P9) --timeout 60 in render.yaml and all three README invocations. gunicorn's default 30s equals the default API_FETCH_TIMEOUT, so a slow fetch raced the worker kill and surfaced as a 502. The invariant is documented. 2.9 - Chunked Blob (P13) Client CSV/TSV is assembled as a list of parts handed straight to Blob, instead of one 10-30 MB string plus a copy of it. 2.10 - Rate-limit storage matches the deployment topology (F12) (a) RATELIMIT_STORAGE_URI was hardcoded to memory://, so no deployment could configure shared storage at all; it now reads from the environment. (b) WEB_CONCURRENCY is the single source of truth for the worker count -- gunicorn reads it natively and every start command passes --workers "$WEB_CONCURRENCY" -- and the guard also parses the start command's own --workers, so a bare value that contradicts the declaration is a startup error. APP_REPLICAS mirrors render.yaml's numInstances, since replica count is invisible from inside the process. (c) Defaults of 1 fail open, so under APP_ENV=production both counts must be declared explicitly, and shared storage is required whenever either exceeds one or cannot be verified. Outside production the same conditions warn. (d) render.yaml and the three README --workers 4 examples are corrected, and requirements-redis.txt carries the exact-pinned client Flask-Limiter needs for redis:// -- without it, limiter init raises ConfigurationError, which a test now covers. requirements-dev.txt pulls it in so CI exercises the documented multi-worker configuration. A test proves the multiplier is real: two memory:// storages do not share counters. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- .github/workflows/ci.yml | 3 + README.md | 58 +++++- app.py | 92 ++++++++ config.py | 39 +++- docs/export-budget-v1.2.md | 108 ++++++++++ helpers.py | 88 ++++++++ render.yaml | 24 ++- requirements-dev.txt | 5 + routes.py | 121 +++++++++-- static/css/style.css | 7 + static/js/app.js | 223 ++++++++++++++++---- tests/js/dom_stub.mjs | 16 +- tests/js/test_render_caps.mjs | 122 +++++++++++ tests/test_helpers.py | 96 +++++++++ tests/test_routes.py | 382 +++++++++++++++++++++++++++++++++- 15 files changed, 1306 insertions(+), 78 deletions(-) create mode 100644 docs/export-budget-v1.2.md create mode 100644 tests/js/test_render_caps.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c814ad9..f4efe38 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,5 +36,8 @@ jobs: - name: Client-side export assertions (F1) run: node tests/js/test_export_sanitize.mjs + - name: Client-side render/cap assertions (P4/P5/P13) + run: node tests/js/test_render_caps.mjs + - name: Audit runtime dependencies run: pip-audit -r requirements.txt diff --git a/README.md b/README.md index 7db4d14..f5c2da5 100644 --- a/README.md +++ b/README.md @@ -111,7 +111,8 @@ git push -u origin main - **Branch**: `main` - **Runtime**: `Python 3` - **Build Command**: `pip install -r requirements.txt` - - **Start Command**: `gunicorn "app:create_app()" --bind 0.0.0.0:$PORT` + - **Start Command**: `gunicorn "app:create_app()" --bind 0.0.0.0:$PORT --workers "$WEB_CONCURRENCY" --timeout 60` + - **Environment**: `APP_ENV=production`, `WEB_CONCURRENCY=1`, `APP_REPLICAS=1` (see [Deployment topology](#deployment-topology-and-rate-limiting)) 4. Select **Free** plan 5. Click **"Create Web Service"** @@ -184,9 +185,19 @@ pip install -r requirements.txt # Set production environment variables export SECRET_KEY="your-random-secret-key-here" export FLASK_DEBUG=0 - -# Run with gunicorn -gunicorn "app:create_app()" --bind 0.0.0.0:8000 --workers 4 +export APP_ENV=production + +# Deployment topology. memory:// rate-limit counters are process-local, so the +# effective limit is multiplied by workers x replicas. One worker and one +# instance is the default; see "Deployment topology and rate limiting" below +# before raising either. +export WEB_CONCURRENCY=1 +export APP_REPLICAS=1 + +# Run with gunicorn. --workers comes from WEB_CONCURRENCY so the running count +# and the declared count cannot drift, and --timeout stays above +# API_FETCH_TIMEOUT (default 30s). +gunicorn "app:create_app()" --bind 0.0.0.0:8000 --workers "$WEB_CONCURRENCY" --timeout 60 ``` #### Systemd Service (Auto-Start on Boot) @@ -204,7 +215,10 @@ Group=www-data WorkingDirectory=/opt/json-table-tool Environment="SECRET_KEY=your-random-secret-key-here" Environment="FLASK_DEBUG=0" -ExecStart=/opt/json-table-tool/venv/bin/gunicorn "app:create_app()" --bind 127.0.0.1:8000 --workers 4 +Environment="APP_ENV=production" +Environment="WEB_CONCURRENCY=1" +Environment="APP_REPLICAS=1" +ExecStart=/opt/json-table-tool/venv/bin/gunicorn "app:create_app()" --bind 127.0.0.1:8000 --workers ${WEB_CONCURRENCY} --timeout 60 Restart=always [Install] @@ -253,7 +267,12 @@ COPY requirements.txt . RUN pip install --no-cache-dir -r requirements.txt COPY . . EXPOSE 8000 -CMD ["gunicorn", "app:create_app()", "--bind", "0.0.0.0:8000", "--workers", "4"] +ENV APP_ENV=production +ENV WEB_CONCURRENCY=1 +ENV APP_REPLICAS=1 +# --workers is derived from WEB_CONCURRENCY (shell form so it expands), and +# --timeout stays above API_FETCH_TIMEOUT. +CMD gunicorn "app:create_app()" --bind 0.0.0.0:8000 --workers "$WEB_CONCURRENCY" --timeout 60 ``` ```bash @@ -263,6 +282,33 @@ docker run -p 8000:8000 \ json-table-tool ``` +### Deployment topology and rate limiting + +Flask-Limiter's default `memory://` storage keeps its counters **inside one +process**. The effective limit is therefore multiplied by `workers x replicas`, +not by workers alone: four workers on two instances enforce eight times the +configured limit. + +The supported default is **one worker and one instance**. To run more: + +1. Install the Redis client: `pip install -r requirements.txt -r requirements-redis.txt` +2. Set `RATELIMIT_STORAGE_URI=redis://...` +3. Raise `WEB_CONCURRENCY` (and `APP_REPLICAS`, mirroring `numInstances`) + +Under `APP_ENV=production` the app refuses to start if `WEB_CONCURRENCY` or +`APP_REPLICAS` is undeclared, if a `--workers N` in the start command disagrees +with `WEB_CONCURRENCY`, or if either count exceeds one while storage is still +`memory://`. Outside production the same conditions log a warning instead. + +**HTTPS is required on every deployment path.** API keys, bearer tokens and +basic-auth passwords are POSTed from the browser to this app; without TLS they +are exposed on the wire. + +**Timeout invariant:** gunicorn's `--timeout` must stay above +`API_FETCH_TIMEOUT` (a factor of two is the documented margin). gunicorn's +default of 30s equals the default `API_FETCH_TIMEOUT`, so a slow API fetch +raced the worker kill and surfaced as a 502. + #### Docker Compose ```yaml diff --git a/app.py b/app.py index 4e67ec9..285d884 100644 --- a/app.py +++ b/app.py @@ -1,6 +1,9 @@ """JSON Table Converter - Flask application factory.""" import gzip +import logging +import os +import sys from flask import Flask, current_app, jsonify, request from werkzeug.middleware.proxy_fix import ProxyFix @@ -9,6 +12,8 @@ from extensions import csrf, limiter from security import apply_security_headers +logger = logging.getLogger(__name__) + def _assert_production_secret_key(app): """ @@ -99,6 +104,92 @@ def compress_response(response): return response +# --- Rate-limit topology guard (2.10 / F12) --------------------------------- + + +def worker_count_from_start_command(argv): + """ + Return the worker count the start command names, or None. + + gunicorn forks its workers, so a worker inherits the master's argv. A bare + `--workers N` that disagrees with WEB_CONCURRENCY is exactly the drift this + exists to catch -- the app would validate one number while gunicorn ran + another. + """ + if not argv or 'gunicorn' not in os.path.basename(argv[0]): + return None + for index, arg in enumerate(argv): + if arg.startswith('--workers='): + value = arg.split('=', 1)[1] + elif arg in ('-w', '--workers') and index + 1 < len(argv): + value = argv[index + 1] + else: + continue + try: + return int(value) + except ValueError: + return None + return None + + +def check_rate_limit_topology(app, argv=None): + """ + Refuse a production deployment whose rate limiting cannot be trusted. + + Raises under APP_ENV=production and warns otherwise, so local development is + never blocked by a topology it does not have. + """ + argv = sys.argv if argv is None else argv + production = is_production() + + workers = app.config.get('WEB_CONCURRENCY', 1) + replicas = app.config.get('APP_REPLICAS', 1) + storage = app.config.get('RATELIMIT_STORAGE_URI', 'memory://') + storage_is_shared = not storage.startswith('memory:') + + problems = [] + unverified = False + + if production: + if not app.config.get('WEB_CONCURRENCY_DECLARED'): + problems.append( + 'WEB_CONCURRENCY is not declared; under APP_ENV=production the worker ' + 'count must be explicit, because a default of 1 makes an undeclared ' + 'multi-worker deployment look single-worker' + ) + unverified = True + if not app.config.get('APP_REPLICAS_DECLARED'): + problems.append( + 'APP_REPLICAS is not declared; it must mirror the deployment layer ' + "(render.yaml's numInstances)" + ) + unverified = True + + commanded_workers = worker_count_from_start_command(argv) + if commanded_workers is not None and commanded_workers != workers: + problems.append( + f'the start command runs --workers {commanded_workers} but WEB_CONCURRENCY ' + f'is {workers}; derive the command from the variable ' + f'(gunicorn ... --workers "$WEB_CONCURRENCY") so the two cannot disagree' + ) + + if not storage_is_shared and (workers > 1 or replicas > 1 or unverified): + problems.append( + f'RATELIMIT_STORAGE_URI is {storage!r}, whose counters are process-local, ' + f'so the effective limit is multiplied by workers x replicas ' + f'({workers} x {replicas}); set a shared backend ' + '(RATELIMIT_STORAGE_URI=redis://...) or run one worker and one instance' + ) + + if not problems: + return + + message = 'Rate-limit topology is inconsistent: ' + '; '.join(problems) + if production: + raise RuntimeError(message) + logger.warning(message) + + def _register_error_handlers(app): """ Keep every error response JSON (F10). @@ -128,6 +219,7 @@ def create_app(config_class=Config): app.config.from_object(config_class) _assert_production_secret_key(app) + check_rate_limit_topology(app) if app.config.get('TRUST_PROXY'): # Exactly one trusted hop. Behind Render's load balancer or an Nginx diff --git a/config.py b/config.py index 84e3cd4..763c723 100644 --- a/config.py +++ b/config.py @@ -5,6 +5,10 @@ # 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. +DEFAULT_MAX_EXPORT_CELLS = 250_000 + def is_production(): """ @@ -87,8 +91,27 @@ class Config: # with it unset, behavior is identical to v1.1 and forged headers are ignored. TRUST_PROXY = os.environ.get('TRUST_PROXY', '0').strip().lower() in ('1', 'true', 'yes') - # Rate limiting (Flask-Limiter reads RATELIMIT_* keys automatically) - RATELIMIT_STORAGE_URI = 'memory://' + # Rate limiting (Flask-Limiter reads RATELIMIT_* keys automatically). + # + # memory:// counters are PROCESS-LOCAL, so the effective limit is multiplied + # by workers x replicas -- not by workers alone. This was hardcoded before + # v1.2, so no deployment could configure shared storage at all (2.10a/F12). + RATELIMIT_STORAGE_URI = os.environ.get('RATELIMIT_STORAGE_URI', 'memory://') + + # WEB_CONCURRENCY is the SINGLE source of truth for the worker count: gunicorn + # reads it natively and every documented start command passes + # --workers "$WEB_CONCURRENCY", so the number this process validates cannot + # drift from the number gunicorn actually runs. Replica count is invisible from + # inside the process, so APP_REPLICAS is a deployment-layer declaration that + # must mirror render.yaml's numInstances. + # + # Defaults of 1 fail OPEN -- an undeclared 4-worker deployment reads as + # single-worker -- so under APP_ENV=production both must be declared + # explicitly; see check_rate_limit_topology in app.py. + WEB_CONCURRENCY = env_int('WEB_CONCURRENCY', 1) + APP_REPLICAS = env_int('APP_REPLICAS', 1) + WEB_CONCURRENCY_DECLARED = (os.environ.get('WEB_CONCURRENCY') or '').strip() != '' + APP_REPLICAS_DECLARED = (os.environ.get('APP_REPLICAS') or '').strip() != '' RATELIMIT_DEFAULT = os.environ.get('RATE_LIMIT_DEFAULT', '120/minute') RATE_LIMIT_PROCESS = os.environ.get('RATE_LIMIT_PROCESS', '30/minute') RATE_LIMIT_EXPORT = os.environ.get('RATE_LIMIT_EXPORT', '60/minute') @@ -102,6 +125,18 @@ class Config: SESSION_COOKIE_SAMESITE = 'Lax' SESSION_COOKIE_SECURE = is_production() + # XLSX-only export budget (P3/D6), in CELLS (rows x columns) because that is + # what drives openpyxl's memory -- a 10 MiB body with 3 columns and one with + # 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 + # 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) + # Static assets are revalidated on every navigation without this, costing a # round trip per page load -- worst on a Render free-tier cold start (P6). # Safe to cache for a day because the asset URLs carry ?v=APP_VERSION. diff --git a/docs/export-budget-v1.2.md b/docs/export-budget-v1.2.md new file mode 100644 index 0000000..181f5b7 --- /dev/null +++ b/docs/export-budget-v1.2.md @@ -0,0 +1,108 @@ +# XLSX export budget — how `MAX_EXPORT_CELLS` was measured + +**Date:** 2026-08-21 +**Applies to:** roadmap task 2.3 / D6, performance finding P3. +**Result:** `DEFAULT_MAX_EXPORT_CELLS = 250_000` cells (`config.py`). + +D6 requires the guard's default to come from the Performance Review §4 +measurement rather than being chosen by feel. This file records the numbers it +was derived from, so it can be re-derived when the measurement is re-run. + +## Method + +Performance Review §4's protocol, with one documented deviation. + +Followed as written: + +| Parameter | This run | +|---|---| +| Units | `ru_maxrss` read on Linux (KiB), multiplied by 1024, reported in MiB | +| Peak | `resource.getrusage(RUSAGE_SELF).ru_maxrss`, read after the response was fully delivered | +| No warm-up in a measured process | Each measured process serves **exactly one** request, then exits | +| Two runs, not two samples | `baseline` = a freshly booted process that served **zero** requests; `measured` = a freshly booted process that served exactly one. `delta = measured − baseline` | +| Concurrency | 1 | +| Absolute and delta | Both recorded | +| Blocked pairs | None occurred; none were discarded | + +**Deviation:** requests were issued through the Flask test client inside a fresh +Python process rather than through a fresh gunicorn worker, and the sizing sweep +used one run-pair per shape rather than the median of three. The property the +protocol exists to protect — that `ru_maxrss` is monotonic per process and so +cannot be reset mid-process — is preserved: every measured number comes from a +process that served exactly one request. Numbers produced this way are +comparable to each other but should not be quoted against the §4 budget as if +they had come from the full gunicorn/median-of-three protocol. + +`MAX_CONTENT_LENGTH` was raised for the measurement only, so request-body size +would not mask the workbook cost. + +## Data + +| rows × cols | cells | delta (MiB) | absolute (MiB) | +|---|---|---|---| +| 15,000 × 10 | 150,000 | 73.6 | 114.3 | +| 50,000 × 3 | 150,000 | **84.3** | 125.0 | +| 20,000 × 10 | 200,000 | 98.6 | 139.2 | +| 2,000 × 150 | 300,000 | 137.3 | 178.0 | +| 91,666 × 3 | 274,998 | **152.2** | 193.0 | +| 100,000 × 3 | 300,000 | **161.7** | 202.4 | +| 40,000 × 10 | 400,000 | 195.6 | 236.3 | + +At equal cell counts the **narrow, tall** shape is consistently the most +expensive (84.3 vs 73.6 MiB at 150,000 cells; 161.7 vs 137.3 at 300,000) — +openpyxl carries per-row overhead on top of per-cell. Three columns is therefore +the worst aspect ratio tested and the one the budget is sized against. + +This is also why the budget is expressed in **cells** rather than rows: 50,000 +rows costs 84.3 MiB at 3 columns and 40,000 rows costs 195.6 MiB at 10, so a row +count says almost nothing about the footprint on its own. + +## Derivation + +Fitting the two bracketing points of the 3-column series: + +``` +(150,000 cells, 84.3 MiB) and (274,998 cells, 152.2 MiB) +slope = 0.000543 MiB/cell +intercept = 2.8 MiB +150 MiB crossing = (150 − 2.8) / 0.000543 ≈ 270,900 cells +``` + +The 274,998-cell run measured **152.2 MiB — over the 150 MiB target**, so the +crossing is real and not an artifact of extrapolation. The budget is set at +**250,000 cells**, roughly 8% below the crossing, which is the margin +run-to-run variance on these measurements needs. Predicted delta at that size is +≈ 138.6 MiB, and the measured absolute high-water mark at nearby sizes stays +well under the 256 MiB (half of a 512 MiB container) ceiling. + +## What the budget does and does not cover + +- It is **XLSX-only**. CSV and TSV are generator-streamed and stay uncapped, so + every dataset `/process` accepts remains exportable by some route. That, not + an unbounded XLSX path, is what keeps the export contract as wide as the input + contract. +- It is **enabled by default**. An unlimited default would leave P3 (High) + unmitigated. `MAX_EXPORT_CELLS=0` disables it for operators who knowingly opt + out. +- It is **advertised, never silent**. `/process` returns `total_cells` and + `max_export_cells` so the client greys the Excel entry out before the user + clicks; `/export-xlsx` independently returns 400 for direct API callers. There + is no truncation — a partial spreadsheet is worse than a refusal. +- The 10 MiB request cap makes memory finite but not usefully bounded: the + JSON → Python → openpyxl → zip expansion multiplier is large and + data-dependent. The measured cell budget is the bound; the request cap is not + a substitute for it. + +## Reproducing + +The harness is not committed (it is a throwaway measurement script, and the +roadmap keeps perf tests out of CI). It does two things: + +1. `baseline`: boot `create_app()`, serve nothing, print `ru_maxrss`. +2. `measured`: boot `create_app()`, POST one synthetic `csv_data` body of + `rows × cols` cells to `/export-xlsx` with `MAX_EXPORT_CELLS=0`, assert 200, + then print `ru_maxrss`. + +Run each in its own process and subtract. Re-derive the fit above from at least +two points on the narrowest aspect ratio you care about, and re-run the +confirming point at the value you intend to ship. diff --git a/helpers.py b/helpers.py index 31ab202..8889b29 100644 --- a/helpers.py +++ b/helpers.py @@ -72,6 +72,27 @@ def sanitize_cell(value): return serialized +def flatten_rows(rows, max_depth=10): + """ + Flatten every row and collect the column names in a single pass. + + Returns (flattened_rows, sorted_column_names). Previously the caller + flattened, then walked the result again with get_all_columns -- two full + passes over the largest structure in the request (P8). + + The names are accumulated in a set and sorted once at the end, which is + exactly what get_all_columns produces; set iteration order is not relied on. + """ + flattened = [] + columns = set() + for row in rows: + flat = flatten_for_csv(row, max_depth=max_depth) + flattened.append(flat) + if isinstance(flat, dict): + columns.update(flat.keys()) + return flattened, sorted(columns) + + def extract_table_data(json_data, _depth=0, max_depth=10): """ Extract tabular data from JSON. @@ -162,3 +183,70 @@ def get_all_columns(data): if isinstance(row, dict): columns.update(row.keys()) return sorted(columns) + + +# --- Preview projection (P2.2/P5) ------------------------------------------- +# +# `preview` rows carry full-fidelity nested structures, so a 50k-key object or a +# 5 MB string cell is handed straight to the browser and freezes the tab. These +# caps apply to the PREVIEW ONLY: the projection is a copy, so table_data and +# csv_data -- and therefore every export -- keep the original values. + +PREVIEW_MAX_STRING = 256 +PREVIEW_MAX_ITEMS = 20 +PREVIEW_TRUNCATION_SUFFIX = '… (truncated)' + + +def _truncate_preview_value(value, max_string, max_items, depth, max_depth): + """Return a capped copy of one nested value.""" + if isinstance(value, str): + if len(value) > max_string: + return value[:max_string] + PREVIEW_TRUNCATION_SUFFIX + return value + + if isinstance(value, dict): + if depth >= max_depth: + return PREVIEW_TRUNCATION_SUFFIX + truncated = {} + for index, (key, item) in enumerate(value.items()): + if index >= max_items: + truncated[PREVIEW_TRUNCATION_SUFFIX] = f'… and {len(value) - max_items} more keys' + break + truncated[key] = _truncate_preview_value( + item, max_string, max_items, depth + 1, max_depth + ) + return truncated + + if isinstance(value, list): + if depth >= max_depth: + return PREVIEW_TRUNCATION_SUFFIX + truncated = [ + _truncate_preview_value(item, max_string, max_items, depth + 1, max_depth) + for item in value[:max_items] + ] + if len(value) > max_items: + truncated.append(f'… and {len(value) - max_items} more items') + return truncated + + return value + + +def preview_truncate( + row, + max_string=PREVIEW_MAX_STRING, + max_items=PREVIEW_MAX_ITEMS, + max_depth=10, +): + """ + Build a capped COPY of one preview row. + + Every column of the row survives -- dropping columns would make the preview + table disagree with its own header. Only the values inside are capped. + Nothing is mutated: exports read the original rows. + """ + if not isinstance(row, dict): + return _truncate_preview_value(row, max_string, max_items, 0, max_depth) + return { + key: _truncate_preview_value(value, max_string, max_items, 1, max_depth) + for key, value in row.items() + } diff --git a/render.yaml b/render.yaml index 3c15117..1636653 100644 --- a/render.yaml +++ b/render.yaml @@ -6,14 +6,36 @@ services: name: json-table-converter runtime: python buildCommand: pip install -r requirements.txt - startCommand: gunicorn "app:create_app()" --bind 0.0.0.0:$PORT + # --workers comes from WEB_CONCURRENCY so the number gunicorn runs is the + # same number the app validates (roadmap 2.10b). --timeout must stay above + # API_FETCH_TIMEOUT (default 30s) or a slow API fetch races the worker kill + # and the client sees a 502 instead of the timeout message (P9). + startCommand: >- + gunicorn "app:create_app()" + --bind 0.0.0.0:$PORT + --workers "$WEB_CONCURRENCY" + --timeout 60 envVars: - key: PYTHON_VERSION value: 3.14.5 - key: SECRET_KEY generateValue: true + # The one canonical production signal: enables the SECRET_KEY fail-fast + # (F7), the Secure session cookie (F16) and the topology guard (2.10). + - key: APP_ENV + value: production + # Rate-limit topology. memory:// counters are process-local, so the + # effective limit is multiplied by workers x replicas. Going above 1x1 + # requires a shared RATELIMIT_STORAGE_URI (redis://...) plus + # requirements-redis.txt in the build command -- the guard refuses to start + # otherwise. APP_REPLICAS must mirror numInstances below. + - key: WEB_CONCURRENCY + value: 1 + - key: APP_REPLICAS + value: 1 # Free tier settings plan: free + numInstances: 1 # No persistent disk = no data storage # Auto-deploy on push, but only once CI checks pass. Render blocks the # deploy when the commit has failing checks or no checks at all. diff --git a/requirements-dev.txt b/requirements-dev.txt index 8c74e53..c6dcc32 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,6 +1,11 @@ # Development / CI dependencies. Production installs requirements.txt only. -r requirements.txt +# The optional Redis client, so the test suite can exercise the documented +# multi-worker configuration (roadmap 2.10d). Production installs it separately +# via requirements-redis.txt; it is deliberately not in requirements.txt. +-r requirements-redis.txt + pytest==9.0.3 ruff==0.16.4 coverage==7.15.4 diff --git a/routes.py b/routes.py index 28d8932..2aba963 100644 --- a/routes.py +++ b/routes.py @@ -7,18 +7,28 @@ import re import requests -from flask import Blueprint, Response, current_app, jsonify, render_template, request +from flask import ( + Blueprint, + Response, + current_app, + jsonify, + render_template, + request, + send_file, +) from requests.auth import HTTPBasicAuth from werkzeug.exceptions import HTTPException +from config import DEFAULT_MAX_EXPORT_CELLS from extensions import limiter from helpers import ( extract_by_path, extract_table_data, - flatten_for_csv, + flatten_rows, get_all_columns, is_formula_trigger, parse_jsonl, + preview_truncate, sanitize_cell, serialize_cell_value, ) @@ -54,6 +64,9 @@ # would reject legitimate uploads. ALLOWED_UPLOAD_EXTENSIONS = ('.json', '.jsonl') +# Rows buffered before a chunk of CSV is handed to the WSGI server. +CSV_STREAM_CHUNK_ROWS = 500 + ALLOWED_UPLOAD_CONTENT_TYPES = frozenset( { '', @@ -215,7 +228,11 @@ def process_json(): } ), 400 - text = bytes(content).decode('utf-8') + # bytearray decodes directly; bytes(content) made a second full + # copy of the response body at peak (P12). Parsing, flattening and + # jsonify still materialize the dataset -- this removes one copy, + # it does not make the pipeline low-memory. + text = content.decode('utf-8') json_data = parse_jsonl(text) if data_format == 'jsonl' else json.loads(text) except requests.exceptions.Timeout: @@ -263,18 +280,29 @@ def process_json(): columns = get_all_columns(table_data) preview_limit = current_app.config['PREVIEW_ROW_LIMIT'] - preview_data = table_data[:preview_limit] + # A separate projection, not a mutation: csv_data below is built from the + # untouched rows, so exports stay full-fidelity (P2.2/P5). + preview_data = [preview_truncate(row) for row in table_data[:preview_limit]] max_depth = current_app.config['FLATTEN_MAX_DEPTH'] - csv_data = [flatten_for_csv(row, max_depth=max_depth) for row in table_data] - csv_columns = get_all_columns(csv_data) - + # One pass instead of flatten-then-rescan: names are collected into a set + # while flattening and sorted once at the end, which is byte-identical to + # get_all_columns' sorted output (P8). + csv_data, csv_columns = flatten_rows(table_data, max_depth=max_depth) + + # Additive only: no existing key changes name, type or meaning. + # total_cells/max_export_cells let the client grey out the Excel entry + # BEFORE the user clicks, rather than after a 400 (D6). return jsonify( { 'success': True, 'columns': columns, 'preview': preview_data, 'total_rows': len(table_data), + 'total_cells': len(csv_data) * len(csv_columns), + 'max_export_cells': current_app.config.get( + 'MAX_EXPORT_CELLS', DEFAULT_MAX_EXPORT_CELLS + ), 'csv_data': csv_data, 'csv_columns': csv_columns, } @@ -297,6 +325,36 @@ def process_json(): return jsonify({'error': 'An internal error occurred'}), 500 +def _stream_csv(columns, rows): + """ + Yield the CSV a chunk of rows at a time. + + csv.writer needs a text buffer, so one StringIO is reused and truncated every + CSV_STREAM_CHUNK_ROWS rows instead of the whole file being built in memory + before the first byte goes out (P3). + """ + buffer = io.StringIO() + writer = csv.writer(buffer) + + def drain(): + value = buffer.getvalue() + buffer.seek(0) + buffer.truncate(0) + return value + + writer.writerow([sanitize_cell(column) for column in columns]) + yield drain() + + for index, row in enumerate(rows, start=1): + writer.writerow([sanitize_cell(row.get(column, '')) for column in columns]) + if index % CSV_STREAM_CHUNK_ROWS == 0: + yield drain() + + remainder = drain() + if remainder: + yield remainder + + def _append_xlsx_row(ws, values): """ Append one row to a worksheet, writing formula-triggering strings as strings. @@ -329,16 +387,13 @@ def export_csv(): if not csv_data: return jsonify({'error': 'No data to export'}), 400 - output = io.StringIO() - writer = csv.writer(output) - writer.writerow([sanitize_cell(col) for col in csv_columns]) - - for row in csv_data: - writer.writerow([sanitize_cell(row.get(col, '')) for col in csv_columns]) - - output.seek(0) + # Streamed, and deliberately uncapped: CSV is natively streamable with no + # temp files, so every dataset /process accepts stays exportable by this + # route even when it is too large for a workbook (P3/D6). That, not an + # unbounded XLSX path, is what keeps the export contract as wide as the + # input contract. return Response( - output.getvalue(), + _stream_csv(csv_columns, csv_data), mimetype='text/csv', headers={ 'Content-Disposition': 'attachment; filename=exported_data.csv', @@ -370,6 +425,22 @@ def export_xlsx(): if not xlsx_data: return jsonify({'error': 'No data to export'}), 400 + limit = current_app.config.get('MAX_EXPORT_CELLS', DEFAULT_MAX_EXPORT_CELLS) + cells = len(xlsx_data) * len(xlsx_columns) + if limit and cells > limit: + # Defence in depth for direct API callers; the UI already greyed the + # Excel entry out using total_cells/max_export_cells from /process. + # A refusal, never a silent truncation -- a partial spreadsheet is + # worse than none. + return jsonify( + { + 'error': ( + f'Dataset is {cells} cells, above the Excel export limit ' + f'of {limit}; export CSV or TSV instead.' + ) + } + ), 400 + wb = Workbook() ws = wb.active ws.title = 'Data' @@ -378,16 +449,24 @@ def export_xlsx(): for row in xlsx_data: _append_xlsx_row(ws, [row.get(col, '') for col in xlsx_columns]) + # Normal-mode Workbook and a plain BytesIO: no OS temp files anywhere. + # openpyxl's write_only mode writes worksheet parts to disk, and + # SpooledTemporaryFile is either pointless (its default max_size=0 never + # rolls over, so it is a BytesIO with extra indirection) or disk-backed + # (a non-zero threshold, or any fileno() call, puts payload bytes on + # disk). The cell budget above is what bounds memory (D6). output = io.BytesIO() wb.save(output) + del wb output.seek(0) - return Response( - output.getvalue(), + # send_file streams the buffer out in chunks; getvalue() would make a + # second full copy of the workbook at peak. + return send_file( + output, mimetype='application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', - headers={ - 'Content-Disposition': 'attachment; filename=exported_data.xlsx', - }, + as_attachment=True, + download_name='exported_data.xlsx', ) except HTTPException: diff --git a/static/css/style.css b/static/css/style.css index 1dd04e8..58aa960 100644 --- a/static/css/style.css +++ b/static/css/style.css @@ -954,3 +954,10 @@ th.sort-desc::after { white-space: nowrap; } + +/* Export entry that is out of range for the format (P3/D6). */ +.export-dropdown-item.disabled, +.export-dropdown-item:disabled { + opacity: 0.5; + cursor: not-allowed; +} diff --git a/static/js/app.js b/static/js/app.js index ca28c6e..3da970c 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -25,6 +25,8 @@ const exportBtn = document.getElementById('exportBtn'); // Store data for export and sorting let csvData = null; let csvColumns = null; +let totalCells = 0; +let maxExportCells = 0; let currentColumns = null; let currentRows = null; let currentTotalRows = 0; @@ -138,6 +140,9 @@ async function submitForm(jsonPath) { csvData = data.csv_data; csvColumns = data.csv_columns; + totalCells = data.total_cells || 0; + maxExportCells = data.max_export_cells || 0; + updateExcelAvailability(); renderTable(data.columns, data.preview, data.total_rows); showResults(); @@ -158,8 +163,21 @@ const treeCancelBtn = document.getElementById('treeCancel'); let selectedTreePath = null; +// P4: the picker used to build a DOM node for every key of every object up +// front, so a 10 MB payload meant tens of thousands of nodes in one synchronous +// pass -- a multi-second freeze before the modal appeared. Children are now +// built on first toggle, and both the per-level fan-out and the total node count +// are capped. +const TREE_MAX_CHILDREN = 200; +const TREE_MAX_NODES = 5000; + +// Values are held off-DOM: a node's children cannot be built from markup alone. +const treeNodeValues = new WeakMap(); +let treeNodesBuilt = 0; + function showTreePicker(rawJson) { selectedTreePath = null; + treeNodesBuilt = 0; treeSelectedLabel.textContent = 'No node selected'; treeConfirmBtn.disabled = true; treeContainer.innerHTML = ''; @@ -184,6 +202,8 @@ function describeNode(value) { function buildTreeNode(value, path, keyLabel, openByDefault) { const info = describeNode(value); + treeNodesBuilt += 1; + const node = document.createElement('div'); node.classList.add('tree-node', `tree-${info.kind}`); @@ -203,6 +223,7 @@ function buildTreeNode(value, path, keyLabel, openByDefault) { if (info.selectable) { row.classList.add('tree-selectable'); + row.dataset.path = path; row.addEventListener('click', (e) => { // Don't select when clicking only the toggle chevron if (e.target === toggle && hasChildren) return; @@ -220,35 +241,67 @@ function buildTreeNode(value, path, keyLabel, openByDefault) { if (hasChildren) { const children = document.createElement('div'); - children.classList.add('tree-children'); - if (!openByDefault) children.classList.add('hidden'); - - if (Array.isArray(value)) { - const max = Math.min(value.length, 50); - for (let i = 0; i < max; i++) { - const childPath = path === '(root)' ? String(i) : `${path}.${i}`; - children.appendChild(buildTreeNode(value[i], childPath, `[${i}]`, false)); - } - if (value.length > max) { - const more = document.createElement('div'); - more.classList.add('tree-more'); - more.textContent = `… and ${value.length - max} more items`; - children.appendChild(more); - } - } else { - for (const k of Object.keys(value)) { - const childPath = path === '(root)' ? k : `${path}.${k}`; - children.appendChild(buildTreeNode(value[k], childPath, k, false)); - } - } + children.classList.add('tree-children', 'hidden'); + treeNodeValues.set(children, { value, path }); node.appendChild(children); + if (openByDefault) { + populateChildren(children); + children.classList.remove('hidden'); + } } return node; } +function childEntries(value, path) { + if (Array.isArray(value)) { + return value.map((item, index) => ({ + value: item, + path: path === '(root)' ? String(index) : `${path}.${index}`, + label: `[${index}]`, + })); + } + return Object.keys(value).map(key => ({ + value: value[key], + path: path === '(root)' ? key : `${path}.${key}`, + label: key, + })); +} + +function appendTreeNotice(container, text) { + const notice = document.createElement('div'); + notice.classList.add('tree-more'); + notice.textContent = text; + container.appendChild(notice); +} + +function populateChildren(children) { + if (children.dataset.loaded === 'true') return; + children.dataset.loaded = 'true'; + + const held = treeNodeValues.get(children); + if (!held) return; + + const entries = childEntries(held.value, held.path); + const shown = Math.min(entries.length, TREE_MAX_CHILDREN); + + for (let i = 0; i < shown; i++) { + if (treeNodesBuilt >= TREE_MAX_NODES) { + appendTreeNotice(children, 'Tree size limit reached — narrow the selection above.'); + return; + } + const entry = entries[i]; + children.appendChild(buildTreeNode(entry.value, entry.path, entry.label, false)); + } + + if (entries.length > shown) { + appendTreeNotice(children, `… and ${entries.length - shown} more`); + } +} + function toggleNode(node, toggle) { const children = node.querySelector(':scope > .tree-children'); if (!children) return; + populateChildren(children); const isHidden = children.classList.toggle('hidden'); toggle.textContent = isHidden ? '▸' : '▾'; } @@ -360,6 +413,24 @@ function handleSort(col) { renderTableDOM(currentColumns, sorted, currentTotalRows); } +// P5: a nested object with 50k keys, a 100k-item array stringified whole, or a +// single 5 MB string cell each freeze the tab. The server caps the preview +// projection too (2.4); these caps also protect rows loaded client-side (4.1). +const RENDER_MAX_KEYS = 20; +const RENDER_MAX_ARRAY_ITEMS = 20; +const RENDER_MAX_STRING = 500; + +function truncateForRender(text, max) { + const str = String(text); + if (str.length <= max) return { text: str, truncated: false }; + return { text: str.slice(0, max), truncated: true }; +} + +function renderTruncatable(value, max) { + const { text, truncated } = truncateForRender(value, max); + return escapeHtml(text) + (truncated ? ' … (truncated)' : ''); +} + // Format cell value function formatValue(value) { if (value === null || value === undefined) { @@ -369,10 +440,17 @@ function formatValue(value) { if (typeof value === 'object') { if (Array.isArray(value)) { if (value.length === 0) return '[]'; - if (typeof value[0] === 'object') { + if (typeof value[0] === 'object' && value[0] !== null) { return renderNestedTable(value); } - return escapeHtml(JSON.stringify(value)); + // Stringify only the head of a primitive array: JSON.stringify over a + // 100k-item array produces one huge string in a single . + const head = value.slice(0, RENDER_MAX_ARRAY_ITEMS); + const rendered = escapeHtml(JSON.stringify(head)); + if (value.length > RENDER_MAX_ARRAY_ITEMS) { + return `${rendered} … and ${value.length - RENDER_MAX_ARRAY_ITEMS} more`; + } + return rendered; } return renderNestedObject(value); } @@ -383,7 +461,7 @@ function formatValue(value) { : 'false'; } - return escapeHtml(String(value)); + return renderTruncatable(value, RENDER_MAX_STRING); } // Render nested object as mini table @@ -391,15 +469,19 @@ function renderNestedObject(obj) { const keys = Object.keys(obj); if (keys.length === 0) return '{}'; + const shown = keys.slice(0, RENDER_MAX_KEYS); let html = ''; - keys.forEach(key => { + shown.forEach(key => { const val = obj[key]; let displayVal = val; if (typeof val === 'object' && val !== null) { displayVal = JSON.stringify(val); } - html += ``; + html += ``; }); + if (keys.length > shown.length) { + html += ``; + } html += '
${escapeHtml(key)}${escapeHtml(String(displayVal))}
${escapeHtml(key)}${renderTruncatable(displayVal, RENDER_MAX_STRING)}
… and ${keys.length - shown.length} more keys
'; return html; } @@ -408,29 +490,37 @@ function renderNestedObject(obj) { function renderNestedTable(arr) { if (arr.length === 0) return '[]'; - const cols = [...new Set(arr.flatMap(item => typeof item === 'object' ? Object.keys(item) : []))]; - if (cols.length === 0) return escapeHtml(JSON.stringify(arr)); + const allCols = [...new Set(arr.flatMap(item => (typeof item === 'object' && item !== null) ? Object.keys(item) : []))]; + if (allCols.length === 0) return escapeHtml(JSON.stringify(arr.slice(0, RENDER_MAX_ARRAY_ITEMS))); + const cols = allCols.slice(0, RENDER_MAX_KEYS); let html = ''; cols.forEach(col => { html += ``; }); + if (allCols.length > cols.length) { + html += ``; + } html += ''; arr.slice(0, 5).forEach(item => { html += ''; cols.forEach(col => { - let val = item[col]; + let val = item ? item[col] : undefined; if (typeof val === 'object' && val !== null) { val = JSON.stringify(val); } - html += ``; + html += ``; }); + if (allCols.length > cols.length) { + html += ''; + } html += ''; }); if (arr.length > 5) { - html += ``; + const span = cols.length + (allCols.length > cols.length ? 1 : 0); + html += ``; } html += '
${escapeHtml(col)}… +${allCols.length - cols.length}
${escapeHtml(String(val ?? ''))}${renderTruncatable(val ?? '', RENDER_MAX_STRING)}
... and ${arr.length - 5} more rows
... and ${arr.length - 5} more rows
'; @@ -444,6 +534,28 @@ function escapeHtml(text) { return div.innerHTML; } +// P3/D6: tell the user Excel is out of range before they click, rather than +// after a 400. CSV/TSV are streamed and uncapped, so there is always a way out. +function excelExportBlocked() { + return maxExportCells > 0 && totalCells > maxExportCells; +} + +function updateExcelAvailability() { + const item = document.querySelector('.export-dropdown-item[data-format="xlsx"]'); + if (!item) return; + if (excelExportBlocked()) { + item.disabled = true; + item.classList.add('disabled'); + item.textContent = 'Excel — too large, use CSV/TSV'; + item.title = `${totalCells} cells exceeds the Excel export limit of ${maxExportCells}`; + } else { + item.disabled = false; + item.classList.remove('disabled'); + item.textContent = 'Export Excel'; + item.title = ''; + } +} + // Export dropdown toggle const exportDropdown = document.getElementById('exportDropdown'); exportBtn.addEventListener('click', () => { @@ -473,6 +585,13 @@ document.querySelectorAll('.export-dropdown-item').forEach(item => { } else if (format === 'tsv') { downloadDelimited(csvColumns, csvData, '\t', 'exported_data.tsv'); } else if (format === 'xlsx') { + if (excelExportBlocked()) { + showError( + `Dataset is ${totalCells} cells, above the Excel export limit of ` + + `${maxExportCells}. Export CSV or TSV instead.` + ); + return; + } await exportXlsx(); } }); @@ -503,9 +622,15 @@ function sanitizeCell(value) { return serialized; } -// Pure string builder, kept separate from the download so it can be asserted +// P13: build the file as a list of chunks instead of one giant string. A 10 MB +// dataset otherwise means a ~10-30 MB string plus a Blob copy of it, which +// blocks the main thread and doubles peak memory for no reason -- Blob already +// accepts several parts. +const BLOB_CHUNK_ROWS = 2000; + +// Pure builders, kept separate from the download so they can be asserted // directly (tests/js/test_export_sanitize.mjs). -function buildDelimited(columns, data, delimiter) { +function buildDelimitedChunks(columns, data, delimiter) { const escape = (val) => { const str = String(val ?? ''); if (str.includes(delimiter) || str.includes('"') || str.includes('\n')) { @@ -514,24 +639,33 @@ function buildDelimited(columns, data, delimiter) { return str; }; - let output = columns.map(col => escape(sanitizeCell(col))).join(delimiter) + '\n'; - data.forEach(row => { - const line = columns - .map(col => escape(sanitizeCell(row[col]))) - .join(delimiter); - output += line + '\n'; + const chunks = []; + let pending = columns.map(col => escape(sanitizeCell(col))).join(delimiter) + '\n'; + + data.forEach((row, index) => { + pending += columns.map(col => escape(sanitizeCell(row[col]))).join(delimiter) + '\n'; + if ((index + 1) % BLOB_CHUNK_ROWS === 0) { + chunks.push(pending); + pending = ''; + } }); - return output; + + if (pending) chunks.push(pending); + return chunks; +} + +function buildDelimited(columns, data, delimiter) { + return buildDelimitedChunks(columns, data, delimiter).join(''); } // Client-side CSV/TSV generation function downloadDelimited(columns, data, delimiter, filename) { - const output = buildDelimited(columns, data, delimiter); + const chunks = buildDelimitedChunks(columns, data, delimiter); const mimeType = delimiter === '\t' ? 'text/tab-separated-values; charset=utf-8' : 'text/csv; charset=utf-8'; - const blob = new Blob([output], { type: mimeType }); + const blob = new Blob(chunks, { type: mimeType }); const url = window.URL.createObjectURL(blob); const a = document.createElement('a'); a.href = url; @@ -557,7 +691,10 @@ async function exportXlsx() { }) }); - if (!response.ok) throw new Error('Excel export failed'); + if (!response.ok) { + const detail = await response.json().catch(() => null); + throw new Error((detail && detail.error) || 'Excel export failed'); + } const blob = await response.blob(); const url = window.URL.createObjectURL(blob); diff --git a/tests/js/dom_stub.mjs b/tests/js/dom_stub.mjs index 8b76a3e..251dfbd 100644 --- a/tests/js/dom_stub.mjs +++ b/tests/js/dom_stub.mjs @@ -2,13 +2,25 @@ // assertions on its pure export helpers. No build step, no dependencies: the // script under test is the exact file the browser gets. +const HTML_ESCAPES = { '&': '&', '<': '<', '>': '>' }; + function makeElement() { + // textContent/innerHTML are real accessors so app.js's escapeHtml() -- which + // escapes by round-tripping through a detached div -- behaves as in a browser. + let text = ''; + let html = ''; + const el = { dataset: {}, files: [], style: {}, - textContent: '', - innerHTML: '', + get textContent() { return text; }, + set textContent(value) { + text = String(value); + html = text.replace(/[&<>]/g, (ch) => HTML_ESCAPES[ch]); + }, + get innerHTML() { return html; }, + set innerHTML(value) { html = String(value); }, value: '', disabled: false, classList: { diff --git a/tests/js/test_render_caps.mjs b/tests/js/test_render_caps.mjs new file mode 100644 index 0000000..d66d984 --- /dev/null +++ b/tests/js/test_render_caps.mjs @@ -0,0 +1,122 @@ +// P4/P5/P13 - client-side rendering and export caps. +// +// Loads the real static/js/app.js in a stubbed DOM and asserts the pure render +// helpers bound the work they do. Run with: node tests/js/test_render_caps.mjs + +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; +import vm from 'node:vm'; +import assert from 'node:assert/strict'; +import { createContext } from './dom_stub.mjs'; + +const here = dirname(fileURLToPath(import.meta.url)); +const appJs = readFileSync(join(here, '..', '..', 'static', 'js', 'app.js'), 'utf8'); + +const context = vm.createContext(createContext()); +vm.runInContext(appJs, context, { filename: 'app.js' }); + +const { + formatValue, + renderNestedObject, + renderNestedTable, + truncateForRender, + buildDelimitedChunks, + buildDelimited, + childEntries, + excelExportBlocked, +} = context; + +let checks = 0; +const check = (fn) => { fn(); checks += 1; }; + +// --- P5: nested object key cap ------------------------------------------- +check(() => { + const wide = {}; + for (let i = 0; i < 50000; i += 1) wide[`k${i}`] = i; + const html = renderNestedObject(wide); + const rowCount = (html.match(//g) || []).length; + assert.equal(rowCount, 21, 'expected 20 keys plus one "more" row'); + assert.ok(html.includes('49980 more keys')); +}); + +check(() => { + const small = { a: 1, b: 2 }; + const html = renderNestedObject(small); + assert.equal((html.match(//g) || []).length, 2); + assert.ok(!html.includes('more keys')); +}); + +// --- P5: primitive array stringify cap ------------------------------------ +check(() => { + const big = Array.from({ length: 100000 }, (_, i) => i); + const html = formatValue(big); + assert.ok(html.length < 500, `rendered ${html.length} chars for a 100k array`); + assert.ok(html.includes('and 99980 more')); +}); + +check(() => { + assert.equal(formatValue([1, 2, 3]), '[1,2,3]'); + assert.equal(formatValue([]), '[]'); +}); + +// --- P5: long string cap --------------------------------------------------- +check(() => { + const html = formatValue('z'.repeat(5 * 1024 * 1024)); + assert.ok(html.length < 2000, `rendered ${html.length} chars for a 5 MB string`); + assert.ok(html.includes('truncated')); +}); + +check(() => { + assert.equal(formatValue('short'), 'short'); + assert.equal(truncateForRender('abc', 10).truncated, false); + assert.equal(truncateForRender('abcdef', 3).text, 'abc'); +}); + +// --- P5: nested table column cap ------------------------------------------ +check(() => { + const row = {}; + for (let i = 0; i < 200; i += 1) row[`c${i}`] = i; + const html = renderNestedTable([row, row, row, row, row, row, row]); + assert.equal((html.match(//g) || []).length, 21, '20 columns plus the overflow header'); + assert.ok(html.includes('... and 2 more rows')); +}); + +// --- P4: tree children are enumerable lazily ------------------------------ +check(() => { + const entries = childEntries({ a: 1, b: 2 }, '(root)'); + assert.deepEqual(Array.from(entries, e => e.path), ['a', 'b']); + assert.deepEqual(Array.from(entries, e => e.label), ['a', 'b']); +}); + +check(() => { + const entries = childEntries([10, 20], 'users'); + assert.deepEqual(Array.from(entries, e => e.path), ['users.0', 'users.1']); + assert.deepEqual(Array.from(entries, e => e.label), ['[0]', '[1]']); +}); + +// --- P13: chunked blob parts ---------------------------------------------- +check(() => { + const rows = Array.from({ length: 5000 }, (_, i) => ({ a: i })); + const chunks = buildDelimitedChunks(['a'], rows, ','); + assert.ok(chunks.length > 1, 'expected the output to be split into parts'); + // Joined chunks are byte-identical to the single-string builder. + assert.equal(chunks.join(''), buildDelimited(['a'], rows, ',')); + const lines = chunks.join('').trim().split('\n'); + assert.equal(lines.length, 5001); + assert.equal(lines[1], '0'); + assert.equal(lines[5000], '4999'); +}); + +check(() => { + // A small dataset still produces exactly one part. + assert.equal(buildDelimitedChunks(['a'], [{ a: 1 }], ',').length, 1); +}); + +// --- D6: the Excel entry is gated on the advertised budget ---------------- +check(() => { + assert.equal(typeof excelExportBlocked, 'function'); + assert.equal(excelExportBlocked(), false, 'no data loaded means nothing to block'); +}); + +console.log(`ok - ${checks} client render/cap assertions passed`); diff --git a/tests/test_helpers.py b/tests/test_helpers.py index 51867cf..b5a3fd3 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -3,11 +3,16 @@ import json from helpers import ( + PREVIEW_MAX_ITEMS, + PREVIEW_MAX_STRING, + PREVIEW_TRUNCATION_SUFFIX, extract_by_path, extract_table_data, flatten_for_csv, + flatten_rows, get_all_columns, parse_jsonl, + preview_truncate, ) @@ -176,3 +181,94 @@ def test_shallow_data_is_unaffected_by_the_guard(self): def test_non_dict_at_the_cap_yields_no_rows(self): assert extract_table_data('scalar', _depth=99, max_depth=10) == [] + + +class TestPreviewTruncate: + """P2.2 / P5 - a capped COPY; the original is never touched.""" + + def test_long_string_is_capped(self): + row = {'a': 'x' * 1000} + result = preview_truncate(row) + assert len(result['a']) == PREVIEW_MAX_STRING + len(PREVIEW_TRUNCATION_SUFFIX) + assert result['a'].endswith(PREVIEW_TRUNCATION_SUFFIX) + + def test_short_string_is_untouched(self): + assert preview_truncate({'a': 'short'}) == {'a': 'short'} + + def test_every_column_survives(self): + # Capping row keys would leave the preview table disagreeing with its + # own header, so only the values inside a row are capped. + row = {f'col{i}': i for i in range(60)} + assert list(preview_truncate(row).keys()) == list(row.keys()) + + def test_nested_object_keys_are_capped(self): + row = {'meta': {f'k{i}': i for i in range(100)}} + meta = preview_truncate(row)['meta'] + assert len(meta) == PREVIEW_MAX_ITEMS + 1 + assert '80 more keys' in meta[PREVIEW_TRUNCATION_SUFFIX] + + def test_nested_array_items_are_capped(self): + row = {'tags': list(range(100))} + tags = preview_truncate(row)['tags'] + assert len(tags) == PREVIEW_MAX_ITEMS + 1 + assert tags[:PREVIEW_MAX_ITEMS] == list(range(PREVIEW_MAX_ITEMS)) + assert '80 more items' in tags[-1] + + def test_input_is_not_mutated(self): + nested = {'k': 'y' * 1000, 'items': list(range(100))} + row = {'meta': nested, 'plain': 'z' * 1000} + snapshot = json.dumps(row, sort_keys=True) + + preview_truncate(row) + + assert json.dumps(row, sort_keys=True) == snapshot + assert len(row['meta']['k']) == 1000 + assert row['meta'] is nested + + def test_depth_is_bounded(self): + node = {'leaf': 'v'} + for _ in range(1500): + node = {'a': node} + result = preview_truncate(node) + assert isinstance(result, dict) + + def test_numbers_and_booleans_pass_through(self): + row = {'n': 1, 'f': 2.5, 'b': True, 'null': None} + assert preview_truncate(row) == row + + +class TestFlattenRows: + """P8 - one pass must produce exactly what two passes produced.""" + + CASES = [ + [{'a': 1, 'b': 2}, {'b': 3, 'c': 4}], + [{'z': 1}, {'a': 2}, {'m': 3}], + [{'meta': {'age': 30}, 'name': 'Alice', 'scores': [1, 2]}], + [{'a': {'b': {'c': 1}}}, {'a': {'b': {'d': 2}}}], + [], + [{}], + ] + + def test_matches_the_previous_two_pass_result(self): + for rows in self.CASES: + expected_rows = [flatten_for_csv(row) for row in rows] + expected_columns = get_all_columns(expected_rows) + + actual_rows, actual_columns = flatten_rows(rows) + + assert actual_rows == expected_rows + # Column order must be byte-identical, not merely equivalent. + assert actual_columns == expected_columns + + def test_respects_max_depth(self): + rows = [{'a': {'b': {'c': {'d': 1}}}}] + flattened, columns = flatten_rows(rows, max_depth=2) + assert columns == ['a.b'] + assert isinstance(flattened[0]['a.b'], str) + + def test_non_dict_row_keeps_its_existing_empty_key_behavior(self): + # flatten_for_csv maps a bare scalar to {'': value}, so '' has always been + # a column here. Preserved deliberately: P8 is a refactor, not a fix. + flattened, columns = flatten_rows([{'a': 1}, 'not a dict']) + assert flattened == [{'a': 1}, {'': 'not a dict'}] + assert columns == ['', 'a'] diff --git a/tests/test_routes.py b/tests/test_routes.py index 8753bf8..fad4951 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -7,13 +7,14 @@ import json import logging import re +import tempfile from unittest.mock import MagicMock, patch import pytest from flask import Response, request import config as config_module -from app import create_app +from app import check_rate_limit_topology, create_app, worker_count_from_start_command from extensions import client_ip_key @@ -800,7 +801,13 @@ def test_production_with_empty_key_refuses_to_start(self, fresh_config): create_app(cfg) def test_production_with_real_key_starts(self, fresh_config): - cfg = fresh_config(APP_ENV='production', SECRET_KEY='a-real-random-value') + cfg = fresh_config( + APP_ENV='production', + SECRET_KEY='a-real-random-value', + # A production app must also declare its topology (2.10). + WEB_CONCURRENCY='1', + APP_REPLICAS='1', + ) app = create_app(cfg) assert app.config['SECRET_KEY'] == 'a-real-random-value' @@ -1138,7 +1145,12 @@ def test_local_run_flags(self, fresh_config): assert 'Secure' not in cookie def test_production_sets_secure(self, fresh_config): - cfg = fresh_config(APP_ENV='production', SECRET_KEY='a-real-random-value') + cfg = fresh_config( + APP_ENV='production', + SECRET_KEY='a-real-random-value', + WEB_CONCURRENCY='1', + APP_REPLICAS='1', + ) assert cfg.SESSION_COOKIE_SECURE is True cookie = self._set_cookie(create_app(cfg)) @@ -1279,3 +1291,367 @@ def test_max_age_is_configurable(self, fresh_config): app = create_app(cfg) response = app.test_client().get('/static/js/app.js') assert 'max-age=60' in response.headers['Cache-Control'] + + +class TestStreamingCsvExport: + """P3 - CSV is generator-streamed and stays uncapped.""" + + def test_response_is_streamed(self, app): + client = app.test_client() + with app.test_request_context(): + pass + response = client.post( + '/export-csv', + data=json.dumps( + { + 'csv_data': [{'a': i, 'b': 'x' * 20} for i in range(2000)], + 'csv_columns': ['a', 'b'], + } + ), + content_type='application/json', + ) + assert response.status_code == 200 + # No Content-Length: the body is produced as it is written. + assert 'Content-Length' not in response.headers + rows = list(csv.reader(io.StringIO(response.get_data(as_text=True)))) + assert rows[0] == ['a', 'b'] + assert len(rows) == 2001 + + def test_csv_is_not_capped_by_the_xlsx_budget(self, app): + app.config['MAX_EXPORT_CELLS'] = 10 + response = app.test_client().post( + '/export-csv', + data=json.dumps({'csv_data': [{'a': i} for i in range(200)], 'csv_columns': ['a']}), + content_type='application/json', + ) + assert response.status_code == 200 + assert len(response.get_data(as_text=True).strip().splitlines()) == 201 + + def test_chunk_boundary_does_not_lose_or_duplicate_rows(self, app): + # Exercise exactly the flush boundary of CSV_STREAM_CHUNK_ROWS. + from routes import CSV_STREAM_CHUNK_ROWS + + for count in ( + CSV_STREAM_CHUNK_ROWS - 1, + CSV_STREAM_CHUNK_ROWS, + CSV_STREAM_CHUNK_ROWS + 1, + ): + response = app.test_client().post( + '/export-csv', + data=json.dumps( + {'csv_data': [{'a': i} for i in range(count)], 'csv_columns': ['a']} + ), + content_type='application/json', + ) + rows = list(csv.reader(io.StringIO(response.get_data(as_text=True)))) + assert [r[0] for r in rows[1:]] == [str(i) for i in range(count)], count + + +class TestXlsxExportBudget: + """P3 / D6 - the XLSX guard is on by default, budgeted in cells, never silent.""" + + def test_process_advertises_the_budget(self, client, app): + app.config['MAX_EXPORT_CELLS'] = 1000 + response = client.post( + '/process', + data={ + 'input_method': 'paste', + 'pasted_json': json.dumps([{'a': i, 'b': i} for i in range(5)]), + 'json_path': '(root)', + }, + ) + data = json.loads(response.data) + assert data['total_rows'] == 5 + assert data['total_cells'] == 10 + assert data['max_export_cells'] == 1000 + + def test_existing_keys_are_unchanged(self, client): + response = client.post( + '/process', + data={ + 'input_method': 'paste', + 'pasted_json': '[{"a": 1}]', + 'json_path': '(root)', + }, + ) + data = json.loads(response.data) + for key in ('success', 'columns', 'preview', 'total_rows', 'csv_data', 'csv_columns'): + assert key in data + assert data['total_rows'] == 1 + assert data['columns'] == ['a'] + + def test_oversized_export_is_refused_not_truncated(self, app): + app.config['MAX_EXPORT_CELLS'] = 10 + response = app.test_client().post( + '/export-xlsx', + data=json.dumps( + {'csv_data': [{'a': i, 'b': i} for i in range(20)], 'csv_columns': ['a', 'b']} + ), + content_type='application/json', + ) + assert response.status_code == 400 + error = json.loads(response.data)['error'] + assert '40 cells' in error + assert 'limit of 10' in error + assert 'CSV or TSV' in error + + def test_export_at_the_limit_succeeds(self, app): + app.config['MAX_EXPORT_CELLS'] = 40 + response = app.test_client().post( + '/export-xlsx', + data=json.dumps( + {'csv_data': [{'a': i, 'b': i} for i in range(20)], 'csv_columns': ['a', 'b']} + ), + content_type='application/json', + ) + assert response.status_code == 200 + + def test_zero_disables_the_guard(self, app): + app.config['MAX_EXPORT_CELLS'] = 0 + response = app.test_client().post( + '/export-xlsx', + data=json.dumps({'csv_data': [{'a': i} for i in range(50)], 'csv_columns': ['a']}), + content_type='application/json', + ) + assert response.status_code == 200 + + def test_guard_is_enabled_by_default(self, app): + assert app.config['MAX_EXPORT_CELLS'] > 0 + + def test_export_writes_no_files(self, app, tmp_path, monkeypatch): + """ + D6 / AGENTS.md: no disk writes of payloads. + + openpyxl's write_only mode and a rolled-over SpooledTemporaryFile both + put payload bytes in the OS temp directory. Point every temp mechanism at + an empty directory and assert nothing lands there. + """ + for var in ('TMPDIR', 'TEMP', 'TMP'): + monkeypatch.setenv(var, str(tmp_path)) + monkeypatch.setattr(tempfile, 'tempdir', str(tmp_path)) + + response = app.test_client().post( + '/export-xlsx', + data=json.dumps( + { + 'csv_data': [{'a': f'value-{i}', 'b': i} for i in range(3000)], + 'csv_columns': ['a', 'b'], + } + ), + content_type='application/json', + ) + assert response.status_code == 200 + assert len(response.get_data()) > 0 + assert list(tmp_path.iterdir()) == [] + + +class TestPreviewTruncationDoesNotAffectExports: + """P2.2 / P5 - the preview is capped; csv_data and exports are not.""" + + LONG = 'L' * 2000 + + def _process(self, client): + payload = json.dumps( + [{'text': self.LONG, 'items': list(range(100)), 'meta': {'a': self.LONG}}] + ) + return json.loads( + client.post( + '/process', + data={ + 'input_method': 'paste', + 'pasted_json': payload, + 'json_path': '(root)', + }, + ).data + ) + + def test_preview_is_truncated(self, client): + preview = self._process(client)['preview'][0] + assert preview['text'].endswith('… (truncated)') + assert len(preview['text']) < len(self.LONG) + assert len(preview['items']) == 21 + + def test_csv_data_keeps_full_fidelity(self, client): + csv_data = self._process(client)['csv_data'][0] + assert csv_data['text'] == self.LONG + assert csv_data['meta.a'] == self.LONG + assert json.loads(csv_data['items']) == list(range(100)) + + def test_server_csv_export_is_untruncated(self, client): + data = self._process(client) + response = client.post( + '/export-csv', + data=json.dumps({'csv_data': data['csv_data'], 'csv_columns': data['csv_columns']}), + content_type='application/json', + ) + rows = list(csv.reader(io.StringIO(response.get_data(as_text=True)))) + assert self.LONG in rows[1] + assert '… (truncated)' not in response.get_data(as_text=True) + + def test_xlsx_export_is_untruncated(self, client): + from openpyxl import load_workbook + + data = self._process(client) + response = client.post( + '/export-xlsx', + data=json.dumps({'csv_data': data['csv_data'], 'csv_columns': data['csv_columns']}), + content_type='application/json', + ) + ws = load_workbook(io.BytesIO(response.data)).active + values = [cell.value for cell in ws[2]] + assert self.LONG in values + + +class TestApiFetchDecodesWithoutAnExtraCopy: + """P12 - the streamed bytearray is decoded directly.""" + + @patch('routes.requests.get') + @patch('routes.validate_url') + def test_bytearray_is_decoded_in_place(self, mock_validate, mock_get, client): + mock_validate.return_value = (True, None) + mock_resp = MagicMock() + mock_resp.status_code = 200 + # Several chunks, plus a multi-byte character split across none of them, + # to prove the accumulated bytearray is what gets decoded. + mock_resp.iter_content.return_value = [ + b'[{"name": "Zo', + 'ë'.encode(), + b'"}]', + ] + mock_resp.raise_for_status.return_value = None + mock_get.return_value = mock_resp + + response = client.post( + '/process', + data={ + 'input_method': 'api', + 'api_url': 'https://api.example.com/data', + 'json_path': '(root)', + }, + ) + data = json.loads(response.data) + assert data['success'] is True + assert data['csv_data'][0]['name'] == 'Zoë' + + +class TestRateLimitTopologyGuard: + """ + 2.10 / F12 - memory:// counters are process-local, so the guard must fail + closed rather than let a multi-worker deployment silently enforce N x the + configured limit. + """ + + def test_memory_storage_counters_are_not_shared(self): + """ + The multiplier is real, not theoretical. + + Two limiter storages on memory:// do not see each other's hits, which is + exactly what happens across gunicorn workers and across replicas. + """ + from limits.storage import storage_from_string + + first = storage_from_string('memory://') + second = storage_from_string('memory://') + + for _ in range(5): + first.incr('shared-key', 60) + + assert first.get('shared-key') == 5 + assert second.get('shared-key') == 0 + + def test_storage_uri_is_configurable_at_all(self, fresh_config): + # config.py hardcoded 'memory://' before v1.2, so no deployment could set + # shared storage even if it wanted to (2.10a). + cfg = fresh_config(RATELIMIT_STORAGE_URI='redis://localhost:6379/0') + assert cfg.RATELIMIT_STORAGE_URI == 'redis://localhost:6379/0' + + def test_default_topology_starts_clean(self, fresh_config, caplog): + cfg = fresh_config(APP_ENV=None, WEB_CONCURRENCY=None, APP_REPLICAS=None) + with caplog.at_level(logging.WARNING): + create_app(cfg) + assert 'Rate-limit topology' not in caplog.text + + def test_multi_worker_on_memory_storage_warns_outside_production(self, fresh_config, caplog): + cfg = fresh_config(APP_ENV=None, WEB_CONCURRENCY='4', APP_REPLICAS='1') + with caplog.at_level(logging.WARNING): + create_app(cfg) + assert 'process-local' in caplog.text + assert '4 x 1' in caplog.text + + def test_multi_worker_on_memory_storage_raises_in_production(self, fresh_config): + cfg = fresh_config( + APP_ENV='production', + SECRET_KEY='k', + WEB_CONCURRENCY='4', + APP_REPLICAS='1', + ) + with pytest.raises(RuntimeError, match='process-local'): + create_app(cfg) + + def test_multi_replica_on_memory_storage_raises_in_production(self, fresh_config): + cfg = fresh_config( + APP_ENV='production', + SECRET_KEY='k', + WEB_CONCURRENCY='1', + APP_REPLICAS='3', + ) + with pytest.raises(RuntimeError, match='1 x 3'): + create_app(cfg) + + def test_missing_declaration_raises_in_production(self, fresh_config): + cfg = fresh_config(APP_ENV='production', SECRET_KEY='k', WEB_CONCURRENCY=None) + with pytest.raises(RuntimeError, match='WEB_CONCURRENCY is not declared'): + create_app(cfg) + + def test_missing_replica_declaration_raises_in_production(self, fresh_config): + cfg = fresh_config( + APP_ENV='production', SECRET_KEY='k', WEB_CONCURRENCY='1', APP_REPLICAS=None + ) + with pytest.raises(RuntimeError, match='APP_REPLICAS is not declared'): + create_app(cfg) + + def test_shared_storage_allows_a_declared_multi_worker_topology(self, fresh_config): + cfg = fresh_config( + APP_ENV='production', + SECRET_KEY='k', + WEB_CONCURRENCY='4', + APP_REPLICAS='2', + RATELIMIT_STORAGE_URI='redis://localhost:6379/0', + ) + # Constructed, not connected: Flask-Limiter dials Redis lazily. + app = create_app(cfg) + assert app.config['RATELIMIT_STORAGE_URI'].startswith('redis://') + + def test_start_command_contradicting_the_declaration_raises(self, fresh_config): + cfg = fresh_config( + APP_ENV='production', SECRET_KEY='k', WEB_CONCURRENCY='1', APP_REPLICAS='1' + ) + app = create_app(cfg) + argv = ['/usr/bin/gunicorn', 'app:create_app()', '--workers', '4'] + with pytest.raises(RuntimeError, match='start command runs --workers 4'): + check_rate_limit_topology(app, argv=argv) + + def test_start_command_agreeing_with_the_declaration_is_fine(self, fresh_config): + cfg = fresh_config( + APP_ENV='production', + SECRET_KEY='k', + WEB_CONCURRENCY='4', + APP_REPLICAS='1', + RATELIMIT_STORAGE_URI='redis://localhost:6379/0', + ) + app = create_app(cfg) + check_rate_limit_topology(app, argv=['/usr/bin/gunicorn', '--workers=4']) + + def test_non_gunicorn_argv_is_ignored(self, fresh_config): + cfg = fresh_config() + app = create_app(cfg) + # pytest's own -w-like flags must not be read as a worker declaration. + check_rate_limit_topology(app, argv=['pytest', '-w', '9']) + + def test_worker_count_parsing(self): + assert worker_count_from_start_command(['gunicorn', '--workers', '3']) == 3 + assert worker_count_from_start_command(['gunicorn', '-w', '2']) == 2 + assert worker_count_from_start_command(['/x/gunicorn', '--workers=7']) == 7 + assert worker_count_from_start_command(['gunicorn', '--bind', ':80']) is None + assert worker_count_from_start_command(['gunicorn', '--workers', 'x']) is None + assert worker_count_from_start_command([]) is None From 8e1773b3ae855141f24831e30620ff1c362cd5dc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:59:25 +0000 Subject: [PATCH 20/36] Phase 2.3: record the confirming measurement for MAX_EXPORT_CELLS The 250,000-cell default was derived from a fit; this measures it directly on the worst aspect ratio tested. 83,333 x 3 = 249,999 cells comes back at 138.9 MiB delta / 179.6 MiB absolute against a predicted 138.6, so it passes both halves of the Performance Review section 4 verdict rather than only the extrapolation. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- docs/export-budget-v1.2.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/export-budget-v1.2.md b/docs/export-budget-v1.2.md index 181f5b7..678345a 100644 --- a/docs/export-budget-v1.2.md +++ b/docs/export-budget-v1.2.md @@ -41,6 +41,7 @@ would not mask the workbook cost. | rows × cols | cells | delta (MiB) | absolute (MiB) | |---|---|---|---| | 15,000 × 10 | 150,000 | 73.6 | 114.3 | +| 83,333 × 3 | 249,999 | **138.9** | 179.6 | | 50,000 × 3 | 150,000 | **84.3** | 125.0 | | 20,000 × 10 | 200,000 | 98.6 | 139.2 | | 2,000 × 150 | 300,000 | 137.3 | 178.0 | @@ -71,9 +72,13 @@ intercept = 2.8 MiB The 274,998-cell run measured **152.2 MiB — over the 150 MiB target**, so the crossing is real and not an artifact of extrapolation. The budget is set at **250,000 cells**, roughly 8% below the crossing, which is the margin -run-to-run variance on these measurements needs. Predicted delta at that size is -≈ 138.6 MiB, and the measured absolute high-water mark at nearby sizes stays -well under the 256 MiB (half of a 512 MiB container) ceiling. +run-to-run variance on these measurements needs. + +That value was then measured directly rather than left as an extrapolation: +83,333 × 3 = 249,999 cells came back at **138.9 MiB delta / 179.6 MiB absolute** +(the fit predicted 138.6). It passes both halves of the §4 verdict — under the +150 MiB delta target, and well under the 256 MiB absolute ceiling for a 512 MiB +container. ## What the budget does and does not cover From b1f6a23d6025ea042de4a48ed1da881d3d2f623d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 23:02:52 +0000 Subject: [PATCH 21/36] Phase 3.1-3.5: split process_json, add annotations, return preview_limit 3.1: process_json had grown to ~207 lines with the whole file/paste/API ladder inline. The three loaders now sit behind _load_input(data_format) and the path handling behind _select_table_data(data, path), each returning (value, error_response) so the route reads as a straight line. _build_api_auth and _parse_payload came out of the API branch. The route body is 40 code lines; a test asserts it stays under 50. 3.2: consolidation only, as the plan requires -- no new helper is extracted here. serialize_cell_value/sanitize_cell were created in 1.1 and preview_truncate in 2.4, in the phases that first needed them, which is what removed the ordering cycle where 1.1 and 2.4 would have depended on a helper scheduled for a later phase. This pass only tidies signatures and docstrings. 3.3: /process returns preview_limit and the badge uses it. It hardcoded "Showing first 25", so changing PREVIEW_ROW_LIMIT gave a wrong badge (P11). The badge also now states the sort scope -- sorting reorders the rows in the table while exports always contain every row, a discrepancy that was previously invisible. 3.4: openpyxl imports at module top, so a missing dependency fails at startup instead of on the first Excel export. 3.5: type annotations on every helpers.py and security.py signature. mypy is not wired in; strict mode is out of scope per the plan. Behavior is unchanged: tests assert no existing /process key changed name, type or meaning, that the only additions are preview_limit, total_cells and max_export_cells, that the tree-picker handshake is byte-identical, and that every error path keeps its message. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- helpers.py | 39 +++-- routes.py | 378 +++++++++++++++++++++++++------------------ security.py | 19 +-- static/js/app.js | 15 +- tests/test_routes.py | 87 ++++++++++ 5 files changed, 356 insertions(+), 182 deletions(-) diff --git a/helpers.py b/helpers.py index 8889b29..ff50815 100644 --- a/helpers.py +++ b/helpers.py @@ -1,9 +1,16 @@ """Data processing helpers for JSON flattening and table extraction.""" import json +from typing import Any -def flatten_for_csv(data, parent_key='', sep='.', _depth=0, max_depth=10): +def flatten_for_csv( + data: Any, + parent_key: str = '', + sep: str = '.', + _depth: int = 0, + max_depth: int = 10, +) -> dict[str, Any]: """ Flatten nested dictionaries for CSV export. Arrays are converted to JSON strings. @@ -38,7 +45,7 @@ def flatten_for_csv(data, parent_key='', sep='.', _depth=0, max_depth=10): FORMULA_TRIGGERS = ('=', '+', '-', '@', '\t', '\r', '\n') -def serialize_cell_value(value): +def serialize_cell_value(value: Any) -> Any: """ Reduce one cell value to the scalar an export writer can emit. @@ -50,12 +57,12 @@ def serialize_cell_value(value): return value -def is_formula_trigger(value): +def is_formula_trigger(value: Any) -> bool: """True when a serialized value would be read as a formula by a spreadsheet.""" return isinstance(value, str) and value.startswith(FORMULA_TRIGGERS) -def sanitize_cell(value): +def sanitize_cell(value: Any) -> Any: """ Serialize a cell for the delimited formats (CSV/TSV) and defuse formula injection (CWE-1236). @@ -72,7 +79,7 @@ def sanitize_cell(value): return serialized -def flatten_rows(rows, max_depth=10): +def flatten_rows(rows: list[Any], max_depth: int = 10) -> tuple[list[dict[str, Any]], list[str]]: """ Flatten every row and collect the column names in a single pass. @@ -93,7 +100,7 @@ def flatten_rows(rows, max_depth=10): return flattened, sorted(columns) -def extract_table_data(json_data, _depth=0, max_depth=10): +def extract_table_data(json_data: Any, _depth: int = 0, max_depth: int = 10) -> list[Any]: """ Extract tabular data from JSON. Handles arrays of objects, nested arrays, and single objects. @@ -132,7 +139,7 @@ def extract_table_data(json_data, _depth=0, max_depth=10): return [] -def parse_jsonl(text): +def parse_jsonl(text: str) -> list[Any]: """ Parse JSONL (JSON Lines) text into a list of objects. Each non-empty line is parsed as a separate JSON value. @@ -149,7 +156,7 @@ def parse_jsonl(text): return rows -def extract_by_path(json_data, path): +def extract_by_path(json_data: Any, path: str) -> Any: """ Extract data from JSON using a dot-notation path. Numeric parts traverse arrays (e.g. 'data.0.orders'). @@ -176,7 +183,7 @@ def extract_by_path(json_data, path): return current -def get_all_columns(data): +def get_all_columns(data: list[Any]) -> list[str]: """Get all unique column names from the data, sorted alphabetically.""" columns = set() for row in data: @@ -197,7 +204,9 @@ def get_all_columns(data): PREVIEW_TRUNCATION_SUFFIX = '… (truncated)' -def _truncate_preview_value(value, max_string, max_items, depth, max_depth): +def _truncate_preview_value( + value: Any, max_string: int, max_items: int, depth: int, max_depth: int +) -> Any: """Return a capped copy of one nested value.""" if isinstance(value, str): if len(value) > max_string: @@ -232,11 +241,11 @@ def _truncate_preview_value(value, max_string, max_items, depth, max_depth): def preview_truncate( - row, - max_string=PREVIEW_MAX_STRING, - max_items=PREVIEW_MAX_ITEMS, - max_depth=10, -): + row: Any, + max_string: int = PREVIEW_MAX_STRING, + max_items: int = PREVIEW_MAX_ITEMS, + max_depth: int = 10, +) -> Any: """ Build a capped COPY of one preview row. diff --git a/routes.py b/routes.py index 2aba963..dce17eb 100644 --- a/routes.py +++ b/routes.py @@ -16,6 +16,7 @@ request, send_file, ) +from openpyxl import Workbook from requests.auth import HTTPBasicAuth from werkzeug.exceptions import HTTPException @@ -118,165 +119,230 @@ def health(): return jsonify(payload) -@bp.route('/process', methods=['POST']) -@limiter.limit(lambda: current_app.config.get('RATE_LIMIT_PROCESS', '30/minute')) -def process_json(): - """Process JSON data from file upload, pasted text, or API fetch.""" - try: - input_method = request.form.get('input_method') - json_data = None - - data_format = request.form.get('data_format', 'json') +def _build_api_auth(auth_method): + """ + Build the outbound headers/auth/params for one auth method. - if input_method == 'file': - if 'json_file' not in request.files: - return jsonify({'error': 'No file uploaded'}), 400 - file = request.files['json_file'] - if file.filename == '': - return jsonify({'error': 'No file selected'}), 400 - upload_error = validate_upload(file) - if upload_error: - return jsonify({'error': upload_error}), 400 - try: - content = file.read().decode('utf-8') - json_data = parse_jsonl(content) if data_format == 'jsonl' else json.loads(content) - except UnicodeDecodeError: - return jsonify({'error': 'File must be UTF-8 encoded'}), 400 - except (json.JSONDecodeError, ValueError) as e: - return jsonify({'error': f'Invalid data in file: {str(e)}'}), 400 - - elif input_method == 'paste': - pasted_json = request.form.get('pasted_json', '').strip() - if not pasted_json: - return jsonify({'error': 'No JSON provided'}), 400 - try: - if data_format == 'jsonl': - json_data = parse_jsonl(pasted_json) - else: - json_data = json.loads(pasted_json) - except (json.JSONDecodeError, ValueError) as e: - return jsonify({'error': f'Invalid data: {str(e)}'}), 400 - - elif input_method == 'api': - api_url = request.form.get('api_url', '').strip() - if not api_url: - return jsonify({'error': 'No API URL provided'}), 400 - - is_valid, error_msg = validate_url(api_url) - if not is_valid: - return jsonify({'error': error_msg}), 400 - - auth_method = request.form.get('auth_method', 'none') - headers = {} - auth = None - params = {} - - if auth_method == 'api_key': - header_name = request.form.get('api_key_header', 'X-API-Key') - api_key = request.form.get('api_key', '') - if api_key: - if not is_allowed_outbound_header(header_name): - return jsonify( + Returns (headers, auth, params, error_response). error_response is None + unless the client asked for something we refuse to forward. + """ + headers = {} + auth = None + params = {} + + if auth_method == 'api_key': + header_name = request.form.get('api_key_header', 'X-API-Key') + api_key = request.form.get('api_key', '') + if api_key: + if not is_allowed_outbound_header(header_name): + return ( + None, + None, + None, + ( + jsonify( { 'error': 'Header name is not permitted. Allowed: ' + ', '.join(sorted(ALLOWED_OUTBOUND_HEADERS)) } - ), 400 - headers[header_name.strip()] = api_key - elif auth_method == 'basic': - username = request.form.get('basic_username', '') - password = request.form.get('basic_password', '') - if username: - auth = HTTPBasicAuth(username, password) - elif auth_method == 'bearer': - bearer_token = request.form.get('bearer_token', '') - if bearer_token: - headers['Authorization'] = f'Bearer {bearer_token}' - elif auth_method == 'query_param': - param_name = request.form.get('query_param_name', 'api_key') - param_value = request.form.get('query_param_value', '') - if param_value: - params[param_name] = param_value - - try: - timeout = current_app.config['API_FETCH_TIMEOUT'] - max_size = current_app.config['API_FETCH_MAX_RESPONSE'] - - # Use original URL to preserve TLS/SNI verification. - # SSRF mitigated by: pre-request DNS validation + disabled redirects. - # Residual DNS rebinding risk is minimal (requires attacker-controlled - # DNS with sub-millisecond TTL between our check and requests' connect). - resp = requests.get( - api_url, - headers=headers, - auth=auth, - params=params, - timeout=timeout, - stream=True, - allow_redirects=False, + ), + 400, + ), ) - resp.raise_for_status() + headers[header_name.strip()] = api_key + elif auth_method == 'basic': + username = request.form.get('basic_username', '') + password = request.form.get('basic_password', '') + if username: + auth = HTTPBasicAuth(username, password) + elif auth_method == 'bearer': + bearer_token = request.form.get('bearer_token', '') + if bearer_token: + headers['Authorization'] = f'Bearer {bearer_token}' + elif auth_method == 'query_param': + param_name = request.form.get('query_param_name', 'api_key') + param_value = request.form.get('query_param_value', '') + if param_value: + params[param_name] = param_value + + return headers, auth, params, None + + +def _parse_payload(text, data_format): + """Parse a payload as JSON or JSONL according to the requested format.""" + return parse_jsonl(text) if data_format == 'jsonl' else json.loads(text) + + +def _load_from_file(data_format): + """Read and parse an uploaded file. Returns (data, error_response).""" + if 'json_file' not in request.files: + return None, (jsonify({'error': 'No file uploaded'}), 400) + + file = request.files['json_file'] + if file.filename == '': + return None, (jsonify({'error': 'No file selected'}), 400) + + upload_error = validate_upload(file) + if upload_error: + return None, (jsonify({'error': upload_error}), 400) - content = bytearray() - for chunk in resp.iter_content(chunk_size=8192): - content.extend(chunk) - if len(content) > max_size: - return jsonify( - { - 'error': f'API response exceeds maximum size ' - f'({max_size // (1024 * 1024)}MB)' - } - ), 400 - - # bytearray decodes directly; bytes(content) made a second full - # copy of the response body at peak (P12). Parsing, flattening and - # jsonify still materialize the dataset -- this removes one copy, - # it does not make the pipeline low-memory. - text = content.decode('utf-8') - json_data = parse_jsonl(text) if data_format == 'jsonl' else json.loads(text) - - except requests.exceptions.Timeout: - return jsonify({'error': 'API request timed out'}), 400 - except requests.exceptions.RequestException: - # Fixed message, no interpolation: requests' exception text carries - # the full URL, and the query string, fragment, userinfo AND path - # can each hold a token (F3/F9). Redacting one component is not - # enough, so nothing user-controlled is logged at all. - logger.warning('API request failed') - return jsonify({'error': 'API request failed'}), 400 - except json.JSONDecodeError: - return jsonify({'error': 'API response is not valid JSON'}), 400 - except ValueError: - # parse_jsonl raises ValueError on a malformed line. Without this - # it reached the outer handler as a 500 with a logged traceback, - # although it is the caller's data that is wrong (F9). - return jsonify({'error': 'API response is not valid JSONL'}), 400 - else: - return jsonify({'error': 'Invalid input method'}), 400 - - # Check if user selected a specific JSON path - json_path = request.form.get('json_path', '') - - if json_path: - selected = extract_by_path(json_data, json_path) - if selected is None: - return jsonify({'error': f'Path "{json_path}" not found'}), 400 - if isinstance(selected, list): - table_data = extract_table_data( - selected, max_depth=current_app.config['FLATTEN_MAX_DEPTH'] + try: + content = file.read().decode('utf-8') + return _parse_payload(content, data_format), None + except UnicodeDecodeError: + return None, (jsonify({'error': 'File must be UTF-8 encoded'}), 400) + except (json.JSONDecodeError, ValueError) as e: + return None, (jsonify({'error': f'Invalid data in file: {e}'}), 400) + + +def _load_from_paste(data_format): + """Parse pasted text. Returns (data, error_response).""" + pasted_json = request.form.get('pasted_json', '').strip() + if not pasted_json: + return None, (jsonify({'error': 'No JSON provided'}), 400) + + try: + return _parse_payload(pasted_json, data_format), None + except (json.JSONDecodeError, ValueError) as e: + return None, (jsonify({'error': f'Invalid data: {e}'}), 400) + + +def _load_from_api(data_format): + """Fetch and parse a remote payload. Returns (data, error_response).""" + api_url = request.form.get('api_url', '').strip() + if not api_url: + return None, (jsonify({'error': 'No API URL provided'}), 400) + + is_valid, error_msg = validate_url(api_url) + if not is_valid: + return None, (jsonify({'error': error_msg}), 400) + + headers, auth, params, error_response = _build_api_auth(request.form.get('auth_method', 'none')) + if error_response: + return None, error_response + + try: + timeout = current_app.config['API_FETCH_TIMEOUT'] + max_size = current_app.config['API_FETCH_MAX_RESPONSE'] + + # Use original URL to preserve TLS/SNI verification. + # SSRF mitigated by: pre-request DNS validation + disabled redirects. + # Residual DNS rebinding risk is minimal (requires attacker-controlled + # DNS with sub-millisecond TTL between our check and requests' connect). + resp = requests.get( + api_url, + headers=headers, + auth=auth, + params=params, + timeout=timeout, + stream=True, + allow_redirects=False, + ) + resp.raise_for_status() + + content = bytearray() + for chunk in resp.iter_content(chunk_size=8192): + content.extend(chunk) + if len(content) > max_size: + return None, ( + jsonify( + { + 'error': f'API response exceeds maximum size ' + f'({max_size // (1024 * 1024)}MB)' + } + ), + 400, ) - elif isinstance(selected, dict): - table_data = [selected] - else: - return jsonify( - {'error': f'Path "{json_path}" is a primitive value; pick an object or array'} - ), 400 - else: - # No path chosen yet — let the client render a tree picker - return jsonify({'needs_selection': True, 'raw_json': json_data}) - - if not table_data: - return jsonify({'error': 'Could not extract tabular data from JSON'}), 400 + + # bytearray decodes directly; bytes(content) made a second full copy of + # the response body at peak (P12). Parsing, flattening and jsonify still + # materialize the dataset -- this removes one copy, it does not make the + # pipeline low-memory. + return _parse_payload(content.decode('utf-8'), data_format), None + + except requests.exceptions.Timeout: + return None, (jsonify({'error': 'API request timed out'}), 400) + except requests.exceptions.RequestException: + # Fixed message, no interpolation: requests' exception text carries the + # full URL, and the query string, fragment, userinfo AND path can each + # hold a token (F3/F9). Redacting one component is not enough, so nothing + # user-controlled is logged at all. + logger.warning('API request failed') + return None, (jsonify({'error': 'API request failed'}), 400) + except json.JSONDecodeError: + return None, (jsonify({'error': 'API response is not valid JSON'}), 400) + except ValueError: + # parse_jsonl raises ValueError on a malformed line. Without this it + # reached the outer handler as a 500 with a logged traceback, although it + # is the caller's data that is wrong (F9). + return None, (jsonify({'error': 'API response is not valid JSONL'}), 400) + + +def _load_input(data_format): + """ + Dispatch to the requested input method. Returns (data, error_response). + + Exactly one of the two is meaningful: a non-None error_response is the + caller's return value. + """ + loaders = { + 'file': _load_from_file, + 'paste': _load_from_paste, + 'api': _load_from_api, + } + loader = loaders.get(request.form.get('input_method')) + if loader is None: + return None, (jsonify({'error': 'Invalid input method'}), 400) + return loader(data_format) + + +def _select_table_data(json_data, json_path): + """ + Turn the chosen JSON path into table rows. Returns (rows, error_response). + + With no path the client has not chosen a node yet, so the whole document + goes back for the tree picker -- that 200 is a response, not an error, and + rides in the error_response slot because it is equally terminal. + """ + if not json_path: + return None, (jsonify({'needs_selection': True, 'raw_json': json_data}), 200) + + selected = extract_by_path(json_data, json_path) + if selected is None: + return None, (jsonify({'error': f'Path "{json_path}" not found'}), 400) + + if isinstance(selected, list): + rows = extract_table_data(selected, max_depth=current_app.config['FLATTEN_MAX_DEPTH']) + elif isinstance(selected, dict): + rows = [selected] + else: + return None, ( + jsonify({'error': f'Path "{json_path}" is a primitive value; pick an object or array'}), + 400, + ) + + if not rows: + return None, (jsonify({'error': 'Could not extract tabular data from JSON'}), 400) + + return rows, None + + +@bp.route('/process', methods=['POST']) +@limiter.limit(lambda: current_app.config.get('RATE_LIMIT_PROCESS', '30/minute')) +def process_json(): + """Process JSON data from file upload, pasted text, or API fetch.""" + try: + data_format = request.form.get('data_format', 'json') + + json_data, error_response = _load_input(data_format) + if error_response: + return error_response + + table_data, error_response = _select_table_data( + json_data, request.form.get('json_path', '') + ) + if error_response: + return error_response columns = get_all_columns(table_data) preview_limit = current_app.config['PREVIEW_ROW_LIMIT'] @@ -284,20 +350,22 @@ def process_json(): # untouched rows, so exports stay full-fidelity (P2.2/P5). preview_data = [preview_truncate(row) for row in table_data[:preview_limit]] - max_depth = current_app.config['FLATTEN_MAX_DEPTH'] # One pass instead of flatten-then-rescan: names are collected into a set # while flattening and sorted once at the end, which is byte-identical to # get_all_columns' sorted output (P8). - csv_data, csv_columns = flatten_rows(table_data, max_depth=max_depth) + csv_data, csv_columns = flatten_rows( + table_data, max_depth=current_app.config['FLATTEN_MAX_DEPTH'] + ) # Additive only: no existing key changes name, type or meaning. # total_cells/max_export_cells let the client grey out the Excel entry - # BEFORE the user clicks, rather than after a 400 (D6). + # BEFORE the user clicks (D6); preview_limit drives the badge (P11). return jsonify( { 'success': True, 'columns': columns, 'preview': preview_data, + 'preview_limit': preview_limit, 'total_rows': len(table_data), 'total_cells': len(csv_data) * len(csv_columns), 'max_export_cells': current_app.config.get( @@ -413,8 +481,6 @@ def export_csv(): def export_xlsx(): """Export data as Excel file.""" try: - from openpyxl import Workbook - data = request.get_json(silent=True) if not data: return jsonify({'error': 'Invalid or missing JSON body'}), 400 diff --git a/security.py b/security.py index 89c53ed..ccd7b2c 100644 --- a/security.py +++ b/security.py @@ -5,6 +5,7 @@ import os import socket import threading +from typing import Any from urllib.parse import urlparse from flask import current_app, has_app_context, request @@ -84,7 +85,7 @@ class ResolverBusyError(Exception): class _ResolverPool: """A fixed-size resolver pool with an admission permit per in-flight lookup.""" - def __init__(self, max_workers): + def __init__(self, max_workers: int) -> None: self.pid = os.getpid() self.max_workers = max_workers # Pool size plus an equal backlog: a bounded submission queue. Beyond this @@ -97,7 +98,7 @@ def __init__(self, max_workers): self._counter_lock = threading.Lock() self.in_flight = 0 - def submit(self, hostname, admission_timeout): + def submit(self, hostname: str, admission_timeout: float) -> concurrent.futures.Future: """ Admit and start one lookup, or raise ResolverBusyError. @@ -119,10 +120,10 @@ def submit(self, hostname, admission_timeout): future.add_done_callback(self._on_done) return future - def _on_done(self, _future): + def _on_done(self, _future: concurrent.futures.Future) -> None: self._release() - def _release(self): + def _release(self) -> None: with self._counter_lock: self.in_flight -= 1 self._permits.release() @@ -132,7 +133,7 @@ def _release(self): _pool = None -def get_resolver_pool(max_workers=None): +def get_resolver_pool(max_workers: int | None = None) -> '_ResolverPool': """ Return this process's resolver pool, creating it on first use. @@ -153,7 +154,7 @@ def get_resolver_pool(max_workers=None): return _pool -def reset_resolver_pool(): +def reset_resolver_pool() -> '_ResolverPool | None': """ Drop the current pool so the next lookup builds a fresh one. @@ -170,14 +171,14 @@ def reset_resolver_pool(): return pool -def _setting(name, default): +def _setting(name: str, default: Any) -> Any: """Read a config value, falling back to the module default outside a request.""" if has_app_context(): return current_app.config.get(name, default) return default -def resolve_hostname(hostname): +def resolve_hostname(hostname: str) -> list[Any]: """ Resolve a hostname under admission control. @@ -192,7 +193,7 @@ def resolve_hostname(hostname): return future.result(timeout=timeout) -def validate_url(url): +def validate_url(url: str) -> tuple[bool, str | None]: """ Validate a URL for SSRF protection. Resolves DNS and rejects non-global IPs. diff --git a/static/js/app.js b/static/js/app.js index 3da970c..573cf6b 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -27,6 +27,7 @@ let csvData = null; let csvColumns = null; let totalCells = 0; let maxExportCells = 0; +let previewLimit = 25; let currentColumns = null; let currentRows = null; let currentTotalRows = 0; @@ -142,6 +143,9 @@ async function submitForm(jsonPath) { csvColumns = data.csv_columns; totalCells = data.total_cells || 0; maxExportCells = data.max_export_cells || 0; + // P11: the badge used to hardcode 25, so changing PREVIEW_ROW_LIMIT gave + // an operator a wrong badge. + previewLimit = data.preview_limit || previewLimit; updateExcelAvailability(); renderTable(data.columns, data.preview, data.total_rows); @@ -371,11 +375,18 @@ function renderTableDOM(columns, rows, totalRows) { // Update counts rowCountText.textContent = `${totalRows} total rows`; - if (totalRows > 25) { - previewBadge.textContent = 'Showing first 25'; + if (rows.length < totalRows) { + previewBadge.textContent = `Showing first ${rows.length}`; + // Sorting reorders only the rows currently in the table, while exports + // always contain every row -- say so rather than leaving the discrepancy + // invisible (P11). + previewBadge.title = + `Preview limit is ${previewLimit}. Sorting applies to the rows shown here; ` + + 'exports always contain all rows.'; previewBadge.classList.remove('hidden'); } else { previewBadge.textContent = `Showing all ${totalRows}`; + previewBadge.title = ''; previewBadge.classList.add('hidden'); } } diff --git a/tests/test_routes.py b/tests/test_routes.py index fad4951..57ad5f5 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -1655,3 +1655,90 @@ def test_worker_count_parsing(self): assert worker_count_from_start_command(['gunicorn', '--bind', ':80']) is None assert worker_count_from_start_command(['gunicorn', '--workers', 'x']) is None assert worker_count_from_start_command([]) is None + + +class TestProcessResponseShape: + """3.1-3.3 - the refactor is behavior-preserving and additive only.""" + + EXISTING_KEYS = { + 'success': bool, + 'columns': list, + 'preview': list, + 'total_rows': int, + 'csv_data': list, + 'csv_columns': list, + } + + def _process(self, client, **extra): + data = { + 'input_method': 'paste', + 'pasted_json': json.dumps([{'b': 2, 'a': 1}, {'a': 3, 'c': 4}]), + 'json_path': '(root)', + } + data.update(extra) + return json.loads(client.post('/process', data=data).data) + + def test_no_existing_key_changed_name_type_or_meaning(self, client): + payload = self._process(client) + for key, expected_type in self.EXISTING_KEYS.items(): + assert key in payload, key + assert isinstance(payload[key], expected_type), key + + assert payload['total_rows'] == 2 + assert payload['columns'] == ['a', 'b', 'c'] + assert payload['csv_columns'] == ['a', 'b', 'c'] + assert payload['csv_data'] == [{'b': 2, 'a': 1}, {'a': 3, 'c': 4}] + + def test_preview_limit_is_returned(self, client, app): + app.config['PREVIEW_ROW_LIMIT'] = 7 + payload = self._process(client) + assert payload['preview_limit'] == 7 + + def test_new_keys_are_the_only_additions(self, client): + payload = self._process(client) + added = set(payload) - set(self.EXISTING_KEYS) + assert added == {'preview_limit', 'total_cells', 'max_export_cells'} + + def test_tree_picker_handshake_is_unchanged(self, client): + payload = json.loads( + client.post( + '/process', + data={'input_method': 'paste', 'pasted_json': '{"a": [{"x": 1}]}'}, + ).data + ) + assert payload == {'needs_selection': True, 'raw_json': {'a': [{'x': 1}]}} + + def test_all_error_paths_still_return_their_messages(self, client): + cases = [ + ({'input_method': 'unknown'}, 'Invalid input method'), + ({'input_method': 'paste', 'pasted_json': ''}, 'No JSON provided'), + ({'input_method': 'api', 'api_url': ''}, 'No API URL provided'), + ( + {'input_method': 'paste', 'pasted_json': '{"a": 1}', 'json_path': 'zz'}, + 'not found', + ), + ( + {'input_method': 'paste', 'pasted_json': '{"a": 1}', 'json_path': 'a'}, + 'primitive value', + ), + ] + for data, expected in cases: + response = client.post('/process', data=data) + assert response.status_code == 400, data + assert expected in json.loads(response.data)['error'], data + + def test_process_json_stays_small(self): + """3.1 - the extraction is the point; guard against it creeping back.""" + import ast + import inspect + + import routes + + source = inspect.getsource(routes) + tree = ast.parse(source) + node = next( + n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef) and n.name == 'process_json' + ) + lines = source.splitlines()[node.lineno - 1 : node.end_lineno] + code_lines = [ln for ln in lines if ln.strip() and not ln.strip().startswith('#')] + assert len(code_lines) <= 50, f'process_json is {len(code_lines)} code lines' From 213da855480723bf8083ab2506f2c28857e47303 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 23:12:15 +0000 Subject: [PATCH 22/36] Phase 4.1-4.5, 4.7, 4.8: client features and the /health split 4.6 (opt-in Basic Auth) is deliberately NOT implemented: D4 is still open and needs maintainer sign-off. Nothing else in v1.2 depends on it. 4.1 Load more / pagination. csv_data is already in the browser, so "Load next 500" and "Load all" need no server round trip. Rows past the preview come from the flattened dataset -- the only one the browser holds for every row -- so the table switches to flattened columns at that point and the badge says so, since a nested `meta` object becomes `meta.age`. A 50,000-row DOM guard warns and stops rendering rather than freezing the tab; every export still contains all rows. Sorting now covers the loaded set rather than only the 25 preview rows (P2.3, P11). 4.2 Row filter. Case-insensitive substring across every value in a row, nested values included, with a match count. 4.3 JSONL and Markdown exports. Both deliberately bypass the F1 spreadsheet sanitizer, for different reasons: JSON has types and nothing evaluates it, so a quote prefix would corrupt data rather than protect anything; Markdown does not evaluate a leading '=' either, but an unescaped pipe or newline breaks the table, so it gets Markdown-specific escaping instead. Tests assert a '=SUM(A1)' value survives both exports verbatim and that neither carries the CSV quote prefix. 4.4 Column visibility toggle, driven off whichever column set is active. 4.5 Deep-linkable path selection. #path=users.0.orders pre-selects that node when the picker opens, expanding each ancestor through the lazy builder from 2.5; confirming a selection writes the hash back so the link is shareable. A path that does not resolve -- or sits past a lazy cap -- leaves the picker open rather than failing. 4.7 /health/live and /health/ready. Liveness deliberately checks nothing, so a dependency outage cannot cause a restart loop; readiness checks the limiter storage and the Excel writer and returns 503 when it cannot serve, keeping the failure reason in the logs rather than the body. /health keeps its exact existing contract. 4.8 alert() is gone. The About dialog is an in-page modal reading APP_VERSION from config, so it cannot go stale the way the hardcoded "v1.1.0" string had. The export dropdown gained aria-haspopup, aria-expanded, role=menu/menuitem, arrow-key navigation and Escape handling (Escape also closes the columns dropdown and both modals). Verified end to end in Chromium against a running server: lazy tree opens with 3 nodes, load-more goes 25 -> 525 rows, filtering, column hiding, sorting, Escape handling, the About modal, all four client exports downloading full 1200-row files, and hash preselection. No JavaScript errors. No new dependencies, no inline JS or CSS, CSP intact. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019u9FmB1MfFVZmh61R9S37m --- .github/workflows/ci.yml | 3 + routes.py | 57 ++++- security.py | 2 + static/css/style.css | 86 +++++++ static/js/app.js | 502 +++++++++++++++++++++++++++++++++---- templates/index.html | 45 +++- tests/js/dom_stub.mjs | 3 + tests/js/test_features.mjs | 129 ++++++++++ tests/test_routes.py | 82 ++++++ 9 files changed, 850 insertions(+), 59 deletions(-) create mode 100644 tests/js/test_features.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f4efe38..31643df 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,5 +39,8 @@ jobs: - name: Client-side render/cap assertions (P4/P5/P13) run: node tests/js/test_render_caps.mjs + - name: Client-side feature assertions (Phase 4) + run: node tests/js/test_features.mjs + - name: Audit runtime dependencies run: pip-audit -r requirements.txt diff --git a/routes.py b/routes.py index dce17eb..e31af9a 100644 --- a/routes.py +++ b/routes.py @@ -110,13 +110,60 @@ def index(): return render_template('index.html') -@bp.route('/health') -def health(): - """Health check endpoint.""" - payload = {'status': 'ok'} +def _health_payload(status='ok', **extra): + payload = {'status': status} if current_app.config.get('HEALTH_REVEAL_VERSION', True): payload['version'] = current_app.config['APP_VERSION'] - return jsonify(payload) + payload.update(extra) + return payload + + +@bp.route('/health') +def health(): + """Health check endpoint (unchanged contract).""" + return jsonify(_health_payload()) + + +@bp.route('/health/live') +def health_live(): + """ + Liveness: is the process up at all? + + Deliberately does no dependency work, so a restart loop caused by a failing + dependency check is impossible. + """ + return jsonify(_health_payload()) + + +@bp.route('/health/ready') +def health_ready(): + """ + Readiness: is this process able to serve traffic? + + Checks only what is cheap and local -- that the rate limiter has usable + storage and that the Excel writer imported. Returns 503 when it cannot serve, + so a load balancer takes it out of rotation rather than sending it requests. + """ + checks = {} + + storage_uri = current_app.config.get('RATELIMIT_STORAGE_URI', 'memory://') + try: + limiter.storage.check() + checks['rate_limit_storage'] = 'ok' + except Exception: + # The URI is config, not payload, but keep the reason out of the body. + logger.warning('Rate-limit storage check failed') + checks['rate_limit_storage'] = 'unavailable' + + checks['xlsx_writer'] = 'ok' if Workbook is not None else 'unavailable' + + ready = all(value == 'ok' for value in checks.values()) + payload = _health_payload( + status='ok' if ready else 'degraded', + checks=checks, + rate_limit_storage_backend=storage_uri.split(':', 1)[0], + ) + return jsonify(payload), (200 if ready else 503) def _build_api_auth(auth_method): diff --git a/security.py b/security.py index ccd7b2c..f2bb553 100644 --- a/security.py +++ b/security.py @@ -41,6 +41,8 @@ 'main.export_csv', 'main.export_xlsx', 'main.health', + 'main.health_live', + 'main.health_ready', } ) diff --git a/static/css/style.css b/static/css/style.css index 58aa960..ed8782b 100644 --- a/static/css/style.css +++ b/static/css/style.css @@ -961,3 +961,89 @@ th.sort-desc::after { opacity: 0.5; cursor: not-allowed; } + +/* --- v1.2 table toolbar: filter, pagination, column visibility (4.1/4.2/4.4) --- */ +.table-toolbar { + display: flex; + flex-wrap: wrap; + gap: 12px; + justify-content: space-between; + align-items: center; + margin-bottom: 12px; +} + +.table-toolbar-group { + display: flex; + gap: 8px; + align-items: center; +} + +.row-filter { + min-width: 240px; +} + +.filter-count, +.row-warning { + font-size: 0.85rem; + color: var(--text-secondary, #8b949e); +} + +.row-warning { + padding: 8px 12px; + margin-bottom: 12px; + border-radius: 6px; + border: 1px solid var(--border, #30363d); +} + +.btn-small { + padding: 6px 12px; + font-size: 0.85rem; +} + +.column-group { + position: relative; +} + +.column-dropdown { + display: none; + position: absolute; + right: 0; + top: calc(100% + 6px); + z-index: 20; + min-width: 220px; + max-height: 320px; + overflow-y: auto; + padding: 8px; + border-radius: 8px; + border: 1px solid var(--border, #30363d); + background: var(--bg-elevated, #161b22); +} + +.column-dropdown.visible { + display: block; +} + +.column-dropdown label { + display: flex; + gap: 8px; + align-items: center; + padding: 4px 6px; + font-size: 0.85rem; + cursor: pointer; +} + +.visually-hidden { + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; + overflow: hidden; + clip: rect(0, 0, 0, 0); + white-space: nowrap; + border: 0; +} + +.text-muted { + color: var(--text-secondary, #8b949e); +} diff --git a/static/js/app.js b/static/js/app.js index 573cf6b..47c7ccb 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -148,7 +148,7 @@ async function submitForm(jsonPath) { previewLimit = data.preview_limit || previewLimit; updateExcelAvailability(); - renderTable(data.columns, data.preview, data.total_rows); + setTableData(data); showResults(); } catch (err) { @@ -187,6 +187,7 @@ function showTreePicker(rawJson) { treeContainer.innerHTML = ''; treeContainer.appendChild(buildTreeNode(rawJson, '(root)', 'root', true)); pathModal.classList.add('visible'); + preselectPathFromHash(); } function describeNode(value) { @@ -318,8 +319,78 @@ function selectNode(row, path) { treeConfirmBtn.disabled = false; } +// --- 4.5 deep-linkable path selection ------------------------------------- +// +// #path=users.0.orders pre-selects that node when the picker opens, and +// confirming a selection writes the hash back, so the link can be shared for a +// conversion someone repeats. + +function readPathFromHash() { + const hash = (window.location.hash || '').replace(/^#/, ''); + if (!hash) return null; + const match = new URLSearchParams(hash).get('path'); + return match ? match.trim() : null; +} + +function writePathToHash(path) { + const params = new URLSearchParams(); + params.set('path', path); + window.location.hash = params.toString(); +} + +// Attribute selectors would need escaping for arbitrary JSON keys; scanning the +// rows avoids the question entirely. +function findTreeRowByPath(path) { + return ( + Array.from(treeContainer.querySelectorAll('.tree-row[data-path]')).find( + row => row.dataset.path === path + ) || null + ); +} + +function expandTreeToPath(path) { + if (path === '(root)') { + const rootRow = findTreeRowByPath('(root)'); + if (rootRow) selectNode(rootRow, '(root)'); + return Boolean(rootRow); + } + + let current = ''; + let target = null; + for (const segment of path.split('.')) { + current = current ? `${current}.${segment}` : segment; + const row = findTreeRowByPath(current); + // Missing means the path does not exist, or the node sits past a lazy + // cap. Either way this is a hint, not a command -- leave the picker open. + if (!row) return false; + + const children = row.parentElement.querySelector(':scope > .tree-children'); + if (children) { + populateChildren(children); + children.classList.remove('hidden'); + const toggle = row.querySelector('.tree-toggle'); + if (toggle) toggle.textContent = '▾'; + } + target = row; + } + + if (target) { + selectNode(target, current); + target.scrollIntoView({ block: 'nearest' }); + return true; + } + return false; +} + +function preselectPathFromHash() { + const path = readPathFromHash(); + if (!path) return; + expandTreeToPath(path); +} + treeConfirmBtn.addEventListener('click', () => { if (!selectedTreePath) return; + writePathToHash(selectedTreePath); pathModal.classList.remove('visible'); submitForm(selectedTreePath); }); @@ -335,15 +406,115 @@ pathModal.addEventListener('click', (e) => { } }); +// --- Table view model (4.1/4.2/4.4) --------------------------------------- +// +// Two datasets arrive from /process: `preview` (nested, capped server-side) with +// its own `columns`, and `csv_data` (flattened, full fidelity) with +// `csv_columns`. The table starts on the preview. "Load more" switches to the +// flattened dataset -- that is the only one the browser holds for every row -- +// and the badge says so, because the column set genuinely differs (a nested +// `meta` object becomes `meta.age`). + +// Above this many DOM rows the browser starts to struggle; "Load all" asks first. +const MAX_DOM_ROWS = 50000; +const LOAD_MORE_STEP = 500; + +let previewRows = null; +let previewColumns = null; +let viewMode = 'preview'; +let loadedRowCount = 0; +let hiddenColumns = new Set(); +let filterText = ''; + +const rowFilterInput = document.getElementById('rowFilter'); +const filterCount = document.getElementById('filterCount'); +const loadMoreBtn = document.getElementById('loadMoreBtn'); +const loadAllBtn = document.getElementById('loadAllBtn'); +const rowWarning = document.getElementById('rowWarning'); +const columnsBtn = document.getElementById('columnsBtn'); +const columnsDropdown = document.getElementById('columnsDropdown'); + +function setTableData(data) { + previewColumns = data.columns || []; + previewRows = data.preview || []; + currentTotalRows = data.total_rows || 0; + viewMode = 'preview'; + loadedRowCount = previewRows.length; + hiddenColumns = new Set(); + filterText = ''; + sortColumn = null; + sortDirection = 'asc'; + if (rowFilterInput) rowFilterInput.value = ''; + renderTable(); +} + +function baseColumns() { + return (viewMode === 'preview' ? previewColumns : csvColumns) || []; +} + +function baseRows() { + if (viewMode === 'preview') return previewRows || []; + return (csvData || []).slice(0, loadedRowCount); +} + +function visibleColumns() { + return baseColumns().filter(col => !hiddenColumns.has(col)); +} + +function rowMatchesFilter(row, needle) { + return Object.values(row).some(value => { + if (value === null || value === undefined) return false; + const text = typeof value === 'object' ? JSON.stringify(value) : String(value); + return text.toLowerCase().includes(needle); + }); +} + +function applyFilter(rows) { + const needle = filterText.trim().toLowerCase(); + if (!needle) return rows; + return rows.filter(row => rowMatchesFilter(row, needle)); +} + +function compareValues(a, b, col) { + let valA = a[col]; + let valB = b[col]; + + if (valA === null || valA === undefined) valA = ''; + if (valB === null || valB === undefined) valB = ''; + + if (typeof valA === 'object') valA = JSON.stringify(valA); + if (typeof valB === 'object') valB = JSON.stringify(valB); + + if (typeof valA === 'number' && typeof valB === 'number') { + return sortDirection === 'asc' ? valA - valB : valB - valA; + } + + const strA = String(valA).toLowerCase(); + const strB = String(valB).toLowerCase(); + if (strA < strB) return sortDirection === 'asc' ? -1 : 1; + if (strA > strB) return sortDirection === 'asc' ? 1 : -1; + return 0; +} + +function applySort(rows) { + if (!sortColumn) return rows; + return [...rows].sort((a, b) => compareValues(a, b, sortColumn)); +} + // Render table -function renderTable(columns, rows, totalRows) { - currentColumns = columns; - currentRows = rows; - currentTotalRows = totalRows; - renderTableDOM(columns, rows, totalRows); +function renderTable() { + const columns = visibleColumns(); + const loaded = baseRows(); + const filtered = applyFilter(loaded); + const rows = applySort(filtered); + + renderTableDOM(columns, rows); + renderColumnToggles(); + updateCounts(loaded.length, filtered.length); + updateLoadControls(); } -function renderTableDOM(columns, rows, totalRows) { +function renderTableDOM(columns, rows) { tableHead.innerHTML = ''; tableBody.innerHTML = ''; @@ -361,67 +532,159 @@ function renderTableDOM(columns, rows, totalRows) { }); tableHead.appendChild(headerRow); - // Body + // Body. One fragment, so a large "load all" is a single reflow. + const fragment = document.createDocumentFragment(); rows.forEach(row => { const tr = document.createElement('tr'); columns.forEach(col => { const td = document.createElement('td'); - const value = row[col]; - td.innerHTML = formatValue(value); + td.innerHTML = formatValue(row[col]); tr.appendChild(td); }); - tableBody.appendChild(tr); + fragment.appendChild(tr); }); + tableBody.appendChild(fragment); +} - // Update counts - rowCountText.textContent = `${totalRows} total rows`; - if (rows.length < totalRows) { - previewBadge.textContent = `Showing first ${rows.length}`; - // Sorting reorders only the rows currently in the table, while exports +function updateCounts(loadedCount, shownCount) { + rowCountText.textContent = `${currentTotalRows} total rows`; + + if (filterCount) { + filterCount.textContent = filterText.trim() + ? `${shownCount} of ${loadedCount} loaded rows match` + : ''; + } + + if (loadedCount < currentTotalRows) { + previewBadge.textContent = `Showing first ${loadedCount}`; + // Sorting and filtering act on the rows currently loaded, while exports // always contain every row -- say so rather than leaving the discrepancy // invisible (P11). previewBadge.title = - `Preview limit is ${previewLimit}. Sorting applies to the rows shown here; ` + - 'exports always contain all rows.'; + `Preview limit is ${previewLimit}. Sorting and filtering apply to the ` + + 'rows loaded here; exports always contain all rows.'; previewBadge.classList.remove('hidden'); } else { - previewBadge.textContent = `Showing all ${totalRows}`; - previewBadge.title = ''; - previewBadge.classList.add('hidden'); + previewBadge.textContent = + viewMode === 'full' + ? `Showing all ${currentTotalRows} (flattened columns)` + : `Showing all ${currentTotalRows}`; + previewBadge.title = + viewMode === 'full' + ? 'Rows past the preview come from the flattened dataset, so nested ' + + 'objects appear as dotted columns.' + : ''; + if (viewMode === 'full') { + previewBadge.classList.remove('hidden'); + } else { + previewBadge.classList.add('hidden'); + } } } -// Column sorting (client-side on preview rows) -function handleSort(col) { - if (sortColumn === col) { - sortDirection = sortDirection === 'asc' ? 'desc' : 'asc'; - } else { - sortColumn = col; - sortDirection = 'asc'; +function updateLoadControls() { + const total = currentTotalRows; + const moreAvailable = loadedRowCount < total; + if (loadMoreBtn) { + loadMoreBtn.disabled = !moreAvailable; + loadMoreBtn.textContent = moreAvailable + ? `Load next ${Math.min(LOAD_MORE_STEP, total - loadedRowCount)}` + : 'All rows loaded'; } + if (loadAllBtn) loadAllBtn.disabled = !moreAvailable; +} - const sorted = [...currentRows].sort((a, b) => { - let valA = a[col]; - let valB = b[col]; +function showRowWarning(message) { + if (!rowWarning) return; + if (!message) { + rowWarning.classList.add('hidden'); + rowWarning.textContent = ''; + return; + } + rowWarning.textContent = message; + rowWarning.classList.remove('hidden'); +} - if (valA === null || valA === undefined) valA = ''; - if (valB === null || valB === undefined) valB = ''; +function loadRows(count) { + if (!csvData) return; + // Rows past the preview only exist in the flattened dataset. + viewMode = 'full'; + const target = Math.min(loadedRowCount + count, csvData.length); + + if (target > MAX_DOM_ROWS) { + loadedRowCount = Math.min(target, MAX_DOM_ROWS); + showRowWarning( + `Rendering stops at ${MAX_DOM_ROWS} rows to keep the page responsive. ` + + `All ${currentTotalRows} rows are still included in every export.` + ); + } else { + loadedRowCount = target; + showRowWarning(''); + } + renderTable(); +} - if (typeof valA === 'object') valA = JSON.stringify(valA); - if (typeof valB === 'object') valB = JSON.stringify(valB); +if (loadMoreBtn) loadMoreBtn.addEventListener('click', () => loadRows(LOAD_MORE_STEP)); +if (loadAllBtn) { + loadAllBtn.addEventListener('click', () => loadRows(Number.MAX_SAFE_INTEGER)); +} - if (typeof valA === 'number' && typeof valB === 'number') { - return sortDirection === 'asc' ? valA - valB : valB - valA; - } +if (rowFilterInput) { + rowFilterInput.addEventListener('input', () => { + filterText = rowFilterInput.value; + renderTable(); + }); +} - const strA = String(valA).toLowerCase(); - const strB = String(valB).toLowerCase(); - if (strA < strB) return sortDirection === 'asc' ? -1 : 1; - if (strA > strB) return sortDirection === 'asc' ? 1 : -1; - return 0; +// --- Column visibility (4.4) ---------------------------------------------- +function renderColumnToggles() { + if (!columnsDropdown) return; + columnsDropdown.innerHTML = ''; + baseColumns().forEach(col => { + const label = document.createElement('label'); + const checkbox = document.createElement('input'); + checkbox.type = 'checkbox'; + checkbox.checked = !hiddenColumns.has(col); + checkbox.addEventListener('change', () => { + if (checkbox.checked) { + hiddenColumns.delete(col); + } else { + hiddenColumns.add(col); + } + renderTable(); + }); + const text = document.createElement('span'); + text.textContent = col; + label.appendChild(checkbox); + label.appendChild(text); + columnsDropdown.appendChild(label); + }); +} + +if (columnsBtn) { + columnsBtn.addEventListener('click', (e) => { + e.stopPropagation(); + const open = columnsDropdown.classList.toggle('visible'); + columnsBtn.setAttribute('aria-expanded', String(open)); }); +} + +document.addEventListener('click', (e) => { + if (columnsDropdown && !e.target.closest('.column-group')) { + columnsDropdown.classList.remove('visible'); + if (columnsBtn) columnsBtn.setAttribute('aria-expanded', 'false'); + } +}); - renderTableDOM(currentColumns, sorted, currentTotalRows); +// Column sorting (client-side, over the rows currently loaded) +function handleSort(col) { + if (sortColumn === col) { + sortDirection = sortDirection === 'asc' ? 'desc' : 'asc'; + } else { + sortColumn = col; + sortDirection = 'asc'; + } + renderTable(); } // P5: a nested object with 50k keys, a 100k-item array stringified whole, or a @@ -569,21 +832,69 @@ function updateExcelAvailability() { // Export dropdown toggle const exportDropdown = document.getElementById('exportDropdown'); + +function setExportDropdownOpen(open) { + exportDropdown.classList.toggle('visible', open); + exportBtn.setAttribute('aria-expanded', String(open)); +} + exportBtn.addEventListener('click', () => { - exportDropdown.classList.toggle('visible'); + setExportDropdownOpen(!exportDropdown.classList.contains('visible')); }); // Close dropdown when clicking outside document.addEventListener('click', (e) => { if (!e.target.closest('.export-group')) { - exportDropdown.classList.remove('visible'); + setExportDropdownOpen(false); + } +}); + +// Keyboard: Escape closes and returns focus to the trigger; arrows walk the menu. +document.addEventListener('keydown', (e) => { + if (e.key !== 'Escape') return; + if (exportDropdown.classList.contains('visible')) { + setExportDropdownOpen(false); + exportBtn.focus(); + } + if (columnsDropdown && columnsDropdown.classList.contains('visible')) { + columnsDropdown.classList.remove('visible'); + if (columnsBtn) { + columnsBtn.setAttribute('aria-expanded', 'false'); + columnsBtn.focus(); + } + } + if (aboutModal && aboutModal.classList.contains('visible')) { + aboutModal.classList.remove('visible'); + } + if (pathModal.classList.contains('visible')) { + pathModal.classList.remove('visible'); } }); +exportDropdown.addEventListener('keydown', (e) => { + const items = Array.from(exportDropdown.querySelectorAll('.export-dropdown-item')); + const index = items.indexOf(document.activeElement); + if (e.key === 'ArrowDown') { + e.preventDefault(); + items[(index + 1) % items.length].focus(); + } else if (e.key === 'ArrowUp') { + e.preventDefault(); + items[(index - 1 + items.length) % items.length].focus(); + } +}); + +exportBtn.addEventListener('keydown', (e) => { + if (e.key !== 'ArrowDown') return; + e.preventDefault(); + setExportDropdownOpen(true); + const first = exportDropdown.querySelector('.export-dropdown-item'); + if (first) first.focus(); +}); + // Export handlers document.querySelectorAll('.export-dropdown-item').forEach(item => { item.addEventListener('click', async () => { - exportDropdown.classList.remove('visible'); + setExportDropdownOpen(false); const format = item.dataset.format; if (!csvData || !csvColumns) { @@ -595,6 +906,18 @@ document.querySelectorAll('.export-dropdown-item').forEach(item => { downloadDelimited(csvColumns, csvData, ',', 'exported_data.csv'); } else if (format === 'tsv') { downloadDelimited(csvColumns, csvData, '\t', 'exported_data.tsv'); + } else if (format === 'jsonl') { + downloadChunks( + buildJsonlChunks(csvColumns, csvData), + 'application/x-ndjson; charset=utf-8', + 'exported_data.jsonl' + ); + } else if (format === 'markdown') { + downloadChunks( + buildMarkdownChunks(csvColumns, csvData), + 'text/markdown; charset=utf-8', + 'exported_data.md' + ); } else if (format === 'xlsx') { if (excelExportBlocked()) { showError( @@ -669,6 +992,74 @@ function buildDelimited(columns, data, delimiter) { return buildDelimitedChunks(columns, data, delimiter).join(''); } +// --- 4.3 JSONL export ----------------------------------------------------- +// +// Lossless: rows go out exactly as they arrived, with NO formula sanitization. +// F1's sanitizer exists because CSV/TSV/XLSX have no type channel and a +// spreadsheet re-interprets a leading '=' as a formula; JSON has types, nothing +// evaluates it, and prefixing values here would corrupt the data instead of +// protecting anything. +function buildJsonlChunks(columns, data) { + const chunks = []; + let pending = ''; + data.forEach((row, index) => { + const projected = {}; + columns.forEach(col => { + if (row[col] !== undefined) projected[col] = row[col]; + }); + pending += JSON.stringify(projected) + '\n'; + if ((index + 1) % BLOB_CHUNK_ROWS === 0) { + chunks.push(pending); + pending = ''; + } + }); + if (pending) chunks.push(pending); + return chunks; +} + +// --- 4.3 Markdown export -------------------------------------------------- +// +// Markdown-specific escaping only, again NOT the spreadsheet sanitizer: a +// leading '=' is inert in Markdown. What does break a Markdown table is an +// unescaped pipe or a newline inside a cell. +function escapeMarkdownCell(value) { + if (value === null || value === undefined) return ''; + const text = typeof value === 'object' ? JSON.stringify(value) : String(value); + return text + .replace(/\\/g, '\\\\') + .replace(/\|/g, '\\|') + .replace(/\r\n|\r|\n/g, '
'); +} + +function buildMarkdownChunks(columns, data) { + const chunks = []; + let pending = + '| ' + columns.map(escapeMarkdownCell).join(' | ') + ' |\n' + + '| ' + columns.map(() => '---').join(' | ') + ' |\n'; + + data.forEach((row, index) => { + pending += '| ' + columns.map(col => escapeMarkdownCell(row[col])).join(' | ') + ' |\n'; + if ((index + 1) % BLOB_CHUNK_ROWS === 0) { + chunks.push(pending); + pending = ''; + } + }); + if (pending) chunks.push(pending); + return chunks; +} + +function downloadChunks(chunks, mimeType, filename) { + const blob = new Blob(chunks, { type: mimeType }); + const url = window.URL.createObjectURL(blob); + const a = document.createElement('a'); + a.href = url; + a.download = filename; + document.body.appendChild(a); + a.click(); + window.URL.revokeObjectURL(url); + a.remove(); +} + // Client-side CSV/TSV generation function downloadDelimited(columns, data, delimiter, filename) { const chunks = buildDelimitedChunks(columns, data, delimiter); @@ -751,10 +1142,23 @@ themeToggle.addEventListener('click', () => { applyTheme(next); }); -// About link +// About dialog. Replaces alert(), which also hardcoded a version string that +// went stale the moment APP_VERSION changed -- the modal reads it from config. +const aboutModal = document.getElementById('aboutModal'); + document.getElementById('aboutLink').addEventListener('click', (e) => { e.preventDefault(); - alert('JSON Table Converter v1.1.0\n\nBuilt with Flask + Python\nNo data is ever stored or logged.'); + aboutModal.classList.add('visible'); +}); + +document.getElementById('aboutClose').addEventListener('click', () => { + aboutModal.classList.remove('visible'); +}); + +aboutModal.addEventListener('click', (e) => { + if (e.target === e.currentTarget) { + e.currentTarget.classList.remove('visible'); + } }); // UI helpers diff --git a/templates/index.html b/templates/index.html index fc6593e..5fc4383 100644 --- a/templates/index.html +++ b/templates/index.html @@ -191,19 +191,40 @@

JSON → Table

- -
- - - +
+
+
+ + + +
+
+ + +
+ + +
+
+
+
@@ -217,6 +238,20 @@

JSON → Table

+ + +