Repository: electron-userland/electron-forge
Pull request #337 (2017-09-26 18:50:51)

fix(generic): ensure config proxy doesn't prevent access to built-ins
ISSUES CLOSED: #336 

* [x] I have read the [contribution documentation](https://github.com/electron-userland/electron-forge/blob/master/CONTRIBUTING.md) for this project.
* [x] I agree to follow the [code of conduct](https://github.com/electron/electron/blob/master/CODE_OF_CONDUCT.md) that this project follows, as appropriate.
* [x] The changes are appropriately documented (if applicable).
* [x] The changes have sufficient test coverage (if applicable).
* [x] The testsuite passes successfully on my local machine (if applicable).

**Summarize your changes:**

Prevent hiding built-ins when proxying config. Previously a check to `hasOwnProperty` fail (since this is a built-in) and no value would be returned.


gerges: It's worth noting returning the real `hasOwnProperty` will likely not respect the env var stuff you're doing. We could alternatively trap this via `getOwnPropertyDescriptor`, but I think you'll just hit this again the next time someone attempts to access a built in.
gerges: I've added that support as well

Patch:
diff --git a/src/util/forge-config.js b/src/util/forge-config.js
index 785401f57..d607af51b 100644
--- a/src/util/forge-config.js
+++ b/src/util/forge-config.js
@@ -18,11 +18,19 @@ const proxify = (object, envPrefix) => {
 
   return new Proxy(newObject, {
     get(target, name) {
-      if (target.hasOwnProperty(name)) return target[name]; // eslint-disable-line no-prototype-builtins
-      if (typeof name === 'string') {
+      // eslint-disable-next-line no-prototype-builtins
+      if (!target.hasOwnProperty(name) && typeof name === 'string') {
         const envValue = process.env[`${envPrefix}_${underscoreCase(name)}`];
         if (envValue) return envValue;
       }
+      return target[name];
+    },
+    getOwnPropertyDescriptor(target, name) {
+      const envValue = process.env[`${envPrefix}_${underscoreCase(name)}`];
+      // eslint-disable-next-line no-prototype-builtins
+      if (target.hasOwnProperty(name) || envValue) {
+        return { configurable: true, enumerable: true };
+      }
     },
   });
 };


Test patch:
diff --git a/test/fast/forge-config_spec.js b/test/fast/forge-config_spec.js
index d617e67de..9d5319972 100644
--- a/test/fast/forge-config_spec.js
+++ b/test/fast/forge-config_spec.js
@@ -37,10 +37,19 @@ describe('forge-config', () => {
       },
     }));
   });
+  it('should allow access to built-ins of proxied objects', async () => {
+    const conf = await findConfig(path.resolve(__dirname, '../fixture/dummy_js_conf'));
+    expect(conf.electronPackagerConfig.baz.hasOwnProperty).to.be.a('function');
+    process.env.ELECTRON_FORGE_S3_SECRET_ACCESS_KEY = 'SecretyThing';
+    // eslint-disable-next-line no-prototype-builtins
+    expect(conf.s3.hasOwnProperty('secretAccessKey')).to.equal(true);
+    delete process.env.ELECTRON_FORGE_S3_SECRET_ACCESS_KEY;
+  });
+
 
   it('should resolve the JS file exports in config.forge points to a JS file', async () => {
     expect(JSON.parse(JSON.stringify(await findConfig(path.resolve(__dirname, '../fixture/dummy_js_conf'))))).to.be.deep.equal(Object.assign({}, defaults, {
-      electronPackagerConfig: { foo: 'bar' },
+      electronPackagerConfig: { foo: 'bar', baz: {} },
     }));
   });
 
diff --git a/test/fixture/dummy_js_conf/forge.config.js b/test/fixture/dummy_js_conf/forge.config.js
index d2c17999a..e24e909ef 100644
--- a/test/fixture/dummy_js_conf/forge.config.js
+++ b/test/fixture/dummy_js_conf/forge.config.js
@@ -5,7 +5,7 @@ module.exports = {
     linux: ['deb', 'rpm'],
     mas: ['zip'],
   },
-  electronPackagerConfig: { foo: 'bar' },
+  electronPackagerConfig: { foo: 'bar', baz: {} },
   electronWinstallerConfig: {},
   electronInstallerDebian: {},
   electronInstallerRedhat: {},