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

Set rejectUnauthorized to true by default #3149

Merged
merged 1 commit into from Nov 28, 2021
Merged

Conversation

scott-ut
Copy link
Contributor

@scott-ut scott-ut commented Jul 16, 2021

Resolve CVE-2020-240-25 by setting rejectUnauthorized to true by default.

Add configuration flag to override this to false if necessary.

Add doc option to README.md

@scott-ut
Copy link
Contributor Author

@scott-ut scott-ut commented Jul 16, 2021

This would resolve #3067

@xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Jul 18, 2021

Thanks! I've added the Release - Next Major flag since this is a breaking change.

Resolve CVE-2020-240-25 by setting rejectUnauthorized to true by default.

Add configuration flag to override this to false if necessary.

extract rejectUnauthorized download option to its own file.

Add doc option to README.md.
@scott-ut scott-ut changed the title WIP: Set rejectUnauthorized to true by default Set rejectUnauthorized to true by default Jul 19, 2021
@scott-ut
Copy link
Contributor Author

@scott-ut scott-ut commented Jul 19, 2021

Added other options for changing the configuration setting and updated README. Have removed WIP tag from PR. A couple of build steps failed but I couldn't see why - is that expected @xzyfer ?

@xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Jul 19, 2021

Yes some of the Alpine build jobs fail. It's expected.

@scott-ut
Copy link
Contributor Author

@scott-ut scott-ut commented Aug 13, 2021

@xzyfer - any update on when this will be merged?

@scott-ut
Copy link
Contributor Author

@scott-ut scott-ut commented Oct 11, 2021

Hi @xzyfer - just wondering when I can expect this to be released?

@nschonni nschonni mentioned this pull request Oct 22, 2021
6 tasks
@xzyfer xzyfer merged commit 0a21792 into sass:master Nov 28, 2021
2 checks passed
xzyfer added a commit that referenced this issue Nov 28, 2021
Fix lint issue introduced in #3149.
@JGJP
Copy link

@JGJP JGJP commented Feb 24, 2022

@xzyfer
If this is a security fix then can we have it in a patch or minor update on 6.x.x? Upgrading to node-sass 7 is causing trouble for my peer dependencies that is not trivial to resolve. I suspect that other users will also avoid the upgrade.

@xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Feb 25, 2022

We put this into in v7 because it was technically a breaking change.

@JGJP
Copy link

@JGJP JGJP commented Feb 25, 2022

@xzyfer what part of it is breaking? I see the addition of a flag, but no change in the existing flags

@xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Feb 25, 2022

@JGJP
Copy link

@JGJP JGJP commented Feb 25, 2022

feels like a minor change to me, it doesn't break functionality right? None of the current binaries should be requiring that flag

@JGJP
Copy link

@JGJP JGJP commented Feb 25, 2022

@xzyfer Would appreciate a deeper look at the feasability of a minor update, I'm definitely not the only one that can't upgrade to 7 easily, and I'll probably ignore this update or patch it manually until I can do a broader tooling upgrade

@JGJP
Copy link

@JGJP JGJP commented Mar 2, 2022

patch file:

diff --git a/node_modules/node-sass/scripts/util/downloadoptions.js b/node_modules/node-sass/scripts/util/downloadoptions.js
index 2352971..e9056b1 100644
--- a/node_modules/node-sass/scripts/util/downloadoptions.js
+++ b/node_modules/node-sass/scripts/util/downloadoptions.js
@@ -1,5 +1,6 @@
 var proxy = require('./proxy'),
-  userAgent = require('./useragent');
+  userAgent = require('./useragent'),
+  rejectUnauthorized = require('./rejectUnauthorized');
 
 /**
  * The options passed to request when downloading the bibary
@@ -14,7 +15,7 @@ var proxy = require('./proxy'),
  */
 module.exports = function() {
   var options = {
-    rejectUnauthorized: false,
+    rejectUnauthorized: rejectUnauthorized(),
     timeout: 60000,
     headers: {
       'User-Agent': userAgent(),
diff --git a/node_modules/node-sass/scripts/util/rejectUnauthorized.js b/node_modules/node-sass/scripts/util/rejectUnauthorized.js
new file mode 100644
index 0000000..a1c8010
--- /dev/null
+++ b/node_modules/node-sass/scripts/util/rejectUnauthorized.js
@@ -0,0 +1,46 @@
+var pkg = require('../../package.json');
+
+/**
+ * Get the value of a CLI argument
+ *
+ * @param {String} name
+ * @param {Array} args
+ * @api private
+ */
+ function getArgument(name, args) {
+  var flags = args || process.argv.slice(2),
+    index = flags.lastIndexOf(name);
+
+  if (index === -1 || index + 1 >= flags.length) {
+    return null;
+  }
+
+  return flags[index + 1];
+}
+
+/**
+ * Get the value of reject-unauthorized
+ * If environment variable SASS_REJECT_UNAUTHORIZED is non-zero,
+ * .npmrc variable sass_reject_unauthorized or
+ * process argument --sass-reject_unauthorized is provided,
+ * set rejectUnauthorized to true
+ * Else set to false by default
+ *
+ * @return {Boolean} The value of rejectUnauthorized
+ * @api private
+ */
+module.exports = function() {
+  var rejectUnauthorized = false;
+
+  if (getArgument('--sass-reject-unauthorized')) {
+    rejectUnauthorized = getArgument('--sass-reject-unauthorized');
+  } else if (process.env.SASS_REJECT_UNAUTHORIZED !== '0') {
+    rejectUnauthorized = true;
+  } else if (process.env.npm_config_sass_reject_unauthorized) {
+    rejectUnauthorized = process.env.npm_config_sass_reject_unauthorized;
+  } else if (pkg.nodeSassConfig && pkg.nodeSassConfig.rejectUnauthorized) {
+    rejectUnauthorized = pkg.nodeSassConfig.rejectUnauthorized;
+  } 
+
+  return rejectUnauthorized;
+};
diff --git a/node_modules/node-sass/test/downloadoptions.js b/node_modules/node-sass/test/downloadoptions.js
index de89638..a6e2d9b 100644
--- a/node_modules/node-sass/test/downloadoptions.js
+++ b/node_modules/node-sass/test/downloadoptions.js
@@ -8,7 +8,7 @@ describe('util', function() {
     describe('without a proxy', function() {
       it('should look as we expect', function() {
         var expected = {
-          rejectUnauthorized: false,
+          rejectUnauthorized: true,
           timeout: 60000,
           headers: {
             'User-Agent': ua(),
@@ -33,7 +33,7 @@ describe('util', function() {
 
       it('should look as we expect', function() {
         var expected = {
-          rejectUnauthorized: false,
+          rejectUnauthorized: true,
           proxy: proxy,
           timeout: 60000,
           headers: {
@@ -57,6 +57,25 @@ describe('util', function() {
         delete process.env.HTTP_PROXY;
       });
 
+      it('should look as we expect', function() {
+        var expected = {
+          rejectUnauthorized: true,
+          timeout: 60000,
+          headers: {
+            'User-Agent': ua(),
+          },
+          encoding: null,
+        };
+
+        assert.deepStrictEqual(opts(), expected);
+      });
+    });
+
+    describe('with SASS_REJECT_UNAUTHORIZED set to false', function() {
+      beforeEach(function() {
+        process.env.SASS_REJECT_UNAUTHORIZED = '0';
+      });
+
       it('should look as we expect', function() {
         var expected = {
           rejectUnauthorized: false,
@@ -70,5 +89,47 @@ describe('util', function() {
         assert.deepStrictEqual(opts(), expected);
       });
     });
+
+    describe('with SASS_REJECT_UNAUTHORIZED set to true', function() {
+      beforeEach(function() {
+        process.env.SASS_REJECT_UNAUTHORIZED = '1';
+      });
+
+      it('should look as we expect', function() {
+        var expected = {
+          rejectUnauthorized: true,
+          timeout: 60000,
+          headers: {
+            'User-Agent': ua(),
+          },
+          encoding: null,
+        };
+
+        assert.deepStrictEqual(opts(), expected);
+      });
+    });
+
+    describe('with npm_config_sass_reject_unauthorized set to true', function() {
+      beforeEach(function() {
+        process.env.npm_config_sass_reject_unauthorized = true;
+      });
+
+      it('should look as we expect', function() {
+        var expected = {
+          rejectUnauthorized: true,
+          timeout: 60000,
+          headers: {
+            'User-Agent': ua(),
+          },
+          encoding: null,
+        };
+
+        assert.deepStrictEqual(opts(), expected);
+      });
+
+      afterEach(function() {
+        process.env.npm_config_sass_reject_unauthorized = undefined;
+      });
+    });
   });
 });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants