Improved restricted field filter validation in API endpoints#26750
Merged
kevinansfield merged 1 commit intomainfrom Mar 10, 2026
Merged
Improved restricted field filter validation in API endpoints#26750kevinansfield merged 1 commit intomainfrom
kevinansfield merged 1 commit intomainfrom
Conversation
The filter field validation was only checking the last segment of dot-separated keys. Updated to check all segments so that compound keys are handled correctly. Also added restricted field filtering to the Admin API users endpoint, and renamed the utility module to better reflect its broader usage across API layers.
Contributor
WalkthroughThis pull request refactors field-level filtering for public and admin API endpoints. It introduces a new utility module 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/legacy/api/content/posts.test.js (1)
104-142: Please add the equivalent nested
ghost/core/core/server/api/endpoints/utils/api-filter-utils.tsnow protects bothpasswordandpasswordpath. A matchingauthors.email.xcase would keep the second restricted field from drifting untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/legacy/api/content/posts.test.js` around lines 104 - 142, Add a new test mirroring "can not filter posts by authors.password.x (3-segment bypass)" that checks the same 3-segment dotted-path bypass for email: insert a test user (reuse userId pattern and create a post-author relation as in the existing test) with a known email value, then perform the API request against posts/?filter=authors.email.x:'<that-email>' (using localUtils.API.getApiQuery and validKey), assert that the response does NOT return the post (throw if data.posts.length === 1), and finally remove the posts_authors and users rows in the same finally cleanup block; use the same test structure and expectations (Content-Type, Cache-Control, expect 200) and mirror the unique identifiers used in the existing test to locate where to add this new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/test/legacy/api/content/posts.test.js`:
- Around line 108-141: The test's fixture setup (inserts into users,
posts_authors and fetching postId) occurs before the try/finally, so if any
setup step throws the finally cleanup never runs and rows leak; move the setup
(the user insert, the posts query/assignment to postId, and the posts_authors
insert) inside the try block that currently wraps the request so that the
finally always executes the cleanup; reference the existing testUtils.knex calls
(users insert, posts.first('id').where('slug','welcome'), posts_authors.insert
with id '644fd18ca1f0b764b0279b2f') and keep the existing finally that deletes
from posts_authors and users.
---
Nitpick comments:
In `@ghost/core/test/legacy/api/content/posts.test.js`:
- Around line 104-142: Add a new test mirroring "can not filter posts by
authors.password.x (3-segment bypass)" that checks the same 3-segment
dotted-path bypass for email: insert a test user (reuse userId pattern and
create a post-author relation as in the existing test) with a known email value,
then perform the API request against
posts/?filter=authors.email.x:'<that-email>' (using localUtils.API.getApiQuery
and validKey), assert that the response does NOT return the post (throw if
data.posts.length === 1), and finally remove the posts_authors and users rows in
the same finally cleanup block; use the same test structure and expectations
(Content-Type, Cache-Control, expect 200) and mirror the unique identifiers used
in the existing test to locate where to add this new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5256d52-1fa7-4ea9-acfb-13bcb84061e2
📒 Files selected for processing (8)
ghost/core/core/server/api/endpoints/authors-public.jsghost/core/core/server/api/endpoints/pages-public.jsghost/core/core/server/api/endpoints/posts-public.jsghost/core/core/server/api/endpoints/users.jsghost/core/core/server/api/endpoints/utils/api-filter-utils.tsghost/core/core/server/api/endpoints/utils/public-endpoint-utils.tsghost/core/test/e2e-api/admin/users.test.jsghost/core/test/legacy/api/content/posts.test.js
💤 Files with no reviewable changes (1)
- ghost/core/core/server/api/endpoints/utils/public-endpoint-utils.ts
Comment on lines
+108
to
+141
| await testUtils.knex('users').insert({ | ||
| id: userId, | ||
| slug: 'brute-force-password-test-user', | ||
| name: 'Brute Force Password Test User', | ||
| email: 'bruteforcepasswordtestuseremail@example.com', | ||
| password: hashedPassword, | ||
| status: 'active', | ||
| created_at: '2019-01-01 00:00:00' | ||
| }); | ||
|
|
||
| const {id: postId} = await testUtils.knex('posts').first('id').where('slug', 'welcome'); | ||
|
|
||
| await testUtils.knex('posts_authors').insert({ | ||
| id: '644fd18ca1f0b764b0279b2f', | ||
| post_id: postId, | ||
| author_id: userId | ||
| }); | ||
|
|
||
| try { | ||
| const res = await request.get(localUtils.API.getApiQuery(`posts/?key=${validKey}&filter=authors.password.x:'${hashedPassword}'`)) | ||
| .set('Origin', testUtils.API.getURL()) | ||
| .expect('Content-Type', /json/) | ||
| .expect('Cache-Control', testUtils.cacheRules.public) | ||
| .expect(200); | ||
|
|
||
| const data = JSON.parse(res.text); | ||
|
|
||
| if (data.posts.length === 1) { | ||
| throw new Error('3-segment key bypass should not return filtered results'); | ||
| } | ||
| } finally { | ||
| await testUtils.knex('posts_authors').where('id', '644fd18ca1f0b764b0279b2f').del(); | ||
| await testUtils.knex('users').where('id', userId).del(); | ||
| } |
Contributor
There was a problem hiding this comment.
Wrap the fixture setup in the try block.
If one of the arrange steps throws after the user insert succeeds, the cleanup never runs and this test can leak rows into later cases.
🧹 Suggested fix
- await testUtils.knex('users').insert({
- id: userId,
- slug: 'brute-force-password-test-user',
- name: 'Brute Force Password Test User',
- email: 'bruteforcepasswordtestuseremail@example.com',
- password: hashedPassword,
- status: 'active',
- created_at: '2019-01-01 00:00:00'
- });
-
- const {id: postId} = await testUtils.knex('posts').first('id').where('slug', 'welcome');
-
- await testUtils.knex('posts_authors').insert({
- id: '644fd18ca1f0b764b0279b2f',
- post_id: postId,
- author_id: userId
- });
-
try {
+ await testUtils.knex('users').insert({
+ id: userId,
+ slug: 'brute-force-password-test-user',
+ name: 'Brute Force Password Test User',
+ email: 'bruteforcepasswordtestuseremail@example.com',
+ password: hashedPassword,
+ status: 'active',
+ created_at: '2019-01-01 00:00:00'
+ });
+
+ const {id: postId} = await testUtils.knex('posts').first('id').where('slug', 'welcome');
+
+ await testUtils.knex('posts_authors').insert({
+ id: '644fd18ca1f0b764b0279b2f',
+ post_id: postId,
+ author_id: userId
+ });
+
const res = await request.get(localUtils.API.getApiQuery(`posts/?key=${validKey}&filter=authors.password.x:'${hashedPassword}'`))
.set('Origin', testUtils.API.getURL())
.expect('Content-Type', /json/)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await testUtils.knex('users').insert({ | |
| id: userId, | |
| slug: 'brute-force-password-test-user', | |
| name: 'Brute Force Password Test User', | |
| email: 'bruteforcepasswordtestuseremail@example.com', | |
| password: hashedPassword, | |
| status: 'active', | |
| created_at: '2019-01-01 00:00:00' | |
| }); | |
| const {id: postId} = await testUtils.knex('posts').first('id').where('slug', 'welcome'); | |
| await testUtils.knex('posts_authors').insert({ | |
| id: '644fd18ca1f0b764b0279b2f', | |
| post_id: postId, | |
| author_id: userId | |
| }); | |
| try { | |
| const res = await request.get(localUtils.API.getApiQuery(`posts/?key=${validKey}&filter=authors.password.x:'${hashedPassword}'`)) | |
| .set('Origin', testUtils.API.getURL()) | |
| .expect('Content-Type', /json/) | |
| .expect('Cache-Control', testUtils.cacheRules.public) | |
| .expect(200); | |
| const data = JSON.parse(res.text); | |
| if (data.posts.length === 1) { | |
| throw new Error('3-segment key bypass should not return filtered results'); | |
| } | |
| } finally { | |
| await testUtils.knex('posts_authors').where('id', '644fd18ca1f0b764b0279b2f').del(); | |
| await testUtils.knex('users').where('id', userId).del(); | |
| } | |
| try { | |
| await testUtils.knex('users').insert({ | |
| id: userId, | |
| slug: 'brute-force-password-test-user', | |
| name: 'Brute Force Password Test User', | |
| email: 'bruteforcepasswordtestuseremail@example.com', | |
| password: hashedPassword, | |
| status: 'active', | |
| created_at: '2019-01-01 00:00:00' | |
| }); | |
| const {id: postId} = await testUtils.knex('posts').first('id').where('slug', 'welcome'); | |
| await testUtils.knex('posts_authors').insert({ | |
| id: '644fd18ca1f0b764b0279b2f', | |
| post_id: postId, | |
| author_id: userId | |
| }); | |
| const res = await request.get(localUtils.API.getApiQuery(`posts/?key=${validKey}&filter=authors.password.x:'${hashedPassword}'`)) | |
| .set('Origin', testUtils.API.getURL()) | |
| .expect('Content-Type', /json/) | |
| .expect('Cache-Control', testUtils.cacheRules.public) | |
| .expect(200); | |
| const data = JSON.parse(res.text); | |
| if (data.posts.length === 1) { | |
| throw new Error('3-segment key bypass should not return filtered results'); | |
| } | |
| } finally { | |
| await testUtils.knex('posts_authors').where('id', '644fd18ca1f0b764b0279b2f').del(); | |
| await testUtils.knex('users').where('id', userId).del(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/test/legacy/api/content/posts.test.js` around lines 108 - 141, The
test's fixture setup (inserts into users, posts_authors and fetching postId)
occurs before the try/finally, so if any setup step throws the finally cleanup
never runs and rows leak; move the setup (the user insert, the posts
query/assignment to postId, and the posts_authors insert) inside the try block
that currently wraps the request so that the finally always executes the
cleanup; reference the existing testUtils.knex calls (users insert,
posts.first('id').where('slug','welcome'), posts_authors.insert with id
'644fd18ca1f0b764b0279b2f') and keep the existing finally that deletes from
posts_authors and users.
peterzimon
pushed a commit
that referenced
this pull request
Mar 10, 2026
ref https://linear.app/ghost/issue/ONC-1525/ The filter field validation was only checking the last segment of dot-separated keys. Updated to check all segments so that compound keys are handled correctly. Also added restricted field filtering to the Admin API users endpoint, and renamed the utility module to better reflect its broader usage across API layers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The filter field validation was only checking the last segment of
dot-separated keys. Updated to check all segments so that compound
keys are handled correctly. Also added restricted field filtering
to the Admin API users endpoint, and renamed the utility module
to better reflect its broader usage across API layers.