favicon: Refactor handler and add tests

src/node/hooks/express/specialpages.js

@@ -2,9 +2,12 @@
 
 const path = require('path');
 const eejs = require('../../eejs');
+const fs = require('fs');
+const fsp = fs.promises;
 const toolbar = require('../../utils/toolbar');
 const hooks = require('../../../static/js/pluginfw/hooks');
 const settings = require('../../utils/Settings');
+const util = require('util');
 const webaccess = require('./webaccess');
 
 exports.expressCreateServer = (hookName, args, cb) => {
@@ -74,23 +77,24 @@ exports.expressCreateServer = (hookName, args, cb) => {
     }));
   });
 
-  args.app.get('/favicon.ico', (req, res) => {
-    let filePath = path.join(
-        settings.root,
-        'src',
-        'static',
-        'skins',
-        settings.skinName,
-        'favicon.ico'
-    );
-    res.setHeader('Cache-Control', `public, max-age=${settings.maxAge}`);
-    res.sendFile(filePath, (err) => {
-      // there is no custom favicon, send the default favicon
-      if (err) {
-        filePath = path.join(settings.root, 'src', 'static', 'favicon.ico');
-        res.sendFile(filePath);
+  args.app.get('/favicon.ico', (req, res, next) => {
+    (async () => {
+      const fns = [
+        path.join(settings.root, 'src', 'static', 'skins', settings.skinName, 'favicon.ico'),
+        path.join(settings.root, 'src', 'static', 'favicon.ico'),
+      ];
+      for (const fn of fns) {
+        try {
+          await fsp.access(fn, fs.constants.R_OK);
+        } catch (err) {
+          continue;
+        }
+        res.setHeader('Cache-Control', `public, max-age=${settings.maxAge}`);
+        await util.promisify(res.sendFile.bind(res))(fn);
+        return;
       }
-    });
+      next();
+    })().catch((err) => next(err || new Error(err)));
   });
 
   return cb();

src/tests/backend/specs/favicon.js

@@ -0,0 +1,55 @@
+'use strict';
+
+const assert = require('assert').strict;
+const common = require('../common');
+const fs = require('fs');
+const fsp = fs.promises;
+const path = require('path');
+const settings = require('../../../node/utils/Settings');
+const superagent = require('superagent');
+
+describe(__filename, function () {
+  let agent;
+  let backupSettings;
+  let skinDir;
+  let wantDefaultIcon;
+  let wantSkinIcon;
+
+  before(async function () {
+    agent = await common.init();
+    wantDefaultIcon = await fsp.readFile(path.join(settings.root, 'src', 'static', 'favicon.ico'));
+    wantSkinIcon = await fsp.readFile(path.join(__dirname, 'favicon-test-skin.png'));
+  });
+
+  beforeEach(async function () {
+    backupSettings = {...settings};
+    skinDir = await fsp.mkdtemp(path.join(settings.root, 'src', 'static', 'skins', 'test-'));
+    settings.skinName = path.basename(skinDir);
+  });
+
+  afterEach(async function () {
+    delete settings.skinName;
+    Object.assign(settings, backupSettings);
+    try {
+      // TODO: The {recursive: true} option wasn't added to fsp.rmdir() until Node.js v12.10.0 so we
+      // can't rely on it until support for Node.js v10 is dropped.
+      await fsp.unlink(path.join(skinDir, 'favicon.ico'));
+      await fsp.rmdir(skinDir, {recursive: true});
+    } catch (err) { /* intentionally ignored */ }
+  });
+
+  it('uses skin favicon if present', async function () {
+    await fsp.writeFile(path.join(skinDir, 'favicon.ico'), wantSkinIcon);
+    const {body: gotIcon} = await agent.get('/favicon.ico')
+        .accept('png').buffer(true).parse(superagent.parse.image)
+        .expect(200);
+    assert(gotIcon.equals(wantSkinIcon));
+  });
+
+  it('falls back to default favicon', async function () {
+    const {body: gotIcon} = await agent.get('/favicon.ico')
+        .accept('png').buffer(true).parse(superagent.parse.image)
+        .expect(200);
+    assert(gotIcon.equals(wantDefaultIcon));
+  });
+});