feat(gateway): record session owners and restrict writes to owner or admin - #260
Merged
Merged
Conversation
Account-token verification runs PBKDF2 (120,000 iterations, about 17 ms of CPU) inside OperatorAccountService's global lock and then rewrites the accounts file. The role checks added in this branch meant that /v1/*, /apps/chat, A2A, /ws, /ws/live, and mutating MCP tools verified the same token twice per request, halving account-token throughput on those surfaces. Cache the verification outcome in HttpContext.Items for the rest of the request. Revocation, disabling, and role changes still apply from the next request. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
/ws/live admitted any authenticated role. It runs no tools, but it opens a live session on the gateway's provider credentials, so a viewer token could spend them. It now uses CanExecuteAgent like /ws and closes below operator with 1008. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Companion opened the chat socket before loading the account's role, so a viewer was admitted and then closed by the gateway. It now loads the role first; when the gateway reports a role below operator it explains that chat needs the operator role instead of connecting, and keeps the read-only status views. A role that is only the no-token placeholder does not block the connection, and the role is applied as soon as the auth session loads rather than after the setup status call. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Session.AuthenticatedUserId scopes per-user capability bindings and is passed to MCP servers as _meta.userId, falling back to SenderId. Only /ws set it, so turns from REST messages, MCP send_message, A2A, /v1/*, and /apps/chat ran as whatever senderId or A2A contextId the caller supplied. - EndpointHelpers.ResolveAuthenticatedAccountId resolves the signed-in account (reusing the per-request token verification). REST and MCP stamp it on queued messages; /v1/*, /apps/chat, and the A2A bridge set it on the session. A2A captures it in middleware through an async-local, so detached SDK work still sees it. - The worker no longer lets an account-less external turn inherit the previous writer's account: after an operator posted into a Telegram session, the Telegram user's turns ran as that operator. System, scheduled, automation, and background-continuation turns still act on the session's behalf and keep its identity. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…admin Sessions are keyed only by id, so any operator who knew a session id could post into another account's conversation. - Session.OwnerAccountId is set once, when a signed-in account creates the session (pipeline, /v1/*, /apps/chat, A2A), and never reassigned. - SessionAccess.CanWrite: owned sessions accept the owner or an admin; unowned sessions (channels, cron, older data) stay open and are never claimed by writing to them; callers without an account (bootstrap, open loopback, channel and system turns) are not restricted. - Enforced on every write surface: REST and MCP send_message refuse before queueing (403 / tool error), the worker refuses pipeline turns including /ws with a reply, /apps/chat returns 403, and the A2A bridge emits an error event. Admin status travels with the message because OIDC admins are not in the account store. - Reads are unchanged. GET /api/integration/sessions?owner=me lists the caller's own sessions, backed by an owner filter in the file and SQLite stores and in the shared summary filter. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (40)
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 |
18 tasks done
Telli
commented
Sep 29, 2026
Telli
left a comment
Telli
left a comment
Contributor
Author
There was a problem hiding this comment.
Reviewed session ownership across HTTP, WebSocket, browser-session authentication, and session-management routes. Three remaining paths bypass the intended owner-or-admin boundary; details are inline.
…role Companion's preflight refused to open chat for any gateway-reported role below operator. With Security.AllowViewerAgentExecution on, the gateway still admits those viewers, so the documented migration switch did not work for Companion users. GET /auth/session now reports canExecuteAgent, computed by the same rule CanExecuteAgent enforces, including the opt-out. Companion blocks only when the gateway says false; when the field is absent (older gateways) it connects and lets the gateway decide. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…p-authenticated-account # Conflicts: # CHANGELOG.md
…rship # Conflicts: # CHANGELOG.md
- /ws resolved its account with a helper that returned nothing on a loopback-bound gateway, even with AlwaysRequireAuth or OIDC. Those turns carried no account, so the owner check treated them as accountless and let them write anywhere, and new sessions had no owner. /ws now uses the same caller resolution as the other surfaces. - /v1/* stable sessions are keyed by the bearer token's hash or, for browser sessions, the client address. Two signed-in accounts behind one address derived the same session and the second ran the first's conversation. Both endpoints now apply the owner check under the session lock and return 403 session_forbidden. - Session management (delete, metadata, abort, branch restore, guided recovery) stayed operator-wide, so another operator could delete an owned session and recreate the id as its own. Those routes now follow the owner-or-admin rule. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Reopening to run CI against main. |
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
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.
Description
Stacked on #259 (← #257). Merge those first, then retarget this PR to
main.Sessions are keyed only by id (
SessionManager.GetOrCreateByIdAsync). Any operator who knew or guessed a session id could post into another account's conversation. Mobile clients, for example, use client-generated session ids. #259 made each turn run as the right account; this PR decides whether that account may post to the session at all.Policy, as agreed:
Summary
Session.OwnerAccountId: set once, when a signed-in account creates the session. That covers the pipeline (fromInboundMessage.AuthenticatedUserId),/v1/*,/apps/chat, and A2A. The owner is never reassigned. It lives in the session JSON, so older sessions simply have none.SessionAccess.CanWrite(session, accountId, isAdmin): writes are allowed if the session is unowned, the caller is the owner, or the caller is an admin. Callers with no account are also allowed: bootstrap, open loopback, and channel/system turns. The first two are admin-equivalent; channel and system turns address sessions by their own keys.POST /api/integration/messagesopenclaw.send_message/wsand anything already queuedPOST /apps/chat/v1/*InboundMessage.AuthenticatedUserIsAdminandA2ACallerContext.IsAdmin, because OIDC admins are not in the account store.WebSocketChannel.HandleConnectionAsyncgains anauthenticatedUserIsAdminparameter.GET /api/integration/sessions?owner=melists the caller's own active and persisted sessions. It uses an owner filter inFileMemoryStore,SqliteMemoryStore(json_extract), and the shared summary/session filters.SessionSummarygainsownerAccountId, and callers without an account own none.Behavior changes
Type of Change
owner=mefilter)Validation
dotnet build OpenClaw.Net.slnx --configuration Release: 0 warnings, 0 errorsdotnet test OpenClaw.Net.slnx --configuration Release --no-build: OpenClaw.Tests 3,156 passed / 11 skipped / 0 failed; NacosLiveAcceptance 25 passed; LayaService 67 passeddotnet run --project samples/OpenClaw.HelloAgent -c Release --no-buildNew tests failed first for the right reason:
/apps/chat, A2A, and worker writes succeeded;owner=mereturned everything;The guards cover the owner and admins still writing, unowned sessions staying open, and callers without an account being unrestricted.
Review Notes
CoreJsonContext)docs/AUTHENTICATION.md§3.6 (with zh-CN) andCHANGELOG.mdCommercial or Customer-Driven Contribution Disclosure
Same origin as #256: surfaced during a spec review for AgentQi Mobile, which needs "only my conversations". Vendor-neutral gateway hardening.
Checklist
dotnet test)🤖 Generated with Claude Code