## Summary A request that is already in flight when the session is rotated can overwrite the browser's valid session cookie with a fresh anonymous one, silently signing the user out. The most reproducible trigger is admin impersonation, which rotates the session from a fully-loaded authenticated page. This was found by @karichards while investigating a flaky spec (https://gitlab.com/gitlab-org/quality/test-failure-issues/-/work_items/43497), but the mechanism is production code, not test-only. Opening this so the analysis is not lost when that work item auto-closes. ## The race 1. `Warden::Proxy#set_user` sets `renew: true` on the Rack session options (`warden-1.2.9/lib/warden/proxy.rb:178-186`). GitLab calls it in `Admin::UsersController#impersonate` (`app/controllers/admin/users_controller.rb:54`), in `Impersonation#stop_impersonation` (`app/controllers/concerns/impersonation.rb:34`), and on every sign-in. 1. Rack's `commit_session` sees `renew`, deletes the old session from Redis and mints a new session ID. 1. A request already in flight still carries the old cookie. `ActionDispatch::Session::CacheStore#find_session` misses in Redis and falls through to `generate_sid`, so that request is handed a **brand new, empty** session. 1. `Gitlab::Sessions::CacheStore#write_session` returns `sid` unconditionally (`lib/gitlab/sessions/cache_store.rb:49`), so Rack emits a `Set-Cookie` carrying that new anonymous SID - overwriting the valid cookie the rotating request had just set. 1. The browser is now unauthenticated. The next request 401s, or fails CSRF verification with a 422. The session write throttling in `Gitlab::Sessions::CacheStore` does not mitigate this. It only decides whether Redis is written, never which cookie is sent. It also cannot engage here at all: a freshly generated empty session has no `LAST_WRITE_AT_KEY`, so `write_throttled?` is always falsey on the offending request. This is a distinct failure from the same-SID stale-snapshot clobber fixed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/247579. ## Evidence From @karichards's investigation, using `log/test_json.log` and `log/api_json.log` in the artifacts of job https://gitlab.com/gitlab-org/gitlab/-/jobs/15265571634: ``` time method status path user_id redis_sessions_read_bytes 13:01:40.758 POST 302 /admin/users/user35/impersonate 161 317 13:01:40.930 GET 401 /api/v4/user_counts - none 13:01:41.832 GET 200 / 162 446 13:01:42.598 DELETE 422 /o/common-org/admin/impersonation - 147 ``` `none` bytes read on the `/api/v4/user_counts` request means the session key was already gone; the 147 bytes read afterwards are a new, empty session. The `DELETE` then fails CSRF verification because the browser is no longer authenticated. ## Impact **It fails closed, so this is a reliability bug rather than a security one.** The clobbering cookie points at a new *empty* session, so the browser drops to anonymous and must re-authenticate. It cannot leave an admin holding the impersonated user's session, or vice versa. No escalation and no cross-user leakage is possible through this path. **User-visible symptom:** an unexpected sign-out, or `422 Can't verify CSRF token authenticity` on the next action. Recoverable by signing in again. **Frequency in production is limited but non-zero.** The trigger observed in CI is `GET /api/v4/user_counts`, which `SidebarsHelper` normally avoids by shipping cached counts (`app/helpers/sidebars_helper.rb:72-73`, `cached_only: true`, 24-hour `COUNT_CACHE_VALIDITY_PERIOD`). But `cached_only: true` still returns `nil` on a genuine miss - first load after expiry, cache eviction, a brand-new user, a Redis flush - and then the same request fires. More importantly, user counts is only *one* candidate: any in-flight request straddling the rotation does this. Impersonation is the most exposed flow because it rotates the session from a page that is already loaded and making requests. ## Why no test covers this The impersonation feature spec was catching this incidentally, as intermittent CSRF failures (68 blocked pipelines across 36 MRs in a 14-day window). That spec has been stabilised in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/247657 by warming the count caches so the fetch never fires, which is the right call for pipeline stability but means nothing exercises this race any more. That MR added deliberately temporary scaffolding to `spec/features/admin/users/admin_impersonates_user_spec.rb`, which should be removed as part of fixing this: - `warm_sidebar_counts` and `sidebar_counts_warmed` helpers and their comment blocks - both `:use_clean_rails_memory_store_caching` tags - both `expect(sidebar_counts_warmed).not_to include(nil)` guard assertions - the `:aggregate_failures` on `'logs in as the user when impersonate is clicked'`, which exists only because the guard added a second expectation The controller-spec coverage added in the same MR (`spec/controllers/admin/impersonations_controller_spec.rb`) is unrelated to this race and must be kept. ## Proposed direction A request that arrives with an unknown SID and never writes anything to the session has nothing to persist, and should not be able to emit a `Set-Cookie`. Two existing patterns are plausible starting points: - `request.session_options[:skip] = true` - already used by `skip_session` in `app/controllers/concerns/jwt_authenticatable.rb:69`. Rack's `commit_session?` returns false when `options[:skip]`, suppressing `Set-Cookie` entirely. - `Gitlab::Middleware::StripCookies` - already strips the `Set-Cookie` response header for `^/assets/`, though currently only wired up in development and test. Neither has been validated for blast radius yet; both need care around CSRF token issuance and session fixation, so this wants Manage::Authentication plus AppSec input. ## Acceptance criteria - [ ] A request arriving with an unknown session ID that does not mutate the session does not emit a `Set-Cookie`, so it cannot overwrite a cookie set by a concurrent rotation. - [ ] Deterministic regression coverage at the request or middleware level, not relying on a `:js` feature spec. - [ ] The temporary scaffolding listed above is removed from `spec/features/admin/users/admin_impersonates_user_spec.rb`, and the spec is confirmed green over repeated sequential runs without it. - [ ] No regression in sign-in, 2FA, passkey, or impersonation flows.