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

[adi-herwana-nus]

Problem

After signing out, a publicly accessible course landing page still rendered as though the user were signed in. The client chrome was correct — Sign in button, no user menu — but the data behind it was still personalised.

The credential that survived is the access_token cookie, and it survived because nothing was capable of deleting it.

ApplicationUserConcern writes it httponly:

cookies.encrypted[:access_token] =
  { value: token_from_request, httponly: true, expires: 1.hour.from_now }

And handleLogout tried to clear it from the browser:

Cookies.remove('access_token');

js-cookie deletes a cookie by writing an expired one of the same name through document.cookie. An httponly cookie is invisible and unwritable from document.cookie by definition — that is the entire purpose of the flag. This line has always been a guaranteed no-op.

The client state was cleared correctly: removeUser() drops the oidc.user: key, so hasStoredUser() is false and isAuthenticated is false. That is why the shell looked signed out. But the backend never saw a sign-out at all. Extraction is token_from_bearer || token_from_cookies, so a browser-issued request with no bearer falls back to the surviving cookie, resolves current_user, and Course::CoursesController#show — publicly accessible, so no authenticate! to fail — serves the personalised payload: registrationInfo, announcements, todos, notifications, all gated on current_course.user?(current_user).

The result was a signed-out shell over signed-in data, for up to an hour. This is a live, usable credential outliving an explicit sign-out, not a rendering artefact.

A second defect in the same function

await otherProps.removeUser();
await adaptedSignOutRedirect();   // full page navigation to Keycloak
localStorage.clear();             // races page teardown
Cookies.remove('access_token');   // ditto, and a no-op regardless

signoutRedirect navigates the document away. Everything sequenced after it is racing teardown and may never run. removeUser() happened to cover the OIDC key, which limited the damage, but any other localStorage state was left to chance.

Changes

AccessTokenController

app/controllers/access_token_controller.rb, routed as DELETE /access_token. It expires the cookie and returns 204.

It is publicly accessible. Signing out has to work with a credential that has already lapsed, which is precisely when a stale cookie is most likely to still be sitting in the browser. Requiring authenticate! would mean an expired user could never clear theirs.

A caller can only ever clear their own cookie. This is structural rather than enforced: the action takes no parameters and identifies no user. It expires the cookie on the response to that very request, so the only browser it can affect is the one that made the call. There is no target to select.

It skips refresh_token_cookie. That before_action — added in the preceding attachment fix — mints the cookie from the request's own bearer on every publicly accessible action, and this is one. Without the skip it would issue a cookie moments before the action deletes it. The net response is identical either way, but this endpoint should only ever be capable of clearing a credential, never of issuing one. Pinned by a test.

handleLogout revokes, then redirects — and nothing in between

client/app/lib/components/wrappers/AuthProvider.tsx. The order is constrained from both directions, and there is no third step to slot between them.

The server call is awaited, which is worth being explicit about because the obvious reading is that it need not be. There is no client-side fallback: the cookie is httponly, so this request is the only thing in the system that can clear it, and an in-flight request is cancelled by the navigation that follows. It is awaited for delivery, not for its result — failures are swallowed and logged, 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. That is the subject of the next section, and it is why removeUser() and localStorage.clear() are gone from this function rather than merely reordered — signoutRedirect performs the removal itself.

Cookies.remove('access_token') is deleted outright rather than kept as belt-and-braces, because it never did anything and reading it suggests otherwise. The js-cookie import went with it; that was its only use.

The Keycloak sign-out confirmation disappears, deliberately

Sign-out currently shows a Keycloak-hosted "Logout" confirmation page. This change removes it. That is a user-visible difference, so it is worth setting out why it was there and why dropping it costs nothing.

It was never a designed feature. Before the Keycloak migration, sign-out was one click. 8e7718f1f shows the original test helper:

await page.getUserMenuButton().click();
await page.getByRole('button', { name: 'Sign out' }).click();
await page.waitForURL('/users/sign_in');

The migration commit 4ededeb78 introduced handleLogout with this ordering:

await auth.removeUser();        // discards the stored user
await auth.signoutRedirect();   // ...which is where the ID token lived

and in the same commit changed the helper's last line to getByRole('button', { name: 'Logout' }).click(). The extra click was added to get past the new prompt, not because anyone wanted it. Three days later 07c956efd moved handleLogout into AuthProvider carrying the same ordering, where it has sat since May 2024.

Why that ordering produces a prompt. signoutRedirect reads the stored user itself to obtain id_token_hint, and removes the user itself once it has (oidc-client-ts, UserManager._signoutStart):

const user = await this._loadUser();                        // null, if we cleared it first
const id_token = args.id_token_hint || user && user.id_token;
if (id_token) args.id_token_hint = id_token;                // skipped
await this.removeUser();                                    // it would have done this anyway

Clearing storage first removes its only source for the hint, so the parameter is omitted from the logout URL. The earlier removeUser() call was not merely harmful — it was redundant.

Keycloak is then obliged to ask. Per OpenID Connect RP-Initiated Logout 1.0 §2, id_token_hint is RECOMMENDED, and:

At the Logout Endpoint, the OP SHOULD ask the End-User whether to log out of the OP as well. Furthermore, the OP MUST ask the End-User this question if an id_token_hint was not provided or if the supplied ID Token does not belong to the current OP session with the RP and/or currently logged in End-User.

§6 Security Considerations gives the reason:

Logout requests without a valid id_token_hint value are a potential means of denial of service; therefore, OPs should obtain explicit confirmation from the End-User before acting upon them.

So the prompt is a forced-logout mitigation. Without the hint, the request carries nothing that proves who asked: client_id is an unauthenticated URL parameter, and the KEYCLOAK_IDENTITY / AUTH_SESSION_ID cookies are ambient authority that a hostile cross-origin navigation would carry identically. Keycloak substitutes a human click, protected by the state_checker value it stores in the identity cookie and renders into the confirmation page.

Keycloak's own behaviour here dates from its move to spec compliance — see Keycloak Upgrading Guide, "OpenID Connect Logout" under Migrating to 19.0.0 and the logout endpoint documentation.

Restoring the hint loses no protection. Both mechanisms rest on the same primitive — prove you can read something same-origin that an attacker cannot:

	what must be readable	at which origin
id_token_hint	the stored ID token	the app, via same-origin localStorage
state_checker	the rendered confirmation page	Keycloak, via a same-origin response body

The ID token is issued at sign-in, kept in localStorage, and — unlike the access token — never transmitted anywhere, including to Rails (includeIdTokenInSilentRenew defaults to false). A cross-origin page cannot read it, so its presence in the request is itself the evidence the prompt was standing in for. Keycloak stops asking because the request is now authenticated, not because a check was disabled.

And the prompt we had was worse than none. Because teardown ran before the redirect, its Cancel button could not cancel anything: the cookie was already revoked and local state already discarded. Cancelling left the app signed out with the Keycloak session still live. The prompt was also a second confirmation of an intent the user had already expressed by clicking Sign out in the app.

No in-app replacement is being added. Sign-out returns to the single click it was before the migration.

On dropping localStorage.clear(). signoutRedirect removes the stored OIDC user itself, which is the only auth-bearing key. The rest of the app's localStorage is already namespaced by user id — see useDismissibleOnce (${userId}:${key}) and the table builder's sort/column state — specifically so two users on one device do not share it. So nothing leaks across a sign-out, and the call is not merely reordered but removed: it could not have run reliably where it sat, after a navigation.

revokeAccessTokenCookie

client/app/utilities/authentication.ts — built on bare fetch rather than BaseAPI. Base.ts imports ErrorHandling.ts, which imports AUTH_USER_MANAGER from AuthProvider for its 401 redirect, so calling the API layer from AuthProvider would close an import cycle.

It costs two round trips: fetch the CSRF token, then DELETE. The alternative was exempting a state-changing endpoint from CSRF protection to save one hop on sign-out, which is the wrong trade — logout CSRF is a real if minor nuisance, and sign-out latency is irrelevant.

Both URLs carry ?format=json. That is what routes a request to Rails; the dev server serves the client app for anything without it. An early revision omitted it, so /csrf_token returned a 200 of HTML, .json() threw, and the DELETE was never sent at all — an entire sign-out flow that looked correct and did nothing. That failure was invisible because the caller swallowed it, so the CSRF fetch is now best-effort (the DELETE proceeds without the header rather than being abandoned), the DELETE throws on a non-ok response, and handleLogout logs a warning when it swallows.

Testing

spec/requests/access_token_spec.rb — a request spec rather than a controller spec, because the cookie is httponly and what matters is the Set-Cookie the browser actually receives.

Test	Asserts
expires the cookie the request arrived with	204, and the cookie comes back nil-valued
succeeds without any credential	sign-out works when the token has already lapsed
does not mint a cookie when a bearer token is presented	the refresh_token_cookie skip holds

The first of those seeds a genuine encrypted cookie rather than a bare string, so the fixture matches what the app actually writes. That cannot go through the integration session's own jar — it is a Rack::Test::CookieJar and has no #encrypted — so the ciphertext is built from a real CookieJar and passed as a raw header. spec/requests/sidekiq_web_spec.rb had already solved this, so the two copies are lifted into spec/support/access_token_cookie.rb. The escaping caveat travels with it: the ciphertext must be escaped the way Rails escapes it, or a + comes back as a space and decryption fails for roughly half of all generated tokens.

Two cases added to tests/tests/courses/landing-page-auth.spec.ts:

• clears the access token cookie — visits the landing page first so the cookie is definitely minted, otherwise the post-sign-out assertion passes vacuously. This is what catches the server call being dropped or re-sequenced after signoutRedirect.
• leaves no credential behind for subresource requests — issues the attachment request through page.request, which shares the context's cookie jar but sends no Authorization header, so it is exactly what an <img> sends. ok() before sign-out, 401 after. This is the one that encodes the reported symptom: not that the UI looks signed out, which it always did, but that the surviving cookie no longer authenticates.

Two sign-out helpers clicked the confirmation button

Removing the Keycloak prompt breaks anything that clicked through it, and there were two such helpers — one per test stack. Both are updated.

tests/helpers.ts — the Playwright signOut fixture, used by every test taking authedPage, not only the two above. It loses the third click and gains a waitForURL('/') in its place. That click was doing double duty: it also made the helper wait for the Keycloak round trip to finish. Without a replacement the helper returns while the revoke and redirect are still in flight, and every assertion after signOut() races them — which the page.request case above would have hit intermittently rather than failing outright, the worst way for it to go wrong.

spec/support/authentication_performers.rb — the Capybara logout helper, called by four feature specs under spec/features/course/assessment/submission/. CI globs spec/**/*_spec.rb, so these run alongside everything else. Here the click is simply removed: the existing expect(page).to_not have_css('div[data-testid="user-menu-button"]') already provides the wait, so nothing takes its place.

What has actually been run

The RSpec request and controller specs are verified green, including a check that the cookie-expiry example fails when cookies.delete is removed rather than passing vacuously.

Not run: the Playwright cases and the four feature specs. Both need the full stack — Keycloak, the test Rails server, dirt-cheap-rocket — and are verified by inspection only. Between them they are the first real exercise of the new sign-out path, so the first CI run is the one to watch.

Notes for review

• Sign-out is now one click. The Keycloak confirmation page is gone, for the reasons set out above, and no in-app replacement is added. Nothing depended on it but the two test helpers, both updated here. It is the change users will notice: an accidental click on Sign out no longer has an undo step. That was equally true before the Keycloak migration, and re-authenticating is a single click while the SSO session is alive.
• This changes behaviour for the Sidekiq dashboard. SidekiqAdminConstraint is the one place that reads the cookie without a bearer, and it previously benefited from the cookie outliving a sign-out. An admin who signs out will now have to go back through the authorization code flow in System::Admin::SidekiqSessionsController. That is correct, but it is a change in what admins experience.
• The INVALID_GRANT_ERROR branch in AuthenticatableApp is dead code. Unrelated to this change and pre-existing, but found while working nearby. react-oidc-context normalises every error it dispatches to { name, message, stack, innerError }, so the OAuth error code the branch tests for only ever exists under innerError. The condition can never match. Either error?.innerError?.error is the one-line fix, or the branch should be removed — but that is a behaviour decision worth taking on its own.
• Description images still 401 for genuinely anonymous visitors on a published course, since AttachmentReferencesController has no publicly_accessible? override. Pre-existing and out of scope here, but this change makes it easier to hit: signing out now really does make you anonymous. Worth a look separately.

[Copilot]

🟡 Changes recommended

The new request spec does not reliably assert the refresh_token_cookie skip and uses a plaintext cookie setup that doesn’t match the app’s encrypted cookie behavior, weakening regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a sign-out security/consistency issue where an HttpOnly access_token cookie could survive logout and continue authenticating browser requests (especially subresource requests without an Authorization header), by adding a server-side cookie revocation endpoint and sequencing client teardown before the OIDC signout navigation.

Changes:

• Add DELETE /access_token (AccessTokenController#destroy) to expire the access_token cookie server-side (publicly accessible, CSRF-protected, and skips refresh_token_cookie).
• Update handleLogout to revoke the cookie and clear client state before calling signoutRedirect, and remove the ineffective js-cookie deletion.
• Add request and Playwright coverage to ensure the cookie is actually cleared and subresource requests no longer authenticate post-logout.

File summaries

File	Description
app/controllers/access_token_controller.rb	Adds a server endpoint to delete the access_token cookie and marks it publicly accessible while skipping cookie refresh.
config/routes.rb	Routes DELETE /access_token to the new controller action.
client/app/utilities/authentication.ts	Adds revokeAccessTokenCookie() (CSRF token fetch + DELETE) to expire the cookie from the client.
client/app/lib/components/wrappers/AuthProvider.tsx	Reorders logout to revoke cookie + clear storage before signoutRedirect; removes js-cookie usage.
spec/requests/access_token_spec.rb	Adds request specs for cookie deletion / no-credential behavior / no-mint behavior.
tests/tests/courses/landing-page-auth.spec.ts	Adds Playwright regression tests verifying cookie removal and no post-logout auth for subresource requests.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

It changes an authentication/logout flow across client+server boundaries (including CSRF and SSO redirect sequencing), which is security-sensitive and warrants final human verification.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite