fix(authorize): build redirect URIs with the WHATWG URL API instead of url.parse() #513

docs/api/response-types/code-response-type.md

@@ -25,7 +25,7 @@
 **Kind**: instance method of [<code>CodeResponseType</code>](#CodeResponseType)  
 **Throws**:
 
-- <code>InvalidArgumentError</code> if redirectUri is missing
+- <code>InvalidArgumentError</code> if redirectUri is missing or not an absolute URI
 
 
 | Param |

docs/api/utils/url-util.md

@@ -0,0 +1,41 @@
+<a name="module_UrlUtil"></a>
+
+## UrlUtil
+
+* [UrlUtil](#module_UrlUtil)
+    * [~parseRedirectUri(redirectUri, [parseQueryString])](#module_UrlUtil..parseRedirectUri) ⇒ <code>object</code>
+    * [~parseAuth(parsed)](#module_UrlUtil..parseAuth) ⇒ <code>string</code> \| <code>null</code>
+
+<a name="module_UrlUtil..parseRedirectUri"></a>
+
+### UrlUtil~parseRedirectUri(redirectUri, [parseQueryString]) ⇒ <code>object</code>
+Parses a redirect URI with the WHATWG `URL` API and returns it in the shape
+of Node's legacy `url.parse()` result, which `url.format()` serializes.
+The redirect builders keep their return type without calling `url.parse()`,
+which Node deprecates at runtime (DEP0169).
+
+**Kind**: inner method of [<code>UrlUtil</code>](#module_UrlUtil)  
+**Returns**: <code>object</code> - the parsed URI: `protocol`, `slashes`, `auth`, `host`, `port`, `hostname`, `hash`, `search`,
+  `query`, `pathname`, `path` and `href`  
+**Throws**:
+
+- <code>InvalidArgumentError</code> if `redirectUri` is not an absolute URI
+
+
+| Param | Type | Default | Description |
+| --- | --- | --- | --- |
+| redirectUri | <code>string</code> |  | an absolute URI |
+| [parseQueryString] | <code>boolean</code> | <code>false</code> | return `query` as an object, as `url.parse(redirectUri, true)` does |
+
+<a name="module_UrlUtil..parseAuth"></a>
+
+### UrlUtil~parseAuth(parsed) ⇒ <code>string</code> \| <code>null</code>
+The userinfo as `url.parse()` returns it: decoded, `user` or `user:password`,
+and `null` when there is none. `url.format()` encodes it again.
+
+**Kind**: inner method of [<code>UrlUtil</code>](#module_UrlUtil)  
+
+| Param | Type |
+| --- | --- |
+| parsed | <code>URL</code> | 
+

lib/handlers/authorize-handler.js

@@ -21,6 +21,7 @@ const tokenUtil = require('../utils/token-util');
 const url = require('url');
 const pkce = require('../pkce/pkce');
 const { parseScope } = require('../utils/scope-util');
+const { parseRedirectUri } = require('../utils/url-util');
 
 /**
  * Response types.
@@ -358,7 +359,7 @@ class AuthorizeHandler {
    */
 
   buildErrorRedirectUri(redirectUri, error) {
-    const uri = url.parse(redirectUri);
+    const uri = parseRedirectUri(redirectUri);
 
     uri.query = {
       error: error.name,

lib/response-types/code-response-type.js

@@ -5,7 +5,7 @@
  */
 
 const InvalidArgumentError = require('../errors/invalid-argument-error');
-const url = require('url');
+const { parseRedirectUri } = require('../utils/url-util');
 
 /**
  * @class
@@ -28,14 +28,14 @@ class CodeResponseType {
   /**
    * @param redirectUri
    * @return {UrlWithParsedQuery}
-   * @throws {InvalidArgumentError} if redirectUri is missing
+   * @throws {InvalidArgumentError} if redirectUri is missing or not an absolute URI
    */
   buildRedirectUri(redirectUri) {
     if (!redirectUri) {
       throw new InvalidArgumentError('Missing parameter: `redirectUri`');
     }
 
-    const uri = url.parse(redirectUri, true);
+    const uri = parseRedirectUri(redirectUri, true);
 
     uri.query.code = this.code;
     uri.search = null;

lib/utils/url-util.js

@@ -0,0 +1,84 @@
+'use strict';
+
+/*
+ * Module dependencies.
+ */
+
+const InvalidArgumentError = require('../errors/invalid-argument-error');
+const querystring = require('querystring');
+
+/**
+ * @module UrlUtil
+ */
+
+/**
+ * Parses a redirect URI with the WHATWG `URL` API and returns it in the shape
+ * of Node's legacy `url.parse()` result, which `url.format()` serializes.
+ * The redirect builders keep their return type without calling `url.parse()`,
+ * which Node deprecates at runtime (DEP0169).
+ *
+ * @param {string} redirectUri an absolute URI
+ * @param {boolean} [parseQueryString=false] return `query` as an object, as `url.parse(redirectUri, true)` does
+ * @return {object} the parsed URI: `protocol`, `slashes`, `auth`, `host`, `port`, `hostname`, `hash`, `search`,
+ *   `query`, `pathname`, `path` and `href`
+ * @throws {InvalidArgumentError} if `redirectUri` is not an absolute URI
+ */
+function parseRedirectUri(redirectUri, parseQueryString = false) {
+  let parsed;
+
+  try {
+    parsed = new URL(redirectUri);
+  } catch {
+    throw new InvalidArgumentError('Invalid parameter: `redirectUri`');
+  }
+
+  // `URL` reports an empty query (`…/cb?`) as `''`, as if there were none;
+  // `url.parse()` keeps it as `'?'`.
+  const search = parsed.search || (parsed.href.split('#')[0].endsWith('?') ? '?' : null);
+  const pathname = parsed.pathname || null;
+  // `url.parse()` gives an IPv6 hostname without its brackets; `host` keeps them.
+  const hostname = parsed.hostname.startsWith('[') ? parsed.hostname.slice(1, -1) : parsed.hostname;
+  let query = search ? search.slice(1) : null;
+
+  if (parseQueryString) {
+    query = querystring.parse(query || '');
+  }
+
+  return {
+    protocol: parsed.protocol,
+    slashes: parsed.href.startsWith(`${parsed.protocol}//`) ? true : null,
+    auth: parseAuth(parsed),
+    host: parsed.host || null,
+    port: parsed.port || null,
+    hostname: hostname || null,
+    hash: parsed.hash || null,
+    search,
+    query,
+    pathname,
+    path: pathname || search ? `${pathname || ''}${search || ''}` : null,
+    href: parsed.href,
+  };
+}
+
+/**
+ * The userinfo as `url.parse()` returns it: decoded, `user` or `user:password`,
+ * and `null` when there is none. `url.format()` encodes it again.
+ *
+ * @param {URL} parsed
+ * @return {string|null}
+ */
+function parseAuth(parsed) {
+  if (!parsed.username && !parsed.password) {
+    return null;
+  }
+
+  const auth = parsed.password ? `${parsed.username}:${parsed.password}` : parsed.username;
+
+  try {
+    return decodeURIComponent(auth);
+  } catch {
+    return auth;
+  }
+}
+
+module.exports = { parseRedirectUri };

test/integration/handlers/authorize-handler_test.js

@@ -19,7 +19,9 @@ const Response = require('../../../lib/response');
 const ServerError = require('../../../lib/errors/server-error');
 const UnauthorizedClientError = require('../../../lib/errors/unauthorized-client-error');
 const should = require('chai').should();
+const sinon = require('sinon');
 const url = require('url');
+const { parseRedirectUri } = require('../../../lib/utils/url-util');
 
 const createModel = (model = {}) => {
   return Model.from({
@@ -457,6 +459,91 @@ describe('AuthorizeHandler integration', function () {
         .should.equal('http://example.com/cb?code=fooobar-long-authzcode-%3F&state=foobarbazstatemoo');
     });
 
+    it('should build the redirect without the deprecated url.parse()', async function () {
+      const client = {
+        id: 'client-12343434',
+        grants: ['authorization_code'],
+        redirectUris: ['http://example.com/cb'],
+      };
+      const model = createModel({
+        getAccessToken: async function () {
+          return {
+            client,
+            user: {},
+            accessTokenExpiresAt: new Date(new Date().getTime() + 10000),
+          };
+        },
+        getClient: async function () {
+          return { ...client };
+        },
+        saveAuthorizationCode: async function () {
+          return { authorizationCode: 'foo', client };
+        },
+      });
+      const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model });
+      const request = new Request({
+        body: { client_id: client.id, response_type: 'code' },
+        headers: { Authorization: 'Bearer foobarbaztokenmoo' },
+        method: {},
+        query: { state: 'foobarbazstatemoo' },
+      });
+      const response = new Response({ body: {}, headers: {} });
+      const parse = sinon.spy(url, 'parse');
+
+      try {
+        await handler.handle(request, response);
+        parse.called.should.equal(false);
+        response.get('location').should.equal('http://example.com/cb?code=foo&state=foobarbazstatemoo');
+      } finally {
+        parse.restore();
+      }
+    });
+
+    it('should build the error redirect without the deprecated url.parse()', async function () {
+      const client = {
+        id: 'client-12343434',
+        grants: ['authorization_code'],
+        redirectUris: ['http://example.com/cb'],
+      };
+      const model = createModel({
+        getAccessToken: async function () {
+          return {
+            client,
+            user: {},
+            accessTokenExpiresAt: new Date(new Date().getTime() + 10000),
+          };
+        },
+        getClient: async function () {
+          return { ...client };
+        },
+      });
+      const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model });
+      const request = new Request({
+        body: { client_id: client.id, response_type: 'code', allowed: 'false' },
+        headers: { Authorization: 'Bearer foobarbaztokenmoo' },
+        method: {},
+        query: { state: 'foobarbazstatemoo' },
+      });
+      const response = new Response({ body: {}, headers: {} });
+      const parse = sinon.spy(url, 'parse');
+
+      try {
+        await handler.handle(request, response);
+
+        should.fail();
+      } catch (e) {
+        e.should.be.an.instanceOf(AccessDeniedError);
+        parse.called.should.equal(false);
+        response
+          .get('location')
+          .should.equal(
+            'http://example.com/cb?error=access_denied&error_description=Access%20denied%3A%20user%20denied%20access%20to%20application&state=foobarbazstatemoo',
+          );
+      } finally {
+        parse.restore();
+      }
+    });
+
     it('should redirect to an error response if `scope` is invalid', async function () {
       const model = createModel({
         getAccessToken: async function () {
@@ -1813,6 +1900,22 @@ describe('AuthorizeHandler integration', function () {
         .format(redirectUri)
         .should.equal('http://example.com/cb?error=invalid_client&error_description=Bad%20Request');
     });
+
+    it('should throw an error if the `redirectUri` is not an absolute URI', function () {
+      const handler = new AuthorizeHandler({
+        authorizationCodeLifetime: 120,
+        model: createModel(),
+      });
+
+      try {
+        handler.buildErrorRedirectUri('/cb', new InvalidClientError());
+
+        should.fail();
+      } catch (e) {
+        e.should.be.an.instanceOf(InvalidArgumentError);
+        e.message.should.equal('Invalid parameter: `redirectUri`');
+      }
+    });
   });
 
   describe('updateResponse()', function () {
@@ -1827,7 +1930,7 @@ describe('AuthorizeHandler integration', function () {
         model,
       });
       const response = new Response({ body: {}, headers: {} });
-      const uri = url.parse('http://example.com/cb');
+      const uri = parseRedirectUri('http://example.com/cb');
 
       handler.updateResponse(response, uri, 'foobar');
 

test/integration/response-types/code-response-type_test.js

@@ -7,6 +7,7 @@
 const CodeResponseType = require('../../../lib/response-types/code-response-type');
 const InvalidArgumentError = require('../../../lib/errors/invalid-argument-error');
 const should = require('chai').should();
+const sinon = require('sinon');
 const url = require('url');
 
 /**
@@ -60,5 +61,29 @@ describe('CodeResponseType integration', function () {
 
       url.format(redirectUri).should.equal('http://example.com/cb?foo=bar&code=foo');
     });
+
+    it('should throw an error if the `redirectUri` is not an absolute URI', function () {
+      const responseType = new CodeResponseType('foo');
+
+      try {
+        responseType.buildRedirectUri('/cb');
+
+        should.fail();
+      } catch (e) {
+        e.should.be.an.instanceOf(InvalidArgumentError);
+        e.message.should.equal('Invalid parameter: `redirectUri`');
+      }
+    });
+
+    it('should not call the deprecated url.parse()', function () {
+      const parse = sinon.spy(url, 'parse');
+
+      try {
+        new CodeResponseType('foo').buildRedirectUri('http://example.com/cb?foo=bar');
+        parse.called.should.equal(false);
+      } finally {
+        parse.restore();
+      }
+    });
   });
 });