Reject non-string source_name/source_id/source_prompt (issue #008)
These accepted any JSON type on both POST and PATCH, silently mangled
into Python repr() strings at save time (a dict became the literal text
"{'a': 1}", unparseable as JSON) - and the immediate response echoed the
raw pre-save value, while a later GET returned the mangled one, an
internally inconsistent API.
Added validate_string_fields(), rejecting non-string values with 400
before they reach save(), matching #005's approach for tags. Verified live
and with new tests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
d1b6a19eb3
commit
09ea0a8b4f
3 changed files with 117 additions and 0 deletions
|
|
@ -237,6 +237,9 @@ class ArtifactDetailView(APIView):
|
||||||
length_error = validate_field_lengths(data, ["title", "source_name", "source_id"])
|
length_error = validate_field_lengths(data, ["title", "source_name", "source_id"])
|
||||||
if length_error:
|
if length_error:
|
||||||
return length_error
|
return length_error
|
||||||
|
string_error = validate_string_fields(data, ["source_name", "source_id", "source_prompt"])
|
||||||
|
if string_error:
|
||||||
|
return string_error
|
||||||
if "title" in data:
|
if "title" in data:
|
||||||
if not data["title"]:
|
if not data["title"]:
|
||||||
return Response({"detail": "title cannot be blank"}, status=400)
|
return Response({"detail": "title cannot be blank"}, status=400)
|
||||||
|
|
@ -860,6 +863,19 @@ def validate_field_lengths(data, fields):
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
def validate_string_fields(data, fields):
|
||||||
|
"""Rejects non-string JSON types (object/array/number) instead of letting
|
||||||
|
them reach save(), where Django's CharField/TextField silently coerce via
|
||||||
|
str() - which for a dict/list produces Python repr() syntax, not JSON,
|
||||||
|
and is irreversible.
|
||||||
|
"""
|
||||||
|
for field in fields:
|
||||||
|
value = data.get(field)
|
||||||
|
if value is not None and not isinstance(value, str):
|
||||||
|
return Response({"detail": f"{field} must be a string"}, status=400)
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
def validate_content_shape(kind, content):
|
def validate_content_shape(kind, content):
|
||||||
lower = (content or "").lower()
|
lower = (content or "").lower()
|
||||||
if kind == Artifact.DECK and "<section" not in lower:
|
if kind == Artifact.DECK and "<section" not in lower:
|
||||||
|
|
@ -889,6 +905,9 @@ def validate_source_fields(data):
|
||||||
length_error = validate_field_lengths(data, ["source", "title", "source_name", "source_id", "theme"])
|
length_error = validate_field_lengths(data, ["source", "title", "source_name", "source_id", "theme"])
|
||||||
if length_error:
|
if length_error:
|
||||||
return length_error
|
return length_error
|
||||||
|
string_error = validate_string_fields(data, ["source_name", "source_id", "source_prompt"])
|
||||||
|
if string_error:
|
||||||
|
return string_error
|
||||||
if source_type := data.get("source_type"):
|
if source_type := data.get("source_type"):
|
||||||
if source_type not in {choice[0] for choice in Artifact.SOURCE_TYPE_CHOICES}:
|
if source_type not in {choice[0] for choice in Artifact.SOURCE_TYPE_CHOICES}:
|
||||||
return Response({"detail": "source_type must be agent, person, or system"}, status=400)
|
return Response({"detail": "source_type must be agent, person, or system"}, status=400)
|
||||||
|
|
|
||||||
70
issues/008-source-fields-untyped-and-mangled.md
Normal file
70
issues/008-source-fields-untyped-and-mangled.md
Normal file
|
|
@ -0,0 +1,70 @@
|
||||||
|
# 008 — `source_name`/`source_id`/`source_prompt` accept any JSON type and get silently mangled into `repr()` strings
|
||||||
|
|
||||||
|
**Status:** Fixed
|
||||||
|
|
||||||
|
Follow-up to the #007 handover, which specifically asked to probe other
|
||||||
|
wrong-typed `PATCH` fields beyond `tags` now that #005 is fixed.
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
Unlike `tags`, `metadata`, and `tag_colors` (all now type-checked, `tags`
|
||||||
|
since #005), `source_name`, `source_id`, and `source_prompt` accept any
|
||||||
|
JSON type — object, array, number — on both `POST` (`source_fields()` in
|
||||||
|
`views.py`) and `PATCH` (`ArtifactDetailView.patch`). Nothing validates
|
||||||
|
they're strings before assignment.
|
||||||
|
|
||||||
|
`source_name`/`source_id` are `CharField`, `source_prompt` is `TextField`.
|
||||||
|
Django's `Field.get_prep_value()` silently coerces non-string input via
|
||||||
|
plain `str(value)` at save time — which for a `dict`/`list` produces
|
||||||
|
**Python `repr()` syntax, not JSON**: `{"a": 1}` becomes the *string*
|
||||||
|
`"{'a': 1}"` (single quotes, unparseable as JSON). This is lossy and
|
||||||
|
irreversible — there's no way to recover the original structured value.
|
||||||
|
|
||||||
|
**Worse: the API response is internally inconsistent.** Immediately after
|
||||||
|
a `POST`/`PATCH` with a non-string value, the response echoes back the
|
||||||
|
*raw, pre-save Python object* (still a real JSON object/array in the
|
||||||
|
response body) — but a subsequent `GET` of the same artifact returns the
|
||||||
|
mangled `repr()` string instead. Same request, two different apparent
|
||||||
|
types for the same field depending only on *when* you read it.
|
||||||
|
|
||||||
|
Reproduced:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
# PATCH source_name with an int, source_id with a list, source_prompt with a dict
|
||||||
|
curl -X PATCH .../api/artifacts/a74de286 -d '{"source_name": 12345}' ...
|
||||||
|
# -> 200, response shows "source_name": 12345 (int)
|
||||||
|
|
||||||
|
curl .../api/artifacts/a74de286 # fresh GET, session auth
|
||||||
|
# -> "source_name": "12345" (string)
|
||||||
|
|
||||||
|
# Actual DB values (confirmed via shell):
|
||||||
|
# source_name = '12345'
|
||||||
|
# source_id = "['a', 'b']" <- repr() of the list, not JSON
|
||||||
|
# source_prompt = "{'nested': 'object'}" <- repr() of the dict, not JSON
|
||||||
|
```
|
||||||
|
|
||||||
|
Also reproduced identically on `POST` (create), not just `PATCH` — a
|
||||||
|
freshly created artifact's immediate response shows
|
||||||
|
`"source_name": {"a": 1}` (real object) even though what's stored (and
|
||||||
|
what any later `GET` returns) is the mangled string form.
|
||||||
|
|
||||||
|
## Suggested Fix
|
||||||
|
|
||||||
|
Add a type check for `source_name`, `source_id`, and `source_prompt`
|
||||||
|
(must be `str` or absent/null) alongside the existing `tags`/`metadata`/
|
||||||
|
`tag_colors` checks in `validate_source_fields` (create path) and
|
||||||
|
`ArtifactDetailView.patch` (update path) — return `400` naming the field,
|
||||||
|
consistent with #005's approach for `tags`. This also removes the
|
||||||
|
response/DB inconsistency, since invalid types would never reach `save()`
|
||||||
|
in the first place.
|
||||||
|
|
||||||
|
## Resolution
|
||||||
|
|
||||||
|
Added `validate_string_fields(data, fields)` in `artifacts/views.py`,
|
||||||
|
rejecting any non-`None`, non-`str` value with `400`. Called from
|
||||||
|
`validate_source_fields` (covers `POST`, both text and binary paths) and
|
||||||
|
directly in `ArtifactDetailView.patch` for `source_name`, `source_id`,
|
||||||
|
`source_prompt` — matching #005's approach for `tags`. Invalid values never
|
||||||
|
reach `save()`, so the response/DB inconsistency the issue described can no
|
||||||
|
longer occur. Verified live against the running dev instance and with new
|
||||||
|
tests in `tests/api/test_artifacts.py`.
|
||||||
|
|
@ -427,3 +427,31 @@ class ArtifactAPITestCase(APITestCase):
|
||||||
def test_create_csv_artifact_with_header_only(self):
|
def test_create_csv_artifact_with_header_only(self):
|
||||||
response = self.create_artifact(kind="csv", content="name,score")
|
response = self.create_artifact(kind="csv", content="name,score")
|
||||||
self.assertEqual(response.status_code, 201)
|
self.assertEqual(response.status_code, 201)
|
||||||
|
|
||||||
|
# -- source_name/source_id/source_prompt type checks (issue #008) -------
|
||||||
|
|
||||||
|
def test_create_rejects_non_string_source_name(self):
|
||||||
|
response = self.create_artifact(source_name=12345)
|
||||||
|
self.assertEqual(response.status_code, 400)
|
||||||
|
self.assertIn("source_name", response.data["detail"])
|
||||||
|
|
||||||
|
def test_create_rejects_non_string_source_id(self):
|
||||||
|
response = self.create_artifact(source_id=["a", "b"])
|
||||||
|
self.assertEqual(response.status_code, 400)
|
||||||
|
|
||||||
|
def test_create_rejects_non_string_source_prompt(self):
|
||||||
|
response = self.create_artifact(source_prompt={"nested": "object"})
|
||||||
|
self.assertEqual(response.status_code, 400)
|
||||||
|
|
||||||
|
def test_patch_rejects_non_string_source_name(self):
|
||||||
|
artifact_id = self.create_artifact().data["id"]
|
||||||
|
response = self.client.patch(
|
||||||
|
reverse("artifact-detail-api", args=[artifact_id]),
|
||||||
|
{"source_name": 12345},
|
||||||
|
format="json",
|
||||||
|
**self.auth_headers(self.token),
|
||||||
|
)
|
||||||
|
self.assertEqual(response.status_code, 400)
|
||||||
|
|
||||||
|
artifact = Artifact.objects.get(pk=artifact_id)
|
||||||
|
self.assertEqual(artifact.source_name, "test-agent") # unchanged, not mangled
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue