fix(Edit Admin): Add validation for untrusted input in path [#62][#81]

danactive · Jan 2, 2017 · 0219037 · 0219037
1 parent 675c401
commit 0219037
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 33 deletions.
diff --git a/plugins/album/lib/json.js b/plugins/album/lib/json.js
@@ -95,35 +95,44 @@ function templatePrepare(result = {}) {
 }
 module.exports.templatePrepare = templatePrepare;
 
-function safeAlbumPath(gallery, albumStem) {
-  const restriction = name => `Valid ${name} contains Alpha-Numeric characters, is at least 1 character long but less than 25,
+function safePath(name, value) {
+  const restriction = () => `Valid ${name} contains Alpha-Numeric characters, is at least 1 character long but less than 25,
     and may contain any special characters including dash (-) or underscore (_)`;
 
-  if (validation.albumStem.validate(albumStem).error || !albumStem) {
-    return boom.notAcceptable(restriction('album id'));
+  if (!value || validation[name].validate(value).error) {
+    return boom.notAcceptable(restriction());
   }
 
-  if (validation.gallery.validate(gallery).error || !gallery) {
-    return boom.notAcceptable(restriction('gallery id'));
+  if (name === 'albumStem') {
+    return `album_${value}.xml`;
   }
 
-  const safeAlbumStem = `album_${albumStem}.xml`;
-  const safeGallery = `gallery-${gallery}`;
+  return `gallery-${value}`;
+}
+module.exports.safePath = safePath;
+
+function ensureSafePath(name, value, reject) {
+  const partialPath = safePath(name, value);
 
-  return path.join(__dirname, '../../../', safeGallery, 'xml', safeAlbumStem);
+  if (partialPath.isBoom === true) {
+    return reject(partialPath);
+  }
+
+  return partialPath;
 }
-module.exports.safeAlbumPath = safeAlbumPath;
 
 
 module.exports.getAlbum = (gallery, albumStem) => new Promise((resolve, reject) => {
   const options = { explicitArray: false, normalizeTags: true, tagNameProcessors: [name => camelCase(name)] };
   const parser = new xml2js.Parser(options);
-  const xmlPath = safeAlbumPath(gallery, albumStem);
 
-  if (typeof xmlPath === 'object') {
-    reject(xmlPath);
-    return;
-  }
+  const xmlPath = path.join(
+    __dirname,
+    '../../../',
+    ensureSafePath('gallery', gallery, reject),
+    'xml',
+    ensureSafePath('albumStem', albumStem, reject),
+  );
 
   fs.readFile(xmlPath, (readError, fileData) => {
     if (readError) {

diff --git a/plugins/album/test/json.spec.js b/plugins/album/test/json.spec.js
@@ -5,29 +5,24 @@ test('Read album XML', { skip: false }, (describe) => {
   const testCases = require('./cases');
 
   describe.test('* Check for safe paths', { skip: false }, (assert) => {
-    let result = lib.safeAlbumPath();
-    assert.ok(result.isBoom, 'Undefined fails');
+    const names = ['gallery', 'albumStem'];
 
-    result = lib.safeAlbumPath('pass');
-    assert.ok(result.isBoom, 'Undefined fails (x2)');
+    names.forEach((name) => {
+      let result = lib.safePath(name, undefined);
+      assert.ok(result.isBoom, `Undefined fails ${name}`);
 
-    result = lib.safeAlbumPath('');
-    assert.ok(result.isBoom, 'Blank fails');
+      result = lib.safePath(name, '');
+      assert.ok(result.isBoom, `Blank fails ${name}`);
 
-    result = lib.safeAlbumPath('pass', '');
-    assert.ok(result.isBoom, 'Blank fails (x2)');
+      result = lib.safePath(name, '@');
+      assert.ok(result.isBoom, `Special char fails ${name}`);
 
-    result = lib.safeAlbumPath('@');
-    assert.ok(result.isBoom, 'Special char fails');
+      result = lib.safePath(name, 'pass');
+      assert.equal(typeof result, 'string', `Pass ${name}`);
 
-    result = lib.safeAlbumPath('pass', '@');
-    assert.ok(result.isBoom, 'Special char fails (x2)');
-
-    result = lib.safeAlbumPath('pass', 'pass');
-    assert.equal(typeof result, 'string', 'Pass');
-
-    result = lib.safeAlbumPath('_PASS--123_', '-456__PASS-');
-    assert.equal(typeof result, 'string', 'Pass (x2)');
+      result = lib.safePath(name, '_PASS--123_');
+      assert.equal(typeof result, 'string', `Pass (x2) ${name}`);
+    });
 
     assert.end();
   });