Skip to content

🐛 Fixed settings import overwriting protected core settings - #30694

Merged
acburdine merged 1 commit into
mainfrom
fix-settings-import-core-group
Sep 10, 2026
Merged

acburdine merged 1 commit into
mainfrom
fix-settings-import-core-group

Conversation

@acburdine

Copy link
Copy Markdown
Member

no ref

Why

The settings importer is meant to drop every core setting (session secrets, private keys, db_hash and similar), but the filter checked the group field supplied in the import file. Only files that omit group get it derived from the key, so a file that relabels a core key as site could overwrite it.

Exports were also including the whole core group. Those settings are never imported, so exporting them only put instance secrets (admin_session_secret, members_private_key, ghost_private_key, etc.) into files that get emailed to support and handed to migration tools.

What

  • Importer: the core/theme filter now looks up each key's group in the schema's default-settings.json and only falls back to the file's group for unknown keys (which Settings.edit can't write anyway). I didn't use settings-key-group-mapper because it has no entry for 7 of the 18 core keys, including machine_payments_secret, members_stripe_webhook_secret and site_uuid.
  • Exporter: the core group is always excluded. The four blocklist entries that were core keys are removed as redundant. The blocklist count test now counts only non-core keys.
  • Tests: added an importer unit test with relabelled core keys, including one the mapper doesn't know about. The exporter integration test now asserts no core settings are exported.

DB backups use the same exporter, so they no longer contain core settings either. Nothing lost: restoring a backup goes through the importer, which already drops them.

Thanks to @zachariahchow-gvt for the report.

🤖 Generated with Claude Code

no ref

Core settings (session secrets, private keys and similar) are meant to be
dropped on import, but the filter trusted the group field from the import
file, so relabelling a core key as `site` let an import overwrite it. The
group now comes from the schema's default settings, which covers every core
key; the API key-group mapper is missing several of them.

Exports also no longer include the core group. Those settings are never
imported, so exporting them only put instance secrets into files that get
shared with support and migration tools.

Thanks to @zachariahchow-gvt for the report.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@nx-cloud

nx-cloud Bot commented Sep 10, 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 19fe054

Command Status Duration Result
nx run ghost:test:ci:integration ✅ Succeeded 4m 40s View ↗
nx run ghost:test:integration ✅ Succeeded 3m 45s View ↗
nx run ghost:test:ci:e2e ✅ Succeeded 4m 16s View ↗
nx run ghost:test:legacy ✅ Succeeded 3m 41s View ↗
nx run ghost:test:e2e ✅ Succeeded 2m 57s View ↗
nx run ghost-monorepo:lint:boundaries ✅ Succeeded 27s View ↗
nx run-many -t test:unit -p ghost ✅ Succeeded 33s View ↗
nx run-many --target=build --projects=tag:publi... ✅ Succeeded 1s View ↗
Additional runs (4) ✅ Succeeded ... View ↗

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


☁️ Nx Cloud last updated this comment at 2026-09-10 18:58:35 UTC

@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview 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: Advanced

Run ID: fd4c8734-455f-4f42-9501-9b582e49bb2e

📥 Commits

Reviewing files that changed from the base of the PR and between 89d2129 and 19fe054.

📒 Files selected for processing (6)
  • ghost/core/core/server/data/exporter/exporter.js
  • ghost/core/core/server/data/exporter/table-lists.js
  • ghost/core/core/server/data/importer/importers/data/settings-importer.js
  • ghost/core/test/integration/exporter/exporter.test.js
  • ghost/core/test/unit/server/data/exporter/index.test.js
  • ghost/core/test/unit/server/data/importer/importers/data/settings.test.js

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

📜 Recent review details
⏰ Context from checks skipped due to timeout. (14)
  • GitHub Check: Build Admin
  • GitHub Check: Build E2E Public App Assets
  • GitHub Check: Build Docker Images
  • GitHub Check: Lint
  • GitHub Check: Acceptance tests (Node 24.20.0, mysql8)
  • GitHub Check: Check app version bump
  • GitHub Check: Legacy tests (Node 22.23.1, mysql8)
  • GitHub Check: Unit tests (Node 22.23.1)
  • GitHub Check: Legacy tests (Node 24.20.0, mysql8)
  • GitHub Check: Unit tests (Node 24.20.0)
  • GitHub Check: Acceptance tests (Node 22.23.1, mysql8)
  • GitHub Check: Typecheck
  • GitHub Check: Detect Tinybird changes
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
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/integration/exporter/exporter.test.js
  • ghost/core/test/unit/server/data/importer/importers/data/settings.test.js
  • ghost/core/test/unit/server/data/exporter/index.test.js
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/core/server/data/exporter/exporter.js
  • ghost/core/test/integration/exporter/exporter.test.js
  • ghost/core/core/server/data/exporter/table-lists.js
  • ghost/core/test/unit/server/data/importer/importers/data/settings.test.js
  • ghost/core/test/unit/server/data/exporter/index.test.js
  • ghost/core/core/server/data/importer/importers/data/settings-importer.js
Prioritise concrete correctness, security, data-integrity, compatibility, and regression risks.

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/core/server/data/exporter/exporter.js
  • ghost/core/test/integration/exporter/exporter.test.js
  • ghost/core/core/server/data/exporter/table-lists.js
  • ghost/core/test/unit/server/data/importer/importers/data/settings.test.js
  • ghost/core/test/unit/server/data/exporter/index.test.js
  • ghost/core/core/server/data/importer/importers/data/settings-importer.js
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/core/server/data/exporter/exporter.js
  • ghost/core/test/integration/exporter/exporter.test.js
  • ghost/core/core/server/data/exporter/table-lists.js
  • ghost/core/test/unit/server/data/importer/importers/data/settings.test.js
  • ghost/core/test/unit/server/data/exporter/index.test.js
  • ghost/core/core/server/data/importer/importers/data/settings-importer.js

Walkthrough

The exporter now excludes all settings in the core group and updates its non-core blocklist. Export tests verify that core settings and sensitive keys are absent. The importer now resolves setting groups from the default schema before applying its core and theme filter. Import tests verify that core settings are removed even when the input file labels them as site.

Suggested reviewers: evanhahn, 9larsons, kevinansfield

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 19fe0

Core secrets are protected from import overwrites and excluded from exports, with coverage for relabelled settings. The change is ready to merge.

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Type-Safe Boundaries ⚠️ Warning The PR adds unvalidated boundary-data consumption. In exporter.js, the new setting.group check reads a field from rows returned by a direct database query (query.select()) without runtime valida… Validate the database rows and imported setting records at their boundaries before the new logic uses them. Add Zod schemas for the relevant setting-row shapes, call safeParse or parse, and handle invalid data explicitly. Validate the f…
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: preventing settings import from overwriting protected core settings.
Description check ✅ Passed The description directly explains the importer and exporter changes, the security rationale, and the test coverage.
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.
New Files Are Typescript ✅ Passed The authoritative pull-request diff contains no added .js/.jsx/.cjs/.mjs files. It contains six JavaScript files, all marked M, and each exists in the base revision. The check does not fail for modifi…
Full details: Type-Safe Boundaries

Explanation

The PR adds unvalidated boundary-data consumption. In exporter.js, the new setting.group check reads a field from rows returned by a direct database query (query.select()) without runtime validation. In settings-importer.js, the new lookup consumes data.key and data.group from JSON import data; the JSON handler only parses JSON and checks the optional {db: [...]} wrapper, then passes arbitrary setting records to the importer. The PR also iterates the file-backed defaultSettings object without validating its shape. No new any, unchecked as, or TypeScript suppression was found.

Resolution

Validate the database rows and imported setting records at their boundaries before the new logic uses them. Add Zod schemas for the relevant setting-row shapes, call safeParse or parse, and handle invalid data explicitly. Validate the file-backed defaultSettings shape before building defaultSettingsGroups. If types are added, derive them with z.infer instead of duplicating schema types.

  • Fix all pre-merge checks with AI
✨ 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 fix-settings-import-core-group

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

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.71%. Comparing base (36e27b8) to head (19fe054).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #30694      +/-   ##
==========================================
+ Coverage   67.66%   67.71%   +0.05%     
==========================================
  Files        1675     1675              
  Lines       60502    60506       +4     
  Branches    10461    10463       +2     
==========================================
+ Hits        40936    40972      +36     
+ Misses      17249    17218      -31     
+ Partials     2317     2316       -1     
Flag Coverage Δ
e2e-tests 70.45% <100.00%> (+0.07%) ⬆️

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.

@acburdine
acburdine merged commit 5d48296 into main Sep 10, 2026
59 checks passed
@acburdine
acburdine deleted the fix-settings-import-core-group branch September 10, 2026 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant