Skip to content

fix(security): remove temporary viewer agent execution bypass - #267

Merged
Telli merged 2 commits into
mainfrom
codex/remove-viewer-agent-execution
Oct 1, 2026
Merged

Telli merged 2 commits into
mainfrom
codex/remove-viewer-agent-execution

Conversation

@Telli

@Telli Telli commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Description

Remove the temporary Security.AllowViewerAgentExecution bypass. Authenticated viewer credentials can no longer run the agent, and GET /auth/session reports the same authorization and role decision enforced by execution endpoints.

If the retired key remains in configuration, startup fails before binding with instructions to remove it and grant affected accounts the operator role. This applies even when the value is false, empty, null, or malformed.

Summary

  • Remove the Core setting, authorization exception, posture risk flag, and obsolete switch tests.
  • Update the admin role hint, Companion comments/tests, English and Chinese authentication guides, and changelog.
  • Add migration regression tests for stale configuration, case-insensitive keys, and environment variables.

Related Issues

Fixes #258.

Type of Change

  • Breaking change
  • Documentation update
  • Tests

Validation

Validated the branch after merging current main at d8d3936b744677c6442e9287340ead9dcdbacea1:

  • dotnet build OpenClaw.Net.slnx --configuration Release — restore succeeded; zero warnings/errors.
  • dotnet test OpenClaw.Net.slnx --configuration Release --no-build — 3,286 passed, 11 opt-in integration tests skipped, zero failures.
  • dotnet run --project samples/OpenClaw.HelloAgent -c Release --no-build — agent/tool smoke passed.
  • Actual Gateway startup rejected the retired environment key set to true, false, or a malformed value, plus JSON and command-line keys set to false, with migration instructions.
  • 281 focused gateway authorization/security, bootstrap, endpoint authentication, MCP App proxy, and Companion tests passed on the original security commit.
  • git diff --check

Review Notes

  • NativeAOT compatibility considered; no new reflection, dependencies, or runtime code generation.
  • Security posture and unsafe defaults considered.
  • Docs/tests updated.
  • Scoped to the tracked security cleanup.

Release sequencing: maintainer-approved for the next release together with the role-gating changes from #256. The earlier draft hold has been lifted. Operators must grant the operator role to accounts that need agent execution and remove the retired configuration key before upgrading.

Commercial or Customer-Driven Contribution Disclosure

Repository security cleanup for #258; no vendor-specific integration or downstream product behavior is added.

Checklist

  • Read CONTRIBUTING and the relevant maintainer review checklist.
  • Followed existing code style.
  • Added tests for the migration failure behavior.
  • All executed tests passed locally.
  • Updated documentation and checked authorization/input-validation implications.
  • Disclosed contribution scope.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 7005d3e0-7725-4528-864c-c3f49e6d7026

📥 Commits

Reviewing files that changed from the base of the PR and between 3a3368a and d8d3936.

📒 Files selected for processing (13)
  • CHANGELOG.md
  • docs/AUTHENTICATION.md
  • docs/zh-CN/AUTHENTICATION.md
  • src/OpenClaw.Companion/ViewModels/MainWindowViewModel.cs
  • src/OpenClaw.Core/Models/GatewayConfig.cs
  • src/OpenClaw.Gateway/Bootstrap/GatewayBootstrapExtensions.cs
  • src/OpenClaw.Gateway/Endpoints/AdminEndpoints.Support.cs
  • src/OpenClaw.Gateway/Endpoints/EndpointHelpers.cs
  • src/OpenClaw.Gateway/SecurityPostureBuilder.cs
  • src/OpenClaw.Gateway/wwwroot/admin.html
  • src/OpenClaw.Tests/CompanionConnectionTests.cs
  • src/OpenClaw.Tests/GatewayAdminEndpointTests.cs
  • src/OpenClaw.Tests/GatewayBootstrapExtensionsTests.cs
💤 Files with no reviewable changes (2)
  • src/OpenClaw.Core/Models/GatewayConfig.cs
  • src/OpenClaw.Gateway/SecurityPostureBuilder.cs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The gateway removes the viewer agent execution setting and rejects startup when the key remains in configuration. Agent execution access and session reporting now use authorization and role checks. Related Companion and admin text and tests are updated.

Changes

Agent access policy

Layer / File(s) Summary
Reject the retired setting
src/OpenClaw.Core/Models/GatewayConfig.cs, src/OpenClaw.Gateway/Bootstrap/GatewayBootstrapExtensions.cs, src/OpenClaw.Tests/GatewayBootstrapExtensionsTests.cs, docs/AUTHENTICATION.md, docs/zh-CN/AUTHENTICATION.md, CHANGELOG.md
The configuration model no longer exposes AllowViewerAgentExecution. Gateway startup rejects the key when present, including case variants and environment-variable configuration. Tests and authentication references describe the rejection and migration guidance.
Use role-based agent access
src/OpenClaw.Gateway/Endpoints/EndpointHelpers.cs, src/OpenClaw.Gateway/Endpoints/AdminEndpoints.Support.cs, src/OpenClaw.Gateway/SecurityPostureBuilder.cs, src/OpenClaw.Companion/ViewModels/MainWindowViewModel.cs, src/OpenClaw.Gateway/wwwroot/admin.html, src/OpenClaw.Tests/CompanionConnectionTests.cs, src/OpenClaw.Tests/GatewayAdminEndpointTests.cs, CHANGELOG.md
Agent execution now depends on authorization and role access. Session reporting, Companion and admin text, and tests reflect this behavior. The posture builder no longer adds the setting’s risk flag or recommendation.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: geffzhang

Merge Risk: ⚪ Minimal · up to d8d39

The change consistently removes viewer execution access and requires removal of the retired configuration key. No actionable issue remains; merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d8d39

The change tightens access without a demonstrated new attack path. Upgrades require coordinated configuration cleanup and role migration. Ongoing-session revocation and deployment-specific rollout behavior remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected authority spans the gateway's HTTP, WebSocket, A2A and MCP execution surfaces, including agent tools and live-provider usage. Removing the exception narrows viewer access to that authority; the inspected change does not grant additional authority to operators or administrators. Production tenant, credential and infrastructure scope was not established.

Trust Boundaries and Controls

  • observed — Inspected execution callers apply the authorization gate before agent dispatch. Both WebSocket paths may accept the handshake before rejecting a disallowed role, but reject before invoking the connection handler or live bridge. Default CanExecuteAgent callers do not request browser-session CSRF validation; MCP App tool calls explicitly do. This control split predates the PR and is not established as a new vulnerability.

Resilience and Maintainability Implications

  • observed — Inspected Apps and A2A execution paths retain per-session locking, write-access checks and authenticated identity stamping after authorization, with persistence in finally during cancellation or failure. OpenAI stable-session handling checks requester binding. These controls bound session interference, but do not establish WebSocket per-frame ownership or mid-session role revocation.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 7 files. (4 skipped: 4… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR meets the coding requirements in [#258]. It removes SecurityConfig.AllowViewerAgentExecution, the viewer authorization branch, the viewer_agent_execution_allowed posture flag, related tests…
Out of Scope Changes check ✅ Passed The changes stay within [#258]. The Companion update, /auth/session update, admin hint update, documentation changes, and migration regression tests support removal of the temporary setting and its …
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: removal of the temporary viewer agent execution bypass.
Full details: Docstring Coverage

Explanation

Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 7 files. (4 skipped: 4 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Telli Telli left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the bypass removal, the shared execution-role decision used by /auth/session and execution endpoints, stale-key startup handling, Companion behavior, documentation, and existing endpoint regression coverage. No additional code defect found.

Fresh validation on e5c84c6: restored and built the test project; 281 focused gateway authorization/security, bootstrap migration, endpoint authentication, MCP App proxy, and Companion tests passed with no failures or skips. Existing GitHub checks on this PR head are green.

The release hold remains applicable: the latest published release is v0.3.0 (2026-09-12), while #256 merged on 2026-09-28 and its commit is not contained in v0.3.0. Issue #258 and this PR explicitly require one release containing the migration switch before its removal. Keep this PR as a reviewed draft until that requirement is met unless the maintainer explicitly changes the release sequence. Its original author commit is unchanged.

@Telli
Telli marked this pull request as ready for review October 1, 2026 09:00
Copilot AI balanced review requested due to automatic review settings October 1, 2026 09:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The bypass is consistently removed across runtime behavior, configuration, tests, UI, and documentation.

Review effort: Balanced
Findings: None

What changed in this PR

Removes the retired viewer execution bypass, enforcing operator-only agent execution and failing startup when stale configuration remains.

Changes:

  • Removed the bypass and posture warning.
  • Added migration validation and regression tests.
  • Updated Companion, admin UI, changelog, and authentication guides.
  • NativeAOT/JIT compatibility is unaffected; no new dependencies or reflection were introduced.
File Description
src/​OpenClaw.Tests/​GatewayBootstrapExtensionsTests.cs Tests stale-setting rejection.
src/​OpenClaw.Tests/​GatewayAdminEndpointTests.cs Updates authorization and UI tests.
src/​OpenClaw.Tests/​CompanionConnectionTests.cs Tests operator execution reporting.
src/​OpenClaw.Gateway/​wwwroot/​admin.html Removes obsolete migration guidance.
src/​OpenClaw.Gateway/​SecurityPostureBuilder.cs Removes retired risk flag.
src/​OpenClaw.Gateway/​Endpoints/​EndpointHelpers.cs Enforces role-only execution authorization.
src/​OpenClaw.Gateway/​Endpoints/​AdminEndpoints.Support.cs Aligns session capability reporting.
src/​OpenClaw.Gateway/​Bootstrap/​GatewayBootstrapExtensions.cs Rejects the retired configuration key.
src/​OpenClaw.Core/​Models/​GatewayConfig.cs Removes the obsolete setting.
src/​OpenClaw.Companion/​ViewModels/​MainWindowViewModel.cs Updates connection guidance.
docs/​zh-CN/​AUTHENTICATION.md Documents migration in Chinese.
docs/​AUTHENTICATION.md Documents migration requirements.
CHANGELOG.md Records the breaking removal.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@Telli Telli left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maintainer has explicitly approved lifting the draft release hold and using an admin merge once validation passes. Current main was merged into this branch without conflicts, preserving the original security commit and contributor history.

Reviewed the combined head d8d3936. The Release solution build completed with zero warnings/errors; all 3,286 local tests passed, with 11 opt-in integration tests skipped. HelloAgent passed. Real Gateway startup rejected the retired environment setting for true, false, and malformed values with the expected migration guidance. The latest Copilot review reported no findings, and there are no unresolved review threads.

Fresh GitHub CI on this exact head is still running. Merge will retain the original commits through a normal merge after applicable checks pass.

@Telli
Telli merged commit 4be73ee into main Oct 1, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove temporary Security.AllowViewerAgentExecution after one release

2 participants