Skip to content

fix(connection): ignore responses whose id is not a RequestId - #167

Open
PerryLink wants to merge 1 commit into
agentclientprotocol:mainfrom
PerryLink:fix/ignore-responses-with-an-invalid-id
Open

PerryLink wants to merge 1 commit into
agentclientprotocol:mainfrom
PerryLink:fix/ignore-responses-with-an-invalid-id

Conversation

@PerryLink

Copy link
Copy Markdown
Contributor

Summary

Connection._handle_response uses the message id as a dictionary key without checking it first:

request_id = message["id"]
future = self._pending.pop(request_id, None)

The same file already validates a request id in two other places with _is_request_id in _track_incoming and _cancel_incoming -- but not here. A peer's response frame can therefore do two things.

An unhashable id escapes the receive loop. {"id": [], ...} raises TypeError on the lookup, the exception leaves _receive_loop, and the connection disconnects with the valid frame queued behind it never handled:

pop([]) -> RAISED TypeError: cannot use 'list' as a dict key (unhashable type: 'list')
after id=[]     : RAISED TypeError: cannot use 'list' as a dict key   <-- escapes the receive loop

true and 1.0 alias onto the integer 1. They hash and compare equal, so they resolve a different request's future:

pop(True) -> 'future of request id 1'   (hash equal to 1: True)
pop(1.0)  -> 'future of request id 1'   (hash equal to 1: True)
after id=True   : future done=True result='HIJACKED'  <-- aliased onto integer 1

That second case is the one _track_incoming already comments on: "Python would alias an invalid one such as true or 1.0 with the integer 1." The guard exists for the outgoing direction and is missing for the incoming one.

Change

src/acp/connection.py: ignore a response whose id is not a JSON-RPC RequestId, matching the two existing call sites.

if not _is_request_id(request_id):
    return

_is_request_id is unchanged: it accepts null, an integer and a string, which is exactly the protocol's RequestId.

Testing

New case in tests/test_connection_recovery.py, beside the #153 frame-shape test: four responses carrying [], {}, true and 1.0, followed by a valid request. It asserts the integer-1 request is still pending, the pending map is untouched, and the frame behind the bad ids was still handled.

$ uv run pytest tests/test_connection_recovery.py -q
8 passed

Written against the defect: removing only the new guard fails the case, because the TypeError stops the loop before the following frame is processed.

--- guard removed ---
E       AssertionError: assert [] == ['request']
FAILED tests/test_connection_recovery.py::test_receive_loop_ignores_responses_with_an_invalid_id

Full suite, and the lint pass:

$ uv run python -m pytest -q --doctest-modules
1 failed, 407 passed, 2 skipped

$ uv run ruff format / ruff check
All checks passed!

The one failure is pre-existing on this machine and unrelated: tests/real_user/test_stdio_limits.py::test_spawn_stdio_transport_custom_limit_handles_large_line compares a line length of 71682 against 71681, which is a CRLF artefact of Windows stdio rather than anything in this change.

Checklist

  • Conventional Commit prefix (fix:)
  • Summarised exercised behaviours, with targeted pytest output above
  • No docs/examples change: no user-visible API or transport behaviour changes, only an invalid frame is now ignored instead of tearing the connection down

`_handle_response` looks the message id up in `_pending` without checking it first.
The same file validates a request id with `_is_request_id` in `_track_incoming` and
`_cancel_incoming`, but not here, so a peer's frame can do two things:

- An unhashable id (an array or an object) raises `TypeError` on the lookup. The
  exception escapes `_receive_loop`, the connection disconnects, and the valid frame
  queued behind it is never handled.
- `true` and `1.0` are equal to the integer `1`, so they resolve the future of the
  integer-1 request -- the same aliasing `_track_incoming` already refuses.

Ignore a response whose id is not a JSON-RPC `RequestId`, matching the two existing
call sites.

This branch has not been deployed

No deployments
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