mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
fix(mail): the message frame can measure itself again
Reported from a live instance: message bodies stopped resizing, with "Executing inline script violates the following Content Security Policy directive 'script-src 'self' 'nonce-…'" on every open. A srcdoc frame inherits the EMBEDDING page's Content-Security-Policy on top of the <meta> one it carries itself, so the height reporter inside it — the only reason `allow-scripts` is on that sandbox at all — has to satisfy both. MessageRenderer minted a nonce of its own, which the frame's policy named and the page's policy had never heard of. The moment the application itself grew a CSP, the script was blocked by the inherited one and every message sat at its 80px floor with a scrollbar inside a scrollbar. The error names the policy that did it: 'self' appears in the page's script-src and never in the frame's. It uses the request's nonce now. Nothing is given away by sharing it — the nonce is already in that document, beside the email's own markup, and the whole point of a nonce is that markup which did not come from us cannot name it. Asking CspNonce for the value is also what tells the response listener the header must carry one. WHY THE SUITE DID NOT CATCH IT, which matters more than the fix. Under APP_DEBUG the full policy is sent Report-Only — deliberately: the profiler injects inline scripts nothing can nonce. Report-Only does not block. The browser stack runs debug, so message-frame-height.spec.ts measured a frame that worked perfectly there while being blocked in production, where the same policy is ENFORCED. Verified: that spec passes with this fix reverted. So the guard is a unit test. It asserts the invariant the enforced policy depends on — the frame's nonce IS the request's — which is true or false whichever header the environment happens to send. It fails without the fix. Also the two deprecation warnings from the same console: Turbo.config.forms.confirm replaces the removed-in-future Turbo.setConfirmMethod, and mobile-web-app-capable joins the Apple- prefixed meta rather than replacing it — older iOS is the platform that tag was written for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
016f445444
commit
ff29d7ba7d
5 files changed
+124
-20
No files matched your search
+4
-1
@@ -45,7 +45,10 @@ export function askConfirm(message) {
|
||||
return Promise.resolve(window.confirm(message));
|
||||
}
|
||||
|
||||
Turbo.setConfirmMethod(askConfirm);
|
||||
// `Turbo.config.forms.confirm`, not the top-level setConfirmMethod: that one is
|
||||
// deprecated in Turbo 8 and warns on every page load, twice, which is two lines
|
||||
// of console noise standing between a reader and a real error.
|
||||
Turbo.config.forms.confirm = askConfirm;
|
||||
|
||||
/**
|
||||
* The same guarantee for forms that opt OUT of Turbo.
|
||||
|
||||
@@ -6,6 +6,7 @@ namespace App\Service\Mail;
|
||||
|
||||
use App\Domain\Enum\Mail\LabelRole;
|
||||
use App\Entity\Mail\Message;
|
||||
use App\Security\Csp\CspNonce;
|
||||
use App\Entity\User\User;
|
||||
use App\Repository\Mail\TrustedImageSenderRepository;
|
||||
use Symfony\Bundle\SecurityBundle\Security;
|
||||
@@ -60,6 +61,7 @@ final readonly class MessageRenderer
|
||||
private TrustedImageSenderRepository $trustedSenders,
|
||||
private Security $security,
|
||||
private RequestStack $requestStack,
|
||||
private CspNonce $cspNonce,
|
||||
) {
|
||||
}
|
||||
|
||||
@@ -90,7 +92,21 @@ final readonly class MessageRenderer
|
||||
// provider disagreeing about whether this really is that sender, and
|
||||
// the safe reading of a disagreement is the cautious one.
|
||||
$allow = (true === $trusted || true === $forceImages) && false === $inSpam;
|
||||
$nonce = bin2hex(random_bytes(16));
|
||||
// The REQUEST's nonce, not one of this renderer's own.
|
||||
//
|
||||
// A srcdoc frame inherits the embedding document's Content-Security-
|
||||
// Policy on top of its own <meta> one, so the height reporter below has
|
||||
// to satisfy both. It used to carry a nonce minted here, which the
|
||||
// frame's own policy named and the page's policy had never heard of —
|
||||
// so the moment the application itself grew a CSP, every message frame
|
||||
// stopped measuring itself and sat at its 80px floor.
|
||||
//
|
||||
// Nothing is given away by sharing it. The nonce is already in this
|
||||
// document, where the email's own markup sits beside it, and the whole
|
||||
// point of a nonce is that markup which did not come from us cannot
|
||||
// name it. Asking CspNonce for the value is also what tells the response
|
||||
// listener the header must carry it.
|
||||
$nonce = $this->cspNonce->value();
|
||||
|
||||
// The blocker settles what the body may load; the collapser then folds
|
||||
// its trailing reply-history behind a "Show quoted text" toggle. Both
|
||||
|
||||
@@ -86,6 +86,12 @@
|
||||
same 'ajax' id — until this tag existed they all sent an empty string. #}
|
||||
<meta name="csrf-token" content="{{ csrf_token('ajax') }}">
|
||||
<meta name="theme-color" content="#a21caf">
|
||||
{# Both spellings. `mobile-web-app-capable` is the standardised one every
|
||||
engine now reads and the only one Chrome does not warn about; the Apple
|
||||
prefix stays because older iOS is the platform that actually needs it,
|
||||
and dropping it would take the home-screen behaviour with it on exactly
|
||||
the devices the tag was written for. #}
|
||||
<meta name="mobile-web-app-capable" content="yes">
|
||||
<meta name="apple-mobile-web-app-capable" content="yes">
|
||||
<meta name="apple-mobile-web-app-title" content="plMail">
|
||||
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Tests\Service\Mail;
|
||||
|
||||
use App\Entity\Mail\Message;
|
||||
use App\Security\Csp\CspNonce;
|
||||
use App\Service\Mail\MessageRenderer;
|
||||
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
|
||||
|
||||
/**
|
||||
* The message frame's script carries the nonce the PAGE's policy names.
|
||||
*
|
||||
* The body is rendered into a `srcdoc` iframe, and a srcdoc document inherits
|
||||
* the embedding page's Content-Security-Policy on top of the <meta> one it
|
||||
* carries itself. So the ~30 lines of height reporting inside it — the only
|
||||
* reason `allow-scripts` is on that sandbox at all — have to satisfy both
|
||||
* policies at once.
|
||||
*
|
||||
* They did not. MessageRenderer minted a nonce of its own, which the frame's
|
||||
* own policy named and the page's policy had never heard of, so the moment the
|
||||
* application itself grew a CSP the script was blocked by the inherited one:
|
||||
* every message sat at its 80px floor with a scrollbar inside a scrollbar, and
|
||||
* the console said so on every open.
|
||||
*
|
||||
* ── Why nothing caught it ────────────────────────────────────────────────────
|
||||
*
|
||||
* The browser suite runs with APP_DEBUG=1, and under debug the full policy is
|
||||
* sent as Report-Only — deliberately, because the profiler injects inline
|
||||
* scripts nothing can nonce. Report-Only does not block, so the frame measured
|
||||
* itself perfectly in every test while being blocked in production, where the
|
||||
* same policy is ENFORCED. message-frame-height.spec.ts passes either way.
|
||||
*
|
||||
* That gap is the reason this test is a unit test rather than a browser one: it
|
||||
* asserts the INVARIANT the enforced policy depends on, which is true or false
|
||||
* regardless of which header the environment happens to send.
|
||||
*/
|
||||
final class MessageFrameNonceTest extends KernelTestCase
|
||||
{
|
||||
public function testTheFrameIsNoncedWithTheRequestsOwnNonce(): void
|
||||
{
|
||||
self::bootKernel();
|
||||
|
||||
$container = self::getContainer();
|
||||
$renderer = $container->get(MessageRenderer::class);
|
||||
$nonce = $container->get(CspNonce::class);
|
||||
|
||||
$message = new Message();
|
||||
$message->subject = 'Nonce';
|
||||
$message->fromAddress = 'sender@example.test';
|
||||
$message->bodyHtmlSafe = '<p>Body</p>';
|
||||
$message->flags = [];
|
||||
|
||||
$render = $renderer->render($message);
|
||||
|
||||
self::assertSame(
|
||||
$nonce->value(),
|
||||
$render->nonce,
|
||||
'the frame carries a nonce the page policy cannot authorise',
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* And the frame's own policy names that same one, so neither half of the
|
||||
* pair can drift without the other.
|
||||
*/
|
||||
public function testTheFramesOwnPolicyNamesTheSameNonce(): void
|
||||
{
|
||||
self::bootKernel();
|
||||
|
||||
$container = self::getContainer();
|
||||
$renderer = $container->get(MessageRenderer::class);
|
||||
|
||||
$message = new Message();
|
||||
$message->subject = 'Nonce';
|
||||
$message->fromAddress = 'sender@example.test';
|
||||
$message->bodyHtmlSafe = '<p>Body</p>';
|
||||
$message->flags = [];
|
||||
|
||||
$render = $renderer->render($message);
|
||||
|
||||
self::assertStringContainsString(
|
||||
sprintf("'nonce-%s'", $render->nonce),
|
||||
$render->csp,
|
||||
"the frame's policy does not name the nonce its script carries",
|
||||
);
|
||||
}
|
||||
}
|
||||
+8
-18
@@ -1,6 +1,7 @@
|
||||
import { test, expect, type Page } from "./support/test";
|
||||
import { INBOX_SUBJECTS, mailRow, seed } from "./support/config";
|
||||
import { settled } from "./support/motion";
|
||||
import { rowAction } from "./support/rows";
|
||||
|
||||
/**
|
||||
* Runs authenticated as this worker's own user, signed in by the worker
|
||||
@@ -78,9 +79,7 @@ test.describe("mail UI actions", () => {
|
||||
const row = mailRow(page, INBOX_SUBJECTS.archive);
|
||||
await expect(row).toBeVisible();
|
||||
|
||||
// Hover-only actions need the row hovered first.
|
||||
await row.hover();
|
||||
await row.getByRole("button", { name: "Archive", exact: true }).click();
|
||||
await rowAction(row, "Archive");
|
||||
|
||||
// _archive.stream removes the row.
|
||||
await expect(row).toHaveCount(0);
|
||||
@@ -111,8 +110,7 @@ test.describe("mail UI actions", () => {
|
||||
const row = mailRow(page, INBOX_SUBJECTS.archive);
|
||||
await expect(row).toBeVisible();
|
||||
|
||||
await row.hover();
|
||||
await row.getByRole("button", { name: "Archive", exact: true }).click();
|
||||
await rowAction(row, "Archive");
|
||||
await expect(row).toHaveCount(0);
|
||||
|
||||
await page.goto("/mail/archive");
|
||||
@@ -149,8 +147,7 @@ test.describe("mail UI actions", () => {
|
||||
const row = mailRow(page, INBOX_SUBJECTS.trash);
|
||||
await expect(row).toBeVisible();
|
||||
|
||||
await row.hover();
|
||||
await row.getByRole("button", { name: "Delete", exact: true }).click();
|
||||
await rowAction(row, "Delete");
|
||||
|
||||
// _delete.stream removes the row from the inbox.
|
||||
await expect(row).toHaveCount(0);
|
||||
@@ -171,8 +168,6 @@ test.describe("mail UI actions", () => {
|
||||
await expect(row).toBeVisible();
|
||||
await expect(row).toHaveAttribute("data-unread", "true");
|
||||
|
||||
await row.hover();
|
||||
|
||||
// Wait for the status POST itself, then reload — the read-state stream
|
||||
// has a known template typo (flagged separately), so assert on the
|
||||
// persisted outcome rather than the inline swap.
|
||||
@@ -181,7 +176,7 @@ test.describe("mail UI actions", () => {
|
||||
r.request().method() === "POST" &&
|
||||
/\/status\/thread\/\d+\/read$/.test(r.url()),
|
||||
);
|
||||
await row.getByRole("button", { name: "Mark as read", exact: true }).click();
|
||||
await rowAction(row, "Mark as read");
|
||||
await readPost;
|
||||
|
||||
await page.reload();
|
||||
@@ -213,11 +208,10 @@ test.describe("mail UI actions", () => {
|
||||
// Step 1 — mark read.
|
||||
let row = mailRow(page, subject);
|
||||
await expect(row).toHaveAttribute("data-unread", "true");
|
||||
await row.hover();
|
||||
let post = page.waitForResponse(
|
||||
(r) => r.request().method() === "POST" && readEndpoint.test(r.url()),
|
||||
);
|
||||
await row.getByRole("button", { name: "Mark as read", exact: true }).click();
|
||||
await rowAction(row, "Mark as read");
|
||||
await post;
|
||||
await page.reload();
|
||||
await settled(page);
|
||||
@@ -225,13 +219,10 @@ test.describe("mail UI actions", () => {
|
||||
// Step 2 — the same row now offers "Mark as unread".
|
||||
row = mailRow(page, subject);
|
||||
await expect(row).toHaveAttribute("data-unread", "false");
|
||||
await row.hover();
|
||||
post = page.waitForResponse(
|
||||
(r) => r.request().method() === "POST" && readEndpoint.test(r.url()),
|
||||
);
|
||||
await row
|
||||
.getByRole("button", { name: "Mark as unread", exact: true })
|
||||
.click();
|
||||
await rowAction(row, "Mark as unread");
|
||||
await post;
|
||||
await page.reload();
|
||||
|
||||
@@ -526,8 +517,7 @@ test.describe("mail UI actions", () => {
|
||||
await settled(page);
|
||||
|
||||
const row = mailRow(page, INBOX_SUBJECTS.read);
|
||||
await row.hover();
|
||||
await row.getByRole("button", { name: "Snooze" }).click();
|
||||
await rowAction(row, "Snooze");
|
||||
|
||||
const menu = row.locator('[data-ui--dropdown-target="menu"]');
|
||||
await expect(menu).toBeVisible();
|
||||
|
||||
Reference in new issue
Block a user