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

Allow CSRF cookie options to be set #104

Merged
merged 7 commits into from
Aug 7, 2017
Merged

Allow CSRF cookie options to be set #104

merged 7 commits into from
Aug 7, 2017

Conversation

stgogm
Copy link
Contributor

@stgogm stgogm commented Jun 2, 2017

This fix allows us to set cookie options and maintain compatibility with current configurations.

Example configurations:

// Using AngularJS with cookie options
lusca.csrf({
  angular: true,
  cookie: {
    options: {
      httpOnly: true,
      secure: true
    }
  }
});
// Using custom cookie name and options.
lusca.csrf({
  cookie: {
    name: 'MyCustomCSRFCookieName',
    options: {
      httpOnly: true,
      secure: true
    }
  }
});
// Using this also works
lusca.csrf({
  cookie: 'MyCustomCSRFCookieName'
});
// Using this works too
lusca.csrf({
  cookie: {
    name: 'MyCustomCSRFCookieName'
  }
});

@stgogm stgogm mentioned this pull request Jun 2, 2017
@doublerebel
Copy link

doublerebel commented Jun 15, 2017

Without being able to set cookie options, I receive warnings when running the OWASP ZAP test because httpOnly and secure are not set on my csrf cookie. LGTM!

EDIT: OWASP reference https://www.owasp.org/index.php/Testing_for_cookies_attributes_(OTG-SESS-002)

@stgogm
Copy link
Contributor Author

stgogm commented Jun 16, 2017

@doublerebel That's the main reason behind this PR as we performed an Acunetix audit on our applications and the only warning we had was with an insecure cookie (XSRF).

Sadly, it's been quite a while since I made this pull request.

Anyway, I've added the respective tests and improved cookie options validations.

If you like, you can install lusca with this fix until it's merged using npm i https://github.com/stgogm/lusca.git#patch-2

@grawk
Copy link
Member

grawk commented Jun 18, 2017

Hey @stgogm. We'll assess this PR this week. Thanks for submitting it.

@grawk
Copy link
Member

grawk commented Jun 18, 2017

Seems fine from my perspective 👍

@stgogm
Copy link
Contributor Author

stgogm commented Jun 19, 2017

@grawk Awesome! Let me know if you need anything else.

@stgogm
Copy link
Contributor Author

stgogm commented Jul 11, 2017

Hi @grawk

Any news regarding this?

@stgogm
Copy link
Contributor Author

stgogm commented Jul 25, 2017

Finally decided to take my own approach on this matter. If anyone else is interested, you're welcome to try Fi Aegis. We've forked this project and added some functionalities, improved documentation and made minor code optimizations.

@linkRace
Copy link
Collaborator

linkRace commented Aug 7, 2017

👍 , will get this in with 1.5.0 release.
sorry this repo has been so neglected :(

@linkRace linkRace merged commit 20ab3ba into krakenjs:master Aug 7, 2017
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.

None yet

4 participants