fix(security): fail closed in pairing.rs::is_authenticated when token is unset (#1919)#2108
Conversation
… is unset (tinyhumansai#1919) Reject empty bearer tokens and unconfigured token sets regardless of require_pairing. Add ensure_core_rpc_token_for_bind to auto-generate and persist a core token on non-loopback binds when OPENHUMAN_CORE_TOKEN is unset. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR hardens RPC authentication and introduces token binding for public interfaces to resolve a critical security vulnerability where unauthenticated RPC access was possible when binding on non-loopback addresses without a token. Authentication now fails closed, and public binds automatically generate and persist tokens unless explicitly provided. ChangesCore RPC Authentication Security
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/security/pairing.rs (1)
310-353: 💤 Low valueConsider logging when empty env token is silently ignored on loopback.
When
env_token = Some("")on a loopback bind, the function silently returnsOk(None)without any log. This might make debugging harder if an operator mistakenly setsOPENHUMAN_CORE_TOKEN=""thinking it's configured. Consider adding a debug log similar to line 335-338 to indicate the empty token was ignored.This is a minor observability improvement—current behavior is safe since loopback doesn't require auth.
🔧 Suggested logging addition
if is_public_bind(host) { log::error!( "[openhuman:pairing] {CORE_TOKEN_ENV_VAR} is set but empty on public bind host={host}" ); return Err(CoreBindTokenError::EmptyEnvToken { host: host.to_string(), }); + } else { + log::debug!( + "[openhuman:pairing] {CORE_TOKEN_ENV_VAR} is set but empty on loopback host={host}, ignoring" + ); } }🤖 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/security/pairing.rs` around lines 310 - 353, In ensure_core_rpc_token_for_bind, when env_token is Some("") the code currently logs an error only for public binds and is silent for loopback; add a debug log in the branch where raw.trim() is empty and is_public_bind(host) is false (the same branch that later logs loopback info) so operators see that an empty OPENHUMAN_CORE_TOKEN was intentionally ignored; include host and CORE_TOKEN_ENV_VAR in the debug message to match existing logging style.
🤖 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.
Nitpick comments:
In `@src/openhuman/security/pairing.rs`:
- Around line 310-353: In ensure_core_rpc_token_for_bind, when env_token is
Some("") the code currently logs an error only for public binds and is silent
for loopback; add a debug log in the branch where raw.trim() is empty and
is_public_bind(host) is false (the same branch that later logs loopback info) so
operators see that an empty OPENHUMAN_CORE_TOKEN was intentionally ignored;
include host and CORE_TOKEN_ENV_VAR in the debug message to match existing
logging style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d19b7d67-2e00-46ee-9269-055de7268c62
📒 Files selected for processing (3)
src/openhuman/security/mod.rssrc/openhuman/security/pairing.rssrc/openhuman/security/pairing_tests.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Solid security fix — the fail-closed is_authenticated change correctly eliminates the auth bypass when require_pairing = false. The ensure_core_rpc_token_for_bind API is well-designed with clear error semantics.
| File | Area | Change |
|---|---|---|
pairing.rs |
Security | Removed require_pairing bypass, added empty-token rejection, new bind-time token API |
pairing_tests.rs |
Tests | Good coverage of empty tokens, no-tokens-configured, and bind-time auto-generation |
mod.rs |
Exports | Re-exports new public symbols |
Two minor observations below — neither blocks merge.
| } | ||
|
|
||
| #[cfg(unix)] | ||
| { |
There was a problem hiding this comment.
[minor] write_all(token.as_bytes()) doesn't append a trailing newline. If core/auth.rs::init_rpc_token writes with a newline (common for token files consumed by shell scripts or cat), the two code paths would produce inconsistent files. Worth checking for parity — a \n suffix is cheap insurance.
| .create(true) | ||
| .truncate(true) | ||
| .mode(0o600) | ||
| .open(path)?; |
There was a problem hiding this comment.
[minor] On Windows the #[cfg(not(unix))] fallback uses std::fs::write which inherits the default ACL — the token file may be readable by other users on the system. If Windows is a supported deployment target for public-bind scenarios, consider using SetNamedSecurityInfo or similar to restrict access. Low priority since Docker/cloud deployments are predominantly Linux.
… is unset (tinyhumansai#1919) (tinyhumansai#2108) Co-authored-by: Cursor <cursoragent@cursor.com>
… is unset (tinyhumansai#1919) (tinyhumansai#2108) Co-authored-by: Cursor <cursoragent@cursor.com>
… is unset (tinyhumansai#1919) (tinyhumansai#2108) Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Summary
PairingGuard::is_authenticatednow fails closed: empty/whitespace bearer tokens are always rejected, and requests are rejected when no paired tokens are configured (regardless ofrequire_pairing).ensure_core_rpc_token_for_bindfor non-loopback binds: uses a non-emptyOPENHUMAN_CORE_TOKENwhen set, otherwise auto-generates a 256-bit token and persists it to{workspace}/core.token(same behavior as standaloneinit_rpc_tokenincore/auth.rs).Problem
When
OPENHUMAN_CORE_HOSTis non-loopback (e.g. Docker’s0.0.0.0) andOPENHUMAN_CORE_TOKENis unset, pairing could be constructed withrequire_pairing = false.is_authenticatedthen returnedtruefor any token—including empty—exposing the full RPC surface on the LAN.Solution
Auth-check layer (
pairing.rs): removed theif !self.require_pairing { return true; }bypass. Authentication always requires a non-empty bearer that matches a configured paired-token hash.Bind-time token policy: chose (b) auto-generate and persist on public bind without env token (aligned with existing standalone CLI
core.tokenflow). ExplicitOPENHUMAN_CORE_TOKEN=""on a public bind returnsCoreBindTokenError::EmptyEnvToken.Coordinates with open PR #2011 (which adds a public-bind warning on
src/core/jsonrpc.rs). This PR fixes the actual auth bypass inpairing.rs; #2011’s warning becomes a defense-in-depth complement. Wiringensure_core_rpc_token_for_bindinto server startup can land in a follow-up outside this batch’s owned paths.Threat model
0.0.0.0.is_authenticated+ token enforcement at bind time viaensure_core_rpc_token_for_bind.OPENHUMAN_CORE_TOKEN=""explicitly on a public bind — still rejected withEmptyEnvToken.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.Related— N/A: no matrix feature IDsdocs/RELEASE-MANUAL-SMOKE.md) — N/ACloses #NNNin theRelatedsectionImpact
require_pairing = falseand no tokens.OPENHUMAN_CORE_TOKENget an auto-generatedcore.tokenon public bind (once startup callsensure_core_rpc_token_for_bind).is_authenticated("") == truewith no tokens will now be denied (intended).Related
ensure_core_rpc_token_for_bindfromrun_server_inner(orthogonal to fix(security): docker hardening, homoglyph detection, async persist, public-bind warning #2011 warning-only PR)AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
cursor/a01-1919-pairing-fail-closedb86efe2b2d3733897fa8c29b397fe5ad6daee5caValidation Run
pnpm --filter openhuman-app format:check— N/A (Rust-only);cargo fmt --all --checkpassedpnpm typecheck— N/A (no TS changes)cargo test -p openhuman --lib security::pairing— 34/34 passedcargo fmt --all --check; pre-pushpnpm rust:checkpassedValidation Blocked
command:pnpm test:rust(full suite)error:not run in this session (focused pairing tests + pre-push hook rust:check only)impact:full regression signal deferred to CI; pairing module fully covered locallyBehavior Changes
core.tokenauto-created on public bind once wiredParity Contract
Nonefromensure_core_rpc_token_for_bind(local dev)is_authenticatedno longer short-circuits onrequire_pairing == falseDuplicate / Superseded PR Handling
Made with Cursor
Summary by CodeRabbit
New Features
Security Improvements