Fixed private site comment read middleware (TryGhost#30045)

no ref

Co-authored-by: UserExistsError <23325451+UserExistsError@users.noreply.github.com>

Author: acburdine

ghost/core/core/frontend/apps/private-blogging/lib/middleware.js

@@ -1,12 +1,11 @@
 const fs = require('fs-extra');
-const session = require('cookie-session');
-const crypto = require('crypto');
 const path = require('path');
 const config = require('../../../../shared/config');
 const urlUtils = require('../../../../shared/url-utils').default;
 const tpl = require('@tryghost/tpl');
 const errors = require('@tryghost/errors');
 const settingsCache = require('../../../../shared/settings-cache');
+const privateSiteAccess = require('../../../../shared/private-site-access');
 // routeKeywords.private: 'private'
 const privateRoute = '/private/';
 
@@ -15,27 +14,6 @@ const messages = {
     wrongAccessCode: 'Incorrect access code.'
 };
 
-function getAccessCode() {
-    const accessCode = settingsCache.get('password');
-    return typeof accessCode === 'string' ? accessCode : '';
-}
-
-function hasAccessCode(accessCode) {
-    return typeof accessCode === 'string' && accessCode.trim().length > 0;
-}
-
-function verifySessionHash(salt, hash) {
-    const accessCode = getAccessCode();
-
-    if (!salt || !hash || !hasAccessCode(accessCode)) {
-        return false;
-    }
-
-    let hasher = crypto.createHash('sha256');
-    hasher.update(accessCode + salt, 'utf8');
-    return hasher.digest('hex') === hash;
-}
-
 function getRedirectUrl(query) {
     try {
         const redirect = decodeURIComponent(query.r || '/');
@@ -57,11 +35,7 @@ function getRedirectUrl(query) {
 }
 
 function authenticatePrivateSession(req, res, next) {
-    const hash = req.session.token || '';
-    const salt = req.session.salt || '';
-    const isVerified = verifySessionHash(salt, hash);
-
-    if (isVerified) {
+    if (privateSiteAccess.hasAccess(req)) {
         return next();
     } else {
         let redirectUrl = urlUtils.urlFor({relativeUrl: privateRoute});
@@ -72,23 +46,7 @@ function authenticatePrivateSession(req, res, next) {
 }
 
 const privateBlogging = {
-    checkIsPrivate: function checkIsPrivate(req, res, next) {
-        let isPrivateBlog = settingsCache.get('is_private');
-
-        if (!isPrivateBlog) {
-            res.isPrivateBlog = false;
-            return next();
-        }
-
-        res.isPrivateBlog = true;
-
-        return session({
-            name: 'ghost-private',
-            maxAge: (30 * 24 * 60 * 60 * 1000), // 30 days in ms
-            signed: false,
-            sameSite: 'none'
-        })(req, res, next);
-    },
+    checkIsPrivate: privateSiteAccess.loadSession,
 
     filterPrivateRoutes: function filterPrivateRoutes(req, res, next) {
         // If this site is not in private mode, skip
@@ -145,11 +103,7 @@ const privateBlogging = {
             return res.redirect(urlUtils.urlFor('home', true));
         }
 
-        const hash = req.session.token || '';
-        const salt = req.session.salt || '';
-        const isVerified = verifySessionHash(salt, hash);
-
-        if (isVerified) {
+        if (privateSiteAccess.hasAccess(req)) {
             // redirect to home if user is already authenticated
             return res.redirect(urlUtils.urlFor('home', true));
         } else {
@@ -164,16 +118,9 @@ const privateBlogging = {
         }
 
         const submittedAccessCode = req.body && req.body.password;
-        const accessCode = getAccessCode();
-        const hasher = crypto.createHash('sha256');
-        const salt = Date.now().toString();
         const forward = getRedirectUrl(req.query);
 
-        if (hasAccessCode(accessCode) && hasAccessCode(submittedAccessCode) && accessCode === submittedAccessCode) {
-            hasher.update(submittedAccessCode + salt, 'utf8');
-            req.session.token = hasher.digest('hex');
-            req.session.salt = salt;
-
+        if (privateSiteAccess.grantAccess(req, submittedAccessCode)) {
             return res.redirect(urlUtils.urlFor({relativeUrl: forward}));
         } else {
             res.error = {

ghost/core/core/server/web/comments/routes.js

@@ -8,9 +8,11 @@ const tpl = require('@tryghost/tpl');
 
 const bodyParser = require('body-parser');
 const membersService = require('../../../server/services/members');
+const privateSiteAccess = require('../../../shared/private-site-access');
 
 const messages = {
-    memberCommentingDisabled: 'Your commenting ability has been disabled.'
+    memberCommentingDisabled: 'Your commenting ability has been disabled.',
+    privateSiteAccessRequired: 'Comment browsing is not available'
 };
 
 /**
@@ -27,29 +29,50 @@ function checkMemberCommenting(req, res, next) {
     next();
 }
 
+/**
+ * Middleware to reject comment read requests without a valid private-site
+ * session when the site is in private mode.
+ */
+function checkCanReadComments(req, res, next) {
+    if (res.isPrivateBlog && !privateSiteAccess.hasAccess(req)) {
+        return next(new errors.NoPermissionError({
+            message: tpl(messages.privateSiteAccessRequired)
+        }));
+    }
+    next();
+}
+
 /**
  * @returns {import('express').Router}
  */
 module.exports = function apiRoutes() {
     const router = express.Router('comment api');
     router.use(bodyParser.json({limit: '50mb'}));
+    router.use(privateSiteAccess.loadSession);
 
-    const countsCache = shared.middleware.cacheControl(
+    const publicCountsCache = shared.middleware.cacheControl(
         'public',
         {maxAge: config.get('caching:commentsCountAPI:maxAge')}
     );
-    router.get('/counts', countsCache, http(api.commentsMembers.counts));
+    const privateCountsCache = shared.middleware.cacheControl('private');
+    const countsCache = (req, res, next) => {
+        if (res.isPrivateBlog) {
+            return privateCountsCache(req, res, next);
+        }
+        return publicCountsCache(req, res, next);
+    };
+    router.get('/counts', checkCanReadComments, countsCache, http(api.commentsMembers.counts));
 
-    // Authenticated Routes
+    // Load the optional member session for member-specific comment state
     router.use(membersService.middleware.loadMemberSession);
 
     // Enforce capped limit parameter
     router.use(shared.middleware.maxLimitCap);
 
-    router.get('/', http(api.commentsMembers.browse));
-    router.get('/post/:post_id', http(api.commentsMembers.browse));
-    router.get('/:id', http(api.commentsMembers.read));
-    router.get('/:id/replies', http(api.commentsMembers.replies));
+    router.get('/', checkCanReadComments, http(api.commentsMembers.browse));
+    router.get('/post/:post_id', checkCanReadComments, http(api.commentsMembers.browse));
+    router.get('/:id', checkCanReadComments, http(api.commentsMembers.read));
+    router.get('/:id/replies', checkCanReadComments, http(api.commentsMembers.replies));
 
     // Write operations require member to have commenting ability enabled
     router.post('/', checkMemberCommenting, http(api.commentsMembers.add));

ghost/core/core/shared/private-site-access/index.js

@@ -0,0 +1,68 @@
+const crypto = require('crypto');
+const session = require('cookie-session');
+const settingsCache = require('../settings-cache');
+
+const privateSession = session({
+    name: 'ghost-private',
+    maxAge: 30 * 24 * 60 * 60 * 1000,
+    signed: false,
+    sameSite: 'none'
+});
+
+function getAccessCode() {
+    const accessCode = settingsCache.get('password');
+    return typeof accessCode === 'string' ? accessCode : '';
+}
+
+function hasAccessCode(accessCode) {
+    return typeof accessCode === 'string' && accessCode.trim().length > 0;
+}
+
+function hashAccessCode(accessCode, salt) {
+    const hasher = crypto.createHash('sha256');
+    hasher.update(accessCode + salt, 'utf8');
+    return hasher.digest('hex');
+}
+
+function hasAccess(req) {
+    const accessCode = getAccessCode();
+    const salt = req.session?.salt || '';
+    const hash = req.session?.token || '';
+
+    if (!salt || !hash || !hasAccessCode(accessCode)) {
+        return false;
+    }
+
+    return hashAccessCode(accessCode, salt) === hash;
+}
+
+function grantAccess(req, submittedAccessCode) {
+    const accessCode = getAccessCode();
+
+    if (!hasAccessCode(accessCode) || !hasAccessCode(submittedAccessCode) || accessCode !== submittedAccessCode) {
+        return false;
+    }
+
+    const salt = Date.now().toString();
+    req.session.token = hashAccessCode(submittedAccessCode, salt);
+    req.session.salt = salt;
+    return true;
+}
+
+function loadSession(req, res, next) {
+    const isPrivateBlog = settingsCache.get('is_private');
+
+    if (!isPrivateBlog) {
+        res.isPrivateBlog = false;
+        return next();
+    }
+
+    res.isPrivateBlog = true;
+    return privateSession(req, res, next);
+}
+
+module.exports = {
+    grantAccess,
+    hasAccess,
+    loadSession
+};

ghost/core/test/e2e-api/members-comments/comments.test.js

@@ -1,11 +1,14 @@
 const assert = require('node:assert/strict');
-const {agentProvider, mockManager, fixtureManager, matchers, configUtils, dbUtils} = require('../../utils/e2e-framework');
+const {agentProvider, mockManager, fixtureManager, matchers, configUtils, dbUtils, cacheRules} = require('../../utils/e2e-framework');
 const {nullable, anyEtag, anyObjectId, anyLocationFor, anyISODateTime, anyErrorId, anyUuid, anyNumber, anyBoolean, stringMatching} = matchers;
 const models = require('../../../core/server/models');
 const moment = require('moment-timezone');
 const settingsCache = require('../../../core/shared/settings-cache');
 const sinon = require('sinon');
 const DomainEvents = require('@tryghost/domain-events');
+const TestAgent = require('../../utils/agents/test-agent');
+const membersService = require('../../../core/server/services/members');
+const privateSiteAccess = require('../../../core/shared/private-site-access');
 
 let membersAgent, membersAgent2, postId, postAuthorEmail, postTitle;
 let emailMockReceiver;
@@ -2539,4 +2542,118 @@ describe('Comments API', function () {
             });
         });
     });
+
+    // Require private-site access for comment reads in private mode
+    describe('When site is in private mode', function () {
+        const privateAccessCode = 'private-comments-test';
+        let comment;
+        let originalIsPrivateSetting;
+        let originalPasswordSetting;
+
+        function createSiteAgent() {
+            return new TestAgent(membersAgent.app, {
+                apiURL: '',
+                originURL: configUtils.config.get('url')
+            });
+        }
+
+        async function loginAsMember(agent) {
+            const magicLink = await membersService.api.getMagicLink('member-any@example.com', 'signin');
+            const token = new URL(magicLink).searchParams.get('token');
+            await agent.get(`/members/?token=${token}`).expectStatus(302);
+        }
+
+        function grantPrivateSiteAccess(agent) {
+            const req = {session: {}};
+            assert.equal(privateSiteAccess.grantAccess(req, privateAccessCode), true);
+
+            const sessionCookie = Buffer.from(JSON.stringify(req.session)).toString('base64');
+            agent.jar.setCookies([`ghost-private=${sessionCookie}; path=/; httponly`]);
+        }
+
+        function commentReadPaths() {
+            return [
+                '/members/api/comments/counts',
+                '/members/api/comments',
+                `/members/api/comments/post/${postId}`,
+                `/members/api/comments/${comment.id}`,
+                `/members/api/comments/${comment.id}/replies`
+            ];
+        }
+
+        async function expectCommentReadStatus(agent, status) {
+            for (const path of commentReadPaths()) {
+                await agent.get(path).expectStatus(status);
+            }
+        }
+
+        beforeAll(async function () {
+            await models.Post.edit({visibility: 'public'}, {id: postId});
+
+            originalIsPrivateSetting = settingsCache.get('is_private', {resolve: false});
+            originalPasswordSetting = settingsCache.get('password', {resolve: false});
+        });
+
+        beforeEach(async function () {
+            settingsCache.set('is_private', {...originalIsPrivateSetting, value: true});
+            settingsCache.set('password', {...originalPasswordSetting, value: privateAccessCode});
+
+            comment = await dbFns.addComment({
+                post_id: postId,
+                member_id: fixtureManager.get('members', 0).id
+            });
+        });
+
+        afterAll(async function () {
+            settingsCache.set('is_private', originalIsPrivateSetting);
+            settingsCache.set('password', originalPasswordSetting);
+        });
+
+        it('Rejects anonymous visitors without private-site access', async function () {
+            await expectCommentReadStatus(createSiteAgent(), 403);
+        });
+
+        it('Does not treat a member session as private-site access', async function () {
+            const agent = createSiteAgent();
+            await loginAsMember(agent);
+            await expectCommentReadStatus(agent, 403);
+        });
+
+        it('Allows visitors with private-site access', async function () {
+            const agent = createSiteAgent();
+            grantPrivateSiteAccess(agent);
+            await expectCommentReadStatus(agent, 200);
+        });
+
+        it('Allows members with private-site access', async function () {
+            const agent = createSiteAgent();
+            await loginAsMember(agent);
+            grantPrivateSiteAccess(agent);
+            await expectCommentReadStatus(agent, 200);
+        });
+
+        it('Invalidates private-site access when the access code changes', async function () {
+            const agent = createSiteAgent();
+            grantPrivateSiteAccess(agent);
+
+            settingsCache.set('password', {...originalPasswordSetting, value: 'changed-private-comments-test'});
+
+            await agent.get('/members/api/comments').expectStatus(403);
+        });
+
+        it('Prevents shared caching of private comment counts', async function () {
+            const agent = createSiteAgent();
+            grantPrivateSiteAccess(agent);
+
+            const response = await agent.get('/members/api/comments/counts').expectStatus(200);
+            assert.equal(response.headers['cache-control'], cacheRules.private);
+        });
+
+        it('Keeps public comment counts publicly cacheable', async function () {
+            settingsCache.set('is_private', {...originalIsPrivateSetting, value: false});
+
+            const response = await createSiteAgent().get('/members/api/comments/counts').expectStatus(200);
+            assert.match(response.headers['cache-control'], /^public, max-age=/);
+        });
+    });
 });