Skip to content

Bound client input at the API boundary - #82

Merged
vladmesh merged 2 commits into
mainfrom
claude/api-bounds-wpm1u9
Sep 29, 2026
Merged

vladmesh merged 2 commits into
mainfrom
claude/api-bounds-wpm1u9

Conversation

@vladmesh

@vladmesh vladmesh commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Card W3-1: API boundary hardening (issue 4f6495b0f969e865d779 remainder + cfba11ddde265d501076 spawn-api-xp-value). Based on main 2f79530. None of the six items was already fixed on main.

Root cause / fix per item

  1. POST /api/frontend-error: it read await request.json() with no schema or size limit and logged message/stack without bounds.

    • New FrontendErrorReport model (message, stack, component, url) with max_length bounds. main.tsx sends {message, stack} and ErrorBoundary adds component; url is optional for later use.
    • The body is read as a stream and rejected with 413 once it passes 128 KiB, so an oversized report is never held in memory in full. An invalid body gets 422.
    • Each logged field is cut to 4000 chars, with a [truncated N chars] marker. component is now logged too (before, it was dropped).
    • The request schema is still in OpenAPI via openapi_extra. The frontend reporters are unchanged and still get 200.
  2. WebSocket message size (player and spectator loops): _parse_json_object_envelope rejects any message over MAX_WS_MESSAGE_BYTES = 64 KiB, measured in UTF-8 bytes, before json.loads. The client gets a localised {"type":"error"} reply and the loop keeps running. The largest legitimate client message is an action envelope with say text, which is far below this limit.

  3. UpdateLayerFileRequest.content: capped at 1 MiB. The largest shipped layer file is library/geography/sword_vale/locations.yaml at about 18 KiB.

  4. GiveItemRequest bounds:

    • price 0..1,000,000 gp
    • reach 5..30 ft
    • base_ac 10..30 (SRD armour runs 11..18)
    • max_dex_bonus 0..10
    • strength_req 0..30
    • ac_bonus 0..10
    • Strings are capped at 200 chars (32 for enum-like fields). damage is capped at 8 entries and grant_actions at 16.
    • The GM GiveItemDialog payload (reach defaults to 5) stays inside these ranges.
  5. Action params: typed param values were already checked on main (rules/action_params.validate_action_params covers string/integer types and required params). Three gaps remained in service/action_parsing.parse_action:

    • Non-object params ("x", [..], 1) were silently replaced with {}. For the dispatcher, an empty dict means "probe", so a malformed action was turned into a different request.
    • Keys that are not strings were not rejected.
    • There was no size bound, so say text of any length went into the event log, saves and LLM prompts.

    Now: params: null or a missing params still means {}. Anything else that is not an object with string keys raises ActionParamsError("not_object"). More than 16 entries, or a string value over 2000 chars, raises ActionParamsError("too_large"). The WS adapter answers with a localised protocol error and the loop continues.

  6. SpawnCreatureRequest.xp_value: added as int | None = Field(default=None, ge=0), the same bound as in PATCH.

    • NPCs already read xp_value through NpcContent, but pydantic dropped the field from the request.
    • The monster path in _parse_spawn now passes it to Creature(xp_value=...).
    • Killing a spawned monster or NPC now gives exactly that much XP.

i18n: three new player-facing strings, added to messages.pot and the ru .po, and the .mo recompiled. I added the .pot entries by hand because running make messages on main produces about 470 lines of unrelated churn: the .pot is stale on main. No frontend i18n was needed, since the WS error text is rendered on the server.

Tests

New file tests/unit/test_api_boundary_bounds.py with 43 cases, all going through the real FastAPI app or WS unless noted:

  • frontend-error:
    • the reporter payloads are logged
    • logged fields are truncated
    • invalid bodies ({"message": 5}, a non-string stack, [], non-JSON) get 422
    • an over-long field gets 422
    • an oversized body gets 413
  • WS size limit:
    • an oversized player message gets an error and the round still works afterwards
    • a multibyte message that is under the limit in chars but over it in bytes is rejected
    • an oversized spectator message gets an error and the loop continues
  • Bad params over WS (a string, a list, over-long text) get a protocol error and the loop continues.
  • parse_action unit cases: non-object params, non-string keys, too many entries, over-long string, and the accepted boundary values.
  • Layer file: oversized content gets 422 on body.content before the service is called.
  • GiveItemRequest: 12 out-of-range cases get 422 and the inventory is unchanged; an in-range reach weapon is accepted.
  • Spawn xp_value: for both monster and NPC, spawning through the API and then killing through combat_resolution.handle_death grants exactly xp_value and emits XP_GAINED. A negative xp_value gets 422 and nothing is spawned.

Checked against main: I ran the same test file against main's src/ (with the new constants inlined), and 40 of 43 fail. The 3 that pass on both are the happy-path guards: reporter payloads logged, bounded params accepted, in-range weapon given.

Checks:

  • make check is green: ruff, mypy (185 files), 2978 backend unit tests, and frontend eslint/tsc/vitest (353 tests).
  • Integration: Docker has no daemon in this environment, so I ran tests/integration/ against a local uvicorn backend with the docker-compose.test.yml settings (DND_DICE_SEED=42, DND_CONTENT_DIR=tests/integration/content, BACKEND_URL=http://localhost:8001). All 184 passed.

Real bug found by the new tests (fixed here)

  • Symptom: POST /api/frontend-error with a JSON body that is not an object (for example [] or "x"), or with a non-JSON body, returned 500 from an unhandled exception instead of a client error.
  • Root cause: the handler called body.get(...) on whatever request.json() returned and had no validation.
  • Fix: item 1 (typed model; a pydantic ValidationError maps to 422 through the existing app handler).
  • Test that catches it: TestFrontendErrorReport::test_invalid_body_is_422[[]] / [not json].

A second one:

  • Symptom: xp_value sent on NPC spawn was silently ignored.
  • Root cause: the field was missing from the request model.
  • Fix: item 6.
  • Test: test_spawned_creature_awards_its_xp_value_on_kill[npc].

Found but not fixed (out of scope)

  • The say text inputs (Perception.tsx, NpcInspectModal.tsx) have no maxLength. The server now rejects text over 2000 chars with an error; the UI could add maxLength={2000} to prevent it up front.
  • messages.pot is stale on main: regenerating it adds or reorders about 30 msgids unrelated to this change.
  • PUT /api/master/worlds/{id}/entities/... and PUT /api/master/catalogs/... take body: dict[str, Any] with no size bound. Pydantic content models validate the shape afterwards.
  • SpawnCreatureRequest has no length bounds on id/name/personality, and its attacks: list[dict] is unbounded.
  • The frontend SpawnCreatureRequest TS type does not include xp_value; no GM UI sends it yet.

- POST /api/frontend-error: typed FrontendErrorReport (message, stack,
  component, url) with max_length bounds; the body is read as a stream and
  rejected with 413 above 128 KiB, invalid bodies get 422 instead of a 500,
  and each logged field is truncated to 4000 chars
- WebSocket (player and spectator): messages above 64 KiB (UTF-8 bytes) get
  a localised error reply and the loop keeps running
- parse_action: params that are not an object (or have non-string keys),
  carry more than 16 entries or a string over 2000 chars are rejected with
  a localised protocol error instead of being silently replaced by {}
- UpdateLayerFileRequest.content capped at 1 MiB
- GiveItemRequest: 5e-sensible numeric ranges (price, reach, base_ac,
  max_dex_bonus, strength_req, ac_bonus) and string/list size caps
- SpawnCreatureRequest accepts xp_value (>= 0); spawned monsters and NPCs
  award it on kill
@vladmesh

Copy link
Copy Markdown
Owner Author

Independent review: PASS

  • All six W3-1 items are covered:
    • /api/frontend-error has a typed model, a 413 streaming cap and truncation of logged fields. Its 500 on a body that is not an object is fixed.
    • WS messages are capped at 64 KiB, measured in UTF-8 bytes, for both player and spectator. The loop keeps running after a rejection.
    • Layer file content is capped at 1 MiB.
    • GiveItemRequest has range and length bounds. The GM dialog payload (reach 5, no armor fields) still fits.
    • parse_action rejects params that are not an object or have non-string keys, and caps params by count and string length. A null or missing params still means {}.
    • SpawnCreatureRequest.xp_value is accepted and reaches the kill reward.
  • Checks: local make check is green, and required CI is green, including integration.
  • Regression check: I ran the new test file against main's src/, with only the new constants and exception class stubbed so it imports. 36 of 39 selected cases fail, and the 3 happy-path guards pass. This matches the PR's own figure of 40/43.
  • Non-blocking:

@vladmesh
vladmesh merged commit c7b2837 into main Sep 29, 2026
4 checks passed
@vladmesh
vladmesh deleted the claude/api-bounds-wpm1u9 branch September 29, 2026 09:07
vladmesh pushed a commit that referenced this pull request Sep 29, 2026
Resolve routes_ws.py: keep #82's message-size and params bounds and send
the params errors through _send_session_error so they use the session
language (the size error is already rendered under the session language
by the envelope parse). Keep both sides' ru translations, regenerate
messages.pot, recompile the .mo, and extend the ru-session WS test with
the three new errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant