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

Error: CSRF token missing #312

Closed
macmichael01 opened this issue Aug 28, 2015 · 13 comments
Closed

Error: CSRF token missing #312

macmichael01 opened this issue Aug 28, 2015 · 13 comments

Comments

@macmichael01
Copy link

First off thanks for this starter kit. Nicely done! I have a form which needs to handle a file upload. However the moment I add this form attr enctype="multipart/form-data" I receive a CSRF error. I tried multiparty, and busboy without success which I believe the problem happens before it even gets to busboy or multiparty. Articles suggest to place file parsing before the app.use(lusca(...)) but still doesn't work for me.

I also tried checking out a fresh copy of this repo and adding the enctype="multipart/form-data" to the login form just to make sure there was nothing that I did to cause the CSRF error and it immediately goes to a 403 Error: CSRF token missing when the form is submitted.

Hoping this is a simple mistake on my end.

@macmichael01
Copy link
Author

Ok I think I understand what is going on here. This is sort of a complicated issue. I really believe that body-parser needs to handle multi-part data.

Here are the 3 cases

  1. parsing only field inputs - Works Great
  2. parsing only file inputs - You will need a file upload middleware to handle this. Also you will need to whitelist the path to your file upload form to prevent CSRF. I would think that everyone would want CSRF protection on a file upload form but since the enctype was changed to multi-part, body-parser does not know how to handle this which will result in a 403 unless you whitelist. - Works but you have to white list and you need an external middleware
  3. parsing both file input and field inputs together. - Not going to happen

I believe this to be the issue that I am experiencing. And I think the solution is for body-parse to handle file inputs. There's an open bug for this here: expressjs/body-parser#88.

Does anyone have a work around for handling both file and field inputs in the same form? I'm fine with using the middleware for the files but I would still like to continue using body-parser for the rest of my form field items but it's doesn't seem to be possible.

@sahat
Copy link
Owner

sahat commented Sep 9, 2015

@macmichael01 I can tell you right now that body-parser is nto going to handle file uploads. It used to do that, but for if I remember correctly for security reasons it was removed. So, instead they suggested users to use packages like busboy and multer and formidable.

I haven't tried this, but did you attempt to add multer middleware to your form submission route? Wouldn't that give you access to both uploaded file and form inputs? If that doesn't work, then I am not sure how to help you. The only time I had to deal with a file upload, was when I needed to do just a single file upload, no other input fields.

I am going to close this issue since this is not directly related to the hackathon starter, nor is there anything I can do to make body-parser handle file uploads.

@sahat sahat closed this as completed Sep 9, 2015
@sahat
Copy link
Owner

sahat commented Sep 9, 2015

Good luck, and let me know if you find a better solution because I will consider adding it as a standalone example to this project, similar to http://hackathonstarter.herokuapp.com/api/scraping.

@gianpaj
Copy link
Contributor

gianpaj commented Sep 20, 2015

I got into the same issue using multer. I think @macmichael01 is correct. body-parser seems to be breaking something.

Here's the code changes I've done to send only a simple file upload:
https://gist.github.com/gianpaj/8204ca9beb3d71adff64

@macmichael01
Copy link
Author

body-parser isn't breaking anything. multipart is not supported by body-parser. The reason for the 403 is that lusca.csrf() is looking for the _csrf in the req.body. The body gets set to {} since multipart data cannot be read from the raw request.

I really like body-parser, I just wished that multipart was supported. Adding multupart support to body-parser is a huge task which is why nobody has added such support yet.

So perhaps lusca should be replaced with something that works with standard and multipart forms. I believe this would also mean that express-validator (assuming that it uses body-parser) would also need to be replaced with something else. formidable perhaps?

@leoj3n
Copy link
Contributor

leoj3n commented Dec 27, 2015

👍 to supporting/accommodating file uploads in tandem with csrf security, if possible. Probably not the end of the world for a hackathon starter, but would be nice to have. Will disable like @gianpaj for now.

@sahat
Copy link
Owner

sahat commented Feb 9, 2016

I have added a simple file upload example. The only way to get it to work with Lusca's CSRF was to explicitly exclude /api/upload path from CSRF check. [40a68c8]

screenshot 2016-02-08 22 33 06

@elmiomar
Copy link

Hello,

First, I would like to thank you for this amazing starter kit!
I had the same issue with the CSRF check and I did the same workaround to get it to work.
I was wondering what is the impact of this "solution" on security? (if there is any)

Cheers.

@krlng
Copy link

krlng commented Aug 21, 2016

I would also be interested in the security implications. If I undestand it correctly, I could use file upload without having the token. So probably I should not mix file-upload and some important information within one form but split it up, correct?

@elmiomar
Copy link

@nik-ffm In a token-base authentication, every request to the server is accompanied by a token which the server uses to verify the authenticity of the request. The flow works like this:

  1. User enters their login credentials
  2. Server verifies the credentials are correct and returns a signed token
  3. This token is stored client-side, most commonly in local storage - but can be stored in session storage or a cookie as well
  4. Subsequent requests to the server include this token as an additional Authorization header or through one of the other methods mentioned above
  5. The server decodes the JSON WEB Token (JWT) and if the token is valid processes the request
  6. Once a user logs out, the token is destroyed client-side, no interaction with the server is necessary

From: Cookies vs Tokens: The Definitive Guide

CSRF is an attack that forces an end user to execute unwanted actions on a web application in which they're currently authenticated. CSRF attacks specifically target state-changing requests, not theft of data, since the attacker has no way to see the response to the forged request.
From: CSRF

So, based on my understanding (correct me if I am wrong), if you disable the CSRF check in your application, you will be exposing your application to a CSRF attack, thus you can be tricked into executing state changing requests like transferring funds, changing their email address, etc.

For your question, I am not able to provide you with a answer as I am myself wondering what are the impacts of this trick on security.

@ghost
Copy link

ghost commented Oct 4, 2018

I have done this another way:

that's the original code:

app.use((req, res, next) => {
  if (req.path === '/api/upload') {
    next();
  } else {
    lusca.csrf()(req, res, next);
  }
});

i changed it a bit:
i'm checking whether req.headers content type contains: multipart/form-data
then i'm checking whether the requested URL equals my file+text endpoints - if it is - then i'm just skipping the csrf as you did.

@bobbysiagian
Copy link

I have done this another way:

that's the original code:

app.use((req, res, next) => {
  if (req.path === '/api/upload') {
    next();
  } else {
    lusca.csrf()(req, res, next);
  }
});

i changed it a bit:
i'm checking whether req.headers content type contains: multipart/form-data
then i'm checking whether the requested URL equals my file+text endpoints - if it is - then i'm just skipping the csrf as you did.

works for me, thanks !

@mayeaux
Copy link

mayeaux commented Aug 30, 2020

I have done this another way:

that's the original code:

app.use((req, res, next) => {
  if (req.path === '/api/upload') {
    next();
  } else {
    lusca.csrf()(req, res, next);
  }
});

i changed it a bit:
i'm checking whether req.headers content type contains: multipart/form-data
then i'm checking whether the requested URL equals my file+text endpoints - if it is - then i'm just skipping the csrf as you did.

Can you post your code for that?

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

No branches or pull requests

8 participants