From 60d066b2437497e970e13184a9db8481fdb52eef Mon Sep 17 00:00:00 2001 From: Naz Date: Thu, 10 Nov 2022 17:29:28 +0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Disabled=20editable=20relations?= =?UTF-8?q?=20by=20default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../core/core/server/models/base/bookshelf.js | 1 + ghost/core/core/server/models/integration.js | 8 ++ ghost/core/core/server/models/member.js | 6 ++ ghost/core/core/server/models/post.js | 17 ++++ ghost/core/core/server/models/product.js | 5 + .../admin/__snapshots__/tiers.test.js.snap | 96 +++++++++++++++++++ ghost/core/test/e2e-api/admin/tiers.test.js | 35 +++++-- ghost/core/test/e2e-frontend/members.test.js | 32 +++++++ .../regression/models/model_members.test.js | 32 +++---- ghost/core/test/utils/fixture-utils.js | 2 +- ghost/members-api/lib/repositories/member.js | 6 -- 11 files changed, 210 insertions(+), 30 deletions(-) diff --git a/ghost/core/core/server/models/base/bookshelf.js b/ghost/core/core/server/models/base/bookshelf.js index 9be94e11bb..4d2bcb5c3c 100644 --- a/ghost/core/core/server/models/base/bookshelf.js +++ b/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: { diff --git a/ghost/core/core/server/models/integration.js b/ghost/core/core/server/models/integration.js index f4da47e5d5..545ddb77f0 100644 --- a/ghost/core/core/server/models/integration.js +++ b/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', diff --git a/ghost/core/core/server/models/member.js b/ghost/core/core/server/models/member.js index fefc6108bb..9e2f1671a8 100644 --- a/ghost/core/core/server/models/member.js +++ b/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 } diff --git a/ghost/core/core/server/models/post.js b/ghost/core/core/server/models/post.js index bcccf85ded..8d36d636f9 100644 --- a/ghost/core/core/server/models/post.js +++ b/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: { diff --git a/ghost/core/core/server/models/product.js b/ghost/core/core/server/models/product.js index 23d41409ca..ac2347e2bc 100644 --- a/ghost/core/core/server/models/product.js +++ b/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' diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/tiers.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/tiers.test.js.snap index db20f21921..572c605863 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/tiers.test.js.snap +++ b/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 [ diff --git a/ghost/core/test/e2e-api/admin/tiers.test.js b/ghost/core/test/e2e-api/admin/tiers.test.js index 080887e5a6..307f1221e1 100644 --- a/ghost/core/test/e2e-api/admin/tiers.test.js +++ b/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 + }) + }); }); }); diff --git a/ghost/core/test/e2e-frontend/members.test.js b/ghost/core/test/e2e-frontend/members.test.js index a35c322e21..561aa574cf 100644 --- a/ghost/core/test/e2e-frontend/members.test.js +++ b/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 () { diff --git a/ghost/core/test/regression/models/model_members.test.js b/ghost/core/test/regression/models/model_members.test.js index 2ed3ba153a..ddfd6fe37b 100644 --- a/ghost/core/test/regression/models/model_members.test.js +++ b/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); diff --git a/ghost/core/test/utils/fixture-utils.js b/ghost/core/test/utils/fixture-utils.js index 9a6f35352c..e0029664b5 100644 --- a/ghost/core/test/utils/fixture-utils.js +++ b/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); diff --git a/ghost/members-api/lib/repositories/member.js b/ghost/members-api/lib/repositories/member.js index b3d11c90e7..5c78b4fbf3 100644 --- a/ghost/members-api/lib/repositories/member.js +++ b/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();