Skip to content

Conversation

@mtrenkmann
Copy link
Contributor

This PR fixes percent-encoding when uri_builder::append_query_key_value_pair() is used.

There are two changes:

  1. The key and value components are percent-encoded separately and then joined together with the = sign as separator.
  2. The encode function detail::encode_query_component() that percent-encodes everything except unreserved characters, slash (/), and question mark (?) has been added for that purpose. This is in line with RFC 3986 section 3.4.

This way it is possible to use = and % in a key/value pair, because they'll be percent-encoded. Currently this is not possible since = and % are excluded from percent-encoding. The latter is even worse, because an URI decoder would expect a percent-encoded octet when reading the unencoded % sign.

  • Some unit tests have also been added.
  • All unit tests passed.

Request for Comments

The uri_builder::append_query() interface is easy to misuse in my opinion, which is sub-optimal for a class whose purpose is to create valid objects. Example:

network::uri_builder ub(...);
ub.append_query("q=" + get_unchecked_input());

If get_unchecked_input() returns a string with characters in /.@&%;=, those characters will not be percent-encoded (see detail::encode_query()) which leads to broken URI strings.

I would suggest to rename this function to uri_builder::append_query_unsafe() or removed it completely. What do you think?

@glynos glynos merged commit ee74d8a into cpp-netlib:master Oct 18, 2018
@glynos
Copy link
Member

glynos commented Oct 18, 2018

Many thanks again for your contribution. I'm happy to integrate these changes.

As for your request, I am aware of the problem with this function. I don't know how much this function is being used in the wild, though I don't expect it is used very much. Can you add this as an issue? I will consider one of the options you propose, but I'd like to be able to trace the reason for any API change if code that depends on it breaks as a consequence.

@mtrenkmann
Copy link
Contributor Author

Ok, I'll add an issue for this (tomorrow). I think I have an idea, how to keep this function while making it more safe.

@mtrenkmann mtrenkmann deleted the fix-percent-encoding-in-query-component branch October 18, 2018 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants