Misc fixes

ghost/core/core/frontend/services/theme-engine/i18n/I18n.js

@@ -14,9 +14,9 @@ const get = require('lodash/get');
 class I18n {
     /**
      * @param {object} [options]
-     * @param {string} basePath - the base path to the translations directory
-     * @param {string} [locale] - a locale string
-     * @param {{dot|fulltext}} [stringMode] - which mode our translation keys use
+     * @param {string} options.basePath - the base path to the translations directory
+     * @param {string} [options.locale] - a locale string
+     * @param {string} [options.stringMode] - which mode our translation keys use
      */
     constructor(options = {}) {
         this._basePath = options.basePath || __dirname;
@@ -100,7 +100,7 @@ class I18n {
     /**
      * Attempt to load strings from a file
      *
-     * @param {sting} [locale]
+     * @param {string} [locale]
      * @returns {object} strings
      */
     _loadStrings(locale) {
@@ -110,7 +110,7 @@ class I18n {
             return this._readTranslationsFile(locale);
         } catch (err) {
             if (err.code === 'ENOENT') {
-                this._handleMissingFileError(locale, err);
+                this._handleMissingFileError(locale);
 
                 if (locale !== this.defaultLocale()) {
                     this._handleFallbackToDefault();
@@ -207,7 +207,7 @@ class I18n {
      */
     _readTranslationsFile(locale) {
         const filePath = path.join(...this._translationFileDirs(), this._translationFileName(locale));
-        const content = fs.readFileSync(filePath);
+        const content = fs.readFileSync(filePath, 'utf8');
         return JSON.parse(content);
     }
 
@@ -276,6 +276,7 @@ class I18n {
     _handleMissingFileError(locale) {
         logging.warn(`i18n was unable to find ${locale}.json.`);
     }
+
     _handleInvalidFileError(locale, err) {
         logging.error(new errors.IncorrectUsageError({
             err,

ghost/core/core/frontend/services/theme-engine/i18n/ThemeI18n.js

@@ -4,9 +4,9 @@ const I18n = require('./I18n');
 
 class ThemeI18n extends I18n {
     /**
-     * @param {objec} [options]
-     * @param {string} basePath - the base path for the translation directory (e.g. where themes live)
-     * @param {string} [locale] - a locale string
+     * @param {object} [options]
+     * @param {string} options.basePath - the base path for the translation directory (e.g. where themes live)
+     * @param {string} [options.locale] - a locale string
      */
     constructor(options = {}) {
         super(options);
@@ -48,6 +48,7 @@ class ThemeI18n extends I18n {
             logging.warn(`Theme translations file locales/${locale}.json not found.`);
         }
     }
+
     _handleInvalidFileError(locale, err) {
         logging.error(new errors.IncorrectUsageError({
             err,

ghost/core/core/server/adapters/storage/LocalStorageBase.js

@@ -58,7 +58,15 @@ class LocalStorageBase extends StorageBase {
         targetFilename = filename;
         await fs.mkdirs(targetDir);
 
-        await fs.copy(file.path, targetFilename);
+        try {
+            await fs.copy(file.path, targetFilename);
+        } catch (err) {
+            if (err.code === 'ENAMETOOLONG') {
+                throw new errors.BadRequestError({err});
+            }
+
+            throw err;
+        }
 
         // The src for the image must be in URI format, not a file system path, which in Windows uses \
         // For local file system storage can use relative path so add a slash

ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap

@@ -71,3 +71,21 @@ Object {
   ],
 }
 `;
+
+exports[`Images API Will error when filename is too long 1: [body] 1`] = `
+Object {
+  "errors": Array [
+    Object {
+      "code": "ENAMETOOLONG",
+      "context": "The request could not be understood.",
+      "details": null,
+      "ghostErrorCode": null,
+      "help": null,
+      "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
+      "message": "Request not understood error, cannot upload image.",
+      "property": null,
+      "type": "BadRequestError",
+    },
+  ],
+}
+`;

ghost/core/test/e2e-api/admin/images.test.js

@@ -185,6 +185,20 @@ describe('Images API', function () {
         await uploadImageCheck({path: originalFilePath, filename: 'loadingcat_square.gif', contentType: 'image/gif'});
     });
 
+    it('Will error when filename is too long', async function () {
+        const originalFilePath = p.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png');
+        const fileContents = await fs.readFile(originalFilePath);
+        const loggingStub = sinon.stub(logging, 'error');
+        await uploadImageRequest({fileContents, filename: `${'a'.repeat(300)}.png`, contentType: 'image/png'})
+            .expectStatus(400)
+            .matchBodySnapshot({
+                errors: [{
+                    id: anyErrorId
+                }]
+            });
+        sinon.assert.calledOnce(loggingStub);
+    });
+
     it('Can not upload a json file', async function () {
         const originalFilePath = p.join(__dirname, '/../../utils/fixtures/data/redirects.json');
         const fileContents = await fs.readFile(originalFilePath);

ghost/core/test/e2e-api/admin/oembed.test.js

@@ -107,7 +107,7 @@ describe('Oembed API', function () {
             .set('Origin', config.get('url'))
             .expect('Content-Type', /json/)
             .expect('Cache-Control', testUtils.cacheRules.private)
-            .expect(401);
+            .expect(422);
 
         requestMock.isDone().should.be.true();
         should.exist(res.body.errors);

ghost/oembed-service/lib/OEmbedService.js

@@ -10,6 +10,7 @@ const messages = {
     noUrlProvided: 'No url provided.',
     insufficientMetadata: 'URL contains insufficient metadata.',
     unknownProvider: 'No provider found for supplied URL.',
+    unableToFetchOembed: 'Unable to fetch requested embed.',
     unauthorized: 'URL contains a private resource.'
 };
 
@@ -101,15 +102,17 @@ class OEmbedService {
         try {
             return await extract(url);
         } catch (err) {
-            if (err.message === 'Request failed with error code 401') {
-                throw new errors.UnauthorizedError({
-                    message: messages.unauthorized
-                });
-            } else {
-                throw new errors.InternalServerError({
-                    message: err.message
+            if (err.message === 'Request failed with error code 401' || err.message === 'Request failed with error code 403') {
+                throw new errors.ValidationError({
+                    message: tpl(messages.unableToFetchOembed),
+                    context: messages.unauthorized
                 });
             }
+
+            throw new errors.ValidationError({
+                message: tpl(messages.unableToFetchOembed),
+                context: err.message
+            });
         }
     }
 

ghost/oembed-service/test/oembed-service.test.js

@@ -0,0 +1,99 @@
+const assert = require('assert/strict');
+const nock = require('nock');
+
+const OembedService = require('../');
+
+describe('oembed-service', function () {
+    /** @type {OembedService} */
+    let oembedService;
+
+    before(function () {
+        oembedService = new OembedService({});
+
+        nock.disableNetConnect();
+    });
+
+    afterEach(function () {
+        nock.cleanAll();
+    });
+
+    describe('known provider', function () {
+        it('should return data if successful', async function () {
+            nock('https://www.youtube.com')
+                .get('/oembed')
+                .query(true)
+                .reply(200, {
+                    title: 'Test Title',
+                    author_name: 'Test Author',
+                    author_url: 'https://www.youtube.com/user/testauthor',
+                    html: '<iframe src="https://www.youtube.com/embed/1234"></iframe>'
+                });
+
+            const response = await oembedService.knownProvider('https://www.youtube.com/watch?v=1234');
+            assert.equal(response.title, 'Test Title');
+            assert.equal(response.author_name, 'Test Author');
+            assert.equal(response.author_url, 'https://www.youtube.com/user/testauthor');
+            assert.equal(response.html, '<iframe src="https://www.youtube.com/embed/1234"></iframe>');
+        });
+
+        it('should return a ValidationError if upstream 401s', async function () {
+            nock('https://www.youtube.com')
+                .get('/oembed')
+                .query(true)
+                .reply(401);
+
+            try {
+                await oembedService.knownProvider('https://www.youtube.com/watch?v=1234');
+            } catch (error) {
+                assert.equal(error.name, 'ValidationError');
+                assert.equal(error.statusCode, 422);
+                assert.equal(error.context, 'URL contains a private resource.');
+            }
+        });
+
+        it('should return a ValidationError if upstream 403s', async function () {
+            nock('https://www.youtube.com')
+                .get('/oembed')
+                .query(true)
+                .reply(403);
+
+            try {
+                await oembedService.knownProvider('https://www.youtube.com/watch?v=1234');
+            } catch (error) {
+                assert.equal(error.name, 'ValidationError');
+                assert.equal(error.statusCode, 422);
+                assert.equal(error.context, 'URL contains a private resource.');
+            }
+        });
+
+        it('should return a ValidationError if upstream 404s', async function () {
+            nock('https://www.youtube.com')
+                .get('/oembed')
+                .query(true)
+                .reply(404);
+
+            try {
+                await oembedService.knownProvider('https://www.youtube.com/watch?v=1234');
+            } catch (error) {
+                assert.equal(error.name, 'ValidationError');
+                assert.equal(error.statusCode, 422);
+                assert.equal(error.context, 'Request failed with error code 404');
+            }
+        });
+
+        it('should return a ValidationError if upstream 500s', async function () {
+            nock('https://www.youtube.com')
+                .get('/oembed')
+                .query(true)
+                .reply(500);
+
+            try {
+                await oembedService.knownProvider('https://www.youtube.com/watch?v=1234');
+            } catch (error) {
+                assert.equal(error.name, 'ValidationError');
+                assert.equal(error.statusCode, 422);
+                assert.equal(error.context, 'Request failed with error code 500');
+            }
+        });
+    });
+});

ghost/recommendations/src/RecommendationMetadataService.ts

@@ -40,25 +40,31 @@ export class RecommendationMetadataService {
     }
 
     async #fetchJSON(url: URL, options?: {timeout?: number}) {
-        // default content type is application/x-www-form-encoded which is what we need for the webmentions spec
-        const response = await this.#externalRequest.get(url.toString(), {
-            throwHttpErrors: false,
-            maxRedirects: 10,
-            followRedirect: true,
-            timeout: 15000,
-            retry: {
-                // Only retry on network issues, or specific HTTP status codes
-                limit: 3
-            },
-            ...options
-        });
+        // Even though we have throwHttpErrors: false, we still need to catch DNS errors
+        // that can arise from externalRequest, otherwise we'll return a HTTP 500 to the user
+        try {
+            // default content type is application/x-www-form-encoded which is what we need for the webmentions spec
+            const response = await this.#externalRequest.get(url.toString(), {
+                throwHttpErrors: false,
+                maxRedirects: 10,
+                followRedirect: true,
+                timeout: 15000,
+                retry: {
+                    // Only retry on network issues, or specific HTTP status codes
+                    limit: 3
+                },
+                ...options
+            });
 
-        if (response.statusCode >= 200 && response.statusCode < 300) {
-            try {
-                return JSON.parse(response.body);
-            } catch (e) {
-                return undefined;
+            if (response.statusCode >= 200 && response.statusCode < 300) {
+                try {
+                    return JSON.parse(response.body);
+                } catch (e) {
+                    return undefined;
+                }
             }
+        } catch (e) {
+            return undefined;
         }
     }
 

ghost/recommendations/test/RecommendationMetadataService.test.ts

@@ -32,6 +32,7 @@ describe('RecommendationMetadataService', function () {
 
     afterEach(function () {
         nock.cleanAll();
+        sinon.restore();
     });
 
     it('Returns metadata from the Ghost site', async function () {
@@ -181,6 +182,13 @@ describe('RecommendationMetadataService', function () {
         });
     });
 
+    it('does not throw an error even if fetching throws an error', async function () {
+        // TODO: simulate DNS resolution failures if possible
+        sinon.stub(got, 'get').rejects(new Error('Failed to fetch'));
+
+        await service.fetch(new URL('https://exampleghostsite.com/subdirectory'));
+    });
+
     it('Nullifies empty oembed data', async function () {
         nock('https://exampleghostsite.com')
             .get('/subdirectory/members/api/site')