diff --git a/ghost/core/core/frontend/services/theme-engine/i18n/I18n.js b/ghost/core/core/frontend/services/theme-engine/i18n/I18n.js
index 01a0695df987..e369d034ae3b 100644
--- a/ghost/core/core/frontend/services/theme-engine/i18n/I18n.js
+++ b/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,
diff --git a/ghost/core/core/frontend/services/theme-engine/i18n/ThemeI18n.js b/ghost/core/core/frontend/services/theme-engine/i18n/ThemeI18n.js
index a8e66d683ed7..4df8fa1a8543 100644
--- a/ghost/core/core/frontend/services/theme-engine/i18n/ThemeI18n.js
+++ b/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,
diff --git a/ghost/core/core/server/adapters/storage/LocalStorageBase.js b/ghost/core/core/server/adapters/storage/LocalStorageBase.js
index 274247fbfaf3..5fa076b190f8 100644
--- a/ghost/core/core/server/adapters/storage/LocalStorageBase.js
+++ b/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
diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap
index 859e570433be..2bc56cbaf001 100644
--- a/ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap
+++ b/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",
+ },
+ ],
+}
+`;
diff --git a/ghost/core/test/e2e-api/admin/images.test.js b/ghost/core/test/e2e-api/admin/images.test.js
index 1df51c5e7e68..8d6467e7aa1d 100644
--- a/ghost/core/test/e2e-api/admin/images.test.js
+++ b/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);
diff --git a/ghost/core/test/e2e-api/admin/oembed.test.js b/ghost/core/test/e2e-api/admin/oembed.test.js
index dfeed17f11fa..f300d71c910b 100644
--- a/ghost/core/test/e2e-api/admin/oembed.test.js
+++ b/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);
diff --git a/ghost/oembed-service/lib/OEmbedService.js b/ghost/oembed-service/lib/OEmbedService.js
index 738ff6895dc2..7715cb1facb3 100644
--- a/ghost/oembed-service/lib/OEmbedService.js
+++ b/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
+ });
}
}
diff --git a/ghost/oembed-service/test/hello.test.js b/ghost/oembed-service/test/hello.test.js
deleted file mode 100644
index d9b992702844..000000000000
--- a/ghost/oembed-service/test/hello.test.js
+++ /dev/null
@@ -1,7 +0,0 @@
-const assert = require('assert/strict');
-
-describe('Hello world', function () {
- it('Runs a test', function () {
- assert.ok(require('../index'));
- });
-});
diff --git a/ghost/oembed-service/test/oembed-service.test.js b/ghost/oembed-service/test/oembed-service.test.js
new file mode 100644
index 000000000000..1ada49a6fa75
--- /dev/null
+++ b/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: ''
+ });
+
+ 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, '');
+ });
+
+ 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');
+ }
+ });
+ });
+});
diff --git a/ghost/recommendations/src/RecommendationMetadataService.ts b/ghost/recommendations/src/RecommendationMetadataService.ts
index 9d6e5f947c51..5006c584f143 100644
--- a/ghost/recommendations/src/RecommendationMetadataService.ts
+++ b/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;
}
}
diff --git a/ghost/recommendations/test/RecommendationMetadataService.test.ts b/ghost/recommendations/test/RecommendationMetadataService.test.ts
index dd58cbffea26..7d6b1f8f5080 100644
--- a/ghost/recommendations/test/RecommendationMetadataService.test.ts
+++ b/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')