mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
fix(security): publish every Mercure update as private
All notifiers published public updates, which the hub delivers to any subscriber that names the topic, so the per-user topic grant in the subscriber cookie protected nothing: a signed-in user could read another user's mail/user/<id> stream. Updates now go through UserUpdate::create(), which always sets private; the cookie already grants exactly that topic. Corrects CODE-REVIEW.md S-06, which claimed the isolation held. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
b9246c8b47
commit
75750e3ebc
11 files changed
+103
-17
No files matched your search
+3
-1
@@ -39,7 +39,7 @@ ohne Test heißt hier „behoben, bis es jemand versehentlich zurückdreht".
|
||||
| **S-03** | ✅ behoben | `a4d09b8` | In Produktion **erzwungen**, nicht nur Report-Only — siehe unten, warum das möglich wurde. |
|
||||
| **S-04** | ✅ behoben | `8c3080d` | Nicht-Bilder fallen auf den selbst erzeugten Platzhalter zurück, nicht auf einen Download. |
|
||||
| **S-05** | ✅ behoben | `8c3080d` | Header-Block im Caddyfile, mit `defer`, damit die strengeren Per-Response-Policies gewinnen. |
|
||||
| **S-06** | ✅ behoben | `8c3080d` | `subscribe: []`. |
|
||||
| **S-06** | ✅ behoben | `8c3080d`, siehe Nachtrag | `subscribe: []`. Die Annahme „Topic-Isolierung greift" war falsch: alle Updates waren öffentlich — seit dem Nachtrag privat (`UserUpdate`). |
|
||||
| **S-07** | ⚪️ bewusst so | — | Siehe „Bewusst so belassen" unten. |
|
||||
| **S-08** | ✅ behoben | `8c3080d` | Mit S-02 mitgezogen. |
|
||||
| **S-09** | ⚪️ bewusst so | — | Siehe unten. |
|
||||
@@ -393,6 +393,8 @@ Der Default ist trotzdem die falsche Vorgabe: ein künftiger Aufruf von `createC
|
||||
|
||||
**Behebung:** `subscribe: []` als Default. Die Anwendung publiziert nur, sie abonniert serverseitig nicht — der Publish-Anspruch bleibt, der Subscribe-Anspruch entfällt und muss dann pro Cookie explizit gesetzt werden, was ohnehin bereits geschieht.
|
||||
|
||||
**Nachtrag — die Isolierung griff nicht.** Die Einschätzung „aktuell nicht ausnutzbar" oben ist falsch. Der Topic-Anspruch im Cookie wirkt nur auf **private** Updates; alle acht Notifier publizierten öffentliche (`new Update(...)` ohne `private: true`), und die liefert der Hub an jeden Subscriber aus, der das Topic nennt — mit oder ohne passenden Anspruch. Jede angemeldete Person konnte also `mail/user/<fremde id>` abonnieren und Sync-Ereignisse, Label-Markup samt CSRF-Tokens und Betreffzeilen fehlgeschlagener Sendungen mitlesen. Behoben mit `App\Infrastructure\Mercure\UserUpdate`, das jedes Update privat baut; `MercureUpdatesArePrivateTest` lässt kein `new Update(` außerhalb davon zu.
|
||||
|
||||
---
|
||||
|
||||
### 🟢 S-07 — Signierte Image-Proxy-URLs sind unbegrenzt gültig
|
||||
|
||||
@@ -0,0 +1,34 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Infrastructure\Mercure;
|
||||
|
||||
use Symfony\Component\Mercure\Update;
|
||||
|
||||
/**
|
||||
* The one way this application builds a Mercure update: always private.
|
||||
*
|
||||
* Every topic plMail publishes to is somebody's own stream (`mail/user/<id>`),
|
||||
* and what goes down it is theirs — sync events, label markup with CSRF tokens
|
||||
* in it, the subject of a send that failed. A PUBLIC update is delivered to any
|
||||
* subscriber who names the topic, authorized or not, so the per-user topic
|
||||
* grant in the subscriber cookie (MercureCookieSubscriber, MercureAuthController)
|
||||
* protected nothing: anyone could open `mail/user/<somebody else>` and read
|
||||
* along. A private update reaches only subscribers whose JWT lists a matching
|
||||
* topic, which is exactly the grant that cookie already carries.
|
||||
*
|
||||
* A factory rather than `private: true` at each call site because the flag is
|
||||
* the kind of argument a new notifier forgets and review does not catch;
|
||||
* MercureUpdatesArePrivateTest fails any `new Update(` outside this file.
|
||||
*/
|
||||
final class UserUpdate
|
||||
{
|
||||
/**
|
||||
* @param list<string> $topics
|
||||
*/
|
||||
public static function create(array $topics, string $data): Update
|
||||
{
|
||||
return new Update(topics: $topics, data: $data, private: true);
|
||||
}
|
||||
}
|
||||
@@ -5,10 +5,10 @@ declare(strict_types=1);
|
||||
namespace App\Service\Ai;
|
||||
|
||||
use App\Domain\Helper\ThrowableSeverity;
|
||||
use App\Infrastructure\Mercure\UserUpdate;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Psr\Log\LogLevel;
|
||||
use Symfony\Component\Mercure\HubInterface;
|
||||
use Symfony\Component\Mercure\Update;
|
||||
|
||||
/**
|
||||
* Tell an open thread that its summary has landed.
|
||||
@@ -33,7 +33,7 @@ final readonly class ThreadSummaryNotifier
|
||||
public function finished(int $userId, int $threadId, string $state): void
|
||||
{
|
||||
try {
|
||||
$this->hub->publish(new Update(
|
||||
$this->hub->publish(UserUpdate::create(
|
||||
topics: [sprintf('mail/user/%d', $userId)],
|
||||
data: (string) json_encode([
|
||||
'type' => 'summary.finished',
|
||||
|
||||
@@ -7,10 +7,10 @@ namespace App\Service\Calendar;
|
||||
use App\Domain\Helper\ThrowableSeverity;
|
||||
use App\Entity\Calendar\Calendar;
|
||||
use App\Entity\User\User;
|
||||
use App\Infrastructure\Mercure\UserUpdate;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Psr\Log\LogLevel;
|
||||
use Symfony\Component\Mercure\HubInterface;
|
||||
use Symfony\Component\Mercure\Update;
|
||||
|
||||
/**
|
||||
* Tells an open page that its calendar moved.
|
||||
@@ -94,7 +94,7 @@ final readonly class CalendarNotifier
|
||||
private function publish(int $userId, array $data): void
|
||||
{
|
||||
try {
|
||||
$this->hub->publish(new Update(
|
||||
$this->hub->publish(UserUpdate::create(
|
||||
topics: [sprintf('mail/user/%d', $userId)],
|
||||
data: json_encode($data, JSON_THROW_ON_ERROR),
|
||||
));
|
||||
|
||||
@@ -6,10 +6,10 @@ namespace App\Service\Insight;
|
||||
|
||||
use App\Domain\Helper\ThrowableSeverity;
|
||||
use App\Entity\User\User;
|
||||
use App\Infrastructure\Mercure\UserUpdate;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Psr\Log\LogLevel;
|
||||
use Symfony\Component\Mercure\HubInterface;
|
||||
use Symfony\Component\Mercure\Update;
|
||||
|
||||
/**
|
||||
* Tells a user's open tabs that their insights moved.
|
||||
@@ -44,7 +44,7 @@ final readonly class InsightNotifier
|
||||
public function publishInsightsChanged(User $user): void
|
||||
{
|
||||
try {
|
||||
$this->hub->publish(new Update(
|
||||
$this->hub->publish(UserUpdate::create(
|
||||
topics: [
|
||||
sprintf('mail/user/%d', $user->id),
|
||||
],
|
||||
|
||||
@@ -6,10 +6,10 @@ namespace App\Service\Job;
|
||||
|
||||
use App\Domain\Helper\ThrowableSeverity;
|
||||
use App\Entity\Job\BackgroundJob;
|
||||
use App\Infrastructure\Mercure\UserUpdate;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Psr\Log\LogLevel;
|
||||
use Symfony\Component\Mercure\HubInterface;
|
||||
use Symfony\Component\Mercure\Update;
|
||||
|
||||
/**
|
||||
* Tell an open page that a job has moved.
|
||||
@@ -36,7 +36,7 @@ final readonly class JobNotifier
|
||||
}
|
||||
|
||||
try {
|
||||
$this->hub->publish(new Update(
|
||||
$this->hub->publish(UserUpdate::create(
|
||||
topics: [sprintf('mail/user/%d', $userId)],
|
||||
data: json_encode([
|
||||
'type' => 'jobs.changed',
|
||||
|
||||
@@ -6,10 +6,10 @@ namespace App\Service\Label;
|
||||
|
||||
use App\Domain\Helper\ThrowableSeverity;
|
||||
use App\Entity\User\User;
|
||||
use App\Infrastructure\Mercure\UserUpdate;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Psr\Log\LogLevel;
|
||||
use Symfony\Component\Mercure\HubInterface;
|
||||
use Symfony\Component\Mercure\Update;
|
||||
|
||||
/**
|
||||
* Tells a user's OTHER tabs that their label structure moved.
|
||||
@@ -94,7 +94,7 @@ final readonly class LabelNotifier
|
||||
}
|
||||
|
||||
try {
|
||||
$this->hub->publish(new Update(
|
||||
$this->hub->publish(UserUpdate::create(
|
||||
topics: [sprintf('mail/user/%d', $userId)],
|
||||
data: json_encode([
|
||||
'type' => 'labels.changed',
|
||||
|
||||
@@ -6,10 +6,10 @@ namespace App\Service\Mail;
|
||||
|
||||
use App\Domain\Helper\ThrowableSeverity;
|
||||
use App\Entity\Mail\Message;
|
||||
use App\Infrastructure\Mercure\UserUpdate;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Psr\Log\LogLevel;
|
||||
use Symfony\Component\Mercure\HubInterface;
|
||||
use Symfony\Component\Mercure\Update;
|
||||
use Twig\Environment;
|
||||
|
||||
/**
|
||||
@@ -72,7 +72,7 @@ final readonly class SendOutcomeNotifier
|
||||
}
|
||||
|
||||
try {
|
||||
$this->hub->publish(new Update(
|
||||
$this->hub->publish(UserUpdate::create(
|
||||
topics: [sprintf('mail/user/%d', $userId)],
|
||||
data: json_encode([
|
||||
'type' => 'mail.send-outcome',
|
||||
|
||||
@@ -7,9 +7,9 @@ namespace App\Service\Mail;
|
||||
use App\Domain\Helper\ThrowableSeverity;
|
||||
use App\Entity\Mail\Account;
|
||||
use App\Entity\Mail\Mailbox;
|
||||
use App\Infrastructure\Mercure\UserUpdate;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Symfony\Component\Mercure\HubInterface;
|
||||
use Symfony\Component\Mercure\Update;
|
||||
|
||||
/**
|
||||
* Mercure publishing only. Contact harvesting is dispatched once per
|
||||
@@ -77,7 +77,7 @@ final readonly class SyncNotifier
|
||||
private function publish(array $topics, array $data): void
|
||||
{
|
||||
try {
|
||||
$this->hub->publish(new Update(topics: $topics, data: json_encode($data, JSON_THROW_ON_ERROR)));
|
||||
$this->hub->publish(UserUpdate::create(topics: $topics, data: json_encode($data, JSON_THROW_ON_ERROR)));
|
||||
} catch (\Throwable $e) {
|
||||
$this->logger->log(
|
||||
ThrowableSeverity::level($e, \Psr\Log\LogLevel::WARNING),
|
||||
|
||||
@@ -6,10 +6,10 @@ namespace App\Service\Rule;
|
||||
|
||||
use App\Domain\Helper\ThrowableSeverity;
|
||||
use App\Entity\Rule\MailRule;
|
||||
use App\Infrastructure\Mercure\UserUpdate;
|
||||
use Psr\Log\LoggerInterface;
|
||||
use Psr\Log\LogLevel;
|
||||
use Symfony\Component\Mercure\HubInterface;
|
||||
use Symfony\Component\Mercure\Update;
|
||||
|
||||
/**
|
||||
* Tells an open settings page that a rule run has moved.
|
||||
@@ -36,7 +36,7 @@ final readonly class RuleRunNotifier
|
||||
}
|
||||
|
||||
try {
|
||||
$this->hub->publish(new Update(
|
||||
$this->hub->publish(UserUpdate::create(
|
||||
topics: [sprintf('mail/user/%d', $rule->usr->id)],
|
||||
data: json_encode([
|
||||
'type' => 'rule.run',
|
||||
|
||||
@@ -0,0 +1,50 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Tests\Infrastructure\Mercure;
|
||||
|
||||
use App\Infrastructure\Mercure\UserUpdate;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
use RecursiveDirectoryIterator;
|
||||
use RecursiveIteratorIterator;
|
||||
use SplFileInfo;
|
||||
|
||||
/**
|
||||
* Every Mercure update this application publishes is private.
|
||||
*
|
||||
* All eight notifiers used to publish public updates, which the hub hands to
|
||||
* any subscriber naming the topic — so the per-user topic grant in the
|
||||
* subscriber cookie guarded nothing, and one signed-in user could read another
|
||||
* user's `mail/user/<id>` stream. The fix is one factory; this pins both that
|
||||
* the factory marks updates private and that nothing goes around it.
|
||||
*/
|
||||
final class MercureUpdatesArePrivateTest extends TestCase
|
||||
{
|
||||
public function testTheFactoryMarksUpdatesPrivate(): void
|
||||
{
|
||||
$update = UserUpdate::create(['mail/user/1'], '{}');
|
||||
|
||||
self::assertTrue($update->isPrivate());
|
||||
self::assertSame(['mail/user/1'], $update->getTopics());
|
||||
}
|
||||
|
||||
public function testNothingInSrcBuildsAnUpdateDirectly(): void
|
||||
{
|
||||
$src = dirname(__DIR__, 3).'/src';
|
||||
$offenders = [];
|
||||
|
||||
/** @var SplFileInfo $file */
|
||||
foreach (new RecursiveIteratorIterator(new RecursiveDirectoryIterator($src, RecursiveDirectoryIterator::SKIP_DOTS)) as $file) {
|
||||
if ('php' !== $file->getExtension() || str_ends_with($file->getPathname(), '/Infrastructure/Mercure/UserUpdate.php')) {
|
||||
continue;
|
||||
}
|
||||
|
||||
if (1 === preg_match('/new\s+(\\\\?Symfony\\\\Component\\\\Mercure\\\\)?Update\s*\(/', (string) file_get_contents($file->getPathname()))) {
|
||||
$offenders[] = substr($file->getPathname(), strlen($src) + 1);
|
||||
}
|
||||
}
|
||||
|
||||
self::assertSame([], $offenders, 'Build Mercure updates through UserUpdate::create() so they are private.');
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user