http.request behavior changed between 0.10.5 and 0.11.2

[bjouhier]

http.request encodes the URL path in 0.11.2 but not in 0.10.5

Repro:

var http = require('http');

console.log("NODE VERSION: " + process.version);

http.createServer(function(req, res) {
    res.writeHead(200, {
        'Content-Type': 'text/plain'
    });
    console.log('server got: ' + req.url)
    res.end('ok: ' + new Date());
}).listen(1337, 'localhost', function() {
    //console.log('Server running at http://127.0.0.1:1337/');
    http.request({
        host: 'localhost',
        port: 1337,
        path: '/abc?foo=bar%20zoo',
        method: 'GET'
    }, function(resp) {
        //console.log('got response: ' + resp.statusCode);
    }).end();
});

v0.10.5 output:

NODE VERSION: v0.10.5
server got: /abc?foo=bar%20zoo

v0.11.2 output:

NODE VERSION: v0.11.2
server got: /abc?foo=bar%2520zoo

If I don't encode the space and set path to '/abc?foo=bar zoo' in the request call, I get a crash in v0.10.5 and a success with server got: /abc?foo=bar%20zoo in v0.11.2.

[bnoordhuis]

That change was introduced (deliberately) in 38149bb.

Rock, hard place: the old behavior makes it easy to create invalid requests but the new behavior is inconvenient and non-intuitive. Maybe I should just revert it and have it throw an error when the path contains illegal characters.

Thoughts?

[awwright]

As a developer I wouldn't rely on any validity parsing, all it should serve to do is uncover bugs. Additionally consider developers may want to send invalid requests. I wouldn't do validity checking for this reason.

If the "path" is checked for validity, keep in mind that there's several valid forms; it may be a literal *, an absolute path, or it may be an absolute URI (any URI, even URNs - a feature I use, a feature that HTTP/1.1 servers are required to support, and one required for making requests to proxies).

[bnoordhuis]

Well, the particular case that commit aims to address is paths with spaces in them. Those are never valid because the request line ends up looking like GET /foo bar HTTP/1.1. You're even able to inject headers that way if the developer is sloppy.

[awwright]

Perhaps this: If it's a Buffer, use the value as-is. If it's a string, then it must be unicode, therefore Node.js should apply the IRI-to-URI transformation as specified in RFC 3987 3.1. This transformation is one that will also convert spaces to %20 etc. This could still mean that an invalid domain name is provided, but that isn't necessarily a problem.

[bjouhier]

The problem is that you had to escape the URL with versions <= 0.10 because http.request did fail if the URL contained spaces. Now, if you upgrade to 0.11 your URL gets escaped twice. This broke many of our unit tests :-(

To be backward compatible you should either 1) do nothing, 2) throw if the URL contains spaces, 3) escape only the space chars. I have a slight preference for 2).

[bjouhier]

Thanks.