diff --git a/artifacts/exception_handlers.py b/artifacts/exception_handlers.py new file mode 100644 index 0000000..fa09eba --- /dev/null +++ b/artifacts/exception_handlers.py @@ -0,0 +1,24 @@ +from rest_framework.response import Response +from rest_framework.views import exception_handler + + +def daybook_exception_handler(exc, context): + """DRF exception handler that additionally maps pathological-input crashes + to a clean 400 instead of an unhandled 500. + + Deeply nested JSON in the request body raises RecursionError inside DRF's + JSONParser (before any view validation runs). RecursionError subclasses + RuntimeError, not ValueError, so DRF's default handler doesn't map it and + it surfaces as a 500. Anything the default handler already knows about is + left to it; anything still unhandled that isn't a RecursionError is + returned as None so Django produces its normal 500. + """ + response = exception_handler(exc, context) + if response is not None: + return response + if isinstance(exc, RecursionError): + return Response( + {"detail": "Request payload is nested too deeply to parse."}, + status=400, + ) + return None diff --git a/artifacts/views.py b/artifacts/views.py index 3e47865..82f733f 100644 --- a/artifacts/views.py +++ b/artifacts/views.py @@ -866,8 +866,14 @@ def wrap_csv_viewer(content, title, request): gridjs_js = request.build_absolute_uri("/static/vendor/gridjs/gridjs.umd.js") gridjs_css = request.build_absolute_uri("/static/vendor/gridjs/mermaid.min.css") rows = [row for row in csv.reader(io.StringIO(content)) if row] - columns = rows[0] if rows else [] - data = rows[1:] if len(rows) > 1 else [] + # Normalize ragged rows to a single width so no cell is ever dropped by + # Grid.js (which keys body cells by column). Widen the header for rows + # longer than it (auto-naming the extra columns), and pad short rows with + # empty strings. Issue #016. + width = max((len(row) for row in rows), default=0) + header = rows[0] if rows else [] + columns = [header[i] if i < len(header) else f"Column {i + 1}" for i in range(width)] + data = [[row[i] if i < len(row) else "" for i in range(width)] for row in rows[1:]] table_json = escape_script_body(json.dumps({"columns": columns, "data": data})) return f""" @@ -1036,7 +1042,9 @@ def validate_content_shape(kind, content): if kind == Artifact.JSON: try: json.loads(content) - except (TypeError, ValueError) as exc: + # RecursionError (deeply nested JSON) is a RuntimeError, not a + # ValueError, so it must be caught explicitly or it becomes a 500. + except (TypeError, ValueError, RecursionError) as exc: return Response({"detail": f"content must be valid JSON: {exc}"}, status=400) return None diff --git a/daybook_project/settings.py b/daybook_project/settings.py index 3edaa59..ee85175 100644 --- a/daybook_project/settings.py +++ b/daybook_project/settings.py @@ -57,6 +57,12 @@ TEMPLATES = [ WSGI_APPLICATION = "daybook_project.wsgi.application" +REST_FRAMEWORK = { + # Maps deeply-nested-JSON RecursionErrors (raised in DRF's parser, before + # view validation) to a 400 instead of an unhandled 500. See issue #015. + "EXCEPTION_HANDLER": "artifacts.exception_handlers.daybook_exception_handler", +} + DATABASES = { "default": { "ENGINE": "django.db.backends.sqlite3", diff --git a/issues/015-deeply-nested-json-recursionerror-500.md b/issues/015-deeply-nested-json-recursionerror-500.md index f5109fa..d794ecc 100644 --- a/issues/015-deeply-nested-json-recursionerror-500.md +++ b/issues/015-deeply-nested-json-recursionerror-500.md @@ -1,6 +1,6 @@ # 015 — Deeply nested JSON raises `RecursionError` → unhandled HTTP 500 (two code paths) -**Status:** Open +**Status:** Fixed **Severity:** Medium — unhandled 500 where a 400 is correct; small (~30 KB) request crashes the handling worker (minor DoS shape); full traceback exposed when `DEBUG=True`. @@ -91,4 +91,21 @@ this. ## Resolution -_(pending)_ +Both paths fixed: + +- **Path 1** (`json`-kind content validation): `validate_content_shape` now + catches `RecursionError` alongside `(TypeError, ValueError)`, returning + the same `400 content must be valid JSON: …`. +- **Path 2** (deep nesting anywhere in the request body, crashing DRF's + parser before the view): added a custom DRF `EXCEPTION_HANDLER` + (`artifacts/exception_handlers.py`, wired via `REST_FRAMEWORK` in + settings) that maps otherwise-unhandled `RecursionError` to + `400 Request payload is nested too deeply to parse.` This also covers the + same class of error in `metadata`/`tag_colors` string parsing. Anything + that isn't a RecursionError still falls through to Django's normal 500. + +Verified live: deep `json` content and a ~5000-deep `metadata` body both +return 400; shallow payloads still 201. Tests added in +`tests/api/test_artifacts.py`. (A depth-cap parser was considered but the +exception-handler approach is simpler and covers every recursive-descent +site at once; prod `DEBUG=False` already suppresses the traceback page.) diff --git a/issues/016-csv-ragged-rows-silent-data-loss-in-render.md b/issues/016-csv-ragged-rows-silent-data-loss-in-render.md index 467fb68..3fa36e1 100644 --- a/issues/016-csv-ragged-rows-silent-data-loss-in-render.md +++ b/issues/016-csv-ragged-rows-silent-data-loss-in-render.md @@ -1,6 +1,6 @@ # 016 — Ragged CSV rows silently lose data in the rendered table (server doesn't normalize row width) -**Status:** Open +**Status:** Fixed **Severity:** Low-Medium — silent data loss in the primary rendered view; raw sidecar is unaffected, so it's a fidelity/display bug, not permanent loss. @@ -82,4 +82,13 @@ raw sidecar. ## Resolution -_(pending)_ +Implemented option (b) in `wrap_csv_viewer`: normalize every row to the +maximum row width before handing data to Grid.js. Rows longer than the +header widen the column set (extra columns auto-named `Column N`); rows +shorter than the width are padded with empty strings. No cell is ever +dropped, matching "server owns normalization, never silently lose content." + +Verified live with the issue's exact input (`a,b,c` / `1,2,3,4,5` / `6,7`): +grid data becomes `columns=[a,b,c,Column 4,Column 5]`, +`data=[[1,2,3,4,5],[6,7,"","",""]]` — the previously-dropped `4` and `5` +now render. Test added in `tests/api/test_artifacts.py`. diff --git a/tests/api/test_artifacts.py b/tests/api/test_artifacts.py index 65ba529..967ed4f 100644 --- a/tests/api/test_artifacts.py +++ b/tests/api/test_artifacts.py @@ -594,3 +594,42 @@ class ArtifactAPITestCase(APITestCase): response = self.client.get(reverse("artifact", args=[aid])) self.assertIsNone(response.headers.get("Content-Security-Policy")) + # -- deeply nested JSON -> 400 not 500 (issue #015) ---------------------- + + def test_deeply_nested_json_content_returns_400(self): + deep = "[" * 5000 + "]" * 5000 # valid JSON, just too deep to parse + response = self.create_artifact(kind="json", content=deep) + self.assertEqual(response.status_code, 400) + + def test_deeply_nested_request_body_returns_400(self): + # Nesting in the request body itself crashes DRF's parser before the + # view runs; the custom exception handler must map it to 400. + nested = "null" + for _ in range(5000): + nested = '{"n":' + nested + "}" + body = ( + '{"source":"t","kind":"markdown","title":"x","content":"hi",' + '"metadata":' + nested + "}" + ) + response = self.client.post( + reverse("artifact-api"), + data=body, + content_type="application/json", + **self.auth_headers(self.token), + ) + self.assertEqual(response.status_code, 400) + + def test_normal_depth_json_still_accepted(self): + response = self.create_artifact(kind="json", content='{"a": {"b": {"c": [1, 2, 3]}}}') + self.assertEqual(response.status_code, 201) + + # -- ragged CSV rows normalized, no data lost (issue #016) --------------- + + def test_ragged_csv_rows_preserve_all_cells(self): + aid = self.create_artifact(kind="csv", content="a,b,c\n1,2,3,4,5\n6,7").data["id"] + html = self._stored_html(aid) + # Long row's extra cells are kept via auto-named columns; short row padded. + self.assertIn('"columns": ["a", "b", "c", "Column 4", "Column 5"]', html) + self.assertIn('["1", "2", "3", "4", "5"]', html) + self.assertIn('["6", "7", "", "", ""]', html) +