fix(security): replace wildcard CORS on core RPC#2328
Conversation
The in-process JSON-RPC server returned `Access-Control-Allow-Origin: *` on every response, meaning any browser origin in possession of the bearer token could make authenticated cross-origin requests against `/rpc`. Replaces the wildcard with an explicit allowlist: - Tauri v2 webview origins (`tauri://localhost`, `http(s)://tauri.localhost`) - Loopback hosts on any port (`http://127.0.0.1:*`, `http://localhost:*`, `http://[::1]:*`) for the Vite dev server and E2E harnesses - Comma-separated env override `OPENHUMAN_CORE_ALLOWED_ORIGINS` for operator-controlled debug harnesses Disallowed origins receive no ACAO header (browser blocks). Non-browser callers (no `Origin` header) are unaffected. `Vary: Origin` is set so intermediate caches keep per-origin responses distinct. Closes tinyhumansai#2262
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe JSON-RPC server's HTTP CORS handling is hardened by replacing ChangesCORS Origin Allowlist
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ 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. Comment |
|
@senamakel #2328 已作为 #2266 的可合并替代 PR 打开并跑绿:24 success / 3 skipped / 0 pending / 0 failure,CodeRabbit approved,review threads 0 unresolved,当前 head |
graycyrus
left a comment
There was a problem hiding this comment.
Review — graycyrus
Solid security fix. The allowlist logic is clean, the test coverage is thorough, and this fully addresses the acceptance criteria in #2262.
What I checked
| Area | Verdict |
|---|---|
| Origin parsing (Tauri, loopback, IPv6) | Correct — tested against lookalike/suffix attacks |
Vary: Origin via append (not insert) |
Correct — preserves existing Vary values |
| Env override exact-match only | Correct — no substring/suffix matching |
with_cors_headers signature change (pub(super), new origin param) |
Only called from cors_middleware + tests — no breakage |
Test isolation (no env::set_var in parallel tests) |
Properly uses is_origin_allowed_with_extra injection instead |
Non-browser callers (no Origin header) |
Unaffected — no ACAO emitted, other CORS headers still present |
Observations (informational, not blocking)
Access-Control-Allow-Methods,-Allow-Headers, and-Max-Ageare still emitted for rejected origins. Not exploitable (browser blocks without ACAO), but slightly reveals server capabilities. Could gate those behind the allowlist check in a follow-up if desired.std::env::varis read per-request inis_origin_allowed. Negligible for a local RPC server, but if the env override feature sees real use, a cached read might be worth it.
No critical or major findings. LGTM — ready for maintainer approval.
# Conflicts: # src/core/jsonrpc.rs # src/core/jsonrpc_cors_tests.rs
|
huge thanks @YOMXXX, locking down that wildcard cors with a proper allowlist is exactly the kind of hardening we love to see 🙌 extra points for killing the process-global env mutation in the tests too, way cleaner. thanks again for another solid one! |
Summary
Varyresponse values while appendingOrigin.Varypreservation.Problem
src/core/jsonrpc.rsemittedAccess-Control-Allow-Origin: *, so any browser origin that obtained the bearer token could call the local RPC surface.Varyheaders.Solution
is_origin_allowed(origin)as the production env-reading entry point.is_origin_allowed_with_extra(origin, extra_origins)so tests can exercise override parsing without mutating process-global environment.with_cors_headersfromheaders.insert(Vary, Origin)toheaders.append(Vary, Origin).Varypreservation and exact-match override behavior.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. CI coverage gate must confirm this; local focused Rust tests cover the changed paths.## Related— N/A: no coverage-matrix feature row applies.Closes #NNNin the## RelatedsectionImpact
OPENHUMAN_CORE_ALLOWED_ORIGINS.Related
Access-Control-Allow-Origin: *) #2262leighstillard/fix/cors-allowlist.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/2262-cors-allowlist9d1341cff29ef1e1b08721885124ce3267f4a99dValidation Run
pnpm --filter openhuman-app format:check— N/A: Rust-only Core RPC change.pnpm typecheck— N/A: Rust-only Core RPC change.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml cors_tests --lib— 8 passed.cargo fmt --manifest-path Cargo.toml --all;GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml --lib;git diff --check.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Access-Control-Allow-Originfor allowlisted browser origins instead of wildcard*.Parity Contract
Varyvalues.Duplicate / Superseded PR Handling
9d1341cff29ef1e1b08721885124ce3267f4a99d.Summary by CodeRabbit
Bug Fixes
Chores
Tests