Refactoring CSRF implementation

krakenjs / lusca

Application security for express apps.

Other

1.79k stars 123 forks source link

Refactoring CSRF implementation #13

Closed jeffharrell closed 10 years ago

jeffharrell commented 10 years ago

Changes a few things in CSRF:

It drops the express/connect dependency for a custom one
Allows customization of CSRF key name in res.locals
Allows for a custom token creation/validation implementation
Adds a few test cases
Misc other cleanup in the licenses, docs, and all around

The default implementation for creating and validating tokens is similar to the algorithm connect uses, but I'm looking for feedback on that.

lmarkus commented 10 years ago

Very nice. Just one comment. HSTS requires a max age , but if the developer makes a mistake (wrong key name, no key, etc) it "fails" silently by not doing anything. Should the module fail, or at least log a warning about bad config?

totherik commented 10 years ago

I think this looks good overall. I didn't go into any depth on non-csrf stuff, however. :+1:?

mstuart commented 10 years ago

Otherwise, :+1:

jeffharrell commented 10 years ago

Upstream changes merged.

@mstuart, I needed to make a tiny tweak to the interface of your code and exposed the properties themselves rather than having the users pass the raw header: https://github.com/paypal/lusca/commit/0926ccfe11a0f32bf308ebba2efaaf657a41d0bc#diff-90ac2b27c938f8b134ba5b88ff85abcdR7

Otherwise, all, any reasons why this shouldn't be merged?

mstuart commented 10 years ago

@jeffharrell Cool, I like that better.

jasisk commented 10 years ago

I spy with my little eye something LGTM.

jeffharrell commented 10 years ago

Three people said it looked good. I AM PRESSING TEH GREEN BUTTON!!!1!