Merge pull request #43 from muffinresearch/dupe-zip-entry

Handle duplicate files in xpi (fixes #38, fixes #37)

src/cli.js

@@ -26,6 +26,11 @@ export default argv
     default: 'text',
     choices: ['json', 'text'],
   })
+  .option('pretty', {
+    describe: 'Prettify JSON output',
+    type: 'boolean',
+    default: false,
+  })
   .option('stack', {
     describe: 'Show stacktraces when errors are thrown',
     type: 'boolean',

src/collector.js

@@ -1,4 +1,5 @@
 import { default as Message } from 'message';
+import * as constants from 'const';
 
 // "I have a display case ready and waiting for our newest acquisitions!"
 // --Taneleer Tivan
@@ -7,15 +8,17 @@ import { default as Message } from 'message';
 export default class Collector {
 
   constructor() {
-    this.errors = [];
-    this.notices = [];
-    this.warnings = [];
+    for (let type of constants.MESSAGE_TYPES) {
+      this[`${type}s`] = [];
+    }
   }
 
   get length() {
-    return this.errors.length +
-      this.notices.length +
-      this.warnings.length;
+    var len = 0;
+    for (let type of constants.MESSAGE_TYPES) {
+      len += this[`${type}s`].length;
+    }
+    return len;
   }
 
   _addMessage(type, opts, _Message=Message) {
@@ -30,14 +33,15 @@ export default class Collector {
   }
 
   addError(opts) {
-    this._addMessage('error', opts);
+    this._addMessage(constants.VALIDATION_ERROR, opts);
   }
 
   addNotice(opts) {
-    this._addMessage('notice', opts);
+    this._addMessage(constants.VALIDATION_NOTICE, opts);
   }
 
   addWarning(opts) {
-    this._addMessage('warning', opts);
+    this._addMessage(constants.VALIDATION_WARNING, opts);
   }
+
 }

src/const.js

@@ -1,4 +1,12 @@
 export const DEFLATE_COMPRESSION = 8;
 export const NO_COMPRESSION = 0;
-export const MESSAGE_TYPES = ['error', 'notice', 'warning'];
-export const SIGNING_SEVERITIES = ['trivial', 'low', 'medium', 'high'];
+
+export const VALIDATION_ERROR = 'error';
+export const VALIDATION_NOTICE = 'notice';
+export const VALIDATION_WARNING = 'warning';
+
+export const MESSAGE_TYPES = [
+  VALIDATION_ERROR,
+  VALIDATION_NOTICE,
+  VALIDATION_WARNING,
+];

src/exceptions.js

@@ -0,0 +1,15 @@
+export class ExtensibleError extends Error {
+  constructor(message) {
+    super();
+    this.message = message;
+    this.name = this.constructor.name;
+    if (Error.hasOwnProperty('captureStackTrace')) {
+      Error.captureStackTrace(this, this.constructor);
+    } else {
+      this.stack = (new Error()).stack;
+    }
+  }
+}
+
+export class DuplicateZipEntryError extends ExtensibleError {}
+

src/message.js

@@ -1,30 +1,41 @@
-import { MESSAGE_TYPES, SIGNING_SEVERITIES } from 'const';
+import { MESSAGE_TYPES } from 'const';
+import { singleLineString } from 'utils';
 
 
-// These are the optional fields we expect to pull out of
-// the opts object passed to the Message constructor.
-export var fields = [
-  'id',
+// These are the props we expect to pull out of
+// the data object passed to the Message constructor.
+export var props = [
+  'code',
   'message',
   'description',
+  'column',
   'file',
   'line',
-  'column',
-  'for_appversions',
-  'compatibility_type',
-  'signing_help',
-  'signing_severity',
 ];
 
+export var requiredProps = [
+  'code',
+  'message',
+  'description',
+];
 
 export default class Message {
 
-  constructor(type, opts={}) {
+  constructor(type, data={}) {
     this.type = type;
-    for (let field of fields) {
-      this[field] = opts[field];
+    for (let prop of props) {
+      this[prop] = data[prop];
+    }
+    var missingProps = [];
+    for (let prop of requiredProps) {
+      if (typeof this[prop] === 'undefined') {
+        missingProps.push(prop);
+      }
+    }
+    if (missingProps.length) {
+      throw new Error(singleLineString`Message data object is missing the
+        following props: ${missingProps.join(', ')}`);
     }
-    this.editorsOnly = opts.editorsOnly || false;
   }
 
   get type() {
@@ -33,24 +44,10 @@ export default class Message {
 
   set type(type) {
     if (MESSAGE_TYPES.indexOf(type) === -1) {
-      throw new Error(
-        `Message type "${type}" is not one of ${MESSAGE_TYPES.join(', ')}`);
+      throw new Error(singleLineString`Message type "${type}"
+        is not one of ${MESSAGE_TYPES.join(', ')}`);
     }
     this._type = type;
   }
 
-  get signing_severity() {
-    return this._signing_severity;
-  }
-
-  set signing_severity(severity) {
-    if (typeof severity !== 'undefined') {
-      if (SIGNING_SEVERITIES.indexOf(severity) === -1) {
-        throw new Error(
-          `Severity "${severity}" is not one of ` +
-          `${SIGNING_SEVERITIES.join(', ')}`);
-      }
-    }
-    this._signing_severity = severity;
-  }
 }

src/messages/index.js

@@ -0,0 +1,4 @@
+// Here we can re-export all our message so one can just do
+// import * as messages from 'messages';
+
+export * from './layout';

src/messages/layout.js

@@ -0,0 +1,10 @@
+import { gettext as _, singleLineString } from 'utils';
+
+
+export const DUPLICATE_XPI_ENTRY = {
+  code: 'DUPLICATE_XPI_ENTRY',
+  message: _('Package contains duplicate entries'),
+  description: _(singleLineString`The package contains multiple entries
+    with the same name. This practice has been banned. Try unzipping
+    and re-zipping your add-on package and try again.`),
+};

src/utils.js

@@ -27,3 +27,12 @@ export function singleLineString(strings, ...vars) {
     return line.replace(/^\s+/gm, '');
   }).join(' ').trim();
 }
+
+/*
+ * Gettext utils. No-op until we have proper
+ * a proper l10n solution.
+ *
+ */
+export function gettext(str) {
+  return str;
+}

src/validator.js

@@ -1,9 +1,15 @@
 import * as fs from 'fs';
-import Xpi from 'xpi';
 
 import promisify from 'es6-promisify';
 import chalk from 'chalk';
 
+import * as messages from 'messages';
+import * as exceptions from 'exceptions';
+import * as constants from 'const';
+
+import Xpi from 'xpi';
+import Collector from 'collector';
+
 export var lstat = promisify(fs.lstat);
 
 
@@ -15,6 +21,7 @@ export default class Validator {
     this.xpi;
     this.chalk = new chalk.constructor(
       {enabled: !this.config.boring});
+    this.collector = new Collector();
   }
 
   handleError(err) {
@@ -25,6 +32,14 @@ export default class Validator {
     }
   }
 
+  print() {
+    if (this.config.output === 'json') {
+      console.log(this.toJSON(this.config.pretty));
+    } else {
+      console.log('Text output not yet implemented!');
+    }
+  }
+
   checkFileExists(filepath, _lstat=lstat) {
     var invalidMessage = new Error(
       `Path "${filepath}" is not a file or does not exist.`);
@@ -47,10 +62,32 @@ export default class Validator {
     });
   }
 
-  scan() {
-    this.checkFileExists(this.packagePath)
+  get output() {
+    var output = {
+      count: this.collector.length,
+      summary: {},
+    };
+    for (let type of constants.MESSAGE_TYPES) {
+      var messageType = `${type}s`;
+      output[messageType] = this.collector[messageType];
+      output.summary[messageType] = this.collector[messageType].length;
+    }
+    return output;
+  }
+
+  toJSON(pretty=false) {
+    var args = [this.output];
+    if (pretty === true) {
+      args.push(null);
+      args.push(4);
+    }
+    return JSON.stringify.apply(null, args);
+  }
+
+  scan(_Xpi=Xpi) {
+    return this.checkFileExists(this.packagePath)
       .then(() => {
-        this.xpi = new Xpi(this.packagePath);
+        this.xpi = new _Xpi(this.packagePath);
         return this.xpi.getMetaData();
       })
       .then((metadata) => {
@@ -58,7 +95,12 @@ export default class Validator {
         console.log(metadata);
       })
       .catch((err) => {
-        return this.handleError(err);
+        if (err instanceof exceptions.DuplicateZipEntryError) {
+          this.collector.addError(messages.DUPLICATE_XPI_ENTRY);
+          this.print();
+        } else {
+          return this.handleError(err);
+        }
       });
   }
 }

src/xpi.js

@@ -1,5 +1,8 @@
 import yauzl from 'yauzl';
 
+import { DuplicateZipEntryError } from 'exceptions';
+
+
 /*
  * Simple Promise wrapper for the Yauzl unzipping lib to unpack add-on .xpis.
  * Note: We're using the autoclose feature of yauzl as a result every operation
@@ -15,6 +18,7 @@ export default class Xpi {
     this.filename = filename;
     this.zipLib = zipLib;
     this.metadata = {};
+    this.entries = [];
   }
 
   handleEntry(entry, zipfile, reject) {
@@ -26,6 +30,11 @@ export default class Xpi {
       if (err) {
         return reject(err);
       }
+      if (this.entries.indexOf(entry.fileName) > -1) {
+        return reject(new DuplicateZipEntryError(
+          `Entry "${entry.fileName}" has already been seen`));
+      }
+      this.entries.push(entry.fileName);
       this.metadata[entry.fileName] = entry;
     });
   }
@@ -55,8 +64,13 @@ export default class Xpi {
             this.handleEntry(entry, zipfile, reject);
           });
           // When the last entry has been processed
-          // cache the metadata and resolve the promise.
-          zipfile.on('end', () => resolve(this.metadata));
+          // and the fd is closed resolve the promise.
+          // Note: we cannot use 'end' here as 'end' is fired
+          // after the last entry event is emitted and streams
+          // may still be being read with openReadStream.
+          zipfile.on('close', () => {
+            resolve(this.metadata);
+          });
           if (_onEventsSubscribed) {
             // Run optional callback when we know the event handlers
             // have been inited. Useful for testing.

tests/helpers.js

@@ -0,0 +1,5 @@
+export const fakeMessageData = {
+  code: 'WHATEVER_CODE',
+  description: 'description',
+  message: 'message',
+};

tests/test.cli.js

@@ -29,6 +29,11 @@ describe('Basic CLI tests', function() {
     assert.equal(args.stack, false);
   });
 
+  it('should default pretty to false', () => {
+    var args = cli.parse(['foo/bar.xpi']);
+    assert.equal(args.pretty, false);
+  });
+
   it('should default selfhosted to false', () => {
     var args = cli.parse(['foo/bar.xpi']);
     assert.equal(args.selfhosted, false);

tests/test.collector.js

@@ -1,5 +1,5 @@
 import { default as Collector } from 'collector';
-
+import { fakeMessageData } from './helpers';
 
 describe('Collector', function() {
 
@@ -21,7 +21,7 @@ describe('Collector', function() {
     assert.throws(() => {
       var FakeMessage = sinon.stub();
       var collection = new Collector();
-      collection._addMessage('whatevar', {}, FakeMessage);
+      collection._addMessage('whatevar', fakeMessageData, FakeMessage);
     }, Error, /Message type "whatevar" not currently collected/);
   });
 
@@ -33,17 +33,17 @@ describe('Collector', function() {
   it('length should reflect number of messages', () => {
     var collection = new Collector();
     assert.equal(collection.length, 0);
-    collection.addError({});
+    collection.addError(fakeMessageData);
     assert.equal(collection.length, 1);
-    collection.addNotice({});
+    collection.addNotice(fakeMessageData);
     assert.equal(collection.length, 2);
-    collection.addWarning({});
+    collection.addWarning(fakeMessageData);
     assert.equal(collection.length, 3);
   });
 
   it('should create an error message', () => {
     var collection = new Collector();
-    collection.addError({});
+    collection.addError(fakeMessageData);
     assert.equal(collection.errors[0].type, 'error');
     assert.equal(collection.notices.length, 0);
     assert.equal(collection.warnings.length, 0);
@@ -51,7 +51,7 @@ describe('Collector', function() {
 
   it('should create a notice message', () => {
     var collection = new Collector();
-    collection.addNotice({});
+    collection.addNotice(fakeMessageData);
     assert.equal(collection.notices[0].type, 'notice');
     assert.equal(collection.errors.length, 0);
     assert.equal(collection.warnings.length, 0);
@@ -59,7 +59,7 @@ describe('Collector', function() {
 
   it('should create a warning message', () => {
     var collection = new Collector();
-    collection.addWarning({});
+    collection.addWarning(fakeMessageData);
     assert.equal(collection.warnings[0].type, 'warning');
     assert.equal(collection.errors.length, 0);
     assert.equal(collection.notices.length, 0);

tests/test.exceptions.js

@@ -0,0 +1,22 @@
+import * as exceptions from 'exceptions';
+
+
+describe('DuplicateZipEntry()', function() {
+
+  it('should have correct message', () => {
+    assert.throws(() => {
+      throw new exceptions.DuplicateZipEntryError('whatever');
+    }, exceptions.DuplicateZipEntry, /whatever/);
+  });
+
+  it('should have correct message', () => {
+    try {
+      throw new exceptions.DuplicateZipEntryError('whatever');
+    } catch (e) {
+      assert.instanceOf(e, exceptions.DuplicateZipEntryError);
+      assert.equal(e.message, 'whatever');
+      assert.equal(e.name, 'DuplicateZipEntryError');
+    }
+  });
+
+});

tests/test.message.js

@@ -1,4 +1,5 @@
-import { default as Message, fields } from 'message';
+import { default as Message, props } from 'message';
+import { fakeMessageData } from './helpers';
 
 /*eslint no-unused-vars:0*/
 
@@ -16,33 +17,29 @@ describe('Message', function() {
     }, Error, /Message type "awooga" is not/);
   });
 
-  it('should throw on incorrect signing_severity', () => {
-    assert.throws(() => {
-      var MyMessage = new Message('error',
-        {signing_severity: 'whatever'});
-    }, Error, /Severity "whatever" is not/);
-  });
-
-  it('should define all expected fields', () => {
-    var fakeOpts = {};
-    for (let field of fields) {
-      fakeOpts[field] = field;
+  it('should define all expected props', () => {
+    var fakeData = {};
+    for (let prop of props) {
+      fakeData[prop] = prop;
     }
-    fakeOpts.signing_severity = 'medium';
-    var MyMessage = new Message('error', fakeOpts);
-    for (let field of fields) {
-      if (field === 'signing_severity') {
-        assert.equal(MyMessage.signing_severity, 'medium');
-      } else {
-        assert.equal(MyMessage[field], field);
-      }
+    var MyMessage = new Message('error', fakeData);
+    for (let prop of props) {
+      assert.equal(MyMessage[prop], prop);
     }
   });
 
   it ("shouldn't define random opts", () => {
-    var MyMessage = new Message('error', {random: 'foo'});
+    var MyMessage = new Message('error',
+      Object.assign({}, fakeMessageData, {random: 'foo'}));
     assert.notEqual(MyMessage.random, 'foo');
   });
 
+  it('should throw on missing required prop', () => {
+    assert.throws(() => {
+      var MyMessage = new Message('error',
+        Object.assign({}, {description: 'foo'}));
+    }, Error, /Message data object is missing the following props/);
+  });
+
 });
 

tests/test.validator.js

@@ -1,5 +1,8 @@
 import Validator from 'validator';
 
+import * as messages from 'messages';
+import { fakeMessageData } from './helpers';
+import { DuplicateZipEntryError } from 'exceptions';
 
 
 describe('Validator', function() {
@@ -61,4 +64,48 @@ describe('Validator', function() {
       });
   });
 
+  it('should provide output via output prop', () => {
+    var addonValidator = new Validator({_: ['bar']});
+    addonValidator.collector.addError(fakeMessageData);
+    var output = addonValidator.output;
+    assert.equal(output.count, 1);
+    assert.equal(output.summary.errors, 1);
+    assert.equal(output.summary.notices, 0);
+    assert.equal(output.summary.warnings, 0);
+  });
+
+  it('should provide JSON via toJSON()', () => {
+    var addonValidator = new Validator({_: ['bar']});
+    addonValidator.collector.addError(fakeMessageData);
+    var json = addonValidator.toJSON();
+    var parsedJSON = JSON.parse(json);
+    assert.equal(parsedJSON.count, 1);
+    assert.equal(parsedJSON.summary.errors, 1);
+    assert.equal(parsedJSON.summary.notices, 0);
+    assert.equal(parsedJSON.summary.warnings, 0);
+  });
+
+  it('should call addError when Xpi rejects with dupe entry', () => {
+    var addonValidator = new Validator({_: ['bar']});
+    addonValidator.checkFileExists = () => Promise.resolve();
+    addonValidator.collector.addError = sinon.stub();
+    addonValidator.print = sinon.stub();
+    class FakeXpi {
+      getMetaData() {
+        return Promise.reject(
+          new DuplicateZipEntryError('Darnit the zip has dupes!'));
+      }
+    }
+    return addonValidator.scan(FakeXpi)
+      .then(() => {
+        assert.fail(null, null, 'Unexpected success');
+      })
+      .catch(() => {
+        assert.ok(
+          addonValidator.collector.addError.calledWith(
+            messages.DUPLICATE_XPI_ENTRY));
+        assert.ok(addonValidator.print.called);
+      });
+  });
+
 });

tests/test.xpi.js

@@ -2,6 +2,7 @@ import { Readable } from 'stream';
 
 import Xpi from 'xpi';
 import { DEFLATE_COMPRESSION, NO_COMPRESSION } from 'const';
+import { DuplicateZipEntryError } from 'exceptions';
 
 const defaultData = {
   compressionMethod: DEFLATE_COMPRESSION,
@@ -19,6 +20,12 @@ const installRdfEntry = Object.assign({}, defaultData, {
   fileName: 'install.rdf',
 });
 
+const dupeInstallRdfEntry = Object.assign({}, defaultData, {
+  compressedSize: 416,
+  uncompressedSize: 851,
+  fileName: 'install.rdf',
+});
+
 const chromeContentDir = {
   compressionMethod: NO_COMPRESSION,
   compressedSize: 0,
@@ -73,8 +80,8 @@ describe('Xpi.getMetaData()', function() {
     this.entryStub = onStub
       .withArgs('entry');
 
-    this.endStub = onStub
-      .withArgs('end');
+    this.closeStub = onStub
+      .withArgs('close');
 
     this.openReadStreamStub = sinon.stub();
 
@@ -120,10 +127,10 @@ describe('Xpi.getMetaData()', function() {
     this.openStub.yieldsAsync(null, this.fakeZipFile);
 
     // Call the openReadStream callback
-    this.openReadStreamStub.yields(null);
+    this.openReadStreamStub.yieldsAsync(null);
 
-    // Call the end event callback
-    this.endStub.yields();
+    // Call the close event callback
+    this.closeStub.yieldsAsync();
 
     // If we could use yields multiple times here we would
     // but sinon doesn't support it when the stub is only
@@ -143,13 +150,32 @@ describe('Xpi.getMetaData()', function() {
       });
   });
 
+  it('should reject on duplicate entries', () => {
+    var myXpi = new Xpi('foo/bar', this.fakeZipLib);
+    this.openStub.yieldsAsync(null, this.fakeZipFile);
+    this.openReadStreamStub.yieldsAsync(null);
+
+    // We'll add the first directly.
+    myXpi.handleEntry(installRdfEntry, this.fakeZipFile);
+
+    // And this one via a yield so we can have it rejected.
+    this.entryStub.yieldsAsync(dupeInstallRdfEntry);
+
+    return myXpi.getMetaData()
+      .then(() => {
+        assert.fail(null, null, 'Unexpected success');
+      })
+      .catch((err) => {
+        assert.instanceOf(err, DuplicateZipEntryError);
+      });
+  });
 
   it('should reject on errors in zipfile.openReadStream()', () => {
     var myXpi = new Xpi('foo/bar', this.fakeZipLib);
 
     this.openStub.yieldsAsync(null, this.fakeZipFile);
-    this.openReadStreamStub.yields(new Error('openReadStream test'));
-    this.entryStub.yields(chromeManifestEntry);
+    this.openReadStreamStub.yieldsAsync(new Error('openReadStream test'));
+    this.entryStub.yieldsAsync(chromeManifestEntry);
 
     return myXpi.getMetaData()
       .then(() => {
@@ -214,7 +240,7 @@ describe('Xpi.getFileAsStream()', function() {
     };
 
     this.openStub.yieldsAsync(null, this.fakeZipFile);
-    this.openReadStreamStub.yields(
+    this.openReadStreamStub.yieldsAsync(
       new Error('getFileAsStream openReadStream test'));
 
     return myXpi.getFileAsStream('install.rdf')