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

[5.5] Fix: verify csrf token allows to except full urls #22619

Merged
merged 2 commits into from
Jan 2, 2018

Conversation

BenCavens
Copy link
Contributor

Currently, you can leave out certain URLs from CSRF token verification by adding them to the except array. This is provided with an out-of-the-box laravel application.

However, this only works if you add the paths and not the full URLs. This PR makes sure it is also possible to provide full URLs to this except array.

@GrahamCampbell GrahamCampbell changed the title Fix: verify csrf token allows to except full urls [5.5] Fix: verify csrf token allows to except full urls Jan 2, 2018
@@ -104,7 +104,7 @@ protected function inExceptArray($request)
$except = trim($except, '/');
}

if ($request->is($except)) {
if ($request->url() == $except || $request->is($except)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be ===?

@taylorotwell taylorotwell merged commit 42fb248 into laravel:5.5 Jan 2, 2018
@vlakoff
Copy link
Contributor

vlakoff commented Jan 2, 2018

Would it need wildcards support as well? For consistency with the previously existing $request->is().

@sisve
Copy link
Contributor

sisve commented Jan 3, 2018

I believe this PR is missing tests. Could you add some?

@tillkruss
Copy link
Contributor

tillkruss commented Jan 4, 2018

@BenCavens: Please add a test. I don't know what this PR does and can't add it the changelog.

@BenCavens
Copy link
Contributor Author

I'll try to add some tests.
It's not so straightforward because currently the verifytoken middleware isn't tested at all.

@vlakoff
Copy link
Contributor

vlakoff commented Jan 4, 2018

Can you please tell if wildcard support should be added? (I'd bend supposing it should be)

Also, may I ask for a use case, where you have to use full urls instead of paths?

@BenCavens
Copy link
Contributor Author

@vlakoff @tillkruss I've created a new PR which adds the wildcard support as well as the necessary tests: #22661

I'm looking forward to your feedback. thx

@tillkruss
Copy link
Contributor

@BenCavens: Thanks, great work!

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.

6 participants