Skip to content

fix(channel): scope webchat conversationId by channel id (#558)#561

Closed
ncw1992120 wants to merge 2 commits into
mateaix:devfrom
ncw1992120:fix/webchat-channel-session-isolation
Closed

fix(channel): scope webchat conversationId by channel id (#558)#561
ncw1992120 wants to merge 2 commits into
mateaix:devfrom
ncw1992120:fix/webchat-channel-session-isolation

Conversation

@ncw1992120

Copy link
Copy Markdown
Contributor

What

Fixes #558 — WebChat channel session isolation.

deriveConversationId identified a channel by the first 8 chars of its apiKey. All generated keys share the 11-char prefix mc_webchat_, so that slice was the constant mc_webch for every channel. Two channels with the same visitorId derived the same conversationId, collapsing their sessions together — channel isolation was entirely lost (sessions cross-leaked in listSessions, collided on the UNIQUE constraint, etc.).

Fix

Embed the stable channel DB id (channel.getId()) into the conversationId instead, and keep pre-fix sessions visible via read-time fallback:

  • deriveConversationId(Long channelId, ...) — new format webchat:<channelId>:<visitor>[:<session>]; hash-folding adapts to channelId length to stay within VARCHAR(64).
  • legacyConversationId(apiKey, ...) — preserves the old key8 derivation purely for resolving existing webchat:mc_webch:... rows. Never used to mint new ids.
  • resolveSessionConversationId(...) — prefers the current id, falls back to legacy if absent, else returns current (new sessions always get the new format).
  • loadVisitorSessions matches both the channel and legacy prefixes; recoverSessionId parses against both bases, so legacy sessions stay listable/readable.
  • Adds ConversationService.conversationExists for the resolver.

Already-collided legacy rows cannot be retroactively split and are left as-is. Pure code fix — no Flyway migration.

Tests

  • Migrated all deriveConversationId(API_KEY, ...) call sites to channelId.
  • New WebChatChannelIsolationTest — the core regression: two channels sharing an apiKey prefix + same visitor → each channel's listSessions returns only its own sessions.
  • New WebChatLegacySessionCompatTest — seeds a legacy webchat:mc_webch:... session → still listable and its messages still readable (fallback path).
  • Full WebChat* suite green (111 tests, 0 failures).

deriveConversationId identified a channel by the first 8 chars of its
apiKey, but all generated keys share the 11-char prefix "mc_webchat_",
so the slice was the constant "mc_webch" for every channel. Two channels
with the same visitorId derived the SAME conversationId, collapsing
their sessions together — channel isolation was entirely lost.

Embed the stable channel DB id (channel.getId()) into the conversationId
instead, and keep pre-fix sessions visible via read-time fallback:
- deriveConversationId now takes Long channelId; new format
  webchat:<channelId>:<visitor>[:<session>]
- legacyConversationId preserves the old key8 derivation for resolving
  existing webchat:mc_webch:... rows; resolveSessionConversationId
  prefers the current id and falls back to legacy
- loadVisitorSessions matches both the channel and legacy prefixes, and
  recoverSessionId parses against both bases so legacy sessions stay
  listable/readable. Already-collided rows cannot be retroactively split.

Collided legacy data is not migrated (pure code fix, no Flyway). Adds
ConversationService.conversationExists for the resolver.

Tests: migrates all call sites to channelId; adds WebChatChannelIsolation
(cross-channel collision regression) and WebChatLegacySessionCompat
(read-time fallback). Full WebChat* suite green (111 tests).
…ateaix#558)

The read-time fallback in the previous commit still leaked: the legacy
prefix webchat:mc_webch: is the same constant for every channel (all
generated keys share the 8-char slice), and username doesn't encode the
channel either, so channel A's listSessions matched channel B's legacy
sessions for the same visitor. Reproduced by an isolation test that
seeds two channels sharing an apiKey prefix.

Root fix: persist the owning channel on each webchat row.

- Flyway V171 (h2/mysql/kingbase): add channel_id BIGINT NULL to
  mate_conversation. No inline backfill — pre-fix rows can't be reliably
  reconstructed and are left NULL.
- ConversationEntity: add channelId field.
- getOrCreateWebchatConversation: new overload carries channelId, written
  on insert; chatStream and createSession now pass channel.getId().
- listWebchatConversations(username, channelId): DB filters channel_id =
  ? OR channel_id IS NULL (channel-scoped rows exactly, pre-fix rows
  still returned for in-memory legacy-prefix matching).
- loadVisitorSessions: channel_id-present rows are accepted as-is (fully
  isolated by the query); NULL rows fall through to the legacy-base
  startsWith, which keeps already-collided rows visible (they cannot be
  split) without leaking the new channel-scoped rows.

Limitation (documented + tested): pre-fix collided rows (channel_id NULL,
shared cid) remain visible across the channels that collided under the
old key8 derivation — this is unrecoverable dirty data, not a regression.
New sessions are strictly isolated.

Tests: WebChatChannelIsolationTest now seeds channel_id-persisted rows
and asserts channel B's session does NOT appear in channel A's listing
(the regression the prior commit failed), plus a shared-legacy row that
correctly stays visible to both. Full WebChat* suite green (114 tests).
@ncw1992120

Copy link
Copy Markdown
Contributor Author

Update — closed the legacy cross-channel leak found in adversarial review

The first commit's read-time fallback still leaked legacy sessions across channels: the legacy prefix webchat:mc_webch: is identical for every channel (all generated keys share the 8-char slice mc_webch), and username doesn't encode the channel either — so channel A's listSessions matched channel B's legacy sessions for the same visitor. Reproduced by a failing isolation test before the fix.

Root fix (commit 2, 9b76fff1): persist the owning channel on each webchat row.

  • Flyway V171 (h2/mysql/kingbase): channel_id BIGINT NULL on mate_conversation. No inline backfill — pre-fix rows can't be reliably reconstructed.
  • getOrCreateWebchatConversation carries channelId (written on insert); chatStream + createSession now pass channel.getId().
  • listWebchatConversations(username, channelId) filters channel_id = ? OR channel_id IS NULL at the DB — channel-scoped rows are isolated exactly.
  • loadVisitorSessions accepts channel_id-present rows directly; only NULL (pre-fix) rows fall through to the legacy-prefix in-memory match.

Documented limitation (tested): pre-fix collided rows (channel_id NULL, shared legacy cid) remain visible across the channels that collided — this is unrecoverable dirty data, not a regression. New sessions are strictly isolated.

WebChatChannelIsolationTest now seeds channel_id-persisted rows and asserts channel B's session does NOT appear in channel A's listing (the exact regression the first commit failed), plus a shared-legacy row that correctly stays visible to both.

Full WebChat* suite green: 114 tests, 0 failures.

@ncw1992120

Copy link
Copy Markdown
Contributor Author

Superseded by #562, which adopts the same approach as the merged IM-channel fix e294b325 (scope conversationId by channel.getId(), no channel_id column). This PR's channel_id + read-time-fallback approach introduced a cross-channel leak in the legacy-prefix path; #562 avoids it entirely by following e294b325's posture (no legacy fallback).

@ncw1992120 ncw1992120 closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant