Inconsistent query parameter handling/encoding between server and client

[zlondrej]

The query parameter URL-encoding has undocumented behavior. I was trying to write some HasServer and HasClient instances working with Query and oh boy, what it took me a while to get out of that rabbit hole.

servant-server is based on wai which uses parseQuery (https://github.com/yesodweb/wai/blob/master/warp/Network/Wai/Handler/Warp/Request.hs#L108). This will always properly decode both query key and value.

However the servant-client introduced a risky approach to client parameter encoding when #1432 was merged (and doesn't even seems necessary). Now only query keys are implicitly encoded. QueryParam' and QueryParams seem to be aware of this (after some bugfixes), and explicitly encode the value using encodeQueryParam (QueryParam' and QueryParams instances).

However since then, few more query handling types were introduced and they seem to be unaware of the fact. Namely QueryString (the main issue here is undocumented behavior - both server and client-vise, developers won't know what to expect while using it) and DeepQuery (no URL-encoding is performed here either).

There have been few issues/PRs related to URL-encoding reported in the past:

• Query parameter encoding #1584
• QueryParam doesn't conform to HTTP #1626

Now there's a proposed solution based on fizruk/http-api-data#120 (toEncodedQueryParam) that is attempting somewhat fixing the symptoms:

• Use toEncodedQueryParam for QueryParam #1628

But the source of the issue is still there. There's nothing saying whether the requestQueryString :: Query in the client's `Request' should or should not be URL-encoded (be it documentation or the types).

---

Proposal

I think the surefire solution would be to switch to PartialEscapeQuery for servant-client, which forces selection of URL-encoding on the type level and encoding it using renderQueryPartialEscape.

Such change will introduce an API incompatibility with older versions, but that's actually for the best as it will force everyone to review their implementations. Also the migration should be quite easy, wrapping any existing values into list of QE's or QN's (EscapeItem's constructors) and replacing Nothing with empty list. Introducting some smart constructors can make it even easier.

And for servant-server documenting that QueryString returns URL-decoded keys and values would also be a good idea. Not sure whether anything more can be done. Wrapping value in something like newtype UrlDecodedQueryValue = QD ByteString (names are just example, not a suggestion) to give the info on the type level seems to be excessive. Maybe a documented type alias like type DecodedQuery = Query would not hurt.

[saurabhnanda]

I think I may have run into this bug as well.... I'm currently in the middle of a bug-fixing session and with DeepQuery, I can see that in my ToDeepQuery instance (["name"], Just "saurabh nanda") is implicitly converted into name=saurabh%20nanda, but I need to manually call urlDecode True in my FromDeepQuery instance.

[tchoutri]

@zlondrej would you be interested in implementing your proposal?

[zlondrej]

Sure, but might take longer as it will not be my priority.