peter
5f872b4b32
#015 — deeply nested JSON raised RecursionError (a RuntimeError, not ValueError) and became an unhandled 500 on two paths: - json-kind content validation now catches RecursionError -> 400. - Added a custom DRF EXCEPTION_HANDLER (artifacts/exception_handlers.py, wired via REST_FRAMEWORK) mapping otherwise-unhandled RecursionError -> 400, covering the parser-level crash on deeply nested request bodies (metadata etc.) too. #016 — wrap_csv_viewer handed Grid.js rows without reconciling width to the header, so over-long rows silently dropped cells. Now normalizes every row to the max width: long rows widen the columns (auto-named "Column N"), short rows pad with "". No cell is dropped. Both verified live; 4 regression tests added. 106 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
111 lines
4.7 KiB
Markdown
111 lines
4.7 KiB
Markdown
# 015 — Deeply nested JSON raises `RecursionError` → unhandled HTTP 500 (two code paths)
|
|
|
|
**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`.
|
|
|
|
Round-3 probe of the `json` kind ("deeply nested / very large" from the
|
|
#013 handover).
|
|
|
|
## Description
|
|
|
|
Two distinct code paths turn deeply-nested JSON into an uncaught
|
|
`RecursionError` and a 500. The root cause is shared: Python's
|
|
`RecursionError` is a subclass of `RuntimeError`, **not** `ValueError`, so
|
|
the usual `except (TypeError, ValueError)` guards around JSON parsing don't
|
|
catch it.
|
|
|
|
### Path 1 — `json` kind content validation
|
|
|
|
`validate_content_shape` (`artifacts/views.py:987`):
|
|
|
|
```python
|
|
if kind == Artifact.JSON:
|
|
try:
|
|
json.loads(content)
|
|
except (TypeError, ValueError) as exc: # <-- misses RecursionError
|
|
return Response({"detail": f"content must be valid JSON: {exc}"}, status=400)
|
|
```
|
|
|
|
Posting a `json` artifact whose `content` is `"[" * 5000 + "]" * 5000`
|
|
(valid, just deep) raises `RecursionError: maximum recursion depth
|
|
exceeded while decoding a JSON array from a unicode string`, which escapes
|
|
the `except` and becomes a 500.
|
|
|
|
Reproduced:
|
|
```
|
|
POST /api/artifacts kind=json content="[[[...5000...]]]"
|
|
-> 500 (RecursionError at views.py:987, via post() at views.py:133)
|
|
```
|
|
|
|
### Path 2 — DRF request-body parser (affects ALL kinds)
|
|
|
|
The same class of payload in the **request body itself** (e.g. a deeply
|
|
nested `metadata` object, or nesting anywhere in the JSON body) crashes in
|
|
DRF's `JSONParser` *before the view runs*, so it isn't `json`-kind
|
|
specific:
|
|
|
|
```
|
|
POST /api/artifacts kind=markdown metadata={"n":{"n":{...5000...}}}
|
|
(body ~30 KB, built as a raw string so it's well-formed)
|
|
-> 500 (RecursionError inside DRF JSONParser)
|
|
|
|
Control: same shape at depth 100 -> 201 (confirms it's depth, not size)
|
|
```
|
|
|
|
### Not a size issue
|
|
|
|
A 50,000-key flat object (~978 KB) posts fine (`201`). It's specifically
|
|
nesting *depth* that triggers it, so a byte-size limit wouldn't address
|
|
this.
|
|
|
|
## Impact
|
|
|
|
- Wrong status: an unhandled 500 where malformed/pathological input should
|
|
be a clean 400.
|
|
- Minor DoS shape: a single ~30 KB request reliably errors out the worker
|
|
handling it. Not a resource-exhaustion amplifier, but a cheap way to
|
|
generate server-side 500s.
|
|
- Info exposure *only* on a `DEBUG=True` instance (like the local dev one
|
|
this was found on): the full Django traceback page is returned. Prod
|
|
should have `DEBUG=False`, so there this degrades to a generic 500 — the
|
|
status-code bug remains either way. (Flagging the traceback exposure only
|
|
as a reminder to confirm prod `DEBUG` is off, not as the core issue.)
|
|
|
|
## Suggested Fix
|
|
|
|
- **Path 1 (cheap, local):** add `RecursionError` to the caught exceptions
|
|
at `views.py:988` (it's a `RuntimeError`, so either list it explicitly or
|
|
catch `(TypeError, ValueError, RecursionError)`), returning the same 400.
|
|
- **Path 2 (needs a global guard):** DRF's default exception handler
|
|
doesn't map `RecursionError` to a 4xx. Options: a custom
|
|
`REST_FRAMEWORK["EXCEPTION_HANDLER"]` that converts `RecursionError`
|
|
(and other parse-time `RuntimeError`s) to `400`; and/or a bounded JSON
|
|
parser / max-nesting-depth check applied to request bodies. A depth cap
|
|
(e.g. reject bodies nested beyond N levels) is the most robust since it
|
|
rejects before the recursive descent exhausts the stack.
|
|
- Consider a single shared "parse JSON safely" helper used by both the
|
|
content validator and any metadata/tag_colors parsing, so the guard
|
|
lives in one place.
|
|
|
|
## Resolution
|
|
|
|
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.)
|