search

noir-clojure / lib-noir

A set of libraries for ring apps, including stateful sessions.
Eclipse Public License 1.0
481 stars 47 forks source link

CSRF attack prevention middleware (partial) #28

Closed runexec closed 11 years ago

runexec commented 11 years ago

It's partial because it lacks session support at the moment. Errors get thrown when I try to apply session functions from within the middleware extension.

I accidentally pushed the wrong emacs buffer 3 times, so please ignore the first 3. Lines 21-40 & Line 112 at https://github.com/runexec/lib-noir/blob/e1fbc81be96117d033ca75c449b08dff79860d03/src/noir/util/middleware.clj

bitemyapp commented 11 years ago

Answering so you aren't left in the dark.

I don't think this needs to be in lib-noir. It's too half-baked.

Adding it to the default middleware hooks is not a great idea either.

Good library middleware is an encapsulated and complete solution to a given problem and exists for the majority of apps being built that use the library.

You should consider writing a library that is an end-to-end Forms (https://github.com/jkk/formative) + CSRF solution for vanilla Ring apps.

It's not up to me, but I wouldn't accept this PR as it is right now. I do like the idea of solving common-case problems like CSRF though.

Cheers.

yogthos commented 11 years ago

I have to agree with @bitemyapp, the idea is good but the implementation is not quite there yet. I would recommend trying it out as a standalone middleware first.

runexec commented 11 years ago

Thanks for informative replies, both @bitemyapp and @yogthos . So the both of you would agree that a generic Ring extension would be better, and just to add it to the project.clj and boilerplate code of the luminus-template project instead of lib-noir?

yogthos commented 11 years ago

I think that would be the right approach to start with, it makes it easier to play around with initially and we can always merge it into lib-noir as it matures a bit. I'd prefer to be a bit conservative in adding features, as it's difficult to change them once they start getting used.

runexec commented 11 years ago

@yogthos That makes sense. I'll remove my fork and try to make a solid library. Once that's done, I'll open up an issue ticket on the luminus-template project so you can review the library before I try to add it to the boilerplate.

yogthos commented 11 years ago

looking forward to it :)

yogthos commented 11 years ago

@runexec btw you might want to check out ring-anti-forgery by weavejester, I just ran across it

runexec commented 11 years ago

@yogthos I just saw the link you sent me right after I finished my version. I'll show you my version here shortly and you can tell me what you think. Thanks again.