Skip to content

Scope stored Responses records by session key - #41209

Open
necoweb3 wants to merge 3 commits into
NousResearch:mainfrom
necoweb3:fix/responses-session-key-scope
Open

necoweb3 wants to merge 3 commits into
NousResearch:mainfrom
necoweb3:fix/responses-session-key-scope

Conversation

@necoweb3

@necoweb3 necoweb3 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Stored Responses API records now remember the X-Hermes-Session-Key that created them, and response chaining, retrieval, and deletion only succeed for the same session key. Legacy stored records without an owner key remain readable for backward compatibility.

The fix also prevents globally named conversation mappings from forcing a different session key into another caller's stored response chain; inaccessible mappings are treated as a fresh conversation.

Why

POST /v1/responses already accepts X-Hermes-Session-Key for long-term memory scoping, but stored response lookup was keyed only by response id. In authenticated multi-client API server deployments, a caller with another response id could use previous_response_id, GET /v1/responses/{id}, or DELETE /v1/responses/{id} without a same-key ownership check.

How This Fixes It

  • Store the creating gateway_session_key alongside each persisted Responses API record.
  • Add a shared visibility check that allows legacy ownerless records but requires owned records to match the caller's current X-Hermes-Session-Key.
  • Apply that check to previous_response_id chaining, named conversation lookup, GET /v1/responses/{response_id}, and DELETE /v1/responses/{response_id}.
  • Treat an inaccessible named conversation mapping as a fresh conversation so one session key cannot force another session key's stored chain.
  • Add regression tests for cross-key read, delete, chaining, and conversation-name reuse.

Tests

python -m pytest tests/gateway/test_api_server.py -k stored_responses_are_scoped_to_session_key -q --timeout-method=thread
python -m pytest tests/gateway/test_api_server.py -k "SessionKeyHeader or stored_responses_are_scoped_to_session_key or conversation_names_are_not_reused" -q --timeout-method=thread
python -m pytest tests/gateway/test_api_server.py -k "ResponsesEndpoint or GetResponse or DeleteResponse" -q --timeout-method=thread
python -m pytest tests/gateway/test_api_server.py -k "not access_refreshes_lru" -q --timeout-method=thread

Note: the full file currently exposes an unrelated pre-existing TestResponseStore.test_access_refreshes_lru failure on this checkout because LRU timestamps can tie inside the same second.

@daimon-nous daimon-nous Bot added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists labels Jun 7, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Verified — session-key scoping on stored Responses API records

Reviewed the diff for gateway/platforms/api_server.py and tests/gateway/test_api_server.py.

  • Session isolation: Confirmed _response_visible_to_session_key() gates all stored-record access paths: GET /v1/responses/{id}, DELETE /v1/responses/{id}, POST /v1/responses with previous_response_id, and conversation-name chaining. Callers with a session key cannot read, delete, or chain from records belonging to a different key.
  • Backward compatibility: Records without a gateway_session_key remain visible to all callers (not owner_key short-circuits to True), which is the correct behavior for pre-scoping records and headerless clients.
  • Persistence: gateway_session_key is stored in _persist_response_snapshot() on both initial and follow-up responses, so scoping applies to the full conversation chain.
  • Test coverage: Two async tests verify cross-key isolation (read/delete/chain return 404) and conversation-name independence across session keys.

The fix is correct and complete. No issues found.

@necoweb3
necoweb3 force-pushed the fix/responses-session-key-scope branch from 93be75c to 33810fb Compare June 26, 2026 16:26
@necoweb3

Copy link
Copy Markdown
Contributor Author
Rebased this PR onto current `origin/main` and reran the focused Responses API / ResponseStore coverage.

Validation:

```text
python -m pytest tests/gateway/test_api_server.py -k "ResponseStore or responses" -q --timeout-method=thread
36 passed, 1 skipped

@necoweb3 necoweb3 closed this Jun 28, 2026
@necoweb3
necoweb3 deleted the fix/responses-session-key-scope branch June 28, 2026 10:45
@necoweb3
necoweb3 restored the fix/responses-session-key-scope branch July 1, 2026 00:01
@necoweb3 necoweb3 reopened this Jul 1, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for addressing a real cross-client isolation gap. Current main still stores Responses records without an owner key at gateway/platforms/api_server.py:3511-3516, retrieves/deletes them by ID alone at :3531-3559, and follows previous_response_id without a key comparison at :3325-3333.

Problems

  • The patch also changes /v1/runs response chaining, but the added tests do not cover that endpoint. Current main accepts previous_response_id at gateway/platforms/api_server.py:4243 and loads it from ResponseStore at :4266-4272; add a cross-key regression case to preserve the intended isolation guarantee.

Suggested changes

  • When salvaging onto current main, relocate the persistence changes to both current storage paths: streamed _persist_response_snapshot() at gateway/platforms/api_server.py:2750-2767 and non-streaming storage at :3509-3520, then re-check GET/DELETE and Runs against their current locations.

Automated hermes-sweeper review.

@@ -3599,6 +3599,95 @@ async def test_responses_endpoint_accepts_session_key(self, auth_adapter):
call_kwargs = mock_run.call_args.kwargs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add the analogous cross-key regression for /v1/runs with previous_response_id: this PR changes that handler too, but this test only covers /v1/responses retrieval, deletion, and chaining.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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 14, 2026
@necoweb3
necoweb3 force-pushed the fix/responses-session-key-scope branch from 33810fb to d1f1870 Compare July 14, 2026 05:59
@necoweb3
necoweb3 force-pushed the fix/responses-session-key-scope branch from d1f1870 to aed7e9f Compare July 14, 2026 06:18
@necoweb3

necoweb3 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Updated the branch again to fix the failing CI slice after rebasing onto current main.

The failure was unrelated to the session-key change: two newly added model probe tests were still mocking urllib.request.urlopen, while probe_api_models() now goes through _urlopen_model_catalog_request(). Updated those tests to mock the current helper path.

Validation:

  • python -m pytest -p no:cacheprovider tests/hermes_cli/test_model_validation.py -q --timeout-method=thread — 89 passed
  • python -m pytest -p no:cacheprovider tests/gateway/test_api_server.py -k "session_key" -q --timeout-method=thread — 12 passed, 187 deselected
  • python -m ruff check --no-cache gateway/platforms/api_server.py tests/gateway/test_api_server.py tests/hermes_cli/test_model_validation.py — passed

@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
@andrexibiza

Copy link
Copy Markdown
Contributor

Rebase-on-behalf delivery — this PR's branch was rebased onto current main (9d6c5a920c7) and the conflicts resolved, so it can land.

Rebased branch: andrexibiza:rebase-on-behalf/41209 (commits 7e08ae6b8f0 + f2d6a333aa6, original authorship preserved)

Conflict resolution (3 hunks):

  • gateway/platforms/api_server.py (2 hunks) — _response_store.put calls: kept main's evolved session_id values (session_id_snapshot or session_id / _effective_session_id) AND added this PR's gateway_session_key field (the scoping feature).
  • tests/gateway/test_api_server.py (1 hunk) — route table: kept main's platform-events route AND added this PR's /v1/runs route.

To adopt (author):

git fetch https://github.com/andrexibiza/hermes-agent.git rebase-on-behalf/41209
git checkout fix/responses-session-key-scope
git reset --hard FETCH_HEAD
git push --force-with-lease origin fix/responses-session-key-scope

py_compile verified on the rebased tree. This unblocks the api_server R4 extraction window (hunk gate).

@andrexibiza

Copy link
Copy Markdown
Contributor

Interlock binding — this PR addresses the api_server.py extraction window of EPIC #78647 (shard #78643). Rebase-on-behalf delivery: andrexibiza:rebase-on-behalf/41209.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists 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 sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants