Description

preserveQaGatewayDebugArtifacts() attempts to constrain preserveToDir to the repository by calling ensureRepoBoundDirectory(), but then performs destructive operations (readdir + rm) on the path after the check using only the returned pathname.

Because the filesystem can change between the validation and use (TOCTOU), an attacker with write access to the repo filesystem can:

• Provide a valid in-repo preserveToDir during ensureRepoBoundDirectory()
• Replace that directory with a symlink to an arbitrary location after validation but before clearQaGatewayArtifactDir()
• Cause clearQaGatewayArtifactDir() to iterate and rm -rf entries in the symlink target directory (outside the repo)

Vulnerable code:

const preserveToDir = params.repoRoot
  ? await ensureRepoBoundDirectory(params.repoRoot, params.preserveToDir, "QA gateway artifact directory", { mode: 0o700 })
  : params.preserveToDir;
await fs.mkdir(preserveToDir, { recursive: true, mode: 0o700 });
await clearQaGatewayArtifactDir(preserveToDir);

While this primarily affects local/CI threat models, it is still an arbitrary delete/write primitive if untrusted code can manipulate the workspace concurrently (e.g., malicious PR code, compromised dependency running tests, or a multi-tenant runner).

Recommendation

Avoid validating a path and then later using it by name for destructive operations.

Mitigations (prefer combining multiple):

1. Re-validate immediately before destructive use and refuse symlinks for the final directory itself:

import { constants as FS_CONST } from "node:fs";

async function assertSafeDir(dir: string, repoRoot: string) {
  ​// Ensure dir itself is not a symlink
  const st = await fs.lstat(dir);
  if (st.isSymbolicLink()) throw new Error("preserveToDir must not be a symlink");

  ​// Ensure realpath is within repo at time-of-use
  const repoReal = await fs.realpath(repoRoot);
  const dirReal = await fs.realpath(dir);
  const rel = path.relative(repoReal, dirReal);
  if (rel.startsWith("..") || path.isAbsolute(rel)) throw new Error("preserveToDir must stay within repo");
}

await assertSafeDir(preserveToDir, params.repoRoot);
await clearQaGatewayArtifactDir(preserveToDir);

2. Avoid following symlinks when opening/operating on the directory by using file-descriptor-based APIs (open with O_NOFOLLOW where supported and fd-relative operations). If full openat semantics aren’t feasible in Node, at least lstat + realpath immediately before each readdir/rm and abort if the directory’s realpath changes.

3. Consider writing artifacts into a fresh, uniquely named directory you create yourself under a trusted root (e.g., ${outputDir}/artifacts/...) rather than clearing an attacker-chosen directory.

Description

In stopChannel, if the channel task does not settle within CHANNEL_STOP_ABORT_TIMEOUT_MS (5s) after abort() is signaled, the function returns early without removing the task/abort handles from internal stores.

Impact:

• A user/admin initiated stop (or shutdown) may not actually stop the channel’s background task, so it can continue to process external events/messages.
• Because store.tasks/store.aborts are not cleared on timeout, subsequent operations can be stuck:
  • startChannelInternal will refuse to start a new task (if (store.tasks.has(id)) return;), leaving the channel in a permanent “running but cannot be restarted/stopped cleanly” state.
  • Repeated stopChannel calls will repeatedly hit the same timeout and return, causing an unbounded resource leak if the task holds sockets/timers, and preventing reliable shutdown.

Vulnerable code:

const stoppedCleanly = await waitForChannelStopGracefully(task, CHANNEL_STOP_ABORT_TIMEOUT_MS);
if (!stoppedCleanly) {
  setRuntime(channelId, id, { running: true, lastError: `channel stop timed out...` });
  return; // leaves store.aborts/store.tasks entries intact
}
store.aborts.delete(id);
store.tasks.delete(id);

This is a security-relevant availability/integrity issue when “stop” is expected to prevent further processing (e.g., disabling an integration) and/or when shutdown expects tasks to terminate.

Recommendation

Ensure stop operations leave the system in a consistent and enforceable state even when graceful shutdown times out.

Recommended fixes (choose one based on desired semantics):

1. Do not return early: clear bookkeeping entries and mark the runtime as stopping/unknown, while continuing to observe/log the task until it settles.

2. Track and expose a distinct stopping: true state, and prevent message handling in the plugin when in stopping state.

3. If a hard stop is required, implement a forceful termination mechanism appropriate for the runtime (e.g., closing underlying connections, canceling consumers), and/or treat timeout as an error that blocks shutdown.

Example (bookkeeping cleanup + explicit stopping state):

if (!stoppedCleanly) {
  store.aborts.delete(id);
  store.tasks.delete(id);
  setRuntime(channelId, id, {
    accountId: id,
    running: false,
    restartPending: false,
    lastError: `channel stop timed out after ${CHANNEL_STOP_ABORT_TIMEOUT_MS}ms`,
    ​// optionally add: stoppingFailed: true
  });
  return;
}

If the task can still run after cleanup, additionally attach a .catch/.finally handler to ensure it cannot become an unobserved resource leak, and ensure plugins check abort signals and stop processing promptly.

Description

The QA suite transport wait helper throws an Error containing the raw outbound message text (or the extracted failure text) when it detects a failure reply. That error is later captured and written into QA suite markdown reports as step/scenario details.

This can leak sensitive information into test artifacts (e.g., .artifacts/qa-e2e/.../report.md) when failure replies include provider error messages or other content that may contain secrets/tokens.

• assertNoFailureReplies() throws new Error(...) containing message text.
• runScenario() catches the error, converts it to text via formatErrorMessage(error), and stores it in the report details.

Vulnerable code:

throw new Error(extractQaFailureReplyText(failureMessage.text) ?? failureMessage.text);

Even though a log redaction helper exists (gateway-log-redaction.ts), it is not applied to failure reply text before it is persisted into reports.

Recommendation

Redact or avoid persisting raw message text in thrown errors and reports.

Options:

1. Redact failure text before throwing (recommended):

import { redactQaGatewayDebugText } from "./gateway-log-redaction.js";

const failureText = extractQaFailureReplyText(failureMessage.text) ?? failureMessage.text;
throw new Error(redactQaGatewayDebugText(failureText));

2. Store a stable, non-sensitive error code and keep the raw transcript only in protected runtime logs (or behind an explicit OPENCLAW_QA_DEBUG=1 gate).

3. If transcript text must be included, apply secret/token scrubbing (Bearer tokens, API keys, OAuth tokens, token= URL params, etc.) consistently for:

• failure reply text
• formatted transcripts (e.g. formatTransportTranscript)
• report rendering inputs

1. extensions/qa-lab/src/gateway-child.ts, line 885-892 (link)

   Extra buildStagedGatewayConfig invocation before the retry loop

   buildStagedGatewayConfig(1) is called here only to extract allowedPluginIds, but it also runs stageQaLiveAnthropicSetupToken and any caller-supplied mutateConfig as side effects — once before the loop, and then again on each retry attempt. The port 1 is never listened on, but the token-staging and mutateConfig callbacks may have observable side effects (writing credential files, transforming config). Consider extracting just the plugin-ID derivation into a helper that doesn't require a full staged config, or document that both callbacks must be idempotent.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: extensions/qa-lab/src/gateway-child.ts
   Line: 885-892
   
   Comment:
   **Extra `buildStagedGatewayConfig` invocation before the retry loop**
   
   `buildStagedGatewayConfig(1)` is called here only to extract `allowedPluginIds`, but it also runs `stageQaLiveAnthropicSetupToken` and any caller-supplied `mutateConfig` as side effects — once before the loop, and then again on each retry attempt. The port `1` is never listened on, but the token-staging and `mutateConfig` callbacks may have observable side effects (writing credential files, transforming config). Consider extracting just the plugin-ID derivation into a helper that doesn't require a full staged config, or document that both callbacks must be idempotent.
   
   How can I resolve this? If you propose a fix, please make it concise.