From 3818445a1819afafb293145edacc8e42fc91939a Mon Sep 17 00:00:00 2001
From: Ronald Langeveld <hi@ronaldlangeveld.com>
Date: Mon, 8 Jul 2024 16:45:20 +0700
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20bad=20redirects=20yaml?=
 =?UTF-8?q?=20overriding=20backed=20up=20working=20yaml=20file=20(#20555)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

ref ENG-945

- Fixed an issue where upload a broken redirects yaml will override the
last working yaml.
- Instead it will now do the validation before saving and overriding the
yaml.
---
 .../custom-redirects/CustomRedirectsAPI.js    |  7 ++-
 .../services/custom-redirects/api.test.js     | 43 +++++++++++++++++++
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/ghost/core/core/server/services/custom-redirects/CustomRedirectsAPI.js b/ghost/core/core/server/services/custom-redirects/CustomRedirectsAPI.js
index 54a54c6fdd..3c46b7de9c 100644
--- a/ghost/core/core/server/services/custom-redirects/CustomRedirectsAPI.js
+++ b/ghost/core/core/server/services/custom-redirects/CustomRedirectsAPI.js
@@ -211,6 +211,9 @@ class CustomRedirectsAPI {
      */
     async setFromFilePath(filePath, ext = '.json') {
         const redirectsFilePath = await this.getRedirectsFilePath();
+        const content = await readRedirectsFile(filePath);
+        const parsed = parseRedirectsFile(content, ext);
+        this.validate(parsed);
 
         if (redirectsFilePath) {
             const backupRedirectsPath = this.getBackupFilePath(redirectsFilePath);
@@ -223,10 +226,6 @@ class CustomRedirectsAPI {
             await fs.move(redirectsFilePath, backupRedirectsPath);
         }
 
-        const content = await readRedirectsFile(filePath);
-        const parsed = parseRedirectsFile(content, ext);
-        this.validate(parsed);
-
         if (ext === '.json') {
             await fs.writeFile(this.createRedirectsFilePath('.json'), JSON.stringify(parsed), 'utf-8');
         } else if (ext === '.yaml') {
diff --git a/ghost/core/test/unit/server/services/custom-redirects/api.test.js b/ghost/core/test/unit/server/services/custom-redirects/api.test.js
index d7836996cf..3913e8e805 100644
--- a/ghost/core/test/unit/server/services/custom-redirects/api.test.js
+++ b/ghost/core/test/unit/server/services/custom-redirects/api.test.js
@@ -259,5 +259,48 @@ describe('UNIT: redirects CustomRedirectsAPI class', function () {
             // two redirects in total
             redirectManager.addRedirect.calledTwice.should.be.true();
         });
+
+        it('does not create a backup file from a bad redirect yaml file', async function () {
+            const incomingFilePath = path.join(__dirname, '/invalid/path/redirects_incoming.yaml');
+            const backupFilePath = path.join(basePath, 'backup.yaml');
+
+            const invalidYaml = `
+                301:
+                    /my-old-blog-post/: /revamped-url/
+                    /my-old-blog-post/: /revamped-url/
+
+                302:
+                    /another-old-blog-post/: /hello-there/
+            `;
+
+            // redirects.json file already exits
+            fs.pathExists.withArgs(`${basePath}redirects.json`).resolves(false);
+            fs.pathExists.withArgs(`${basePath}redirects.yaml`).resolves(true);
+            // incoming redirects file
+            fs.readFile.withArgs(incomingFilePath, 'utf-8').resolves(invalidYaml);
+            // backup file DOES not exists yet
+            fs.pathExists.withArgs(backupFilePath).resolves(false);
+            // should not be called
+            fs.unlink.withArgs(backupFilePath).resolves(false);
+            fs.move.withArgs(`${basePath}redirects.yaml`, backupFilePath).resolves(true);
+
+            customRedirectsAPI = new CustomRedirectsAPI({
+                basePath,
+                redirectManager,
+                getBackupFilePath: () => backupFilePath,
+                validate: () => {}
+            });
+
+            try {
+                await customRedirectsAPI.setFromFilePath(incomingFilePath, '.yaml');
+                should.fail('setFromFilePath did not throw');
+            } catch (err) {
+                should.exist(err);
+                err.errorType.should.eql('BadRequestError');
+            }
+
+            fs.unlink.called.should.not.be.true();
+            fs.move.called.should.not.be.true();
+        });
     });
 });