Fix cross-user identity binding race in PROXY auth mode websocket setup

[herdiyana256]

Summary

In security.authentication.mode: PROXY deployments, server.js captures the auth token / user id used to make backend API calls over the websocket resource-polling channel (server/aggregator.js) in two module-level variables, authToken and userid:

let authToken, userid;
sockServer.on('connection', function(c) {
  ...
  c.authToken = authToken;
  c.userid = userid;
});
server.addListener('upgrade', function(req, socket) {
  authToken = req.headers.authorization;
  userid = req.headers[userIdProperty];
  ...
});

These variables are shared across every in-flight sockjs session on the process, not scoped per session. That has two consequences:

1. Race under concurrency: if two users' sessions are being set up around the same time, a second upgrade event can overwrite authToken/userid before an earlier session's connection handler reads them back. The earlier session ends up bound to the other user's identity for the lifetime of that websocket connection -- every subsequent backend API call made through it (via Aggregator, server/aggregator.js) executes as the wrong user.
2. Deterministic misbinding for non-websocket transports: sockjs falls back to plain HTTP transports (xhr-streaming, xhr-polling, eventsource) whenever a websocket upgrade isn't available (some corporate proxies, older browsers). Those transports never fire Node's upgrade event at all, so every session opened this way is bound to whatever authToken/userid some unrelated, earlier websocket connection last left behind (or undefined if none has occurred yet in the process's life).

Verified both with the real sockjs/ws packages, not a synthetic re-implementation:

• 50 concurrent websocket connections against the current logic produced 50/50 cross-bound identities (each connection ended up bound to whichever connection's upgrade fired last, not its own).
• A single xhr-streaming request, sent with its own distinct Authorization/identity headers, deterministically inherited an unrelated, earlier websocket session's identity instead of its own.

This is a real design gap, not a documented tradeoff -- the existing comment block above this code explicitly describes the intent as "we take those values and add to the connection object" (i.e. meant to be per-connection), but the implementation used a variable shared across all connections instead.

Fix

Extracted the capture/lookup into server/socket-identity.mjs, keyed by sockjs session id (parsed from the request URL, e.g. /_sock/000/<session>/<transport>) instead of a single shared variable:

• capture(req) records the identity for a specific session id, called from both the upgrade listener (websocket transport) and a new request listener (covers the polling/streaming fallback transports, which never fire upgrade).
• consume(url) looks up and removes a specific session's identity, called once from the connection handler using that connection's own url.

Also moved the Origin check ahead of identity capture in the upgrade listener -- previously a rejected-origin request still populated the shared state before being denied, which no longer matters for a different reason now (state is scoped per session) but was tightened anyway since it's the same code region.

Test plan

• node -e "require('@babel/core').parse(...)" (via the project's actual @babel/core): both server.js and server/socket-identity.mjs parse cleanly.
• server/socket-identity.test.js (Jest, describe/test/expect) unit-tests getSockjsSessionId and createSocketIdentityStore directly, including a regression test asserting that capturing two different sessions' identities back-to-back (simulating the interleaving that used to clobber the shared variable) does not leak one into the other.
• yarn install did not complete in the environment I used for this (same large/older dependency tree issue noted on Fix arbitrary code execution and predictable session tokens in /updateTheme #1425) -- node_modules/pretty-format itself is missing internal files, so I couldn't run this through the project's actual Jest config. I instead: (a) installed a fresh, isolated jest/babel toolchain and confirmed all 6 tests in server/socket-identity.test.js pass against the unmodified file; (b) installed the real sockjs/ws packages standalone and ran the exact pre-fix and post-fix logic (copied verbatim, not reimplemented) against real HTTP/websocket traffic to produce the before/after numbers above. Happy to re-verify against the real Jest run if that's easier for a reviewer with the deps already installed.

Related to the auth wiring discussed in #1425, but a distinct root cause in a different file (server.js/server/aggregator.js, PROXY-mode websocket setup) untouched by that PR.

[gemini-code-assist]

Code Review

This pull request introduces a SocketIdentityStore to manage sockjs session identities by session ID instead of a single shared variable, preventing identity leakage across concurrent connections—especially for HTTP fallback transports. The reviewer feedback highlights critical issues: first, same-origin fallback requests lacking an Origin header will fail validation, so isAllowedOrigin should support an optional flag to allow missing origins; second, the store's timer management can leak timeouts or cause premature deletion races when multiple requests occur for the same session, which can be resolved by tracking and clearing active timeouts.

[gemini-code-assist]

In same-origin HTTP requests (such as fallback transports like xhr-streaming or eventsource), browsers typically do not send an Origin header. Under the current implementation, isAllowedOrigin will return false when req.headers.origin is undefined, causing the request listener to skip capturing the session identity. This completely breaks fallback transports in PROXY mode.

We should update isAllowedOrigin to accept an optional allowMissing parameter (defaulting to false to preserve strict origin validation for WebSocket upgrades) and return true if the Origin header is missing and allowMissing is enabled.

    function isAllowedOrigin(req, allowMissing = false) {
      const origin = req.headers.origin;
      if (!origin) {
        return allowMissing;
      }
      return allowedOrigin.indexOf(origin) !== -1;
    }

[herdiyana256]

Fixed in 622b70b. isAllowedOrigin now takes an allowMissing flag, and the request listener passes true since these fallback transports don't reliably send Origin on same-origin requests. Upgrade path stays strict.

[gemini-code-assist]

Update the request listener to call isAllowedOrigin(req, true) so that same-origin fallback transport requests without an Origin header are allowed to have their identities captured.

Suggested change

	server.addListener('request', function(req) {
	if (!isAllowedOrigin(req)) {
	return;
	}
	socketIdentities.capture(req);
	});
	server.addListener('request', function(req) {
	if (!isAllowedOrigin(req, true)) {
	return;
	}
	socketIdentities.capture(req);
	});

[herdiyana256]

Done in 622b70b, request listener now calls isAllowedOrigin(req, true).

[gemini-code-assist]

There are two issues with the current timer management in createSocketIdentityStore:

1. Dangling/Leaked Timeouts: Every call to capture (which happens on every polling request for fallback transports) schedules a new setTimeout that is never cleared, even after the identity is successfully consumed or overwritten. This leads to unnecessary CPU overhead and memory churn.
2. Premature Deletion Race: If multiple handshake or transport requests for the same session ID occur before the connection is established, multiple timeouts are scheduled. When the first timeout fires, it will delete the session identity from pending, even if a subsequent request refreshed it.

Additionally, we should add defensive checks for cdapConfig to prevent runtime errors if it is null or undefined.

We can resolve these issues by storing the timer reference in the pending map, clearing any existing timeout when overwriting or consuming the session, and adding defensive checks.

export function createSocketIdentityStore({ prefix, cdapConfig, getAuthHeaderFromRawCookies, ttlMs = 60000 }) {
  const pending = new Map();

  /** Call for every request that might be starting/continuing a sockjs session. */
  function capture(req) {
    const sessionId = getSockjsSessionId(prefix, req.url);
    if (!sessionId) {
      return;
    }

    // Clear any existing timeout for this session to avoid premature deletion
    const existing = pending.get(sessionId);
    if (existing && existing.timer) {
      clearTimeout(existing.timer);
    }

    req.headers.authorization = getAuthHeaderFromRawCookies(req);
    const userIdProperty = cdapConfig && cdapConfig['security.authentication.proxy.user.identity.header'];
    
    const timer = setTimeout(() => pending.delete(sessionId), ttlMs);
    if (typeof timer.unref === 'function') {
      timer.unref();
    }

    pending.set(sessionId, {
      authToken: req.headers.authorization,
      userid: userIdProperty ? req.headers[userIdProperty] : undefined,
      timer,
    });
  }

  /** Call once a sockjs 'connection' fires, with that connection's own url. */
  function consume(url) {
    const sessionId = getSockjsSessionId(prefix, url);
    if (!sessionId) {
      return {};
    }
    const identity = pending.get(sessionId);
    if (!identity) {
      return {};
    }
    if (identity.timer) {
      clearTimeout(identity.timer);
    }
    pending.delete(sessionId);
    return {
      authToken: identity.authToken,
      userid: identity.userid,
    };
  }

  return { capture, consume };
}

[herdiyana256]

Fair point, polling transports call capture() once per poll for the same session so the old timers were piling up and one could delete a fresher entry before it got consumed. Fixed in 622b70b: the timer is now stored on the entry and cleared on overwrite/consume, plus added a fake-timers test for it.

[GnsP]

We appreciate the time and effort that went into this fix.
However, we think the entire websocket setup can be safely removed - with a minor refactor. We are evaluating this possibility.

For this reason, I am putting this PR on hold for a couple days. I will get back regarding this fix once we have verified the feasibility of removing websockets entirely.

[herdiyana256]

Thanks, that makes sense. If the resource-polling websocket channel can be removed outright with a small refactor, that retires this whole identity-capture surface and is a cleaner outcome than hardening it, so I am happy to hold.

Whichever way the evaluation lands, I can help close it out: if websockets go away entirely I will close this PR, and if any of that setup has to stay I will rebase this onto whatever remains. Just let me know.

[herdiyana256]

Hi @GnsP, checking in on this one.

It has been about a week since the hold, so I wanted to ask how the evaluation went. Did removing the websocket setup entirely turn out to be feasible?

No rush either way, and my offer from before still stands: if websockets are going away I will close this PR, and if any of that setup survives the refactor I will rebase this onto whatever remains. Just let me know which direction it lands.