From a1640677b845529bb02edc5f08cd4eaddc36bd6d Mon Sep 17 00:00:00 2001 From: adi-herwana-nus Date: Sun, 6 Sep 2026 00:25:50 +0800 Subject: [PATCH] fix(sign_out): invalidate cookie server-side on user sign out --- app/controllers/access_token_controller.rb | 30 +++++++++ .../lib/components/wrappers/AuthProvider.tsx | 26 ++++++-- client/app/utilities/authentication.ts | 52 ++++++++++++++++ config/routes.rb | 1 + spec/requests/access_token_spec.rb | 59 ++++++++++++++++++ spec/requests/sidekiq_web_spec.rb | 12 ---- spec/support/access_token_cookie.rb | 25 ++++++++ spec/support/authentication_performers.rb | 4 +- tests/helpers.ts | 10 ++- tests/tests/courses/landing-page-auth.spec.ts | 61 +++++++++++++++++++ 10 files changed, 262 insertions(+), 18 deletions(-) create mode 100644 app/controllers/access_token_controller.rb create mode 100644 spec/requests/access_token_spec.rb create mode 100644 spec/support/access_token_cookie.rb diff --git a/app/controllers/access_token_controller.rb b/app/controllers/access_token_controller.rb new file mode 100644 index 00000000000..05e27949ffb --- /dev/null +++ b/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 diff --git a/client/app/lib/components/wrappers/AuthProvider.tsx b/client/app/lib/components/wrappers/AuthProvider.tsx index 27316e08e60..66c6f777eee 100644 --- a/client/app/lib/components/wrappers/AuthProvider.tsx +++ b/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 => { - 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 { diff --git a/client/app/utilities/authentication.ts b/client/app/utilities/authentication.ts index 786bb9ff8d6..64f26cd5a68 100644 --- a/client/app/utilities/authentication.ts +++ b/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 => { + 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 => { + 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}`, + ); +}; diff --git a/config/routes.rb b/config/routes.rb index 008618471aa..b33a421d82d 100644 --- a/config/routes.rb +++ b/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 diff --git a/spec/requests/access_token_spec.rb b/spec/requests/access_token_spec.rb new file mode 100644 index 00000000000..30e6bd5d559 --- /dev/null +++ b/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 diff --git a/spec/requests/sidekiq_web_spec.rb b/spec/requests/sidekiq_web_spec.rb index af91704d26a..252627f3ca3 100644 --- a/spec/requests/sidekiq_web_spec.rb +++ b/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 diff --git a/spec/support/access_token_cookie.rb b/spec/support/access_token_cookie.rb new file mode 100644 index 00000000000..bbfccf4f8fd --- /dev/null +++ b/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 diff --git a/spec/support/authentication_performers.rb b/spec/support/authentication_performers.rb index 2fb618d8e62..828d5c088f2 100644 --- a/spec/support/authentication_performers.rb +++ b/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 diff --git a/tests/helpers.ts b/tests/helpers.ts index 4bc4aac3909..4a4edc98d7a 100644 --- a/tests/helpers.ts +++ b/tests/helpers.ts @@ -140,10 +140,18 @@ export const test = base.extend({ 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('/'); }, }); }, diff --git a/tests/tests/courses/landing-page-auth.spec.ts b/tests/tests/courses/landing-page-auth.spec.ts index bfca4134de6..3365536d7e5 100644 --- a/tests/tests/courses/landing-page-auth.spec.ts +++ b/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 => { 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: `

Welcome

`, + }, + }); + }); + + 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 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); + }); +});