favicon: Redo favicon customization

CHANGELOG.md

@@ -1,3 +1,12 @@
+# Next release
+
+### Compatibility changes
+
+* The `favicon` setting is now interpreted as a pathname to a favicon file, not
+  a URL. Please see the documentation comment in `settings.json.template`.
+* The undocumented `faviconPad` and `faviconTimeslider` settings have been
+  removed.
+
 # 1.8.13
 
 ### Notable fixes

settings.json.docker

@@ -80,10 +80,12 @@
   "title": "${TITLE:Etherpad}",
 
   /*
-   * favicon default name
-   * alternatively, set up a fully specified Url to your own favicon
+   * Pathname of the favicon you want to use. If null, the skin's favicon is
+   * used if one is provided by the skin, otherwise the default Etherpad favicon
+   * is used. If this is a relative path it is interpreted as relative to the
+   * Etherpad root directory.
    */
-  "favicon": "${FAVICON:favicon.ico}",
+  "favicon": "${FAVICON}",
 
   /*
    * Skin name.

settings.json.template

@@ -71,10 +71,12 @@
   "title": "Etherpad",
 
   /*
-   * favicon default name
-   * alternatively, set up a fully specified Url to your own favicon
+   * Pathname of the favicon you want to use. If null, the skin's favicon is
+   * used if one is provided by the skin, otherwise the default Etherpad favicon
+   * is used. If this is a relative path it is interpreted as relative to the
+   * Etherpad root directory.
    */
-  "favicon": "favicon.ico",
+  "favicon": null,
 
   /*
    * Skin name.

src/node/hooks/express/specialpages.js

@@ -80,6 +80,7 @@ exports.expressCreateServer = (hookName, args, cb) => {
   args.app.get('/favicon.ico', (req, res, next) => {
     (async () => {
       const fns = [
+        ...(settings.favicon ? [path.resolve(settings.root, settings.favicon)] : []),
         path.join(settings.root, 'src', 'static', 'skins', settings.skinName, 'favicon.ico'),
         path.join(settings.root, 'src', 'static', 'favicon.ico'),
       ];

src/node/utils/Settings.js

@@ -50,11 +50,12 @@ console.log('All relative paths will be interpreted relative to the identified '
 exports.title = 'Etherpad';
 
 /**
- * The app favicon fully specified url, visible e.g. in the browser window
+ * Pathname of the favicon you want to use. If null, the skin's favicon is
+ * used if one is provided by the skin, otherwise the default Etherpad favicon
+ * is used. If this is a relative path it is interpreted as relative to the
+ * Etherpad root directory.
  */
-exports.favicon = 'favicon.ico';
-exports.faviconPad = `../${exports.favicon}`;
-exports.faviconTimeslider = `../../${exports.favicon}`;
+exports.favicon = null;
 
 /*
  * Skin name.

src/templates/index.html

@@ -8,7 +8,7 @@
         <meta charset="utf-8">
         <meta name="referrer" content="no-referrer">
         <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=0">
-        <link rel="shortcut icon" href="<%=settings.favicon%>">
+        <link rel="shortcut icon" href="favicon.ico">
 
         <link rel="localizations" type="application/l10n+json" href="locales.json">
         <script type="text/javascript" src="static/js/vendors/html10n.js?v=<%=settings.randomVersionString%>"></script>

src/templates/pad.html

@@ -39,7 +39,7 @@
   <meta name="robots" content="noindex, nofollow">
   <meta name="referrer" content="no-referrer">
   <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=0">
-  <link rel="shortcut icon" href="<%=settings.faviconPad%>">
+  <link rel="shortcut icon" href="../favicon.ico">
 
   <% e.begin_block("styles"); %>
   <link href="../static/css/pad.css?v=<%=settings.randomVersionString%>" rel="stylesheet">

src/templates/timeslider.html

@@ -33,7 +33,7 @@
   <meta name="robots" content="noindex, nofollow">
   <meta name="referrer" content="no-referrer">
   <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=0">
-  <link rel="shortcut icon" href="<%=settings.faviconTimeslider%>">
+  <link rel="shortcut icon" href="../../favicon.ico">
   <% e.begin_block("timesliderStyles"); %>
   <link rel="stylesheet" href="../../static/css/pad.css?v=<%=settings.randomVersionString%>">
   <link rel="stylesheet" href="../../static/css/iframe_editor.css?v=<%=settings.randomVersionString%>">

src/tests/backend/specs/favicon.js

@@ -12,11 +12,13 @@ describe(__filename, function () {
   let agent;
   let backupSettings;
   let skinDir;
+  let wantCustomIcon;
   let wantDefaultIcon;
   let wantSkinIcon;
 
   before(async function () {
     agent = await common.init();
+    wantCustomIcon = await fsp.readFile(path.join(__dirname, 'favicon-test-custom.png'));
     wantDefaultIcon = await fsp.readFile(path.join(settings.root, 'src', 'static', 'favicon.ico'));
     wantSkinIcon = await fsp.readFile(path.join(__dirname, 'favicon-test-skin.png'));
   });
@@ -28,6 +30,7 @@ describe(__filename, function () {
   });
 
   afterEach(async function () {
+    delete settings.favicon;
     delete settings.skinName;
     Object.assign(settings, backupSettings);
     try {
@@ -38,8 +41,40 @@ describe(__filename, function () {
     } catch (err) { /* intentionally ignored */ }
   });
 
+  it('uses custom favicon if set (relative pathname)', async function () {
+    settings.favicon =
+        path.relative(settings.root, path.join(__dirname, 'favicon-test-custom.png'));
+    assert(!path.isAbsolute(settings.favicon));
+    const {body: gotIcon} = await agent.get('/favicon.ico')
+        .accept('png').buffer(true).parse(superagent.parse.image)
+        .expect(200);
+    assert(gotIcon.equals(wantCustomIcon));
+  });
+
+  it('uses custom favicon if set (absolute pathname)', async function () {
+    settings.favicon = path.join(__dirname, 'favicon-test-custom.png');
+    assert(path.isAbsolute(settings.favicon));
+    const {body: gotIcon} = await agent.get('/favicon.ico')
+        .accept('png').buffer(true).parse(superagent.parse.image)
+        .expect(200);
+    assert(gotIcon.equals(wantCustomIcon));
+  });
+
+  it('falls back if custom favicon is missing', async function () {
+    // The previous default for settings.favicon was 'favicon.ico', so many users will continue to
+    // have that in their settings.json for a long time. There is unlikely to be a favicon at
+    // path.resolve(settings.root, 'favicon.ico'), so this test ensures that 'favicon.ico' won't be
+    // a problem for those users.
+    settings.favicon = 'favicon.ico';
+    const {body: gotIcon} = await agent.get('/favicon.ico')
+        .accept('png').buffer(true).parse(superagent.parse.image)
+        .expect(200);
+    assert(gotIcon.equals(wantDefaultIcon));
+  });
+
   it('uses skin favicon if present', async function () {
     await fsp.writeFile(path.join(skinDir, 'favicon.ico'), wantSkinIcon);
+    settings.favicon = null;
     const {body: gotIcon} = await agent.get('/favicon.ico')
         .accept('png').buffer(true).parse(superagent.parse.image)
         .expect(200);
@@ -47,6 +82,7 @@ describe(__filename, function () {
   });
 
   it('falls back to default favicon', async function () {
+    settings.favicon = null;
     const {body: gotIcon} = await agent.get('/favicon.ico')
         .accept('png').buffer(true).parse(superagent.parse.image)
         .expect(200);