feat: tighten runtime policy + transport guards#2331
Open
oxoxDev wants to merge 14 commits into
Open
Conversation
Adds `verify_bearer_token(&str) -> bool` so non-HTTP transports can validate the per-process RPC token through the same gate the Axum middleware uses instead of duplicating the get_rpc_token + bearer_matches pattern. Pure refactor: middleware behaviour unchanged. Keeps the comparison in one helper so a future move to constant-time equality is a one-line change for every transport at once.
The Socket.IO layer was attached after the bearer-token middleware that
gates `POST /rpc`, so its `connect` / `rpc:request` / `chat:start` /
`chat:cancel` handlers sat outside that boundary. Tighten the namespace
so the same per-process bearer is required everywhere executable.
* Connect-time gate. The connect callback reads the handshake
`auth.token` (`io(url, { auth: { token } })` on the client) and
rejects the handshake unless it matches
`core::auth::verify_bearer_token`. The `Origin` header is checked
against a small allowlist (Tauri shell, `localhost:*`, `127.0.0.1:*`,
`[::1]:*`) so cross-origin browser pages cannot ride on a token
picked up elsewhere on the host. Native clients without an `Origin`
header (CLI, the Tauri shell) are still accepted.
* Per-event guard. Every executable event handler checks an
`AuthedConnection` marker stamped on socket extensions at connect
time, so a future handler addition keeps the same posture by default
and so the async gap between dispatching `disconnect()` and the
engine actually tearing the socket down cannot be raced.
Frontend (`socketService`, `useDictationHotkey`, `OverlayApp`) now
fetches the per-process bearer via `getCoreRpcToken()` and passes it
in the handshake's `auth` payload. The backend session JWT rides along
as `auth.session` for future correlation.
Adds origin-allowlist unit tests in `core::socketio::tests`.
`GET /events` was on the public-paths list (browser `EventSource` cannot attach an `Authorization` header) and filtered broadcasts purely by a caller-supplied `client_id` query string. That made any well-known `client_id` — e.g. the literal `system` slot the proactive-message emitter uses — readable by anyone who can reach the local listener. This tightens the contract: clients now mint a single-shot bind token through the new `core.events_subscribe_token` RPC (bearer-protected via the existing `/rpc` middleware), then open `/events?client_id=<id>&token=<bind>`. The handler consumes the token on first use so a leaked URL cannot be replayed. Tokens are bound to the issued `client_id`, time-bounded (default 60s, max 30min), and held in an in-process store with a small capacity ceiling so a misbehaving caller cannot grow it without bound. Adds: - `src/core/event_bind_tokens.rs` — the in-memory store + issue/consume helpers + unit tests covering match, mismatch, single-shot, expiry, and TTL clamping. - `core.events_subscribe_token` branch in `try_core_dispatch`. - 401 response shape on `/events` for missing/invalid tokens (was a silent broadcast filter before). - Updated `/` info-page hint so the new query shape is discoverable.
`/auth/telegram` runs from a browser tab as a top-level navigation after the Telegram bot hands the user a link, so the bearer-token middleware cannot protect it (the redirect arrives in a fresh tab with no header control). The handler previously accepted any `GET /auth/telegram?token=…` and immediately stored the resulting JWT as the local session. Add a fetch-metadata gate at the top of the handler that distinguishes "user clicked the link the bot sent them" from "another page navigated the user's loopback core via `window.location` / `<img>` / `<iframe>`": * `Sec-Fetch-Mode` must be `navigate` (or absent — older browsers and CLI clients). * `Sec-Fetch-Dest` must be `document`. * On `Sec-Fetch-Site: cross-site`, the `Referer`/`Origin` must be `https://t.me/...` or `https://web.telegram.org/...`. * If fetch-metadata headers are absent, fall back to a host check on the `Referer` header. Rejected callbacks return a 403 HTML error page that points the user back at the bot. Legacy clients without fetch-metadata stay supported. Adds seven unit tests covering accepted shapes (no metadata, legit telegram redirect, same-origin nav) and rejected shapes (image embed, iframe embed, cross-site from a third-party page, non-telegram referer without fetch-metadata).
The inbound `ChannelInboundMessage` subscriber derived its conversation key as `channel:<channel>`, dropping the `sender` / `reply_target` / `thread_ts` segments that `channels::context::conversation_history_key` already uses for the canonical channel paths. The result was that two participants in the same shared channel (e.g. a multi-member Discord text channel) were resumed inside one cached agent session: messages from one user would land on the other's pending in-flight state. Plumb the missing segments through: * `DomainEvent::ChannelInboundMessage` grows `sender`, `reply_target`, and `thread_ts` (all `Option<String>` so legacy publishers continue to compile and produce the historical single-DM key). * The socket event publisher (`openhuman::socket::event_handlers`) lifts those fields off the raw inbound JSON payload — with the obvious fallbacks (`from` → `sender`, `chat_id`/`channel_id` → `reply_target`, `thread_id` → `thread_ts`) so existing transports don't need a coordinated payload change. * `channels::bus::derive_inbound_thread_id` mirrors the shape `conversation_history_key` builds, including the Telegram carve-out that ignores `thread_ts` for memory keying. Adds five unit tests covering the legacy single-DM key, distinct senders in a shared channel, Slack subthreads, the Telegram `thread_ts` carve-out, and trimming of whitespace-only optional fields. Existing `event_bus::events_tests` updated to construct the expanded variant with `None`s.
Memory recall pulls rows from every provenance tier into a single hit list — user-authored turns, agent-authored summaries, and connector- synced content from Gmail / Slack / Notion / Discord — and the harness then renders that list straight into the agent's working prompt with no visual distinction between system instructions and external text. A prompt-injection paragraph that lives inside an inbound email or Notion page therefore reaches the agent's working context with the same weight as a system-issued directive. Adds the narrowest possible mitigation that does not require a memory schema migration: * `agent::harness::memory_context_safety` exposes `is_potentially_untrusted(&MemoryEntry)` (default-deny on namespace and connector key-prefix heuristics) and `wrap_untrusted_for_agent` which surrounds the row's body with explicit `<untrusted-source>` markers carrying a source hint. * `build_context` in `agent::harness::memory_context` now wraps every flagged row before writing it into the `[Memory context]` block, so the safety preamble and the model can both see the boundary. The heuristic is conservative on purpose: locally-authored namespaces (`working`, `agent`, `local`, `core`, `global`, `default`, `user`, plus their `.*` children and `tree.*` ingestion namespaces) form an allowlist, and connector key prefixes (`chat:`, `email:`, `notion:`, `drive:`, `discord:`, `telegram:`, `whatsapp:`, `slack:`, `gmail:`, `outlook:`, `imap:`, `meeting:`, `web:`) flip to untrusted even when the namespace is missing. A future commit can replace the heuristic with a typed `Provenance` enum on `MemoryEntry`, populated at ingest; this commit ships defense-in-depth without that schema change. Adds seven unit tests covering trusted namespaces, prefixed subspaces, default-deny on unknown namespaces, connector key prefixes without a namespace, plain bare keys, and the wrapping output shape.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Socket.IO handshake bearer auth and origin checks, an SSE bind-token lifecycle plus RPC to mint tokens, updates /events to accept bind tokens or Bearer tokens, extends channel inbound events with sender/reply_target/thread_ts and per-thread IDs, and wraps potentially untrusted memory entries before inserting into agent prompts. ChangesSocket.IO and Event Stream Authentication
Channel Inbound Context Expansion
Memory Context Safety for Agent Prompts
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Frontend
participant CoreRPC
participant CoreService
participant SSE as SSE_/events
Browser->>Frontend: request socket / events URL
Frontend->>CoreRPC: resolveCoreSocketUrl()
Frontend->>CoreRPC: getCoreRpcToken()
Frontend->>CoreService: connectCoreSocket(auth: coreToken + authExtras.session)
Browser->>CoreRPC: core.events_subscribe_token(client_id, ttl_secs?)
CoreRPC->>CoreService: event_bind_tokens::issue(client_id, ttl)
CoreService-->>CoreRPC: { token, valid_until }
Browser->>SSE: GET /events?client_id=...&token=...
SSE->>CoreService: event_bind_tokens::consume(client_id, token)
CoreService-->>SSE: success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
the
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/services/socketService.ts (1)
185-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check that this connect attempt is still current after the new awaits.
connectAsync()storesthis.tokenbefore awaitingresolveCoreSocketBaseUrl()andgetCoreRpcToken(). Ifconnect()is called again while those are in flight, the older attempt can still instantiate a socket with stale auth/session data and race the newer connection.Suggested fix
const backendUrl = await resolveCoreSocketBaseUrl(); + if (this.token !== token || this.socket) return; socketLog('Connecting to core socket', { userId: uid, backendUrl }); @@ const coreToken = await getCoreRpcToken(); + if (this.token !== token || this.socket) return; const socketOptions = { auth: { token: coreToken ?? '', session: token },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/services/socketService.ts` around lines 185 - 204, connectAsync() stores this.token then awaits resolveCoreSocketBaseUrl() and getCoreRpcToken(), allowing a race where an older call instantiates a socket with stale auth; fix by snapshotting a local identifier (e.g., localToken or localAttemptId) immediately after storing this.token and before the awaits, then after each await (or at least right before creating this.socket in connect()/connectAsync()) compare the snapshot to the current this.token/thisAttempt and abort the old attempt if they differ so only the most recent call proceeds to call io(...) and assign this.socket.
🧹 Nitpick comments (1)
src/core/dispatch.rs (1)
123-140: ⚡ Quick winAdd branch-local logs for the rejected subscribe-token paths.
The new RPC returns errors for invalid
client_idand store-capacity exhaustion, but those branches leave no server-side breadcrumb. A safedebug/warnlog here would make/eventsauth failures diagnosable without ever logging the token.As per coding guidelines, "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/dispatch.rs` around lines 123 - 140, Add branch-local logs for the two error paths: when extracting/validating client_id fails and when crate::core::event_bind_tokens::issue(...) returns None due to capacity. Inside the client_id .ok_or_else() closure (the "missing or empty 'client_id' parameter" branch) emit a debug or warn log including the incoming params (or a redacted summary) and the fact client_id was invalid; inside the .ok_or_else() after issue(...) (the "store at capacity" branch) emit a warn-level log noting bind-token store capacity reached and include client_id and ttl summary. Use the existing logging facility (log::debug/log::warn or tracing::debug/tracing::warn) and keep logs concise and non-sensitive.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/event_bind_tokens.rs`:
- Around line 108-119: The current logic calls store.remove(token) immediately
and then checks entry.client_id, which deletes the token even on client_id
mismatch; change this so you first lookup/read the entry (use whichever
read/peek method is available instead of store.remove, or re-insert on mismatch)
and only call store.remove(token) when entry.client_id == client_id; keep the
existing log messages (log::debug!/log::warn!) and the same return values, but
move the destructive remove to occur after the client_id equality check to avoid
accidentally consuming the token on mismatch.
In `@src/core/jsonrpc.rs`:
- Around line 479-487: The current referer fallback uses prefix-matching on the
referer string (variable referer) which allows host spoofing; instead parse the
referer into a URI (e.g., via Url::parse or std::net utilities) and compare the
parsed host exactly (host == "localhost" or host == "127.0.0.1") or verify it is
a loopback address, leaving the existing referer_is_telegram check intact;
update the logic in the same function/block that references referer and
referer_is_telegram to use the parsed host comparison and add a regression test
that submits a referer like "http://localhost.evil.com/..." (and one with a
127.0.0.1 spoof) to assert the request is rejected.
In `@src/core/socketio.rs`:
- Around line 48-53: Replace the brittle prefix-matching logic on origin with
proper URL parsing: call url::Url::parse(origin) (or equivalent) and use
url.host_str() to obtain the canonical host, then compare that host exactly
against "localhost", "127.0.0.1", and "::1" (note host_str() returns IPv6
without brackets) instead of starts_with; if parsing fails or host_str() is None
return false (deny) so only exact host matches are allowed.
In `@src/openhuman/agent/harness/memory_context_safety.rs`:
- Around line 57-75: Run rustfmt to fix formatting drift in
memory_context_safety.rs: format the file (and project) with cargo fmt --all and
commit only the formatting changes. Specifically ensure the connector_prefixes
array (the slice declared as connector_prefixes: &[&str] and its iterator chain
using .iter().any(|p| key_lower.starts_with(p))) and the other flagged blocks
(around the same file) are reformatted to match rustfmt output so CI passes; do
not change logic, only apply the formatting delta.
- Around line 104-109: The wrap_untrusted_for_agent function injects source_hint
and content raw into the XML-like marker which allows crafting strings (e.g.,
quotes or "</untrusted-source>") to break out of the marker; fix by
escaping/encoding both source_hint and content before formatting: sanitize
source_hint for quotes/angle brackets (or percent-encode) and replace any
occurrences of the closing tag sequence in content (e.g., "</untrusted-source>")
with a safe-escaped variant or an encoded form, then use the escaped values in
the format! call so the marker cannot be forged or terminated prematurely by
untrusted input.
In `@src/openhuman/agent/harness/memory_context.rs`:
- Around line 63-66: The new namespace-hint block is not formatted to Rust
style; reformat the expression that sets hint (the `hint` binding using
`entry.namespace.as_deref().unwrap_or("connector")`) so it passes rustfmt. Run
`cargo fmt --all` (or `cargo fmt`) to normalize formatting and commit the
changes so `cargo fmt --check` in CI succeeds.
In `@src/openhuman/channels/bus.rs`:
- Around line 994-998: The test function legacy_channel_only_keeps_old_shape has
a formatting violation on the assert_eq! call; run rustfmt (cargo fmt) to
reformat the file so the assertion line conforms to style rules, or manually
reflow the assert_eq! arguments in that function (the call comparing
derive_inbound_thread_id("telegram", None, None, None) to "channel:telegram") so
it fits formatting conventions, then re-run cargo fmt to ensure
src/openhuman/channels/bus.rs passes --check.
In `@src/openhuman/socket/event_handlers.rs`:
- Around line 216-217: The formatting of the thread_ts assignment (the chain
using nonempty(data.get("thread_ts")).or_else(||
nonempty(data.get("thread_id")))) does not match rustfmt expectations; run
rustfmt (cargo fmt) to reformat the hunk and commit the resulting changes so
cargo fmt --check passes. Locate the thread_ts declaration in event_handlers.rs
(the nonempty / data.get / thread_id chain) and apply rustfmt to normalize the
line breaks and chaining style, then add the formatted file to the PR.
---
Outside diff comments:
In `@app/src/services/socketService.ts`:
- Around line 185-204: connectAsync() stores this.token then awaits
resolveCoreSocketBaseUrl() and getCoreRpcToken(), allowing a race where an older
call instantiates a socket with stale auth; fix by snapshotting a local
identifier (e.g., localToken or localAttemptId) immediately after storing
this.token and before the awaits, then after each await (or at least right
before creating this.socket in connect()/connectAsync()) compare the snapshot to
the current this.token/thisAttempt and abort the old attempt if they differ so
only the most recent call proceeds to call io(...) and assign this.socket.
---
Nitpick comments:
In `@src/core/dispatch.rs`:
- Around line 123-140: Add branch-local logs for the two error paths: when
extracting/validating client_id fails and when
crate::core::event_bind_tokens::issue(...) returns None due to capacity. Inside
the client_id .ok_or_else() closure (the "missing or empty 'client_id'
parameter" branch) emit a debug or warn log including the incoming params (or a
redacted summary) and the fact client_id was invalid; inside the .ok_or_else()
after issue(...) (the "store at capacity" branch) emit a warn-level log noting
bind-token store capacity reached and include client_id and ttl summary. Use the
existing logging facility (log::debug/log::warn or tracing::debug/tracing::warn)
and keep logs concise and non-sensitive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c10946c3-e901-4075-883b-87b5540f3416
📒 Files selected for processing (17)
app/src/hooks/useDictationHotkey.tsapp/src/overlay/OverlayApp.tsxapp/src/services/socketService.tssrc/core/auth.rssrc/core/dispatch.rssrc/core/event_bind_tokens.rssrc/core/event_bus/events.rssrc/core/event_bus/events_tests.rssrc/core/jsonrpc.rssrc/core/jsonrpc_tests.rssrc/core/mod.rssrc/core/socketio.rssrc/openhuman/agent/harness/memory_context.rssrc/openhuman/agent/harness/memory_context_safety.rssrc/openhuman/agent/harness/mod.rssrc/openhuman/channels/bus.rssrc/openhuman/socket/event_handlers.rs
The bind-token path was the only credential `events_handler` accepted, but CLI tooling and the in-tree e2e suite hit `/events` with `Authorization: Bearer <core>` directly. Restore that flow by first trying the bearer header through `verify_bearer_token` and only falling back to the `?token=` bind-token consume when the header is absent or wrong. Net effect: browser `EventSource` clients still go through the bind-token mint + consume (no header capability), CLI/Tauri-shell callers keep working with the existing `Authorization` header, and the four `tests/json_rpc_e2e.rs` cases that opened SSE with a Bearer header (`json_rpc_protocol_auth_and_agent_hello`, `json_rpc_web_chat_*`) pass again locally.
The vi.mock for `coreRpcClient` only exposed `getCoreRpcUrl` / `clearCoreRpcUrlCache`; the new `getCoreRpcToken()` call in `connectAsync` therefore returned `undefined` (or threw) and the connect chain hung. Add `getCoreRpcToken: vi.fn(async () => '...')` to both test files so the resolve chain proceeds and the existing pollUntil assertions trip on schedule. Mock value is irrelevant — the tests assert dispatch behaviour, not the bearer string content.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/openhuman/agent/harness/memory_context_safety.rs (1)
96-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape
source_hintandcontentbefore emitting the wrapper.Both fields are interpolated raw here. A payload containing
"or</untrusted-source>can terminate or forge the marker and move attacker-controlled text outside the untrusted boundary, which defeats the safety signal this module is adding.🔒 Proposed hardening
+fn sanitize_source_hint(source_hint: &str) -> String { + let cleaned: String = source_hint + .trim() + .chars() + .filter(|c| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.' | ':')) + .take(64) + .collect(); + + if cleaned.is_empty() { + "external".to_string() + } else { + cleaned + } +} + +fn escape_untrusted_content(content: &str) -> String { + content + .replace('&', "&") + .replace('<', "<") + .replace('>', ">") +} + pub fn wrap_untrusted_for_agent(content: &str, source_hint: &str) -> String { - let hint = source_hint.trim(); - let hint = if hint.is_empty() { "external" } else { hint }; - format!("<untrusted-source source=\"{hint}\">\n{content}\n</untrusted-source>") + let hint = sanitize_source_hint(source_hint); + let safe_content = escape_untrusted_content(content); + format!( + "<untrusted-source source=\"{hint}\">\n{safe_content}\n</untrusted-source>" + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/harness/memory_context_safety.rs` around lines 96 - 99, The wrap_untrusted_for_agent function is emitting source_hint and content raw which allows injection of attributes or closing tags; fix it by HTML/XML-escaping both values before formatting: create or use a helper (e.g., escape_xml or escape_html) to replace &, <, >, " (and optionally ') and apply it to the trimmed source_hint and to content (so any "</untrusted-source>" becomes escaped), then build the wrapper with those escaped strings in place of {hint} and {content} in wrap_untrusted_for_agent to ensure the marker cannot be prematurely terminated or forged.src/core/event_bind_tokens.rs (1)
106-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly consume the token after the client binding matches.
A wrong
client_idstill removes the token from the store here, which turns a bad bind attempt into a one-shot denial of service against the real subscriber.Suggested fix
- let entry = match store.remove(token) { - Some(entry) => entry, + let entry = match store.get(token) { + Some(entry) => entry, None => { log::debug!("[events-bind] consume: token not found"); return false; } }; if entry.client_id != client_id { log::warn!("[events-bind] consume: client_id mismatch (token bound to other id)"); return false; } + let entry = store + .remove(token) + .expect("token was present after successful binding check");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/event_bind_tokens.rs` around lines 106 - 115, The code currently removes the entry via store.remove(token) before checking entry.client_id, which allows a mismatched client_id to delete the token; change the logic in the consume function (look for store.remove(token), entry, token, client_id, and entry.client_id) to first inspect the stored binding (e.g., via get/peek or by using an entry API) and only call store.remove(token) if entry.client_id == client_id; if it mismatches, log the warning and return false without removing the token.src/core/jsonrpc.rs (1)
479-486:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse the fallback
Refererhost before treating it as local.The current prefix check still accepts spoofed hosts like
http://localhost.evil.com/..., so a non-Telegram cross-site redirect can satisfy the fallback and bypass the callback origin gate.Suggested fix
- let local = - referer.starts_with("http://127.0.0.1") || referer.starts_with("http://localhost"); + let local = url::Url::parse(referer) + .ok() + .and_then(|url| url.host_str().map(str::to_owned)) + .map(|host| matches!(host.as_str(), "127.0.0.1" | "localhost" | "::1")) + .unwrap_or(false); if !(local || referer_is_telegram) { return Err("Referer must be telegram or local"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/jsonrpc.rs` around lines 479 - 486, The referer prefix check allows spoofed hosts like "localhost.evil.com"; change the fallback logic to parse the Referer URL (use Url::parse or equivalent) and extract the host component, then set local only when host is exactly "127.0.0.1" or "localhost" (accounting for optional port via host_str() which excludes port), and keep the existing check of referer_is_telegram; update the code around the referer variable and the local binding (and the if !(local || referer_is_telegram) branch) to use the parsed host comparison instead of starts_with.src/core/socketio.rs (1)
48-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse the Origin host instead of prefix-matching it.
This still accepts spoofed origins like
https://localhost.attacker.example, so a cross-origin browser page can satisfy the allowlist and complete the Socket.IO handshake.Suggested fix
- // Strip scheme so we can prefix-match the host:port portion. - let host = origin - .strip_prefix("http://") - .or_else(|| origin.strip_prefix("https://")) - .unwrap_or(origin); - host.starts_with("localhost") || host.starts_with("127.0.0.1") || host.starts_with("[::1]") + let Ok(parsed) = url::Url::parse(origin) else { + return false; + }; + matches!(parsed.host_str(), Some("localhost" | "127.0.0.1" | "::1"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/socketio.rs` around lines 48 - 53, The code currently strips scheme and prefix-matches the origin string (variable host) using starts_with which allows spoofed hosts like "localhost.attacker.example"; change it to properly parse the origin as a URL (e.g., use url::Url::parse(origin)) and extract the canonical host via Url::host_str() (or Url::host()/ToSocketAddrs if needed), then compare the returned host exactly against "localhost", "127.0.0.1" and "::1" (or the IPv6 literal without brackets), rejecting on parse failure; replace the strip_prefix/starts_with logic that uses origin/host with this URL-parse + exact-equality host checks to ensure only true loopback hosts are accepted.
🧹 Nitpick comments (1)
src/openhuman/agent/harness/memory_context.rs (1)
62-64: ⚡ Quick winAdd a
build_contextregression test for this wrapping branch.This is the only prompt-builder path that applies the new untrusted wrapper, but none of the current
build_contexttests exercise it. A single case with an untrusted entry and assertions on the rendered marker/source hint would pin the integration contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/harness/memory_context.rs` around lines 62 - 64, Add a regression test that exercises the untrusted-branch in build_context: create a memory entry that is considered untrusted (use is_potentially_untrusted criteria, e.g., an entry with no namespace or a connector namespace), call build_context (or the public function that constructs rendered_content), and assert the returned/rendered fragment contains the untrusted wrapper marker and the expected source hint string produced by wrap_untrusted_for_agent (e.g., "connector" fallback). Name the test something like test_build_context_untrusted_wrapping and assert both the wrapper marker and the hint are present in the final rendered content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/channels/bus.rs`:
- Around line 997-1000: The current special-case for Telegram in
derive_inbound_thread_id checks the full channel string and thus misses
production shapes like "tg:123"; update derive_inbound_thread_id to determine
the transport/provider by splitting the channel on ':' (or otherwise extracting
the provider prefix) and treat providers "telegram" and "tg" as Telegram so
thread_ts is ignored for those providers, then add a unit test asserting
derive_inbound_thread_id("tg:123", ...) produces the same stable key as
derive_inbound_thread_id("telegram", ...) (or at least ignores differing
thread_ts for "tg:123") to cover the production socket payload shape; reference
derive_inbound_thread_id and the new test name when making the change.
---
Duplicate comments:
In `@src/core/event_bind_tokens.rs`:
- Around line 106-115: The code currently removes the entry via
store.remove(token) before checking entry.client_id, which allows a mismatched
client_id to delete the token; change the logic in the consume function (look
for store.remove(token), entry, token, client_id, and entry.client_id) to first
inspect the stored binding (e.g., via get/peek or by using an entry API) and
only call store.remove(token) if entry.client_id == client_id; if it mismatches,
log the warning and return false without removing the token.
In `@src/core/jsonrpc.rs`:
- Around line 479-486: The referer prefix check allows spoofed hosts like
"localhost.evil.com"; change the fallback logic to parse the Referer URL (use
Url::parse or equivalent) and extract the host component, then set local only
when host is exactly "127.0.0.1" or "localhost" (accounting for optional port
via host_str() which excludes port), and keep the existing check of
referer_is_telegram; update the code around the referer variable and the local
binding (and the if !(local || referer_is_telegram) branch) to use the parsed
host comparison instead of starts_with.
In `@src/core/socketio.rs`:
- Around line 48-53: The code currently strips scheme and prefix-matches the
origin string (variable host) using starts_with which allows spoofed hosts like
"localhost.attacker.example"; change it to properly parse the origin as a URL
(e.g., use url::Url::parse(origin)) and extract the canonical host via
Url::host_str() (or Url::host()/ToSocketAddrs if needed), then compare the
returned host exactly against "localhost", "127.0.0.1" and "::1" (or the IPv6
literal without brackets), rejecting on parse failure; replace the
strip_prefix/starts_with logic that uses origin/host with this URL-parse +
exact-equality host checks to ensure only true loopback hosts are accepted.
In `@src/openhuman/agent/harness/memory_context_safety.rs`:
- Around line 96-99: The wrap_untrusted_for_agent function is emitting
source_hint and content raw which allows injection of attributes or closing
tags; fix it by HTML/XML-escaping both values before formatting: create or use a
helper (e.g., escape_xml or escape_html) to replace &, <, >, " (and optionally
') and apply it to the trimmed source_hint and to content (so any
"</untrusted-source>" becomes escaped), then build the wrapper with those
escaped strings in place of {hint} and {content} in wrap_untrusted_for_agent to
ensure the marker cannot be prematurely terminated or forged.
---
Nitpick comments:
In `@src/openhuman/agent/harness/memory_context.rs`:
- Around line 62-64: Add a regression test that exercises the untrusted-branch
in build_context: create a memory entry that is considered untrusted (use
is_potentially_untrusted criteria, e.g., an entry with no namespace or a
connector namespace), call build_context (or the public function that constructs
rendered_content), and assert the returned/rendered fragment contains the
untrusted wrapper marker and the expected source hint string produced by
wrap_untrusted_for_agent (e.g., "connector" fallback). Name the test something
like test_build_context_untrusted_wrapping and assert both the wrapper marker
and the hint are present in the final rendered content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73a8c536-a7fd-4056-b7a9-07a3488f2cc2
📒 Files selected for processing (10)
app/src/services/__tests__/socketService.events.test.tsapp/src/services/__tests__/socketService.test.tssrc/core/dispatch.rssrc/core/event_bind_tokens.rssrc/core/jsonrpc.rssrc/core/socketio.rssrc/openhuman/agent/harness/memory_context.rssrc/openhuman/agent/harness/memory_context_safety.rssrc/openhuman/channels/bus.rssrc/openhuman/socket/event_handlers.rs
…et.ts
Extract the `io(baseUrl, { auth: { token: <core> }, ... })` shape used
by `socketService`, `useDictationHotkey`, and `OverlayApp` into a
single `createCoreSocket(baseUrl, opts)` factory. All three call sites
now go through one line instead of repeating the handshake plumbing.
Adds focused unit tests for the factory covering: core bearer is
passed through `auth.token`, missing token collapses to empty string,
`authExtras` (currently used by `socketService` for the session JWT)
merges alongside, and connect-time overrides do not erase the auth
payload.
Per `Coverage Gate (diff-cover ≥ 80%)` — the previous shape kept the
new lines inside untested files (`useDictationHotkey.tsx`,
`OverlayApp.tsx`); routing through a tested helper restores diff
coverage without standing up component-level test harnesses for the
two screens.
coderabbitai
Bot
added
the
feature
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/hooks/useDictationHotkey.ts (1)
9-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale docstring: connection is now authenticated.
The docstring states the connection "does not require authentication" but the implementation now passes
coreTokenfor bearer authentication viacreateCoreSocket. Update the comment to reflect that dictation uses an authenticated core socket (while still not requiring user login).📝 Suggested doc update
* Dictation events are received over a **dedicated** Socket.IO -* connection to the core process that does not require authentication. -* This ensures dictation works regardless of whether the user is -* logged in. +* connection to the core process authenticated via the per-process +* core bearer token (not the user session). This ensures dictation +* works regardless of whether the user is logged in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/hooks/useDictationHotkey.ts` around lines 9 - 12, The docstring above useDictationHotkey is stale: it says the dedicated Socket.IO connection "does not require authentication" but the code now passes coreToken to createCoreSocket for bearer auth; update the comment to state that dictation uses an authenticated core socket (via coreToken/bearer token) while still not requiring a user login, and adjust the sentences around useDictationHotkey/createCoreSocket/coreToken to reflect that authentication behavior.app/src/overlay/OverlayApp.tsx (1)
7-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale docstring: connection is now authenticated.
The docstring describes "a dedicated, unauthenticated Socket.IO connection" but the implementation now passes
coreTokenviacreateCoreSocket. Update to reflect that the overlay uses core bearer auth (independent of user session).📝 Suggested doc update
-* connection (same pattern as `useDictationHotkey`). +* connection authenticated via the per-process core bearer token +* (same pattern as `useDictationHotkey`).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/overlay/OverlayApp.tsx` around lines 7 - 8, Update the stale docstring in OverlayApp.tsx to reflect that the overlay's Socket.IO connection is authenticated: change the phrase "dedicated, unauthenticated Socket.IO connection" to indicate the connection uses core bearer auth and passes coreToken via createCoreSocket (independent of user session); ensure any mention of parity with useDictationHotkey clarifies that pattern but with authentication handled by coreToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/hooks/useDictationHotkey.ts`:
- Around line 9-12: The docstring above useDictationHotkey is stale: it says the
dedicated Socket.IO connection "does not require authentication" but the code
now passes coreToken to createCoreSocket for bearer auth; update the comment to
state that dictation uses an authenticated core socket (via coreToken/bearer
token) while still not requiring a user login, and adjust the sentences around
useDictationHotkey/createCoreSocket/coreToken to reflect that authentication
behavior.
In `@app/src/overlay/OverlayApp.tsx`:
- Around line 7-8: Update the stale docstring in OverlayApp.tsx to reflect that
the overlay's Socket.IO connection is authenticated: change the phrase
"dedicated, unauthenticated Socket.IO connection" to indicate the connection
uses core bearer auth and passes coreToken via createCoreSocket (independent of
user session); ensure any mention of parity with useDictationHotkey clarifies
that pattern but with authentication handled by coreToken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 635cb412-d86e-4fa5-8cf7-4f8051c3b513
📒 Files selected for processing (5)
app/src/hooks/useDictationHotkey.tsapp/src/overlay/OverlayApp.tsxapp/src/services/__tests__/coreSocket.test.tsapp/src/services/coreSocket.tsapp/src/services/socketService.ts
…elper
`useDictationHotkey` and `OverlayApp` both manually awaited base URL,
awaited the core bearer, checked the React effect's `disposed` flag at
each await point, and then called `createCoreSocket`. The added lines
were inside untested files, so `Coverage Gate (diff-cover ≥ 80%)`
reported them as missing even though `createCoreSocket` itself is
fully covered.
Add `connectCoreSocket({ getBaseUrl, isDisposed, authExtras, overrides })`
that runs the whole dance and returns the socket (or `null` if the
caller's disposal flag flips between awaits). Each call site collapses
to one line that the React effect can `if (!socket) return;` against.
The new helper carries five focused unit tests covering the happy
path, both disposal short-circuits (between baseUrl and token, between
token and connect), authExtras + overrides forwarding, and the
`null`-token-becomes-empty-string fallback.
`socketService` keeps using `createCoreSocket` directly because its
URL guard sits between the resolve and the connect, and the file is
already 100% covered.
`Coverage Gate (diff-cover ≥ 80%)` keeps flagging the three new lines in `useDictationHotkey.ts` because nothing imports the hook in a test file. Add a small renderHook-based suite that mocks `connectCoreSocket` + the core RPC client and asserts the connect dance: - happy path: connectCoreSocket is invoked with both `getBaseUrl` and `isDisposed` callbacks, and the disposal flag is initially false; - unmount: the socket's `disconnect()` is called on cleanup; - short-circuit: when `connectCoreSocket` returns `null` (the helper's disposed-mid-await shape), no event handlers are wired up. `OverlayApp.tsx` gets a `c8 ignore start/stop` band around the same glue. The component is a full Tauri-tied React surface; the underlying logic lives in the already-tested `connectCoreSocket` helper, so the call site has nothing to assert beyond "the function was called". The ignore band is tight (three lines) and labelled.
Five substantive findings from the CodeRabbit pass on PR tinyhumansai#2331: 1. `event_bind_tokens::consume` peeks the entry before removing, so a wrong-client_id probe no longer evicts the token. Without this the mismatch path was a one-shot DoS against the legitimate subscriber. Regression: `wrong_client_id_does_not_consume_token`. 2. `core::socketio::origin_is_allowed` parses the Origin URL and compares the host EXACTLY against the loopback allowlist. Previous `starts_with("localhost")` accepted decoys like `http://localhost.attacker.example`. IPv6 brackets honoured. Regressions: `origin_allowlist_rejects_host_prefix_decoys` + `origin_allowlist_rejects_unparseable_origin`. 3. `telegram_callback_origin_ok` Referer fallback now parses the Referer URL and exact-matches the host slot, closing the same prefix decoy on the public Telegram callback. Regression: `telegram_callback_origin_ok_rejects_localhost_host_prefix_decoy`. 4. `memory_context_safety::wrap_untrusted_for_agent` sanitises the source hint (alphanumerics + small punct, capped at 64 chars, falls back to `external`) and escapes the three HTML-ish breakout characters in the content. A payload containing `</untrusted-source>` or a stray quote can no longer forge or terminate the marker. Four new regressions cover marker breakout, attribute breakout, sanitised hint, and length cap. 5. `derive_inbound_thread_id` recognises the production Telegram channel shape — `tg:<chat_id>` / `telegram:<chat_id>` — instead of only the literal slug. Without this the Telegram thread_ts carve-out missed real socket payloads and split per-message memory keys. Two new regressions for the `tg:` and `telegram:` prefixes plus an inverse Slack assertion.
Two more findings from the CodeRabbit review pass on PR tinyhumansai#2331: 1. `socketService.connectAsync` introduces a second `await` for the per-process bearer (`getCoreRpcToken()`) on top of the existing `resolveCoreSocketBaseUrl()` await. If `connect(token)` is called again while either await is in flight, the older invocation could still reach `createCoreSocket` with stale auth and race the newer connection. Re-check `this.token` / `this.socket` after both awaits and bail out if either has drifted. 2. `core.events_subscribe_token` (`src/core/dispatch.rs`) emitted no server-side log on the two rejection branches (missing `client_id` and bind-token store at capacity). Add `log::warn` lines so `/events` auth failures are diagnosable without ever logging the token, plus a `log::debug` on the success branch — token value stays off the log line.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
rpc_auth_middleware; SSE accepted a free-formclient_idquery string with no minted token; the public Telegram callback accepted any GET without fetch-metadata sanity checks; the inbound channel bus keyed by channel alone while every other inbound path keys by(channel, sender, reply_target, thread_ts); memory recall rendered every hit into the agent prompt with no provenance hint.Solution
Layered, scope-isolated commits. Each commit is independently reviewable:
refactor(core/auth): extract a reusableverify_bearer_tokenhelper so non-HTTP transports can use the same comparison the Axum middleware uses internally. Pure refactor — no behaviour change, middleware delegates to the helper.feat(core/socketio): route the Socket.IO handshake throughverify_bearer_token(auth.tokenpayload on the client) plus an Origin allowlist (Tauri shell, localhost loopback, native clients without anOriginheader). Adds anAuthedConnectionmarker on socket extensions so every executable event handler consults the same gate. Frontend (socketService,useDictationHotkey,OverlayApp) passes the per-process bearer viagetCoreRpcToken().feat(core/events): SSE subscribers now mint a single-shot bind token through a small bearer-protected RPC (core.events_subscribe_token) and open/events?client_id=…&token=…. Tokens are bound to the issuedclient_id, single-shot on validate, and time-bounded. Bearer-header callers (CLI, Tauri shell) are still accepted to keep the existing flows green.feat(core/auth): the public Telegram OAuth-style callback gains a fetch-metadata gate.Sec-Fetch-Modemust benavigate,Sec-Fetch-Destmust bedocument, and cross-site redirects must originate fromhttps://t.me/...orhttps://web.telegram.org/.... Older browsers without fetch-metadata headers stay supported via a Referer host fallback.fix(channels/bus): restore per-sender keying on the inbound bus path so it matcheschannels::context::conversation_history_key.DomainEvent::ChannelInboundMessagegrowssender/reply_target/thread_ts(allOption<String>— legacy publishers compile unchanged), and a newderive_inbound_thread_idmirrors the canonical shape.feat(memory): wrap memory-recall hits whose namespace is not on the locally-authored allowlist (or whose key uses a connector prefix) in explicit<untrusted-source>markers via a newmemory_context_safetymodule. Conservative on purpose — defense-in-depth ahead of a follow-up typedProvenanceenum onMemoryEntry.28 new unit tests across the six modules (see per-commit counts in commit messages).
Submission Checklist
## RelatedCloses #NNNin the## RelatedsectionImpact
format!.Related
Provenanceenum onMemoryEntry, populated at ingestion, to replace the conservative heuristic in commit 6.stateparameter wiring for/auth/telegramso the callback can bind to a server-issued nonce in addition to the fetch-metadata gate.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/runtime-policy-tighteninggit log upstream/main..HEAD— 6 commits, layered foundation → memory.Validation Run
pnpm --filter openhuman-app format:check— clean aftercargo fmtpasspnpm typecheck— cleancargo test --lib core::auth,core::socketio::tests,core::event_bind_tokens,core::jsonrpc::tests::telegram_callback_*,openhuman::channels::bus::inbound_thread_id_tests,openhuman::agent::harness::memory_context_safety— all passcargo check --manifest-path Cargo.tomlcleanValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
auth.tokenand rejects non-allowlisted origins; SSE/eventsrequires either aBearerheader or a minted bind token; Telegram callback validates fetch-metadata; inbound channel keying matchesconversation_history_key; memory recall wraps connector-namespaced hits in<untrusted-source>.Parity Contract
/eventsstill accepted;/auth/telegrambrowsers without fetch-metadata headers still served via Referer fallback; inbound bus publishers withoutsenderproduce the historical channel-only key.AuthedConnectionmarker + connect-time disconnect; bind-token single-shot with TTL clamp; telegram referer host fallback; memory namespace allowlist + connector key-prefix default-deny.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Security
Tests