From 592d02fd23a3a440f0be06d8f8cdce18a4ec3742 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Fri, 19 Nov 2021 09:43:48 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20perms=20error=20when=20b?= =?UTF-8?q?uilding=20public=20assets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes: https://github.com/TryGhost/Ghost/issues/13739 - Ghost cannot write to the core folder in correctly configured production installations - Built assets therefore need to be written to the content directory - Ghost does not overwrite anything in the content folder as part of an upgrade, therefore static files that are provided by Ghost must still live inside /core - So as a result, we now have core/frontend/public and content/public --- .gitignore | 3 +- .npmignore | 2 + Gruntfile.js | 1 - content/public/README.md | 3 ++ core/frontend/services/card-assets/service.js | 14 +++---- .../web/middleware/serve-public-file.js | 22 ++++++---- core/frontend/web/site.js | 12 +++--- core/shared/config/defaults.json | 3 +- core/shared/config/helpers.js | 2 + .../frontend/services/card-assets.test.js | 13 ++++++ .../web/middleware/serve-public-file.test.js | 40 +++++++++++++++---- test/unit/shared/config/loader.test.js | 1 + 12 files changed, 84 insertions(+), 32 deletions(-) create mode 100644 content/public/README.md diff --git a/.gitignore b/.gitignore index afbbcfb0e8..92b894161d 100644 --- a/.gitignore +++ b/.gitignore @@ -106,6 +106,7 @@ projectFilesBackup /content/images/**/* /content/media/**/* /content/files/**/* +/content/public/* /content/adapters/storage/**/* /content/adapters/scheduling/**/* !/content/themes/casper @@ -125,8 +126,6 @@ test/coverage # Built asset files /core/built /core/server/web/admin/views/*.html -/core/frontend/public/*.min.css -/core/frontend/public/*.min.js # Caddyfile - for local development with ssl + caddy Caddyfile diff --git a/.npmignore b/.npmignore index 298ea4c5d0..7ab15113fb 100644 --- a/.npmignore +++ b/.npmignore @@ -13,6 +13,8 @@ content/adapters/** !content/adapters/README.md content/apps/** !content/apps/README.md +content/public/** +!content/public/README.md content/data/** !content/data/README.md content/images/** diff --git a/Gruntfile.js b/Gruntfile.js index 582c6e88c0..d19707f222 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -75,7 +75,6 @@ module.exports = function (grunt) { 'core/shared/**/*.js', 'core/frontend/**/*.js', 'core/frontend/src/**/*.css', - '!core/frontend/public/**', 'core/*.js', 'index.js', 'config.*.json', diff --git a/content/public/README.md b/content/public/README.md new file mode 100644 index 0000000000..15aa59bfee --- /dev/null +++ b/content/public/README.md @@ -0,0 +1,3 @@ +# Content / Public + +Ghost will store any built assets here. This goes hand in hand with core/frontend/public where static assets are stored. diff --git a/core/frontend/services/card-assets/service.js b/core/frontend/services/card-assets/service.js index b450f66d74..32fbe898bb 100644 --- a/core/frontend/services/card-assets/service.js +++ b/core/frontend/services/card-assets/service.js @@ -3,12 +3,12 @@ const _ = require('lodash'); const path = require('path'); const fs = require('fs').promises; const logging = require('@tryghost/logging'); +const config = require('../../../shared/config'); class CardAssetService { constructor(options = {}) { - // @TODO: use our config paths concept - this.src = options.src || path.join(__dirname, '../../src/cards'); - this.dest = options.dest || path.join(__dirname, '../../public'); + this.src = options.src || path.join(config.get('paths').assetSrc, 'cards'); + this.dest = options.dest || config.getContentPath('public'); this.minifier = new Minifier({src: this.src, dest: this.dest}); if ('config' in options) { @@ -90,12 +90,12 @@ class CardAssetService { /** * A theme can declare which cards it supports, and we'll do the rest * - * @param {Array|boolean} config + * @param {Array|boolean} cardAssetConfig * @returns */ - async load(config) { - if (config) { - this.config = config; + async load(cardAssetConfig) { + if (cardAssetConfig) { + this.config = cardAssetConfig; } await this.clearFiles(); diff --git a/core/frontend/web/middleware/serve-public-file.js b/core/frontend/web/middleware/serve-public-file.js index 8071528fa3..5f017d8ae8 100644 --- a/core/frontend/web/middleware/serve-public-file.js +++ b/core/frontend/web/middleware/serve-public-file.js @@ -11,10 +11,16 @@ const messages = { fileNotFound: 'File not found' }; -function createPublicFileMiddleware(file, type, maxAge) { +function createPublicFileMiddleware(location, file, mime, maxAge) { let content; - const publicFilePath = config.get('paths').publicFilePath; - const filePath = file.match(/^public/) ? path.join(publicFilePath, file.replace(/^public/, '')) : path.join(publicFilePath, file); + // These files are provided by Ghost, and therefore live inside of the core folder + const staticFilePath = config.get('paths').publicFilePath; + // These files are built on the fly, and must be saved in the content folder + const builtFilePath = config.getContentPath('public'); + + let locationPath = location === 'static' ? staticFilePath : builtFilePath; + + const filePath = file.match(/^public/) ? path.join(locationPath, file.replace(/^public/, '')) : path.join(locationPath, file); const blogRegex = /(\{\{blog-url\}\})/g; return function servePublicFileMiddleware(req, res, next) { @@ -24,7 +30,7 @@ function createPublicFileMiddleware(file, type, maxAge) { } // send image files directly and let express handle content-length, etag, etc - if (type.match(/^image/)) { + if (mime.match(/^image/)) { return res.sendFile(filePath, (err) => { if (err && err.status === 404) { // ensure we're triggering basic asset 404 and not a templated 404 @@ -57,13 +63,13 @@ function createPublicFileMiddleware(file, type, maxAge) { let str = buf.toString(); - if (type === 'text/xsl' || type === 'text/plain' || type === 'application/javascript') { + if (mime === 'text/xsl' || mime === 'text/plain' || mime === 'application/javascript') { str = str.replace(blogRegex, urlUtils.urlFor('home', true).replace(/\/$/, '')); } content = { headers: { - 'Content-Type': type, + 'Content-Type': mime, 'Content-Length': Buffer.from(str).length, ETag: `"${crypto.createHash('md5').update(str, 'utf8').digest('hex')}"`, 'Cache-Control': `public, max-age=${maxAge}` @@ -78,8 +84,8 @@ function createPublicFileMiddleware(file, type, maxAge) { // ### servePublicFile Middleware // Handles requests to robots.txt and favicon.ico (and caches them) -function servePublicFile(file, type, maxAge) { - const publicFileMiddleware = createPublicFileMiddleware(file, type, maxAge); +function servePublicFile(location, file, type, maxAge) { + const publicFileMiddleware = createPublicFileMiddleware(location, file, type, maxAge); return function servePublicFileMiddleware(req, res, next) { if (req.path === '/' + file) { diff --git a/core/frontend/web/site.js b/core/frontend/web/site.js index c7c9f3c701..149e968664 100644 --- a/core/frontend/web/site.js +++ b/core/frontend/web/site.js @@ -104,15 +104,15 @@ module.exports = function setupSiteApp(options = {}) { siteApp.use(mw.serveFavicon()); // Serve sitemap.xsl file - siteApp.use(mw.servePublicFile('sitemap.xsl', 'text/xsl', constants.ONE_DAY_S)); + siteApp.use(mw.servePublicFile('static', 'sitemap.xsl', 'text/xsl', constants.ONE_DAY_S)); // Serve stylesheets for default templates - siteApp.use(mw.servePublicFile('public/ghost.css', 'text/css', constants.ONE_HOUR_S)); - siteApp.use(mw.servePublicFile('public/ghost.min.css', 'text/css', constants.ONE_YEAR_S)); + siteApp.use(mw.servePublicFile('static', 'public/ghost.css', 'text/css', constants.ONE_HOUR_S)); + siteApp.use(mw.servePublicFile('static', 'public/ghost.min.css', 'text/css', constants.ONE_YEAR_S)); // Card assets - siteApp.use(mw.servePublicFile('public/cards.min.css', 'text/css', constants.ONE_YEAR_S)); - siteApp.use(mw.servePublicFile('public/cards.min.js', 'text/js', constants.ONE_YEAR_S)); + siteApp.use(mw.servePublicFile('built', 'public/cards.min.css', 'text/css', constants.ONE_YEAR_S)); + siteApp.use(mw.servePublicFile('built', 'public/cards.min.js', 'text/js', constants.ONE_YEAR_S)); // Serve blog images using the storage adapter siteApp.use(STATIC_IMAGE_URL_PREFIX, mw.handleImageSizes, storage.getStorage('images').serve()); @@ -147,7 +147,7 @@ module.exports = function setupSiteApp(options = {}) { debug('Themes done'); // Serve robots.txt if not found in theme - siteApp.use(mw.servePublicFile('robots.txt', 'text/plain', constants.ONE_HOUR_S)); + siteApp.use(mw.servePublicFile('static', 'robots.txt', 'text/plain', constants.ONE_HOUR_S)); // site map - this should probably be refactored to be an internal app sitemapHandler(siteApp); diff --git a/core/shared/config/defaults.json b/core/shared/config/defaults.json index 2e0fdefcda..c79c3c08e4 100644 --- a/core/shared/config/defaults.json +++ b/core/shared/config/defaults.json @@ -16,7 +16,8 @@ "useMinFiles": true, "paths": { "contentPath": "content/", - "fixtures": "core/server/data/schema/fixtures/fixtures" + "fixtures": "core/server/data/schema/fixtures/fixtures", + "assetSrc": "core/frontend/src" }, "adapters": { "sso": { diff --git a/core/shared/config/helpers.js b/core/shared/config/helpers.js index b96125f1b4..2155017762 100644 --- a/core/shared/config/helpers.js +++ b/core/shared/config/helpers.js @@ -84,6 +84,8 @@ const getContentPath = function getContentPath(type) { return path.join(this.get('paths:contentPath'), 'data/'); case 'settings': return path.join(this.get('paths:contentPath'), 'settings/'); + case 'public': + return path.join(this.get('paths:contentPath'), 'public/'); default: // new Error is allowed here, as we do not want config to depend on @tryghost/error // @TODO: revisit this decision when @tryghost/error is no longer dependent on all of ghost-ignition diff --git a/test/unit/frontend/services/card-assets.test.js b/test/unit/frontend/services/card-assets.test.js index 1526a4be7d..0d4b7d7ef8 100644 --- a/test/unit/frontend/services/card-assets.test.js +++ b/test/unit/frontend/services/card-assets.test.js @@ -73,6 +73,19 @@ describe('Card Asset Service', function () { cardAssets.files.should.eql(['cards.min.css']); }); + it('can correctly load nothing when config is false', async function () { + const cardAssets = new CardAssetService({ + src: srcDir, + dest: destDir + }); + + await fs.writeFile(path.join(srcDir, 'css', 'test.css'), '.test { color: #fff }'); + + await cardAssets.load(false); + + cardAssets.files.should.eql([]); + }); + it('can clearFiles', async function () { const cardAssets = new CardAssetService({ src: srcDir, diff --git a/test/unit/frontend/web/middleware/serve-public-file.test.js b/test/unit/frontend/web/middleware/serve-public-file.test.js index 5fc97394ff..c0ef33d4ea 100644 --- a/test/unit/frontend/web/middleware/serve-public-file.test.js +++ b/test/unit/frontend/web/middleware/serve-public-file.test.js @@ -1,6 +1,7 @@ const should = require('should'); const sinon = require('sinon'); const fs = require('fs-extra'); +const path = require('path'); const servePublicFile = require('../../../../../core/frontend/web/middleware/serve-public-file'); describe('servePublicFile', function () { @@ -19,20 +20,20 @@ describe('servePublicFile', function () { }); it('should return a middleware', function () { - const result = servePublicFile('robots.txt', 'text/plain', 3600); + const result = servePublicFile('static', 'robots.txt', 'text/plain', 3600); result.should.be.a.Function(); }); it('should skip if the request does NOT match the file', function () { - const middleware = servePublicFile('robots.txt', 'text/plain', 3600); + const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); req.path = '/favicon.ico'; middleware(req, res, next); next.called.should.be.true(); }); it('should load the file and send it', function () { - const middleware = servePublicFile('robots.txt', 'text/plain', 3600); + const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); const body = 'User-agent: * Disallow: /'; req.path = '/robots.txt'; @@ -58,11 +59,11 @@ describe('servePublicFile', function () { }); it('should send the correct headers', function () { - const middleware = servePublicFile('robots.txt', 'text/plain', 3600); + const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); const body = 'User-agent: * Disallow: /'; req.path = '/robots.txt'; - sinon.stub(fs, 'readFile').callsFake(function (file, cb) { + let fileStub = sinon.stub(fs, 'readFile').callsFake(function (file, cb) { cb(null, body); }); @@ -72,7 +73,9 @@ describe('servePublicFile', function () { }; middleware(req, res, next); + next.called.should.be.false(); + fileStub.firstCall.args[0].should.endWith('core/frontend/public/robots.txt'); res.writeHead.called.should.be.true(); res.writeHead.args[0][0].should.equal(200); res.writeHead.calledWith(200, sinon.match.has('Content-Type')).should.be.true(); @@ -82,7 +85,7 @@ describe('servePublicFile', function () { }); it('should replace {{blog-url}} in text/plain', function () { - const middleware = servePublicFile('robots.txt', 'text/plain', 3600); + const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); const body = 'User-agent: {{blog-url}}'; req.path = '/robots.txt'; @@ -103,7 +106,7 @@ describe('servePublicFile', function () { }); it('should 404 for ENOENT on general files', function () { - const middleware = servePublicFile('robots.txt', 'text/plain', 3600); + const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); req.path = '/robots.txt'; sinon.stub(fs, 'readFile').callsFake(function (file, cb) { @@ -122,4 +125,27 @@ describe('servePublicFile', function () { next.called.should.be.true(); next.calledWith(sinon.match({errorType: 'NotFoundError', code: 'PUBLIC_FILE_NOT_FOUND'})).should.be.true(); }); + + it('can serve a built asset file as well as public files', function () { + const middleware = servePublicFile('built', 'something.css', 'text/css', 3600); + const body = '.foo {bar: baz}'; + req.path = '/something.css'; + + let fileStub = sinon.stub(fs, 'readFile').callsFake(function (file, cb) { + cb(null, body); + }); + + res = { + writeHead: sinon.spy(), + end: sinon.spy() + }; + + middleware(req, res, next); + + next.called.should.be.false(); + res.writeHead.called.should.be.true(); + res.writeHead.args[0][0].should.equal(200); + + fileStub.firstCall.args[0].should.endWith('content/public/something.css'); + }); }); diff --git a/test/unit/shared/config/loader.test.js b/test/unit/shared/config/loader.test.js index 03ba431564..30b901ea7a 100644 --- a/test/unit/shared/config/loader.test.js +++ b/test/unit/shared/config/loader.test.js @@ -96,6 +96,7 @@ describe('Config Loader', function () { Object.keys(pathConfig).should.eql([ 'contentPath', 'fixtures', + 'assetSrc', 'appRoot', 'corePath', 'clientAssets',