Skip to content

fix(api-server): validate previous_response_id type and length (salvage of #2961 by @aydnOktay) - #62758

Open
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:salvage/2961-previous-response-id
Open

fix(api-server): validate previous_response_id type and length (salvage of #2961 by @aydnOktay)#62758
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:salvage/2961-previous-response-id

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Salvages #2961 by @aydnOktay onto current main.

What the original PR fixed

previous_response_id accepted non-strings and unbounded lengths before store lookup.

Why it needed salvage

Still open; endpoint code has grown (multiple handlers). Port validation to current parse paths with tests.

Changes from original

  • Apply validation in both body-parse sites that read previous_response_id
  • Unit tests for invalid type and overlong ID

Testing

python3 -m pytest tests/gateway/test_api_server.py::TestPreviousResponseIdHardening -q

Full credit to @aydnOktay.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for carrying the original hardening forward. The premise is live on current main: both handlers pass a client-provided previous_response_id into ResponseStore.get without validation (gateway/platforms/api_server.py:3285-3286, gateway/platforms/api_server.py:4222-4223), and the PR validates both paths before that lookup.

Problems

  • The added tests cover only POST /v1/responses. The test app registers that route at tests/gateway/test_api_server.py:616, but not POST /v1/runs; production registers /v1/runs at gateway/platforms/api_server.py:4805. The new _handle_runs validation therefore has no regression coverage.

Suggested changes

  • Add invalid-type and overlength requests for POST /v1/runs, by extending the test app helper or invoking _handle_runs directly, and assert the same 400 contract.

This is an automated hermes-sweeper review.

Comment thread tests/gateway/test_api_server.py Outdated
@@ -4119,3 +4120,38 @@ class FakeRunner:
assert adapter._session_model_override_for("chan-1") == {"model": "user/model"}
assert adapter._session_model_override_for("chan-2") is None
assert adapter._session_model_override_for(None) is None


class TestPreviousResponseIdHardening:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests exercise only /v1/responses, but this PR also changes _handle_runs. Please add equivalent 400 regression coverage for POST /v1/runs; the current _create_app helper registers /v1/responses but not that production route.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery labels Jul 11, 2026
@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026
@Bartok9

Bartok9 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Review addressed:

  • `_create_app` now registers production route `POST /v1/runs`.
  • Added `test_runs_previous_response_id_invalid_type_returns_400` and `test_runs_previous_response_id_too_long_returns_400`.

`pytest tests/gateway/test_api_server.py::TestPreviousResponseIdHardening` → 4 passed.

@Bartok9

Bartok9 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (resolved _create_app conflict: kept both /v1/runs and the platforms events route).

Review (jquery/teknium keep_open) was already addressed on this salvage:

  • _create_app registers production POST /v1/runs
  • TestPreviousResponseIdHardening covers invalid-type + overlong for both /v1/responses and /v1/runs

Local: pytest tests/gateway/test_api_server.py::TestPreviousResponseIdHardening → 4 passed.

Ready for re-review when CI is green. Not merging from author side.

@Bartok9
Bartok9 force-pushed the salvage/2961-previous-response-id branch from e46abeb to efa0ca7 Compare July 17, 2026 17:06
…ge of NousResearch#2961 by @aydnOktay)

Rebuilt on latest main (Bartok9 hygiene 2026-08-01).
Original: NousResearch#62758
@Bartok9

Bartok9 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebuilt onto latest main via patch re-apply (force-push). Please re-run CI.

— Bartok9 public PR hygiene 2026-08-01

@Bartok9
Bartok9 force-pushed the salvage/2961-previous-response-id branch from efa0ca7 to c0024ca Compare August 1, 2026 17:37
…tok9

Per-PR attribution so check-attribution passes on this branch (Teknium).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants