From ea8846154f31f27692a80377dab8cd43b28fbdfe Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Tue, 20 Apr 2021 00:53:22 -0400
Subject: [PATCH] favicon: Redo favicon customization

---
 CHANGELOG.md                                  |   9 +++++
 settings.json.docker                          |   8 ++--
 settings.json.template                        |   8 ++--
 src/node/hooks/express/specialpages.js        |   1 +
 src/node/utils/Settings.js                    |   9 +++--
 src/templates/index.html                      |   2 +-
 src/templates/pad.html                        |   2 +-
 src/templates/timeslider.html                 |   2 +-
 .../backend/specs/favicon-test-custom.png     | Bin 0 -> 635 bytes
 src/tests/backend/specs/favicon.js            |  36 ++++++++++++++++++
 10 files changed, 64 insertions(+), 13 deletions(-)
 create mode 100644 src/tests/backend/specs/favicon-test-custom.png

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d5441ac48..d28dd816d 100644
--- a/CHANGELOG.md
+++ b/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
diff --git a/settings.json.docker b/settings.json.docker
index 228c70d2a..030ab88f1 100644
--- a/settings.json.docker
+++ b/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.
diff --git a/settings.json.template b/settings.json.template
index 8a676b56d..90a89970c 100644
--- a/settings.json.template
+++ b/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.
diff --git a/src/node/hooks/express/specialpages.js b/src/node/hooks/express/specialpages.js
index 725139c09..66ee0221e 100644
--- a/src/node/hooks/express/specialpages.js
+++ b/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'),
       ];
diff --git a/src/node/utils/Settings.js b/src/node/utils/Settings.js
index 60f4e6442..b31cf16b4 100644
--- a/src/node/utils/Settings.js
+++ b/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.
diff --git a/src/templates/index.html b/src/templates/index.html
index 8f20c08da..4d08312f9 100644
--- a/src/templates/index.html
+++ b/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>
diff --git a/src/templates/pad.html b/src/templates/pad.html
index 7bf2346f9..d7c3c0823 100644
--- a/src/templates/pad.html
+++ b/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">
diff --git a/src/templates/timeslider.html b/src/templates/timeslider.html
index b3fd3d006..fe43668c8 100644
--- a/src/templates/timeslider.html
+++ b/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%>">
diff --git a/src/tests/backend/specs/favicon-test-custom.png b/src/tests/backend/specs/favicon-test-custom.png
new file mode 100644
index 0000000000000000000000000000000000000000..9c6532c9610ab912675719e1a896618b7ae6fc65
GIT binary patch
literal 635
zcmV->0)+jEP)<h;3K|Lk000e1NJLTq000mG000mO1^@s6AM^iV0006)Nkl<ZIE|H)
zQAm?f7>0M5-E^;aMpPHM>C%wOREX)$uV_IK5nYHG-E@%}Ni>};3$kL7I)|7QY7-+A
z6wTS6mCf^uVSzYMD-bQ|V%16)Y)=={%}t{^@ZKE0_u>2Ab3UCe?l5N3BSiz6GL0qz
zVJXu{(ZGaBA739ej%FsHi6%59h^Fud1(CwRXl6pJ`F-{lq*<Nn%;Mg~J@i~RviveX
zVhl-zFKRJM&yEn13P?qbfgpv$H*BKj)OKDwGikTzv6bu~I3`$dAH{a{Jmt3ARMy?$
zvh@P9{l`EPj#abSz<9LnkQDew@_2Lq0L!BVR8?2e_H=@wneV7`emrlzR94sEdr=%j
znhm<uz#{GT*YD+YUJ{cJ58~-MNuAS$d)ALS=jW4e8Q+f;nq2*~-95|Kk-|n@IPECl
z#+jXznv&?S=oxrWg55P3WA=RX)6(amtir;h<}$AiO{}a3Jh_%esVNDqbw5+R$GLRV
z$>KV*d)AMwxs#$3CCm>N1$0OeSPuwE!P3CtNc#5rbF_3kX6W5Ky^{;vYVYHUt(>ew
z`}p=eKd@nhc>VGe(9>E<O>G4hYbCDcGJd`)BsV7=pPKL85aPcoHz%FXgQms}&$y<U
z?rgehQ|YQrWu`lOQ^2q@#~A(#nsCGyxU2@;_6*$i3|v;jrhw$+<ZaPCkc#>rBd94x
z<6;C5g$T!Amq`mR({6p*hZsNvG@*poSqd+j5<pV|NZ}3NnGI`oyHe7|6B84+{szLY
VfscrpV5I;6002ovPDHLkV1ir7F9ZMp

literal 0
HcmV?d00001

diff --git a/src/tests/backend/specs/favicon.js b/src/tests/backend/specs/favicon.js
index 76140b72f..a5e3095de 100644
--- a/src/tests/backend/specs/favicon.js
+++ b/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);