diff --git a/ghost/collections/src/Collection.ts b/ghost/collections/src/Collection.ts index 155dda09b1..7aff50cd3c 100644 --- a/ghost/collections/src/Collection.ts +++ b/ghost/collections/src/Collection.ts @@ -18,6 +18,51 @@ const messages = { slugMustBeUnique: 'Slug must be unique' }; +function validateFilter(filter: string | null, type: 'manual' | 'automatic', isAllowedEmpty = false) { + const allowedProperties = ['featured', 'published_at', 'tag', 'tags'] + if (type === 'manual') { + if (filter !== null) { + throw new ValidationError({ + message: tpl(messages.invalidFilterProvided.message), + context: tpl(messages.invalidFilterProvided.context) + }); + } + return; + } + + // type === 'automatic' now + if (filter === null) { + throw new ValidationError({ + message: tpl(messages.invalidFilterProvided.message), + context: tpl(messages.invalidFilterProvided.context) + }); + } + + if (filter.trim() === '' && !isAllowedEmpty) { + throw new ValidationError({ + message: tpl(messages.invalidFilterProvided.message), + context: tpl(messages.invalidFilterProvided.context) + }); + } + + try { + const parsedFilter = nql(filter); + const keysUsed: string[] = []; + nql.utils.mapQuery(parsedFilter.toJSON(), function (value: unknown, key: string) { + keysUsed.push(key); + }); + if (keysUsed.some(key => !allowedProperties.includes(key))) { + throw new ValidationError({ + message: tpl(messages.invalidFilterProvided.message) + }); + } + } catch (err) { + throw new ValidationError({ + message: tpl(messages.invalidFilterProvided.message) + }); + } +} + export class Collection { id: string; title: string; @@ -45,25 +90,11 @@ export class Collection { return this._filter; } set filter(value) { - // Cannot change the filter of these collections if (this.slug === 'latest' || this.slug === 'featured') { return; } - if (this.type === 'manual') { - if (value !== null) { - throw new ValidationError({ - message: tpl(messages.invalidFilterProvided.message), - context: tpl(messages.invalidFilterProvided.context) - }); - } - } else { - if (value === null || value === '') { - throw new ValidationError({ - message: tpl(messages.invalidFilterProvided.message), - context: tpl(messages.invalidFilterProvided.context) - }); - } - } + validateFilter(value, this.type); + this._filter = value; } featureImage: string | null; createdAt: Date; @@ -195,13 +226,9 @@ export class Collection { }); } - if (data.type === 'automatic' && (data.slug !== 'latest') && !data.filter) { - // @NOTE: add filter validation here - throw new ValidationError({ - message: tpl(messages.invalidFilterProvided.message), - context: tpl(messages.invalidFilterProvided.context) - }); - } + const type = data.type === 'automatic' ? 'automatic' : 'manual'; + const filter = typeof data.filter === 'string' ? data.filter : null; + validateFilter(filter, type, data.slug === 'latest'); if (!data.title) { throw new ValidationError({ @@ -214,8 +241,8 @@ export class Collection { title: data.title, slug: data.slug, description: data.description || null, - type: data.type || 'manual', - filter: data.filter || null, + type: type, + filter: filter, featureImage: data.feature_image || null, createdAt: Collection.validateDateField(data.created_at, 'created_at'), updatedAt: Collection.validateDateField(data.updated_at, 'updated_at'), diff --git a/ghost/collections/test/Collection.test.ts b/ghost/collections/test/Collection.test.ts index 053064babb..29ac2ee920 100644 --- a/ghost/collections/test/Collection.test.ts +++ b/ghost/collections/test/Collection.test.ts @@ -189,6 +189,43 @@ describe('Collection', function () { assert.equal(collection.slug, 'edited-slug'); }); + it('Throws when the collection filter is malformed', async function () { + const collection = await Collection.create({ + title: 'Testing edits', + type: 'automatic', + filter: 'featured:true' + }); + + assert.throws(() => { + collection.filter = 'my name is, my name is, my name is, wicka wicka slim shady'; + }, (err: any) => { + assert.equal(err.message, 'Invalid filter provided for automatic Collection', 'Error message should match'); + return true; + }); + }); + + it('Throws when the collection filter is invalid', async function () { + assert.rejects(async () => { + await Collection.create({ + title: 'Testing creating collections with invalid filter', + type: 'automatic', + filter: 'unknown:egg' + }); + }); + const collection = await Collection.create({ + title: 'Testing edits', + type: 'automatic', + filter: 'featured:true' + }); + + assert.throws(() => { + collection.filter = 'unknown:true'; + }, (err: any) => { + assert.equal(err.message, 'Invalid filter provided for automatic Collection', 'Error message should match'); + return true; + }); + }); + it('Throws when the collection filter is empty', async function () { const collection = await Collection.create({ title: 'Testing edits', @@ -216,6 +253,32 @@ describe('Collection', function () { collection.filter = ''; }); + it('throws when trying to set an empty filter on an automatic collection', async function () { + assert.rejects(async () => { + await Collection.create({ + title: 'Testing Creating Automatic With Empty Filter', + slug: 'testing-creating-automatic-with-empty-filter', + type: 'automatic', + filter: '' + }); + }); + + const collection = await Collection.create({ + title: 'Testing Editing Automatic With Empty Filter', + slug: 'testing-editing-automatic-with-empty-filter', + type: 'automatic', + filter: 'featured:true' + }); + + assert.throws(() => { + collection.filter = ''; + }, (err: any) => { + assert.equal(err.message, 'Invalid filter provided for automatic Collection', 'Error message should match'); + assert.equal(err.context, 'Automatic type of collection should always have a filter value', 'Error message should match'); + return true; + }); + }); + it('throws when trying to set filter on a manual collection', async function () { const collection = await Collection.create({ title: 'Testing Manual Filter', diff --git a/ghost/collections/test/collections.test.ts b/ghost/collections/test/collections.test.ts index e608a4edd6..9c2e664d0d 100644 --- a/ghost/collections/test/collections.test.ts +++ b/ghost/collections/test/collections.test.ts @@ -276,7 +276,7 @@ describe('CollectionsService', function () { let updatedCollection = await collectionsService.edit({ id: collection.id, - filter: 'id:post-3-featured' + filter: 'featured:true+published_at:>2023-05-20' }); assert.equal(updatedCollection?.posts.length, 1, 'Collection should have one post'); diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/collections.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/collections.test.js.snap index 591a3a9804..9cd182cc8a 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/collections.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/collections.test.js.snap @@ -1262,7 +1262,7 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, "description": "All posts", "feature_image": null, - "filter": null, + "filter": "", "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, "slug": "latest", "title": "Latest", @@ -1298,7 +1298,7 @@ exports[`Collections API Browse Can browse Collections 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": "578", + "content-length": "576", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -1317,7 +1317,7 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, "description": "All posts", "feature_image": null, - "filter": null, + "filter": "", "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, "slug": "latest", "title": "Latest", @@ -1356,7 +1356,7 @@ exports[`Collections API Browse Can browse Collections and include the posts cou 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": "619", + "content-length": "617", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snap index bc961df4ea..c0fca1e378 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snap @@ -1587,7 +1587,7 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.\\\\d\\{3\\}Z/, "description": "All posts", "feature_image": null, - "filter": null, + "filter": "", "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, "posts": Array [ Object { @@ -1758,7 +1758,7 @@ exports[`Posts API Update Can add and remove collections 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": "5209", + "content-length": "5207", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -1798,7 +1798,7 @@ Object { "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.\\\\d\\{3\\}Z/, "description": "All posts", "feature_image": null, - "filter": null, + "filter": "", "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, "posts": Array [ Object { @@ -1969,7 +1969,7 @@ exports[`Posts API Update Can add and remove collections 6: [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": "5203", + "content-length": "5201", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,