Skip to content

feat(security): enable logout CSRF protection#491

Merged
CybotTM merged 3 commits into
mainfrom
feat/logout-csrf
Jul 2, 2026
Merged

feat(security): enable logout CSRF protection#491
CybotTM merged 3 commits into
mainfrom
feat/logout-csrf

Conversation

@CybotTM

@CybotTM CybotTM commented Jul 2, 2026

Copy link
Copy Markdown
Member

Description

Enable CSRF protection on logout. The old enable_csrf: false carried the comment "because ExtJS frontend uses simple link for logout" — the ExtJS shell is gone, and with the stateless CSRF setup already used by the login form, protecting the plain logout links costs nothing at the UX level while blocking cross-site forced logout.

Mechanics (verified against the installed Symfony 8.1 vendor code): with stateless_token_ids: [authenticate, logout], csrf_token('logout') renders a constant marker and validation is purely origin-based — Sec-Fetch-Site: same-origin (primary), Origin/Referer fallback. A real user clicking a logout link passes; a cross-site <img>/link/form hard-fails.

Type of Change

  • New feature (non-breaking change that adds functionality)

Related Issue

Follow-up to #490 (which removed every other ExtJS reference; this PR retires the last one).

Changes Made

  • security.yaml: enable_csrf: true on the logout listener; comment states the actual mechanism instead of blaming the removed ExtJS frontend.
  • All logout triggers use Symfony's logout_path()/logout_url() — the LogoutUrlGenerator appends _csrf_token automatically and stays correct if the flag is ever toggled: header badge + mobile drawer, 403 page, and the SPA's logoutUrl (command palette).
  • AccessDeniedSubscriber: the remember-me/not-fully-authenticated case now logs out programmatically (Security::logout(false)) instead of redirecting through /logout — a browser following that redirect from an address-bar navigation carries no same-origin fetch metadata and would hit the CSRF wall (the empirical weak spot identified during design).
  • e2e logout flows click the link instead of page.goto() (CDP navigations send Sec-Fetch-Site: none and would fail even though real users pass); the tests also assert the link carries the token.
  • Functional tests send HTTP_SEC_FETCH_SITE: same-origin (BrowserKit emits no fetch metadata); a new test pins the 403 for tokenless logout requests; the remember-me tests assert the new direct-to-login flow.

Testing

  • Unit tests green locally (1557, incl. two new subscriber tests: logout response used as-is + null fallback)
  • phpstan level 10, cs-fixer, phpat, twig lint, lint:container (dev/prod/test) — all green locally
  • Functional + e2e — CI (the test env inherits enable_csrf: true via Symfony's config merge; the click-based e2e logout genuinely exercises CSRF-on logout)

Code Quality

  • Code follows project coding standards
  • Self-review completed
  • Documentation updated (config comment)
  • No breaking changes for real users (bookmarked bare /logout URLs now 403 by design — standard Symfony logout-CSRF behavior)

Migration Notes

None for deployments. Users who bookmarked the bare /logout URL get a 403; logging out via the UI works unchanged.

Checklist

Copilot AI review requested due to automatic review settings July 2, 2026 01:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enables CSRF protection for the logout flow in the Symfony security firewall, updates all logout links/URLs to use Symfony’s logout URL/path generators (so the CSRF token is appended automatically), and adjusts backend + test behavior to avoid CSRF failures in edge cases (notably remember-me users).

Changes:

  • Enable enable_csrf: true for the main firewall’s logout listener and document the stateless/same-origin validation mechanism.
  • Replace direct _logout route URL generation in Twig/SPA config with logout_path() / logout_url() so logout links include _csrf_token.
  • Update the remember-me access-denied flow to log out programmatically (bypassing the CSRF-guarded /logout round-trip) and adapt functional/e2e tests accordingly.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
config/packages/security.yaml Enables logout CSRF protection and documents the stateless same-origin validation behavior.
src/EventSubscriber/AccessDeniedSubscriber.php Switches remember-me “not fully authenticated” handling to programmatic logout with login redirect fallback.
templates/ui/index.html.twig Injects a CSRF-tokenized logout URL into SPA config via logout_url().
templates/partials/header.html.twig Updates header + drawer logout links to logout_path() (CSRF-tokenized).
templates/bundles/TwigBundle/Exception/error403.html.twig Updates 403 page logout button to logout_path() (CSRF-tokenized).
tests/Security/RememberMeRedirectTest.php Adds/logout CSRF expectations for BrowserKit (403 without token; same-origin header + token for success) and adjusts remember-me redirect expectation.
tests/EventSubscriber/AccessDeniedSubscriberTest.php Updates unit tests to assert programmatic logout behavior and fallback redirect.
tests/Controller/SecurityControllerTest.php Adapts logout test to include same-origin fetch metadata header for stateless CSRF validation.
e2e/login.spec.ts Changes logout e2e to click the logout link and assert it contains _csrf_token.
e2e/helpers/auth.ts Changes shared e2e logout helper to click the logout link (vs page.goto).

Comment thread config/packages/security.yaml
Comment thread tests/Security/RememberMeRedirectTest.php

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enables stateless CSRF protection for the logout endpoint. It updates templates to use logout_path() and logout_url() to append the CSRF token, and updates end-to-end and integration tests to simulate same-origin navigations (using clicks or passing the Sec-Fetch-Site header) to satisfy the stateless CSRF validation. Additionally, the AccessDeniedSubscriber is updated to perform programmatic logouts instead of redirecting to /logout to avoid failing CSRF checks on redirects. I have no further feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.15%. Comparing base (671421f) to head (b431fb3).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #491      +/-   ##
============================================
+ Coverage     84.14%   84.15%   +0.01%     
  Complexity     2814     2814              
============================================
  Files           187      187              
  Lines          7588     7588              
============================================
+ Hits           6385     6386       +1     
+ Misses         1203     1202       -1     
Flag Coverage Δ
integration 53.14% <50.00%> (+0.01%) ⬆️
unit 49.51% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

CybotTM added a commit that referenced this pull request Jul 2, 2026
Review round on #491 plus the Integration Tests failure it exposed:

- error403.html.twig: logout_path() needs firewall/request context. The
  page renders through the global error handler (error_controller is
  null, so the kernel rethrows for HTML and the ErrorHandler renders
  outside any request) — TwigErrorRenderer has no fallback, so every
  prod 403 would have become a raw 500. Append the stateless
  _csrf_token manually instead; verified by rendering the template
  against a booted test kernel with an empty request stack.
- RememberMeRedirectTest: assert the wrapped AccessDeniedHttpException
  for tokenless logout (with error_controller: null there is no 403
  response inside the kernel to assert); rename the remember-me test to
  match the logged-out-not-redirected behavior; hoist repeated literals
  to constants and drop the generic throw from the new helper
  (SonarCloud S1192/S112); PHPUnit 13: expectExceptionMessage is
  deprecated — use try/catch.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM added 2 commits July 2, 2026 07:40
The logout listener now validates a CSRF token, blocking cross-site
forced logout (any <img>/link on a foreign origin pointing at /logout).
With the stateless token setup (framework.csrf_protection,
stateless_token_ids: [authenticate, logout]) validity comes from the
Sec-Fetch-Site/Origin/Referer headers of a genuine same-origin
navigation — the same mechanism the login form already uses.

- security.yaml: enable_csrf: true on logout.
- Templates use logout_path()/logout_url(): Symfony's LogoutUrlGenerator
  appends the _csrf_token automatically (and stays correct if the flag
  is ever toggled) — header badge + drawer, 403 page, and the SPA's
  logoutUrl (command palette).
- AccessDeniedSubscriber: the remember-me/not-fully-authenticated case
  now logs out programmatically (Security::logout(false)) instead of
  redirecting through /logout — a browser following that redirect from
  an address-bar navigation carries no same-origin fetch metadata and
  would hit the CSRF wall. Unit tests cover both the logout response
  and the null fallback.
- e2e logout flows click the link instead of page.goto(): CDP-initiated
  navigations send Sec-Fetch-Site: none and would fail CSRF even though
  real users pass; the tests also assert the link carries the token.
- Functional tests send HTTP_SEC_FETCH_SITE: same-origin (BrowserKit
  emits no fetch metadata) and a new test pins the 403 for tokenless
  logout requests.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Review round on #491 plus the Integration Tests failure it exposed:

- error403.html.twig: logout_path() needs firewall/request context. The
  page renders through the global error handler (error_controller is
  null, so the kernel rethrows for HTML and the ErrorHandler renders
  outside any request) — TwigErrorRenderer has no fallback, so every
  prod 403 would have become a raw 500. Append the stateless
  _csrf_token manually instead; verified by rendering the template
  against a booted test kernel with an empty request stack.
- RememberMeRedirectTest: assert the wrapped AccessDeniedHttpException
  for tokenless logout (with error_controller: null there is no 403
  response inside the kernel to assert); rename the remember-me test to
  match the logged-out-not-redirected behavior; hoist repeated literals
  to constants and drop the generic throw from the new helper
  (SonarCloud S1192/S112); PHPUnit 13: expectExceptionMessage is
  deprecated — use try/catch.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
@CybotTM CybotTM force-pushed the feat/logout-csrf branch from bbd9ece to 59d8b2c Compare July 2, 2026 05:40
SonarCloud php:S112 on the two remaining generic RuntimeException
throws (pulled into new-code scope by the constant hoist) — use the
same assert() invariant pattern as the token helper.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
@sonarqubecloud

sonarqubecloud Bot commented Jul 2, 2026

Copy link
Copy Markdown

@CybotTM CybotTM merged commit 441ce91 into main Jul 2, 2026
26 checks passed
@CybotTM CybotTM deleted the feat/logout-csrf branch July 2, 2026 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants