🐛 Made database views portable across environments - #28989
Merged
Conversation
github-actions
Bot
added
the
migration
Contributor
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 45s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 8s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 50s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 1m 56s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run-many -t test:unit -p @tryghost/posts,@tr... |
✅ Succeeded | 38s | View ↗ |
nx run-many -t lint -p @tryghost/posts,@tryghos... |
✅ Succeeded | 37s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 9s | View ↗ |
Additional runs (2) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-06-30 11:43:50 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/data/schema/commands.js (1)
543-554: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
DatabaseInfo.isMySQL()here. The schema layer already uses@tryghost/database-infofor dialect checks, so this avoids a directtransaction.client.config.client === 'mysql2'dependency and keeps the branching style consistent.🤖 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/data/schema/commands.js` around lines 543 - 554, The dialect check in createViewOrReplace is using a direct transaction.client.config.client comparison, which should be replaced with DatabaseInfo.isMySQL() for consistency with the schema layer. Update the branching in createViewOrReplace to use the existing `@tryghost/database-info` helper instead of checking for 'mysql2' directly, while keeping the same raw CREATE OR REPLACE behavior for MySQL and the schema.createViewOrReplace path for other dialects.
🤖 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 `@ghost/core/core/server/data/schema/commands.js`:
- Around line 543-554: The dialect check in createViewOrReplace is using a
direct transaction.client.config.client comparison, which should be replaced
with DatabaseInfo.isMySQL() for consistency with the schema layer. Update the
branching in createViewOrReplace to use the existing `@tryghost/database-info`
helper instead of checking for 'mysql2' directly, while keeping the same raw
CREATE OR REPLACE behavior for MySQL and the schema.createViewOrReplace path for
other dialects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02880f4a-0f57-48cf-9208-5e00a6916b02
📒 Files selected for processing (7)
apps/posts/src/views/members/member-query-params.test.tsapps/posts/src/views/members/member-query-params.tsghost/core/core/server/data/migrations/init/1-create-tables.jsghost/core/core/server/data/migrations/versions/6.50/2026-06-30-09-00-00-set-members-resolved-subscription-view-security-invoker.jsghost/core/core/server/data/schema/commands.jsghost/core/core/server/data/schema/views.jsghost/core/test/unit/server/data/schema/commands.test.js
rob-ghost
force-pushed
the
fix/ber-3756-subscription-view-portability
branch
from
June 30, 2026 10:46
904ffea to
ecaf46b
Compare
Contributor
There was a problem hiding this comment.
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/test/integration/migrations/view-security.test.js`:
- Around line 9-27: The current test only checks fresh schema creation via
startGhost(), so it does not verify the repair migration for existing installs.
Update the view-security test around members_resolved_subscription to first
reproduce the pre-6.50/default SECURITY DEFINER state (or load an older
fixture), then apply the
2026-06-30-09-00-00-set-members-resolved-subscription-view-security-invoker
migration and assert the resulting SECURITY_TYPE is INVOKER. Keep the assertion
tied to members_resolved_subscription and the migration path rather than the
initial startup state.
🪄 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: 10da1c02-0ebd-4dae-be2b-b9df32a274fa
📒 Files selected for processing (8)
apps/posts/src/views/members/member-query-params.test.tsapps/posts/src/views/members/member-query-params.tsghost/core/core/server/data/migrations/init/1-create-tables.jsghost/core/core/server/data/migrations/versions/6.50/2026-06-30-09-00-00-set-members-resolved-subscription-view-security-invoker.jsghost/core/core/server/data/schema/commands.jsghost/core/core/server/data/schema/views.jsghost/core/test/integration/migrations/view-security.test.jsghost/core/test/unit/server/data/schema/commands.test.js
✅ Files skipped from review due to trivial changes (1)
- ghost/core/core/server/data/schema/views.js
🚧 Files skipped from review as they are similar to previous changes (6)
- ghost/core/core/server/data/migrations/init/1-create-tables.js
- ghost/core/core/server/data/migrations/versions/6.50/2026-06-30-09-00-00-set-members-resolved-subscription-view-security-invoker.js
- ghost/core/core/server/data/schema/commands.js
- ghost/core/test/unit/server/data/schema/commands.test.js
- apps/posts/src/views/members/member-query-params.ts
- apps/posts/src/views/members/member-query-params.test.ts
rob-ghost
force-pushed
the
fix/ber-3756-subscription-view-portability
branch
from
June 30, 2026 10:53
ecaf46b to
dd35535
Compare
Contributor
Author
End-to-end demo: the bug, and that this PR fixes itThe PR already includes an automated CI proof (MySQL-only integration test in For anyone who wants to watch the failure and fix happen against a throwaway MySQL — including the customer-facing cross-user angle (an unprivileged app user, as on a real site, gets verify-invoker-end-to-end.sh#!/bin/bash
#
# End-to-end proof for BER-3756 / Ghost#28989.
#
# Reproduces the bug and proves the fix against a real MySQL 8, using the actual
# members_resolved_subscription view definition:
#
# Stage A view created with SQL SECURITY DEFINER bound to an account that
# does not exist on this server (what a restored dump looks like) —
# querying it as the app user fails with ERROR 1449. This is the
# failure that surfaces as Stripe webhook 500s on restored sites.
#
# Stage B same view recreated with SQL SECURITY INVOKER — exactly the
# statement commands.createViewOrReplace emits — queried as the same
# app user succeeds.
#
# Requires docker. Self-contained: starts a throwaway container and removes it.
set -uo pipefail
CONTAINER=ber3756-mysql
ROOTPW=rootpw
cleanup() { docker rm -f "$CONTAINER" >/dev/null 2>&1; }
trap cleanup EXIT
cleanup
echo "==> Starting MySQL 8..."
docker run -d --name "$CONTAINER" -e MYSQL_ROOT_PASSWORD="$ROOTPW" mysql:8.0 >/dev/null
echo -n "==> Waiting for MySQL to be ready"
for _ in $(seq 1 60); do
if docker exec "$CONTAINER" mysql -uroot -p"$ROOTPW" -e "SELECT 1" >/dev/null 2>&1; then
ready=1; break
fi
echo -n "."; sleep 2
done
echo
[ "${ready:-}" = 1 ] || { echo "MySQL did not start"; exit 1; }
run_root() { docker exec -i "$CONTAINER" mysql -uroot -p"$ROOTPW" "$@" 2>&1 | grep -v "Using a password"; }
run_app() { docker exec -i "$CONTAINER" mysql -ublog_new -papppw ghosttest "$@" 2>&1 | grep -v "Using a password"; }
echo "==> Creating schema, data, and a separate app user (no SUPER)..."
run_root <<'SQL'
CREATE DATABASE ghosttest;
USE ghosttest;
CREATE TABLE members_stripe_customers (member_id varchar(24), customer_id varchar(255));
CREATE TABLE members_stripe_customers_subscriptions (id varchar(24), customer_id varchar(255), status varchar(50), created_at datetime);
INSERT INTO members_stripe_customers VALUES ('m1','cus_1');
INSERT INTO members_stripe_customers_subscriptions VALUES
('sub_old','cus_1','canceled','2024-01-01 00:00:00'),
('sub_new','cus_1','active','2024-06-01 00:00:00');
CREATE USER 'blog_new'@'%' IDENTIFIED WITH mysql_native_password BY 'apppw';
GRANT SELECT ON ghosttest.* TO 'blog_new'@'%';
FLUSH PRIVILEGES;
SQL
echo "--- sanity: app user can connect and run a trivial query:"
SANITY=$(run_app -e "SELECT 1 AS app_can_connect")
echo "$SANITY"
echo "$SANITY" | grep -q "app_can_connect" || { echo " [FAIL] app user cannot connect — setup problem, not the view"; exit 1; }
VIEW_BODY="SELECT member_id, subscription_id FROM (
SELECT msc.member_id, mscs.id as subscription_id,
ROW_NUMBER() OVER (PARTITION BY msc.member_id ORDER BY
CASE WHEN mscs.status IN ('active','trialing','past_due','unpaid') THEN 0 ELSE 1 END,
mscs.created_at DESC, mscs.id ASC) as rn
FROM members_stripe_customers_subscriptions mscs
JOIN members_stripe_customers msc ON msc.customer_id = mscs.customer_id
WHERE mscs.status NOT IN ('incomplete','incomplete_expired')
) ranked WHERE rn = 1"
echo
echo "==> STAGE A: DEFINER-bound view referencing a non-existent account (a restored dump)"
run_root ghosttest -e "CREATE OR REPLACE DEFINER=\`ghost_old\`@\`10.0.0.1\` SQL SECURITY DEFINER VIEW members_resolved_subscription AS $VIEW_BODY"
echo "--- query as app user 'blog_new' (expect ERROR 1449):"
A_OUT=$(run_app -e "SELECT * FROM members_resolved_subscription")
echo "$A_OUT"
echo
echo "==> STAGE B: recreate with SQL SECURITY INVOKER (verbatim what the helper emits)"
run_root ghosttest -e "CREATE OR REPLACE SQL SECURITY INVOKER VIEW \`members_resolved_subscription\` AS $VIEW_BODY"
echo "--- query as app user 'blog_new' (expect the resolved row sub_new):"
B_OUT=$(run_app -e "SELECT * FROM members_resolved_subscription")
echo "$B_OUT"
echo "--- recorded SECURITY_TYPE in information_schema:"
run_root -e "SELECT table_name, security_type FROM information_schema.views WHERE table_name='members_resolved_subscription'" ghosttest
echo
echo "==> RESULT"
PASS=1
# Stage A: the app user connects fine (proven above), so any error here is the
# view itself being unusable because its DEFINER account is absent.
echo "$A_OUT" | grep -qiE "1449|1045|denied|does not exist" && echo " [ok] Stage A reproduced the failure (DEFINER view unusable by app user): $(echo "$A_OUT" | head -1)" || { echo " [FAIL] Stage A did not error as expected"; PASS=0; }
echo "$B_OUT" | grep -q "sub_new" && echo " [ok] Stage B returned the resolved subscription under the app user" || { echo " [FAIL] Stage B did not return data"; PASS=0; }
echo "$B_OUT" | grep -qiE "1449|1045|denied|error" && { echo " [FAIL] Stage B still errored"; PASS=0; }
[ "$PASS" = 1 ] && echo "==> END-TO-END PASS: INVOKER fixes the cross-user restore failure" || echo "==> END-TO-END FAIL"
exit $((1 - PASS))Captured runNote on error codes: the unprivileged app user sees |
rob-ghost
force-pushed
the
fix/ber-3756-subscription-view-portability
branch
2 times, most recently
from
June 30, 2026 11:13
5842cf0 to
0c0f792
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/posts/src/views/members/member-query-params.test.ts (1)
122-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the subscription date columns too.
The source change also moved
subscriptions.start_dateandsubscriptions.current_period_endtomember.current_subscription; adding assertions here would keep the fallback-removal contract covered for all affected subscription columns.🧪 Suggested test additions
it('reads status and plan from current_subscription', () => { const m = member({current_subscription: activeMonthly, subscriptions: [activeMonthly, cancelledYearly]}); expect(getActiveColumnValue({key: 'subscriptions.status', label: 'Status'}, m, 'UTC')?.text).toBe('Active'); expect(getActiveColumnValue({key: 'subscriptions.plan_interval', label: 'Plan'}, m, 'UTC')?.text).toBe('Monthly'); + expect(getActiveColumnValue({key: 'subscriptions.start_date', label: 'Start date'}, m, 'UTC')?.text).toBe('1 Jan 2024'); + expect(getActiveColumnValue({key: 'subscriptions.current_period_end', label: 'Current period end'}, m, 'UTC')?.text).toBe('1 Jan 2030'); });🤖 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 `@apps/posts/src/views/members/member-query-params.test.ts` around lines 122 - 134, The current test cases in member-query-params.test.ts cover status and plan, but not the subscription date fields moved to current_subscription. Update the existing member query param tests around getActiveColumnValue and member() to add assertions for subscriptions.start_date and subscriptions.current_period_end using current_subscription, and include null/absent cases if applicable so the fallback-removal behavior is covered for all affected subscription columns.
🤖 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 `@apps/posts/src/views/members/member-query-params.test.ts`:
- Around line 122-134: The current test cases in member-query-params.test.ts
cover status and plan, but not the subscription date fields moved to
current_subscription. Update the existing member query param tests around
getActiveColumnValue and member() to add assertions for subscriptions.start_date
and subscriptions.current_period_end using current_subscription, and include
null/absent cases if applicable so the fallback-removal behavior is covered for
all affected subscription columns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc5ad529-c031-4b4c-9457-b48bbe979e98
📒 Files selected for processing (3)
apps/posts/src/views/members/member-query-params.test.tsapps/posts/src/views/members/member-query-params.tsghost/core/core/server/data/schema/views.js
✅ Files skipped from review due to trivial changes (1)
- ghost/core/core/server/data/schema/views.js
Views were created without an explicit security context, so MySQL stamped them with SQL SECURITY DEFINER bound to the account that ran the migration. Such a view errors at query time once the database is restored under a different MySQL user (Ghost(Pro) restores, self-host server moves) because the original account does not exist on the target, and the account name leaks into every mysqldump. Route all view creation through a single commands.createViewOrReplace helper that forces SQL SECURITY INVOKER on MySQL, used by init, the new repair migration, and the test template builder (which otherwise recreated fork views with the default DEFINER security). SQLite has no such concept and is unchanged. Tests (MySQL): the shipped view is INVOKER; the 6.50 migration flips an existing DEFINER view to INVOKER (the upgrade/repair path); and a view whose definer account is absent fails with 1449 until the helper recovers it. A unit test also pins the helper's emitted SQL.
The current_subscription field has been returned by the backend for several releases (shipped in 6.45, now on 6.50), well beyond any deploy-skew window, so the admin no longer needs to resolve the current subscription locally. Drop the mostRelevantSubscription fallback. The remaining getCurrentSubscription wrapper then only re-coalesced an already-optional field, and every call site optional-chains its result, so inline member.current_subscription directly and remove the wrapper too.
rob-ghost
force-pushed
the
fix/ber-3756-subscription-view-portability
branch
from
June 30, 2026 11:33
0c0f792 to
02abe40
Compare
kevinansfield
approved these changes
Jun 30, 2026
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
2 participants
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.
ref BER-3756
Problem
members_resolved_subscription(the first VIEW Ghost ships, added in 6.45) is created with no explicit security context. MySQL therefore stamps it withSQL SECURITY DEFINERbound to the account that ran the migration. That makes the view non-portable:The user specified as the definer ... does not exist). Because the resolved-subscription lookup is touched on the Stripe subscription webhook path, this surfaces as repeated webhook failures on an otherwise healthy-looking site.mysqldump, including the archives handed to customers.This is a brand-new failure mode: before this view, Ghost shipped no views/procedures/triggers, so dumps carried no
DEFINERat all.Fix
commands.createViewOrReplacehelper that forcesSQL SECURITY INVOKERon MySQL (the view runs with the querying user's privileges and binds to no specific account). SQLite has no such concept and keeps the plain builder.knex-migrator initand versioned migrations — through the helper, so every view is portable by default and future views inherit the behaviour.members_resolved_subscriptionin place (CREATE OR REPLACE) so existing installs are fixed without dropping the view.SQL SECURITY INVOKERand noDEFINER; SQLite uses the plain builder.Also: removed a now-unneeded fallback
getCurrentSubscriptioninapps/postskept a localmostRelevantSubscriptionresolver as a fallback for the deploy-skew window before the backend returnedcurrent_subscription. That field has shipped since 6.45 (now 6.50), well beyond the window, so the fallback is removed and the field is read directly. Separate commit; easy to split out if preferred.Notes
DEFINERvalue from existing dumps/archives is handled separately in the Ghost(Pro) backup tooling (also tracked under BER-3756).mostRelevantSubscriptionhelper in the legacy Ember admin (ghost/admin) is untouched.Test plan
yarn workspace ghost test— schema command unit tests (newcreateViewOrReplacecases)information_schema.views.security_typeformembers_resolved_subscriptionisINVOKERafter migrateapps/postsunit tests formember-query-params