Skip to content

Commit

Permalink
serve auth-frame from public
Browse files Browse the repository at this point in the history
  • Loading branch information
mike182uk committed Dec 13, 2024
1 parent da2c376 commit 496bb44
Show file tree
Hide file tree
Showing 10 changed files with 184 additions and 86 deletions.
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ 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

# Caddyfile - for local development with ssl + caddy
Caddyfile

Expand Down
10 changes: 1 addition & 9 deletions ghost/core/core/bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {adminAuthAssets, cardAssets} = require('./frontend/services/assets-minification');
const {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');
Expand Down Expand Up @@ -51,10 +51,6 @@ class Bridge {
return themeEngine.getActive();
}

ensureAdminAuthAssetsMiddleware() {
return adminAuthAssets.serveMiddleware();
}

async activateTheme(loadedTheme, checkedTheme) {
let settings = {
locale: settingsCache.get('locale')
Expand All @@ -69,10 +65,6 @@ class Bridge {
const cardAssetConfig = this.getCardAssetConfig();
debug('reload card assets config', cardAssetConfig);
cardAssets.invalidate(cardAssetConfig);

// TODO: is this in the right place?
// rebuild asset files
adminAuthAssets.invalidate();
} catch (err) {
logging.error(new errors.InternalServerError({
message: tpl(messages.activateFailed, {theme: loadedTheme.name}),
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
const AdminAuthAssets = require('./AdminAuthAssets');
const CardAssets = require('./CardAssets');

const adminAuthAssets = new AdminAuthAssets();
const cardAssets = new CardAssets();

module.exports = {
adminAuthAssets,
cardAssets
};
8 changes: 3 additions & 5 deletions ghost/core/core/server/web/admin/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 bridge = require('../../../bridge');
const createServeAuthFrameFileMw = require('./middleware/serve-auth-frame-file');

/**
*
Expand Down Expand Up @@ -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', bridge.ensureAdminAuthAssetsMiddleware(), function authFrameMw(req, res, next) {
adminApp.use('/auth-frame', 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 {
Expand All @@ -52,9 +52,7 @@ module.exports = function setupAdminApp() {
} catch (err) {
next(err);
}
}, serveStatic(
path.join(config.getContentPath('public'), 'admin-auth')
));
}, createServeAuthFrameFileMw(config, urlUtils));

// Ember CLI's live-reload script
if (config.get('env') === 'development') {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
const path = require('node:path');
const fs = require('node:fs/promises');

function createServeAuthFrameFileMw(config, urlUtils) {
const placeholders = {
'{{SITE_ORIGIN}}': new URL(urlUtils.getSiteUrl()).origin
};

return function serveAuthFrameFileMw(req, res, next) {
const filename = path.parse(req.url).base;

let basePath = path.join(config.get('paths').publicFilePath, 'admin-auth');
let filePath;

if (filename === '') {
filePath = path.join(basePath, 'index.html');
} else {
filePath = path.join(basePath, filename);
}

return fs.readFile(filePath).then((data) => {
let dataString = data.toString();

for (const [key, value] of Object.entries(placeholders)) {
dataString = dataString.replace(key, value);
}

res.end(dataString);
}).catch(() => {
return next();
});
};
}

module.exports = createServeAuthFrameFileMw;
2 changes: 1 addition & 1 deletion ghost/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"archive": "npm pack",
"dev": "node --watch index.js",
"build:assets:css": "postcss core/frontend/public/ghost.css --no-map --use cssnano -o core/frontend/public/ghost.min.css",
"build:assets:js": "../minifier/bin/minify core/frontend/public/comment-counts.js core/frontend/public/comment-counts.min.js && ../minifier/bin/minify core/frontend/public/member-attribution.js core/frontend/public/member-attribution.min.js",
"build:assets:js": "../minifier/bin/minify core/frontend/public/comment-counts.js core/frontend/public/comment-counts.min.js && ../minifier/bin/minify core/frontend/public/member-attribution.js core/frontend/public/member-attribution.min.js && ../minifier/bin/minify core/frontend/public/admin-auth/message-handler.js core/frontend/public/admin-auth/admin-auth.min.js",
"build:assets": "yarn build:assets:css && yarn build:assets:js",
"test": "yarn test:unit",
"test:base": "mocha --reporter dot --require=./test/utils/overrides.js --exit --trace-warnings --recursive --extension=test.js",
Expand Down
142 changes: 142 additions & 0 deletions ghost/core/test/unit/server/web/admin/middleware.test.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
const should = require('should');
const fs = require('node:fs/promises');
const sinon = require('sinon');

// 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 () {
Expand Down Expand Up @@ -60,5 +63,144 @@ 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();
});
});
});
});

0 comments on commit 496bb44

Please sign in to comment.