From 75153f5b792692b16f39e12358e7a6383c08e8eb Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sat, 11 Jul 2026 02:06:34 +0000 Subject: [PATCH] fix(share): always-on redaction boundary + rebase/UX fixes (gate findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial security re-gate on the rebased head found 4 issues; fixed: 1. [CORE] api/shares.py public snapshot could leak credentials/paths/tool payloads. The sanitizer relied on redact_session_data (gated on the user-toggleable api_redact_enabled) — disabling it exposed API keys, a dict assistant content was stringified as raw tool data, and session.workspace survived in transcript text. Now the per-message sanitizer applies ALWAYS-ON credential redaction (_redact_fn_cached, force=True, ignores the setting) + scrubs known session/workspace/home paths, and refuses to stringify non-text structured content. Title is force-redacted too. Regression test proves the boundary holds with api_redact_enabled=False. 2. [SILENT] api/models.py: my rebase wrongly kept anchor_activity_scenes in METADATA_FIELDS, moving 250-480KB scene bodies before messages and undoing master's metadata fast-path (#5854). Removed; 17 #5854 tests pass again. 3. [SILENT] static/index.html: the Share/StopSharing/ExportHTML button triplet was double-applied by the rebase (duplicate IDs → only the first wired). Deduped to one each + a count assertion test. 4. tests: fixed the stale Session.load-after-/api/session/new setup (master keeps new sessions memory-only until first message). Also (UX, found in the visual flow sweep): a clipboard-copy failure after a SUCCESSFUL share showed a red 'Share failed' toast and skipped opening the page. The copy is now isolated — the share succeeds, the page opens, and the toast shows the link if the clipboard write fails, never a false failure. Co-authored-by: MicoRobot. --- api/models.py | 2 +- api/shares.py | 68 +++++++++++++++++++++-- static/index.html | 3 - static/sessions.js | 17 ++++-- tests/test_session_public_share.py | 6 +- tests/test_session_public_share_static.py | 51 +++++++++++++++++ 6 files changed, 132 insertions(+), 15 deletions(-) diff --git a/api/models.py b/api/models.py index bc93f625..0c5de56f 100644 --- a/api/models.py +++ b/api/models.py @@ -1270,7 +1270,7 @@ class Session: 'parent_session_id', 'worktree_path', 'worktree_branch', 'worktree_repo_root', 'worktree_created_at', 'is_cli_session', 'source_tag', 'raw_source', 'session_source', 'source_label', 'read_only', - 'enabled_toolsets', 'composer_draft', 'anchor_activity_scenes', + 'enabled_toolsets', 'composer_draft', 'process_wakeup_pause', 'share_token', 'share_created_at', ] diff --git a/api/shares.py b/api/shares.py index 9c740657..b42a078b 100644 --- a/api/shares.py +++ b/api/shares.py @@ -19,6 +19,12 @@ from pathlib import Path from api.config import STATE_DIR from api.helpers import redact_session_data +# _redact_fn_cached is the ALWAYS-ON credential redactor (agent redactor with +# force=True + local fallback regex). Unlike redact_session_data it does NOT +# consult the user-toggleable api_redact_enabled setting — a public share is a +# hard safety boundary that must redact credentials even if the operator turned +# API-response redaction off. +from api.helpers import _redact_fn_cached as _force_redact_credentials logger = logging.getLogger(__name__) @@ -67,14 +73,38 @@ def _share_message_text(message: dict) -> str: parts = [] for item in content: if not isinstance(item, dict): + # Non-dict list items (e.g. nested structures) are NOT plain text — + # never stringify them into the public snapshot. continue if item.get("type") == "text": parts.append(str(item.get("text") or "")) return "".join(parts).strip() - return str(content or "").strip() + if isinstance(content, str): + return content.strip() + # A dict/other structured content (e.g. a tool-result object) is NOT shareable + # text — do NOT str() it (that would publish raw structured/tool payload). + return "" -def _sanitize_message(message: dict) -> dict | None: +def _redact_share_paths(text: str, extra_paths) -> str: + """Strip known local session/workspace/home paths out of public-share text. + + A workspace path or Hermes home can be embedded inside message prose (an + agent quoting a file path, a traceback, etc.). Redact the concrete local + paths so a public share never discloses the operator's filesystem layout. + """ + if not isinstance(text, str) or not text: + return text + for p in extra_paths: + if not p: + continue + p = str(p).strip() + if len(p) >= 4 and p in text: + text = text.replace(p, "[redacted-path]") + return text + + +def _sanitize_message(message: dict, *, redact_paths=()) -> dict | None: if not isinstance(message, dict): return None role = str(message.get("role") or "").strip().lower() @@ -83,6 +113,12 @@ def _sanitize_message(message: dict) -> dict | None: text = _share_message_text(message) if not text: return None + # ALWAYS-ON hardening for the public boundary, independent of any setting: + # (1) force credential redaction, (2) strip known local paths. + text = _force_redact_credentials(text) + text = _redact_share_paths(text, redact_paths) + if not text.strip(): + return None sanitized = { "role": role, "content": text, @@ -112,16 +148,38 @@ def _public_share_payload(payload: dict) -> dict: def build_share_snapshot(session) -> dict: - safe_session = redact_session_data(getattr(session, "__dict__", {}) or {}) + raw_dict = getattr(session, "__dict__", {}) or {} + # redact_session_data respects the api_redact_enabled setting; keep it as a + # first pass, but the per-message sanitizer below applies ALWAYS-ON credential + # + path redaction that does NOT depend on that setting (the public boundary + # must hold even if the operator disabled api_redact_enabled). + safe_session = redact_session_data(raw_dict) + # Concrete local paths to scrub from any message prose / title. + redact_paths = [] + for key in ("workspace", "worktree_path", "worktree_repo_root"): + val = raw_dict.get(key) + if val: + redact_paths.append(str(val)) + try: + from api.profiles import get_active_hermes_home + redact_paths.append(str(get_active_hermes_home())) + except Exception: + pass + try: + redact_paths.append(str(Path.home())) + except Exception: + pass safe_messages = [] for raw in safe_session.get("messages") or []: - sanitized = _sanitize_message(raw) + sanitized = _sanitize_message(raw, redact_paths=redact_paths) if sanitized: safe_messages.append(sanitized) if not safe_messages: raise ValueError("This conversation has no shareable messages yet.") + title = _force_redact_credentials(str(safe_session.get("title") or "Untitled")) + title = _redact_share_paths(title, redact_paths) or "Untitled" return { - "title": str(safe_session.get("title") or "Untitled"), + "title": title, "messages": safe_messages, "message_count": len(safe_messages), } diff --git a/static/index.html b/static/index.html index 41e0d242..9e80551e 100644 --- a/static/index.html +++ b/static/index.html @@ -943,9 +943,6 @@ - - - diff --git a/static/sessions.js b/static/sessions.js index 4395045b..340e25b5 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -4144,8 +4144,9 @@ async function _createOrRefreshSessionShare(session){ cancelLabel:t('share_session_refresh_snapshot'), }); if(reuse){ - await _copyTextToClipboard(existing); - showToast(t('share_session_link_copied')); + let copied=true; + try{ await _copyTextToClipboard(existing); }catch(_){ copied=false; } + showToast(copied?t('share_session_link_copied'):(t('share_session_status_active')+' — '+existing),copied?2500:6000); window.open(existing,'_blank','noopener'); return; } @@ -4153,8 +4154,16 @@ async function _createOrRefreshSessionShare(session){ const res=await api('/api/share/create',{method:'POST',body:JSON.stringify({session_id:session.session_id})}); if(res&&res.session) _syncSessionShareState(session,res.session); const href=new URL(String(res&&res.share&&res.share.url||''),location.origin).href; - await _copyTextToClipboard(href); - showToast(existing?t('share_session_link_copied'):t('share_session_created')); + // The share is now created server-side. A clipboard-copy failure (permissions, + // focus, non-secure context) must NOT be reported as "Share failed" — the link + // exists and we still open it. Only surface the copied-vs-not-copied distinction. + let copied=true; + try{ await _copyTextToClipboard(href); }catch(_){ copied=false; } + if(copied){ + showToast(existing?t('share_session_link_copied'):t('share_session_created')); + }else{ + showToast((existing?t('share_session_created'):t('share_session_created'))+' — '+href,6000); + } window.open(href,'_blank','noopener'); } diff --git a/tests/test_session_public_share.py b/tests/test_session_public_share.py index 0eb082ce..57d6db26 100644 --- a/tests/test_session_public_share.py +++ b/tests/test_session_public_share.py @@ -40,8 +40,10 @@ def _make_session_with_messages(): sid = created["session"]["session_id"] from api.models import Session - session = Session.load(sid) - assert session is not None + # Current master keeps a freshly /api/session/new session memory-only until + # its first message is persisted, so Session.load(sid) can return None here. + # Construct + persist the session directly so the share path has a real file. + session = Session.load(sid) or Session(session_id=sid) session.title = "Shared Test" session.messages = [ {"role": "system", "content": "internal system instructions should stay private"}, diff --git a/tests/test_session_public_share_static.py b/tests/test_session_public_share_static.py index 954d18e8..e8b82b10 100644 --- a/tests/test_session_public_share_static.py +++ b/tests/test_session_public_share_static.py @@ -74,3 +74,54 @@ def test_session_action_menu_exposes_public_share_actions(): def test_public_share_page_does_not_render_provider_details_blocks(): assert "provider-error-details" not in SHARE_JS assert "provider_details_label||'Provider details'" not in SHARE_JS + + +def test_share_action_buttons_are_not_duplicated(): + # Regression: a rebase double-applied the Share/StopSharing/ExportHTML button + # triplet, leaving duplicate IDs so getElementById wired only the first copy + # (three visible-but-inert buttons). Each control must appear exactly once. + for _id in ("btnShareSession", "btnStopSharingSession", "btnExportHTML"): + assert INDEX_HTML.count(f'id="{_id}"') == 1, f"{_id} must appear exactly once" + + +def test_share_snapshot_redaction_is_always_on_regardless_of_setting(): + # The public-share boundary must redact credentials + local paths and drop + # non-text/tool/system content EVEN IF the operator disabled api_redact_enabled. + import os, tempfile, importlib + os.environ.setdefault("HERMES_WEBUI_STATE_DIR", tempfile.mkdtemp()) + import api.config as config + import api.shares as shares + # Force the user-toggleable API redaction OFF (redact_session_data reads + # api.config.load_settings() at call time). + _orig = config.load_settings + config.load_settings = lambda: {"api_redact_enabled": False} + try: + class _S: + pass + s = _S() + s.title = "Deploy sk-ABCDEF1234567890TOKEN" + s.workspace = "/very/private/workspace" + s.messages = [ + {"role": "system", "content": "internal system prompt stays private"}, + {"role": "user", "content": "key sk-SECRET1234567890abcdef path /very/private/workspace/x.py"}, + {"role": "assistant", "content": {"tool": "terminal", "output": "raw structured tool payload"}}, + {"role": "tool", "content": "raw tool output"}, + {"role": "assistant", "content": "Here is the answer."}, + ] + snap = shares.build_share_snapshot(s) + import json + blob = json.dumps(snap) + # credentials masked (in both title and message text) + assert "sk-SECRET1234567890" not in blob + assert "sk-ABCDEF1234567890" not in blob + # local paths scrubbed + assert "/very/private/workspace" not in blob + # structured/dict content never stringified into the public payload + assert "raw structured tool payload" not in blob + # system + tool roles excluded + roles = [m["role"] for m in snap["messages"]] + assert "system" not in roles and "tool" not in roles + # the real answer is preserved + assert any("Here is the answer" in m["content"] for m in snap["messages"]) + finally: + config.load_settings = _orig