mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-30 06:32:24 +00:00
## Summary - Replaces wildcard Core RPC CORS behavior with an explicit allowlist for Tauri and loopback origins. - Preserves existing `Vary` response values while appending `Origin`. - Removes unsafe process-global env mutation from CORS tests by injecting env override input directly. - Adds regression coverage for env override exact matching and existing `Vary` preservation. ## Problem - `src/core/jsonrpc.rs` emitted `Access-Control-Allow-Origin: *`, so any browser origin that obtained the bearer token could call the local RPC surface. - The prior fix in #2266 addressed the core issue but still had two review blockers: unsafe env mutation in parallel Rust tests and overwriting existing `Vary` headers. - I could not push directly to #2266's fork branch, so this PR carries the same security fix plus the review follow-ups. ## Solution - Keep `is_origin_allowed(origin)` as the production env-reading entry point. - Add `is_origin_allowed_with_extra(origin, extra_origins)` so tests can exercise override parsing without mutating process-global environment. - Change `with_cors_headers` from `headers.insert(Vary, Origin)` to `headers.append(Vary, Origin)`. - Add focused tests for existing `Vary` preservation and exact-match override behavior. ## Submission Checklist - [x] Tests added or updated (happy path + at least one failure / edge case) per [Testing Strategy](../gitbooks/developing/testing-strategy.md#failure-path-requirement) - [x] **Diff coverage ≥ 80%** — changed lines (Vitest + cargo-llvm-cov merged via `diff-cover`) meet the gate enforced by [`.github/workflows/coverage.yml`](../.github/workflows/coverage.yml). CI coverage gate must confirm this; local focused Rust tests cover the changed paths. - [x] Coverage matrix updated — N/A: security boundary fix covered by focused Rust tests; no feature matrix row added/removed/renamed. - [x] All affected feature IDs from the matrix are listed in the PR description under `## Related` — N/A: no coverage-matrix feature row applies. - [x] No new external network dependencies introduced (mock backend used per [Testing Strategy](../gitbooks/developing/testing-strategy.md#mock-policy)) - [x] Manual smoke checklist updated if this touches release-cut surfaces — N/A: Core RPC header behavior only; no manual release checklist surface changed. - [x] Linked issue closed via `Closes #NNN` in the `## Related` section ## Impact - Security: arbitrary non-allowlisted browser origins no longer receive ACAO for local Core RPC responses. - Compatibility: Tauri webview origins, loopback dev/E2E origins, and non-browser callers remain supported. - Operators can still add exact additional debug origins with `OPENHUMAN_CORE_ALLOWED_ORIGINS`. ## Related - Closes #2262 - Supersedes #2266 because I do not have permission to push review fixes to `leighstillard/fix/cors-allowlist`. - Follow-up PR(s)/TODOs: none. --- ## AI Authored PR Metadata (required for Codex/Linear PRs) ### Linear Issue - Key: N/A - URL: N/A ### Commit & Branch - Branch: `fix/2262-cors-allowlist` - Commit SHA: `9d1341cff29ef1e1b08721885124ce3267f4a99d` ### Validation Run - [x] `pnpm --filter openhuman-app format:check` — N/A: Rust-only Core RPC change. - [x] `pnpm typecheck` — N/A: Rust-only Core RPC change. - [x] Focused tests: `GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml cors_tests --lib` — 8 passed. - [x] Rust fmt/check (if changed): `cargo fmt --manifest-path Cargo.toml --all`; `GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml --lib`; `git diff --check`. - [x] Tauri fmt/check (if changed): N/A: Tauri shell unchanged. ### Validation Blocked - `command:` N/A - `error:` N/A - `impact:` N/A ### Behavior Changes - Intended behavior change: Core RPC only echoes `Access-Control-Allow-Origin` for allowlisted browser origins instead of wildcard `*`. - User-visible effect: none expected for packaged app, loopback dev, E2E, or non-browser callers. ### Parity Contract - Legacy behavior preserved: Tauri origins, loopback origins, debug env overrides, CORS methods/headers/max-age, and no-Origin non-browser callers remain supported. - Guard/fallback/dispatch parity checks: focused CORS unit tests cover allowed origins, denied origins, no-Origin callers, exact env override matching, and preserved `Vary` values. ### Duplicate / Superseded PR Handling - Duplicate PR(s): #2266 - Canonical PR: this PR if maintainers prefer an immediately updated branch; otherwise #2266 can cherry-pick `9d1341cff29ef1e1b08721885124ce3267f4a99d`. - Resolution (closed/superseded/updated): #2266 remains open; this PR carries the requested review fixes because direct push to the fork branch was denied. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Tightened CORS handling to enforce an origin allowlist; only trusted local schemes and loopback addresses are allowed by default, and disallowed origins no longer receive CORS responses. * **Chores** * Added support for configuring extra allowed origins via environment configuration. * **Tests** * Added comprehensive tests for allowlist decisions, header emission (including Vary behavior), and edge cases. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2328?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Leigh Stillard <leigh@stillard.com> Co-authored-by: 李冠辰 <liguanchen@xiaomi.com> Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
143 lines
3.9 KiB
Rust
143 lines
3.9 KiB
Rust
//! Unit tests for the CORS allowlist and header-emission logic in `jsonrpc.rs`.
|
|
|
|
use axum::http::{header, HeaderValue, StatusCode};
|
|
use axum::response::{IntoResponse, Response};
|
|
|
|
use super::{is_origin_allowed, is_origin_allowed_with_extra, with_cors_headers};
|
|
|
|
fn ok_response() -> Response {
|
|
(StatusCode::OK, "").into_response()
|
|
}
|
|
|
|
fn allow_origin(response: &Response) -> Option<String> {
|
|
response
|
|
.headers()
|
|
.get(header::ACCESS_CONTROL_ALLOW_ORIGIN)
|
|
.and_then(|v| v.to_str().ok())
|
|
.map(str::to_owned)
|
|
}
|
|
|
|
#[test]
|
|
fn allows_tauri_webview_origins() {
|
|
for origin in [
|
|
"tauri://localhost",
|
|
"http://tauri.localhost",
|
|
"https://tauri.localhost",
|
|
] {
|
|
assert!(is_origin_allowed(origin), "expected {origin} to be allowed");
|
|
let r = with_cors_headers(ok_response(), Some(origin));
|
|
assert_eq!(allow_origin(&r).as_deref(), Some(origin));
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn allows_loopback_with_any_port() {
|
|
for origin in [
|
|
"http://127.0.0.1:1420",
|
|
"http://localhost:5173",
|
|
"http://[::1]:4444",
|
|
"http://localhost",
|
|
] {
|
|
assert!(is_origin_allowed(origin), "expected {origin} to be allowed");
|
|
let r = with_cors_headers(ok_response(), Some(origin));
|
|
assert_eq!(allow_origin(&r).as_deref(), Some(origin));
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn rejects_disallowed_origins() {
|
|
for origin in [
|
|
"https://attacker.example",
|
|
"http://evil.localhost.attacker.example",
|
|
"https://127.0.0.1.attacker.example",
|
|
// HTTPS variant of localhost is NOT a configuration we ship — refuse.
|
|
"https://localhost",
|
|
"null",
|
|
] {
|
|
assert!(
|
|
!is_origin_allowed(origin),
|
|
"expected {origin} to be rejected"
|
|
);
|
|
let r = with_cors_headers(ok_response(), Some(origin));
|
|
assert!(
|
|
allow_origin(&r).is_none(),
|
|
"disallowed origin {origin} leaked Access-Control-Allow-Origin"
|
|
);
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn missing_origin_emits_no_acao_but_sets_vary() {
|
|
let r = with_cors_headers(ok_response(), None);
|
|
assert!(allow_origin(&r).is_none());
|
|
assert_eq!(
|
|
r.headers().get(header::VARY).and_then(|v| v.to_str().ok()),
|
|
Some("Origin")
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn env_override_allows_extra_origins() {
|
|
let extra_origins = Some("https://debug.internal, http://harness:9000");
|
|
|
|
assert!(is_origin_allowed_with_extra(
|
|
"https://debug.internal",
|
|
extra_origins
|
|
));
|
|
assert!(is_origin_allowed_with_extra(
|
|
"http://harness:9000",
|
|
extra_origins
|
|
));
|
|
assert!(!is_origin_allowed_with_extra(
|
|
"https://debug.internal.attacker.example",
|
|
extra_origins
|
|
));
|
|
}
|
|
|
|
#[test]
|
|
fn preserves_existing_vary_values() {
|
|
let mut response = ok_response();
|
|
response
|
|
.headers_mut()
|
|
.insert(header::VARY, HeaderValue::from_static("Accept-Encoding"));
|
|
|
|
let r = with_cors_headers(response, None);
|
|
let values = r
|
|
.headers()
|
|
.get_all(header::VARY)
|
|
.iter()
|
|
.map(|v| v.to_str().unwrap_or("<invalid>"))
|
|
.collect::<Vec<_>>();
|
|
|
|
assert_eq!(values, vec!["Accept-Encoding", "Origin"]);
|
|
}
|
|
|
|
#[test]
|
|
fn env_override_does_not_allow_lookalike_suffixes() {
|
|
assert!(!is_origin_allowed_with_extra(
|
|
"https://debug.internal.attacker.example",
|
|
Some("https://debug.internal")
|
|
));
|
|
}
|
|
|
|
#[test]
|
|
fn always_sets_methods_headers_and_max_age() {
|
|
let r = with_cors_headers(ok_response(), Some("tauri://localhost"));
|
|
let h = r.headers();
|
|
assert_eq!(
|
|
h.get(header::ACCESS_CONTROL_ALLOW_METHODS)
|
|
.and_then(|v| v.to_str().ok()),
|
|
Some("GET, POST, OPTIONS")
|
|
);
|
|
assert_eq!(
|
|
h.get(header::ACCESS_CONTROL_ALLOW_HEADERS)
|
|
.and_then(|v| v.to_str().ok()),
|
|
Some("Content-Type, Authorization")
|
|
);
|
|
assert_eq!(
|
|
h.get(header::ACCESS_CONTROL_MAX_AGE)
|
|
.and_then(|v| v.to_str().ok()),
|
|
Some("86400")
|
|
);
|
|
}
|