mozilla / ping-centre

INACTIVE - http://mzl.la/ghe-archive
Mozilla Public License 2.0
8 stars 10 forks source link

Better validation before importing user-specified schemas? #45

Closed pdehaan closed 4 months ago

pdehaan commented 7 years ago

Ref: https://github.com/mozilla/ping-centre/pull/36#discussion_r99694219

Currently in the PingCentre() constructor we "blindly" try require()ing a user-specified topic schema from disk. We may want to do a bit more validation before loading files, in case user's try and pass funny path strings like "../../../../etc/host" or something (not that it'd work, but still).

jaredhirsch commented 7 years ago

One solution might be to have an index.js inside the schemas folder, then explicitly load all supported schemas there, exported as an object:

// schemas/index.js
'use strict';

module.exports = {
  'testpilot': require('./testpilot'),
  'common': require('./commonSchema')
};

Then, instead of this:

// inside PingCentre constructor
const schema = require(`./schemas/${topic}`);
this._schema = schema || commonSchema;

could do something like:

const schemas = require('./schemas')
this._schema = schemas[topic] || schemas['common']
pdehaan commented 7 years ago

:+1: Although not sure if we want to do a silent "common" fallback versus a more aggressive error of "Unknown schema ${topic}".


FWIW, eslint-plugin-security flags the variable require as a potential threat:

➜  ping-centre git:(webpack) $ npm run test:lint

> ping-centre@1.0.0 test:lint /Users/pdehaan/dev/github/mozilla/ping-centre
> eslint . --ext .js

/Users/pdehaan/dev/github/mozilla/ping-centre/src/ping-centre.js
  18:20  warning  Found non-literal argument in require
    18:      const schema = require(`./schemas/${topic}`);  security/detect-non-literal-require