🐛 Fixed theme preview not decoding values properly

- Theme preview was not showing the same behaviour as a real theme because nulls were being encoded and decoded incorrectly causing nulls/empty strings to be treasted as truthy values
- Swap from using split to using proper query param parsing so that the code is more robust
- this still creates empty strings and the string 'null' so added a small function to decode these back to real nulls
- moved to its own file ready to be split out - there needs to be a bigger picture plan for this
- added unit tests to cover the known issues + some potential breakages from converting the header string to a query param object

core/frontend/services/themes/middleware.js

@@ -7,6 +7,7 @@ const errors = require('@tryghost/errors');
 const settingsCache = require('../../../server/services/settings/cache');
 const labs = require('../../../server/services/labs');
 const activeTheme = require('./active');
+const preview = require('./preview');
 
 // ### Ensure Active Theme
 // Ensure there's a properly set & mounted active theme before attempting to serve a site request
@@ -75,41 +76,11 @@ function haxGetMembersPriceData() {
     }
 }
 
-// The preview header contains a query string with the custom preview data
-// This is deliberately slightly obscure & means we don't need to add body parsing to the frontend :D
-// If we start passing in strings like title or description we will probably need to change this
-const PREVIEW_HEADER_NAME = 'x-ghost-preview';
-
-function handlePreview(previewHeader, siteData) {
-    // Keep the string shorter with short codes for certain parameters
-    const supportedSettings = {
-        c: 'accent_color',
-        icon: 'icon',
-        logo: 'logo',
-        cover: 'cover_image'
-    };
-
-    // @TODO: change this to use a proper query string parser - maybe build a fake url and use the url lib
-    let opts = decodeURIComponent(previewHeader).split('&');
-
-    opts.forEach((opt) => {
-        let [key, value] = opt.split('=');
-        if (supportedSettings[key]) {
-            _.set(siteData, supportedSettings[key], value);
-        }
-    });
-
-    siteData._preview = previewHeader;
-
-    return siteData;
-}
-
 function getSiteData(req) {
     let siteData = settingsCache.getPublic();
 
-    if (req.header(PREVIEW_HEADER_NAME)) {
-        siteData = handlePreview(req.header(PREVIEW_HEADER_NAME), siteData);
-    }
+    // @TODO: it would be nicer if this was proper middleware somehow...
+    siteData = preview.handle(req, siteData);
 
     return siteData;
 }

core/frontend/services/themes/preview.js

@@ -0,0 +1,47 @@
+// @TODO: put together a plan for how the frontend should exist as modules and move this out
+// The preview header contains a query string with the custom preview data
+// This is deliberately slightly obscure & means we don't need to add body parsing to the frontend :D
+// If we start passing in strings like title or description we will probably need to change this
+const PREVIEW_HEADER_NAME = 'x-ghost-preview';
+
+const _ = require('lodash');
+
+function decodeValue(value) {
+    if (value === '' || value === 'null' || value === 'undefined') {
+        return null;
+    }
+
+    return value;
+}
+
+function getPreviewData(previewHeader, siteData) {
+    // Keep the string shorter with short codes for certain parameters
+    const supportedSettings = {
+        c: 'accent_color',
+        icon: 'icon',
+        logo: 'logo',
+        cover: 'cover_image'
+    };
+
+    let opts = new URLSearchParams(previewHeader);
+
+    opts.forEach((value, key) => {
+        value = decodeValue(value);
+        if (supportedSettings[key]) {
+            _.set(siteData, supportedSettings[key], value);
+        }
+    });
+
+    siteData._preview = previewHeader;
+
+    return siteData;
+}
+
+module.exports._PREVIEW_HEADER_NAME = PREVIEW_HEADER_NAME;
+module.exports.handle = (req, siteData) => {
+    if (req && req.header(PREVIEW_HEADER_NAME)) {
+        siteData = getPreviewData(req.header(PREVIEW_HEADER_NAME), siteData);
+    }
+
+    return siteData;
+};

test/unit/services/themes/preview_spec.js

@@ -0,0 +1,55 @@
+const should = require('should');
+const sinon = require('sinon');
+
+const preview = require('../../../../core/frontend/services/themes/preview');
+
+describe('Theme Preview', function () {
+    let req, previewString = '';
+
+    before(function () {
+        req = {
+            header: () => {
+                return previewString;
+            }
+        };
+    });
+
+    it('can handle empty strings', function () {
+        previewString = 'logo=';
+
+        let siteData = preview.handle(req, {});
+
+        siteData.should.be.an.Object().with.properties('logo');
+        should(siteData.logo).be.null();
+    });
+
+    it('can handle nulls', function () {
+        previewString = 'cover=null';
+
+        let siteData = preview.handle(req, {});
+
+        siteData.should.be.an.Object().with.properties('cover_image');
+        should(siteData.cover_image).be.null();
+    });
+
+    it('can handle URIEncoded accent colors', function () {
+        previewString = 'c=%23f02d2d';
+
+        let siteData = preview.handle(req, {});
+
+        siteData.should.be.an.Object().with.properties('accent_color');
+        should(siteData.accent_color).eql('#f02d2d');
+    });
+
+    it('can handle multiple values', function () {
+        previewString = 'c=%23f02d2d&icon=&logo=&cover=null';
+
+        let siteData = preview.handle(req, {});
+        siteData.should.be.an.Object().with.properties('accent_color', 'icon', 'logo', 'cover_image');
+
+        should(siteData.accent_color).eql('#f02d2d');
+        should(siteData.icon).be.null();
+        should(siteData.logo).be.null();
+        should(siteData.cover_image).be.null();
+    });
+});