🔒 Disabled editable relations by default

refs https://github.com/TryGhost/Ghost/security/advisories/GHSA-9gh8-wp53-ccc6
refs https://github.com/TryGhost/Toolbox/issues/465

- Bookshelf relations allows us to edit relational records by default, which was used liberally in the codebase.
- Not having a clear track record of editable relations left the model layer prone to triggering unwanted nested saves and created a vulnerability where members were able to edit newsletter settings.
- With explicit editable relations it's easier to keep track of relations having editable access to related records. Makes the relational data modification pattern safer to use too.
- Anyone running 5.x should update to 5.24.1

Credits: Dave McDaniel and other members of [Cisco Talos](https://talosintelligence.com/vulnerability_reports)

ghost/core/core/server/models/base/bookshelf.js

@@ -66,6 +66,7 @@ ghostBookshelf.plugin(require('./plugins/relations'));
 ghostBookshelf.plugin('bookshelf-relations', {
     allowedOptions: ['context', 'importing', 'migrating'],
     unsetRelations: true,
+    editRelations: false,
     extendChanged: '_changed',
     attachPreviousRelations: true,
     hooks: {

ghost/core/core/server/models/integration.js

@@ -10,6 +10,14 @@ const Integration = ghostBookshelf.Model.extend({
     actionsResourceType: 'integration',
 
     relationships: ['api_keys', 'webhooks'],
+    relationshipConfig: {
+        api_keys: {
+            editable: true
+        },
+        webhooks: {
+            editable: true
+        }
+    },
 
     relationshipBelongsTo: {
         api_keys: 'api_keys',

ghost/core/core/server/models/member.js

@@ -128,6 +128,12 @@ const Member = ghostBookshelf.Model.extend({
     // do not delete email_recipients records when a member is destroyed. Recipient
     // records are used for analytics and historical records
     relationshipConfig: {
+        products: {
+            editable: true
+        },
+        labels: {
+            editable: true
+        },
         email_recipients: {
             destroyRelated: false
         }

ghost/core/core/server/models/post.js

@@ -97,6 +97,23 @@ Post = ghostBookshelf.Model.extend({
     },
 
     relationships: ['tags', 'authors', 'mobiledoc_revisions', 'post_revisions', 'posts_meta', 'tiers'],
+    relationshipConfig: {
+        tags: {
+            editable: true
+        },
+        authors: {
+            editable: true
+        },
+        mobiledoc_revisions: {
+            editable: true
+        },
+        post_revisions: {
+            editable: true
+        },
+        posts_meta: {
+            editable: true
+        }
+    },
 
     // NOTE: look up object, not super nice, but was easy to implement
     relationshipBelongsTo: {

ghost/core/core/server/models/product.js

@@ -14,6 +14,11 @@ const Product = ghostBookshelf.Model.extend({
     },
 
     relationships: ['benefits'],
+    relationshipConfig: {
+        benefits: {
+            editable: true
+        }
+    },
 
     relationshipBelongsTo: {
         benefits: 'benefits'

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

@@ -48,6 +48,102 @@ Object {
 }
 `;
 
+exports[`Tiers API Can edit tier properties and relations 1: [body] 1`] = `
+Object {
+  "meta": Object {
+    "pagination": Object {
+      "limit": 2,
+      "next": null,
+      "page": 1,
+      "pages": 1,
+      "prev": null,
+      "total": 2,
+    },
+  },
+  "tiers": Array [
+    Object {
+      "active": true,
+      "benefits": Array [],
+      "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
+      "description": null,
+      "id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
+      "name": "Free",
+      "slug": "free",
+      "trial_days": 0,
+      "type": "free",
+      "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
+      "visibility": "none",
+      "welcome_page_url": "/welcome-free",
+    },
+    Object {
+      "active": true,
+      "benefits": Array [],
+      "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
+      "currency": "USD",
+      "description": null,
+      "id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
+      "monthly_price": 500,
+      "name": "Default Product",
+      "slug": "default-product",
+      "trial_days": 0,
+      "type": "paid",
+      "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
+      "visibility": "public",
+      "welcome_page_url": "/welcome-paid",
+      "yearly_price": 5000,
+    },
+  ],
+}
+`;
+
+exports[`Tiers API Can edit tier properties and relations 2: [headers] 1`] = `
+Object {
+  "access-control-allow-origin": "http://127.0.0.1:2369",
+  "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0",
+  "content-length": "725",
+  "content-type": "application/json; charset=utf-8",
+  "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
+  "vary": "Accept-Version, Origin, Accept-Encoding",
+  "x-powered-by": "Express",
+}
+`;
+
+exports[`Tiers API Can edit tier properties and relations 3: [body] 1`] = `
+Object {
+  "tiers": Array [
+    Object {
+      "active": true,
+      "benefits": Array [
+        "daily cat pictures",
+        "delicious avo toast",
+      ],
+      "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
+      "description": "Updated description",
+      "id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
+      "name": "Free",
+      "slug": "free",
+      "trial_days": 0,
+      "type": "free",
+      "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
+      "visibility": "none",
+      "welcome_page_url": "/welcome-free",
+    },
+  ],
+}
+`;
+
+exports[`Tiers API Can edit tier properties and relations 4: [headers] 1`] = `
+Object {
+  "access-control-allow-origin": "http://127.0.0.1:2369",
+  "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0",
+  "content-length": "343",
+  "content-type": "application/json; charset=utf-8",
+  "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
+  "vary": "Accept-Version, Origin, Accept-Encoding",
+  "x-powered-by": "Express",
+}
+`;
+
 exports[`Tiers API Errors when price is negative 1: [body] 1`] = `
 Object {
   "errors": Array [

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

@@ -5,6 +5,7 @@ const {
     mockManager,
     matchers
 } = require('../../utils/e2e-framework');
+const {anyEtag} = matchers;
 
 describe('Tiers API', function () {
     let agent;
@@ -127,19 +128,41 @@ describe('Tiers API', function () {
         assert(updatedTier.trial_days === 0, `The trial_days should have been set to 0`);
     });
 
-    it('Can edit description', async function () {
-        const {body: {tiers: [tier]}} = await agent.get('/tiers/?type:paid&limit=1');
+    it('Can edit tier properties and relations', async function () {
+        let {body: {tiers: [tier]}} = await agent.get('/tiers/?type:paid&limit=1')
+            .expectStatus(200)
+            .matchHeaderSnapshot({
+                etag: anyEtag
+            })
+            .matchBodySnapshot({
+                // @NOTE: bug here, the returned array of tiers should be '1' NOT '2' 
+                tiers: Array(2).fill({
+                    id: matchers.anyObjectId,
+                    created_at: matchers.anyISODateTime,
+                    updated_at: matchers.anyISODateTime
+                })
+            });
 
         await agent.put(`/tiers/${tier.id}/`)
             .body({
                 tiers: [{
-                    description: 'Updated description'
+                    description: 'Updated description',
+                    benefits: ['daily cat pictures', 'delicious avo toast']
                 }]
             })
             .expectStatus(200);
 
-        const {body: {tiers: [updatedTier]}} = await agent.get(`/tiers/${tier.id}/`);
-
-        assert.strictEqual('Updated description', updatedTier.description);
+        const {body: {tiers: [updatedTier]}} = await agent.get(`/tiers/${tier.id}/`)
+            .expectStatus(200)
+            .matchHeaderSnapshot({
+                etag: anyEtag
+            })
+            .matchBodySnapshot({
+                tiers: Array(1).fill({
+                    id: matchers.anyObjectId,
+                    created_at: matchers.anyISODateTime,
+                    updated_at: matchers.anyISODateTime
+                })
+            });
     });
 });

ghost/core/test/e2e-frontend/members.test.js

@@ -188,6 +188,8 @@ describe('Front-end members behavior', function () {
 
             // Can update newsletter subscription
             const originalNewsletters = getJsonResponse.newsletters;
+            const originalNewsletterName = originalNewsletters[0].name;
+            originalNewsletters[0].name = 'cannot change me';
 
             const res = await request.put(`/members/api/member/newsletters?uuid=${memberUUID}`)
                 .send({
@@ -212,6 +214,36 @@ describe('Front-end members behavior', function () {
             restoreJsonResponse.should.have.properties(['email', 'uuid', 'status', 'name', 'newsletters']);
             restoreJsonResponse.should.not.have.property('id');
             restoreJsonResponse.newsletters.should.have.length(1);
+            // @NOTE: this seems like too much exposed information, needs a review
+            restoreJsonResponse.newsletters[0].should.have.properties([
+                'id',
+                'uuid',
+                'name',
+                'description',
+                'feedback_enabled',
+                'slug',
+                'sender_name',
+                'sender_email',
+                'sender_reply_to',
+                'status',
+                'visibility',
+                'subscribe_on_signup',
+                'sort_order',
+                'header_image',
+                'show_header_icon',
+                'show_header_title',
+                'title_font_category',
+                'title_alignment',
+                'show_feature_image',
+                'body_font_category',
+                'footer_content',
+                'show_badge',
+                'show_header_name',
+                'created_at',
+                'updated_at'
+            ]);
+
+            should.equal(restoreJsonResponse.newsletters[0].name, originalNewsletterName);
         });
 
         it('should serve theme 404 on members endpoint', async function () {

ghost/core/test/regression/models/model_members.test.js

@@ -112,11 +112,13 @@ describe('Member Model', function run() {
     describe('stripeCustomers', function () {
         it('Is correctly mapped to the stripe customers', async function () {
             const context = testUtils.context.admin;
-            await Member.add({
-                email: 'test@test.member',
-                stripeCustomers: [{
-                    customer_id: 'fake_customer_id1'
-                }]
+            const testMember = await Member.add({
+                email: 'test@test.member'
+            }, context);
+
+            await MemberStripeCustomer.add({
+                member_id: testMember.id,
+                customer_id: 'fake_customer_id1'
             }, context);
 
             const customer1 = await MemberStripeCustomer.findOne({
@@ -270,7 +272,7 @@ describe('Member Model', function run() {
     });
 
     describe('products', function () {
-        it('Products can be created & added to members by the product array', async function () {
+        it('Products can be added to members by the product array', async function () {
             const context = testUtils.context.admin;
             const product = await Product.add({
                 name: 'Product-Add-Test',
@@ -280,9 +282,6 @@ describe('Member Model', function run() {
                 email: 'testing-products@test.member',
                 products: [{
                     id: product.id
-                }, {
-                    name: 'Product-Create-Test',
-                    type: 'paid'
                 }]
             }, {
                 ...context,
@@ -290,17 +289,13 @@ describe('Member Model', function run() {
             });
 
             const createdProduct = await Product.findOne({
-                name: 'Product-Create-Test'
+                name: 'Product-Add-Test'
             }, context);
 
             should.exist(createdProduct, 'Product should have been created');
 
             const products = member.related('products').toJSON();
 
-            should.exist(
-                products.find(model => model.name === 'Product-Create-Test')
-            );
-
             should.exist(
                 products.find(model => model.name === 'Product-Add-Test')
             );
@@ -311,12 +306,15 @@ describe('Member Model', function run() {
         it('Should allow filtering on products', async function () {
             const context = testUtils.context.admin;
 
+            const vipProduct = await Product.add({
+                name: 'VIP',
+                slug: 'vip',
+                type: 'paid'
+            });
             await Member.add({
                 email: 'filter-test@test.member',
                 products: [{
-                    name: 'VIP',
-                    slug: 'vip',
-                    type: 'paid'
+                    id: vipProduct.id
                 }]
             }, context);
 

ghost/core/test/utils/fixture-utils.js

@@ -545,7 +545,7 @@ const fixtures = {
 
                     // TODO: replace with full member/product associations
                     if (member.email === 'with-product@test.com') {
-                        member.products = [{slug: product.get('slug')}];
+                        member.products = [{id: product.id}];
                     }
 
                     return models.Member.add(member, context.internal);

ghost/members-api/lib/repositories/member.js

@@ -408,12 +408,6 @@ module.exports = class MemberRepository {
             'expertise'
         ]);
 
-        if (data.newsletters) {
-            data.newsletters = data.newsletters.map(newsletter => ({
-                id: newsletter.id
-            }));
-        }
-
         // Trim whitespaces from expertise
         if (memberData.expertise) {
             memberData.expertise = memberData.expertise.trim();