Optional validation of header contents

[ocramz]

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 to Options.

Originally posted by @fumieval in #359 (comment)

Reference: #92

[pbrinkmeier]

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 like InvalidHeaderName Text and InvalidHeaderValue 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 when validateHeaders is set, the name and value of the header are validated.
• Define what characters are invalid. Disallow \r\n in Header text #92 gives some pointers for this:
  • Just \NUL, \n, \r
  • Any control characters

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 :)

[fumieval]

Instead of my original suggestion, we could also implement the whole validation logic as a WAI middleware

[pbrinkmeier]

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.

[pbrinkmeier]

@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

[fumieval]

@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?

[ocramz]

@fumieval but then, why use a flag at all when the user can just insert a middleware?

[fumieval]

@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.

[pbrinkmeier]

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.

[fumieval]

+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)

[pbrinkmeier]

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.

[pbrinkmeier]

I just added a PR at wai-extra to address this issue: yesodweb/wai#990

[pbrinkmeier]

The PR in wai-extra is merged by now, so we could add a comment to the docs referencing that middleware.