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

[9.x] Allow VerifyCsrfToken's CSRF cookie to be extended #41342

Merged
merged 4 commits into from
Mar 4, 2022
Merged

[9.x] Allow VerifyCsrfToken's CSRF cookie to be extended #41342

merged 4 commits into from
Mar 4, 2022

Conversation

jaggy
Copy link
Contributor

@jaggy jaggy commented Mar 4, 2022

This PR moves how the CSRF token is created to its own method.

Why?

There are some cases in multi-tenant systems that the user might want to change the CSRF token name to prevent 419 errors. Multiple Auth providers make this happen as well mainly in XHR requests. This also allows multi-tenant systems to update the token's domain (ie, pull the current tenant's custom domain) from the middleware layer.

I think this is going to help a lot with people using Inertia to allow customization in how XSRF-TOKEN is named by adding the tenant ID, or even the user type.

By allowing the cookie to be extended in user-land, the user can create 2 different VerifyCsrfToken classes and allow it to be modified without having to extend the whole addCookieToResponse method.

class VerifyCsrfToken extends Middleware {
    protected function newCookie($request, $config)
    {
        return new Cookie(
            "XSRF-TOKEN-{$request->user()->type}",
            $request->session()->token(),
            $this->availableAt(60 * $config['lifetime']),
            $config['path'],
            $config['domain'],
            $config['secure'],
            false,
            false,
            $config['same_site'] ?? null,
        );
    }
}

@jaggy jaggy changed the title [9.x] Allow VerifyCsrfToken's CSRF cookie to be extended much easier [9.x] Allow VerifyCsrfToken's CSRF cookie to be extended Mar 4, 2022
Comment on lines 205 to 211
protected function newCookie($request, $config)
{
return new Cookie(
'XSRF-TOKEN', $request->session()->token(), $this->availableAt(60 * $config['lifetime']),
$config['path'], $config['domain'], $config['secure'], false, false, $config['same_site'] ?? null
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just passing the request, since the only usage of the method is this one, you could just get the config in there, if missing? (Then you could remove src/Illuminate/Foundation/Http/Middleware/VerifyCsrfToken.php:185)

Suggested change
protected function newCookie($request, $config)
{
return new Cookie(
'XSRF-TOKEN', $request->session()->token(), $this->availableAt(60 * $config['lifetime']),
$config['path'], $config['domain'], $config['secure'], false, false, $config['same_site'] ?? null
);
}
protected function newCookie($request, $config = null)
{
$config = $config ?: config('session');
return new Cookie(
'XSRF-TOKEN', $request->session()->token(), $this->availableAt(60 * $config['lifetime']),
$config['path'], $config['domain'], $config['secure'], false, false, $config['same_site'] ?? null
);
}

@taylorotwell taylorotwell merged commit 101cd01 into laravel:9.x Mar 4, 2022
@jaggy jaggy deleted the allow-csrf-cookie-to-be-easily-extendable branch March 4, 2022 21:53
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.

4 participants