fix: Protect config files from declaring invalid keys #792

shubheksha · 2017-02-06T18:40:29Z

Fixes #772

shubheksha · 2017-02-06T18:41:04Z

src/config.js

 type ApplyConfigToArgvParams = {|
  argv: Object,
-  configObject: Object,
+  configObject?: Object,


@kumar303, this was an unintended error, I'll fix this in the next commit.

ok (in reply to https://github.com/mozilla/web-ext/pull/792/files#r99653520)

coveralls · 2017-02-06T18:57:02Z

Coverage remained the same at 100.0% when pulling 71fd8f6 on shubheksha:fix/772 into 07e1f3b on mozilla:master.

kumar303

Great start! In addition to these change requests, the config function should also throw an error for non camel-cased options (because they won't end up being applied to the program). I saw your note that you couldn't think of a way to do it but there is a way. Start by adding a failing test and I can help you think through it.

kumar303 · 2017-02-06T19:05:14Z

src/config.js

 type ApplyConfigToArgvParams = {|
  argv: Object,
-  configObject: Object,
+  configObject?: Object,


ok (in reply to https://github.com/mozilla/web-ext/pull/792/files#r99653520)

kumar303 · 2017-02-06T19:11:21Z

tests/unit/test.config.js

        },
      });
      const configObject = {
        foo: '/configured/foo',


The same comment applies to this test. Change foo to a valid option.

kumar303 · 2017-02-06T19:12:49Z

tests/unit/test.config.js

        },
      });
      const configObject = {
        foo: true,


The same comment applies to this test. Change foo to a valid option.

kumar303 · 2017-02-06T19:20:11Z

tests/unit/test.config.js

+      };
+      assert.throws(() => {
+        applyConfigToArgv({argv, configObject, defaultValues});
+      }, UsageError, /Please fix your config and try again/);


This regex should make sure that the option name was specified in the error message

kumar303 · 2017-02-06T19:21:00Z

tests/unit/test.config.js

        },
      });
      const configObject = {
        foo: '/configured/foo',


This test should not throw an error because it's testing that default values are preserved. You can fix it by changing foo to some kind of valid option. However, you will have to define a new option in addition to source-dir to do that.

coveralls · 2017-02-06T21:06:39Z

Coverage remained the same at 100.0% when pulling ca9dd2e on shubheksha:fix/772 into 07e1f3b on mozilla:master.

kumar303

Thanks for the quick fixes, this is getting closer

kumar303 · 2017-02-06T21:29:01Z

src/config.js

    }
-    if (!argv.hasOwnProperty(option)) {
+    if (!argv.hasOwnProperty(option) &&
+      !argv.hasOwnProperty(decamelize(option, '-'))) {


can't this just be if (!argv.hasOwnProperty(decamelize(option, '-'))) { ... ?

kumar303 · 2017-02-06T21:29:33Z

src/config.js

        'because this is an unknown option');
-      continue;
+      throw new UsageError(`The option ${option} is invalid.` +
+        ' Please fix your config and try again.');


This error message should have the path to the config so that the user can find out which config they need to fix. This means applyConfigToArgv() will need to accept a filename argument.

kumar303 · 2017-02-06T21:30:13Z

src/config.js

-    if (!argv.hasOwnProperty(option)) {
+    if (!argv.hasOwnProperty(option) &&
+      !argv.hasOwnProperty(decamelize(option, '-'))) {
      log.debug(`Ignoring configuration: ${option}=${configObject[option]} ` +


this debug statement isn't necessary anymore because the developer will see an informative error about it.

kumar303 · 2017-02-06T21:33:30Z

src/config.js

-    // this happening
+    // of this happening
+    if (camelCase(option) !== option) {
+      throw new UsageError(`Please use camel case to specify ${option} ` +


since you have the function already you can just tell them what to do (which will help them fix it faster):

throw new UsageError( `The config option "${option}" must be specified in camel case: "${camelCase(option)}"` );

kumar303 · 2017-02-06T21:38:21Z

src/config.js

        'because this is an unknown option');
-      continue;
+      throw new UsageError(`The option ${option} is invalid.` +
+        ' Please fix your config and try again.');


Rather than say "please fix [thing]," I think it's more useful to state exactly what the error was as a matter of fact. I would suggest something more like:

throw new UsageError( `The config file at ${configFilename} specified an unknown option: "${option}"` );

coveralls · 2017-02-06T22:12:57Z

Coverage remained the same at 100.0% when pulling 8b3d2c5 on shubheksha:fix/772 into 07e1f3b on mozilla:master.

kumar303

Looking good, just some minor change requests

kumar303 · 2017-02-06T23:30:16Z

src/config.js

  argv: Object,
  configObject: Object,
  defaultValues: Object,
+  configFileName?: string,


This should be a required argument. Why did you want it optional? For the tests?

Yes, if I don't make this optional, I'll have to add a file path to all the tests.

kumar303 · 2017-02-06T23:34:21Z

tests/unit/test.config.js

+        'source-dir': 'fake/value/',
+      };
+      assert.throws(() => {
+        applyConfigToArgv({argv, configObject, defaultValues});


you could make a shorter helper function just for testing to make it easier to omit defaults. Something like this:

const applyConf = (params) => applyConfigToArgv({ configFilename: 'some/path/to/config.js', ...params, });

kumar303 · 2017-02-06T23:36:51Z

tests/unit/test.config.js

+      });
+      const configFileName = 'fake/path/to/config';
+      const configObject = {
+        artifacts: 'fake/value/',


Since the check for invalid options relies on the original hyphenated key, I think it would be good to make this artifactsDir to cover that part of the logic.

coveralls · 2017-02-07T12:35:45Z

Coverage remained the same at 100.0% when pulling 21c6b6a on shubheksha:fix/772 into 07e1f3b on mozilla:master.

shubheksha · 2017-02-07T16:35:14Z

@kumar303, could you review this again? Thanks!

kumar303

This looks great, thanks. I can resolve the package.json conflict before merging.

coveralls · 2017-02-08T16:23:47Z

Coverage remained the same at 100.0% when pulling 6c650f0 on shubheksha:fix/772 into f664620 on mozilla:master.

throw an error for invalid keys in config

71fd8f6

muffinresearch added the state: pull request ready label Feb 6, 2017

shubheksha commented Feb 6, 2017

View reviewed changes

kumar303 suggested changes Feb 6, 2017

View reviewed changes

kumar303 self-assigned this Feb 6, 2017

shubheksha added 3 commits February 7, 2017 02:19

add check for camel cased options

f84d574

fix tests unnecessary errors

d475a0e

add tests for invalid option

ca9dd2e

kumar303 suggested changes Feb 6, 2017

View reviewed changes

fix nits

8b3d2c5

kumar303 suggested changes Feb 6, 2017

View reviewed changes

fix nits in tests

21c6b6a

kumar303 approved these changes Feb 8, 2017

View reviewed changes

Merge branch 'master' into fix/772

6c650f0

kumar303 merged commit bbfb253 into mozilla:master Feb 8, 2017

kumar303 removed the state: pull request ready label Feb 8, 2017

fix: Protect config files from declaring invalid keys #792

fix: Protect config files from declaring invalid keys #792

Uh oh!

Conversation

shubheksha commented Feb 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumar303 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumar303 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumar303 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shubheksha commented Feb 7, 2017

Uh oh!

kumar303 left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Feb 6, 2017 •

edited

Loading

coveralls commented Feb 6, 2017 •

edited

Loading

coveralls commented Feb 6, 2017 •

edited

Loading

coveralls commented Feb 7, 2017 •

edited

Loading

coveralls commented Feb 8, 2017 •

edited

Loading