Ghost: Permissions error when trying to update settings on 3.23.x

[daniellockyer]

reports:

• https://forum.ghost.org/t/saving-settings-configs-error-permission-error-cannot-edit-setting-attempted-to-access-core-setting-from-external-request/15731
• https://forum.ghost.org/t/upgrade-3-22-issues/15667/11

Issue Summary

Updating to 3.23.x from 3.21.1, and then trying to edit settings will result in the following error:

To Reproduce

1. ghost install 3.21.1 --local
2. ghost upgrade 3.23.0 --local
3. Run the following query:

sqlite> select `key`,`type`,`group` from settings where key like 'portal_%';
portal_name|boolean|portal
portal_button|boolean|portal
portal_plans|array|portal
portal_button_style|string|core
portal_button_icon|string|core
portal_button_signup_text|string|core

• portal_button_* settings seem to have been placed in the core group, instead of the desired portal group.
• core settings cannot be set from outside Ghost, hence the permissions error.

Technical details:

• Ghost Version: 3.23.x
• Node Version: any
• Browser/OS: any
• Database: SQLite (probably any)

---

Tasks

• [x] We must to add explicit group and flag migrations for any new setting.
• [x] We should add a new test to ensure this happens. The quick version for this would be an integrity check for default-settings.json - as this at least stops edits to that file without thinking.

[rishabhgrg]

The root cause for this behavior is related to change in columns of settings table and init flow done in 3.22.

Problem

Any new setting added since version 3.23 (and future versions), when migrating from any < 3.22 version will always end up with group column value as core in DB and the actual value in default-settings.json is ignored. core setting values are not allowed to be edited via API, though Admin will send settings API request with the new settings causing permission error when trying to edit settings.

Reason

1. The before hook of knex-migrate here populates all default settings from default-settings.json, which is called as soon as DB backup is done and before running through migrations.

2. populateDefaults adds all the new settings added since old version we are migrating from, and logic here ensures any missing columns in db schema are ignored. This means all the new settings are added to DB with only their type value, while the group or flag values are skipped since the columns don’t exist yet (when migrating from version < 3.22).

3. Migrations in 3.22 run, which adds here new group column to db defaulting to value as core for all settings in DB.

4. Another migration in 3.22 here updates all the known settings till 3.22 to correct group, but that means all the new settings added after 3.22 stay with group as core in DB

5. Since we don’t allow editing core settings from external resource, the new settings when attempted to save from Admin throw the permission error

NOTE: This is not a problem on a new install or when migrating from >=3.22 since the new settings columns will already exist in schema, which means on step (2) the correct group and flags value will be added for setting in DB already in those cases.

[sbounmy]

Same issue here from 3.14.0 -> 3.23.0

[rishabhgrg]

Note: We are working on releasing a fix for this in next release asap, meanwhile please avoid migrating to any 3.23.x version unless you are on version 3.22.x already.

[ErisDS]

Fundamentally the problem here is a 🐔 and 🥚 situation between our prerequisite to populateDefaults before boot vs our need to change the table structure for settings during migrations.

For now the solution is:

• We must to add explicit group and flag migrations for any new setting.
• We should add a new test to ensure this happens. The quick version for this would be an integrity check[1] for default-settings.json - as this at least stops edits to that file without thinking.
• We should figure out a way to resolve this long term E.g. with a major change in 4.0 that allows us to stop doing this.

I have added the first two as tasks to this issue and added the requirement to fix this in 4.0 to our 4.0 tracker.

[1] Note: the integrity check should:

test that select count(*) from settings where type='core' and key not in ('db_hash',...) == 0 and maintain a whitelist in the test of the expected core settings

Integrity checks live in test/unit/data/schema/integrity_spec.js

[rishabhgrg]

3.24.0 is now released with fix for this issue.