Add an option to disable URL Encoding

[uharaqo]

Subsystem
ktor-http/common

Motivation
ktor-client cannot be used for URLs which are not supported by the URL Encoding format. It's because ktor-client always uses URLBuidler and there's no way to bypass the query encoding.

This PR adds an option to disable URL Encoding via ParametersBuilder (which can be passed to URLBuidler) without breaking changes.

(I mentioned in this issue that /api?k1=v1;k1=v1 (non-encoded ;) is preventing us to use ktor-client. We needed to switch to another library just because of one character in the URL.)

Solution
formUrlEncodeTo() method is doing actual encoding. Added an option to bypass the encoding to ParametersBuilder and passed it to the method.

• Created UrlEncodingOption enum which provides encoding options (DEFAULT, KEY_ONLY, VALUE_ONLY, NO_ENCODING)
• Added the option to ParametersBuilder constructor
• Added the option to Parameters interface as a method and to ParametersImpl as a private property
• Changed formUrlEncodeTo() method to do URL Encoding only when it's enabled by the option
• The option added to methods and interfaces are defaulted to DEFAULT (i.e. keys and values are encoded) so as not to break existing code

[Prototik]

Please fix breaking ABI and run ./gradlew -Poverwrite.output=true :binary-compatibility-validator:jvmTest to ensure that all original methods exists.

[uharaqo]

Please fix breaking ABI and run ./gradlew -Poverwrite.output=true :binary-compatibility-validator:jvmTest to ensure that all original methods exists.

Thanks for reviewing this @Prototik . Could you review this again?
I've recovered the original code to keep binary compatibility, and updated the file in binary-compatibility-validator/reference-public-api.

[Prototik]

Looks like a hack for me.

Move urlEncodingOption to primary constructor and provide a secondary deprecated constructor to keep abi (the same applies to ParametersBuilder too).

All other things LGFM

[uharaqo]

Now the code looks straightforward to me 🙂 Thanks a lot for many useful advice!

[Prototik]

lgfm

[cy6erGn0m]

The only question here: why do we disable escaping at all? The format you mentioned is not URL parameters at all so the only you need is to specify raw query string without parameters. Disabling escaping for sure will lead to security issues and strange bugs.

[uharaqo]

I totally agree with you. Raw query string would be much better. Actually I tried to add raw query support, but I couldn't come up with any solution. If you have any ideas, please let me know.
These are the restrictions which prevented to me to go forward:

• actual query string is built from the Parameters in the format like encodedKey=encodedVal&...
• Parameters wraps Map<String, List<String>> and built from ParametersBuilder
• ParametersBuilder is exposed as a property of URLBuilder which is used everywhere

By the way, it seems that ?k1=v1;k1=v2 is a valid query.
(I spent some time to check URL and query specifications. Good discussions in here. The syntax for query is defined in here. query can be anything as long as they are composed of valid characters. It doesn't need to be key/value pairs. W3C defines the k1=v1&k2=v2 syntax in here, but that is specific to application/x-www-form-urlencoded. It seems that the well known 'query param' syntax is just a convention, which is surprising to me. For example, W3C used to recommend ; as a separator instead of &.)

So, the answer to this question why do we disable escaping at all? would be:

• Since query param syntax is not defined in the URL spec, raw query string should be supported and query param support should be an additional feature
• But raw query support requires many changes. I don't think those edge case scenarios deserve the effort
• Letting users take care of the encoding looks the easiest and reasonable solution

I hope this would help other developers who need to call legacy APIs...

[kevincianfarini]

@cy6erGn0m

The only question here: why do we disable escaping at all? The format you mentioned is not URL parameters at all so the only you need is to specify raw query string without parameters. Disabling escaping for sure will lead to security issues and strange bugs.

My personal use case is that the bittorrent protocal specifies that data encoded in the URL must be in binary format when making announce calls. It's a binary protocall after all.

Note that all binary data in the URL (particularly info_hash and peer_id) must be properly escaped. This means any byte not in the set 0-9, a-z, A-Z, '.', '-', '_' and '~', must be encoded using the "%nn" format, where nn is the hexadecimal value of the byte. (See RFC1738 for details.)

For a 20-byte hash of \x12\x34\x56\x78\x9a\xbc\xde\xf1\x23\x45\x67\x89\xab\xcd\xef\x12\x34\x56\x78\x9a.

The right encoded form is %124Vx%9A%BC%DE%F1%23Eg%89%AB%CD%EF%124Vx%9A

So, when I make the following request

        val response = client.get<ByteArray>(announceUrl) {
            url {
                parameters.append("uploaded", uploaded.toString())
                parameters.append("downloaded", downloaded.toString())
                parameters.append("compact", if (acceptCompact) "1" else "0")
                parameters.append(
                        "info_hash",
                        infoHash.chunked(2).joinToString("") { "%$it" }
                )
                parameters.append("peer_id", peerId)
                parameters.append("port", port.toString())
                parameters.append("left", "${torrentSize - downloaded}")
            }
        }

I expect that the % characters I put in the URl have a way to avoid being encoded. I expect my call to look something like this

GET /announce?info_hash=%65%14%5e%d4%d7%45%cf%c9%3f%5f%fe%34%92%e9%cd%e5%4b%55%7d%9f&peer_id=-CR0001-012345678901&uploaded=0&downloaded=0&left= 2082816000&port=6889&compact=1 HTTP/1.1
Host: torrent.ubuntu.com
Accept: */*
Cache-Control: no-cache
Host: torrent.ubuntu.com
Accept-Encoding: gzip, deflate
Connection: keep-alive
cache-control: no-cache

but instead I get this.

https://torrent.ubuntu.com/announce?uploaded=0&downloaded=0&compact=1&info_hash=%2565%2514%255e%25d4%25d7%2545%25cf%25c9%253f%255f%25fe%2534%2592%25e9%25cd%25e5%254b%2555%257d%259f&peer_id=-CR0001-012345678901&port=0&left=2082816000

notice here that the % have been encoded. I see no way around this without having a way to disable URL encoding.

[kevincianfarini]

Is there any more discussion on this that needs to happen?

I've since moved to retrofit which allows optional encoding via @Query("info_hash", encoded = true) infoHash: String where encoded specifies that the user has already URL encoded the value.

Retrofit is heavy handed for my use case and for the time being limits multiplatform capabilities.

[kevincianfarini]

@uharaqo could you rebase this branch on master? @e5l said he'd be able to review it :)

[jschneidereit]

I'm affected by this as well in a pretty unpleasant way, and am wondering about getting this added 🤔

[klausner17]

Some news about this issue?

[uharaqo]

I've rebased this branch on the latest master.

Please let me know if I need to change the code. I stopped using Ktor because of this issue, but updating this again because I received messages from some people who faced similar problems.

[hhariri]

https://youtrack.jetbrains.com/issue/KTOR-1717

[e5l]

LGTM

[e5l]

We don't need this annotation anymore

[e5l]

Thanks for the PR! Merged.