Skip to content

Reset all authentication action in Danger Zone#27820

Open
rob-ghost wants to merge 3 commits into
mainfrom
feat/danger-zone-reset-actions
Open

Reset all authentication action in Danger Zone#27820
rob-ghost wants to merge 3 commits into
mainfrom
feat/danger-zone-reset-actions

Conversation

@rob-ghost
Copy link
Copy Markdown
Contributor

@rob-ghost rob-ghost commented May 11, 2026

ref https://linear.app/ghost/issue/BER-3628/danger-zone-rotate-api-keys
ref https://linear.app/ghost/issue/BER-3629/danger-zone-reset-all-staff-passwords

Problem

After a suspected credential compromise, a site owner needs to invalidate every credential a leaked database snapshot might contain. There's no UI for that. Each integration key has to be rotated by hand and each user reset individually, with no guarantee of completeness.

Solution

A single new entry in Settings → Advanced → Danger Zone: "Reset all authentication". Confirming the action rotates every API key, signs every staff user out and requires them to reset their password on next sign-in, destroys every active session (including the triggering user's), and re-issues every queued scheduler callback under the new key so scheduled posts, gift reminders, and the automations poll chain don't break across the key rotation. The audit log records the action with counts.

Members are unaffected.

Behind the dangerZoneResetAuth private labs flag for now.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 11, 2026

Review Change Stack

Walkthrough

This PR adds a "Reset all authentication" destructive action to Ghost Admin's Danger zone. Backend changes introduce a reset-authentication service, admin POST routes, and new permission fixtures; the frontend adds a ResetAuthResponse contract and useResetAuth hook, a feature-flagged Danger Zone UI action with confirmation flow, and navigation/feature-flag wiring. E2E and unit tests validate the service, user-locking behavior, fixture integrity, and the end-to-end reset flow including post-reset authentication rejection.

Possibly related PRs

  • TryGhost/Ghost#27849: Related work on internalKeys snapshot/clearing and rescheduling contracts used by the reset-authentication flow.

Suggested reviewers

  • kevinansfield
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding a 'Reset all authentication' action to the Danger Zone settings section.
Description check ✅ Passed The description clearly explains the problem, solution, and implementation details, directly relating to the changeset's addition of the reset authentication feature.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 feat/danger-zone-reset-actions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions github-actions Bot added the migration [pull request] Includes migration for review label May 11, 2026
@github-actions
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
  • Tested in MySQL and SQLite

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

@codecov
Copy link
Copy Markdown

codecov Bot commented May 11, 2026

Codecov Report

❌ Patch coverage is 63.57895% with 173 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.76%. Comparing base (7a6d35f) to head (318df6c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
.../core/server/services/auth/reset-authentication.ts 47.82% 60 Missing ⚠️
...st/core/core/server/services/gifts/gift-service.ts 23.25% 33 Missing ⚠️
...server/services/post-scheduling/post-scheduling.ts 87.58% 18 Missing ⚠️
...t/core/core/server/api/endpoints/authentication.js 50.00% 17 Missing ⚠️
ghost/core/core/server/services/users.js 54.05% 17 Missing ⚠️
...server/services/gifts/gift-bookshelf-repository.ts 20.00% 8 Missing ⚠️
...dpoints/utils/serializers/output/authentication.js 30.00% 7 Missing ⚠️
ghost/core/core/server/models/api-key.js 72.22% 5 Missing ⚠️
ghost/core/core/server/models/user.js 61.53% 5 Missing ⚠️
...ost/core/core/server/services/automations/index.js 90.47% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #27820      +/-   ##
==========================================
- Coverage   73.79%   73.76%   -0.04%     
==========================================
  Files        1522     1523       +1     
  Lines      128738   128982     +244     
  Branches    15450    15451       +1     
==========================================
+ Hits        95002    95141     +139     
- Misses      32778    32907     +129     
+ Partials      958      934      -24     
Flag Coverage Δ
admin-tests 53.54% <ø> (ø)
e2e-tests 73.76% <63.57%> (-0.04%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
e2e/helpers/pages/admin/settings/sections/danger-zone-section.ts (1)

28-28: ⚡ Quick win

Fragile selector: .last() assumes button order.

The .getByRole('button').last() selector assumes the OK button is always the last button in the modal. If the modal's button order changes (e.g., additional action buttons are added), this will break.

Consider using a more robust selector:

  • Add a data-testid='url-confirmation-ok-button' to the OK button in the modal component
  • Or use getByRole('button', {name: /rotate|reset|ok/i}) if the button has distinguishable accessible text
♻️ Proposed fix with data-testid

In url-confirmation-modal.tsx, add data-testid to the OK button (assuming Modal component supports it):

 <Modal
   ...
   okLabel={taskState === 'running' ? okRunningLabel : okLabel}
+  okButtonTestId='url-confirmation-ok-button'

Then update the page object:

-this.urlConfirmationOkButton = this.urlConfirmationModal.getByRole('button').last();
+this.urlConfirmationOkButton = page.getByTestId('url-confirmation-ok-button');
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/helpers/pages/admin/settings/sections/danger-zone-section.ts` at line 28,
The selector this.urlConfirmationOkButton =
this.urlConfirmationModal.getByRole('button').last() is fragile; update the
modal component (where the OK button is rendered) to add a stable attribute such
as data-testid="url-confirmation-ok-button" on the OK button, then change the
page object to select the button via
this.urlConfirmationModal.getByTestId('url-confirmation-ok-button')
(alternatively use this.urlConfirmationModal.getByRole('button', { name:
/ok|confirm|rotate|reset/i }) if the button text is stable). Ensure references
to urlConfirmationOkButton and urlConfirmationModal are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@e2e/helpers/pages/admin/settings/sections/danger-zone-section.ts`:
- Line 28: The selector this.urlConfirmationOkButton =
this.urlConfirmationModal.getByRole('button').last() is fragile; update the
modal component (where the OK button is rendered) to add a stable attribute such
as data-testid="url-confirmation-ok-button" on the OK button, then change the
page object to select the button via
this.urlConfirmationModal.getByTestId('url-confirmation-ok-button')
(alternatively use this.urlConfirmationModal.getByRole('button', { name:
/ok|confirm|rotate|reset/i }) if the button text is stable). Ensure references
to urlConfirmationOkButton and urlConfirmationModal are updated accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 01e7f94e-a234-42a2-ba72-ce078b4419be

📥 Commits

Reviewing files that changed from the base of the PR and between 9f9364c and ad9e57d.

⛔ Files ignored due to path filters (1)
  • ghost/core/test/e2e-api/admin/__snapshots__/security.test.js.snap is excluded by !**/*.snap
📒 Files selected for processing (24)
  • apps/admin-x-framework/src/api/security.ts
  • apps/admin-x-settings/src/components/settings/advanced/danger-zone.tsx
  • apps/admin-x-settings/src/components/settings/advanced/danger-zone/url-confirmation-modal.tsx
  • e2e/helpers/pages/admin/settings/sections/danger-zone-section.ts
  • e2e/helpers/pages/admin/settings/sections/index.ts
  • e2e/helpers/pages/admin/settings/settings-page.ts
  • e2e/tests/admin/settings/danger-zone.test.ts
  • ghost/admin/package.json
  • ghost/core/core/server/api/endpoints/index.js
  • ghost/core/core/server/api/endpoints/security.js
  • ghost/core/core/server/api/endpoints/utils/serializers/output/index.js
  • ghost/core/core/server/api/endpoints/utils/serializers/output/security.js
  • ghost/core/core/server/data/migrations/versions/6.38/2026-05-11-12-00-00-add-security-action-permissions.js
  • ghost/core/core/server/data/schema/fixtures/fixtures.json
  • ghost/core/core/server/services/security-actions/security-actions-service.js
  • ghost/core/core/server/web/api/endpoints/admin/routes.js
  • ghost/core/package.json
  • ghost/core/test/e2e-api/admin/security.test.js
  • 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/unit/server/services/security-actions/security-actions-service.test.js
  • ghost/core/test/utils/fixtures/fixtures.json
  • scripts/build-e2e-image.sh

@rob-ghost rob-ghost force-pushed the feat/danger-zone-reset-actions branch from 2a840fa to 4718733 Compare May 12, 2026 14:21
@rob-ghost rob-ghost changed the title Danger Zone — rotate API keys and reset all staff passwords Refactored internal scheduler integration caching for rotation May 12, 2026
@rob-ghost rob-ghost changed the title Refactored internal scheduler integration caching for rotation Danger Zone — rotate API keys and reset all staff passwords May 12, 2026
@rob-ghost rob-ghost force-pushed the feat/danger-zone-reset-actions branch 2 times, most recently from c196615 to bd5a594 Compare May 12, 2026 23:20
@rob-ghost rob-ghost changed the base branch from main to chore/internal-api-key-caching May 13, 2026 10:01
@rob-ghost rob-ghost force-pushed the chore/internal-api-key-caching branch from a62d453 to 5bde1c5 Compare May 13, 2026 10:03
@rob-ghost rob-ghost force-pushed the feat/danger-zone-reset-actions branch 2 times, most recently from fc59f12 to aa08cf6 Compare May 13, 2026 22:49
@rob-ghost rob-ghost added the preview Deploy a PR preview environment label May 13, 2026
@rob-ghost rob-ghost force-pushed the chore/internal-api-key-caching branch from 5bde1c5 to 2827cdb Compare May 13, 2026 22:55
@rob-ghost rob-ghost force-pushed the feat/danger-zone-reset-actions branch 2 times, most recently from e626ba3 to 4077acd Compare May 14, 2026 09:01
@github-actions
Copy link
Copy Markdown
Contributor

E2E Tests Failed

To view the Playwright test report locally, run:

REPORT_DIR=$(mktemp -d) && gh run download 25851485288 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR"

@rob-ghost rob-ghost force-pushed the chore/internal-api-key-caching branch 4 times, most recently from af3d00b to 3669367 Compare May 14, 2026 14:40
@rob-ghost rob-ghost force-pushed the feat/danger-zone-reset-actions branch from 4077acd to e421aec Compare May 14, 2026 15:13
@rob-ghost rob-ghost added preview Deploy a PR preview environment and removed preview Deploy a PR preview environment labels May 14, 2026
@Ghost-Slimer Ghost-Slimer temporarily deployed to pr-preview-27820 May 14, 2026 15:31 Destroyed
@github-actions
Copy link
Copy Markdown
Contributor

E2E Tests Failed

To view the Playwright test report locally, run:

REPORT_DIR=$(mktemp -d) && gh run download 25868089042 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR"

@rob-ghost rob-ghost force-pushed the chore/internal-api-key-caching branch from 3669367 to 9e647fe Compare May 14, 2026 15:36
@rob-ghost rob-ghost force-pushed the feat/danger-zone-reset-actions branch from e421aec to 5068664 Compare May 14, 2026 15:38
@rob-ghost rob-ghost force-pushed the feat/danger-zone-reset-actions branch 6 times, most recently from 8f4d12b to bcc31a4 Compare May 19, 2026 11:59
ref https://linear.app/ghost/issue/BER-3628/danger-zone-rotate-api-keys
ref https://linear.app/ghost/issue/BER-3629/danger-zone-reset-all-staff-passwords

After a suspected credential compromise a site owner needs to invalidate
every credential a leaked database snapshot might contain, but there was
no UI for it. Each integration key had to be rotated and each user reset
manually, with no guarantee of completeness.

A new Settings → Advanced → Danger Zone row, "Reset all authentication",
opens a confirmation modal explaining the consequences. Confirming hits
a new admin endpoint at POST /authentication/reset which rotates every
api_keys.secret in one transaction, clears the internal-keys cache so
cached secrets reload, re-issues every queued scheduler callback under
the new key (posts/pages, automations polls, gift reminders), locks
every staff user and requires them to reset their password on next
sign-in, and destroys every active session including the triggering
user's. The audit log records the action with counts.

The orphaned /authentication/global_password_reset endpoint is removed.
The new endpoint supersedes it and no callers referenced it. The
existing "Reset all passwords" permission row is renamed to "Reset
authentication" via migration to keep the permissions table coherent.

The userService.resetAllPasswords method is renamed to lockAll: it sets
a 'locked' status with a random password rather than mass-emailing
reset notifications, honors an outer transaction so rotation, lock, and
audit commit atomically, and accepts an optional roles filter for a
future staff-only variant.

Behind the \`dangerZoneResetAuth\` private labs flag.
@rob-ghost rob-ghost force-pushed the feat/danger-zone-reset-actions branch from bcc31a4 to 4c6b2eb Compare May 19, 2026 12:33
@rob-ghost rob-ghost marked this pull request as ready for review May 19, 2026 13:13
@rob-ghost rob-ghost requested a review from 9larsons as a code owner May 19, 2026 13:13
@rob-ghost rob-ghost requested a review from weylandswart May 19, 2026 13:14
@rob-ghost rob-ghost added preview Deploy a PR preview environment and removed preview Deploy a PR preview environment labels May 19, 2026
@rob-ghost rob-ghost self-assigned this May 19, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx`:
- Around line 79-81: Update the feature description for the entry with flag
'dangerZoneResetAuth' so its scope matches actual behavior: instead of saying
"locks every user" clarify it only locks staff accounts (e.g., "rotates every
API key, locks all staff accounts, and destroys every session") and add a short
note that this is a staff-only action and members are unaffected; edit the
description field in private-features.tsx (the object with title, description,
flag) accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5915a17f-aa31-43ee-ae67-d246ade7731e

📥 Commits

Reviewing files that changed from the base of the PR and between ad9e57d and 4c6b2eb.

📒 Files selected for processing (9)
  • apps/admin-x-framework/src/api/security.ts
  • apps/admin-x-settings/src/components/settings/advanced/danger-zone.tsx
  • apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx
  • apps/admin-x-settings/src/components/sidebar.tsx
  • apps/admin-x-settings/test/acceptance/advanced/dangerzone.test.ts
  • e2e/helpers/pages/admin/settings/sections/danger-zone-section.ts
  • e2e/helpers/pages/admin/settings/sections/index.ts
  • e2e/helpers/pages/admin/settings/settings-page.ts
  • e2e/tests/admin/settings/danger-zone.test.ts
✅ Files skipped from review due to trivial changes (1)
  • e2e/helpers/pages/admin/settings/sections/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • e2e/helpers/pages/admin/settings/settings-page.ts

Comment thread apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx Outdated
Comment thread ghost/core/core/server/services/auth/reset-authentication.ts Outdated
@rob-ghost
Copy link
Copy Markdown
Contributor Author

ℹ️ @weylandswart this is behind a labs flag, but there's still non-flagged changes which will go out immediately on merge:

Adding "Danger Zone" to the settings sidebar, it was missing and you needed to scroll to find it:
Screenshot 2026-05-19 at 14 41 55

This is what the new "Danger Zone" section looks like with the flag off, since before there was only one option and now we have multiple:
Screenshot 2026-05-19 at 14 42 00


The following is what it looks like with the flag on, we're safe to iterate on these if we want to change UX / copy before release and doesn't have to block this PR:

Now with the flag on:
Screenshot 2026-05-19 at 14 42 11

And the confirmation modal:
Screenshot 2026-05-19 at 14 42 16


Comment on lines +637 to +643
/**
* Re-issue every queued reminder under the current scheduler key. Pass
* the pre-rotation secret as `previousKey` so each adapter-queued URL
* can be reconstructed for unschedule before resigning with the new
* key. Reminders whose fire time has already passed are skipped — the
* daily cron will pick them up.
*/
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @mike182uk this is the change to Gift reminder scheduling when we need to rotate the scheduler API key in order to re-schedule the reminders.

AFAIK we don't need to change anything about boot-time scheduling as that's already handled / not an issue?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh nothing needs to be done on the boot, the daily cron should pick up schedules that do not survive a reboot (i.e self hosted site)

Comment on lines +70 to +72
rescheduleAll() {
this.#enqueuePollNow?.();
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @EvanHahn we're introducing the ability to rotate all API keys, and this breaks anything scheduled with the scheduler as their tokens become invalid.

Posts and Gifts re-schedule explicitly on key rotation, I've added the same for automations but I'm not 100% sure its needed.

Your input would be greatly appreciated 🙏🏻

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't quite right. We'll need to re-enqueue any future job. I believe that'll involve a database query; happy to help you write this.

Comment on lines +10 to +14
export default new PostScheduling({
apiUrl: urlUtils.urlFor('api', {type: 'admin'}, true),
adapter: adapterManager.getAdapter('scheduling'),
internalKeys
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: This side-effect inducing export of a singleton (constructor registers global event listeners) feels like a new pattern compared to our previous explicit .init() calls. Is this a preferred pattern going forwards?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few other examples of this "eager" construction of the singletons as opposed to the .init() deferred style.

I went a few rounds on this concern, and I eventually came down to these criteria for deciding between eager and deferred singleton construction:

  1. We want the classes to be self contained and not concern themselves with an "initialisation" state. A class instance should be ready to go from construction. That is only possible if:
  2. A classes dependencies are resolvable at import time, and not boot time. In this case each of the dependencies was available at import time.
  3. Import-time side-effects - in this case we're registering event listeners which won't be called until the senders are initialised and start sending, which is safe. If the class or initialisation had other non-desirable side-effects (like emitting events) then we'd likely defer to an init call later (and that init would live on the module scope and not the class scope).

So I settled on this style and I think based on the above criteria its the best choice in this situation.

rob-ghost added 2 commits May 19, 2026 15:45
ref https://linear.app/ghost/issue/BER-3628/danger-zone-rotate-api-keys

A post-commit reschedule call could throw and skip the deleteAllSessions
step, leaving session rows in the DB after the user lock had committed.
A stolen session cookie would have remained usable for that window. PR
feedback from kevinansfield flagged this as edge case but real.

Reorder so deleteAllSessions runs immediately after the rotate/lock/audit
transaction commits, and propagates errors. The three reschedule calls
move into Promise.allSettled with per-failure logging, so one failing
doesn't block the others or prevent the session wipe. Reschedules are
operational continuity, not security; the daily cron and event paths
catch up on the next trigger.

Also fix the labs blurb wording (the action locks staff users only, not
every user; members are unaffected) and drop the em-dash from its title.
ref https://linear.app/ghost/issue/BER-3628/danger-zone-rotate-api-keys

reset-authentication calls userService.lockAll with a transacting handle
so rotate, lock, and audit commit atomically. The existing lockAll tests
all call without transacting (exercising the branch that opens its own
transaction). The outer-transaction branch had no direct coverage.

Add a unit test that passes a transacting field and asserts no inner
transaction is opened, pinning the behaviour reset-authentication relies
on against accidental regression.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@ghost/core/core/server/services/auth/reset-authentication.ts`:
- Around line 68-75: Replace the specific event string with the canonical
generic operation and keep the specific action only in context.action_name:
change the audit payload's event property (currently 'reset_authentication') to
the generic operation (e.g. 'update') while leaving context.action_name as
'reset_authentication' in the audit creation code in reset-authentication.ts so
consumers parse the generic event from event and the specific action from
context.action_name.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c7c6070e-e12a-4850-be9f-1838ff33165d

📥 Commits

Reviewing files that changed from the base of the PR and between 4c6b2eb and 318df6c.

📒 Files selected for processing (4)
  • apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx
  • ghost/core/core/server/services/auth/reset-authentication.ts
  • ghost/core/test/unit/server/services/auth/reset-authentication.test.ts
  • ghost/core/test/unit/server/services/users/users-service.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx

Comment on lines +68 to +75
event: 'reset_authentication',
resource_type: 'security_action',
resource_id: null,
actor_type: 'user',
actor_id: actorId,
context: {
action_name: 'reset_authentication',
api_keys_rotated: rotated,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use the canonical audit event shape and keep the specific action in context.action_name.

event: 'reset_authentication' mixes operation and action name. This can break consumers that expect the generic operation type in event and parse the specific action from context.action_name.

Suggested adjustment
                 await models.Action.add({
-                    event: 'reset_authentication',
+                    event: 'edited',
                     resource_type: 'security_action',
                     resource_id: null,
                     actor_type: 'user',
                     actor_id: actorId,
                     context: {
                         action_name: 'reset_authentication',

Based on learnings: In Ghost's e2e API tests, audit events should separate the generic operation from the specific action (event as CRUD type, specific action in context.action_name).

📝 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
event: 'reset_authentication',
resource_type: 'security_action',
resource_id: null,
actor_type: 'user',
actor_id: actorId,
context: {
action_name: 'reset_authentication',
api_keys_rotated: rotated,
event: 'edited',
resource_type: 'security_action',
resource_id: null,
actor_type: 'user',
actor_id: actorId,
context: {
action_name: 'reset_authentication',
api_keys_rotated: rotated,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ghost/core/core/server/services/auth/reset-authentication.ts` around lines 68
- 75, Replace the specific event string with the canonical generic operation and
keep the specific action only in context.action_name: change the audit payload's
event property (currently 'reset_authentication') to the generic operation (e.g.
'update') while leaving context.action_name as 'reset_authentication' in the
audit creation code in reset-authentication.ts so consumers parse the generic
event from event and the specific action from context.action_name.

* key. Reminders whose fire time has already passed are skipped — the
* daily cron will pick them up.
*/
async rescheduleAll({previousKey}: {previousKey?: InternalApiKey} = {}): Promise<void> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you want to keep this named the same as the other places (automations, postScheduling)?

I think the name is a bit too generic from the outside - giftService.rescheduleAll - sounds like we are rescheduling gifts, which we are not

Could call it something more specific like resignScheduledReminders?

Comment on lines +654 to +676
for (const gift of pending) {
if (!gift.consumesAt) {
continue;
}
const time = gift.consumesAt.getTime() - GIFT_REMINDER_LEAD_MS;
if (time <= Date.now()) {
continue;
}

const buildJob = (key: InternalApiKey) => {
const signedAdminToken = getSignedAdminToken({
publishedAt: new Date(time).toISOString(),
apiUrl,
key
});
const url = new URL(urlJoin(apiUrl, 'gifts', 'flush_reminders'));
url.searchParams.set('token', signedAdminToken);
return {time, url: url.toString(), extra: {httpMethod: 'PUT'}};
};

schedulerAdapter.unschedule(buildJob(unscheduleKey));
schedulerAdapter.schedule(buildJob(currentKey));
}
Copy link
Copy Markdown
Member

@mike182uk mike182uk May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest DRYing this up and maybe re-using the existing scheduleReminder but I think this ends up getting a little messy?

(would probably need to promote buildJob to class level and invoke it in both places, then have scheduleReminder optionally take a scheduler key so it could use the new one in this case, but when called outside of this context, resolve from getSchedulerKey as it currently does)

Comment on lines +637 to +643
/**
* Re-issue every queued reminder under the current scheduler key. Pass
* the pre-rotation secret as `previousKey` so each adapter-queued URL
* can be reconstructed for unschedule before resigning with the new
* key. Reminders whose fire time has already passed are skipped — the
* daily cron will pick them up.
*/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh nothing needs to be done on the boot, the daily cron should pick up schedules that do not survive a reboot (i.e self hosted site)

return (
<TopLevelGroup
customHeader={
<SettingGroupHeader description='Permanently delete all posts and tags from the database, a hard reset' title='Danger zone' />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, but mentioning just in case: if the flag is disabled, we are changing this part of the UI.

const response = await resetAuth(null);
const result = response?.security_action?.[0];
showToast({
title: `Rotated ${result?.api_keys_rotated ?? 0} API keys and locked ${result?.users_locked ?? 0} users. You will be signed out shortly.`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "API keys" and "users" are not properly pluralized. For example, if only 1 user was locked, it will say "1 users".

// (api keys, passwords, sessions) in one shot — so we rename the existing
// permission row to match its new contract rather than introduce a fresh row
// alongside a stale one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we delete the old permission?

status: 'locked',
// secretlint-disable-next-line @secretlint/secretlint-rule-pattern
password: security.identifier.uid(50)
}, Object.assign({}, options, {patch: true}));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we use spreading here?

Suggested change
}, Object.assign({}, options, {patch: true}));
}, {...options, patch: true});

Comment on lines +11 to +18
// eslint-disable-next-line no-unused-vars
postScheduling: {rescheduleAll(opts: {previousKey?: InternalApiKey}): Promise<void>};
// eslint-disable-next-line no-unused-vars
automations: {rescheduleAll(opts: {previousKey?: InternalApiKey}): Promise<void> | void};
// eslint-disable-next-line no-unused-vars
giftService: {rescheduleAll(opts: {previousKey?: InternalApiKey}): Promise<void>};
// eslint-disable-next-line no-unused-vars
userService: {lockAll(options: any): Promise<{count: number}>};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think these eslint-disables are unnecessary. (I don't get errors when I delete them.)

Suggested change
// eslint-disable-next-line no-unused-vars
postScheduling: {rescheduleAll(opts: {previousKey?: InternalApiKey}): Promise<void>};
// eslint-disable-next-line no-unused-vars
automations: {rescheduleAll(opts: {previousKey?: InternalApiKey}): Promise<void> | void};
// eslint-disable-next-line no-unused-vars
giftService: {rescheduleAll(opts: {previousKey?: InternalApiKey}): Promise<void>};
// eslint-disable-next-line no-unused-vars
userService: {lockAll(options: any): Promise<{count: number}>};
postScheduling: {rescheduleAll(opts: {previousKey?: InternalApiKey}): Promise<void>};
automations: {rescheduleAll(opts: {previousKey?: InternalApiKey}): Promise<void> | void};
giftService: {rescheduleAll(opts: {previousKey?: InternalApiKey}): Promise<void>};
userService: {lockAll(options: any): Promise<{count: number}>};

// open our own.
const users = frameOptions.transacting
? await lockUsers(frameOptions)
: await this.models.Base.transaction(t => lockUsers(Object.assign({}, frameOptions, {transacting: t})));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we use object spreading here?

Suggested change
: await this.models.Base.transaction(t => lockUsers(Object.assign({}, frameOptions, {transacting: t})));
: await this.models.Base.transaction(t => lockUsers({...frameOptions, transacting: t}));

Comment on lines +92 to +93
// If the caller supplied an outer transaction, run inside it; otherwise
// open our own.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think it's clear without this comment.

Suggested change
// If the caller supplied an outer transaction, run inside it; otherwise
// open our own.

const lockUsers = async (txOptions) => {
const findOptions = Object.assign({}, txOptions);
if (roles && roles.length) {
findOptions.filter = `roles.name:[${roles.join(',')}]`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever actually use this filter? I don't see it in the code, but maybe I'm misreading. If it's unused, I think we should remove it.

},
User: {
findAll: sinon.mock().resolves([userToReset])
get: sinon.stub().withArgs('email').returns(email ?? 'test_email@example.com')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is fine, but could be clearer if you change the signature of this function.

function makeUser({email = 'test_email@example.com'} = {}) {

* actually got locked, not on what arguments we passed around.
*/
function makeService({users} = {users: [makeUser()]}) {
const userRoles = new Map(); // user → string[]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you actually type this with JSDoc types, rather than this annotation?

Suggested change
const userRoles = new Map(); // user → string[]
/** @type {Map<ReturnType<typeof makeUser>, string[]>} */
const userRoles = new Map();

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 preview Deploy a PR preview environment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants