Reverted admin auth middleware & minification changes (#23179)

ref #23006
ref https://linear.app/ghost/issue/ONC-919/

Following the minification changes to the various frontend public
assets, it seems the auth frame is broken on Ghost Pro. It's unclear why
this is - it still works perfectly fine locally, and I wasn't able to
easily track down the cause given the urgency vs. benefit. Frankly,
there's not much benefit here besides trying to establish a new model
that a bit more purposeful.

It seems likely it was an issue in the middleware and not the
minification process.

Author: 9larsons

.gitignore

@@ -122,7 +122,6 @@ test/functional/*.png
 /ghost/core/core/frontend/public/ghost.min.css
 /ghost/core/core/frontend/public/comment-counts.min.js
 /ghost/core/core/frontend/public/member-attribution.min.js
-/ghost/core/core/frontend/public/admin-auth/admin-auth.min.js
 /ghost/core/core/frontend/public/ghost-stats.min.js
 # Caddyfile - for local development with ssl + caddy
 Caddyfile

ghost/core/core/bridge.js

@@ -16,7 +16,7 @@ const logging = require('@tryghost/logging');
 const tpl = require('@tryghost/tpl');
 const themeEngine = require('./frontend/services/theme-engine');
 const appService = require('./frontend/services/apps');
-const {cardAssets} = require('./frontend/services/assets-minification');
+const {adminAuthAssets, cardAssets} = require('./frontend/services/assets-minification');
 const routerManager = require('./frontend/services/routing').routerManager;
 const settingsCache = require('./shared/settings-cache');
 const urlService = require('./server/services/url');
@@ -51,6 +51,10 @@ class Bridge {
         return themeEngine.getActive();
     }
 
+    ensureAdminAuthAssetsMiddleware() {
+        return adminAuthAssets.serveMiddleware();
+    }
+
     async activateTheme(loadedTheme, checkedTheme) {
         let settings = {
             locale: settingsCache.get('locale')
@@ -65,6 +69,9 @@ class Bridge {
             const cardAssetConfig = this.getCardAssetConfig();
             debug('reload card assets config', cardAssetConfig);
             cardAssets.invalidate(cardAssetConfig);
+
+            // rebuild asset files
+            adminAuthAssets.invalidate();
         } catch (err) {
             logging.error(new errors.InternalServerError({
                 message: tpl(messages.activateFailed, {theme: loadedTheme.name}),
@@ -106,4 +113,4 @@ class Bridge {
 
 const bridge = new Bridge();
 
-module.exports = bridge;
+module.exports = bridge;

ghost/core/core/frontend/public/admin-auth/admin-auth.min.js

@@ -0,0 +1,68 @@
+// const debug = require('@tryghost/debug')('comments-counts-assets');
+const path = require('path');
+const fs = require('fs');
+const logging = require('@tryghost/logging');
+const config = require('../../../shared/config');
+const urlUtils = require('../../../shared/url-utils');
+
+const Minifier = require('./Minifier');
+const AssetsMinificationBase = require('./AssetsMinificationBase');
+
+module.exports = class AdminAuthAssets extends AssetsMinificationBase {
+    constructor(options = {}) {
+        super(options);
+
+        this.src = options.src || path.join(config.get('paths').assetSrc, 'admin-auth');
+        /** @private */
+        this.dest = options.dest || path.join(config.getContentPath('public'), 'admin-auth');
+
+        this.minifier = new Minifier({src: this.src, dest: this.dest});
+
+        try {
+            fs.mkdirSync(this.dest, {recursive: true});
+            fs.copyFileSync(path.join(this.src, 'index.html'), path.join(this.dest, 'index.html'));
+        } catch (error) {
+            if (error.code === 'EACCES') {
+                logging.error('Ghost was not able to write admin-auth asset files due to permissions.');
+                return;
+            }
+
+            throw error;
+        }
+    }
+
+    /**
+     * @override
+     */
+    generateGlobs() {
+        return {
+            'admin-auth.min.js': '*.js'
+        };
+    }
+
+    /**
+     * @private
+     */
+    generateReplacements() {
+        // Clean the URL, only keep schema, host and port (without trailing slashes or subdirectory)
+        const url = new URL(urlUtils.getSiteUrl());
+        const origin = url.origin;
+
+        return {
+            // Properly encode the origin
+            '\'{{SITE_ORIGIN}}\'': JSON.stringify(origin)
+        };
+    }
+
+    /**
+     * Minify, move into the destination directory, and clear existing asset files.
+     *
+     * @override
+     * @returns {Promise<void>}
+     */
+    async load() {
+        const globs = this.generateGlobs();
+        const replacements = this.generateReplacements();
+        await this.minify(globs, {replacements});
+    }
+};

ghost/core/core/frontend/services/assets-minification/AdminAuthAssets.js

@@ -1,6 +1,9 @@
 const CardAssets = require('./CardAssets');
+const AdminAuthAssets = require('./AdminAuthAssets');
 const cardAssets = new CardAssets();
+const adminAuthAssets = new AdminAuthAssets();
 
 module.exports = {
-    cardAssets
+    cardAssets,
+    adminAuthAssets
 };

ghost/core/core/frontend/services/assets-minification/index.js

@@ -9,7 +9,7 @@ const shared = require('../shared');
 const errorHandler = require('@tryghost/mw-error-handler');
 const sentry = require('../../../shared/sentry');
 const redirectAdminUrls = require('./middleware/redirect-admin-urls');
-const createServeAuthFrameFileMw = require('./middleware/serve-auth-frame-file');
+const bridge = require('../../../bridge');
 
 /**
  *
@@ -39,7 +39,7 @@ module.exports = function setupAdminApp() {
     // request to the Admin API /users/me/ endpoint to check if the user is logged in.
     //
     // Used by comments-ui to add moderation options to front-end comments when logged in.
-    adminApp.use('/auth-frame', function authFrameMw(req, res, next) {
+    adminApp.use('/auth-frame', bridge.ensureAdminAuthAssetsMiddleware(), function authFrameMw(req, res, next) {
         // only render content when we have an Admin session cookie,
         // otherwise return a 204 to avoid JS and API requests being made unnecessarily
         try {
@@ -52,7 +52,9 @@ module.exports = function setupAdminApp() {
         } catch (err) {
             next(err);
         }
-    }, createServeAuthFrameFileMw(config, urlUtils));
+    }, serveStatic(
+        path.join(config.getContentPath('public'), 'admin-auth')
+    ));
 
     // Ember CLI's live-reload script
     if (config.get('env') === 'development') {
@@ -93,4 +95,4 @@ module.exports = function setupAdminApp() {
     debug('Admin setup end');
 
     return adminApp;
-};
+};

ghost/core/core/frontend/public/admin-auth/index.html renamed to ghost/core/core/frontend/src/admin-auth/index.html

@@ -1,9 +1,6 @@
 const sinon = require('sinon');
-const fs = require('node:fs/promises');
 // Thing we are testing
 const redirectAdminUrls = require('../../../../../core/server/web/admin/middleware/redirect-admin-urls');
-const createServeAuthFrameFileMw = require('../../../../../core/server/web/admin/middleware/serve-auth-frame-file');
-const path = require('node:path');
 
 describe('Admin App', function () {
     afterEach(function () {
@@ -61,144 +58,5 @@ describe('Admin App', function () {
                 res.redirect.called.should.be.false();
             });
         });
-
-        describe('serveAuthFrameFile', function () {
-            let config;
-            let urlUtils;
-            let readFile;
-
-            const siteUrl = 'https://foo.bar';
-            const publicFilePath = 'foo/bar/public';
-
-            const indexContent = '<html><body><h1>Hello, world!</h1></body></html>';
-            const fooJsContent = '(function() { console.log("Hello, world!"); })();';
-            const fooJsContentWithSiteOrigin = '(function() { console.log("{{SITE_ORIGIN}}"); })();';
-
-            function createMiddleware() {
-                return createServeAuthFrameFileMw(config, urlUtils);
-            }
-
-            beforeEach(function () {
-                config = {
-                    get: sinon.stub()
-                };
-
-                config.get.withArgs('paths').returns({
-                    publicFilePath
-                });
-
-                urlUtils = {
-                    getSiteUrl: sinon.stub().returns(siteUrl)
-                };
-                readFile = sinon.stub(fs, 'readFile');
-
-                const adminAuthFilePath = filename => path.join(publicFilePath, 'admin-auth', filename);
-
-                readFile.withArgs(adminAuthFilePath('index.html'))
-                    .resolves(Buffer.from(indexContent));
-                readFile.withArgs(adminAuthFilePath('foo.js'))
-                    .resolves(Buffer.from(fooJsContent));
-                readFile.withArgs(adminAuthFilePath('foo-2.js'))
-                    .resolves(Buffer.from(fooJsContentWithSiteOrigin));
-                readFile.withArgs(adminAuthFilePath('bar.js'))
-                    .rejects(new Error('File not found'));
-            });
-
-            afterEach(function () {
-                readFile.restore();
-            });
-
-            it('should serve index.html if the url is /', async function () {
-                const middleware = createMiddleware();
-
-                const req = {
-                    url: '/'
-                };
-                const res = {
-                    end: sinon.stub()
-                };
-                const next = sinon.stub();
-
-                await middleware(req, res, next);
-
-                res.end.calledWith(indexContent).should.be.true();
-                next.called.should.be.false();
-            });
-
-            it('should serve the correct file corresponding to the url', async function () {
-                const middleware = createMiddleware();
-
-                const req = {
-                    url: '/foo.js'
-                };
-                const res = {
-                    end: sinon.stub()
-                };
-                const next = sinon.stub();
-
-                await middleware(req, res, next);
-
-                res.end.calledWith(fooJsContent).should.be.true();
-                next.called.should.be.false();
-            });
-
-            it('should replace {{SITE_ORIGIN}} with the site url', async function () {
-                const middleware = createMiddleware();
-
-                const req = {
-                    url: '/foo-2.js'
-                };
-                const res = {
-                    end: sinon.stub()
-                };
-                const next = sinon.stub();
-
-                await middleware(req, res, next);
-
-                res.end.calledOnce.should.be.true();
-
-                const args = res.end.getCall(0).args;
-                args[0].toString().includes(siteUrl).should.be.true();
-                args[0].toString().includes(`{{SITE_ORIGIN}}`).should.be.false();
-            });
-
-            it('should not allow path traversal', async function () {
-                const middleware = createMiddleware();
-
-                const req = {
-                    url: '/foo/../../foo.js'
-                };
-                const res = {
-                    end: sinon.stub()
-                };
-                const next = sinon.stub();
-
-                await middleware(req, res, next);
-
-                res.end.calledOnce.should.be.true();
-
-                // Because we use base name when resolving the file, foo.js should be served
-                res.end.calledWith(fooJsContent).should.be.true();
-
-                next.calledOnce.should.be.false();
-            });
-
-            it('should call next if the file is not found', async function () {
-                const middleware = createMiddleware();
-
-                const req = {
-                    url: '/bar.js'
-                };
-                const res = {
-                    end: sinon.stub()
-                };
-                const next = sinon.stub();
-
-                await middleware(req, res, next);
-
-                res.end.calledOnce.should.be.false();
-                next.calledOnce.should.be.true();
-            });
-        });
     });
 });