fix(gateway,companion): role-gate /ws/live and check role before Companion connects - #257
Merged
Merged
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe gateway now reports agent-execution permission through ChangesOperator-role checks across WebSocket connections
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Companion
participant AuthSessionEndpoint
participant Gateway
participant ChatWebSocket
Companion->>AuthSessionEndpoint: Request /auth/session
AuthSessionEndpoint->>Gateway: Resolve agent-execution permission
Gateway-->>AuthSessionEndpoint: Return permission
AuthSessionEndpoint-->>Companion: Return canExecuteAgent
alt Permission is false
Companion-->>Companion: Remain disconnected and show role message
else Permission is true or unavailable
Companion->>ChatWebSocket: Attempt chat connection
end
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change adds an operator-role check to /ws/live and lets Companion stop before connecting when the gateway reports the caller cannot run the agent. Gateway enforcement remains authoritative, and older gateways that omit the field still connect. No blocking merge risk was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The live connection gains a server-enforced role check, while Companion uses the Gateway’s permission report to avoid connections it knows will be denied. No material increase in access was established. Token decisions can remain unchanged during a single request, so immediate revocation is not guaranteed. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 9 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
19 of 20 tasks
Telli
force-pushed
the
fix/ws-live-role-and-companion-role-hint
branch
from
September 28, 2026 19:15
90a2848 to
1afa9a2
Compare
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>
Telli
force-pushed
the
fix/ws-live-role-and-companion-role-hint
branch
from
September 28, 2026 21:55
1afa9a2 to
4e64d0e
Compare
This was referenced Sep 28, 2026
18 tasks done
Contributor
Author
|
Reopening to run CI now that this PR targets main. |
Telli
commented
Sep 29, 2026
Telli
left a comment
Telli
left a comment
Contributor
Author
There was a problem hiding this comment.
Reviewed the role gate and Companion connection flow. One compatibility issue needs a fix before the documented viewer migration setting works consistently.
…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>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| public void Dispose() | ||
| { | ||
| try { Directory.Delete(_storagePath, recursive: true); } | ||
| catch { } |
| public void Dispose() | ||
| { | ||
| try { Directory.Delete(_storagePath, recursive: true); } | ||
| catch { } |
|
|
||
| private MainWindowViewModel CreateViewModelWithAuthSession(string authSessionJson) | ||
| { | ||
| var dir = Path.Combine(Path.GetTempPath(), "openclaw-companion-connection-tests", Guid.NewGuid().ToString("N")); |
|
|
||
| public sealed class EndpointHelpersAuthenticationTests : IDisposable | ||
| { | ||
| private readonly string _storagePath = Path.Combine(Path.GetTempPath(), "openclaw-endpoint-auth-tests", Guid.NewGuid().ToString("N")); |
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
Follow-up to #256. This branch also carries #261's commit (
perf(gateway): verify each request's account token once); merge #261 first and it drops out of this diff.Two gaps left after #256:
/ws/liveadmitted any authenticated role. The live model bridge runs no tools, but it opens a live session on the gateway's provider credentials, so a viewer token could spend them.OnClosedhandling from fix(gateway): role-gate agent execution and close /apps loopback bypass #256.Summary
/ws/liveusesEndpointHelpers.CanExecuteAgentlike/ws. Belowoperatorit is accepted, then closed with 1008 and the operator-role reason.AllowViewerAgentExecutionand the denial log cover it like the other surfaces./auth/sessionbefore connecting. When the gateway reports a role belowoperator, Companion says so ("Chat needs the operator role; ask an admin to change this account's role") and skips the chat connection. Read-only status views still load./ws/livedoesn't run the agent.Behavior changes
/auth/sessionrequest before the WebSocket connect rather than after it.Type of Change
Validation
dotnet build OpenClaw.Net.slnx --configuration Release: 0 warnings, 0 errorsdotnet test OpenClaw.Net.slnx --configuration Release --no-build: OpenClaw.Tests 3,125 passed / 11 skipped / 0 failed (rebased onto fix(gateway): role-gate agent execution and close /apps loopback bypass #256's latest head, a4a5697); NacosLiveAcceptance 25 passed; LayaService 67 passeddotnet run --project samples/OpenClaw.HelloAgent -c Release --no-buildNew tests failed first for the right reason:
/ws/liveconnection stayed open;Guards cover an operator on
/ws/livestaying open, and a token-less Companion still attempting to connect.Review Notes
docs/AUTHENTICATION.mdwith zh-CN,CHANGELOG.md)Commercial or Customer-Driven Contribution Disclosure
Same origin as #256: surfaced during a spec review for AgentQi Mobile. Vendor-neutral gateway and Companion hardening.
Checklist
dotnet test)🤖 Generated with Claude Code
Summary by CodeRabbit
/ws/liveconnections without the operator role are closed with a policy-violation response. The Companion checks the gateway’s reported execution permission before connecting and explains when access is denied; read-only status views remain available. If the gateway omits this permission information, the Companion proceeds with the connection attempt./ws/liveresponse for lower-privilege roles, and the execution-permission information reported by/auth/session.