fix(reply): resolve active channel/account SecretRefs in reply runs#66796
Conversation
🔒 Aisle Security AnalysisWe found 3 potential security issue(s) in this PR:
1. 🟠 Global runtime config snapshot used for queued reply secret resolution (cross-tenant/config confusion)
Description
This creates a realistic cross-context information disclosure risk when:
Impact is increased by the newly added second-pass scoped secret resolution, which may resolve channel/account-scoped secrets (e.g., Vulnerable behavior:
Vulnerable code: export function resolveQueuedReplyRuntimeConfig(config: OpenClawConfig): OpenClawConfig {
return (
(typeof getRuntimeConfigSnapshot === "function" ? getRuntimeConfigSnapshot() : null) ?? config
);
}
// ... later used as base for secret resolution
const runtimeConfig = resolveQueuedReplyRuntimeConfig(config);
const { resolvedConfig } = await resolveCommandSecretRefsViaGateway({
config: runtimeConfig,
commandName: "reply",
targetIds: getAgentRuntimeCommandSecretTargetIds(),
});RecommendationAvoid using a process-global, mutable runtime snapshot as the implicit base for queued execution secret resolution. Recommended fixes (pick one):
Example (conceptual) approach using an explicit snapshot key: // when enqueuing
queueItem.runtimeConfigKey = { tenantId, configHash };
// when executing
const runtimeConfig = getRuntimeConfigSnapshot(queueItem.runtimeConfigKey) ?? queueItem.config;Also consider guarding the new scoped resolution so it only ever resolves within allowed tenant/account boundaries derived from the queued job (not from global state). 2. 🟠 allowedPaths not enforced on gateway secret assignments (scoped secret resolution bypass)
Description
This means that if the gateway returns assignments for additional secret paths (for example other Why this matters in the new change:
Vulnerable code: for (const assignment of parsed.assignments) {
...
setPathExistingStrict(resolvedConfig, pathSegments, assignment.value);
}No RecommendationEnforce For example: for (const assignment of parsed.assignments) {
const pathSegments = assignment.pathSegments.filter((s) => s.length > 0);
if (pathSegments.length === 0) continue;
const path = pathSegments.join(".");
if (params.allowedPaths && !params.allowedPaths.has(path)) {
continue; // or record a diagnostic / throw
}
setPathExistingStrict(resolvedConfig, pathSegments, assignment.value);
}Additionally, consider sending 3. 🟠 Channel-scoped secret resolution may disclose other accounts' secrets when accountId is missing (allowedPaths omitted)
Description
Impact:
Why it happens:
Vulnerable code: const scopedTargets = getScopedChannelsCommandSecretTargets({
config: baseResolvedConfig,
channel: scope.channel,
accountId: scope.accountId,
});
const scopedResolved = await resolveCommandSecretRefsViaGateway({
config: baseResolvedConfig,
commandName: "reply",
targetIds: scopedTargets.targetIds,
...(scopedTargets.allowedPaths ? { allowedPaths: scopedTargets.allowedPaths } : {}),
});RecommendationTreat Options:
if (!scope.channel || !scope.accountId) return baseResolvedConfig;
// if channel is set but accountId is not, allow only channel-level paths
if (channel && !normalizedAccountId) {
const allowedPaths = new Set<string>();
for (const target of discoverConfigSecretTargetsByIds(params.config, targetIds)) {
const [root, channelId, maybeAccounts] = target.pathSegments;
if (root === "channels" && channelId === channel && maybeAccounts !== "accounts") {
allowedPaths.add(target.path);
}
}
return { targetIds, allowedPaths };
}
allowedPaths: scopedTargets.allowedPaths ?? new Set(),Any of these ensures scoped resolution cannot expand to other accounts’ secrets when only a channel is known. Analyzed PR: #66796 at commit Last updated on: 2026-04-14T21:10:06Z |
Greptile SummaryThis PR fixes a bug where The fix adds a second, scoped secret resolution pass keyed on the active channel/account ( Confidence Score: 5/5Safe to merge — the fix is correctly scoped, backward-compatible, and fully covered by new regression tests. All changes are targeted and additive: the optional No files require special attention. Reviews (1): Last reviewed commit: "Reply: resolve active channel/account Se..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaa5b3d84c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| queued.run.config = await resolveQueuedReplyExecutionConfig(queued.run.config, { | ||
| originatingChannel: queued.originatingChannel, | ||
| messageProvider: queued.run.messageProvider, | ||
| originatingAccountId: queued.originatingAccountId, | ||
| agentAccountId: queued.run.agentAccountId, |
There was a problem hiding this comment.
Keep scoped secret resolution in queued followup execution
This new scoped call can return a freshly resolved config object, but in queued runs that value is immediately superseded by resolveQueuedReplyRuntimeConfig(...) when a runtime snapshot is present, so the channel/account SecretRefs resolved here are dropped before runEmbeddedPiAgent executes. In the common case where the snapshot has base runtime secrets resolved but channel secrets still as SecretRefs, queued followups will continue using unresolved channel config (the exact regression this commit aims to fix), so message-action discovery can still fail for channels like Discord.
Useful? React with 👍 / 👎.
aaa5b3d to
7f4f01a
Compare
|
Landed via temp rebase onto main.
Thanks @CONTRIB! |
|
Correction (resolved placeholders): Landed via temp rebase onto main. Thanks @joshavant! |
|
Verification note: the full
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f4f01a070
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| return async (queued: FollowupRun) => { | ||
| queued.run.config = await resolveQueuedReplyExecutionConfig(queued.run.config); | ||
| queued.run.config = await resolveQueuedReplyExecutionConfig(queued.run.config, { |
There was a problem hiding this comment.
Preserve scoped secret resolution in queued followup runs
resolveQueuedReplyExecutionConfig(...) now computes scoped channel/account secret resolution and assigns it to queued.run.config, but in the queued runner this value is still superseded immediately afterward by resolveQueuedReplyRuntimeConfig(...) when a runtime snapshot exists, so runEmbeddedPiAgent continues with snapshot config and can miss the newly resolved channel/account secrets (for example, Discord token SecretRefs during message-action discovery). Fresh evidence in this commit: the new scoped call is present here, and the subsequent snapshot re-read path remains unchanged, so queued followups still drop the scoped result.
Useful? React with 👍 / 👎.
…penclaw#66796) * Reply: resolve active channel/account SecretRefs in agent runs * tests(reply): assert queued config scope wiring * fix: document reply secret-scope regression coverage (openclaw#66796) (thanks @joshavant)
…penclaw#66796) * Reply: resolve active channel/account SecretRefs in agent runs * tests(reply): assert queued config scope wiring * fix: document reply secret-scope regression coverage (openclaw#66796) (thanks @joshavant)
…penclaw#66796) * Reply: resolve active channel/account SecretRefs in agent runs * tests(reply): assert queued config scope wiring * fix: document reply secret-scope regression coverage (openclaw#66796) (thanks @joshavant)
…penclaw#66796) * Reply: resolve active channel/account SecretRefs in agent runs * tests(reply): assert queued config scope wiring * fix: document reply secret-scope regression coverage (openclaw#66796) (thanks @joshavant)
…penclaw#66796) * Reply: resolve active channel/account SecretRefs in agent runs * tests(reply): assert queued config scope wiring * fix: document reply secret-scope regression coverage (openclaw#66796) (thanks @joshavant)
…penclaw#66796) * Reply: resolve active channel/account SecretRefs in agent runs * tests(reply): assert queued config scope wiring * fix: document reply secret-scope regression coverage (openclaw#66796) (thanks @joshavant)
…penclaw#66796) * Reply: resolve active channel/account SecretRefs in agent runs * tests(reply): assert queued config scope wiring * fix: document reply secret-scope regression coverage (openclaw#66796) (thanks @joshavant)
Summary
Describe the problem and fix in 2–5 bullets:
channels.*SecretRefs (for the active reply channel) could remain unresolved during agent tool discovery.[message-action-discovery]errors and degraded/incomplete turns even when channel status/probe looks healthy.originatingChannel>messageProvider,originatingAccountId>agentAccountId).Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
resolveQueuedReplyExecutionConfigonly resolvedgetAgentRuntimeCommandSecretTargetIds()targets. By default that excludeschannels.*, so active channel SecretRefs could remain raw in reply runs.Regression Test Plan (if applicable)
src/auto-reply/reply/agent-runner-utils.secret-resolution.test.tssrc/auto-reply/reply/agent-runner-direct-runtime-config.test.tssrc/auto-reply/reply/followup-runner.test.tsextensions/discord/src/token.test.tsalready asserts unresolved Discord SecretRefs throw in strict token resolution.User-visible / Behavior Changes
Diagram (if applicable)
Security Impact (required)
Yes/No)Yes/No)Yes/No)Yes/No)Yes/No)Yes, explain risk + mitigation:Repro + Verification
Environment
gpt-5.4(invalid API key used intentionally to end run quickly)channels.discord.tokenconfigured asenv:default:DISCORD_BOT_TOKENSecretRefSteps
resolveQueuedReplyExecutionConfig(...)scoped channel/account resolution first.Expected
Actual
[message-action-discovery] discord.actions.describeMessageTool failed: ... unresolved SecretRef "env:default:DISCORD_BOT_TOKEN"Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
resolveQueuedReplyExecutionConfigresolveQueuedReplyExecutionConfigallowedPathswhen account is knownmessageProvider/agentAccountIdwhen originating values are missingReview Conversations
Compatibility / Migration
Yes/No)Yes/No)Yes/No)Risks and Mitigations