Fix owner-only auth and overlapping skill env regressions by vincentkoc · Pull Request #38548 · openclaw/openclaw

vincentkoc · 2026-03-07T04:06:11Z

Summary

Problem: a recent auth change treated any identified direct-message sender as an owner when commands.ownerAllowFrom was unset, which widened access to ownerOnly tools.
Why it matters: shared or misconfigured messaging surfaces could expose owner-only command/tool paths beyond explicitly authorized owners.
What changed: owner resolution now only trusts explicit owner matches, wildcard owner config, or internal operator.admin sessions; overlapping skill env overrides are now reference-counted so ACP child-process env stripping remains active until all overlapping restores complete.
What did NOT change (scope boundary): default command authorization behavior and non-skill host env handling remain unchanged.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #
Related #

User-visible / Behavior Changes

Direct-message senders are no longer implicitly treated as owners when commands.ownerAllowFrom is unset.
Overlapping skill env overrides keep ACP secret stripping active until all overrides have been restored.

Security Impact (required)

New permissions/capabilities? (No)
Secrets/tokens handling changed? (Yes)
New/changed network calls? (No)
Command/tool execution surface changed? (Yes)
Data access scope changed? (No)
If any Yes, explain risk + mitigation:
This narrows owner-only tool eligibility back to explicit owner signals and hardens skill env cleanup so overlapping runs cannot prematurely stop ACP env stripping for injected credentials.

Repro + Verification

Environment

OS: macOS
Runtime/container: local checkout with pnpm
Model/provider: N/A
Integration/channel (if any): Discord auth path and ACP harness
Relevant config (redacted): default command auth config; skill entry with apiKey override

Steps

Run pnpm exec vitest run src/auto-reply/command-auth.owner-default.test.ts src/auto-reply/command-control.test.ts src/agents/skills.test.ts src/acp/client.test.ts.
Run pnpm check.
Confirm direct-message senders without ownerAllowFrom are not owners and overlapping env restores keep getActiveSkillEnvKeys() populated until the final restore.

Expected

Owner-only gating stays tied to explicit owner authorization.
ACP env stripping stays active across overlapping skill env overrides.

Actual

Verified after the fix.

Evidence

Attach at least one:

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios: focused Vitest coverage for auth ownership, command control, skills env overrides, and ACP client spawning; full pnpm check.
Edge cases checked: direct/group senders without owner allowlists, wildcard/internal admin owner resolution, overlapping env override restore ordering.
What you did not verify: full pnpm test suite on this branch.

Compatibility / Migration

Backward compatible? (Yes)
Config/env changes? (No)
Migration needed? (No)
If yes, exact upgrade steps:

Failure Recovery (if this breaks)

How to disable/revert this change quickly: revert the two commits on this branch.
Files/config to restore: src/auto-reply/command-auth.ts, src/agents/skills/env-overrides.ts, and their targeted tests.
Known bad symptoms reviewers should watch for: owner-only tools becoming unavailable for explicit owners, or skill env overrides not restoring to baseline after nested use.

Risks and Mitigations

Risk: overlapping overrides with different values still preserve the first active injected value for the lifetime of the overlap.
- Mitigation: current skill env injection semantics only apply when the env var is otherwise unset, and the added regression test locks the no-leak restore behavior that ACP depends on.

greptile-apps · 2026-03-07T04:09:31Z

Greptile Summary

This PR fixes two regressions: a security issue in owner authorization (where any identified direct-message sender was implicitly granted senderIsOwner when commands.ownerAllowFrom was unconfigured) and a reference-counting bug in skill env overrides (where the first of two overlapping applySkillEnvOverrides restore calls would prematurely strip env keys still needed by the second active override).

Changes:

src/auto-reply/command-auth.ts: Removes four lines that granted senderIsOwner = true for direct-chat senders when no owner allowlist was configured. Owner status is now only assigned via an explicit identity match against ownerAllowFrom, a wildcard ("*"), or an internal operator.admin scope. This is a deliberate behavior change that narrows the attack surface.
src/agents/skills/env-overrides.ts: Replaces the flat Set<string> tracking active skill env keys with a reference-counted Map<string, ActiveSkillEnvEntry>. New acquireActiveSkillEnvKey / releaseActiveSkillEnvKey helpers maintain a per-key count so overlapping applySkillEnvOverrides calls keep the key and ACP stripping active until the last caller restores.
Tests: Both changes have targeted regression tests; the auth test correctly asserts isAuthorizedSender remains true while senderIsOwner becomes false, confirming no over-correction on command access.

Minor observation: In acquireActiveSkillEnvKey, the baseline field inside ActiveSkillEnvEntry is always recorded as undefined (the branch that creates a new entry is only reached when process.env[key] === undefined). This makes the else restore branch in releaseActiveSkillEnvKey (line 68–70) unreachable in the current code. It is not a correctness issue — the delete path is always taken and is correct — but the field and its restore branch are dead code for now.

Confidence Score: 5/5

Safe to merge — both fixes are narrowly scoped, well-tested, and reduce attack surface without regressions in normal command authorization flow.
The auth fix is a one-line simplification that removes an implicit privilege escalation path; the env-overrides fix is a straightforward reference-counting refactor with a dedicated regression test. No external API surface, no config schema changes, and existing test coverage for all affected paths is updated.
No files require special attention, but reviewers should confirm their deployment has commands.ownerAllowFrom explicitly configured if owner-only tools are required in direct-message setups, since the implicit grant is now removed.

_{Last reviewed commit: bb085eb}

* main: (45 commits) chore: update dependencies except carbon test(memory): make mcporter EINVAL retry test deterministic refactor(extensions): reuse shared helper primitives refactor(core): extract shared dedup helpers fix(ci): re-enable detect-secrets on main docs: reorder 2026.3.7 changelog highlights chore: bump version to 2026.3.7 fix(android): align run command with app id docs: add changelog entry for Android package rename (openclaw#38712) fix(android): rename app package to ai.openclaw.app fix(gateway): stop stale-socket restarts before first event (openclaw#38643) fix(gateway): skip stale-socket restarts for Telegram polling (openclaw#38405) fix(gateway): invalidate bootstrap cache on session rollover (openclaw#38535) docs: update changelog for reply media delivery (openclaw#38572) fix: contain final reply media normalization failures fix: contain block reply media failures fix: normalize reply media paths Fix owner-only auth and overlapping skill env regressions (openclaw#38548) fix(feishu): disable block streaming to prevent silent reply drops (openclaw#38422) fix: suppress ACP NO_REPLY fragments in console output (openclaw#38436) ...

…8548)

…8548) (cherry picked from commit 15a5e39) Partial: only command-auth files — gutted skills files discarded

…8548)

vincentkoc added 2 commits March 6, 2026 23:04

fix(auth): restore explicit owner-only tool gating

da5bcea

fix(skills): keep overlapping env overrides tracked

9652c20

vincentkoc self-assigned this Mar 7, 2026

openclaw-barnacle Bot added agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels Mar 7, 2026

vincentkoc marked this pull request as ready for review March 7, 2026 04:06

Merge branch 'main' into vincentkoc-code/auth-skill-env-regressions

bb085eb

vincentkoc merged commit 15a5e39 into main Mar 7, 2026
29 of 30 checks passed

vincentkoc deleted the vincentkoc-code/auth-skill-env-regressions branch March 7, 2026 04:33

drvoss mentioned this pull request Mar 7, 2026

fix: deduplicate snapshot field reads and add test coverage #38620

Closed

14 tasks

alexyyyander mentioned this pull request Mar 7, 2026

fix/gateway token mismatch 38617 #38676

Closed

joshavant pushed a commit that referenced this pull request Mar 7, 2026

Fix owner-only auth and overlapping skill env regressions (#38548)

591d8ce

vincentkoc added a commit to BryanTegomoh/openclaw-upstream that referenced this pull request Mar 8, 2026

Fix owner-only auth and overlapping skill env regressions (openclaw#3…

32c1de9

…8548)

jenawant pushed a commit to jenawant/openclaw that referenced this pull request Mar 10, 2026

Fix owner-only auth and overlapping skill env regressions (openclaw#3…

77f9841

…8548)

V-Gutierrez pushed a commit to V-Gutierrez/openclaw-vendor that referenced this pull request Mar 17, 2026

Fix owner-only auth and overlapping skill env regressions (openclaw#3…

0d6f6ba

…8548)

alexey-pelykh mentioned this pull request Mar 20, 2026

Cherry-pick issue #842: compaction-config-venice-kilocode-auth-nodes remoteclaw/remoteclaw#1725

Merged

lovewanwan pushed a commit to lovewanwan/openclaw that referenced this pull request Apr 28, 2026

Fix owner-only auth and overlapping skill env regressions (openclaw#3…

e274a29

…8548)

ogt-redknie pushed a commit to ogt-redknie/OPENX that referenced this pull request May 2, 2026

Fix owner-only auth and overlapping skill env regressions (openclaw#3…

a0fa7ef

…8548)

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 9, 2026

Fix owner-only auth and overlapping skill env regressions (openclaw#3…

34639b2

…8548)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix owner-only auth and overlapping skill env regressions#38548

Fix owner-only auth and overlapping skill env regressions#38548
vincentkoc merged 3 commits intomainfrom
vincentkoc-code/auth-skill-env-regressions

vincentkoc commented Mar 7, 2026

Uh oh!

greptile-apps Bot commented Mar 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

vincentkoc commented Mar 7, 2026

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

User-visible / Behavior Changes

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Compatibility / Migration

Failure Recovery (if this breaks)

Risks and Mitigations

Uh oh!

greptile-apps Bot commented Mar 7, 2026

Greptile Summary

Confidence Score: 5/5

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant