fix(security): replace wildcard CORS on core RPC with origin allowlist

[leighstillard]

Summary

Replaces Access-Control-Allow-Origin: * on the in-process JSON-RPC server with an explicit allowlist tied to the origins we actually ship. Possession of the per-launch bearer token is no longer sufficient for an arbitrary browser origin to call /rpc.

Closes #2262.

What changes

src/core/jsonrpc.rs:

• New is_origin_allowed(origin) matches against:
  • Tauri v2 webview origins: tauri://localhost, http://tauri.localhost, https://tauri.localhost
  • Loopback hosts on any port: http://127.0.0.1:*, http://localhost:*, http://[::1]:* (Vite dev server, E2E harnesses)
  • Comma-separated env override OPENHUMAN_CORE_ALLOWED_ORIGINS for operator-controlled debug harnesses
• cors_middleware reads the request's Origin header and passes it to with_cors_headers.
• with_cors_headers(response, origin):
  • Echoes the allowlisted origin in Access-Control-Allow-Origin.
  • Omits the header for disallowed origins (the browser then refuses to surface the response).
  • Always sets Vary: Origin so intermediate caches keep per-origin responses distinct.
  • Non-browser callers (no Origin header) are unaffected.
  • Logs a tracing::warn! line on rejected origins for diagnosis.

src/core/jsonrpc_cors_tests.rs (new, wired via #[path] like jsonrpc_tests.rs):

• Allowed Tauri origins return ACAO matching the request.
• Loopback hosts (IPv4, IPv6 literal, named, with/without port) allowed.
• Common disallowed origins (https://attacker.example, look-alike subdomains, https://localhost, null) refused with no ACAO header.
• Missing Origin header → no ACAO, but Vary: Origin still set.
• Env override allows exact-match additional origins; does not allow look-alike suffixes.
• Allow-Methods, Allow-Headers, and Max-Age are always set.

Why

The bearer token is the only protection on /rpc. Possible token-leak paths include shared error logs, screenshots, Sentry breadcrumbs, or a malicious script inside a CEF child webview (Slack/Gmail/etc.). With wildcard CORS, possession of the token from any origin in any browser is sufficient — the desktop RPC surface includes shell tool calls, memory access, credential reads, and sending messages on the user's behalf, so the blast radius is high.

Compatibility

• Tauri webview (coreRpcClient.ts fetch() path): tauri://localhost / http(s)://tauri.localhost are allowlisted. No frontend change required.
• pnpm dev (Vite at http://localhost:1420): allowlisted via the loopback rule.
• E2E (tauri-driver on :4444, Appium): allowlisted via the loopback rule.
• Non-browser callers (CLI, curl, integration tests that don't send Origin): unchanged.
• Debug harnesses on non-loopback origins: set OPENHUMAN_CORE_ALLOWED_ORIGINS=https://my-harness,....

Test plan

• cargo test --lib cors_tests — 6 / 6 passing locally.
• cargo check --lib — clean.
• cargo fmt --check on changed files — clean.
• Smoke pnpm dev:app on the Tauri shell and confirm the frontend RPC fetches still succeed.
• CI: full test suite + coverage gate.

Out of scope

Tracked as follow-ups from the same security audit:

• Webhook signature / replay verification.
• OS-keychain backing for the encryption master key.
• Prompt-injection enforcement (currently detect-only).
• Dual-channel trust model for scraped third-party content vs. user instructions.

Summary by CodeRabbit

• New Features

  • Added CORS origin allowlisting with configurable environment variable support.
  • Only approved origins (Tauri, loopback, and configured) can access the API.

• Security

  • Replaced permissive cross-origin access policy with restrictive allowlist that rejects unknown origins.
  • Disallowed origins no longer receive confirmation headers in responses.

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaces wildcard CORS with explicit origin allowlisting for Tauri and loopback origins. Introduces is_origin_allowed() validation, updates CORS middleware to conditionally emit Access-Control-Allow-Origin based on allowlist, and adds comprehensive test coverage for allowed/rejected origins and required response headers.

Changes

CORS Allowlist and Origin Validation

Layer / File(s)	Summary
CORS allowlist validation and middleware src/core/jsonrpc.rs	Introduces is_origin_allowed() to validate origins against hardcoded Tauri and loopback origins, plus optional OPENHUMAN_CORE_ALLOWED_ORIGINS environment override. Updates with_cors_headers() to conditionally emit Access-Control-Allow-Origin and Vary: Origin headers only for allowed origins, logging rejections; integrates origin validation into CORS middleware preflight and normal response paths.
CORS allowlist test coverage src/core/jsonrpc_cors_tests.rs	Comprehensive test suite covering Tauri webview origin acceptance, loopback origin variants (IPv4, IPv6, hostname with ports), rejection of attacker-crafted origins, omission of Allow-Origin when origin is missing, environment variable override validation, and verification that required CORS response headers (Allow-Methods, Allow-Headers, Max-Age) are always present.

Sequence Diagram

flowchart LR
  A["HTTP Request<br/>with Origin header"] --> B["Extract Origin<br/>from request"]
  B --> C["is_origin_allowed<br/>checks:<br/>• Tauri origins<br/>• Loopback 127.0.0.1<br/>• Loopback localhost<br/>• Loopback ::1<br/>• ENV override"]
  C -->|Origin allowed| D["Response:<br/>Access-Control-Allow-Origin: origin<br/>Vary: Origin<br/>CORS headers"]
  C -->|Origin denied| E["Response:<br/>No Allow-Origin<br/>Vary: Origin<br/>CORS headers<br/>warn log"]
  C -->|No Origin header| F["Response:<br/>No Allow-Origin<br/>Vary: Origin<br/>CORS headers"]
  D --> G["Browser allows<br/>cross-origin use"]
  E --> H["Browser blocks<br/>cross-origin use"]
  F --> I["Non-browser or<br/>same-origin unaffected"]

Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested Labels

rust-core, working

Suggested Reviewers

• senamakel

Poem

🐰 A wildcard once danced in the headers with glee,
But we've tamed it to Tauri and loopback's decree—
No more * for the rogues with a borrowed token's aim,
Just Localhost, origins true, and the allowlist's name! 🔐

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: replacing wildcard CORS with an origin allowlist on the core RPC server.
Linked Issues check	✅ Passed	The PR implements all coding requirements from #2262: allowlist mechanism for Tauri/loopback origins, env-var override, origin echoing, header handling, logging, and comprehensive unit tests.
Out of Scope Changes check	✅ Passed	All changes are directly related to implementing the origin allowlist mechanism specified in #2262; no unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 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/jsonrpc_cors_tests.rs`:
- Around line 80-103: The test env_override_allows_extra_origins unsafely
mutates process-global env via unsafe set_var/remove_var; instead stop touching
the environment and either (A) call the pure helper that parses the
ALLOWED_ORIGINS value directly (e.g., add/use a function like
parse_allowed_origins(allowlist: &str) and test is_origin_allowed against that
parsed set) or (B) serialize the test with the serial_test crate by annotating
the test with #[serial] (and remove unsafe env mutations), or (C) use an
env-locking utility to safely set/remove ALLOWED_ORIGINS_ENV; update the test to
reference ALLOWED_ORIGINS_ENV and is_origin_allowed (or the new parse helper)
accordingly so no unsafe std::env::set_var/remove_var is used.

In `@src/core/jsonrpc.rs`:
- Around line 687-689: with_cors_headers currently overwrites any existing Vary
header; update the function (with_cors_headers) to preserve and append "Origin"
instead of replacing it: read headers.get(header::VARY), if present combine the
existing value and "Origin" (comma-separated) while avoiding duplicate "Origin",
otherwise insert HeaderValue::from_static("Origin"); then set the resulting
header back into headers_mut() so inner responses keep their original Vary
contract.

🪄 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: a1f54936-5130-47ab-8b31-fb909fd6ff02

📥 Commits

Reviewing files that changed from the base of the PR and between 094d482 and 9df8938.

📒 Files selected for processing (2)

• src/core/jsonrpc.rs
• src/core/jsonrpc_cors_tests.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

What are the safety requirements for Rust's std::env::set_varandstd::env::remove_var, and are they sound to call inside parallel unit tests that may read environment variables concurrently?

💡 Result:

The functions std::env::set_var and std::env::remove_var are inherently unsafe to use in multi-threaded programs on most operating systems (specifically Unix-like systems) [1][2][3]. Safety Requirements: These functions are safe to call only in single-threaded programs or on Windows, where the operating system provides thread-safe environment manipulation [1][2][3]. On other platforms, they are considered unsafe because the underlying C library functions (such as setenv and unsetenv) are not thread-safe [4][5]. Because the environment is a global mutable state for the entire process, any concurrent read or write to the environment—even by other parts of the standard library or external C code—can result in undefined behavior [1][6][3]. It is practically impossible to guarantee that no other thread is reading the environment, as many functions (including those for DNS lookups or other system interactions) may access it internally without your knowledge [1][2][3]. Soundness in Parallel Unit Tests: It is generally not sound to call std::env::set_var or std::env::remove_var in parallel unit tests [7][5]. Because Rust's test runner executes tests in parallel by default, multiple tests may attempt to modify or read the environment simultaneously, leading to data races and undefined behavior [7][8]. To safely test code that relies on environment variables, you should: 1. Run tests serially: You can force tests to run one at a time using the --test-threads=1 flag with cargo test [9]. 2. Use synchronization: Use a crate like env-lock, which provides a mutex-based mechanism to ensure that only one test can modify or access the environment at a time [10][8]. 3. Avoid environment variables: If possible, refactor your code to accept configuration via arguments or dependency injection rather than relying on global environment variables [1][5].

Citations:

• 1: https://doc.rust-lang.org/std/env/fn.set_var.html
• 2: https://public-docs.ferrocene.dev/main/std/env/fn.set_var.html
• 3: https://doc.rust-lang.org/std/env/fn.remove_var.html
• 4: https://users.rust-lang.org/t/why-env-set-var-unsafe-now/134233
• 5: https://doc.rust-lang.org/edition-guide/rust-2024/newly-unsafe-functions.html
• 6: Consider deprecating and/or modifying behavior of std::env::set_var rust-lang/rust#90308
• 7: std::env::{set_var, remove_var} is called in tests without safety documentation rust-lang/rust#148432
• 8: https://crates.io/crates/env-lock
• 9: https://users.rust-lang.org/t/passing-test-threads-1-to-cargo-test-by-default/87225
• 10: https://docs.rs/env-lock/latest/env_lock

---

Remove unsafe environment mutation from parallel unit test.

The unsafe { set_var/remove_var } calls are unsound in a parallel test harness. Rust's default test runner executes tests concurrently, and on Unix-like systems, the underlying C library functions for environment manipulation are not thread-safe. Even if no other test in this crate intentionally reads ALLOWED_ORIGINS_ENV, the environment is process-global state—other parts of the standard library, external dependencies, or system code may access it concurrently, leading to undefined behavior.

Refactor to either: (1) extract a pure helper that accepts the allowlist as a parameter and test that directly without env mutation, (2) use a synchronization crate like env-lock to serialize access, or (3) mark this test with #[serial] and run it serially (requires serial_test crate).

🤖 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_cors_tests.rs` around lines 80 - 103, The test
env_override_allows_extra_origins unsafely mutates process-global env via unsafe
set_var/remove_var; instead stop touching the environment and either (A) call
the pure helper that parses the ALLOWED_ORIGINS value directly (e.g., add/use a
function like parse_allowed_origins(allowlist: &str) and test is_origin_allowed
against that parsed set) or (B) serialize the test with the serial_test crate by
annotating the test with #[serial] (and remove unsafe env mutations), or (C) use
an env-locking utility to safely set/remove ALLOWED_ORIGINS_ENV; update the test
to reference ALLOWED_ORIGINS_ENV and is_origin_allowed (or the new parse helper)
accordingly so no unsafe std::env::set_var/remove_var is used.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve any existing Vary values.

Line 689 overwrites the whole Vary header, so an inner response that already set Vary loses that cache contract. Please append Origin instead of replacing the header.

💡 Minimal fix

-    headers.insert(header::VARY, HeaderValue::from_static("Origin"));
+    headers.append(header::VARY, HeaderValue::from_static("Origin"));

🤖 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 687 - 689, with_cors_headers currently
overwrites any existing Vary header; update the function (with_cors_headers) to
preserve and append "Origin" instead of replacing it: read
headers.get(header::VARY), if present combine the existing value and "Origin"
(comma-separated) while avoiding duplicate "Origin", otherwise insert
HeaderValue::from_static("Origin"); then set the resulting header back into
headers_mut() so inner responses keep their original Vary contract.

[antfleet-ops]

Thanks for the CORS allowlist fix! Two test-safety issues in jsonrpc_cors_tests.rs worth addressing before this merges:

1. Race condition in parallel test runs

env_override_allows_extra_origins mutates the process-global ALLOWED_ORIGINS_ENV var using unsafe { std::env::set_var(...) }. The current SAFETY comment says "no other test reads this env var", but allows_tauri_webview_origins, allows_loopback_with_any_port, rejects_disallowed_origins, and others all call is_origin_allowed() — which does read ALLOWED_ORIGINS_ENV. Cargo runs tests in parallel by default, so these tests can observe a partially-mutated env state, causing spurious failures or incorrect test coverage.

Fix: add a module-level RwLock, have all env-reading tests take a shared read lock, and have env_override_allows_extra_origins take the exclusive write lock.

2. Vary: Origin not asserted in always_sets_methods_headers_and_max_age

The test verifies Access-Control-Allow-Origin, Access-Control-Allow-Methods, Access-Control-Allow-Headers, and Access-Control-Max-Age — but doesn't assert Vary: Origin. Omitting Vary: Origin on credentialed or origin-gated responses causes shared caches (CDNs, reverse proxies) to serve one origin's response to another, breaking the allowlist guarantee.

Fix: add an assertion for Vary: Origin in that test.

---

Both fixes are implemented in antfleet-ops/openhuman@fix/cors-test-safety (single commit d71ca112). Happy to add them to this PR if that's easier.

[YOMXXX]

I could not push directly to leighstillard/fix/cors-allowlist (Permission to leighstillard/openhuman.git denied to YOMXXX), so I opened #2328 with the same CORS allowlist fix plus the two outstanding review follow-ups:

• removes unsafe process-global env mutation from jsonrpc_cors_tests.rs by testing override parsing through an injected helper
• appends Vary: Origin instead of overwriting existing Vary values, with regression coverage

Local validation on #2328 head 9d1341cff29ef1e1b08721885124ce3267f4a99d:

cargo fmt --manifest-path Cargo.toml --all
GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml cors_tests --lib
GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml --lib
git diff --check

Focused CORS tests: 8 passed. Maintainers can either review/merge #2328 or cherry-pick 9d1341cff29ef1e1b08721885124ce3267f4a99d into this branch.

[senamakel]

huge welcome @leighstillard and massive thanks for landing your first one here 🙌 swapping the wildcard cors for a real origin allowlist on the rpc server is exactly the kind of hardening we love to see. you've earned a spot in our builder discord, come hang with us at https://discord.tinyhumans.ai/ ✨

[leighstillard]

Cheers I'm already there as primax. or primax#1590. @senamakel - is there a roadmap or contributor forum anywhere? I have a few features I want to add but they should be discussed first.