Skip to content

fix: close realtime connection on logout to prevent ghost sessions (#1158) - #1394

Open
rohanesor wants to merge 3 commits into
RocketChat:developfrom
rohanesor:fix/1158-close-socket-on-logout
Open

rohanesor wants to merge 3 commits into
RocketChat:developfrom
rohanesor:fix/1158-close-socket-on-logout

Conversation

@rohanesor

Copy link
Copy Markdown

Fixes #1158

Problem

EmbeddedChatApi.logout() only awaited this.auth.logout(), which clears the auth token. The DDP connection and its _activeSubscriptions were left running, so after a "logout" the client kept receiving live messages and events for a session that was no longer authenticated — a privacy leak, and inconsistent state if another user logged in without a full page refresh.

The class already has a close() method that stops _activeSubscriptions and calls sdk.connection.close(). logout() simply never called it.

Fix

logout() now tears the connection down via the existing close(), with two deliberate details:

  • the teardown runs in a finally block, so the socket is also closed when the logout request itself fails (a failed logout previously left a fully live session behind);
  • close() errors are logged rather than propagated, so they cannot mask the logout result. Other call sites already treat close() as rejectable (EmbeddedChat.js wraps it in .catch()).

Verification

packages/api has no test runner ("test": "echo \"Error: no test specified\""), so I verified behaviourally with a harness that bundles the real EmbeddedChatApi.ts (deps stubbed) and asserts the teardown contract. It is reproducible and included below.

Run against this branch — all pass:

PASS  successful logout stops all subscriptions
PASS  successful logout closes the connection
PASS  subscription list is cleared
PASS  failed logout still closes the connection
PASS  failed logout still stops subscriptions
PASS  failed logout still returns the error
PASS  close() failure does not propagate out of logout

ALL CHECKS PASSED

Run against unpatched develop — the same harness fails, confirming it genuinely exercises the bug:

FAIL  successful logout stops all subscriptions
FAIL  successful logout closes the connection
FAIL  subscription list is cleared
FAIL  failed logout still closes the connection
FAIL  failed logout still stops subscriptions
PASS  failed logout still returns the error
PASS  close() failure does not propagate out of logout

5 CHECK(S) FAILED

Also run: tsc --noEmit on packages/api (clean), prettier --check (clean), yarn lint (0 errors; warning count unchanged from develop), and yarn workspace @embeddedchat/api build (succeeds).

If you'd prefer this as a committed test, I'm happy to add a runner to packages/api in a follow-up — I kept it out of this PR to hold the diff to the one-file fix.

Prior art

@Aryan-Verma-999's earlier #1171 identified the same root cause and was closed unmerged. This PR keeps that insight and adds the finally handling so a failed logout cannot leave a live socket behind.

EmbeddedChatApi.logout() only cleared the auth token via auth.logout().
The DDP connection and its active subscriptions were left running, so the
client kept receiving live messages and events for a session that was no
longer authenticated.

logout() now tears the connection down via the existing close() method.
The teardown runs in a finally block so the socket is also closed when the
logout request itself fails, and close() errors are logged rather than
propagated so they cannot mask the logout result.

Fixes RocketChat#1158
// keeps receiving messages for a session that is no longer
// authenticated.
try {
await this.close();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not cancel an in-flight _doConnect(). If logout happens while connect() is awaiting joinRoom(), _doConnect() can resume afterward and add new stream subscriptions. The DDP client retains and replays those on the next connection.

Please cancel/invalidate pending connects

@Aryan-Verma-999

Copy link
Copy Markdown
Collaborator

Thanks for picking this up :)
please address the comments. Thanks.

close() now invalidates a connect() which is still awaiting, so that
subscriptions are not registered after the socket has been torn down.

Previously, if logout happened while _doConnect() was awaiting
joinRoom() (or the connection/login steps), the connect would resume
afterwards and register the stream subscriptions on the closed
connection, which the DDP client replays on the next connection.

Addresses review feedback on RocketChat#1394.
@rohanesor

Copy link
Copy Markdown
Author

Thanks for the review @Aryan-Verma-999 — you're right, and the window was wider than just joinRoom().

_doConnect() awaits in four places before it subscribes (connection.connect(), getCurrentUser(), loginWithToken(), joinRoom()). A close() during any of them left the connect to resume and register the streams afterwards.

Fixed in dceb793 with a connection generation counter:

  • close() increments _connectionGeneration and stops the active subscriptions.
  • _doConnect() snapshots the generation after its initial close() and bails out if it no longer matches, checked after each await.
  • A final check after the subscriptions are registered stops them if a close() landed while they were being set up, which closes the last gap.

I also pulled the teardown out into _stopSubscriptions() so close() and that late check share it.

Verification

packages/api has no test runner, so I verified behaviourally: bundled the real source with the transport stubbed, made joinRoom() block, called close() mid-connect, then released it — which is exactly the sequence you described.

Before:

subscriptions created after logout : 5
subscriptions still active (leaked) : 5
  ['room-messages', 'notify-room', 'notify-room', 'notify-user', 'notify-user']
FAIL: ghost subscriptions remain

After:

subscriptions created after logout : 0
subscriptions still active (leaked) : 0
PASS: no ghost subscriptions

I ran the harness against the previous commit first to confirm it actually reproduces the bug rather than passing vacuously.

Also run: tsc --noEmit on packages/api (clean), prettier --check (clean), yarn lint (0 errors, warning count unchanged from develop).

One thing still outside this PR's reach: CI hasn't run on any of my PRs yet — the workflow runs are sitting at action_required pending a maintainer approval for a first-time contributor. If you're able to approve those, it'd be good to get real CI signal on this.

rohanesor added a commit to rohanesor/EmbeddedChat that referenced this pull request Oct 25, 2026
…ocketChat#1158) (#4)

* fix: close realtime connection on logout to prevent ghost sessions

EmbeddedChatApi.logout() only cleared the auth token via auth.logout().
The DDP connection and its active subscriptions were left running, so the
client kept receiving live messages and events for a session that was no
longer authenticated.

logout() now tears the connection down via the existing close() method.
The teardown runs in a finally block so the socket is also closed when the
logout request itself fails, and close() errors are logged rather than
propagated so they cannot mask the logout result.

Fixes RocketChat#1158

* [DOCS] Align Node.js requirement in docs README with package.json engines

* feat: keep emoji picker open for multiple selections

* chore: format code

* fix: cancel in-flight connect on logout to prevent ghost subscriptions

close() now invalidates a connect() which is still awaiting, so that
subscriptions are not registered after the socket has been torn down.

Previously, if logout happened while _doConnect() was awaiting
joinRoom() (or the connection/login steps), the connect would resume
afterwards and register the stream subscriptions on the closed
connection, which the DDP client replays on the next connection.

Addresses review feedback on RocketChat#1394.

---------

Co-authored-by: swarnendusahu10-dotcom <swarnendusahu10@gmail.com>
Co-authored-by: Aryan Verma <121835781+Aryan-Verma-999@users.noreply.github.com>
Co-authored-by: iRahul360 <rahulburman669@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ghost Session: WebSocket Connection Leaks After Logout

2 participants