refactor(whatsapp): centralize account connection lifecycle by mcaxtr · Pull Request #65427 · openclaw/openclaw

mcaxtr · 2026-04-12T15:30:13Z

Summary

Problem: WhatsApp connection lifecycle ownership was split across auto-reply/monitor, inbound/monitor, active-listener, login, and login-qr, so socket state, reconnect state, timers, and login recovery could drift apart.
Why it matters: that split has been a recurring source of reconnect, QR login, watchdog, and outbound readiness regressions.
What changed: added one account-scoped connection controller that owns live socket lifecycle, reconnect/watchdog state, active-listener state, and shared login restart/logged-out recovery, then rewired the WhatsApp monitor/login paths around it.
What did NOT change (scope boundary): no broad WhatsApp parsing/routing cleanup, no public Plugin SDK surface changes, and no unrelated allowlist/group-policy behavior changes.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #
Related #
This PR fixes a bug or regression

Root Cause (if applicable)

Root cause: WhatsApp maintained multiple mutable lifecycle owners for the same account connection, so socket replacement, close normalization, active listener registration, watchdog timers, and login restart handling were not synchronized through a single state machine.
Missing detection / guardrail: there was no focused coverage around one authoritative per-account owner for reconnect, login restart, active-listener resolution, and debounced inbound shutdown behavior together.
Contributing context (if known): reconnect and login paths evolved in separate modules over time, and compatibility shims became de facto state owners.

Regression Test Plan (if applicable)

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file:
- extensions/whatsapp/src/login.test.ts
- extensions/whatsapp/src/login.coverage.test.ts
- extensions/whatsapp/src/login-qr.test.ts
- extensions/whatsapp/src/active-listener.test.ts
- extensions/whatsapp/src/monitor-inbox.streams-inbound-messages.test.ts
- extensions/whatsapp/src/auto-reply.web-auto-reply.connection-and-logging.e2e.test.ts
Scenario the test should lock in: one account-scoped owner preserves outbound readiness and reconnect behavior, shared 515 restart/logged-out recovery stays consistent between CLI and QR login, and compatibility listener resolution reflects controller state.
Why this is the smallest reliable guardrail: the regressions come from state drift across module seams, so seam/e2e coverage is more reliable than helper-local unit tests.
Existing test that already covers this (if any): the full WhatsApp slice already covers reconnect, watchdog, and monitor behavior; this PR extends the targeted assertions around the consolidated owner.
If no new test is added, why not: N/A

User-visible / Behavior Changes

None.

Diagram (if applicable)

Before:
[monitor/login/inbound/send] -> [separate socket refs + retry/timer/login state] -> [state drift]

After:
[monitor/login/send] -> [WhatsAppConnectionController per account] -> [shared socket lifecycle + listener state]

Security Impact (required)

New permissions/capabilities? (Yes/No) No
Secrets/tokens handling changed? (Yes/No) No
New/changed network calls? (Yes/No) No
Command/tool execution surface changed? (Yes/No) No
Data access scope changed? (Yes/No) No
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: Linux
Runtime/container: Node 24 / pnpm workspace
Model/provider: N/A
Integration/channel (if any): WhatsApp
Relevant config (redacted): default local test config + WhatsApp test harness config

Steps

Start the WhatsApp monitor/login flows against mocked sockets and reconnect events.
Force reconnect, watchdog, 515 restart, and logged-out paths.
Verify active listener resolution, send readiness, and monitor/login behavior after socket replacement.

Expected

One account-scoped owner keeps reconnect, outbound readiness, watchdog, and login recovery consistent.

Actual

Verified through targeted and full WhatsApp test coverage after consolidating lifecycle ownership.

Evidence

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

Human Verification (required)

Verified scenarios:
- full local WhatsApp test slice
- CLI login shared 515 restart path
- QR login shared 515 restart and logged-out recovery path
- active listener resolution against controller-backed state
- reconnect/watchdog coverage in WhatsApp e2e tests
Edge cases checked:
- default account vs explicit account listener resolution
- outbound send capability after socket replacement
- debounced inbound work flushing on close
What you did not verify:
- live WhatsApp setup against a real linked device

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

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

Risks and Mitigations

Risk: the new controller is now the central lifecycle owner, so subtle behavior changes could show up in less-common reconnect/login edges.
- Mitigation: preserved the old send-facing active-listener API as a compatibility facade and ran the full WhatsApp local test slice plus build after rebasing onto latest main.

aisle-research-bot · 2026-04-12T15:30:21Z

🔒 Aisle Security Analysis

We found 3 potential security issue(s) in this PR:

#	Severity	Title
1	🟠 High	Arbitrary directory deletion via unvalidated authDir in logoutWeb()
2	🟠 High	Global Symbol.for registry allows in-process hijacking of WhatsApp connection controller (listener interception)
3	🟡 Medium	CLI/log injection via unescaped WhatsApp accountId in formatted command hint

1. 🟠 Arbitrary directory deletion via unvalidated authDir in logoutWeb()

Property	Value
Severity	High
CWE	CWE-73
Location	`extensions/whatsapp/src/auth-store.ts:116-134`

Description

logoutWeb() deletes the entire authDir recursively when isLegacyAuthDir is false. The path is derived from params.authDir via resolveUserPath(), which allows absolute paths and .. segments (it essentially path.resolve()s the input).

With the new shared login flow (waitForWhatsAppLoginResult) automatically invoking logoutWeb() when Baileys reports a logged-out/401 state, any configuration/API surface that lets an attacker influence authDir can be escalated into arbitrary directory deletion on the host.

Key points:

Input: params.authDir (from account configuration / caller)
Dangerous action: fs.rm(resolvedAuthDir, { recursive: true, force: true })
Amplifier: remote-triggerable logged-out (401) path now clears credentials automatically, increasing the chance of the deletion path being hit.

Vulnerable code:

const resolvedAuthDir = resolveUserPath(params.authDir ?? resolveDefaultWebAuthDir());
...
await fs.rm(resolvedAuthDir, { recursive: true, force: true });

Recommendation

Constrain authDir to a trusted base directory and refuse to delete anything outside it.

Suggested approach:

Compute an expected base (e.g., path.join(resolveOAuthDir(), "whatsapp") or equivalent).
Resolve and normalize the candidate path.
Verify it is within the base using path.relative() (and reject empty///drive root).

Example:

import path from "node:path";

const base = path.resolve(path.join(resolveOAuthDir(), "whatsapp"));
const candidate = path.resolve(resolveUserPath(params.authDir ?? resolveDefaultWebAuthDir()));

const rel = path.relative(base, candidate);
if (rel.startsWith("..") || path.isAbsolute(rel) || rel === "") {
  throw new Error(`Refusing to delete authDir outside base: ${candidate}`);
}

await fs.rm(candidate, { recursive: true, force: true });

Additionally:

Consider only deleting known credential files rather than rm -r of the directory.
Log a warning and require an explicit user action/flag before destructive deletion on 401.

2. 🟠 Global Symbol.for registry allows in-process hijacking of WhatsApp connection controller (listener interception)

Property	Value
Severity	High
CWE	CWE-284
Location	`extensions/whatsapp/src/connection-controller-registry.ts:11-39`

Description

The WhatsApp extension stores connection controllers in a global Map on globalThis under a public, guessable Symbol.for(...) key. Any other code running in the same Node.js process (e.g., a third-party/untrusted plugin) can:

Access the registry state directly via globalThis[Symbol.for("openclaw.whatsapp.connectionControllerRegistry")]
Overwrite entries for an accountId by calling registerWhatsAppConnectionController(...) (or by directly mutating the map)
Cause outbound operations to resolve and use an attacker-controlled getActiveListener() implementation (see active-listener.ts / send.ts which call getRegisteredWhatsAppConnectionController(id)?.getActiveListener())

Impact:

Attacker can intercept/redirect outbound messages, polls, reactions, and presence/composing signals for a targeted accountId.
Creates a cross-module trust boundary break if the host supports loading untrusted code in-process.

Vulnerable code:

const CONNECTION_REGISTRY_KEY = Symbol.for("openclaw.whatsapp.connectionControllerRegistry");
...
getConnectionRegistryState().controllers.set(accountId, controller);

Recommendation

Avoid using a public global registry keyed by Symbol.for for security-sensitive routing.

Options (pick one appropriate to your architecture):

Capability-based registry: require a secret/capability token (generated by the trusted host) to register/unregister controllers.

// generated/stored only in trusted host code
const REGISTRY_CAPABILITY = Symbol("openclaw.whatsapp.registry.capability");

export function registerWhatsAppConnectionController(
  capability: symbol,
  accountId: string,
  controller: WhatsAppConnectionControllerHandle,
) {
  if (capability !== REGISTRY_CAPABILITY) throw new Error("unauthorized");
  getConnectionRegistryState().controllers.set(accountId, controller);
}

Don’t expose mutation APIs to untrusted modules: keep register/unregister in a host-private module that untrusted plugins cannot import.
If isolation is needed, run untrusted plugins out-of-process (or in a hardened VM context) so they cannot access globalThis of the host runtime.

Additionally:

Normalize accountId (e.g., trim/lowercase) consistently at registration and lookup to reduce collision/confusion risks.

3. 🟡 CLI/log injection via unescaped WhatsApp accountId in formatted command hint

Property	Value
Severity	Medium
CWE	CWE-74
Location	`extensions/whatsapp/src/send.ts:37-44`

Description

requireOutboundActiveWebListener() embeds resolvedAccountId directly into a terminal-facing command string.

Input source: options.accountId (passed into sendMessageWhatsApp / other outbound helpers) is only .trim()’d and can contain arbitrary characters.
Sink: the value is interpolated into an error message and a CLI hint generated via formatCliCommand(...).
formatCliCommand() does not quote or escape arguments; it only rewrites the CLI name and appends flags.

Impact:

An attacker who can influence accountId (e.g., via API caller, config injection, or untrusted integration code) can inject shell metacharacters or terminal control sequences into logs/errors.
This can enable copy/paste command injection (operator pastes the suggested command) or terminal escape injection (misleading output, log forging).

Vulnerable code:

throw new Error(
  `... link WhatsApp with: ${formatCliCommand(`openclaw channels login --channel whatsapp --account ${resolvedAccountId}`)}.`,
);

Recommendation

Constrain or safely render accountId before including it in terminal-facing strings.

Options:

Normalize/validate account IDs to a safe charset (recommended):

import { normalizeAccountId } from "openclaw/plugin-sdk/account-core";

const resolvedAccountId = normalizeAccountId(accountId ?? resolveDefaultWhatsAppAccountId(cfg));

Quote/escape the argument when constructing a shell command hint (if you must preserve the raw value). For example, add a helper that shell-quotes arguments and build the command from tokens:

const cmd = formatCliCommand(
  `openclaw channels login --channel whatsapp --account ${shellQuote(resolvedAccountId)}`,
);

Additionally, consider stripping ASCII control characters (e.g., \x1b) from values included in errors/logs to mitigate terminal escape injection.

Analyzed PR: #65427 at commit aee6b61

_{Last updated on: 2026-04-12T18:07:55Z}

greptile-apps · 2026-04-12T15:36:19Z

Greptile Summary

This PR centralizes WhatsApp account connection lifecycle into a new WhatsAppConnectionController per account, replacing previously scattered socket, reconnect, watchdog, active-listener, and login-recovery state spread across auto-reply/monitor, inbound/monitor, login, and login-qr. The registry uses Symbol.for-keyed global state for cross-module-instance stability, which is verified by the new runtime-plugin-boundary.whatsapp.test.ts boundary test. Prior review concerns (socket leak on waitForWaConnection failure, heartbeatSeconds dual-use) have been addressed or explicitly deferred in scope.

Confidence Score: 5/5

Safe to merge — no logic bugs found; all P0/P1 concerns from prior rounds are resolved.

The central lifecycle refactor is internally consistent: resolveCloseDecision is always called before closeCurrentConnection (so the stability-window reset reads a valid current), shutdown is idempotent across all call sites in the retry loop and finally block, the registry identity check in unregisterWhatsAppConnectionController prevents a racing replacement controller from being evicted, and openConnection only registers the controller after listener creation succeeds so a failed open preserves the prior registered controller. All remaining findings are P2 or lower.

No files require special attention.

_{Reviews (3): Last reviewed commit: "fix(whatsapp): isolate controller regist..." | Re-trigger Greptile}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 25ddce6133

ℹ️ 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".

mcaxtr · 2026-04-12T15:56:12Z

Follow-up on bot review after the latest push (5b6c969455):

Fixed the controller open-cleanup gap called out by Greptile. openConnection() now closes the socket if waitForWaConnection() fails before listener setup, and there is a focused regression test in extensions/whatsapp/src/connection-controller.test.ts.
Fixed the QR login background waiter path called out by Codex. attachLoginWaiter() now catches unexpected waitForWhatsAppLoginResult(...) failures and folds them into structured login error state, with coverage in extensions/whatsapp/src/login-qr.test.ts.
I did not broaden this PR to address the Aisle authDir deletion hardening or connection-open timeout suggestions. Both are reasonable follow-up hardening topics, but neither is a regression introduced by this lifecycle refactor, and I want to keep this PR scoped to concrete behavior introduced here.

@greptile review
@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5b6c969455

ℹ️ 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".

mcaxtr · 2026-04-12T16:29:25Z

Follow-up on the second review round after the latest push (7a776db033):

Fixed the controller takeover timing called out by Codex. A controller now enters the per-account registry only after openConnection() has a live listener, and there is a regression test in extensions/whatsapp/src/connection-controller.test.ts that proves a failed replacement controller does not hide an already-live controller.
Fixed the light-runtime dependency issue called out by Codex. The shared controller registry now lives in lightweight extensions/whatsapp/src/connection-controller-registry.ts, so extensions/whatsapp/src/active-listener.ts no longer imports the heavy connection/session stack.
Fixed the duplicated relink instruction called out by Greptile in extensions/whatsapp/src/login.ts; the logged-out CLI message now prints the rerun guidance only once.
I still did not broaden this PR to address the Aisle authDir deletion hardening suggestion. That looks like real hardening work in extensions/whatsapp/src/auth-store.ts, but it is not introduced by this lifecycle refactor and should be handled as a separate targeted change.

@greptile review
@codex review

chatgpt-codex-connector · 2026-04-12T16:38:14Z

Codex Review: Didn't find any major issues. Nice work!

ℹ️ 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".

…#65427) * refactor(whatsapp): centralize account connection lifecycle * fix(whatsapp): harden controller open failure cleanup * refactor(whatsapp): remove active listener fallback path * fix(whatsapp): isolate controller registry state * debug(whatsapp): trace typing presence updates * docs(changelog): add whatsapp lifecycle fix note * debug(whatsapp): log global presence mode * chore(whatsapp): remove debug presence logs --------- Co-authored-by: Vincent Koc <[email protected]>

openclaw-barnacle Bot added channel: whatsapp-web Channel integration: whatsapp-web size: XL maintainer Maintainer-authored PR labels Apr 12, 2026

greptile-apps Bot reviewed Apr 12, 2026

View reviewed changes

Comment thread extensions/whatsapp/src/connection-controller.ts Outdated

Comment thread extensions/whatsapp/src/connection-controller.ts

chatgpt-codex-connector Bot reviewed Apr 12, 2026

View reviewed changes

Comment thread extensions/whatsapp/src/login-qr.ts

chatgpt-codex-connector Bot reviewed Apr 12, 2026

View reviewed changes

Comment thread extensions/whatsapp/src/connection-controller.ts Outdated

Comment thread extensions/whatsapp/src/active-listener.ts Outdated

vincentkoc force-pushed the refactor/whatsapp-connection-controller branch from b2cdaf9 to 97669a7 Compare April 12, 2026 17:06

mcaxtr force-pushed the refactor/whatsapp-connection-controller branch from 47addbc to 7016cee Compare April 12, 2026 17:48

mcaxtr and others added 8 commits April 12, 2026 15:03

refactor(whatsapp): centralize account connection lifecycle

085b1cf

fix(whatsapp): harden controller open failure cleanup

68ba7ca

refactor(whatsapp): remove active listener fallback path

e9f90ec

fix(whatsapp): isolate controller registry state

4bfa926

debug(whatsapp): trace typing presence updates

d54ffbe

docs(changelog): add whatsapp lifecycle fix note

2feb166

debug(whatsapp): log global presence mode

be66d7d

chore(whatsapp): remove debug presence logs

aee6b61

mcaxtr force-pushed the refactor/whatsapp-connection-controller branch from 7016cee to aee6b61 Compare April 12, 2026 18:05

mcaxtr merged commit aa023e4 into main Apr 12, 2026
38 of 42 checks passed

mcaxtr deleted the refactor/whatsapp-connection-controller branch April 12, 2026 18:24

github-actions Bot mentioned this pull request Apr 12, 2026

📡 Upstream Digest — 2026-04-12 18:36 UTC curtismercier/openclaw-mods#552

Open

This was referenced Apr 20, 2026

fix: WhatsApp QR login failures in Control UI #43316

Closed

fix: add connection timeout and retry logic for WhatsApp Web QR generation #47503

Closed

fix(whatsapp): refresh rotated QR refs and preserve pair flow across 515 restart #48994

Closed

sukhdeepjohar mentioned this pull request Apr 21, 2026

WhatsApp Web listener does not survive session logout - gateway breaks permanently until manual restart #49305

Closed

mcaxtr mentioned this pull request Apr 25, 2026

fix(whatsapp): add reconnect safety timer to connection controller #65758

Closed

clawsweeper Bot mentioned this pull request May 3, 2026

feat(whatsapp): add poll, reaction, event, and RSVP message support #48570

Closed

11 tasks

This was referenced May 11, 2026

WhatsApp: add composing-aware inbound debounce extension #47542

Closed

[Bug]: unable to connect WhatsApp and Telegram accounts #56498

Closed

fix: add timeout to waitForWaConnection to prevent indefinite hangs #63037

Closed

This was referenced May 14, 2026

Improve WhatsApp/Baileys group message reliability #7433

Closed

WhatsApp: Backfill missed messages after reconnection #50093

Open

clawsweeper Bot mentioned this pull request May 24, 2026

WhatsApp pairing can stick at Logging in until gateway restart finalizes 515 recovery #85868

Closed

This was referenced Jul 2, 2026

fix(whatsapp): serialize overlapping Web sends #99050

Merged

fix(whatsapp): replace expired terminal QR on refresh #100159

Merged

Leighroyus mentioned this pull request Jul 22, 2026

WhatsApp auto-reply fails with 'No active WhatsApp Web listener' on 2026.7.2 — durable delivery path now affects auto-reply too (extends #52781) #110504

Closed

Uh oh!

Uh oh!

Conversation

mcaxtr commented Apr 12, 2026

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Root Cause (if applicable)

Regression Test Plan (if applicable)

User-visible / Behavior Changes

Diagram (if applicable)

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Risks and Mitigations

Uh oh!

aisle-research-bot Bot commented Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔒 Aisle Security Analysis

Description

Recommendation

Description

Recommendation

Description

Recommendation

Uh oh!

greptile-apps Bot commented Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

mcaxtr commented Apr 12, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

mcaxtr commented Apr 12, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aisle-research-bot Bot commented Apr 12, 2026 •

edited

Loading

greptile-apps Bot commented Apr 12, 2026 •

edited

Loading