fix(security): GHSA-2x4x-cc5g-qmmg add caller permissions validation to node.pair.approve#57809
fix(security): GHSA-2x4x-cc5g-qmmg add caller permissions validation to node.pair.approve#57809EthanHunter1229 wants to merge 22 commits into
Conversation
…ension The msteams extension depends on @microsoft/agents-hosting but this dependency was not included in the root package.json, causing runtime errors when the MS Teams channel starts. Fixes openclaw#44857
…tems Add memory check before generating shell completion cache during updates. Skip cache generation if free memory is below 512MB to prevent OOM errors on low-memory systems (issue openclaw#45065). Also increased Node.js memory limit for the completion generation process and added error handling for graceful failure.
Fix GHSA-qpjj-47vm-64pj: Missing authentication for local browser-control endpoints. Previously, when gateway.auth.mode was set to 'none' or 'trusted-proxy', the browser control server would start without authentication, allowing unauthorized local callers to invoke privileged browser operations. This fix removes the bypass for 'none' and 'trusted-proxy' modes, ensuring browser control always requires authentication regardless of gateway auth mode. CVSS: 7.5 (High) CWE: CWE-306 Missing Authentication for Critical Function
…urce Resolves openclaw#46494 - health command was showing running=false and tokenSource=none for Telegram even when channels status showed running=true and tokenSource=config. The health command was building channel status from config only, not querying the gateway for runtime status. Now it calls channels.status from gateway and merges runtime fields.
- Add runtime validation for gateway response structure - Extract getGatewayChannelData helper to avoid code duplication - Use proper TypeScript types instead of unsafe as assertions - Use const assertion for key arrays for better type safety
Resolves openclaw#46369 - cron.remove now cleans up: 1. Session record in sessions.json (by sessionKey or jobId) 2. Run history file in ~/.openclaw/cron/runs/ The fix adds cleanup logic in the gateway's cron.remove method to delete associated session records and history files when a cron job is removed.
Resolves openclaw#46411 - console.log with PATH was printed on every node start. Now only shown when OPENCLAW_DEBUG or VERBOSE is set.
Resolves openclaw#46520 - Origin validation now accepts custom URL schemes like tauri://, electron://, capacitor:// when listed in allowedOrigins. The parseOrigin function now handles non-standard schemes that fail new URL() parsing.
- fix(origin-check): support custom URL schemes (tauri://, electron://) - fix(node-host): hide PATH debug log by default - fix(cron): clean up session records when removing cron jobs - refactor(health): improve type safety and reduce code duplication - fix(health): use gateway channels.status for accurate running/tokenSource (60 files changed, +1269/-748)
…models When a user selects a model without explicit provider prefix (e.g., "claude-sonnet-4-6" instead of "anthropic/claude-sonnet-4-6"), the system now tries to infer the correct provider from configured models instead of defaulting to the current session's provider. This fixes the bug where switching models via UI incorrectly prepended the current session's provider prefix, causing: GatewayRequestError: model not allowed: zai-glm5/claude-sonnet-4-6 The fix uses inferUniqueProviderFromConfiguredModels() to find the correct provider when the model string doesn't include a "/". Fixes: openclaw#48224
The return statement after the try-catch block was unreachable and would cause a TypeScript error (result not in scope).
Cherry-pick from upstream commit 8eac939 This fix enforces configWrites against both the originating account and the targeted account scope for /config and config-backed /allowlist edits, blocking sibling-account mutations while preserving gateway operator.admin flows. Fixes GHSA-8jhh-jcqg-mj5p (High severity) - Channel commands could bypass account-scoped configWrites restrictions - In affected versions, channel-initiated config mutations were authorized against the originating account's configWrites policy but did not consistently re-check the targeted account scope - An authorized sender on one account could mutate protected sibling-account configuration when the target account had configWrites: false Thanks @tdjackey for reporting and @steipete for the fix.
…TY.md writes Fixes GHSA-7xr2-q9vf-x4r5 (CVE-2026-32013 variant) The patch for CVE-2026-32013 introduced symlink resolution for agents.files.get/set, but agents.create and agents.update still used raw fs.appendFile on IDENTITY.md without symlink containment checks. Attack: Attacker plants symlink workspace/IDENTITY.md -> /etc/crontab, then agents.create appends attacker-controlled content to arbitrary files. Fix: Add resolveIdentityPathForWrite() helper that uses resolveAgentWorkspaceFilePath() to validate the path and block symlink escape via assertNoPathAliasEscape().
Fixes GHSA-cg6c-q2hx-69h7 (Plivo V2 verified replay identity drifts on query-only variants) The Plivo V2 replay key was generated using the full verificationUrl (including query string), while V2 signature validation only uses baseUrl (without query). This caused GET requests (with query-only payloads) to have inconsistent replay protection between V2 and V3. V3 correctly uses the full URL (with query string) for both signature and replay validation. The fix aligns V2 replay key generation to match V2 signature validation by using baseUrl. This ensures consistent replay protection for all Plivo webhook variants (GET/POST, V2/V3). Co-authored-by: OpenClaw Security Team <security@openclaw.ai>
Resolves GHSA-8gc5-j5rx-235r (fast-xml-parser numeric entity expansion bypass) See: GHSA-8gc5-j5rx-235r
Resolves 4 High severity vulnerabilities: - GHSA-f269-vfmq-vjvj: Malicious WebSocket 64-bit length overflows - GHSA-9fhg-24mf-j994: Unbounded Memory Consumption in WebSocket - GHSA-2g65-4v4v-pjh7: Unhandled Exception in WebSocket Client - Additional WebSocket parsing vulnerabilities Co-ordinated via npm audit findings.
…ilege runtime scopes Cherry-picked from upstream fix commit ec2dbcf. Fixes GHSA-qm2m-28pf-hgjw: Gateway Plugin HTTP auth previously created a runtime scope set that included operator.admin regardless of caller-granted scopes. This fix ensures plugin HTTP handlers only receive the least-privilege WRITE_SCOPE, preserving caller scope boundaries.
…imal scopes Previously, dispatchGatewayMethod used a synthetic operator client with elevated operator.admin scope as a fallback when no request scope was available. This allowed plugins with limited scopes to escalate privileges and perform admin-only operations like sessions.delete. Fix by: - Creating a limited fallback client with only operator.read/write scopes - Storing the fallback client in fallbackGatewayContextState - Using the limited fallback instead of synthetic admin client This prevents privilege escalation when plugins call privileged methods without an explicit request scope. Fixes: GHSA-h4jx-hjr3-fhgc
…to node.pair.approve - Add validateCallerPermissions() function to check caller has required permissions - Update approveNodePairing to accept callerPermissions parameter - Validate that caller has every permission the node requests before approval - Log warning when callerPermissions not provided (backward compatibility) This fixes the security vulnerability where low-privilege operators could approve malicious nodes requesting elevated permissions (system.run, etc.) without having those permissions themselves.
openclaw-barnacle
Bot
added
channel: voice-call
Greptile SummaryThis PR attempts to fix two privilege-escalation vulnerabilities: GHSA-2x4x-cc5g-qmmg (
Confidence Score: 1/5Not safe to merge — the primary security fix for GHSA-2x4x-cc5g-qmmg is a no-op and the vulnerability remains exploitable. Two independent P0-level issues mean the stated security goal is not achieved: the call site never forwards caller permissions to the new validation function, and the validation function itself only checks
|
| Filename | Overview |
|---|---|
| src/infra/node-pairing.ts | Adds validateCallerPermissions and updates approveNodePairing signature, but the validation is bypassed in two ways: the call site never passes callerPermissions, and the check only covers pending.permissions while nodes typically declare capabilities via pending.commands. |
| src/gateway/server-methods/nodes.ts | The node.pair.approve handler calls approveNodePairing(requestId) without the new callerPermissions argument, making the entire GHSA-2x4x-cc5g-qmmg fix a no-op at the only production call site. |
| src/gateway/server-plugins.ts | Correctly replaces the synthetic operator.admin fallback client with a minimal-scope createFallbackOperatorClient() that only carries operator.read / operator.write, fixing GHSA-h4jx-hjr3-fhgc. |
| src/gateway/server/plugins-http.ts | Simplifies createPluginRouteRuntimeClient to always return [WRITE_SCOPE] regardless of gatewayAuthSatisfied, removing the path that previously granted operator.admin to gateway-authenticated plugin routes. |
| src/infra/node-pairing.test.ts | Existing tests still call approveNodePairing without callerPermissions; the new permission validation path is never tested, and no negative test (rejection case) is added. |
| 2026-03-30-security-ghsa.md | Documents GHSA-h4jx-hjr3-fhgc (plugin subagent fallback privilege escalation) but is filed under the PR for GHSA-2x4x-cc5g-qmmg — advisory IDs are inconsistently tracked across the PR. |
Comments Outside Diff (2)
-
src/gateway/server-methods/nodes.ts, line 520 (link)Security fix is a no-op —
callerPermissionsnever passedThe entire GHSA-2x4x-cc5g-qmmg fix depends on
approveNodePairingreceiving acallerPermissionsargument, but the call site here passes onlyrequestId. BecausecallerPermissionsdefaults toundefined,validateCallerPermissionsalways hits the backward-compatibility branch and returnstruewith a console warning — the permission check is never enforced. A low-privilege operator can still approve a node requestingsystem.runor any other elevated permission.The
contextobject is available in this handler and presumably carries the caller's scopes. The fix should extract the caller's permissions fromcontextand forward them:const approved = await approveNodePairing(requestId, undefined, context.callerPermissions);
Until this call site is updated, the vulnerability remains exploitable.
Prompt To Fix With AI
This is a comment left during a code review. Path: src/gateway/server-methods/nodes.ts Line: 520 Comment: **Security fix is a no-op — `callerPermissions` never passed** The entire GHSA-2x4x-cc5g-qmmg fix depends on `approveNodePairing` receiving a `callerPermissions` argument, but the call site here passes only `requestId`. Because `callerPermissions` defaults to `undefined`, `validateCallerPermissions` always hits the backward-compatibility branch and returns `true` with a console warning — the permission check is **never enforced**. A low-privilege operator can still approve a node requesting `system.run` or any other elevated permission. The `context` object is available in this handler and presumably carries the caller's scopes. The fix should extract the caller's permissions from `context` and forward them: ```typescript const approved = await approveNodePairing(requestId, undefined, context.callerPermissions); ``` Until this call site is updated, the vulnerability remains exploitable. How can I resolve this? If you propose a fix, please make it concise.
-
src/infra/node-pairing.test.ts, line 21 (link)No test exercises the new permission enforcement path
setupPairedNodeapproves a node requestingcommands: ["system.run"]without passingcallerPermissions, so the entire new validation path is never exercised in the test suite. The test will still pass because the backward-compatibility branch silently allows the approval.Tests should be added to cover at least:
- Rejecting approval when
callerPermissionsis provided but missing the required permission - Allowing approval when
callerPermissionscontains all required permissions - Handling nodes that request permissions via
commands(once that path is validated)
Prompt To Fix With AI
This is a comment left during a code review. Path: src/infra/node-pairing.test.ts Line: 21 Comment: **No test exercises the new permission enforcement path** `setupPairedNode` approves a node requesting `commands: ["system.run"]` without passing `callerPermissions`, so the entire new validation path is never exercised in the test suite. The test will still pass because the backward-compatibility branch silently allows the approval. Tests should be added to cover at least: - Rejecting approval when `callerPermissions` is provided but missing the required permission - Allowing approval when `callerPermissions` contains all required permissions - Handling nodes that request permissions via `commands` (once that path is validated) How can I resolve this? If you propose a fix, please make it concise.
- Rejecting approval when
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/nodes.ts
Line: 520
Comment:
**Security fix is a no-op — `callerPermissions` never passed**
The entire GHSA-2x4x-cc5g-qmmg fix depends on `approveNodePairing` receiving a `callerPermissions` argument, but the call site here passes only `requestId`. Because `callerPermissions` defaults to `undefined`, `validateCallerPermissions` always hits the backward-compatibility branch and returns `true` with a console warning — the permission check is **never enforced**. A low-privilege operator can still approve a node requesting `system.run` or any other elevated permission.
The `context` object is available in this handler and presumably carries the caller's scopes. The fix should extract the caller's permissions from `context` and forward them:
```typescript
const approved = await approveNodePairing(requestId, undefined, context.callerPermissions);
```
Until this call site is updated, the vulnerability remains exploitable.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/node-pairing.ts
Line: 156-162
Comment:
**`commands` array is not validated — only `permissions` record is checked**
`NodePairingNodeMetadata` has two separate fields for expressing what a node can do: `commands?: string[]` and `permissions?: Record<string, boolean>`. The new validation only checks `pending.permissions`. The existing test (and, in practice, most node pairing requests) uses `commands: ["system.run"]` — not `permissions` — so `pending.permissions` will be `undefined` and the guard at line 156 returns `true` immediately, allowing the approval regardless of caller privileges.
A malicious node only needs to omit the `permissions` field (and use `commands` instead) to bypass this check entirely. The validation should also cover `pending.commands`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: 2026-03-30-security-ghsa.md
Line: 7-13
Comment:
**GHSA identifier mismatch between advisory file and PR**
The PR title, description, and all code comments reference **GHSA-2x4x-cc5g-qmmg** (the `node.pair.approve` missing callerScopes validation), but this advisory document documents **GHSA-h4jx-hjr3-fhgc** (Plugin Subagent Fallback `deleteSession` Uses Synthetic `operator.admin`). These are two distinct, separately tracked vulnerabilities.
The `server-plugins.ts` / `plugins-http.ts` changes do correctly fix GHSA-h4jx-hjr3-fhgc, but this file should reflect that advisory ID rather than mixing the tracking identifiers. The two vulnerabilities should have separate advisory documents.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/node-pairing.test.ts
Line: 21
Comment:
**No test exercises the new permission enforcement path**
`setupPairedNode` approves a node requesting `commands: ["system.run"]` without passing `callerPermissions`, so the entire new validation path is never exercised in the test suite. The test will still pass because the backward-compatibility branch silently allows the approval.
Tests should be added to cover at least:
- Rejecting approval when `callerPermissions` is provided but missing the required permission
- Allowing approval when `callerPermissions` contains all required permissions
- Handling nodes that request permissions via `commands` (once that path is validated)
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(security): GHSA-2x4x-cc5g-qmmg add c..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!nodePermissions || Object.keys(nodePermissions).length === 0) { | ||
| return true; // No permissions required, always allowed | ||
| } | ||
| if (!callerPermissions) { | ||
| // GHSA-2x4x-cc5g-qmmg: No caller permissions provided - log warning but allow for backward compatibility | ||
| log?.("warn: node.pair.approve called without callerPermissions - scope validation skipped"); | ||
| return true; // Skip validation for backward compatibility |
There was a problem hiding this comment.
commands array is not validated — only permissions record is checked
NodePairingNodeMetadata has two separate fields for expressing what a node can do: commands?: string[] and permissions?: Record<string, boolean>. The new validation only checks pending.permissions. The existing test (and, in practice, most node pairing requests) uses commands: ["system.run"] — not permissions — so pending.permissions will be undefined and the guard at line 156 returns true immediately, allowing the approval regardless of caller privileges.
A malicious node only needs to omit the permissions field (and use commands instead) to bypass this check entirely. The validation should also cover pending.commands.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/node-pairing.ts
Line: 156-162
Comment:
**`commands` array is not validated — only `permissions` record is checked**
`NodePairingNodeMetadata` has two separate fields for expressing what a node can do: `commands?: string[]` and `permissions?: Record<string, boolean>`. The new validation only checks `pending.permissions`. The existing test (and, in practice, most node pairing requests) uses `commands: ["system.run"]` — not `permissions` — so `pending.permissions` will be `undefined` and the guard at line 156 returns `true` immediately, allowing the approval regardless of caller privileges.
A malicious node only needs to omit the `permissions` field (and use `commands` instead) to bypass this check entirely. The validation should also cover `pending.commands`.
How can I resolve this? If you propose a fix, please make it concise.| **GHSA:** GHSA-h4jx-hjr3-fhgc | ||
|
|
||
| ## Vulnerability Details | ||
|
|
||
| ### Summary | ||
| OpenClaw Gateway Plugin Subagent Fallback `deleteSession` Uses Synthetic `operator.admin` | ||
|
|
There was a problem hiding this comment.
The PR title, description, and all code comments reference GHSA-2x4x-cc5g-qmmg (the node.pair.approve missing callerScopes validation), but this advisory document documents GHSA-h4jx-hjr3-fhgc (Plugin Subagent Fallback deleteSession Uses Synthetic operator.admin). These are two distinct, separately tracked vulnerabilities.
The server-plugins.ts / plugins-http.ts changes do correctly fix GHSA-h4jx-hjr3-fhgc, but this file should reflect that advisory ID rather than mixing the tracking identifiers. The two vulnerabilities should have separate advisory documents.
Prompt To Fix With AI
This is a comment left during a code review.
Path: 2026-03-30-security-ghsa.md
Line: 7-13
Comment:
**GHSA identifier mismatch between advisory file and PR**
The PR title, description, and all code comments reference **GHSA-2x4x-cc5g-qmmg** (the `node.pair.approve` missing callerScopes validation), but this advisory document documents **GHSA-h4jx-hjr3-fhgc** (Plugin Subagent Fallback `deleteSession` Uses Synthetic `operator.admin`). These are two distinct, separately tracked vulnerabilities.
The `server-plugins.ts` / `plugins-http.ts` changes do correctly fix GHSA-h4jx-hjr3-fhgc, but this file should reflect that advisory ID rather than mixing the tracking identifiers. The two vulnerabilities should have separate advisory documents.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dc1c3f43c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!callerPermissions) { | ||
| // GHSA-2x4x-cc5g-qmmg: No caller permissions provided - log warning but allow for backward compatibility | ||
| log?.("warn: node.pair.approve called without callerPermissions - scope validation skipped"); | ||
| return true; // Skip validation for backward compatibility |
There was a problem hiding this comment.
validateCallerPermissions currently returns success when callerPermissions is absent, which nullifies the new scope check in the default node.pair.approve path: the only gateway call site (src/gateway/server-methods/nodes.ts) still calls approveNodePairing(requestId) without a third argument, so this branch is taken and approval proceeds even when caller scopes do not cover requested node permissions. For a security fix, this should reject (or require scoped caller permissions) instead of allowing-by-default.
Useful? React with 👍 / 👎.
| if (unauthorized) { | ||
| return unauthorized; | ||
| } | ||
| const allowInternalReadOnlyShow = | ||
| configCommand.action === "show" && isInternalMessageChannel(params.command.channel); | ||
| const nonOwner = allowInternalReadOnlyShow ? null : rejectNonOwnerCommand(params, "/config"); | ||
| if (nonOwner) { | ||
| return nonOwner; | ||
| } | ||
| const disabled = requireCommandFlagEnabled(params.cfg, { |
There was a problem hiding this comment.
This handler now checks only isAuthorizedSender and no longer enforces owner status before /config operations. In setups where multiple non-owner senders are authorized (for example via command/channel allowlists), those users can now run /config set|unset and persist config changes to disk, which is a privilege expansion from prior behavior and contradicts owner-only expectations for sensitive command paths.
Useful? React with 👍 / 👎.
| const keyToDelete = Object.keys(store).find( | ||
| (k) => k.includes(sessionKey) || k.includes(jobId) | ||
| ); |
There was a problem hiding this comment.
The cleanup logic deletes the first session-store key that merely contains sessionKey or jobId, which can remove the wrong session when IDs overlap (for example, deleting job-1 can match a key for job-10). Because session-store keys are structured strings that often embed job IDs, this substring match can corrupt unrelated session state; cleanup should resolve and compare exact expected keys instead.
Useful? React with 👍 / 👎.
…ility Resolves GHSA-j3q9-mxjg-w52f and GHSA-37ch-88jc-xwx2 The path-to-regexp package versions <8.4.0 are vulnerable to Regular Expression Denial of Service via sequential optional groups. This fix adds a pnpm override to ensure all transitive dependencies use a patched version.
Cherry-picked from upstream fix commit 6a79324. Fixes GHSA-8rh7-6779-cjqq: CWD .env environment variable injection bypasses host-env policy and allows config takeover.
…bility Cherry-pick from upstream fix. Fixes GHSA-c2c7-rcm5-vvqj: Picomatch has a ReDoS vulnerability via extglob quantifiers. This affects both the jscpd>micromatch>picomatch and tsdown>picomatch dependency paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dba54dc903
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (unauthorized) { | ||
| return unauthorized; | ||
| } | ||
| const nonOwner = rejectNonOwnerCommand(params, "/debug"); | ||
| if (nonOwner) { | ||
| return nonOwner; | ||
| } | ||
| const disabled = requireCommandFlagEnabled(params.cfg, { |
There was a problem hiding this comment.
handleDebugCommand now stops after rejectUnauthorizedCommand, so any sender who is authorized by channel allowlists can execute /debug set|unset|reset. In the previous logic this path also enforced rejectNonOwnerCommand; removing it expands privileges to non-owners and lets them change process-wide runtime overrides without owner approval.
Useful? React with 👍 / 👎.
| const cronRunsDir = join(process.env.OPENCLAW_HOME || process.env.HOME || "~", ".openclaw", "cron", "runs"); | ||
| await rm(join(cronRunsDir, `${jobId}.jsonl`), { force: true }); |
There was a problem hiding this comment.
cron.remove now hardcodes run-log deletion to OPENCLAW_HOME/HOME/.openclaw/cron/runs, but run logs are written from the configured cron store location (resolveCronRunLogPath({ storePath, jobId }) in server-cron.ts). When cron.store or state-dir overrides are used, this cleanup targets the wrong directory, leaving real logs undeleted and risking deletion of an unrelated default-path log with the same job id.
Useful? React with 👍 / 👎.
|
Closing this as implemented after Codex review. Current What I checked:
So I’m closing this as already implemented rather than keeping a duplicate issue open. Review notes: reviewed against 7e49cc87f9e9; fix evidence: commit 7e49cc87f9e9. |
3 participants
Summary
Fixes GHSA-2x4x-cc5g-qmmg:
node.pair.approvemissing callerScopes validation allows low-privilege operator to approve malicious nodes.Changes
validateCallerPermissions()function to check caller has required permissionsapproveNodePairingto acceptcallerPermissionsparameterSecurity Impact
This fixes the vulnerability where low-privilege operators could approve malicious nodes requesting elevated permissions (
system.run,system.which, etc.) without having those permissions themselves.Testing