From 52aaed3b3c8dbda82f4da0cc76efe9fe466742f3 Mon Sep 17 00:00:00 2001 From: Kid <44045911+kidonng@users.noreply.github.com> Date: Mon, 4 Jul 2022 11:06:12 +0000 Subject: [PATCH 1/4] esm: treat `307` and `308` as redirects in HTTPS imports Per RFC 7231 and 7238, HTTP `307` and `308` status code are also for redirect responses. Fixes: https://github.com/nodejs/node/issues/43679 Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7 Refs: https://datatracker.ietf.org/doc/html/rfc7238 --- lib/internal/modules/esm/fetch_module.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js index c65587e34c488f..c816f59bf4a072 100644 --- a/lib/internal/modules/esm/fetch_module.js +++ b/lib/internal/modules/esm/fetch_module.js @@ -107,7 +107,11 @@ function fetchWithRedirects(parsed) { // `finally` on network error/timeout. const { 0: res } = await once(req, 'response'); try { - const isRedirect = res.statusCode >= 300 && res.statusCode <= 303; + const isRedirect = res.statusCode >= 300 && ( + res.statusCode <= 303 || + res.statusCode === 307 || + res.statusCode === 308 + ); const hasLocation = ObjectPrototypeHasOwnProperty(res.headers, 'location'); if (isRedirect && hasLocation) { const location = new URL(res.headers.location, parsed); @@ -127,7 +131,12 @@ function fetchWithRedirects(parsed) { err.message = `Cannot find module '${parsed.href}', HTTP 404`; throw err; } - if (res.statusCode > 303 || res.statusCode < 200) { + if ( + ( + res.statusCode > 303 && + res.statusCode !== 307 && + res.statusCode !== 308 + ) || res.statusCode < 200) { throw new ERR_NETWORK_IMPORT_DISALLOWED( res.headers.location, parsed.href, From 54ee622bb8fa4daa1fce34960bb0e8b4c33c41ff Mon Sep 17 00:00:00 2001 From: Kid <44045911+kidonng@users.noreply.github.com> Date: Wed, 6 Jul 2022 09:29:07 +0000 Subject: [PATCH 2/4] Split `isRedirect` into its own function per review --- lib/internal/modules/esm/fetch_module.js | 27 +++++++++++++----------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js index c816f59bf4a072..a72fdbcbc06274 100644 --- a/lib/internal/modules/esm/fetch_module.js +++ b/lib/internal/modules/esm/fetch_module.js @@ -89,6 +89,18 @@ function createUnzip() { return createUnzip(); } +/** + * Redirection status code as per section 6.4 of RFC 7231: + * https://datatracker.ietf.org/doc/html/rfc7231#section-6.4 + * and RFC 7238: + * https://datatracker.ietf.org/doc/html/rfc7238 + * @param {number} statusCode + * @returns {boolean} + */ +function isRedirect(statusCode) { + return [300, 301, 302, 303, 307, 308].includes(statusCode); +} + /** * @param {URL} parsed * @returns {Promise | CacheEntry} @@ -107,13 +119,8 @@ function fetchWithRedirects(parsed) { // `finally` on network error/timeout. const { 0: res } = await once(req, 'response'); try { - const isRedirect = res.statusCode >= 300 && ( - res.statusCode <= 303 || - res.statusCode === 307 || - res.statusCode === 308 - ); const hasLocation = ObjectPrototypeHasOwnProperty(res.headers, 'location'); - if (isRedirect && hasLocation) { + if (isRedirect(res.statusCode) && hasLocation) { const location = new URL(res.headers.location, parsed); if (location.protocol !== 'http:' && location.protocol !== 'https:') { throw new ERR_NETWORK_IMPORT_DISALLOWED( @@ -131,12 +138,8 @@ function fetchWithRedirects(parsed) { err.message = `Cannot find module '${parsed.href}', HTTP 404`; throw err; } - if ( - ( - res.statusCode > 303 && - res.statusCode !== 307 && - res.statusCode !== 308 - ) || res.statusCode < 200) { + if ((res.statusCode > 303 && !isRedirect(res.statusCode)) || + res.statusCode < 200) { throw new ERR_NETWORK_IMPORT_DISALLOWED( res.headers.location, parsed.href, From 542b5cf203c75da458d1e4193c0dc67e8b9281bf Mon Sep 17 00:00:00 2001 From: Kid <44045911+kidonng@users.noreply.github.com> Date: Wed, 6 Jul 2022 16:39:18 +0000 Subject: [PATCH 3/4] Apply review suggestions --- lib/internal/modules/esm/fetch_module.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js index a72fdbcbc06274..f8d7e30c65533f 100644 --- a/lib/internal/modules/esm/fetch_module.js +++ b/lib/internal/modules/esm/fetch_module.js @@ -1,5 +1,6 @@ 'use strict'; const { + ArrayPrototypeIncludes, ObjectPrototypeHasOwnProperty, PromisePrototypeThen, SafeMap, @@ -98,7 +99,7 @@ function createUnzip() { * @returns {boolean} */ function isRedirect(statusCode) { - return [300, 301, 302, 303, 307, 308].includes(statusCode); + return ArrayPrototypeIncludes([300, 301, 302, 303, 307, 308], statusCode); } /** @@ -138,8 +139,7 @@ function fetchWithRedirects(parsed) { err.message = `Cannot find module '${parsed.href}', HTTP 404`; throw err; } - if ((res.statusCode > 303 && !isRedirect(res.statusCode)) || - res.statusCode < 200) { + if (res.statusCode < 200 || res.statusCode >= 400) { throw new ERR_NETWORK_IMPORT_DISALLOWED( res.headers.location, parsed.href, From 9ba00b1d9c236182c64a47019f02082d4b036c88 Mon Sep 17 00:00:00 2001 From: Kid <44045911+kidonng@users.noreply.github.com> Date: Thu, 7 Jul 2022 11:46:43 +0800 Subject: [PATCH 4/4] Use a switch --- lib/internal/modules/esm/fetch_module.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js index f8d7e30c65533f..19ff8c4b946346 100644 --- a/lib/internal/modules/esm/fetch_module.js +++ b/lib/internal/modules/esm/fetch_module.js @@ -1,6 +1,5 @@ 'use strict'; const { - ArrayPrototypeIncludes, ObjectPrototypeHasOwnProperty, PromisePrototypeThen, SafeMap, @@ -99,7 +98,17 @@ function createUnzip() { * @returns {boolean} */ function isRedirect(statusCode) { - return ArrayPrototypeIncludes([300, 301, 302, 303, 307, 308], statusCode); + switch (statusCode) { + case 300: // Multiple Choices + case 301: // Moved Permanently + case 302: // Found + case 303: // See Other + case 307: // Temporary Redirect + case 308: // Permanent Redirect + return true; + default: + return false; + } } /**