Escape reserved symbols in default implementation of toEncodedUrlPiece

[lagunoff]

Hi) I was investigating an issue with our application where filters wouldn't work correctly if the user enters a plus sign. First I noticed that at some point HTTP request is sent to an endpoint that contains unescaped plus signs, for example if user types "foo+bar@gmail.com" the request would be like "GET /?search_by=foo+bar@gmail.com" (with plus symbol unescaped). We are using servant and servant-client libraries between our backends, I dug deeper into servant-client internals and everything points to default implementation of toEncodedUrlPiece method being the root cause of the problem.

Actually the real source might be further down the stack in the http-types package where I also going to send a pull request, but I suggest you to merge this changes here as well. Currently toEncodedUrlPiece uses encodePathSegmentsRelative which under the hood calls urlEncodeBuilder False which is wrong, because it doesn't escape characters :@&=+$, and the documentation for toEncodedUrlPiece clearly states that it should escape all special characters

[lagunoff]

Raised an issue in http-types aristidb/http-types#102

[phadej]

toEncodedUrlPiece shouldn't be used for query params, their and url piece encoding is different.

Note the comment in implementation of urlEncodeBuilder:

-- | Percent-encoding for URLs (using 'B.Builder').
urlEncodeBuilder
    :: Bool -- ^ Whether input is in query string. True: Query string, False: Path element

toUrlPiece: path element, toQueryParam: query string.

---

Also I checked using nc and curl

Server:

nc - l 8080

and curl

curl 'http://localhost:8080/foo+bar/?param=quu+puu'

The request looks like:

GET /foo+bar/?param=quu+puu HTTP/1.1
Host: localhost:8080
User-Agent: curl/7.58.0
Accept: */*

So plus is not escaped.

[lagunoff]

toEncodedUrlPiece shouldn't be used for query params,

@phadej Hm, but servant-client uses this exacly this method to encode query parameters. It seems that ToHttpApiData should provide some way for encoding query params, if this is done with toEncodedUrlPiece, maybe it should percent-encode these characters :@&=+$, since percent-encoding them inside the piece also works.

Your example doesn't contradict presence of the problem, curl left plus signs intact, as is, but if you would inspect the values in server like php or servant, they would replace plus in second case i.e. server would see param value as quu puu

[phadej]

but if you would inspect the values in server like php or servant,....

>>> parseQueryParam @Text "foo+bar" 
Right "foo+bar"

http-api-data doesn't decode + to . That would be wrong (it's not url encoding/decoding).

I'd like to see a proper reproducer, I'm not convinced the problem is in http-api-data, and as far as HTTP protocol is concerned http-api-data seems to behave correctly. If some other lib (whether it's a client or a server) does something weird, http-api-data shouldn't work around that.

[phadej]

Wait, you said

but servant-client uses this exacly this method to encode query parameters

And that is the cause, it's wrong. Fix servant-client.

[lagunoff]

@phadej As I understand from examples in the docs toQueryParam/parseQueryParam are meant to encode non-text values (like UTCTime for example) into Text and they don't do URL encodong/decoding so the result of toQueryParam might contain special characters, which supposed to be encoded later. And in servant-client this is done with toEncodedUrlPiece
for both pieces and params because ToHttpAPIData doesn't provide other means. If users of http-api-data end up using toEncodedUrlPiece this way, maybe make sense to encode :@&=+$, as it works for both types.

[lagunoff]

@phadej And that is the cause, it's wrong. Fix servant-client.

Alright, maybe you right, also some people might rely on current behavior of toEncodedUrlPiece, but it looks like a design flaw that ToHttpData only provides way to encode pieces not params