DEFAULT_ENCODE_SET doesn't percent-encode / and %

[cmbrandenburg]

I'm writing a CouchDB client crate. With CouchDB, unlike with many file-serving web servers, the / and % characters are often valid characters within a path component. For example, foo/bar is a valid database name, and qux%baz is a valid document name. An HTTP request line to get a document might look something like this:

GET /foo%2Fbar/qux%25baz HTTP/1.1

My crate starts with a base Url provided by the application (e.g., http://couchdb-server:1234/) and then manipulates the Url's path to add a database name, document name, etc. to form an HTTP request. The following code snippet demonstrates the path manipulation:

let app_input = "http://couchdb-server/";
let mut u = url::Url::parse(app_input).unwrap();
{
    let mut p = u.path_mut().unwrap();
    p.clear();
    p.push("foo/bar".to_string());
    p.push("qux%baz".to_string());
}
println!("database: {}", u.path().unwrap()[0]);
println!("document: {}", u.path().unwrap()[1]);
println!("URI     : {}", u);

Output I would like to see:

database: foo/bar
document: qux%baz
URI     : http://couchdb-server/foo%2Fbar/qux%25baz

Actual output:

database: foo/bar
document: qux%baz
URI     : http://couchdb-server/foo/bar/qux%baz

Fair enough. After reading through #149, I understand the url crate's goals aren't exactly aligned with my hopes. I only need to be explicit about the percent-encoding and everything will work, right? Wrong.

let app_input = "http://couchdb-server/";
let mut u = url::Url::parse(app_input).unwrap();
{
    use url::percent_encoding::{percent_encode, DEFAULT_ENCODE_SET};
    let mut p = u.path_mut().unwrap();
    p.clear();
    p.push(percent_encode("foo/bar".as_bytes(), DEFAULT_ENCODE_SET));
    p.push(percent_encode("qux%baz".as_bytes(), DEFAULT_ENCODE_SET));
}
println!("database: {}", u.path().unwrap()[0]);
println!("document: {}", u.path().unwrap()[1]);
println!("URI     : {}", u);

Output I expect:

database: foo/bar
document: qux%baz
URI     : http://couchdb-server/foo%2Fbar/qux%25baz

Actual output:

database: foo/bar
document: qux%baz
URI     : http://couchdb-server/foo/bar/qux%baz

WTF? Percent-encoding using the DEFAULT_ENCODE_SET, which is for path components, doesn't percent-encode / and % characters? This means percent-encoding and percent-decoding aren't inverses.

use url::percent_encoding::{percent_decode, percent_encode, DEFAULT_ENCODE_SET};
let a = "qux%baz";
let b = percent_encode(a.as_bytes(), DEFAULT_ENCODE_SET);
let c = String::from_utf8(percent_decode(b.as_bytes())).unwrap();
assert!(a == c);

Actual output:

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: FromUtf8Error { bytes: [113, 117, 120, 186, 122], error: Utf8Error { valid_up_to: 3 } }', ../src/libcore/result.rs:738
Process didn't exit successfully: `target/debug/rust-scratch` (exit code: 101)

I think this violates the Principle of Least Astonishment. Percent-decoding should undo percent-encoding.

Anyway, for my CouchDB use case, the workaround is to explicitly use string methods to replace / and % characters with %2F and %25 before calling the url::percent_encode function. However, this is a tedious workaround—and it requires a couple extra allocations.

[SimonSapin]

I haven’t read in details yet, sorry. (Gotta run soon.) Would #151 help?

[SimonSapin]

Also, some notes:

• The contents of url.path() are percent-encoded, same as in the serialization.
• The percent_encode function is primarily written to support the parsing algorithm at https://url.spec.whatwg.org/#url-parsing , which in turn is designed to deal with input like <a href="/Été%20et%20hiver/"> which can contain both characters (such as non-ASCII) that need to be percent-encoded, and already-percent-encoded sequences that shouldn’t be double-encoded.
• My goal with this library is to support Servo, where compatibility with existing web content (even stupidly broken) is more important than a "Principle of Least Astonishment", which by the way is very subjective.

[cmbrandenburg]

@SimonSapin Thank you for the quick response. Yes, #151 may help, but I think we can do better.

(This is a long comment, but I have a concrete proposal at the end of it. Everything between here and that proposal is to make sure we understand each others' intentions. You know the url crate a lot better than I do.)

If I understand correctly, a custom EncodeSet implementation would allow use cases such as mine to avoid the explicit string replacement operations and extra allocations I mentioned in my previous comment. Very nice! Furthermore, I could take an existing encode set, such as DEFAULT_ENCODE_SET, and wrap it with a shim to add support for percent-encoding / and %. Even better! Being able to extend an existing encode set is important to me because I want to minimize the amount of percent-encoding logic in my CouchDB crate.

That said, I think my use case will prove inconveniently common for your url crate. As much as you want to focus on supporting Servo, your Url type is used in hyper, and hyper is on its way to being the bedrock of most things Web in Rust. This means your Url type is fundamental to a lot of crates. Take mine as an example. I'm using your Url type because I'm using hyper—and my situation is very different than Servo's.

However, popularity needn't be a problem. I accept that my use case isn't the only one—or even the most important one—and I'm okay with whole-string-parsing not handling percent-encoding as I would like it to. Ergo, whole-string-parsing behavior should match whatever makes the most sense in Servo. But I'm not doing whole-string-parsing. I'm manipulating individual path components, and I expect that to handle percent-encoding. This is what my examples are about in my previous comment. Please take a few minutes to understand them.

Why do I expect individual-path-component-manipulation to handle percent-encoding? Suppose I take an existing Url instance and replace all path components (via the path_mut method) with a single path component of the value foo/bar. I expect that to serialize to a URI string with one path component, foo%2Fbar, but the current Url implementation serializes to a string with two components, foo and bar. If I foo and bar then I would have used something like vec!["foo", "bar"]. What's the point of being able to manipulate individual path components if there's no guarantee the contents of one path component won't spill over into other path components?

A proposal

Maybe what's best is to add another method to Url. Maybe something like:

pub fn replace_path_components(&mut self, components: Vec<String>);

The replace_path_components method would percent-encode each string in the vector, including the / and % characters, and then do something like: *self.path_mut() = components.

The new replace_path_components method would allow the path and path_mut methods to remain unchanged for backwards compatibility while allowing crates like mine to have safe access to individual path components without the need no deal with percent-encoding logic. I think this is a win for everybody.

I can implement this and would like to. What do you think?

[cmbrandenburg]

Yet another idea is to have a normalizing function for path components. Maybe something like:

fn normalize_path_component<T: Into<String>>(component: T) -> String;

This would be a standalone function that, given a path component string, returns the percent-encoded version of the string suitable for remaining a single, valid path component when serialized. This means encoding the string according to DEFAULT_ENCODE_SET as well as percent-encoding / and % characters.

assert_eq!("foobar",    normalize_path_component("foobar"));
assert_eq!("foo%20bar", normalize_path_component("foo bar"));
assert_eq!("foo%3Fbar", normalize_path_component("foo?bar"));
assert_eq!("foo%2Fbar", normalize_path_component("foo/bar"));
assert_eq!("foo%25bar", normalize_path_component("foo%bar"));

An application could “normalize” the path portion of a Url instance, like so:

uri.path_mut().into_iter().map(|x| normalize_path_component(x));

This would contain all percent-encoding logic within the url crate, maintain full compatibility with the crate's existing API in v0.5, and generally keeps things simple for use cases such as Servo's and use cases such as mine.

[SimonSapin]

#176 adds Url::push_path_segment that percent-encodes % and /. You can construct the desired URL with repeated calls. (It also adds Url::pop_path_segment). I’ve set the relevant commit to automatically close this issue when it is merged.