From 4c35e007212ae538f37c5debc419685bb1420e32 Mon Sep 17 00:00:00 2001
From: Daniel Lockyer <hi@daniellockyer.com>
Date: Mon, 6 May 2024 11:44:53 +0200
Subject: [PATCH] Fixed handling of invalid `Accept-Version` header

fix https://linear.app/tryghost/issue/SLO-96/invalid-version-must-be-a-string-got-type-object-an-unexpected-error

- in the event that a non-semver Accept-Version header is given, the
  current code will throw an error because the semver lib can't compare null
  against a valid version
- the error in question is `Must be a string. Got type "object"`
- to fix this, we can just detect a null and early return with a
  BadRequestError
- also adds a breaking test
---
 .../mw-error-handler/lib/mw-error-handler.js  | 81 ++++++++++---------
 .../test/mw-error-handler.test.js             | 20 +++++
 2 files changed, 65 insertions(+), 36 deletions(-)

diff --git a/ghost/mw-error-handler/lib/mw-error-handler.js b/ghost/mw-error-handler/lib/mw-error-handler.js
index 22dfb631df..d606282ec8 100644
--- a/ghost/mw-error-handler/lib/mw-error-handler.js
+++ b/ghost/mw-error-handler/lib/mw-error-handler.js
@@ -21,6 +21,7 @@ const messages = {
         context: 'Provided client accept-version {acceptVersion} is behind current Ghost version {ghostVersion}.',
         help: 'Try upgrading your Ghost API client.'
     },
+    badVersion: 'Requested version is not supported.',
     actions: {
         images: {
             upload: 'upload image'
@@ -225,45 +226,53 @@ const prepareUserMessage = function prepareUserMessage(err, req) {
 };
 
 module.exports.resourceNotFound = function resourceNotFound(req, res, next) {
-    if (req && req.headers && req.headers['accept-version']
-        && res.locals && res.locals.safeVersion
-        && semver.compare(semver.coerce(req.headers['accept-version']), semver.coerce(res.locals.safeVersion)) !== 0) {
-        const versionComparison = semver.compare(
-            semver.coerce(req.headers['accept-version']),
-            semver.coerce(res.locals.safeVersion)
-        );
-
-        let notAcceptableError;
-        if (versionComparison === 1) {
-            notAcceptableError = new errors.RequestNotAcceptableError({
-                message: tpl(
-                    messages.methodNotAcceptableVersionAhead.message
-                ),
-                context: tpl(messages.methodNotAcceptableVersionAhead.context, {
-                    acceptVersion: req.headers['accept-version'],
-                    ghostVersion: `v${res.locals.safeVersion}`
-                }),
-                help: tpl(messages.methodNotAcceptableVersionAhead.help),
-                code: 'UPDATE_GHOST'
-            });
-        } else {
-            notAcceptableError = new errors.RequestNotAcceptableError({
-                message: tpl(
-                    messages.methodNotAcceptableVersionBehind.message
-                ),
-                context: tpl(messages.methodNotAcceptableVersionBehind.context, {
-                    acceptVersion: req.headers['accept-version'],
-                    ghostVersion: `v${res.locals.safeVersion}`
-                }),
-                help: tpl(messages.methodNotAcceptableVersionBehind.help),
-                code: 'UPDATE_CLIENT'
-            });
+    if (req?.headers?.['accept-version'] && res.locals?.safeVersion) {
+        // Protect against invalid `Accept-Version` headers
+        const acceptVersionSemver = semver.coerce(req.headers['accept-version']);
+        if (!acceptVersionSemver) {
+            return next(new errors.BadRequestError({
+                message: tpl(messages.badVersion)
+            }));
         }
 
-        next(notAcceptableError);
-    } else {
-        next(new errors.NotFoundError({message: tpl(messages.resourceNotFound)}));
+        if (semver.compare(acceptVersionSemver, semver.coerce(res.locals.safeVersion)) !== 0) {
+            const versionComparison = semver.compare(
+                acceptVersionSemver,
+                semver.coerce(res.locals.safeVersion)
+            );
+
+            let notAcceptableError;
+            if (versionComparison === 1) {
+                notAcceptableError = new errors.RequestNotAcceptableError({
+                    message: tpl(
+                        messages.methodNotAcceptableVersionAhead.message
+                    ),
+                    context: tpl(messages.methodNotAcceptableVersionAhead.context, {
+                        acceptVersion: req.headers['accept-version'],
+                        ghostVersion: `v${res.locals.safeVersion}`
+                    }),
+                    help: tpl(messages.methodNotAcceptableVersionAhead.help),
+                    code: 'UPDATE_GHOST'
+                });
+            } else {
+                notAcceptableError = new errors.RequestNotAcceptableError({
+                    message: tpl(
+                        messages.methodNotAcceptableVersionBehind.message
+                    ),
+                    context: tpl(messages.methodNotAcceptableVersionBehind.context, {
+                        acceptVersion: req.headers['accept-version'],
+                        ghostVersion: `v${res.locals.safeVersion}`
+                    }),
+                    help: tpl(messages.methodNotAcceptableVersionBehind.help),
+                    code: 'UPDATE_CLIENT'
+                });
+            }
+
+            return next(notAcceptableError);
+        }
     }
+
+    next(new errors.NotFoundError({message: tpl(messages.resourceNotFound)}));
 };
 
 module.exports.pageNotFound = function pageNotFound(req, res, next) {
diff --git a/ghost/mw-error-handler/test/mw-error-handler.test.js b/ghost/mw-error-handler/test/mw-error-handler.test.js
index 8a64e347a0..25e42c0d26 100644
--- a/ghost/mw-error-handler/test/mw-error-handler.test.js
+++ b/ghost/mw-error-handler/test/mw-error-handler.test.js
@@ -285,6 +285,26 @@ describe('Resource Not Found', function () {
         });
     });
 
+    it('Returns 406 Request Not Acceptable Error for invalid version', function (done) {
+        const req = {
+            headers: {
+                'accept-version': 'foo'
+            }
+        };
+
+        const res = {
+            locals: {
+                safeVersion: '4.3'
+            }
+        };
+
+        resourceNotFound(req, res, (error) => {
+            assert.equal(error.statusCode, 400);
+            assert.equal(error.message, 'Requested version is not supported.');
+            done();
+        });
+    });
+
     it('Returns 406 Request Not Acceptable Error for when requested version is behind current version', function (done) {
         const req = {
             headers: {