Skip to content

fix: isolate profile cookie per webui instance#1756

Closed
ng-technology-llc wants to merge 1 commit intonesquena:masterfrom
ng-technology-llc:fix/profile-cookie-isolation
Closed

fix: isolate profile cookie per webui instance#1756
ng-technology-llc wants to merge 1 commit intonesquena:masterfrom
ng-technology-llc:fix/profile-cookie-isolation

Conversation

@ng-technology-llc
Copy link
Copy Markdown
Contributor

@ng-technology-llc ng-technology-llc commented May 6, 2026

Summary

  • Adds WEBUI_PROFILE_COOKIE_NAME so multi-instance WebUI deployments can isolate the active-profile cookie per process.
  • Keeps the default cookie name as hermes_profile for backwards compatibility.
  • Adds regression coverage for default behavior, custom cookie read/write, and ignoring stale default cookies when a custom cookie name is configured.

Why

Browsers share cookies across ports on the same host. When multiple Hermes WebUI instances run on the same hostname but different ports, a profile switch in one instance can send the same hermes_profile cookie to another instance. That other instance may then treat the foreign profile name as active and initialize profile-scoped state under the wrong Hermes home.

This keeps the existing per-client cookie + thread-local profile isolation model, but lets operators choose a distinct profile cookie name per WebUI process.

Test plan

  • python -m pytest tests/test_issue803.py::TestProfileCookieHelpers -q
  • python -m pytest tests/test_issue803.py tests/test_issue798.py tests/test_issue1611_session_profile_filtering.py -q
  • python -m pytest tests/test_sprint31.py tests/test_settings_navigation_and_detail_refresh.py -q

@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Reading api/helpers.py:252-294 on origin/master, the diff at HEAD, and server.py:122-156 to confirm where get_profile_cookie() is consumed — the change is small and surgical, the regression coverage in tests/test_issue803.py:76-106 looks right, and the motivation is real: browsers do scope cookies by host, not host+port, so two WebUI instances on 127.0.0.1:8787 and 127.0.0.1:8788 will trample each other's hermes_profile cookie. This is the same class of bug WEBUI_AUTH_COOKIE_NAME already addresses for auth.

A couple of small edges worth tightening before merge:

Empty-string env var slips through

os.getenv() returns the literal value when the variable is set, including "". So if an operator does export WEBUI_PROFILE_COOKIE_NAME= (or sets it to empty in Docker compose), the helper will return "":

def get_profile_cookie_name() -> str:
    return os.getenv('WEBUI_PROFILE_COOKIE_NAME', PROFILE_COOKIE_NAME)

That feeds cookie[''] = name in build_profile_cookie and cookie.get('') in get_profile_cookie, which silently produces a malformed Set-Cookie header (the morsel name is empty) and reads nothing back. Suggest or-coalescing instead of relying on the default:

def get_profile_cookie_name() -> str:
    name = os.getenv('WEBUI_PROFILE_COOKIE_NAME', '').strip()
    return name or PROFILE_COOKIE_NAME

One extra test asserting "empty env var falls back to hermes_profile" would lock that in.

No validation of cookie-name shape

RFC 6265 cookie names can't contain whitespace, ;, =, or controls. If someone sets WEBUI_PROFILE_COOKIE_NAME=hermes profile main, http.cookies.SimpleCookie will raise CookieError on assignment and the response will 500. Two reasonable options:

  1. Validate at startup (in server.py boot) and log+abort with a clear error if invalid.
  2. Validate inside get_profile_cookie_name() via a _TOKEN_RE and silently fall back to PROFILE_COOKIE_NAME if it doesn't match.

Option 1 is more honest — a misconfigured cookie name is an operator error, not something to paper over. Either way it's worth at least one test exercising the invalid-name path so the behaviour is pinned.

Cookie clearing path

I don't see a corresponding "clear cookie" / "logout" path in the diff that would need updating, and the existing tests in tests/test_issue803.py:42-72 only cover read/build. Worth a quick grep -n "hermes_profile" api/ confirmation that all writers go through build_profile_cookie — from my read, routes.py:4036-4048 is the only Set-Cookie site for the profile cookie, so this should be fully covered.

Cross-port cookie scope is the right framing

The PR description correctly identifies the underlying browser behaviour. For operators who want shared profile state across instances on the same host, the existing default keeps working; for operators who want isolation, the env var gives them an opt-in escape hatch without breaking any existing deployment. That's the right shape for this change.

Verdict

LGTM in principle — the two edges above (empty-string fallback, invalid-name handling) are worth addressing before merge but neither is large. Test coverage is in the right place and reads cleanly against the existing TestProfileCookieHelpers style.

@ng-technology-llc ng-technology-llc force-pushed the fix/profile-cookie-isolation branch from ab2c2b5 to 8385f90 Compare May 6, 2026 19:17
nesquena-hermes added a commit that referenced this pull request May 6, 2026
v0.51.14 — 4-PR contributor batch (#1756, #1757, #1760, #1761)
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Thanks @ng-technology-llc — this shipped in v0.51.14 (commit 2106083) as part of a 4-PR full-sweep contributor batch. Stage rebased your branch onto current master, ran the full pre-release gate (4649 pytest, browser tests, Opus advisor verdict SHIP all 4), and merged via release PR #1763.

GitHub didn't auto-close because the merge commit only references the squash-merged stage branch, not your fork's commit directly — closing manually for hygiene.

Live now on existing installs after git pull + restart.

Release notes: https://github.com/nesquena/hermes-webui/releases/tag/v0.51.14

iosub pushed a commit to iosub/HERMES-hermes-webui2 that referenced this pull request May 6, 2026
iosub pushed a commit to iosub/HERMES-hermes-webui2 that referenced this pull request May 6, 2026
nesquena#1757, nesquena#1760, nesquena#1761)

Constituent PRs:
- nesquena#1760 (@ai-ag2026) preserve pending user turn on stream errors. Closes nesquena#1361.
- nesquena#1761 (@dso2ng) scope terminal stream cleanup to owner session. Refs nesquena#1694.
  AUTO-FIX applied: restored !INFLIGHT[S.session.session_id] disjunct in
  _setActivePaneIdleIfOwner (regression introduced by helper centralization).
- nesquena#1756 (@ng-technology-llc) isolate profile cookie per webui instance. Closes nesquena#803.
- nesquena#1757 (@skspade) tri-state gateway status (alive: True/False/None).

Tests: 4642 → 4662 collected (+20). 4649 passed, 9 skipped (test-isolation
prong-2 noise), 3 xpassed, 0 failed in 152s.

Pre-release verification:
- All 4 PRs CI-green or rebased clean (nesquena#1757 had stale base; CHANGELOG conflict
  auto-resolved by dropping the PR's redundant entry).
- node -c clean on static/messages.js + static/panels.js.
- 11/11 browser API endpoints PASS.
- Pre-stamp re-fetch: all PR heads match local rebases.
- Opus advisor: SHIP, all 5 verification questions clean, 0 MUST-FIX, 0 SHOULD-FIX.
- Two NICE-TO-HAVE coverage gaps absorbed in-release:
  (1) test_sprint36.py asserts !INFLIGHT[...] disjunct in helper body
  (2) test_issue1361_cancel_data_loss.py adds structural-grep test to pin
      _materialize_pending_user_turn_before_error call sites at error branches.

Closes nesquena#803, nesquena#1361, nesquena#1694.
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.

2 participants