Optional validation of header contents
ocramz opened this issue · 12 comments
I suggest validating the header inside
changeHeader
and throw 500 (via ScottyException) if it contains CR/LF/NUL. For the sake of compatibility or performance optimisation, we could make this behaviour toggleble by adding a boolean field toOptions
.
Originally posted by @fumieval in #359 (comment)
Reference: #92
Hi, if I understand correctly addressing this would require these steps:
- Add a constructor to
ScottyException
, e.g.InvalidHeader Text Text Text
(header, value, error message). Perhaps it could be refined into something likeInvalidHeaderName Text
andInvalidHeaderValue Text
or create a sum type etc. - Add a field to
Options
to allow users to specify whether headers should be validated, e.g.validateHeaders
. It needs to be decided whether validation is on by default (breaking change) or not. - Modify
changeHeader
such that whenvalidateHeaders
is set, the name and value of the header are validated. - Define what characters are invalid. #92 gives some pointers for this:
- Just
\NUL
,\n
,\r
- Any control characters
- Just
It would also be possible to let the user decide what characters to accept, e.g. by adding a validHeaderChar :: Char -> Bool
predicate to Options
instead.
Before writing the necessary code I'd like to hear your opinions :)
Instead of my original suggestion, we could also implement the whole validation logic as a WAI middleware
So basically we replace the
Modify changeHeader such that when validateHeaders is set, the name and value of the header are validated.
by
Add a middleware that validates name and value of each response header when
validateHeaders
is set
? I can make a PR for that.
@fumieval you would prefer a WAI middleware I believe? I can implement this but I would prefer to have your go-ahead
Personally I think putting this in changeHeader
is more transparent API behavior but that's a nitpick
@pbrinkmeier I believe the middleware approach makes implementation much easier. I don't have a strong opinion about the user-facing API, but how about adding validateHeaders :: Bool
field to Options
, and make it insert the middleware if it's True?
@fumieval but then, why use a flag at all when the user can just insert a middleware?
@ocramz I'd personally prefer letting users insert the middleware, but I thought it might be an option if we care about ease of use.
Okay, so should we add the middleware to Web.Scotty
? Or a seperate module perhaps? I will get working on the implementation and we can move it around before merging.
+1 to exporting the middleware from Web.Scotty (IMO the right place for this middleware is https://hackage.haskell.org/package/wai-extra, but it shouldn't stop going forward in scotty)
While implementing this I noticed that scottyExceptionHandler
doesn't handle exceptions thrown in middlewares, so I guess we could simply return a 500 with an appropriate error text in the middlware?
Edit: At that point I could also move the PR to wai-extra
as it becomes completely decoupled from scotty
.
I just added a PR at wai-extra
to address this issue: yesodweb/wai#990
The PR in wai-extra
is merged by now, so we could add a comment to the docs referencing that middleware.