Release PD (v0.51.443): [security] scope session by-id reads + exports to active profile (#3982, #3991)#4269
Conversation
…ctive profile (paired, re-cut onto v0.51.442, conflict-resolved properly)
…profile (#3982, #3991) Paired Hinotoi-agent security PRs, re-cut onto v0.51.442 (avoided the stale-base revert of the #4171 passkey gate that Codex caught). Both /api/session and /api/session/export now apply _profiles_match→404. Codex SAFE + Opus SHIP + 49 tests green; passkey gate verified intact. Co-authored-by: Hinotoi-agent <Hinotoi-agent@users.noreply.github.com>
|
| Filename | Overview |
|---|---|
| api/routes.py | Adds profile-scoping guard to GET /api/session and GET /api/session/export; uses a new _session_visible_to_active_profile helper for the detail paths and an inline _profiles_match check for export; profile check positioned correctly (before any data emission) in all three places. |
| tests/test_issue1611_session_profile_filtering.py | Adds 6 new tests covering foreign-profile rejection and same-profile pass-through for both the detail endpoint (WebUI session, metadata-only, cookieless, CLI fallback) and the export endpoint; all tests properly mock the profile name, session loader, and response sinks. |
| tests/test_issue2762_state_sync_profile_kwarg.py | Adds a monkeypatch for _get_active_profile_name to return "maiko" so the new profile gate does not block the pre-existing metadata-only load test that was already operating in that profile. |
| tests/test_webui_state_db_reconciliation.py | Adds monkeypatch for _get_active_profile_name to keep the deferred-model-resolution test passing now that the detail endpoint enforces the profile boundary. |
| CHANGELOG.md | Adds v0.51.443 release entry with a Security section describing the two fixed by-id endpoints. |
Sequence Diagram
sequenceDiagram
participant C as Client
participant R as routes.py
participant P as api/profiles.py (TLS)
participant S as Session Store
Note over R,P: GET /api/session?session_id=…
C->>R: "GET /api/session?session_id=X"
R->>S: get_session(X)
S-->>R: "session (profile=other)"
R->>P: _get_active_profile_name() [TLS]
P-->>R: default
R->>R: "_session_visible_to_active_profile(other, handler) = False"
R-->>C: 404 Session not found
Note over R,P: GET /api/session?session_id=… (CLI fallback)
C->>R: "GET /api/session?session_id=Y"
R->>S: get_session(Y) raises KeyError
R->>R: check SESSION_INDEX_FILE
R->>S: _lookup_cli_session_metadata(Y)
S-->>R: "profile=other"
R->>P: _get_active_profile_name() [TLS]
P-->>R: default
R->>R: "_session_visible_to_active_profile(other, handler) = False"
R-->>C: 404 Session not found
Note over R,P: GET /api/session/export?session_id=…
C->>R: "GET /api/session/export?session_id=Z"
R->>S: get_session(Z)
S-->>R: "session (profile=other)"
R->>P: get_active_profile_name() [TLS]
P-->>R: default
R->>R: "_profiles_match(other, default) = False"
R-->>C: 404 Session not found
Reviews (1): Last reviewed commit: "Release PD: [security] scope session by-..." | Re-trigger Greptile
| def _session_visible_to_active_profile(session_profile, handler=None) -> bool: | ||
| """Return whether a detail-load session belongs to the active profile. | ||
|
|
||
| Real request handlers must enforce the same profile boundary as | ||
| /api/sessions, even when the request has no hermes_profile cookie and the | ||
| process-level active profile is the default/root profile. Direct unit-callers | ||
| without a request handler keep the historical metadata-load behavior. | ||
| """ | ||
| if handler is None: | ||
| return True | ||
| active_profile = _get_active_profile_name() | ||
| if not isinstance(session_profile, str): | ||
| session_profile = None | ||
| return _profiles_match(session_profile, active_profile) |
There was a problem hiding this comment.
handler parameter acts only as a sentinel
_session_visible_to_active_profile accepts handler but never reads any value from it — it only checks handler is None to decide whether to enforce the profile boundary. The actual profile is read from _get_active_profile_name() (TLS), not from the handler object. A reader unfamiliar with the codebase will naturally expect the handler to be consulted for cookie/header data, and might look for profile extraction code that isn't there.
A boolean flag (e.g. enforce: bool = False) would make the contract explicit, or the docstring could note explicitly that profile resolution is TLS-based and the handler is a presence-only sentinel.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| active_profile = get_active_profile_name() | ||
| if not _profiles_match(getattr(s, "profile", None), active_profile): | ||
| return bad(handler, "Session not found", 404) |
There was a problem hiding this comment.
Export uses inline check while detail paths use the helper
The two detail-path guards route through _session_visible_to_active_profile (which normalises non-string profiles to None before passing to _profiles_match), but the export guard calls _profiles_match directly with the raw getattr value. For the current codebase this makes no observable difference because profile is always a string or None. However, if a session ever carried a non-string truthy profile (e.g., an integer migration artefact), the export path would evaluate row = 42 or 'default' → 42, which would not match 'default', producing a spurious 404 for a same-profile session. Routing through _session_visible_to_active_profile (or at minimum mirroring the isinstance normalisation) would keep the three paths consistent.
Release PD — v0.51.443
Ships #3982 + #3991 (Hinotoi-agent, paired) —
[security]scope session by-id reads + exports to the active profile. Nathan-approved to ship together; self-rebased onto v0.51.442 and gated fresh (Codex + Opus + full suite).Threat closed
/api/sessions(the list) scopes rows to the active profile, but two by-id endpoints did not:GET /api/session?session_id=…(transcript/metadata read + CLI-session fallback)GET /api/session/export?session_id=…(full transcript JSON download)Both loaded a session purely by id, so a stale/leaked/guessed session id from another profile could disclose that profile's transcript or export. Both now apply
_profiles_match(...)and return the same404used for missing sessions (no foreign-profile existence disclosure). Default/legacy-root aliasing preserved;/api/sessionsunchanged.First stage attempt was cut from an old base; the 3-dot diffs (180/401 commits behind) would have reverted the #4171 passkey gate shipped minutes earlier in v0.51.442. Codex caught it; I re-cut onto current master and re-verified the passkey gate is intact (
_require_passkey_registration_authpresent + called on both registration endpoints).Gate results
_profiles_match→404reused, 49 passing tests covering reject + same-profile/alias allow.These are the surgical, proven subset of the parked draft #3999 (which stays parked for the broader ~44-site sweep). Closes #3982, #3991.
Co-authored-by: Hinotoi-agent