fix(security): remove temporary viewer agent execution bypass - #267
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 (13)
💤 Files with no reviewable changes (2)
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 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. ChangesAgent access policy
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to 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 ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Telli
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
Description
Remove the temporary
Security.AllowViewerAgentExecutionbypass. Authenticated viewer credentials can no longer run the agent, andGET /auth/sessionreports 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
operatorrole. This applies even when the value isfalse, empty, null, or malformed.Summary
Related Issues
Fixes #258.
Type of Change
Validation
Validated the branch after merging current
mainatd8d3936b744677c6442e9287340ead9dcdbacea1: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.true,false, or a malformed value, plus JSON and command-line keys set tofalse, with migration instructions.git diff --checkReview Notes
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
operatorrole 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