Added CSRF middleware (fix issue #243) #271
Conversation
middleware/csrf.go
Outdated
| // the supplied Referer header. | ||
| ErrBadReferer = errors.New("referer invalid") | ||
| // ErrNoToken is returned if no CSRF token is supplied in the request. | ||
| ErrNoToken = errors.New("CSRF token not found in request") |
There was a problem hiding this comment.
This should probably be ErrNoCSRFToken since this is available for the entire middleware package, it's potentially stealing that error name from another piece of middleware.
middleware/csrf.go
Outdated
| ErrNoToken = errors.New("CSRF token not found in request") | ||
| // ErrBadToken is returned if the CSRF token in the request does not match | ||
| // the token in the session, or is otherwise malformed. | ||
| ErrBadToken = errors.New("CSRF token invalid") |
There was a problem hiding this comment.
Same here, should probably be ErrBadCSRFToken.
middleware/csrf.go
Outdated
|
|
||
| // EnableCSRF enable CSRF protection on routes using this middleware. | ||
| // This middleware is adapted from gorilla/csrf | ||
| func EnableCSRF() buffalo.MiddlewareFunc { |
There was a problem hiding this comment.
Rename to CSRF or CSRFProtection something like that.
Also, there's no need to return a buffalo.MiddlewareFunc here, just make the function itself implement it.
func CSRF(next buffalo.Handler) buffalo.Handler {
...
}
middleware/csrf_test.go
Outdated
|
|
||
| func ctCSRFApp() *buffalo.App { | ||
| h := func(c buffalo.Context) error { | ||
| return c.Render(200, render.String(c.(*buffalo.DefaultContext).Data()["authenticity_token"].(string))) |
There was a problem hiding this comment.
return c.Render(200, render.String(c.Value("authenticity_token").(string)))- it's a lot shorter and easier on the eyes. :)
|
Overall looks great! I just had a few small comments that will make a lot easier, and cleaner. Thanks for this! |
middleware/csrf_test.go
Outdated
| } | ||
| a := buffalo.Automatic(buffalo.Options{}) | ||
| a.GET("/csrf", middleware.EnableCSRF()(h)) | ||
| a.POST("/csrf", middleware.EnableCSRF()(h)) |
There was a problem hiding this comment.
a.GET("/csrf", middleware.CSRF(h))
a.POST("/csrf", middleware.CSRF(h))When you update the CSRF function to be a buffalo.MiddlewareFunc itself, it becomes a lot easier to wrap buffalo.Handler functions.
You could also do the same thing with:
a.Use(middleware.CSRF)
a.GET("/csrf", h)
a.POST("/csrf", h)
|
@markbates Thanks for your review! |
|
Awesome! Thanks! |
|
No problem. :) |
Adapt gorilla/csrf into a Buffalo middleware.