Use node 7+ WHATWG parser for hostname, fixes #1002 #1004

fl0w · 2017-06-17T21:28:31Z

really, the ideal solution is a separate module that supports it. if you know of any, let us know!

Well, there's the node 7+ WHATWG compliant url.URL, which could potentially help Koa "de-depend" parseurl and querystring as well (I think).

This PR could be expanded to substitute public API for host/hostname/query and maybe a few more.
There would be some minor adjustments to output like URL.port returning '' when :80 or :443 is provided (special scheme ports). (performance impacted to heavily)

I'm also assuming url.URL would come with some ~~minor~~major performance hit.

const { URL } = require('url')
[
  "https://github.com",
  "http://www.github.com:80",
  "https://127.0.0.1:8080",
  "http://[::1]",
  "https://[::1]:801",
  "https://[2001:db8::1428:57ab]",
  "http://[2001:db8::1428:57ab]:805/index.js"
].forEach(url => {
  let parsed = new URL(url)
  console.log(`${url} - hostname: ${parsed.hostname}, port: ${parsed.port}`)
})

$ node -v
v8.1.0
$ node whatwg.js
https://github.com - hostname: github.com, port: 
http://www.github.com:80 - hostname: www.github.com, port: 
https://127.0.0.1:8080 - hostname: 127.0.0.1, port: 8080
http://[::1] - hostname: [::1], port: 
https://[::1]:801 - hostname: [::1], port: 801
https://[2001:db8::1428:57ab] - hostname: [2001:db8::1428:57ab], port: 
http://[2001:db8::1428:57ab]:805/index.js - hostname: [2001:db8::1428:57ab], port: 805

notice that all port 80 returns empty string, as http and https are special scheme, thus parsed implicitly

codecov · 2017-06-17T21:31:37Z

Codecov Report

Merging #1004 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1004   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files           5       5           
  Lines         356     366   +10     
======================================
+ Hits          356     366   +10

Impacted Files	Coverage Δ
lib/context.js	`100% <ø> (ø)`	⬆️
lib/request.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beec26e...84da722. Read the comment docs.

fl0w · 2017-06-17T21:40:39Z

lib/request.js

@@ -5,6 +5,7 @@
 * Module dependencies.
 */

+const URL = require('url').URL;
 const net = require('net');
 const contentType = require('content-type');
 const stringify = require('url').format;


Thought about destructing here (const { format: stringify, URL } = require('url')),
though I'm not seeing it elsewhere so assumed it was taboo in koajs/koa.

@fl0w i think we didn't add it for node v4 support previously

hiroppy

I think you should change node's engines in package.json to 7.0.0.

fl0w · 2017-06-19T12:42:25Z

@abouthiroppy that will mess with users who transpile Koa down to e.g. node 6.

fl0w · 2017-06-19T12:44:37Z

lib/request.js

@@ -242,9 +243,12 @@ module.exports = {
   */

  get hostname() {


This needs some adjusting - the code path is weird and for some reason I'm not using host whilst also assigning const host = this.host (relic from the past). I'll clean this up later today.

fengmk2 · 2017-06-19T14:29:52Z

lib/request.js

+    if (!this.parsedUrl) {
+      const host = this.host;
+      if (!host) return '';
+      this.parsedUrl = new URL(`${this.protocol}://${this.host}`);


How about the performance? Slow down or faster?

Use WHATWG parser for hostname when host start with [, otherwise keep the old way to get high performance.

Mind sharing your bench src?

@fl0w https://gist.github.com/fengmk2/5b25066b3bdedbfc7223f5a59902c725

seems like a potential module to me :)

@jonathanong the hostname validation or bench src? :)

fengmk2

Consider performance before merge.

fl0w · 2017-06-19T23:40:10Z

@fengmk2 @jonathanong
I exposed parsed URL and made it complete with originalUrl ~~(maybe used for other stuff now or in the future, like Punycode? Where request.hostname will return empty)~~ Edit: actually this was presumptuous of me - I'm not 100% sure about current behaviour.

The naming might be conflicting with url?
Kept memoization.

I'll add docs and tests for get URL() if this is an acceptable solution (and naming).

fl0w · 2017-06-19T23:54:54Z

lib/request.js

+      const protocol = this.protocol;
+      const host = this.host;
+      const originalUrl = this.originalUrl || ''; // avoid undefined in template string
+      this.memoizedURL = new URL(`${protocol}://${host}${originalUrl}`);


URL will throw TypeError if something's off.
Koa generally has value || '' type of solutions in most cases.

Maybe wrap this in a try-catch and memoize empty object so that this.URL.hostname when malformed does not throw/crash the entire app?

A try-catch shouldn't deopt since node 7.x (v8/v8@9aac80f)

Maybe wrap this in a try-catch and memoize empty object so that this.URL.hostname when malformed does not throw/crash the entire app?

+1

fengmk2 · 2017-06-20T01:21:23Z

lib/request.js

@@ -244,10 +245,29 @@ module.exports = {
  get hostname() {
    const host = this.host;
    if (!host) return '';
+    if ('[' == host[0]) return this.URL.hostname; // IPv6


this.URL can be an empty object, return this.URL.hostname || '' here

urugator · 2017-06-22T14:49:16Z

I am not sure if this is a fortunate solution.
I think it brings an inconsitency into how request object state relates to the underlying req object depending on the hostname format.
Until now the request was always up to date with the req object. So any changes of req were accordingly projected to the request.
Currently due to a memoization this is no longer true - if I do request.headers['host'] = 'rewrittenHost' the output of request.hostname will depend on the hostname format and whether it has been already accessed or not. Even wierdly the request.host can mismatch the request.hostname.
I don't know to which extend the modifications of req inside middlewares are supported, but I think the behavior should not depend on hostname format as it can be a source of confusion and unpredictability.
Also note that returned URL object can be modified outside of request object, possibly changing the outcome of request.hostname again based on the hostname format...
Maybe you could make everything (protocol, hostname, url...) dependent on that URL object by default, keeping it as a source of truth instead of the wrapped req. It woudn't be ideal, but at least quite consistent.

fl0w · 2017-06-22T14:52:36Z

Yes, this is a side effect I've been pondering about. As a quick solution, you can simply clear memoization by setting request.memoizedURL = undefined, this will force a new parse.

This was done because the parse felt quite costly (as noted by fengmk2).

edit

Maybe you could make everything (protocol, hostname, url...) dependent on that URL object by default, keeping it as a source of truth instead of the wrapped req. It woudn't be ideal, but at least quite consistent.

This was my initial proposition, but currently it seems to be hitting performance with significance. This was in comparison to request.host, not sure about the actual implications on a real world application.

fl0w · 2017-06-22T15:00:09Z

@urugator though correct me if I'm wrong - this should only be of concern when hostname is called with an IPv6 host?

As far as inconsistencies, memoization could be removed but it would still require one to fetch a new URL object every time request.req.[headers|host|*] is changed.

urugator · 2017-06-22T17:09:30Z

@fl0w The concern is that code behaves differently based on the hostname format.
In one request hostname depends on req.headers[host] and in another on memoizedUrl (where both are exposed to user as different things).
If I have a middleware for changing headers like req.headers[host] = newHostname, it will work for some requests, but not for anothers.
Similary middleware which modifies URL object like request.URL.hostname = newHostname; sometimes also modifies request.hostname and sometimes not.
The same goes for modifying request.URL.host, which sometimes also modifies request.hostname, but never request.host.
The behavior also depends on to which format the headers["host"] was possibly updated, whether it happened after or before URL was acessed (memoized), etc.
I think it should be clear how things depend on one another and how changing one thing affects another and this behavior should stay always the same and shouldn't depend on host format.

Actually I would say that simply stating that Ipv6 hostname is not supported is a more acceptable solution, because seems easier to reason about.

However I still wonder whether the WHATWG parser is really neccessary for this.

require one to fetch a new URL

It has to be kept in sync manually, but not re-constructed:

const newHost = "myLittleHost"
request.headers["host"] = newHost;
request.URL.host = newHost;

Use node 7+ WHATWG parser for hostname, fixes koajs#1002

477c5aa

fl0w commented Jun 17, 2017

View reviewed changes

jonathanong approved these changes Jun 18, 2017

View reviewed changes

hiroppy suggested changes Jun 19, 2017

View reviewed changes

fl0w commented Jun 19, 2017

View reviewed changes

fengmk2 reviewed Jun 19, 2017

View reviewed changes

fengmk2 requested changes Jun 19, 2017

View reviewed changes

only use URL if host is IPv6, expose parsed URL

1218adf

fl0w commented Jun 19, 2017

View reviewed changes

fengmk2 reviewed Jun 20, 2017

View reviewed changes

fengmk2 approved these changes Jun 20, 2017

View reviewed changes

fl0w added 2 commits June 20, 2017 09:11

catch invalid URLs, memoize empty obj

bdf53fa

hostname returns empty string when URL throws

84da722

jonathanong merged commit 327b65c into koajs:master Jun 20, 2017

jonathanong mentioned this pull request Jul 16, 2017

have problem to handle urlencoded GET param #1000

Closed

fl0w deleted the whatwg-hostname branch September 22, 2017 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use node 7+ WHATWG parser for hostname, fixes #1002 #1004

Use node 7+ WHATWG parser for hostname, fixes #1002 #1004

fl0w commented Jun 17, 2017 •

edited

Loading

codecov bot commented Jun 17, 2017 •

edited

Loading

fl0w Jun 17, 2017 •

edited

Loading

jonathanong Jun 18, 2017

hiroppy left a comment

fl0w commented Jun 19, 2017

fl0w Jun 19, 2017

fengmk2 Jun 19, 2017

fengmk2 Jun 19, 2017

fengmk2 Jun 19, 2017

fl0w Jun 19, 2017 •

edited

Loading

fengmk2 Jun 19, 2017

jonathanong Jun 19, 2017

fl0w Jun 19, 2017 •

edited

Loading

fengmk2 left a comment

fl0w commented Jun 19, 2017 •

edited

Loading

fl0w Jun 19, 2017 •

edited

Loading

fl0w Jun 20, 2017

dead-horse Jun 20, 2017

fengmk2 Jun 20, 2017

dead-horse Jun 20, 2017

urugator commented Jun 22, 2017

fl0w commented Jun 22, 2017 •

edited

Loading

fl0w commented Jun 22, 2017

urugator commented Jun 22, 2017 •

edited

Loading

Use node 7+ WHATWG parser for hostname, fixes #1002 #1004

Use node 7+ WHATWG parser for hostname, fixes #1002 #1004

Conversation

fl0w commented Jun 17, 2017 • edited Loading

codecov bot commented Jun 17, 2017 • edited Loading

Codecov Report

fl0w Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiroppy left a comment

Choose a reason for hiding this comment

fl0w commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fl0w Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fl0w Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

fengmk2 left a comment

Choose a reason for hiding this comment

fl0w commented Jun 19, 2017 • edited Loading

fl0w Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urugator commented Jun 22, 2017

fl0w commented Jun 22, 2017 • edited Loading

fl0w commented Jun 22, 2017

urugator commented Jun 22, 2017 • edited Loading

fl0w commented Jun 17, 2017 •

edited

Loading

codecov bot commented Jun 17, 2017 •

edited

Loading

fl0w Jun 17, 2017 •

edited

Loading

fl0w Jun 19, 2017 •

edited

Loading

fl0w Jun 19, 2017 •

edited

Loading

fl0w commented Jun 19, 2017 •

edited

Loading

fl0w Jun 19, 2017 •

edited

Loading

fl0w commented Jun 22, 2017 •

edited

Loading

urugator commented Jun 22, 2017 •

edited

Loading