Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Whatwg url #239

Merged
merged 6 commits into from Mar 5, 2019
Merged

Whatwg url #239

Changes from 5 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -123,15 +123,23 @@ internals.Client.prototype.request = function (method, url, options = {}) {

internals.Client.prototype._request = function (method, url, options, relay, _trace) {

const uri = Url.parse(url);

const uri = {};
let parsedUri;
if (options.socketPath) {
uri.socketPath = options.socketPath;
delete options.socketPath;
parsedUri = Url.parse(url);
}
else {
uri.setHost = false;
This conversation was marked as resolved by geek

This comment has been minimized.

Copy link
@geek

geek Feb 28, 2019

Member

Should this impact the code on line 142?

This comment has been minimized.

Copy link
@WesTyler

WesTyler Mar 1, 2019

Author Contributor

So it turns out that in order to maintain compatibility with Url.parse, the http request agent will override host with hostname if both are present. By setting setHost: false and manually setting the host header on L142 I was able to preserve the port in the host header.

This isn't strictly needed, but without it this is a technically breaking change:

const promise = Wreck.request('get', '/foo', { baseUrl: 'http://localhost:0/' });
await expect(promise).to.reject();
expect(promise.req._headers.host).to.equal('localhost:0');
/*Expected 'localhost' to equal specified value: 'localhost:0'*/

Similar to the auth prop on the event, I'll let you decide on the importance of preserving that port in the header. Just let me know :)

This comment has been minimized.

Copy link
@geek

geek Mar 1, 2019

Member

Got it, that makes sense to keep it how you currently have it implemented then.

parsedUri = new Url.URL(url);
}

internals.applyUrlToOptions(uri, parsedUri);

uri.method = method.toUpperCase();
uri.headers = options.headers || {};
uri.headers.host = parsedUri.host;
const hasContentLength = internals.findHeader('content-length', uri.headers) !== undefined;

if (options.payload && typeof options.payload === 'object' && !(options.payload instanceof Stream) && !Buffer.isBuffer(options.payload)) {
@@ -170,7 +178,7 @@ internals.Client.prototype._request = function (method, url, options, relay, _tr
uri.agent = options.agent;
}
else {
uri.agent = uri.protocol === 'https:' ? this.agents.https : this.agents.http;
uri.agent = parsedUri.protocol === 'https:' ? this.agents.https : this.agents.http;
This conversation was marked as resolved by geek

This comment has been minimized.

Copy link
@geek

geek Mar 5, 2019

Member

can this change to uri?

This comment has been minimized.

Copy link
@WesTyler

WesTyler Mar 5, 2019

Author Contributor

Done. I finally did a findAll on parsedUri and I think the only 2 uses left are the needed ones.

This comment has been minimized.

Copy link
@geek

geek Mar 5, 2019

Member

You rock, thanks for this PR... I'll release as 14.2.0. I'm going to bring in the changes from the other open PR as well around adding a new event.

}

if (options.secureProtocol !== undefined) {
@@ -650,5 +658,29 @@ internals.findHeader = function (headerName, headers) {
return foundKey && headers[foundKey];
};

internals.applyUrlToOptions = (options, url) => {
This conversation was marked as resolved by geek

This comment has been minimized.

Copy link
@geek

geek Feb 28, 2019

Member

Any thoughts on copying over the node version, which will also have the auth bits: https://github.com/nodejs/node/blob/master/lib/internal/url.js#L1257-L1276

This comment has been minimized.

Copy link
@WesTyler

WesTyler Mar 1, 2019

Author Contributor

Yeah I initially did just pull in the Node function, then I pared it down to what the current implementation was as far as I could tell.

I'm all for including the auth bits if you'd like. I can add some tests around it, too. Was just trying to keep the initial pass at this as small as possible. :)


options.origin = url.origin;
options.searchParams = url.searchParams;
options.protocol = url.protocol;
options.hostname = url.hostname;
options.hash = url.hash;
options.search = url.search;
options.pathname = url.pathname;
options.path = `${url.pathname}${url.search || ''}`;
options.href = url.href;
if (url.port !== '') {
options.port = Number(url.port);
}

if (url.username || url.password) {
options.auth = `${url.username}:${url.password}`;
options.username = url.username;
options.password = url.password;
}

return options;
};


module.exports = new internals.Client();
@@ -419,7 +419,12 @@ describe('request()', () => {
redirected: (statusCode, location, req) => {

expect(location).to.equal('https://hapijs.com');
expect(req.output[0]).to.include('hapijs.com');
if (req.output) {
expect(req.output[0]).to.include('hapijs.com');
}
else {
expect(req.outputData[0].data).to.include('hapijs.com');
}
}
};

@@ -437,6 +442,13 @@ describe('request()', () => {
server.close();
});

it('handles uri with WHATWG parsing', async () => {

const promise = Wreck.request('get', 'http://localhost%60malicious.org',);
await expect(promise).to.reject();
expect(promise.req._headers.host).to.equal('localhost`malicious.org');
});

it('reaches max redirections count', async () => {

let gen = 0;
@@ -889,6 +901,22 @@ describe('request()', () => {
Wreck.agents.http.maxSockets = Infinity;
});

it('sets the auth value on the request', async () => {

const promise = Wreck.request('get', '/foo', { baseUrl: 'http://username:password@localhost:0/' });
await expect(promise).to.reject();
expect(promise.req._headers.host).to.equal('localhost:0');
expect(promise.req._headers).to.include('authorization');
});

it('sets the auth value on the request with missing username', async () => {

const promise = Wreck.request('get', '/foo', { baseUrl: 'http://:password@localhost:0/' });
await expect(promise).to.reject();
expect(promise.req._headers.host).to.equal('localhost:0');
expect(promise.req._headers).to.include('authorization');
});

describe('unix socket', () => {

it('requests a resource', async () => {
@@ -1896,13 +1924,14 @@ describe('Events', () => {
const wreck = Wreck.defaults({ events: true });
wreck.events.once('request', (uri, options) => {

expect(uri.href).to.equal('http://localhost:' + server.address().port + '/');
expect(uri.href).to.equal('http://user:pass@localhost:' + server.address().port + '/');
expect(options).to.exist();
expect(uri.auth).to.equal('user:pass');

uri.headers.foo = 'bar';
});

const { res, payload } = await wreck.put('http://localhost:' + server.address().port);
const { res, payload } = await wreck.put('http://user:pass@localhost:' + server.address().port);
expect(res.statusCode).to.equal(200);
expect(payload.toString()).to.equal('ok');
server.close();
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.