fix(security): audit open dm tool exposure by yu-xin-c · Pull Request #92883 · openclaw/openclaw

yu-xin-c · 2026-06-14T05:43:10Z

Summary

Extends the security exposure matrix from only groupPolicy="open" to all open inbound conversation policies that can expose tools:

channels.<id>.groupPolicy
channels.<id>.dmPolicy
legacy channels.<id>.dm.policy
the same policy paths under channels.<id>.accounts.<accountId>

The existing exposure check IDs are kept stable, but their title/detail/remediation text now covers open groups and DMs.

Real behavior proof

Behavior addressed: security audit warned critically for open groups with elevated/runtime/fs tools, but an equivalent open DM path only showed the lower-severity multi-user heuristic.

Real environment tested: local OpenClaw checkout on this branch, using the production collectExposureMatrixFindings implementation from src/security/audit-extra.sync.ts.

Exact steps or command run after this patch:

./node_modules/.bin/tsx --conditions=development - <<'TS'
import { collectExposureMatrixFindings } from "./src/security/audit-extra.sync.ts";

const cfg = {
  channels: {
    feishu: {
      groupPolicy: "disabled",
      dmPolicy: "open",
    },
  },
  tools: {
    elevated: {
      enabled: true,
      allowFrom: {
        feishu: ["ou_123"],
      },
    },
  },
};

for (const finding of collectExposureMatrixFindings(cfg)) {
  console.log(`${finding.severity.toUpperCase()} ${finding.checkId}`);
  console.log(finding.title);
  console.log(finding.detail);
  console.log(`Fix: ${finding.remediation}`);
  console.log("---");
}
TS

Evidence after fix:

CRITICAL security.exposure.open_groups_with_elevated
Open group/DM policy with elevated tools enabled
Found groupPolicy/dmPolicy="open" at:
- channels.feishu.dmPolicy
With tools.elevated enabled, a prompt injection in those conversations can become a high-impact incident.
Fix: Set groupPolicy/dmPolicy="allowlist" and keep elevated allowlists extremely tight.
---
CRITICAL security.exposure.open_groups_with_runtime_or_fs
Open group/DM policy with runtime/filesystem tools exposed
Found groupPolicy/dmPolicy="open" at:
- channels.feishu.dmPolicy
Risky tool exposure contexts:
- agents.defaults (sandbox=off; runtime=[exec, process]; fs=[read, write, edit, apply_patch]; fs.workspaceOnly=false)
Prompt injection in open conversations can trigger command/file actions in these contexts.
Fix: For open groups or DMs, prefer tools.profile="messaging" (or deny group:runtime/group:fs), set tools.fs.workspaceOnly=true, and use agents.defaults.sandbox.mode="all" for exposed agents.
---

Observed result after fix: dmPolicy="open" now produces both CRITICAL exposure findings, with the affected policy path shown as channels.feishu.dmPolicy.

What was not tested: full CLI rendering and provider-specific live Feishu delivery were not exercised; this patch is limited to the synchronous audit finding collector and regression tests around that collector.

Validation

node scripts/run-vitest.mjs src/security/audit-trust-model.test.ts
node scripts/run-vitest.mjs src/security/audit-extra.sync.test.ts
node scripts/run-vitest.mjs src/security/audit-exec-surface.test.ts src/security/audit-channel-dm-policy.test.ts
./node_modules/.bin/oxfmt --check --threads=1 src/security/audit-extra.sync.ts src/security/audit-trust-model.test.ts
node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/security/audit-extra.sync.ts src/security/audit-trust-model.test.ts
git diff --check -- src/security/audit-extra.sync.ts src/security/audit-trust-model.test.ts

clawsweeper · 2026-06-14T05:45:19Z

Codex review: needs changes before merge. Reviewed June 15, 2026, 2:48 PM ET / 18:48 UTC.

Summary
The PR updates the synchronous security audit exposure collector to treat open DM policies like open group policies and adds trust-model regression tests for open dmPolicy critical findings.

PR surface: Source +10, Tests +30. Total +40 across 2 files.

Reproducibility: yes. Source inspection shows current main returns from collectExposureMatrixFindings unless groupPolicy is open, while the PR body provides after-fix terminal output for dmPolicy="open".

Review metrics: 1 noteworthy metric.

Audit Output Contract: 2 existing check IDs keep IDs but change text. Maintainers should notice the stable-ID and user-visible-title/detail/remediation tradeoff before merging a security audit output change.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Update the public security audit docs to include DM and legacy DM policy coverage for the affected check IDs.
Maintainers should choose this PR as the canonical branch over the older overlapping open PR before landing.

Risk before merge

[P1] The PR keeps the existing check IDs but changes human-readable audit titles, details, and remediation for existing groupPolicy findings; maintainers should accept that compatibility tradeoff or require split wording before merge.
[P1] Public security-audit docs still describe the affected check IDs as group-only unless the PR is updated.
[P1] An older overlapping open PR remains in the same linked issue cluster, so maintainers should pick one canonical branch and close the other after one lands.

Maintainer options:

Accept Stable-ID Text Change (recommended)
Land the focused fix after docs are aligned, accepting that existing group-policy findings keep stable check IDs while their human-readable text now mentions DMs.
Preserve Group-Only Wording
If audit text compatibility matters, split the output wording so group-only configs retain the old phrasing while DM-inclusive configs use the new wording.
Defer To One Canonical PR
If maintainers prefer the older broader branch, pause this PR until that branch covers legacy dm.policy and becomes the chosen landing path.

Next step before merge

[P2] A narrow docs-only repair can align the public audit references; maintainers still need to accept or change the audit-output compatibility tradeoff before merge.

Security
Cleared: The diff broadens local security-audit detection and tests without adding execution, dependency, secret, permission, workflow, or supply-chain surface.

Review findings

[P3] Update audit docs for DM exposure coverage — src/security/audit-extra.sync.ts:1184-1188

Review details

Best possible solution:

Land one canonical focused fix that covers canonical and legacy channel/account DM policy paths, updates public audit docs, and either accepts or avoids the stable-check-ID wording change before closing the linked issue and older overlapping PR.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main returns from collectExposureMatrixFindings unless groupPolicy is open, while the PR body provides after-fix terminal output for dmPolicy="open".

Is this the best way to solve the issue?

Yes, mostly. Centralizing open inbound policy collection in the existing exposure matrix is the narrow maintainable fix; the branch should still align public docs and get maintainer acceptance of the audit-text compatibility tradeoff.

Full review comments:

[P3] Update audit docs for DM exposure coverage — src/security/audit-extra.sync.ts:1184-1188
The PR changes these check IDs from group-only to group/DM, but the public audit docs still say the exposure matrix only covers open groups. Without updating the docs, operators looking up the check IDs after merge will see the wrong scope.
Confidence: 0.84

Overall correctness: patch is correct
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c40db057da33.

Label changes

Label justifications:

P1: The PR addresses a security-audit false negative for public DM access with elevated/runtime/filesystem tool exposure.
merge-risk: 🚨 compatibility: The diff changes human-readable audit output for existing group-policy findings even though the check IDs remain stable.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal output from the production synchronous audit collector showing critical findings for channels.feishu.dmPolicy.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from the production synchronous audit collector showing critical findings for channels.feishu.dmPolicy.

Evidence reviewed

PR surface:

Source +10, Tests +30. Total +40 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	27	17	+10
Tests	1	30	0	+30
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	57	17	+40

Acceptance criteria:

[P1] pnpm docs:list.
[P1] git diff --check -- docs/cli/security.md docs/gateway/security/audit-checks.md.
[P1] pnpm docs:check-mdx.

What I checked:

Repository policy applied: Root AGENTS.md was read fully; no scoped AGENTS.md owns src/security, and docs/AGENTS.md plus the technical-documentation review overlay were read before assessing the public docs mismatch. (AGENTS.md:1, c40db057da33)
Current main misses the linked behavior: collectExposureMatrixFindings still calls listGroupPolicyOpen and returns when no open groupPolicy exists, so a config with only dmPolicy="open" does not reach the critical exposure checks on current main. (src/security/audit-extra.sync.ts:1162, c40db057da33)
Sibling audit path already treats DMs as multi-user: listPotentialMultiUserSignals already records canonical dmPolicy="open" and legacy dm.policy="open" at channel and account scope, supporting the PR's owner-boundary choice to extend the existing exposure matrix rather than add a separate rule. (src/security/audit-extra.sync.ts:423, c40db057da33)
Runtime entry point includes the collector: runSecurityAudit loads the non-deep audit facade and pushes collectExposureMatrixFindings before the multi-user heuristic, so this collector affects normal security audit output. (src/security/audit.ts:1228, c40db057da33)
PR covers canonical, legacy, and account-scoped paths: At the PR head, listOpenInboundPolicies checks groupPolicy, dmPolicy, and legacy dm.policy for both top-level channel config and account-scoped channel config. (src/security/audit-extra.sync.ts:385, 49a4872b9137)
PR adds focused regression coverage: The test diff adds open dmPolicy cases for elevated tools and runtime/filesystem exposure, asserting critical findings and the dmPolicy path in details. (src/security/audit-trust-model.test.ts:150, 49a4872b9137)

Likely related people:

steipete: Git history shows Peter Steinberger added the original open-group exposure audit, added the later multi-user DM/legacy DM trust-model audit, and recently documented security audit helpers. (role: feature-history owner and recent area contributor; confidence: high; commits: 049b8b14bc78, 4d124e4a9b75, 0e8c5fd85d20; files: src/security/audit-extra.sync.ts, src/security/audit-trust-model.test.ts, docs/cli/security.md)
coygeek: Recent GitHub commit history for src/security/audit-extra.sync.ts includes hooks-token and Gateway-auth audit hardening by Coy Geek on the same collector surface. (role: recent adjacent security-audit contributor; confidence: medium; commits: 3509f7613ec1, d51f26850d53; files: src/security/audit-extra.sync.ts)
shakkernerd: Recent history for src/security/audit-trust-model.test.ts includes trust-model assertion tightening on the same regression-test surface. (role: recent adjacent test contributor; confidence: medium; commits: ea1220016b7d; files: src/security/audit-trust-model.test.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

vincentkoc · 2026-06-16T06:38:36Z

Maintainer verification complete.

Focused security coverage: 4 files, 30 tests passed.
oxfmt, oxlint, MDX validation, and git diff --check passed.
Fresh maintainer autoreview: clean, confidence 0.94.
The repair applies canonical dmPolicy ?? dm.policy precedence independently at root and account scope, with canonical/legacy/conflict/nested-path coverage and path-neutral remediation.
Exact-head CI is green for the touched security/docs surface. The sole failing shard, checks-node-core-src-security, is unrelated and reproduces identically on current main: the same three assertions in src/skills/runtime/session-snapshot.test.ts at lines 131, 148, and 209.

No runtime config or migration behavior changed. No known proof gaps remain for the audit collector and documentation surface.

* fix(security): audit open dm tool exposure * fix(security): align open DM audit precedence --------- Co-authored-by: Vincent Koc <25068+vincentkoc@users.noreply.github.com>

yu-xin-c requested a review from a team as a code owner June 14, 2026 05:43

openclaw-barnacle Bot added size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 14, 2026

openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 14, 2026

vincentkoc self-assigned this Jun 16, 2026

yu-xin-c and others added 2 commits June 16, 2026 14:32

fix(security): audit open dm tool exposure

176d356

fix(security): align open DM audit precedence

fddafb7

vincentkoc force-pushed the codex/security-audit-open-dm-exposure branch from 49a4872 to fddafb7 Compare June 16, 2026 06:33

openclaw-barnacle Bot added docs Improvements or additions to documentation gateway Gateway runtime security Security documentation labels Jun 16, 2026

vincentkoc merged commit bbfea21 into openclaw:main Jun 16, 2026
186 of 189 checks passed

github-actions Bot mentioned this pull request Jun 16, 2026

📡 Upstream Digest — 2026-06-16 09:19 UTC curtismercier/openclaw-mods#1087

Open

clawsweeper Bot mentioned this pull request Jun 16, 2026

feat(infra): Add structured custom error handler via OPENCLAW_ERROR_HANDLER #93310

Closed

amtellezfernandez mentioned this pull request Jun 29, 2026

fix(security): align browser audit with plugin policy #97732

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(security): audit open dm tool exposure#92883

fix(security): audit open dm tool exposure#92883
vincentkoc merged 2 commits into
openclaw:mainfrom
yu-xin-c:codex/security-audit-open-dm-exposure

yu-xin-c commented Jun 14, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jun 14, 2026 •

edited

Loading

Uh oh!

vincentkoc commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

yu-xin-c commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Real behavior proof

Validation

Uh oh!

clawsweeper Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincentkoc commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yu-xin-c commented Jun 14, 2026 •

edited

Loading

clawsweeper Bot commented Jun 14, 2026 •

edited

Loading