url: show input in parse error message

joyeecheung · MylesBorins · commit f6755182e544 · 2017-03-28T19:06:18.000-04:00
PR-URL: #11934 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
diff --git a/lib/internal/url.js b/lib/internal/url.js
@@ -83,8 +83,6 @@ class TupleOrigin {
 
 function onParseComplete(flags, protocol, username, password,
                          host, port, path, query, fragment) {
-  if (flags & binding.URL_FLAGS_FAILED)
-    throw new TypeError('Invalid URL');
   var ctx = this[context];
   ctx.flags = flags;
   ctx.scheme = protocol;
@@ -102,20 +100,24 @@ function onParseComplete(flags, protocol, username, password,
   initSearchParams(this[searchParams], query);
 }
 
+function onParseError(flags, input) {
+  const error = new TypeError('Invalid URL: ' + input);
+  error.input = input;
+  throw error;
+}
+
 // Reused by URL constructor and URL#href setter.
 function parse(url, input, base) {
   input = String(input);
   const base_context = base ? base[context] : undefined;
   url[context] = new StorageObject();
   binding.parse(input.trim(), -1,
                 base_context, undefined,
-                onParseComplete.bind(url));
+                onParseComplete.bind(url), onParseError);
 }
 
 function onParseProtocolComplete(flags, protocol, username, password,
                                  host, port, path, query, fragment) {
-  if (flags & binding.URL_FLAGS_FAILED)
-    return;
   const newIsSpecial = (flags & binding.URL_FLAGS_SPECIAL) !== 0;
   const s = this[special];
   const ctx = this[context];
@@ -144,8 +146,6 @@ function onParseProtocolComplete(flags, protocol, username, password,
 
 function onParseHostComplete(flags, protocol, username, password,
                              host, port, path, query, fragment) {
-  if (flags & binding.URL_FLAGS_FAILED)
-    return;
   const ctx = this[context];
   if (host) {
     ctx.host = host;
@@ -159,8 +159,6 @@ function onParseHostComplete(flags, protocol, username, password,
 
 function onParseHostnameComplete(flags, protocol, username, password,
                                  host, port, path, query, fragment) {
-  if (flags & binding.URL_FLAGS_FAILED)
-    return;
   const ctx = this[context];
   if (host) {
     ctx.host = host;
@@ -172,15 +170,11 @@ function onParseHostnameComplete(flags, protocol, username, password,
 
 function onParsePortComplete(flags, protocol, username, password,
                              host, port, path, query, fragment) {
-  if (flags & binding.URL_FLAGS_FAILED)
-    return;
   this[context].port = port;
 }
 
 function onParsePathComplete(flags, protocol, username, password,
                              host, port, path, query, fragment) {
-  if (flags & binding.URL_FLAGS_FAILED)
-    return;
   const ctx = this[context];
   if (path) {
     ctx.path = path;
@@ -192,16 +186,12 @@ function onParsePathComplete(flags, protocol, username, password,
 
 function onParseSearchComplete(flags, protocol, username, password,
                                host, port, path, query, fragment) {
-  if (flags & binding.URL_FLAGS_FAILED)
-    return;
   const ctx = this[context];
   ctx.query = query;
 }
 
 function onParseHashComplete(flags, protocol, username, password,
                              host, port, path, query, fragment) {
-  if (flags & binding.URL_FLAGS_FAILED)
-    return;
   const ctx = this[context];
   if (fragment) {
     ctx.fragment = fragment;
diff --git a/src/node_url.cc b/src/node_url.cc
@@ -1246,7 +1246,8 @@ namespace url {
                     enum url_parse_state state_override,
                     Local<Value> base_obj,
                     Local<Value> context_obj,
-                    Local<Function> cb) {
+                    Local<Function> cb,
+                    Local<Value> error_cb) {
     Isolate* isolate = env->isolate();
     Local<Context> context = env->context();
     HandleScope handle_scope(isolate);
@@ -1267,20 +1268,19 @@ namespace url {
 
     // Define the return value placeholders
     const Local<Value> undef = Undefined(isolate);
-    Local<Value> argv[9] = {
-      undef,
-      undef,
-      undef,
-      undef,
-      undef,
-      undef,
-      undef,
-      undef,
-      undef,
-    };
-
-    argv[ARG_FLAGS] = Integer::NewFromUnsigned(isolate, url.flags);
     if (!(url.flags & URL_FLAGS_FAILED)) {
+      Local<Value> argv[9] = {
+        undef,
+        undef,
+        undef,
+        undef,
+        undef,
+        undef,
+        undef,
+        undef,
+        undef,
+      };
+      argv[ARG_FLAGS] = Integer::NewFromUnsigned(isolate, url.flags);
       if (url.flags & URL_FLAGS_HAS_SCHEME)
         argv[ARG_PROTOCOL] = OneByteString(isolate, url.scheme.c_str());
       if (url.flags & URL_FLAGS_HAS_USERNAME)
@@ -1297,22 +1297,31 @@ namespace url {
         argv[ARG_PORT] = Integer::New(isolate, url.port);
       if (url.flags & URL_FLAGS_HAS_PATH)
         argv[ARG_PATH] = Copy(env, url.path);
+      (void)cb->Call(context, recv, arraysize(argv), argv);
+    } else if (error_cb->IsFunction()) {
+      Local<Value> argv[2] = { undef, undef };
+      argv[ERR_ARG_FLAGS] = Integer::NewFromUnsigned(isolate, url.flags);
+      argv[ERR_ARG_INPUT] =
+        String::NewFromUtf8(env->isolate(),
+                            input,
+                            v8::NewStringType::kNormal).ToLocalChecked();
+      (void)error_cb.As<Function>()->Call(context, recv, arraysize(argv), argv);
     }
-
-    (void)cb->Call(context, recv, 9, argv);
   }
 
   static void Parse(const FunctionCallbackInfo<Value>& args) {
     Environment* env = Environment::GetCurrent(args);
     CHECK_GE(args.Length(), 5);
-    CHECK(args[0]->IsString());
-    CHECK(args[2]->IsUndefined() ||
+    CHECK(args[0]->IsString());  // input
+    CHECK(args[2]->IsUndefined() ||  // base context
           args[2]->IsNull() ||
           args[2]->IsObject());
-    CHECK(args[3]->IsUndefined() ||
+    CHECK(args[3]->IsUndefined() ||  // context
           args[3]->IsNull() ||
           args[3]->IsObject());
-    CHECK(args[4]->IsFunction());
+    CHECK(args[4]->IsFunction());  // complete callback
+    CHECK(args[5]->IsUndefined() || args[5]->IsFunction());  // error callback
+
     Utf8Value input(env->isolate(), args[0]);
     enum url_parse_state state_override = kUnknownState;
     if (args[1]->IsNumber()) {
@@ -1325,7 +1334,8 @@ namespace url {
           state_override,
           args[2],
           args[3],
-          args[4].As<Function>());
+          args[4].As<Function>(),
+          args[5]);
   }
 
   static void EncodeAuthSet(const FunctionCallbackInfo<Value>& args) {
diff --git a/src/node_url.h b/src/node_url.h
@@ -463,6 +463,10 @@ static inline void PercentDecode(const char* input,
   XX(ARG_QUERY)                                                               \
   XX(ARG_FRAGMENT)
 
+#define ERR_ARGS(XX)                                                          \
+  XX(ERR_ARG_FLAGS)                                                           \
+  XX(ERR_ARG_INPUT)                                                           \
+
 static const char kEOL = -1;
 
 enum url_parse_state {
@@ -484,6 +488,12 @@ enum url_cb_args {
 #undef XX
 };
 
+enum url_error_cb_args {
+#define XX(name) name,
+  ERR_ARGS(XX)
+#undef XX
+} url_error_cb_args;
+
 static inline bool IsSpecial(std::string scheme) {
 #define XX(name, _) if (scheme == name) return true;
   SPECIALS(XX);
diff --git a/test/parallel/test-whatwg-url-parsing.js b/test/parallel/test-whatwg-url-parsing.js
@@ -13,15 +13,30 @@ if (!common.hasIntl) {
 
 // Tests below are not from WPT.
 const tests = require(path.join(common.fixturesDir, 'url-tests'));
+const failureTests = tests.filter((test) => test.failure).concat([
+  { input: '' },
+  { input: 'test' },
+  { input: undefined },
+  { input: 0 },
+  { input: true },
+  { input: false },
+  { input: null },
+  { input: new Date() },
+  { input: new RegExp() },
+  { input: () => {} }
+]);
 
-for (const test of tests) {
-  if (typeof test === 'string')
-    continue;
-
-  if (test.failure) {
-    assert.throws(() => new URL(test.input, test.base),
-                  /^TypeError: Invalid URL$/);
-  }
+for (const test of failureTests) {
+  assert.throws(
+    () => new URL(test.input, test.base),
+    (error) => {
+      // The input could be processed, so we don't do strict matching here
+      const match = (error + '').match(/^TypeError: Invalid URL: (.*)$/);
+      if (!match) {
+        return false;
+      }
+      return error.input === match[1];
+    });
 }
 
 const additional_tests = require(
