Skip to content

Answer malformed walk and API requests with their documented 4xx - #78

Merged
dmccoystephenson merged 1 commit into
mainfrom
feature/walks-malformed-requests
Nov 5, 2026
Merged

dmccoystephenson merged 1 commit into
mainfrom
feature/walks-malformed-requests

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

  • A negative Content-Length is now treated as absent by _contentLength, so rfile.read(-1) is no longer reached and the size caps are enforced. 411 is returned by POST /api/walks and PUT .../versions/<version>, and 400 is returned by POST .../current.
  • The connection is now always closed when a Content-Length is present but unusable (negative or non-numeric), because the end of the body is unknown. The close is set inside _contentLength, so the non-numeric case in _drain and _rollback, where the connection was previously left open, is covered too. The close already given to the negative case by _drain is preserved.
  • RecursionError is now caught in _leaveWalk and _rollback, and 400 is returned.
  • An integer too large for a float is now refused by walks._number, so Refused is raised by clean and 400 is returned.
  • The 411 wording in the README's PUT and walks rows and the CHANGELOG [Unreleased] section were updated.

Closes #73

Test plan

  • python3 -m pytest -q on Python 3.8.10: 421 passed, 13 skipped
  • New tests were confirmed to fail without the fix: 9 failed with server.py and walks.py stashed, and all passed once they were restored
  • CI: test (3.8), test (3.12), docker. The check-registry step and black could not be run locally in this dispatch because both needed approval, so check-registry is gated by CI and black was not run

Notes for review

  • Traefik normally rejects these requests before they reach arcade, and that path was not exercised. Only a local makeServer over a raw socket was tested.
  • Forms such as +5 and 1_000 are still accepted by int(). These were left as they are, since a usable length is yielded and they were outside the scope of POST /api/walks can leave malformed requests unanswered #73.
  • The token-checking lines in _authorise and the body of _drain were not changed. The handling of a bad length in _drain is inherited from _contentLength.

Issues not picked this cycle

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

🤖 Generated with Claude Code

A negative Content-Length passed the size cap and read until the client
hung up; it now counts as absent. A value that is present but unusable
closes the connection, since the end of the body is unknown. Bodies
nested too deeply to parse and coordinates too large for a float get a
400 instead of a dropped connection.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric (a self-review: no independent reviewer was involved):

  • Scope: PASS — six files were changed, and all are needed for POST /api/walks can leave malformed requests unanswered #73 (fixes, tests, README wording, CHANGELOG).
  • Tests-new: PASS — no public function was added; _contentLength and _number are exercised by new server and walk tests.
  • Tests-fix: PASS — with server.py and walks.py stashed, 9 of the selected tests failed; after git stash pop, tests/test_server.py and tests/test_walks.py gave 105 passed, 3 skipped. The non-numeric upload and walk cases passed in both states, since a 411 and a close were already given there; they are kept as guards.
  • Sibling structure: PASS — tests were added beside their neighbours, using the existing arcade fixtures. No new file was added.
  • Sibling renames: PASS — nothing was renamed.
  • Docs: PASS — the Phase 7 rows for the API table and CHANGELOG were checked against server.py. "Two finite numbers" in the walks row and "not that JSON" in the rollback row remain accurate without a change.
  • Issue resolution: PASS — all three table rows and all three suggested fixes in POST /api/walks can leave malformed requests unanswered #73 are covered. In the deep-nesting case on Python 3.12 the parse may succeed on some builds, and a 400 is still given because the payload is then refused.
  • CI: PASS — test (3.8), test (3.12), docker and engine were green on the PR head, which also covers check-registry.
  • Stdlib-only: PASS — no import was added under src/arcade/; import socket was added in tests/test_walks.py only.
  • Py3.8: PASS — a scan of added lines found no listed construct, and test (3.8) is green. The suite was also run on Python 3.8.10 locally.
  • Vendored client untouched: PASS — src/arcade/trace_client.py is not in git diff --name-only.
  • Naming: PASS — the new locals value and length and the test helper _untilClosed follow the conventions.
  • Isolation headers: PASS — no response path was added; the 400 and 411 replies still go through _json.
  • Config documented: PASS — no ARCADE_* name was changed.
  • CLI documented: PASS — no subcommand or flag was changed.
  • Changelog: PASS — an entry was added under [Unreleased], and __version__ was not changed.

Findings and gaps:

  • black was not run locally because approval was needed for it in this dispatch, so formatting is UNVERIFIED. It is not a CI job. The changed source lines were kept under the 110 limit by hand.
  • src/arcade/server.py:790 — Content-Length values such as +5 and 1_000 are still accepted by int(). This was left out of scope. A stricter parse could be considered separately, since a proxy may read such a value differently.
  • No path in git diff --name-only is on the do-not-auto-merge list, and no server.py hunk touches hmac, Authorization, hashToken, ISOLATION_HEADERS or CORS.

This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit ce9109e into main Nov 5, 2026
4 checks passed
@dmccoystephenson
dmccoystephenson deleted the feature/walks-malformed-requests branch November 5, 2026 10:32
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Correction to the PR description and self-review: black and check-registry were reported as not runnable locally, and that was inaccurate. Only the forms that were tried needed approval. The module forms listed in Stephenson-Software/arcade-dev-loop#1 were available, and they were run on main after the merge (ce9109e):

  • check-registry was run in-process: examples/games.yaml: 1 game(s) OK.
  • python3 -m black --check --diff was run on src/arcade/server.py and src/arcade/walks.py. Both would be reformatted, but every reported hunk is on a line that was not changed by this PR, so the drift is pre-existing. tests/test_server.py and tests/test_walks.py were not checked.

The merge was made while black was still recorded as UNVERIFIED. It is not a CI job and it was not a merge gate, so no change is needed.

This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

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.

POST /api/walks can leave malformed requests unanswered

1 participant