Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.x] Fix signed routes with expires parameter #38111

Merged
merged 2 commits into from
Jul 23, 2021

Conversation

sebdesign
Copy link
Contributor

What

This PR fixes a bug regarding signed route URLs allowing arbitrary expiration timestamps to be injected.

Why

When the URL is created with a parameter named expires, it will produce unexpected results that could be abused. This affects both temporary and non-temporary signed URLs, and routes with or without an expires parameter.

How

For example, a temporary signed route that should expire after 30 minutes, will not fail if we pass an expires parameter with a timestamp 30 days in the future:

Route::get('unsubscribe/{id}', UnsubscribeController::class)
    ->name('unsubscribe')
    ->middleware('signed');

$in30minutes = now()->addMinutes(30); // 1627000475
$in30days = now()->addDays(30); // 1629590675

// https://example.com/unsubscribe/1?signature=...&expires=1629590675
$url = URL::temporarySignedRoute(
    name: 'unsubscribe',
    expiration: $in30minutes,
    parameters: ['user' => 1, 'expires' => $in30days->timestamp],
);

Carbon::setTestNow(now()->addMinutes(31));

$this->get($url)->assertForbidden(); // Response status code [200] is not a forbidden status code

This temporary signed URL uses the timestamp from the parameters instead of the expiration timestamp, so URL is considered valid even after 30 minutes have passed.

Similarly, if we pass 'expires' => 0, the URL will never expire because the signatureHasNotExpired() will treat the expires query parameter as empty and will not compare it to the current timestamp.

Furthermore, when passing a non-empty array e.g. 'expires' => ['https://www.youtube.com/watch?v=dQw4w9WgXcQ'], the URL will never be expired (arrays are always greater in comparison to numbers).

Another example with a signed route that should not expire, will fail if we pass and expires parameter with a timestamp in the past:

Route::get('unsubscribe/{id}', UnsubscribeController::class)
    ->name('unsubscribe')
    ->middleware('signed');

$in30minutes = now()->addMinutes(30); // 1627000475

// https://example.com/unsubscribe/1?signature=...&expires=1627000475
$url = URL::signedRoute(
    name: 'unsubscribe',
    parameters: ['user' => 1, 'expires' => $in30minutes->timestamp],
);

Carbon::setTestNow(now()->addMinutes(31));

$this->get($url)->assertOk(); // Response status code [403] does not match expected 200 status code

This signed URL uses the timestamp from the parameters in the query string, and signatureHasNotExpired() will treat it as non-empty and expired after 30 minutes.

Changes details

The fix is similar to #30444: it will throw an exception when the parameters array contains a key named expires.

In the first commit I've only included a failing test that demonstrates the bug report, and the second commit includes the fix and more tests.

@GrahamCampbell
Copy link
Member

This is not a bug fix, this is a new feature for DX.

@sebdesign
Copy link
Contributor Author

I've included a failing test that proves this is a bug. How is this a feature? The exception is not there for DX, but for validation against the argument that produces unexpected timestamps.

@taylorotwell taylorotwell merged commit cd49e7e into laravel:6.x Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants