[security] fix(auth): bind active profile cookie to session#4023
Conversation
|
| Filename | Overview |
|---|---|
| api/auth.py | Adds sign_profile_cookie_value and verify_profile_cookie_value helpers; both call verify_session() before proceeding, so expired/revoked sessions are correctly rejected. Also adds _resolve_cookie_name() for configurable auth cookie name. Core HMAC scheme is sound: profile names cannot contain dots, so rsplit('.', 1) reliably separates the name from its SHA-256 signature. |
| api/helpers.py | Updates get_profile_cookie to call verify_profile_cookie_value when auth is enabled, and build_profile_cookie to raise RuntimeError on signing failure instead of silently returning an unsigned value. Exception handlers now log with exc_info=True. |
| api/routes.py | Adds _get_or_materialize_session and updates the three mutation endpoints; wires handler into build_profile_cookie on the profile switch path. The messaging-session creation block is unreachable dead code. |
| api/config.py | Introduces _get_providers_cfg() and _get_provider_cfg() helpers that guard against non-dict providers values, and threads model context into set_reasoning_effort. |
| tests/test_issue803.py | Adds comprehensive regression coverage for the profile cookie security fix: valid signed cookie, unsigned forgery rejection, wrong-session rejection, emitted signed cookie verification, and fail-closed behavior. |
Reviews (3): Last reviewed commit: "Bind profile cookie to auth session" | Re-trigger Greptile
406c918 to
194d891
Compare
|
Addressed the Greptile review feedback in
Validation: |
|
Reviewed the diff at The cookie-binding core is correct ✅
sig = hmac.new(_signing_key(), f"profile:{token}:{profile_name}".encode(), hashlib.sha256).hexdigest()
return f"{profile_name}.{sig}"I verified every helper this leans on already exists on master: The scope concernThe PR body says it's "intentionally narrow and limited to active-profile cookie trust," but def _session_visible_to_active_profile(session_profile, handler=None) -> bool:
if handler is None: return True
if getattr(handler, "_hermes_internal_trusted", False): return True
active_profile = _get_active_profile_name()
...
return _profiles_match(session_profile, active_profile)That is effectively the entire fix for issue #4000 ("session-by-id endpoints don't enforce the active-profile boundary"), not just cookie hardening. And it collides directly with two already-open security PRs:
All three edit the same routes file and the same RecommendationSplit this PR. Keep the cookie-trust core here ( Per repo policy I read the tests rather than running them, so the |
194d891 to
4dca506
Compare
|
Addressed the split recommendation in What changed:
Current PR diff file list: Validation rerun on the master-based branch: |
|
Re-fetched after the force-push ( The single Cookie-trust core re-verified
if is_auth_enabled():
val = verify_profile_cookie_value(raw_val, parse_cookie(handler))
return val if val and _valid_profile_name(val) else None
...
# No-auth mode: the cookie is a per-browser UI preference, not an authz
# boundary, so retain the legacy plain profile-name format.
return raw_val if _valid_profile_name(raw_val) else NoneThis is the right shape: auth-on requires a session-bound signature; auth-off keeps the legacy plain The Net: this is now the narrow, reviewable security boundary the PR body claimed, and the session-by-id guard work is correctly parked for the #4000 / #3982 / #3991 track. My structural concern from the last pass is resolved — clearing it on my end. Per repo policy I read the tests rather than running them, so the |
…n gate + require handler when auth enabled Opus independent security review concurred SAFE and surfaced 2 LOW defense-in-depth items, both applied: (1) verify_profile_cookie_value now validates the profile name against _PROFILE_ID_RE itself (not only in get_profile_cookie) so a future second caller can't return an unvalidated name; (2) build_profile_cookie raises when auth is enabled and no handler is passed, so a future call site can't silently emit an unsigned (session-unbound) profile cookie. +3 regression tests.
9e96f5f
…e-pattern gate + require handler when auth enabled Opus independent security review concurred SAFE and surfaced 2 LOW defense-in-depth items, both applied: (1) verify_profile_cookie_value now validates the profile name against _PROFILE_ID_RE itself (not only in get_profile_cookie) so a future second caller can't return an unvalidated name; (2) build_profile_cookie raises when auth is enabled and no handler is passed, so a future call site can't silently emit an unsigned (session-unbound) profile cookie. +3 regression tests.
…s v0.51.368 (Release MG)
Release MG — v0.51.368 — bind active-profile cookie to auth session (nesquena#4023, fixes nesquena#803)
Thinking Path
hermes_profilecookie value.Summary
This PR hardens the active-profile trust boundary used by the profile-scoped session guards added in #3999.
hermes_profileto the currenthermes_sessionwhen WebUI auth is enabled.Security issues covered
hermes_profile=<target-profile>and make guarded session/file operations evaluate under another profileBefore this PR
hermes_profilerequest cookie.get_profile_cookie()validated the value syntactically, but did not authenticate it or bind it to the logged-in WebUI session.After this PR
hermes_sessiontoken./api/profile/switchemits the session-bound profile cookie format when auth is enabled.Why this matters
Profiles are used as a security-sensitive boundary by the new session visibility checks. If the server trusts a raw browser cookie as the authorization input for that boundary, any authenticated client can choose the profile that the guard evaluates against.
That weakens the intended isolation added by #3999: the guard may be present and called correctly, but it can still be evaluated under attacker-supplied profile state.
How this differs from related issue/PR
This is related to #3999, but it is not the same issue as “session-by-id endpoints missing active-profile guards.”
Attack flow
Affected code
server.py,api/helpers.py,api/routes.py,api/auth.pyRoot cause
Issue: client-controlled active profile cookie can influence profile-scoped authorization
server.pysets request-local profile state fromget_profile_cookie(handler)before route handling.get_profile_cookie()previously accepted any syntactically valid profile name from thehermes_profilecookie.api/routes.pyprofile visibility checks compare target session metadata against the request-local active profile.CVSS assessment
CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:H/A:NRationale:
Safe reproduction steps
Enable WebUI auth and create or use two profiles, for example
aliceandbob.Create or identify a session/file operation that should only be visible while
bobis the active authorized profile.Authenticate as a WebUI client with a valid
hermes_session.On vulnerable code, send the guarded request with a forged plain profile cookie:
Cookie: hermes_session=<valid-session>; hermes_profile=bobObserve that the active-profile guard can evaluate the request under
bobbecause the cookie value is accepted as request-local profile state.With this PR, the same unsigned
hermes_profile=bobvalue is ignored when auth is enabled, and a value signed for a different session is also ignored.Expected vulnerable behavior
hermes_profilevalue should never determine the profile used for authorization.Nonefromget_profile_cookie()when auth is enabled.Changes in this PR
sign_profile_cookie_value()andverify_profile_cookie_value()helpers inapi/auth.py.get_profile_cookie()to require a valid session-bound signature when auth is enabled.build_profile_cookie()and/api/profile/switchso profile switches emit the session-bound cookie in auth-enabled deployments.Files changed
api/auth.pyapi/helpers.pyhermes_profilebefore returning it when auth is enabled; preserves no-auth legacy behaviorapi/routes.pybuild_profile_cookie()so the Set-Cookie value can be bound to the current sessiontests/test_issue803.pyMaintainer impact
HttpOnly,SameSite=Lax, path-scoped cookie shape.Fix rationale
The active profile can influence which profile-scoped sessions and files are visible. In auth-enabled deployments, that makes it authorization-relevant state. Binding the profile cookie to the authenticated WebUI session keeps the UI preference mechanism while preventing clients from supplying arbitrary profile names across sessions.
The fix is narrow because it changes only the cookie trust boundary and the profile-switch Set-Cookie path. The regression tests lock the security behavior at the helper boundary where request profile state is derived.
Type of change
Test plan
git diff --check.pytest tests/ -v --timeout=60was not run locally; this PR ran the focused profile/session tests that cover the changed code paths.Executed with:
python3 -m py_compile api/auth.py api/helpers.py api/routes.pypython3 -m pytest tests/test_issue803.py tests/test_session_ops.py tests/test_issue1611_session_profile_filtering.py tests/test_profile_switch_ux.py tests/test_profile_switch_1200.py -qgit diff --checkResult:
101 passed, 1 warningRisks / Follow-ups
hermes_profilecookie may fall back to the default/requested server profile until the user switches profiles again.Contract Routing
This is a security-sensitive behavior fix in the server/profile-cookie path. It does not intentionally change a documented public contract, RFC, UI flow, or product-semantics document. Evidence used: focused helper and session/profile tests listed in the test plan.
Model Used
AI-assisted implementation and PR drafting via Hermes Agent using OpenAI GPT-5.5. The final code changes were validated locally with the commands listed above.
Disclosure notes