Skip to content

fix(outbound): reject reserved meta-strings as literal targets#91402

Closed
bladin wants to merge 2 commits into
openclaw:mainfrom
bladin:fix/91372-reserved-meta-string-target
Closed

fix(outbound): reject reserved meta-strings as literal targets#91402
bladin wants to merge 2 commits into
openclaw:mainfrom
bladin:fix/91372-reserved-meta-string-target

Conversation

@bladin

@bladin bladin commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What changed

Added a reserved meta-string check in the outbound target resolver (src/infra/outbound/targets-resolve-shared.ts) that rejects literal strings "current", "self", "this", and "me" as outbound targets.

Why

The gateway's session-target resolver previously accepted literal strings like "current" and looked them up as if they were chat handles. When no session named "current" exists, the resolver could fall back to a public Telegram channel named @current, causing message delivery to the wrong destination.

This happened in production incidents where internal target="current" calls (intended to mean "the session that's currently running") leaked outbound messages to a public third-party channel.

Real behavior proof

Before fix

Passing target: "current" to the outbound resolver would proceed through the normal target resolution path, potentially matching a public channel named @current.

After fix

Evidence type: Terminal output (test results)

Running the new test suite shows the reserved meta-string rejection working correctly:

 ✓ rejects reserved meta-string 'current' as literal target
 ✓ rejects reserved meta-string 'CURRENT (uppercase)' as literal target
 ✓ rejects reserved meta-string 'current with whitespace' as literal target
 ✓ rejects reserved meta-string 'self' as literal target
 ✓ rejects reserved meta-string 'this' as literal target
 ✓ rejects reserved meta-string 'me' as literal target

The error message returned is:

Resolver: reserved meta-string "current" cannot be a literal target. Use explicit { chatId, threadId } instead.

Test plan

  • Added unit tests for isReservedTargetMetaString() helper
  • Added unit tests for reserved meta-string error messages
  • Added integration tests in runResolveOutboundTargetCoreTests() for all reserved strings
  • All existing outbound target tests pass
  • TypeScript type check passes
  • Code formatting passes

Fixes #91372

@bladin
fix: minSecurity inverted security rank order
9db3273 
Fixes issue openclaw#91283 where minSecurity() had the security rank order backwards,
treating 'full' as the most restrictive instead of the least restrictive.

The order is now correct:
- full: 0 (least restrictive - allows all commands)
- allowlist: 1 (medium restriction - allows only whitelisted commands)
- deny: 2 (most restrictive - denies all commands)

This allows session overrides with security='full' to properly override
agent defaults with security='allowlist'.

Also fixes the test cases that were asserting the incorrect behavior.
@openclaw-barnacle openclaw-barnacle Bot added size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 8, 2026
@clawsweeper

clawsweeper Bot commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 8, 2026, 7:11 AM ET / 11:11 UTC.

Summary
The branch adds reserved outbound target meta-string helpers/tests and also changes minSecurity exec approval precedence and tests.

PR surface: Source +49, Tests +81. Total +130 across 6 files.

Reproducibility: yes. for the review blockers by source inspection: current message-action target resolution maps target to to and runs resolveChannelTarget directory matching before the PR's direct-send guard. I did not run a live Telegram reproduction in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Security policy helper semantics: 1 changed. minSecurity is a shared exec approval helper, so changing its ordering affects gateway/node command execution policy beyond the outbound target bug.
  • Resolver coverage for reserved strings: 1 guarded, 1 unguarded. The direct outbound helper is guarded, but the message-action resolveChannelTarget directory path still accepts the same reserved literal.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • Remove or split out the minSecurity change.
  • Move the reserved-string rejection before shared directory lookup and cover message-action sends.
  • [P1] Add redacted real Telegram or gateway proof showing target: "current" is rejected without delivery.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists test output and checkboxes only; it does not show an after-fix real gateway or Telegram run with private details redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A real Telegram proof can demonstrate that target: "current" now errors instead of resolving or sending to a public channel. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

telegram desktop proof: after the resolver fix, send message action with target "current" and verify it errors without delivering to @current.

Risk before merge

  • [P1] Merging as-is would weaken exec approval security precedence by allowing the host approvals file to broaden a requested deny or allowlist policy to full.
  • [P1] The reserved-string rejection only covers direct resolveOutboundTarget calls; message-action target/to values can still reach directory lookup before the new guard runs.
  • [P1] The PR body only provides unit/integration test output, not real Telegram or gateway behavior proof showing target: "current" is rejected without delivery.

Maintainer options:

  1. Repair both blockers before merge (recommended)
    Drop the minSecurity relaxation and move the reserved-string guard into the common target-resolution path that runs before directory lookup for message actions and direct sends.
  2. Split the exec-policy change out
    If maintainers want to revisit exec approval precedence, handle it in a separate security-focused PR with explicit upgrade and threat-model review.
  3. Require live transport proof after code repair
    After the resolver is fixed, verify a real Telegram or gateway flow where target: "current" errors without attempting delivery to a public channel.

Next step before merge

  • [P1] This needs contributor/maintainer repair and real behavior proof; automation should not take over because missing real setup proof remains a contributor merge gate.

Security
Needs attention: The PR introduces a concrete exec approval security regression by changing a shared security-composition helper to prefer less restrictive policy.

Review findings

  • [P1] Keep host security from being broadened — src/infra/exec-approvals.ts:1531
  • [P1] Reject reserved targets before directory lookup — src/infra/outbound/targets-resolve-shared.ts:76-79
Review details

Best possible solution:

Remove the exec-approval change, put the reserved meta-string rejection in the shared target-resolution path before directory lookup for both message actions and direct sends, then prove it with focused tests plus real Telegram/gateway evidence.

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

Yes for the review blockers by source inspection: current message-action target resolution maps target to to and runs resolveChannelTarget directory matching before the PR's direct-send guard. I did not run a live Telegram reproduction in this read-only review.

Is this the best way to solve the issue?

No. The outbound fix is in a plausible but incomplete layer, and the unrelated minSecurity change is the opposite of the existing exec approval contract; the best fix is a common target guard plus no exec-policy churn.

Full review comments:

  • [P1] Keep host security from being broadened — src/infra/exec-approvals.ts:1531
    This flips minSecurity so the least restrictive value wins. Callers use this helper to combine the requested exec policy with ~/.openclaw/exec-approvals.json, and resolveExecHostApprovalContext explicitly says the host file may tighten the requested contract but must not broaden it; with this change a host full value can override a requested deny/allowlist policy and weaken command execution controls.
    Confidence: 0.94
  • [P1] Reject reserved targets before directory lookup — src/infra/outbound/targets-resolve-shared.ts:76-79
    The new guard only runs in resolveOutboundTargetWithPlugin, but message-action sends normalize target into to and call resolveChannelTarget first. That async resolver still strips/query-matches directory entries for current, so the reported target: "current" path can still resolve a Telegram directory match before the direct-send guard ever sees it.
    Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 310d28f719b8.

Label changes

Label changes:

  • add P1: The item concerns wrong-channel outbound delivery and the current patch also introduces a high-impact exec security boundary regression.
  • add merge-risk: 🚨 message-delivery: The patch changes outbound routing but misses the message-action directory path that can still misroute reserved target strings.
  • add merge-risk: 🚨 security-boundary: The unrelated minSecurity change can broaden requested exec security, weakening command execution approval boundaries.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists test output and checkboxes only; it does not show an after-fix real gateway or Telegram run with private details redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • add mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The linked bug is visible Telegram delivery behavior, so a short Telegram proof after the resolver repair would materially help review.

Label justifications:

  • P1: The item concerns wrong-channel outbound delivery and the current patch also introduces a high-impact exec security boundary regression.
  • merge-risk: 🚨 message-delivery: The patch changes outbound routing but misses the message-action directory path that can still misroute reserved target strings.
  • merge-risk: 🚨 security-boundary: The unrelated minSecurity change can broaden requested exec security, weakening command execution approval boundaries.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists test output and checkboxes only; it does not show an after-fix real gateway or Telegram run with private details redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The linked bug is visible Telegram delivery behavior, so a short Telegram proof after the resolver repair would materially help review.
Evidence reviewed

PR surface:

Source +49, Tests +81. Total +130 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 4 51 2 +49
Tests 2 83 2 +81
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 6 134 4 +130

Security concerns:

  • [high] Exec approval policy can be broadened — src/infra/exec-approvals.ts:1531
    Changing minSecurity to rank full as the minimum lets a less restrictive host approval policy win over a stricter requested policy, contrary to the current host-security contract.
    Confidence: 0.94

What I checked:

  • PR diff includes unrelated exec security change: The PR patch changes minSecurity to return the less restrictive security value, even though the PR title/body describe outbound target routing. (src/infra/exec-approvals.ts:1531, 9db327316aca)
  • Current exec contract says host policy may tighten but not broaden: resolveExecHostApprovalContext combines requested policy and host policy with minSecurity, and the nearby comment states the host file must not silently broaden the caller's requested contract. (src/agents/bash-tools.exec-host-shared.ts:231, 310d28f719b8)
  • Policy reporting expects stricter security to win: The effective policy summary uses minSecurity and labels the changed result as stricter host security wins, so flipping the helper invalidates the current reporting contract too. (src/infra/exec-approvals-effective.ts:371, 310d28f719b8)
  • Message-action targets still resolve through directory lookup: runMessageAction resolves canonical message-action targets through resolveActionTarget, which calls resolveChannelTarget; that path strips/query-matches directory entries before any new reserved-string guard in the PR's direct target helper would run. (src/infra/outbound/message-action-runner.ts:1553, 310d28f719b8)
  • PR guard is limited to direct outbound target resolution: The proposed reserved-string check is inserted in resolveOutboundTargetWithPlugin, which protects direct sync resolution but not the async resolveChannelTarget directory resolver used earlier by message actions. (src/infra/outbound/targets-resolve-shared.ts:76, 652bad8e7b70)
  • Current Telegram target surface uses plugin normalization and target resolver metadata: Telegram exposes normalizeTarget, targetPrefixes, and targetResolver metadata, so the reserved-word fix needs to cover the shared target-resolution layer before Telegram directory matching. (extensions/telegram/src/channel.ts:784, 310d28f719b8)

Likely related people:

  • vincentkoc: Current blame and recent history on both outbound target resolution and exec approval helpers point to Vincent Koc's recent fix(outbound): materialize buffer-only sends and release-baseline work. (role: recent area contributor; confidence: high; commits: 6a0fdea90ad0, 2e08f0f4221f; files: src/infra/outbound/targets-resolve-shared.ts, src/infra/outbound/target-resolver.ts, src/infra/exec-approvals.ts)
  • Peter Steinberger: Git history shows earlier cross-context message resolver work and several exec approval/security refactors on the same decision surfaces. (role: adjacent owner; confidence: medium; commits: 46015a3dd8b1, 61718d2da5, 6afdf10266; files: src/infra/outbound/target-resolver.ts, src/infra/outbound/message-action-runner.ts, src/infra/exec-approvals.ts)
  • Gustavo Madeira Santana: Git history shows target-resolution heuristics and exec policy reporting work close to the affected resolver and approval-policy paths. (role: adjacent contributor; confidence: medium; commits: e93412b5ce, ba735d0158, d78512b09d1d; files: src/infra/outbound/target-resolver.ts, src/infra/exec-approvals-policy.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.

@bladin
fix(outbound): reject reserved meta-strings as literal targets
652bad8 
Gateway session-target resolver now rejects literal strings "current",
"self", "this", and "me" as outbound targets before they can leak as
chat handle lookups. Previously, a bare target="current" could match
a public channel named @current on Telegram, causing message delivery
to the wrong destination.

The fix adds a reserved-word check in resolveOutboundTargetWithPlugin
that returns a clear error message instead of falling through to
channel-specific normalization or directory lookups.

Fixes openclaw#91372
@bladin
bladin force-pushed the fix/91372-reserved-meta-string-target branch from 1d62490 to 652bad8 Compare June 8, 2026 11:01
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jun 8, 2026
@clawsweeper
clawsweeper Bot temporarily deployed to qa-live-shared June 8, 2026 11:13 Inactive
@bladin bladin closed this Jun 8, 2026
@bladin
bladin deleted the fix/91372-reserved-meta-string-target branch June 8, 2026 11:19
This was referenced Jun 17, 2026
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Gateway session resolver should reject literal target="current" strings

1 participant

@bladin