fix(sign_out): invalidate cookie server-side on user sign out

app/controllers/access_token_controller.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+# Expires the +access_token+ cookie that ApplicationUserConcern writes.
+#
+# The cookie is httponly, so the client cannot clear it on its own: js-cookie deletes by writing
+# through +document.cookie+, which an httponly cookie is invisible to by definition. Signing out
+# therefore has to ask the server to expire it, or it outlives the sign out as a usable credential
+# for browser-issued requests, which fall back to the cookie when no bearer token is present.
+#
+# A caller can only ever clear their own cookie. This action takes no parameters and identifies no
+# user - it expires the cookie on the response to this very request, so the only browser it can
+# affect is the one that made it.
+class AccessTokenController < ApplicationController
+  # +refresh_token_cookie+ would otherwise mint the cookie from this request's own bearer token
+  # moments before the action deletes it. The net result is the same, but this endpoint should only
+  # ever be capable of clearing a credential, never of issuing one.
+  skip_before_action :refresh_token_cookie
+
+  def destroy
+    cookies.delete(:access_token)
+    head :no_content
+  end
+
+  protected
+
+  # Signing out has to work with a credential that has already lapsed, which is exactly when a
+  # stale cookie is most likely to still be sitting in the browser.
+  def publicly_accessible?
+    true
+  end
+end

client/app/lib/components/wrappers/AuthProvider.tsx

@@ -4,7 +4,6 @@ import {
   AuthProvider as OIDCAuthProvider,
   useAuth,
 } from 'react-oidc-context';
-import Cookies from 'js-cookie';
 import {
   type SigninRedirectArgs,
   type SignoutRedirectArgs,
@@ -13,6 +12,7 @@ import {
   UserManager,
   WebStorageStateStore,
 } from 'oidc-client-ts';
+import { revokeAccessTokenCookie } from 'utilities/authentication';
 
 interface AuthProviderProps {
   children: ReactNode;
@@ -108,11 +108,29 @@ export const useAuthAdapter = (): AuthAdapterProps => {
   // Not supported yet as signoutCallback from oidc-client-ts is not called in react-oidc-context.
   // Has been fixed in v3.1.0 in react-oidc-context but not released yet.
 
+  /**
+   * The order here is load-bearing in both directions, and there is no third step to slot in
+   * between them.
+   *
+   * `revokeAccessTokenCookie` must come first and must be awaited. The cookie is httponly, so that
+   * request is the only thing in the system that can clear it, and `signoutRedirect` navigates the
+   * document away - an in-flight request is cancelled with it. It is awaited for delivery, not for
+   * its result: a failure is swallowed, since a user signing out should never be shown an error and
+   * the cookie's own JWT lapses shortly regardless.
+   *
+   * Nothing may clear stored auth state before `signoutRedirect`. It reads the stored user itself
+   * to build `id_token_hint`, and removes the user itself once it has. Clearing storage first
+   * leaves the hint out of the request, and Keycloak then prompts for confirmation rather than
+   * ending the session - so a user who does not complete that prompt stays signed in upstream.
+   */
   const handleLogout = async (): Promise<void> => {
-    await otherProps.removeUser();
+    await revokeAccessTokenCookie().catch((error) => {
+      // Swallowed so a failure never blocks signing out, but never silently: swallowing this once
+      // hid a revocation that was not happening at all.
+      console.warn('Access token cookie was not revoked on sign out', error);
+    });
+
     await adaptedSignOutRedirect();
-    localStorage.clear();
-    Cookies.remove('access_token');
   };
 
   return {

client/app/utilities/authentication.ts

@@ -19,3 +19,55 @@ export const getUserToken = (): string => {
  */
 export const hasStoredUser = (): boolean =>
   Boolean(localStorage.getItem(OIDC_STORAGE_KEY));
+
+// The `format=json` is what routes a request to Rails rather than to the client app: the dev server
+// serves the SPA for anything without it, so omitting it returns an HTML page with a 200.
+const CSRF_TOKEN_URL = '/csrf_token?format=json' as const;
+const ACCESS_TOKEN_URL = '/access_token?format=json' as const;
+
+/**
+ * Best effort, because a missing token is not worth abandoning the sign out over: the DELETE is
+ * still attempted without the header, and fails loudly there instead of silently here.
+ */
+const fetchCsrfToken = async (): Promise<string | undefined> => {
+  try {
+    const response = await fetch(CSRF_TOKEN_URL, {
+      credentials: 'include',
+      headers: { Accept: 'application/json' },
+    });
+
+    if (!response.ok) return undefined;
+
+    return (await response.json()).csrfToken;
+  } catch {
+    return undefined;
+  }
+};
+
+/**
+ * Asks the server to expire the `access_token` cookie.
+ *
+ * The cookie is httponly, so nothing here can clear it directly - `document.cookie`, which every
+ * client-side cookie library writes through, cannot see it at all. Only a server response can, and
+ * until one does the cookie stays a usable credential for requests that carry no bearer token.
+ *
+ * Deliberately built on bare `fetch` rather than `BaseAPI`: the API layer reaches back into
+ * `AuthProvider` for its 401 handling, and importing it from there would close an import cycle.
+ */
+export const revokeAccessTokenCookie = async (): Promise<void> => {
+  const csrfToken = await fetchCsrfToken();
+
+  const response = await fetch(ACCESS_TOKEN_URL, {
+    method: 'DELETE',
+    credentials: 'include',
+    headers: {
+      Accept: 'application/json',
+      ...(csrfToken && { 'X-CSRF-Token': csrfToken }),
+    },
+  });
+
+  if (!response.ok)
+    throw new Error(
+      `Could not revoke the access token cookie: ${response.status}`,
+    );
+};

config/routes.rb

@@ -79,6 +79,7 @@
   }
 
   get 'csrf_token' => 'csrf_token#csrf_token'
+  delete 'access_token' => 'access_token#destroy'
 
   # Sidekiq's own dashboard, gated on the same access token as the rest of the app rather than on
   # separate HTTP Basic credentials. Guarded because the sidekiq gems are in the :production (and

spec/requests/access_token_spec.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+require 'rails_helper'
+
+# The cookie under test is httponly, so only a server response can expire it. That makes this a
+# request spec: what matters is the Set-Cookie the browser actually receives.
+RSpec.describe 'Access token cookie' do
+  let(:instance) { Instance.default }
+
+  with_tenant(:instance) do
+    let(:administrator) { create(:administrator) }
+
+    before { host! instance.host }
+
+    # Mirrors the controller specs, which sidestep Keycloak the same way.
+    before do
+      allow(Authentication::AuthenticationService).to receive(:validate_token) do |access_token, _method|
+        if access_token == 'a-valid-token'
+          Authentication::VerificationService::Response.new(
+            { email: administrator.email, session_state: 'a-session' }, nil
+          )
+        else
+          error = Authentication::VerificationService::Error.new('Invalid token', :unauthorized)
+          Authentication::VerificationService::Response.new(nil, error)
+        end
+      end
+    end
+
+    def deleted_in_response?
+      # Rails expires a cookie by sending it back empty, so the header names it with a nil value.
+      response.cookies.key?('access_token') && response.cookies['access_token'].nil?
+    end
+
+    it 'expires the cookie the request arrived with' do
+      delete '/access_token', headers: access_token_cookie('a-previously-issued-token')
+
+      expect(response).to have_http_status(:no_content)
+      expect(deleted_in_response?).to be(true)
+    end
+
+    # Signing out is exactly when the credential is most likely to be unusable already, so this
+    # must not require one.
+    it 'succeeds without any credential' do
+      delete '/access_token'
+
+      expect(response).to have_http_status(:no_content)
+    end
+
+    # ApplicationUserConcern#refresh_token_cookie mints the cookie on every publicly accessible
+    # action, and this is one. Skipping it here keeps the endpoint incapable of issuing a
+    # credential, so a bearer token cannot leave a freshly minted cookie behind.
+    it 'does not mint a cookie when a bearer token is presented' do
+      expect_any_instance_of(AccessTokenController).not_to receive(:refresh_token_cookie)
+      delete '/access_token', headers: { 'Authorization' => 'Bearer a-valid-token' }
+
+      expect(response).to have_http_status(:no_content)
+      expect(response.cookies['access_token']).to be_nil
+    end
+  end
+end

spec/requests/sidekiq_web_spec.rb

@@ -274,17 +274,5 @@ def redirect_query
     def bearer(token)
       { 'Authorization' => "Bearer #{token}" }
     end
-
-    # Mirrors ApplicationUserConcern#add_token_to_cookie: a browser's copy of the access token is an
-    # encrypted, httponly cookie, so build one exactly as the app writes it. The ciphertext has to
-    # be escaped the way Rails escapes it on the way out, otherwise any '+' it happens to contain
-    # is read back as a space and decryption fails for roughly half of all generated tokens.
-    def access_token_cookie(token)
-      env = Rack::MockRequest.env_for('/', 'HTTP_HOST' => 'test.host').merge(Rails.application.env_config)
-      jar = ActionDispatch::Cookies::CookieJar.build(ActionDispatch::Request.new(env), {})
-      jar.encrypted[:access_token] = token
-
-      { 'HTTP_COOKIE' => "access_token=#{Rack::Utils.escape(jar[:access_token])}" }
-    end
   end
 end

spec/support/access_token_cookie.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+# Builds the access_token cookie that ApplicationUserConcern#add_token_to_cookie writes, for specs
+# that need a request to arrive carrying one.
+#
+# It cannot be seeded through an integration session's own jar: that is a Rack::Test::CookieJar,
+# which has no #encrypted. So the ciphertext is produced from a real CookieJar and handed over as a
+# raw request header.
+module AccessTokenCookieHelpers
+  # @param [String] token The access token to encrypt into the cookie.
+  # @return [Hash] Headers to merge into a request, carrying the cookie as the browser would.
+  def access_token_cookie(token)
+    env = Rack::MockRequest.env_for('/', 'HTTP_HOST' => 'test.host').merge(Rails.application.env_config)
+    jar = ActionDispatch::Cookies::CookieJar.build(ActionDispatch::Request.new(env), {})
+    jar.encrypted[:access_token] = token
+
+    # The ciphertext has to be escaped the way Rails escapes it on the way out, otherwise any '+' it
+    # happens to contain is read back as a space and decryption fails for roughly half of all
+    # generated tokens.
+    { 'HTTP_COOKIE' => "access_token=#{Rack::Utils.escape(jar[:access_token])}" }
+  end
+end
+
+RSpec.configure do |config|
+  config.include AccessTokenCookieHelpers, type: :request
+end

spec/support/authentication_performers.rb

@@ -20,11 +20,13 @@ def login_as(user, **kwargs)
     expect(page).to have_css('div[data-testid="user-menu-button"]')
   end
 
+  # The Logout button this used to click was Keycloak's logout confirmation page, shown only because
+  # the client cleared its stored auth state before signoutRedirect could read the ID token out of
+  # it. With id_token_hint restored, Keycloak ends the session without prompting.
   def logout(*_)
     find('div[data-testid="user-menu-button"]').click
     wait_for_animation
     find('li', text: 'Sign out').click
-    click_button('Logout')
     expect(page).to_not have_css('div[data-testid="user-menu-button"]')
   end
 

tests/helpers.ts

@@ -140,10 +140,18 @@ export const test = base.extend<TestFixtures>({
 
     await extend(use, page.originalPage, {
       user,
+      // The third click this used to need was Keycloak's logout confirmation page, which only
+      // appeared because `handleLogout` cleared stored auth state before `signoutRedirect` could
+      // read the ID token out of it. With `id_token_hint` restored, Keycloak ends the session
+      // without prompting, as it did before the Keycloak migration.
       signOut: async () => {
         await page.getUserMenuButton().click();
         await page.getByRole('button', { name: 'Sign out' }).click();
-        await page.getByRole('button', { name: 'Logout' }).click();
+
+        // Sign out revokes the cookie, then round-trips through Keycloak back to the origin. The
+        // click alone returns long before any of that lands, so wait for the destination or every
+        // assertion after `signOut()` races it. (Callers already on `/` get no wait from this.)
+        await page.waitForURL('/');
       },
     });
   },

tests/tests/courses/landing-page-auth.spec.ts

@@ -57,6 +57,11 @@ const watchAttachmentResponses = (page: Page, id: string): number[] => {
   return statuses;
 };
 
+const getAccessTokenCookie = async (
+  page: Page,
+): Promise<{ name: string } | undefined> =>
+  (await page.context().cookies()).find((c) => c.name === ATTACHMENT_COOKIE);
+
 const expectAttachmentToLoad = async (statuses: number[]): Promise<void> => {
   await expect
     .poll(() => statuses.length, {
@@ -192,3 +197,59 @@ test.describe('signing in from a publicly accessible course page', () => {
     });
   });
 });
+
+test.describe('signing out', () => {
+  let course: { id: number };
+  let attachmentId: string;
+
+  test.beforeEach(async () => {
+    const attachment = await manufacture({ attachment_reference: {} });
+    attachmentId = attachment.id;
+
+    course = await manufacture({
+      course: {
+        traits: ['published'],
+        description: `<p>Welcome</p><img src="/attachments/${attachmentId}">`,
+      },
+    });
+  });
+
+  test('clears the access token cookie', async ({ authedPage: page }) => {
+    // Visiting the landing page mints the cookie, so it is definitely present
+    // before we sign out and the assertion below cannot pass vacuously.
+    await page.goto(`/courses/${course.id}`);
+    await expect.poll(() => getAccessTokenCookie(page)).toBeDefined();
+
+    await page.signOut();
+
+    // The cookie is httponly, so nothing on the page can remove it. Only the
+    // server call in `handleLogout` can, and if that call is ever dropped or
+    // sequenced after `signoutRedirect` this is what catches it.
+    await expect.poll(() => getAccessTokenCookie(page)).toBeUndefined();
+  });
+
+  test('leaves no credential behind for subresource requests', async ({
+    authedPage: page,
+  }) => {
+    await page.goto(`/courses/${course.id}`);
+
+    // The cookie is minted by the course fetch the app makes after load, not by
+    // the navigation itself, so wait for it rather than racing it: `goto`
+    // resolves long before the app has called the API. Without this the request
+    // below reliably 401s, since `AttachmentReferencesController` requires
+    // authentication and this request carries no bearer token to fall back on.
+    await expect.poll(() => getAccessTokenCookie(page)).toBeDefined();
+
+    // Requested through the context rather than the page, so it carries the
+    // cookie and no Authorization header - exactly what an <img> sends.
+    const whileSignedIn = await page.request.get(`/attachments/${attachmentId}`);
+    expect(whileSignedIn.ok()).toBe(true);
+
+    await page.signOut();
+
+    // The symptom that prompted this: a signed-out user whose surviving cookie
+    // still authenticated them to the backend for up to an hour.
+    const afterSignOut = await page.request.get(`/attachments/${attachmentId}`);
+    expect(afterSignOut.status()).toBe(401);
+  });
+});