From ef143978e7289cbeb8be6ebc7a03f25641105ec5 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Tue, 12 Mar 2024 12:27:18 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20Reduced=20requests=20and=20403?= =?UTF-8?q?=20responses=20for=20comments=20auth=20check=20(#19840)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://linear.app/tryghost/issue/ENG-721 ref https://linear.app/tryghost/issue/ENG-708 Comments-UI loads `/ghost/admin-frame/` in an iframe to check if a Staff User is authenticated in order to show moderation options. That iframe request loads a HTML page which in turn contains a script that fires off an API request that attempts to fetch the logged-in user details, resulting in a 403 "error" showing up when not authenticated. In the vast majority of cases there will be no staff user authenticated so lots of extra requests and "errors" are seen unnecessarily. - adjusted the `/ghost/auth-frame/` endpoint to check if the request contains an Admin session cookie - if it does, continue as before with rendering the HTML page so the script is loaded - if it doesn't, return an empty 204 response avoiding the script request and subsequent 403-generating API request - eliminates the 403 error being generated for all typical visitor traffic, the error should only be seen when an Admin was previously logged in but their cookie is no longer valid (either from logging out, or going past the 6month validity period) --- apps/comments-ui/test/e2e/auth-frame.test.ts | 7 ++++-- apps/comments-ui/test/utils/e2e.ts | 8 +++++++ ghost/core/core/server/web/admin/app.js | 18 ++++++++++++++- ghost/core/test/e2e-server/admin.test.js | 24 ++++++++++++++++++++ ghost/core/test/utils/admin-utils.js | 5 ++++ 5 files changed, 59 insertions(+), 3 deletions(-) diff --git a/apps/comments-ui/test/e2e/auth-frame.test.ts b/apps/comments-ui/test/e2e/auth-frame.test.ts index 3ad23c9df8..7882524820 100644 --- a/apps/comments-ui/test/e2e/auth-frame.test.ts +++ b/apps/comments-ui/test/e2e/auth-frame.test.ts @@ -1,4 +1,4 @@ -import {MOCKED_SITE_URL, MockedApi, initialize, mockAdminAuthFrame} from '../utils/e2e'; +import {MOCKED_SITE_URL, MockedApi, initialize, mockAdminAuthFrame, mockAdminAuthFrame204} from '../utils/e2e'; import {expect, test} from '@playwright/test'; const admin = MOCKED_SITE_URL + '/ghost/'; @@ -34,8 +34,11 @@ test.describe('Auth Frame', async () => { await expect(iframeElement).toHaveCount(1); }); - test('has no admin options', async ({page}) => { + test('has no admin options when not signed in to Ghost admin', async ({page}) => { + await mockAdminAuthFrame204({page, admin}); + const mockedApi = new MockedApi({}); + mockedApi.addComment({ html: '

This is comment 1

' }); diff --git a/apps/comments-ui/test/utils/e2e.ts b/apps/comments-ui/test/utils/e2e.ts index 34082fc387..356c05fcf4 100644 --- a/apps/comments-ui/test/utils/e2e.ts +++ b/apps/comments-ui/test/utils/e2e.ts @@ -76,6 +76,14 @@ export async function mockAdminAuthFrame({admin, page}) { }); } +export async function mockAdminAuthFrame204({admin, page}) { + await page.route(admin + 'auth-frame/', async (route) => { + await route.fulfill({ + status: 204 + }); + }); +} + export async function initialize({mockedApi, page, bodyStyle, ...options}: { mockedApi: MockedApi, page: Page, diff --git a/ghost/core/core/server/web/admin/app.js b/ghost/core/core/server/web/admin/app.js index 8217439c21..2fa6b3aa4a 100644 --- a/ghost/core/core/server/web/admin/app.js +++ b/ghost/core/core/server/web/admin/app.js @@ -30,7 +30,23 @@ module.exports = function setupAdminApp() { } )); - adminApp.use('/auth-frame', serveStatic( + // Auth Frame renders a HTML page that loads some JS which then makes an API + // 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', (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 { + if (req.headers.cookie?.includes('ghost-admin-api-session')) { + next(); + } else { + res.sendStatus(204); + } + } catch (err) { + next(err); + } + }, serveStatic( path.join(config.getContentPath('public'), 'admin-auth') )); diff --git a/ghost/core/test/e2e-server/admin.test.js b/ghost/core/test/e2e-server/admin.test.js index 09c6fc4bb6..2d10ebc9a4 100644 --- a/ghost/core/test/e2e-server/admin.test.js +++ b/ghost/core/test/e2e-server/admin.test.js @@ -44,6 +44,30 @@ describe('Admin Routing', function () { }); }); + describe('Auth Frame', function () { + before(function () { + // ensure the admin-auth folder exists so serveStatic doesn't fall through + adminUtils.stubAuthFrameFiles(configUtils.config.getContentPath('public')); + }); + + it('Renders 204 with no admin session cookie', async function () { + await request + .get('/ghost/auth-frame/') + .set('Origin', config.get('url')) + .expect(204); + }); + + it('Renders static file with admin session cookie', async function () { + await request + .get('/ghost/auth-frame/') + .set('Origin', config.get('url')) + .set('Cookie', [ + 'ghost-admin-api-session=abc; Path=/; HttpOnly; Secure; SameSite=Strict' + ]) + .expect(200); + }); + }); + describe('Admin Redirects', function () { it('should redirect /GHOST/ to /ghost/', async function () { await request.get('/GHOST/') diff --git a/ghost/core/test/utils/admin-utils.js b/ghost/core/test/utils/admin-utils.js index df2e1c8d65..793f8c970f 100644 --- a/ghost/core/test/utils/admin-utils.js +++ b/ghost/core/test/utils/admin-utils.js @@ -15,3 +15,8 @@ module.exports.stubAdminFiles = () => { fs.ensureFileSync(filePath); }); }; + +module.exports.stubAuthFrameFiles = (publicPath) => { + const filePath = path.resolve(publicPath, 'admin-auth/index.html'); + fs.ensureFileSync(filePath); +};