Skip to content

Removed the permission to read member custom field definitions - #30497

Merged
rob-ghost merged 1 commit into
chore/metafields-terminology-vocabularyfrom
chore/metafields-open-definition-reads
Sep 3, 2026
Merged

Removed the permission to read member custom field definitions#30497
rob-ghost merged 1 commit into
chore/metafields-terminology-vocabularyfrom
chore/metafields-open-definition-reads

Conversation

@rob-ghost

@rob-ghost rob-ghost commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

ref https://linear.app/ghost/issue/BER-3922/rename-the-code-behind-the-metafields-api-to-match-the-domain-language

Background

Ghost lets a publisher define extra fields on their member records — a shoe size, a company name. A definition records that the site collects a shoe size; a value is what one member entered for it. Definitions are grouped under a namespace naming whoever owns them. The publisher's namespace is custom, the only one today; others are intended for installed apps.

Ghost guards these with a permission called member_custom_field, carrying five actions: browse, read, add, edit and destroy. Administrators and Admin Integrations hold all five. The Super Editor role holds browse and read, and nothing else.

Problem 1: the read permission protects nothing, and is inverted

A definition records only that the site collects a shoe size. Every signed-in member is already shown the complete list of them through Portal, Ghost's member-facing interface. So withholding that list from staff keeps back nothing that is not already on offer elsewhere.

It is also the wrong way round. An integration holding permission to browse members receives those members' field values on the member payload — that code path never consults this permission. The same integration asking for the definitions that describe those values is refused. It can read the data but not the schema.

The HTTP routes already draw the correct line, and do so deliberately: reads are not behind the feature flag, so Admin can ask any site for the list and get an empty one back, while everything that changes something is behind it. Only the permission layer had not caught up.

Problem 2: two checks run in the wrong order

Defining a field requires two things to be true: fields must be definable in the namespace named in the URL, and the caller must be allowed to define them. Ghost asked the second question first.

So a request naming a namespace that nobody can define fields in was refused for want of a permission. That answer sends the caller off to acquire a permission that nobody holds and that would not have helped them, when the real answer is that the namespace was never writable by anyone.

Solution

Reads need no permission. The browse and read handlers stop checking one. The two permission records are removed from Ghost's fixtures, along with the Super Editor grant, which consisted of nothing else. A migration removes both from sites that already have them. What remains on the permission is defining a field, which stays with the publisher.

The namespace settles before the caller. That check moves out of the definitions service into a module of its own, and the endpoint runs it first. A namespace nobody can define in is now refused on that ground, naming the namespace, whatever role the caller holds.

Consequence worth noting

Staff reads widen from Super Editor and above to any authenticated staff member. This is intended: a Contributor can now see a list that any signed-in member could already see.

Why the permission keeps its name

It governs who may define fields in the publisher's namespace, and it will only ever govern that. Other namespaces are to be owned by installed apps, and an app managing its own fields does not hold a staff role — it has to be that app, which is an ownership check rather than a permission check. Naming this permission after metafields generally would claim an authority it is never going to have.

The new namespace module is where that ownership check will live.

Verification

The migration replay test passes against both MySQL and SQLite, along with the metafields suites and the fixture and schema integrity checks. Three existing tests changed to match the new behaviour, and one was added pinning the new ordering: a role without the define permission, naming a namespace nobody owns, is now told about the namespace rather than about a permission.

Where this sits

#30492 shared field-type package
#30494 database tables and columns
#30495 server-side service and its types
this PR permission to read and define fields

Last of four. Stacked on #30495, on #30494, on #30492 — review those first. Merging this one brings the whole stack in.

https://claude.ai/code/session_01XFCbqgYHYhd9rZ5WXtqZyT

ref https://linear.app/ghost/issue/BER-3922/rename-the-code-behind-the-metafields-api-to-match-the-domain-language

Reading a field definition is no longer gated by a permission. A definition says
only that the site collects a shoe size, and every signed-in member is already
shown the whole list through Portal, so holding it back from staff protected
nothing that was not already on offer elsewhere.

It was also inverted. An integration holding member browse received members'
field values on the member payload, which never consults this permission, while
a request for the definitions describing those same values was refused. It could
read the data but not the schema, which is the wrong way round. The routes had
already drawn the line correctly and deliberately: reads are unflagged so Admin
can ask any site and get an empty list, and everything that changes something is
flagged. Only the permission layer had not caught up. What remains on the
resource is defining fields, which stays with the publisher.

The smaller change alongside it is one of order. Which namespace is being
written to now settles before who is asking, so a namespace nobody can define in
refuses on that ground whatever role the caller holds, rather than answering
with a missing permission that no one could have been granted. That is also
where namespace ownership belongs once an app owns a namespace of its own, since
the caller will then have to be that app rather than hold a role.

Claude-Session: https://claude.ai/code/session_01XFCbqgYHYhd9rZ5WXtqZyT
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Team

Run ID: 5da3b722-7460-4133-9ed9-1bc6d2691019

📥 Commits

Reviewing files that changed from the base of the PR and between 45274cd and 5e83246.

📒 Files selected for processing (10)
  • ghost/core/core/server/api/endpoints/member-metafields.ts
  • ghost/core/core/server/data/migrations/versions/6.63/2026-09-03-11-03-31-remove-member-custom-field-read-permissions.js
  • ghost/core/core/server/data/schema/fixtures/fixtures.json
  • ghost/core/core/server/services/members-metafields/definitions-service.ts
  • ghost/core/core/server/services/members-metafields/namespaces.ts
  • ghost/core/test/e2e-api/admin/member-custom-fields.test.ts
  • ghost/core/test/integration/migrations/migration.test.js
  • ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
  • ghost/core/test/unit/server/data/schema/integrity.test.js
  • ghost/core/test/utils/fixtures/fixtures.json
💤 Files with no reviewable changes (2)
  • ghost/core/core/server/data/schema/fixtures/fixtures.json
  • ghost/core/test/utils/fixtures/fixtures.json

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (12)
Review migration safety beyond lint: schema and migration parity, existing-data shape and volume, deploy/rollback compatibility, transaction and locking risk, idempotency, export/integrity updates, and preservation of constraints/defaults.

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/core/server/data/migrations/versions/6.63/2026-09-03-11-03-31-remove-member-custom-field-read-permissions.js
Review new or changed service boundaries for explicit dependency ownership, deterministic/idempotent initialisation, boot ordering, transaction and event semantics, cache coherence, and restart/multi-instance safety.

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/core/server/services/members-metafields/definitions-service.ts
  • ghost/core/core/server/services/members-metafields/namespaces.ts
Review API contract semantics: authentication and permissions, validation at untrusted boundaries, writable-field allowlists, accidental response-data exposure, stable error codes/statuses, pagination/filter consistency, cache invalidation,...

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/core/server/api/endpoints/member-metafields.ts
Review whether tests prove changed behaviour, meaningful error/edge paths, and externally observable contracts without coupling to implementation details.

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/test/unit/server/data/schema/integrity.test.js
  • ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
  • ghost/core/test/integration/migrations/migration.test.js
  • ghost/core/test/e2e-api/admin/member-custom-fields.test.ts
New source files must be TypeScript: flag new JS files as a required change unless exempt (DB migrations, apps/ember-admin/, tool/config files, scripts/, docker/, generated code).

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/test/unit/server/data/schema/integrity.test.js
  • ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
  • ghost/core/core/server/data/migrations/versions/6.63/2026-09-03-11-03-31-remove-member-custom-field-read-permissions.js
  • ghost/core/test/integration/migrations/migration.test.js
Review lens: "where does this data become trusted?" Boundary data (HTTP input, external API/SDK responses, env/config, DB/filesystem reads, queue/webhook/event payloads) is `unknown` until validated — Zod by default.

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/core/server/services/members-metafields/definitions-service.ts
  • ghost/core/core/server/services/members-metafields/namespaces.ts
  • ghost/core/core/server/api/endpoints/member-metafields.ts
  • ghost/core/test/e2e-api/admin/member-custom-fields.test.ts
Prioritise concrete correctness, security, data-integrity, compatibility, and regression risks.

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/test/unit/server/data/schema/integrity.test.js
  • ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
  • ghost/core/core/server/services/members-metafields/definitions-service.ts
  • ghost/core/core/server/services/members-metafields/namespaces.ts
  • ghost/core/core/server/api/endpoints/member-metafields.ts
  • ghost/core/core/server/data/migrations/versions/6.63/2026-09-03-11-03-31-remove-member-custom-field-read-permissions.js
  • ghost/core/test/integration/migrations/migration.test.js
  • ghost/core/test/e2e-api/admin/member-custom-fields.test.ts
Boot owns service initialization; do not initialize on the first request.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • ghost/core/core/server/services/members-metafields/definitions-service.ts
  • ghost/core/core/server/services/members-metafields/namespaces.ts
Type-safe boundaries: Fail only if the PR: consumes boundary data (HTTP input, external API/SDK responses, env/config, DB/filesystem reads, queue/webhook/event payloads) without validating it first — Zod by default, another format only wher...

📄 CodeRabbit inference engine (Custom checks)

Files:

  • ghost/core/core/server/services/members-metafields/definitions-service.ts
  • ghost/core/core/server/services/members-metafields/namespaces.ts
  • ghost/core/core/server/api/endpoints/member-metafields.ts
  • ghost/core/test/e2e-api/admin/member-custom-fields.test.ts
New standalone services use TypeScript; keep CommonJS only at existing `require()` boundaries.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • ghost/core/core/server/services/members-metafields/definitions-service.ts
  • ghost/core/core/server/services/members-metafields/namespaces.ts
New files are TypeScript: Fail if the PR adds a new .js/.jsx/.cjs/.mjs source file, unless it is: a DB migration (ghost/core/core/server/data/migrations/), under apps/ember-admin/, a tool/config file, under scripts/ or docker/, or generated...

📄 CodeRabbit inference engine (Custom checks)

Files:

  • ghost/core/test/unit/server/data/schema/integrity.test.js
  • ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
  • ghost/core/core/server/data/migrations/versions/6.63/2026-09-03-11-03-31-remove-member-custom-field-read-permissions.js
  • ghost/core/test/integration/migrations/migration.test.js
Always use `pnpm`, never npm or Yarn.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • ghost/core/test/unit/server/data/schema/integrity.test.js
  • ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
  • ghost/core/core/server/services/members-metafields/definitions-service.ts
  • ghost/core/core/server/services/members-metafields/namespaces.ts
  • ghost/core/core/server/api/endpoints/member-metafields.ts
  • ghost/core/core/server/data/migrations/versions/6.63/2026-09-03-11-03-31-remove-member-custom-field-read-permissions.js
  • ghost/core/test/integration/migrations/migration.test.js
  • ghost/core/test/e2e-api/admin/member-custom-fields.test.ts
🔇 Additional comments (8)
ghost/core/core/server/services/members-metafields/namespaces.ts (1)

1-29: LGTM!

ghost/core/core/server/services/members-metafields/definitions-service.ts (1)

9-9: LGTM!

Also applies to: 137-137, 187-187, 394-394

ghost/core/core/server/api/endpoints/member-metafields.ts (1)

2-2: LGTM!

Also applies to: 14-36, 47-47, 60-60, 72-72, 88-88, 104-104, 122-122

ghost/core/core/server/data/migrations/versions/6.63/2026-09-03-11-03-31-remove-member-custom-field-read-permissions.js (1)

1-30: LGTM!

ghost/core/test/integration/migrations/migration.test.js (1)

90-90: LGTM!

Also applies to: 480-480

ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js (1)

401-401: LGTM!

ghost/core/test/unit/server/data/schema/integrity.test.js (1)

41-41: LGTM!

ghost/core/test/e2e-api/admin/member-custom-fields.test.ts (1)

2233-2233: LGTM!

Also applies to: 2242-2244, 2260-2269


Walkthrough

Member metafield browse and read endpoints no longer check member_custom_field permissions. Add, reorder, edit, and destroy validate the namespace before checking mutation permissions. Shared namespace helpers replace service-local validation. A migration removes obsolete browse and read permissions from roles. Schema and test fixtures, permission counts, fixture counts, integrity hashes, and API authorization tests are updated.

Suggested reviewers: 9larsons, kevinansfield

Merge Risk: ⚪ Minimal · up to 5e832

Authenticated staff can now read member custom-field definitions without dedicated read permissions, while definition changes remain restricted to the publisher-owned custom namespace and existing mutation permissions. The migration and updated tests align with this behavior, with no current merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the removal of read permission for member custom field definitions, which is a primary change. It omits the browse permission and namespace validation changes, but the titl…
Description check ✅ Passed The description accurately explains the permission changes, namespace validation order, migration, fixture updates, tests, and intended authorization behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Type-Safe Boundaries ✅ Passed No explicit type-safe-boundary failure is introduced. The only new boundary-related use is frame.options.namespace in canDefine; the endpoint receives it from the :namespace route parameter, dec…
New Files Are Typescript ✅ Passed The PR adds two files: one .ts source file and one .js database migration. The only added JavaScript file is under ghost/core/core/server/data/migrations/, which the check explicitly allows. All…
Full details: Title check

Explanation

The title clearly describes the removal of read permission for member custom field definitions, which is a primary change. It omits the browse permission and namespace validation changes, but the title does not need to cover every change.

Full details: Type-Safe Boundaries

Explanation

No explicit type-safe-boundary failure is introduced. The only new boundary-related use is frame.options.namespace in canDefine; the endpoint receives it from the :namespace route parameter, declares the API framework's required option validation, and then applies the explicit assertDefinable allow-list check before the permission call. The remaining changes are internal calls, static migration/fixture data, or tests. The diff introduces no any, @ts-nocheck, @ts-ignore, or new unchecked as cast. Existing casts in definitions-service.ts and the endpoint were not introduced by this commit.

Full details: New Files Are Typescript

Explanation

The PR adds two files: one .ts source file and one .js database migration. The only added JavaScript file is under ghost/core/core/server/data/migrations/, which the check explicitly allows. All other JavaScript files are pre-existing and modified only.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/metafields-open-definition-reads

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the migration [pull request] Includes migration for review label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

It looks like this PR contains a migration 👀
Here's the checklist for reviewing migrations:

General requirements

  • ⚠️ Tested performance on staging database servers, as performance on local machines is not comparable to a production environment
  • Satisfies idempotency requirement (both up() and down())
  • Does not reference models
  • Filename is in the correct format (and correctly ordered)
  • Targets the next minor version
  • All code paths have appropriate log messages
  • Uses the correct utils
  • Contains a minimal changeset
  • Does not mix DDL/DML operations

Schema changes

  • Both schema change and related migration have been implemented
  • For index changes: has been performance tested for large tables
  • For new tables/columns: fields use the appropriate predefined field lengths
  • For new tables/columns: field names follow the appropriate conventions
  • Does not drop a non-alpha table outside of a major version

Data changes

  • Mass updates/inserts are batched appropriately
  • Does not loop over large tables/datasets
  • Defends against missing or invalid data
  • For settings updates: follows the appropriate guidelines

@nx-cloud

nx-cloud Bot commented Sep 3, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit 5e83246

Command Status Duration Result
nx run ghost:test:ci:integration ✅ Succeeded 4m 39s View ↗
nx run ghost:test:integration ✅ Succeeded 2m 51s View ↗
nx run ghost:test:ci:e2e ✅ Succeeded 4m 17s View ↗
nx run ghost:test:legacy ✅ Succeeded 3m 24s View ↗
nx run ghost:test:e2e ✅ Succeeded 2m 17s View ↗
nx run ghost-monorepo:lint:boundaries ✅ Succeeded 23s View ↗
nx run-many -t test:unit -p ghost ✅ Succeeded 34s View ↗
nx run-many -t lint -p ghost,ghost-monorepo ✅ Succeeded 19s View ↗
Additional runs (3) ✅ Succeeded ... View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-09-03 11:30:33 UTC

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 67.50%. Comparing base (45274cd) to head (5e83246).

Files with missing lines Patch % Lines
...e/server/services/members-metafields/namespaces.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@                             Coverage Diff                             @@
##           chore/metafields-terminology-vocabulary   #30497      +/-   ##
===========================================================================
- Coverage                                    67.52%   67.50%   -0.02%     
===========================================================================
  Files                                         1664     1665       +1     
  Lines                                        60060    60061       +1     
  Branches                                     10386    10386              
===========================================================================
- Hits                                         40554    40546       -8     
- Misses                                       17217    17224       +7     
- Partials                                      2289     2291       +2     
Flag Coverage Δ
e2e-tests 70.29% <92.85%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rob-ghost
rob-ghost merged commit 1690bce into main Sep 3, 2026
60 checks passed
@rob-ghost
rob-ghost deleted the chore/metafields-open-definition-reads branch September 3, 2026 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

migration [pull request] Includes migration for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant