mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
fix(security): require a CSRF token to log out
A plain GET /logout signed the user out, so any page could do it with an image tag. The firewall now checks the stateless `logout` token, and both sign-out controls (user menu, 2FA prompt) are POST forms carrying it. The E2E helpers that navigated to /logout submit the page's form instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
3c0fb20e16
commit
1065ae2aed
8 files changed
+120
-18
No files matched your search
@@ -89,6 +89,12 @@ security:
|
||||
trusted_parameter_name: _trusted
|
||||
logout:
|
||||
path: app_logout
|
||||
# Without a token a plain GET logged you out, so any page on the
|
||||
# internet could do it with an <img src>. The id is `logout`,
|
||||
# stateless in csrf.yaml, so what refuses the forged request is
|
||||
# the same-origin check. Every sign-out control is a POST form
|
||||
# carrying csrf_token('logout').
|
||||
enable_csrf: true
|
||||
|
||||
# Easy way to control access for large sections of your site
|
||||
# Note: Only the *first* access control that matches will be used
|
||||
|
||||
@@ -612,12 +612,16 @@
|
||||
snapshot back on screen with the back button. A full page load
|
||||
is the only way to be sure nothing is left holding a session
|
||||
that was just thrown away. #}
|
||||
<a href="{{ path('app_logout') }}"
|
||||
data-turbo="false"
|
||||
class="flex items-center gap-2 px-4 py-2 text-sm text-ink-soft hover:bg-hover">
|
||||
<i class="fa-solid fa-right-from-bracket w-4 text-gray-400" aria-hidden="true"></i>
|
||||
{{ 'header.user_menu.sign_out'|trans }}
|
||||
</a>
|
||||
{# A POST form, not a link: logout checks a CSRF token, so a
|
||||
cross-site GET can no longer sign anyone out. #}
|
||||
<form method="post" action="{{ path('app_logout') }}" data-turbo="false">
|
||||
<input type="hidden" name="_csrf_token" value="{{ csrf_token('logout') }}">
|
||||
<button type="submit"
|
||||
class="w-full flex items-center gap-2 px-4 py-2 text-sm text-ink-soft hover:bg-hover text-left cursor-pointer">
|
||||
<i class="fa-solid fa-right-from-bracket w-4 text-gray-400" aria-hidden="true"></i>
|
||||
{{ 'header.user_menu.sign_out'|trans }}
|
||||
</button>
|
||||
</form>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
|
||||
@@ -102,12 +102,18 @@
|
||||
</form>
|
||||
</div>
|
||||
|
||||
<p class="text-center text-sm text-ink-muted mt-6">
|
||||
{# A div, not a p: a <p> cannot hold the form below, and the parser
|
||||
would close it early. #}
|
||||
<div class="text-center text-sm text-ink-muted mt-6">
|
||||
{{ 'two_factor.recovery_hint'|trans }}
|
||||
<a href="{{ logoutPath }}" class="block text-accent font-medium hover:underline mt-2">
|
||||
{{ 'two_factor.cancel'|trans }}
|
||||
</a>
|
||||
</p>
|
||||
{# A POST form like the user menu's: logout checks a CSRF token. #}
|
||||
<form method="post" action="{{ path('app_logout') }}" data-turbo="false" class="mt-2">
|
||||
<input type="hidden" name="_csrf_token" value="{{ csrf_token('logout') }}">
|
||||
<button type="submit" class="text-accent font-medium hover:underline cursor-pointer">
|
||||
{{ 'two_factor.cancel'|trans }}
|
||||
</button>
|
||||
</form>
|
||||
</div>
|
||||
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -0,0 +1,73 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Tests\Security;
|
||||
|
||||
use App\Entity\User\User;
|
||||
use Doctrine\ORM\EntityManagerInterface;
|
||||
use Symfony\Bundle\FrameworkBundle\KernelBrowser;
|
||||
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
|
||||
|
||||
/**
|
||||
* Logging out takes a CSRF token, so a GET cannot do it.
|
||||
*
|
||||
* Before enable_csrf, `GET /logout` signed you out — any page on the internet
|
||||
* could do that to a plMail user with an image tag. The user menu's control is
|
||||
* a POST form carrying csrf_token('logout'), and that one still works.
|
||||
*/
|
||||
final class LogoutCsrfTest extends WebTestCase
|
||||
{
|
||||
private KernelBrowser $client;
|
||||
private EntityManagerInterface $em;
|
||||
|
||||
protected function setUp(): void
|
||||
{
|
||||
$this->client = static::createClient();
|
||||
$this->client->disableReboot();
|
||||
|
||||
$this->em = static::getContainer()->get(EntityManagerInterface::class);
|
||||
$this->em->getConnection()->beginTransaction();
|
||||
|
||||
$user = new User();
|
||||
$user->email = 'logout-'.bin2hex(random_bytes(6)).'@plmail.test';
|
||||
$user->nameFirst = 'Log';
|
||||
$user->nameLast = 'Out';
|
||||
$user->password = 'not-used';
|
||||
$this->em->persist($user);
|
||||
$this->em->flush();
|
||||
|
||||
$this->client->loginUser($user);
|
||||
}
|
||||
|
||||
protected function tearDown(): void
|
||||
{
|
||||
if (true === $this->em->getConnection()->isTransactionActive()) {
|
||||
$this->em->getConnection()->rollBack();
|
||||
}
|
||||
|
||||
parent::tearDown();
|
||||
}
|
||||
|
||||
public function testAGetNoLongerLogsOut(): void
|
||||
{
|
||||
$this->client->request('GET', '/logout');
|
||||
|
||||
$this->client->request('GET', '/mail/inbox');
|
||||
|
||||
self::assertResponseIsSuccessful('a plain GET to /logout ended the session');
|
||||
}
|
||||
|
||||
public function testTheUserMenusFormStillLogsOut(): void
|
||||
{
|
||||
$crawler = $this->client->request('GET', '/mail/inbox');
|
||||
$form = $crawler->filter('form[action="/logout"]')->form();
|
||||
|
||||
$this->client->submit($form);
|
||||
|
||||
$this->client->request('GET', '/mail/inbox');
|
||||
|
||||
self::assertResponseRedirects();
|
||||
self::assertStringContainsString('/login', (string) $this->client->getResponse()->headers->get('Location'));
|
||||
}
|
||||
}
|
||||
@@ -290,7 +290,10 @@ final class TrustedDeviceLifecycleTest extends WebTestCase
|
||||
|
||||
private function signOut(): void
|
||||
{
|
||||
$this->follow(fn () => $this->client->request('GET', '/logout'));
|
||||
// Through the user menu's form: logout takes a CSRF token now.
|
||||
$crawler = $this->client->request('GET', '/mail/inbox');
|
||||
|
||||
$this->follow(fn () => $this->client->submit($crawler->filter('form[action="/logout"]')->form()));
|
||||
}
|
||||
|
||||
private function follow(callable $request): void
|
||||
|
||||
@@ -26,7 +26,7 @@ test.describe("the end of a session", () => {
|
||||
test("logout is a full page load, not a Turbo visit", async ({ page }) => {
|
||||
await page.goto("/mail/inbox");
|
||||
|
||||
await expect(page.locator('a[href*="logout"]').first())
|
||||
await expect(page.locator('form[action="/logout"]').first())
|
||||
.toHaveAttribute("data-turbo", "false");
|
||||
});
|
||||
|
||||
|
||||
@@ -184,6 +184,16 @@ export function mailRow(page: Page, subject: string): Locator {
|
||||
* Drives the real login form at /login and waits for the authenticated
|
||||
* shell to land on the inbox.
|
||||
*/
|
||||
/**
|
||||
* Signs out the way the user menu does: logout takes a CSRF token, so a plain
|
||||
* `page.goto("/logout")` is refused. Submits the page's own logout form, which
|
||||
* the topbar and the code prompt both render, so the request is same-origin.
|
||||
*/
|
||||
export async function logout(page: Page): Promise<void> {
|
||||
await page.locator('form[action="/logout"]').first().evaluate((form) => (form as HTMLFormElement).submit());
|
||||
await expect(page).toHaveURL(/\/login/);
|
||||
}
|
||||
|
||||
export async function login(
|
||||
page: Page,
|
||||
email: string = TEST_USER.email,
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { test, expect, type Page } from "./support/test";
|
||||
import { TEST_USER, consoleCommand, login } from "./support/config";
|
||||
import { TEST_USER, consoleCommand, login, logout } from "./support/config";
|
||||
import { totp } from "./support/totp";
|
||||
import { acceptConfirm } from "./support/confirm";
|
||||
|
||||
@@ -85,7 +85,7 @@ test.describe("two-factor authentication", () => {
|
||||
await login(page);
|
||||
const secret = await enrol(page);
|
||||
|
||||
await page.goto("/logout");
|
||||
await logout(page);
|
||||
|
||||
// Password alone now stops at the code form rather than the inbox.
|
||||
await page.goto("/login");
|
||||
@@ -142,7 +142,7 @@ test.describe("two-factor authentication", () => {
|
||||
await page.goto("/settings?section=security");
|
||||
await expect(page.getByText("this device")).toBeVisible();
|
||||
|
||||
await page.goto("/logout");
|
||||
await logout(page);
|
||||
|
||||
// Remembered: straight to the inbox, no code asked for.
|
||||
await page.goto("/login");
|
||||
@@ -169,7 +169,7 @@ test.describe("two-factor authentication", () => {
|
||||
),
|
||||
).toBeVisible();
|
||||
|
||||
await page.goto("/logout");
|
||||
await logout(page);
|
||||
await page.goto("/login");
|
||||
await page.locator("#inputEmail").fill(TEST_USER.email);
|
||||
await page.locator("#password").fill(TEST_USER.password);
|
||||
@@ -193,7 +193,7 @@ test.describe("two-factor authentication", () => {
|
||||
await login(page);
|
||||
const secret = await enrol(page);
|
||||
|
||||
await page.goto("/logout");
|
||||
await logout(page);
|
||||
await page.goto("/login");
|
||||
await page.locator("#inputEmail").fill(TEST_USER.email);
|
||||
await page.locator("#password").fill(TEST_USER.password);
|
||||
|
||||
Reference in new issue
Block a user