Double encoding of percent signs (servant-client)

[GambolingPangolin]

First of all, this is a wonderful project and very useful. Now, down to business.

When using defaults, servant-client code can double-encode percent signs in query parameters. Consider this (incomplete) example:

newtype UrlEncoded = UrlEncoded { unUrlEncoded :: ByteString }

instance ToHttpApiData UrlEncoded where
  toQueryParam = decodeUtf8 . urlEncode True . unUrlEncoded

type ExampleEndpoint = QueryParam "value" UrlEncoded :> GetNoContent

exampleEndpoint :: UrlEncoded -> ClientM ()
exampleEndpoint = client $ Proxy @ExampleEndpoint

problemCall :: ClientM ()
problemCall = exampleEndpoint $ UrlEncoded "\x01"

Running problemCall creates a request with query string ?value=%2501. I believe the reason is that the HasClient instance of QueryParam uses toQueryParam when adding the parameter to the (servant) Request it builds up, then defaultMakeClientRequest calls renderQuery which re-encodes the string while marshalling a (http-client) Request.

It is my understanding that ToHttpApiData (toQueryParam) is meant to produce escaped output. Therefore, it seems that defaultMakeClientRequest should use a custom version of renderQuery which does not escape values.

[alpmestan]

@GambolingPangolin From a cursory look it does seem like you're right. Would you be interested in putting together a patch for this? (and perhaps even a test)

[GambolingPangolin]

@alpmestan Sure, I'd be happy to!

[alpmestan]

@GambolingPangolin Great, thanks! Do let us know if you hit any roadblock along the way.

[GambolingPangolin]

I got stuck making this change. After reading this issue, I decided to handle the changes to the client by using toEncodedUrlParam when marshalling a (servant) request, as well as using a query string builder that does not do escaping.

I did not see a way to make a change to the server code without significantly changing the behavior. FromHttpApiData uses Text to represent query parameters in raw form. Text cannot represent arbitrary binary data, but modifying the server to defer URL encoding so that the Text going into e.g. parseQueryParam is still URL-encoded will break many downstream applications.

Handling arbitrary binary data in query parameters is pretty niche (I need it for a hobby implementation of the BitTorrent protocol) so I think it makes sense to simply add a warning that servant-server endpoints should not be used if you need to send query parameter values whose encoding does not factor through a UTF-8 textual representation.

BTW the client changes that I made, should not change the behavior of clients which use the default implementation of toEncodedUrlPiece for their data types (probably the overwhelming majority).

[GambolingPangolin]

Here is my:

• branch = https://github.com/GambolingPangolin/servant/tree/fixes-1418
• diff = master...GambolingPangolin:fixes-1418