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

CSRF not defined when requesting HEAD #3865

Closed
bonanzakrak opened this issue Oct 26, 2016 · 6 comments
Closed

CSRF not defined when requesting HEAD #3865

bonanzakrak opened this issue Oct 26, 2016 · 6 comments

Comments

@bonanzakrak
Copy link

Sails version: 0.12.8
Node version: v4.2.6
NPM version: 3.5.2
Operating system: Ubuntu 16.04 Server


I deployed my app on server today and wanted to test it using webpagetest.org. Every test i got error from sails that '_csrf is not defined', but site displayed properly. I investigated nginx log files and found that the request that was cousing it was a HEAD request. I used ApacheBenchmark to reproduce that error and every HEAD request throws a view error.

ab -n 1 -m HEAD "http://test.site/"

Looks like csrf hook is not performing with that kind of request.

@bonanzakrak
Copy link
Author

It can be connected with this issue
#3707

@sgress454
Copy link
Member

sgress454 commented Oct 26, 2016

Ah, well this makes more sense. The CSRF protection only applies to CRUD routes (GET, PUT, POST, PATCH, DELETE), but a HEAD request will run the matching GET handler (so that it can get the content length of the potential response). So if you have a route like:

'/': {view: 'homepage'}

and your homepage view has <%-_csrf%> in it, then hitting that route with a HEAD request will definitely give you an error. You can work around that easily by changing your view to use:

<% if (typeof _csrf !== 'undefined') { %><%-_csrf%><% } %>

instead.

@sgress454
Copy link
Member

In the next version of Sails, we'll have res.locals._csrf added to every response (defaulting to empty string) when CSRF protection is enabled, so that you won't run into this issue.

@mikermcneil
Copy link
Member

mikermcneil commented Nov 10, 2016

@bonanzakrak just to add a bit of additional color to what @sgress454 mentioned about Sails v1, you'll be able to remove the typeof check from the example above, so it's just:

<%-_csrf %>

That said, bear in mind it'll just be empty string in those cases. But since empty string is falsey in JavaScript, I think the pros outweigh the cons here. Since this is a breaking change, we can't publish it until we do Sails v1 (shooting for doing a release candidate of that some time in the next few weeks, and a proper publish before the end of the year)

will wait to close this issue until we're a bit closer to that publish date

@bonanzakrak
Copy link
Author

Temporary I fixed it using policy. I think this is best solution. When you publish Sails v1 I can just remove this policy, but it will not brake anything.

if(!res.locals._csrf){
    res.locals._csrf = '';
}

@sailsbot
Copy link

@bonanzakrak,@sgress454,@mikermcneil: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

@sailsbot sailsbot added waiting to close This label is deprecated. Please don't use it anymore. and removed waiting to close This label is deprecated. Please don't use it anymore. labels Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants