Skip to content

Commit cf9c66f

Browse files
committed
Security hardening: encapsulate internals, throttle routes, validate RFC 8058 body
- Make `legacySubscriberType` private with a public getter to prevent external mutation of the Subscriber singleton - Add `throttle:60,1` middleware to both the main and legacy unsubscribe routes - Validate RFC 8058 POST body (`List-Unsubscribe=One-Click`) in UnsubscribeController; return 400 for invalid requests - Deduplicate `unsubscribeLink()` call in SubscribableNotification::via() - Warn in stub that the route should not be inside the `web` middleware group to avoid CSRF token verification on one-click POST requests - Update POST tests to send the correct RFC 8058 body; add test asserting 400 is returned when the body is missing https://claude.ai/code/session_01R4pAjWwGY8xKspsdU8xnsy
1 parent 5158d7f commit cf9c66f

7 files changed

Lines changed: 35 additions & 10 deletions

src/Concerns/SubscribableNotification.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@ public static function via(
2121
: null;
2222
$listValue = $list instanceof \BackedEnum ? $list->value : $list;
2323

24+
$unsubscribeUrl = $notifiable->unsubscribeLink($listValue);
25+
2426
if ($listValue !== null) {
25-
$message->viewData['unsubscribeLink'] = $notifiable->unsubscribeLink($listValue);
27+
$message->viewData['unsubscribeLink'] = $unsubscribeUrl;
2628
}
2729
$message->viewData['unsubscribeLinkForAll'] = $notifiable->unsubscribeLink();
2830

29-
$unsubscribeUrl = $notifiable->unsubscribeLink($listValue);
30-
3131
return $message->withSymfonyMessage(function (Email $email) use ($unsubscribeUrl) {
3232
$email->getHeaders()->addTextHeader('List-Unsubscribe', sprintf('<%s>', $unsubscribeUrl));
3333
$email->getHeaders()->addTextHeader('List-Unsubscribe-Post', 'List-Unsubscribe=One-Click');

src/Controllers/LegacyUnsubscribeController.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ public function __invoke(Request $request, mixed $subscriberId, ?string $mailing
1717
{
1818
return app(UnsubscribeController::class)(
1919
$request,
20-
$this->subscriber->legacySubscriberType,
20+
$this->subscriber->getLegacySubscriberType(),
2121
$subscriberId,
2222
$mailingList
2323
);

src/Controllers/UnsubscribeController.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@ public function __invoke(Request $request, string $subscriberType, mixed $subscr
4646
event(new UserUnsubscribed($subscriber, $mailingList));
4747

4848
if ($request->isMethod('post')) {
49+
if ($request->input('List-Unsubscribe') !== 'One-Click') {
50+
abort(400, __('Invalid unsubscribe request'));
51+
}
52+
4953
return response('', 204);
5054
}
5155

src/Subscriber.php

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,12 @@ final class Subscriber
1313
public string $routeName = 'unsubscribe';
1414

1515
/** Morph type stored when legacyRoutes() is called — used by LegacyUnsubscribeController. */
16-
public ?string $legacySubscriberType = null;
16+
private ?string $legacySubscriberType = null;
17+
18+
public function getLegacySubscriberType(): ?string
19+
{
20+
return $this->legacySubscriberType;
21+
}
1722

1823
private ?\Closure $onUnsubscribeFromMailingList = null;
1924
private ?\Closure $onUnsubscribeFromAllMailingLists = null;
@@ -30,7 +35,8 @@ public function routes(mixed $router = null): void
3035
$router = $router ?? $this->app->make('router');
3136
$router->match(['GET', 'POST'], $this->uri, $this->handler)
3237
->name($this->routeName)
33-
->where('subscriberType', '[^\d/][^/]*');
38+
->where('subscriberType', '[^\d/][^/]*')
39+
->middleware('throttle:60,1');
3440
}
3541

3642
public function legacyRoutes(string $defaultModel, mixed $router = null): void
@@ -44,7 +50,8 @@ public function legacyRoutes(string $defaultModel, mixed $router = null): void
4450
['GET', 'POST'],
4551
'unsubscribe/{subscriberId}/{mailingList?}',
4652
'\YlsIdeas\SubscribableNotifications\Controllers\LegacyUnsubscribeController'
47-
)->name($this->routeName . '.legacy');
53+
)->name($this->routeName . '.legacy')
54+
->middleware('throttle:60,1');
4855
}
4956

5057
public function routeName(): string

stubs/SubscribableServiceProvider.stub

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ class SubscribableServiceProvider extends ServiceProvider
99
{
1010
public function boot(): void
1111
{
12+
// Note: register this route outside the 'web' middleware group to avoid CSRF token
13+
// verification on the POST (RFC 8058 one-click) endpoint.
1214
Subscriber::routes();
1315

1416
Subscriber::onUnsubscribeFromMailingList(function ($notifiable, string $mailingList) {

tests/Controllers/LegacyUnsubscribeControllerTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ public function test_it_returns_204_for_rfc8058_post_via_legacy_url()
8181

8282
$url = URL::signedRoute('unsubscribe.legacy', ['subscriberId' => $user->id]);
8383

84-
$this->post($url)->assertNoContent();
84+
$this->post($url, ['List-Unsubscribe' => 'One-Click'])->assertNoContent();
8585
}
8686

8787
public function test_legacy_url_does_not_interfere_with_new_route()

tests/Controllers/UnsubscribeControllerTest.php

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ public function test_it_returns_200_for_rfc8058_one_click_post_unsubscribe()
182182
$called = true;
183183
});
184184

185-
$this->post($expectedUser->unsubscribeLink())
185+
$this->post($expectedUser->unsubscribeLink(), ['List-Unsubscribe' => 'One-Click'])
186186
->assertNoContent();
187187

188188
$this->assertTrue($called);
@@ -205,9 +205,21 @@ public function test_it_returns_200_for_rfc8058_one_click_post_unsubscribe_from_
205205
$this->assertEquals('newsletter', $list);
206206
});
207207

208-
$this->post($expectedUser->unsubscribeLink('newsletter'))
208+
$this->post($expectedUser->unsubscribeLink('newsletter'), ['List-Unsubscribe' => 'One-Click'])
209209
->assertNoContent();
210210

211211
$this->assertTrue($called);
212212
}
213+
214+
public function test_it_rejects_post_without_rfc8058_body()
215+
{
216+
$expectedUser = DummyUser::create([
217+
'name' => 'test',
218+
'email' => 'test@testing.local',
219+
'password' => 'test',
220+
]);
221+
222+
$this->post($expectedUser->unsubscribeLink())
223+
->assertStatus(400);
224+
}
213225
}

0 commit comments

Comments
 (0)