mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
fix(csp): thirteen controls rendered fine and did nothing when used
Reported against the log level toggle added in v0.1.44, and it turned out
to be twelve older ones plus that.
Executing inline event handler violates the following Content Security
Policy directive 'script-src 'self' 'nonce-…'
`script-src` is enforced in production with a nonce, and the spec is
explicit that NEITHER a nonce NOR a hash authorises an inline event
handler without `unsafe-hashes`. So every `onchange="this.form.
requestSubmit()"` in this application was refused: the log browser's two
filters, the clock picker, the timezone picker, two compose defaults, two
compose-behaviour radios, the three push delivery filters, a copy field's
onclick, and the new log level select.
WHY IT LASTED
An inline handler violates nothing when the page LOADS. csp.spec.ts walks
every surface asserting a clean load and it was clean — the pages were
fine, the controls were fine, and nothing happened when you touched one.
Silent unless a console was open, and a control that does nothing reads
as "this setting is broken" rather than as a policy refusal.
Tom Select is what surfaced it: it re-dispatches a real `change` on the
hidden original element, which is exactly what fired the handler the
policy then refused — so the console names tom-select as the culprit and
it is nothing of the sort.
THE FIX, AND WHERE THE CONTROLLER GOES
`ui--auto-submit`, served from a file the policy already allows.
`data-controller` on the FORM and `data-action` on the input, because
Stimulus resolves an action against the element or an ancestor — on the
input alone it silently never fires, which is the same failure again.
requestSubmit(), not submit(): it fires the submit event so Turbo sees it
and validation runs, and on the two forms here that opt out with
data-turbo="false" it behaves exactly as submit() did.
`ui--broken-image` for the last one, which was doing something else
entirely: an attachment thumbnail's `onerror="this.remove()"`. Thumbnails
may legitimately not exist — a PDF, a format GD cannot read, one that has
been pruned — and the chip is fine without one, but a broken-image icon
in the middle of it is a defect rather than a fallback. That icon has
been there the whole time.
THE TEST IS STATIC, DELIBERATELY
A browser test would have to drive thirteen widgets, several of them Tom
Select with a hidden original, to state one rule. And the load-time walk
already exists and already passed. So the rule is asserted directly:
no template carries an inline event handler, with the message pointing at
the controller to use instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
672b38d6c2
commit
b275265d07
11 files changed
+214
-24
No files matched your search
@@ -0,0 +1,64 @@
|
||||
import { Controller } from "@hotwired/stimulus";
|
||||
|
||||
/**
|
||||
* Submit the form when a control in it changes.
|
||||
*
|
||||
* WHY THIS EXISTS RATHER THAN `onchange="this.form.requestSubmit()"`
|
||||
* ─────────────────────────────────────────────────────────────────
|
||||
* That is what ten of these were, and under the enforced Content Security
|
||||
* Policy none of them worked. Inline event handlers cannot be authorised by a
|
||||
* nonce or a hash — the spec is explicit that neither applies to them without
|
||||
* `unsafe-hashes` — so every one was refused with:
|
||||
*
|
||||
* Executing inline event handler violates the following Content Security
|
||||
* Policy directive 'script-src 'self' 'nonce-…''
|
||||
*
|
||||
* It failed quietly. The control moved, the page did not, and nothing said why
|
||||
* unless a console was open — which is why it survived in the log browser's
|
||||
* filters, the clock and timezone pickers, the compose defaults and the push
|
||||
* delivery filter for as long as it did.
|
||||
*
|
||||
* A Stimulus action is the same behaviour from a file the policy already
|
||||
* allows.
|
||||
*
|
||||
* PUT THE CONTROLLER ON THE FORM
|
||||
* ──────────────────────────────
|
||||
* Stimulus resolves an action against the element or an ancestor, and the
|
||||
* control that changes is inside the form — so `data-controller` belongs on the
|
||||
* `<form>` and `data-action` on the input. On the input alone, the action
|
||||
* silently never fires, which is the same failure this replaces.
|
||||
*/
|
||||
export default class extends Controller {
|
||||
submit(event) {
|
||||
const form = event.target?.form ?? this.element.closest("form") ?? this.element;
|
||||
|
||||
if (false === form instanceof HTMLFormElement) {
|
||||
return;
|
||||
}
|
||||
|
||||
// requestSubmit(), not submit(): it fires the submit event, so Turbo
|
||||
// sees it and constraint validation still runs. On the two forms here
|
||||
// that opt out of Turbo with data-turbo="false" it behaves exactly as
|
||||
// submit() did.
|
||||
if ("function" === typeof form.requestSubmit) {
|
||||
form.requestSubmit();
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
form.submit();
|
||||
}
|
||||
|
||||
/**
|
||||
* Select the whole value, for a read-only field somebody is about to copy.
|
||||
*
|
||||
* Here rather than in a controller of its own because it is the same
|
||||
* problem — `onclick="this.select()"` is an inline handler the policy
|
||||
* refuses — and one more file for one more line would be worse than a
|
||||
* second verb on a controller that is already about "a control was touched,
|
||||
* do the obvious thing".
|
||||
*/
|
||||
selectAll(event) {
|
||||
event.target?.select?.();
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,24 @@
|
||||
import { Controller } from "@hotwired/stimulus";
|
||||
|
||||
/**
|
||||
* Take an image off the page when it fails to load.
|
||||
*
|
||||
* For the attachment thumbnails, which are generated lazily and may legitimately
|
||||
* not exist: a PDF, a format GD cannot read, a file whose thumbnail has been
|
||||
* pruned. The chip is perfectly good without one — it falls back to a paperclip
|
||||
* — but a broken-image icon in the middle of it is not a fallback, it is a
|
||||
* defect.
|
||||
*
|
||||
* This was `onerror="this.remove()"`, which the enforced CSP refuses: a nonce
|
||||
* cannot authorise an inline event handler. So the icon stayed. It is the kind
|
||||
* of thing nobody reports, because a broken thumbnail looks like a broken
|
||||
* attachment rather than like a bug in the page.
|
||||
*
|
||||
* `error` does not bubble, and does not need to — Stimulus binds the listener
|
||||
* to the element carrying the action, which is the image itself.
|
||||
*/
|
||||
export default class extends Controller {
|
||||
remove() {
|
||||
this.element.remove();
|
||||
}
|
||||
}
|
||||
@@ -19,11 +19,11 @@
|
||||
{# Its own row in a narrow pane, where the two selects plus Refresh
|
||||
are wider than it and were being clipped by its overflow-hidden. #}
|
||||
<form method="get" action="{{ path('app_admin_logs') }}"
|
||||
class="w-full @2xl:w-auto @2xl:ml-auto flex flex-wrap items-center gap-2">
|
||||
class="w-full @2xl:w-auto @2xl:ml-auto flex flex-wrap items-center gap-2" data-controller="ui--auto-submit">
|
||||
<select name="level"
|
||||
{{ select.enhance() }}
|
||||
class="select-field h-8 text-xs"
|
||||
onchange="this.form.requestSubmit();">
|
||||
data-action="change->ui--auto-submit#submit">
|
||||
{% for value, name in levels %}
|
||||
<option value="{{ value }}" {{ minLevel == value ? 'selected' : '' }}>≥ {{ name }}</option>
|
||||
{% endfor %}
|
||||
@@ -31,7 +31,7 @@
|
||||
<select name="channel"
|
||||
{{ select.enhance() }}
|
||||
class="select-field h-8 text-xs"
|
||||
onchange="this.form.requestSubmit();">
|
||||
data-action="change->ui--auto-submit#submit">
|
||||
<option value="">{{ 'admin.logs.all_channels'|trans }}</option>
|
||||
{% for ch in channels %}
|
||||
<option value="{{ ch }}" {{ channel == ch ? 'selected' : '' }}>{{ ch }}</option>
|
||||
@@ -46,7 +46,7 @@
|
||||
|
||||
{# Deletes the current filter's entries, not the whole table. #}
|
||||
<form method="post" action="{{ path('app_admin_logs_clear') }}" data-turbo-frame="_top"
|
||||
data-turbo-confirm="{{ 'admin.logs.clear_confirm'|trans({ '%count%': total }) }}">
|
||||
data-turbo-confirm="{{ 'admin.logs.clear_confirm'|trans({ '%count%': total }) }}" data-controller="ui--auto-submit">
|
||||
<input type="hidden" name="_token" value="{{ csrf_token('admin_logs_clear') }}">
|
||||
<input type="hidden" name="level" value="{{ minLevel }}">
|
||||
<input type="hidden" name="channel" value="{{ channel }}">
|
||||
@@ -74,12 +74,12 @@
|
||||
<span class="text-xs text-ink-muted">{{ 'admin.logs.capture.label'|trans }}</span>
|
||||
|
||||
<form method="post" action="{{ path('app_admin_logs_level') }}" data-turbo-frame="_top"
|
||||
class="flex items-center gap-2">
|
||||
class="flex items-center gap-2" data-controller="ui--auto-submit">
|
||||
<input type="hidden" name="_token" value="{{ csrf_token('admin_logs_level') }}">
|
||||
<select name="capture"
|
||||
{{ select.enhance() }}
|
||||
class="select-field h-8 text-xs"
|
||||
onchange="this.form.requestSubmit();">
|
||||
data-action="change->ui--auto-submit#submit">
|
||||
{# The empty option is how an install gets back to following
|
||||
its environment once a level has been chosen here — see
|
||||
LogSettings on why that is not the same as storing the
|
||||
|
||||
@@ -46,11 +46,11 @@ deliberately not stored.
|
||||
{# Its own row in a narrow pane: three selects are wider than the
|
||||
heading beside them and were clipped by the overflow-hidden. #}
|
||||
<form method="get" action="{{ path('app_admin_push_deliveries') }}"
|
||||
class="w-full @2xl:w-auto @2xl:ml-auto flex flex-wrap items-center gap-2">
|
||||
class="w-full @2xl:w-auto @2xl:ml-auto flex flex-wrap items-center gap-2" data-controller="ui--auto-submit">
|
||||
<select name="usr"
|
||||
{{ select.enhance() }}
|
||||
class="select-field h-8 text-xs"
|
||||
onchange="this.form.requestSubmit();">
|
||||
data-action="change->ui--auto-submit#submit">
|
||||
<option value="">{{ 'admin.push.deliveries.all_users'|trans }}</option>
|
||||
{% for candidate in users %}
|
||||
<option value="{{ candidate.id }}" {{ usr == candidate.id ? 'selected' : '' }}>{{ candidate.email }}</option>
|
||||
@@ -59,7 +59,7 @@ deliberately not stored.
|
||||
<select name="transport"
|
||||
{{ select.enhance() }}
|
||||
class="select-field h-8 text-xs"
|
||||
onchange="this.form.requestSubmit();">
|
||||
data-action="change->ui--auto-submit#submit">
|
||||
<option value="">{{ 'admin.push.deliveries.all_transports'|trans }}</option>
|
||||
{% for case in transports %}
|
||||
<option value="{{ case.value }}" {{ transport and transport.value == case.value ? 'selected' : '' }}>
|
||||
@@ -70,7 +70,7 @@ deliberately not stored.
|
||||
<select name="outcome"
|
||||
{{ select.enhance() }}
|
||||
class="select-field h-8 text-xs"
|
||||
onchange="this.form.requestSubmit();">
|
||||
data-action="change->ui--auto-submit#submit">
|
||||
<option value="">{{ 'admin.push.deliveries.all_outcomes'|trans }}</option>
|
||||
{% for case in outcomes %}
|
||||
<option value="{{ case.value }}" {{ outcome and outcome.value == case.value ? 'selected' : '' }}>
|
||||
|
||||
@@ -68,7 +68,10 @@ console to say why. Turbo leaves a link carrying `download` to the browser.
|
||||
alt=""
|
||||
loading="lazy"
|
||||
decoding="async"
|
||||
onerror="this.remove()"
|
||||
{# A thumbnail may legitimately not exist — see ui--broken-image.
|
||||
Was an inline onerror, which the policy refuses to run. #}
|
||||
data-controller="ui--broken-image"
|
||||
data-action="error->ui--broken-image#remove"
|
||||
class="w-8 h-8 shrink-0 rounded object-cover bg-sunken">
|
||||
{% else %}
|
||||
<i class="fa-solid fa-paperclip text-[11px]" aria-hidden="true"></i>
|
||||
|
||||
@@ -21,14 +21,14 @@ Variables:
|
||||
#}
|
||||
{% import '_partials/_select.html.twig' as select %}
|
||||
<div class="px-4 py-3">
|
||||
<form method="post" action="{{ path('app_settings_clock_update') }}" data-turbo="false">
|
||||
<form method="post" action="{{ path('app_settings_clock_update') }}" data-turbo="false" data-controller="ui--auto-submit">
|
||||
<input type="hidden" name="_token" value="{{ csrf_token('settings-clock') }}">
|
||||
|
||||
<label for="settings-clock" class="sr-only">{{ 'settings.clock.label'|trans }}</label>
|
||||
|
||||
<select id="settings-clock"
|
||||
name="clock"
|
||||
onchange="this.form.requestSubmit();"
|
||||
data-action="change->ui--auto-submit#submit"
|
||||
{{ select.enhance() }}
|
||||
class="select-field w-full h-9 text-sm">
|
||||
<option value="" {{ activeClock is null ? 'selected' : '' }}>
|
||||
|
||||
@@ -27,7 +27,7 @@ idiom every boolean on the appearance page wears.
|
||||
so the four cards on this page have one rhythm. #}
|
||||
<div class="px-4 py-3">
|
||||
|
||||
<form method="post" action="{{ path('app_settings_compose_behavior_update') }}" data-turbo="false">
|
||||
<form method="post" action="{{ path('app_settings_compose_behavior_update') }}" data-turbo="false" data-controller="ui--auto-submit">
|
||||
<input type="hidden" name="_token" value="{{ csrf_token('settings-compose-behavior') }}">
|
||||
|
||||
<div class="flex items-start justify-between gap-3">
|
||||
@@ -42,7 +42,7 @@ idiom every boolean on the appearance page wears.
|
||||
<label>
|
||||
<input type="radio" name="forwardQuoteCollapsed" class="peer sr-only"
|
||||
value="{{ folded ? '1' : '0' }}"
|
||||
onchange="this.form.submit()"
|
||||
data-action="change->ui--auto-submit#submit"
|
||||
{{ forwardQuoteCollapsed == folded ? 'checked' : '' }}>
|
||||
<span class="{{ segText }}">
|
||||
{{ (folded ? 'settings.compose_behavior.folded' : 'settings.compose_behavior.open')|trans }}
|
||||
@@ -57,7 +57,7 @@ idiom every boolean on the appearance page wears.
|
||||
control submits on change, and a shared form would post both keys every time
|
||||
either moved. The controller only writes the key it is sent, so the two
|
||||
cannot overwrite each other. #}
|
||||
<form method="post" action="{{ path('app_settings_compose_behavior_update') }}" data-turbo="false" class="mt-4">
|
||||
<form method="post" action="{{ path('app_settings_compose_behavior_update') }}" data-turbo="false" class="mt-4" data-controller="ui--auto-submit">
|
||||
<input type="hidden" name="_token" value="{{ csrf_token('settings-compose-behavior') }}">
|
||||
|
||||
<div class="flex items-start justify-between gap-3">
|
||||
@@ -72,7 +72,7 @@ idiom every boolean on the appearance page wears.
|
||||
<label>
|
||||
<input type="radio" name="sendFeedback" class="peer sr-only"
|
||||
value="{{ option }}"
|
||||
onchange="this.form.submit()"
|
||||
data-action="change->ui--auto-submit#submit"
|
||||
{{ sendFeedback == option ? 'checked' : '' }}>
|
||||
<span class="{{ segText }}">
|
||||
{{ ('settings.compose_behavior.send_' ~ option)|trans }}
|
||||
|
||||
@@ -38,7 +38,7 @@ Variables:
|
||||
<form method="post"
|
||||
action="{{ path('app_compose_defaults_read_receipt_default', { id: account.id }) }}"
|
||||
data-turbo-stream
|
||||
class="flex items-center justify-between gap-3 py-1">
|
||||
class="flex items-center justify-between gap-3 py-1" data-controller="ui--auto-submit">
|
||||
<input type="hidden" name="_token"
|
||||
value="{{ csrf_token('compose-defaults-receipt' ~ account.id) }}">
|
||||
|
||||
@@ -49,7 +49,7 @@ Variables:
|
||||
|
||||
<select id="receipt-default-{{ account.id }}"
|
||||
name="mode"
|
||||
onchange="this.form.requestSubmit()"
|
||||
data-action="change->ui--auto-submit#submit"
|
||||
class="shrink-0 h-9 rounded-lg border border-line bg-surface text-sm px-2 text-ink">
|
||||
{% for mode in modes %}
|
||||
<option value="{{ mode }}" {{ accountMode == mode ? 'selected' : '' }}>
|
||||
@@ -76,7 +76,7 @@ Variables:
|
||||
<form method="post"
|
||||
action="{{ path('app_compose_defaults_read_receipt_alias', { id: account.id, aliasId: alias.id }) }}"
|
||||
data-turbo-stream
|
||||
class="shrink-0">
|
||||
class="shrink-0" data-controller="ui--auto-submit">
|
||||
<input type="hidden" name="_token"
|
||||
value="{{ csrf_token('compose-defaults-receipt-alias' ~ alias.id) }}">
|
||||
|
||||
@@ -86,7 +86,7 @@ Variables:
|
||||
|
||||
<select id="receipt-alias-{{ alias.id }}"
|
||||
name="mode"
|
||||
onchange="this.form.requestSubmit()"
|
||||
data-action="change->ui--auto-submit#submit"
|
||||
class="h-9 rounded-lg border border-line bg-surface text-sm px-2 text-ink">
|
||||
{# The empty option is what "follow the account
|
||||
default" posts, and the controller REMOVES the
|
||||
|
||||
@@ -15,7 +15,7 @@ Variables:
|
||||
#}
|
||||
{% import '_partials/_select.html.twig' as select %}
|
||||
<div class="px-4 py-3">
|
||||
<form method="post" action="{{ path('app_settings_timezone_update') }}" data-turbo="false">
|
||||
<form method="post" action="{{ path('app_settings_timezone_update') }}" data-turbo="false" data-controller="ui--auto-submit">
|
||||
<input type="hidden" name="_token" value="{{ csrf_token('settings-timezone') }}">
|
||||
|
||||
<label for="settings-timezone" class="sr-only">{{ 'settings.timezone.label'|trans }}</label>
|
||||
@@ -26,7 +26,7 @@ Variables:
|
||||
real change event on this element. #}
|
||||
<select id="settings-timezone"
|
||||
name="timezone"
|
||||
onchange="this.form.requestSubmit();"
|
||||
data-action="change->ui--auto-submit#submit"
|
||||
{{ select.enhance() }}
|
||||
class="select-field w-full h-9 text-sm">
|
||||
<option value="" {{ activeTimezone is null ? 'selected' : '' }}>
|
||||
|
||||
@@ -46,7 +46,8 @@ Variables:
|
||||
<input type="text"
|
||||
readonly
|
||||
value="{{ mintedUrl }}"
|
||||
onclick="this.select()"
|
||||
data-controller="ui--auto-submit"
|
||||
data-action="click->ui--auto-submit#selectAll"
|
||||
aria-label="{{ 'calendar.share.minted.heading'|trans }}"
|
||||
class="mt-2 w-full h-9 px-3 rounded-lg text-xs font-mono bg-field border border-field text-ink
|
||||
focus:outline-none focus-visible:ring-2 focus-visible:ring-accent">
|
||||
|
||||
@@ -0,0 +1,98 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Tests\Infrastructure\Templating;
|
||||
|
||||
use PHPUnit\Framework\TestCase;
|
||||
use RecursiveDirectoryIterator;
|
||||
use RecursiveIteratorIterator;
|
||||
use SplFileInfo;
|
||||
|
||||
/**
|
||||
* No template writes an inline event handler, because the policy refuses them.
|
||||
*
|
||||
* `script-src` is enforced in production with a nonce, and the CSP spec is
|
||||
* explicit that neither a nonce nor a hash authorises an inline event handler
|
||||
* without `unsafe-hashes`. So `onchange="this.form.requestSubmit()"` is not a
|
||||
* shortcut that works — it is a control that does nothing:
|
||||
*
|
||||
* Executing inline event handler violates the following Content Security
|
||||
* Policy directive 'script-src 'self' 'nonce-…''
|
||||
*
|
||||
* TWELVE OF THEM SURVIVED, and the reason they survived is the reason this test
|
||||
* is static rather than a browser check. An inline handler violates nothing
|
||||
* when the page loads — it violates when somebody touches the control. The
|
||||
* existing csp.spec.ts walks every surface and asserts a clean load, and it was
|
||||
* clean: the log browser's filters, the clock and timezone pickers, the compose
|
||||
* defaults and the push delivery filter all rendered perfectly and then did
|
||||
* nothing when used. The failure was silent unless a console was open.
|
||||
*
|
||||
* A browser test could be written for it, but it would have to drive each
|
||||
* widget — several are Tom Select, whose original element is hidden — and
|
||||
* would test twelve interactions to state one rule. The rule is what matters:
|
||||
* behaviour goes in a Stimulus controller, which is served from a file the
|
||||
* policy already allows. `ui--auto-submit` exists for the common case.
|
||||
*/
|
||||
final class NoInlineEventHandlersTest extends TestCase
|
||||
{
|
||||
/**
|
||||
* The attributes a browser treats as script. Not an exhaustive list of
|
||||
* every `on*` in the HTML spec — a deliberately short list of the ones
|
||||
* anybody actually reaches for, because a regex broad enough to catch all
|
||||
* of them also catches `data-...on-something` and words in prose.
|
||||
*/
|
||||
private const array HANDLERS = [
|
||||
'onclick', 'onchange', 'oninput', 'onsubmit', 'onfocus', 'onblur',
|
||||
'onkeydown', 'onkeyup', 'onmouseover', 'onmouseout', 'onload', 'onerror',
|
||||
];
|
||||
|
||||
public function testNoTemplateCarriesAnInlineEventHandler(): void
|
||||
{
|
||||
$pattern = '/\s(' . implode('|', self::HANDLERS) . ')\s*=/i';
|
||||
$offences = [];
|
||||
|
||||
foreach ($this->templates() as $file) {
|
||||
$contents = (string) file_get_contents($file->getPathname());
|
||||
|
||||
foreach (explode("\n", $contents) as $number => $line) {
|
||||
if (1 === preg_match($pattern, $line, $matches)) {
|
||||
$offences[] = sprintf(
|
||||
'%s:%d %s=',
|
||||
str_replace(self::projectDir() . '/', '', $file->getPathname()),
|
||||
$number + 1,
|
||||
mb_strtolower($matches[1]),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
self::assertSame(
|
||||
[],
|
||||
$offences,
|
||||
"These are inline event handlers, and the enforced CSP refuses to run them —\n"
|
||||
. "the control will render and then do nothing when somebody uses it.\n"
|
||||
. "Move the behaviour into a Stimulus controller; ui--auto-submit covers\n"
|
||||
. "\"submit the form when this changes\" and \"select all of this on click\".\n",
|
||||
);
|
||||
}
|
||||
|
||||
/** @return iterable<SplFileInfo> */
|
||||
private function templates(): iterable
|
||||
{
|
||||
$iterator = new RecursiveIteratorIterator(
|
||||
new RecursiveDirectoryIterator(self::projectDir() . '/templates'),
|
||||
);
|
||||
|
||||
foreach ($iterator as $file) {
|
||||
if ($file instanceof SplFileInfo && 'twig' === $file->getExtension()) {
|
||||
yield $file;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private static function projectDir(): string
|
||||
{
|
||||
return \dirname(__DIR__, 3);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user