Fixed 500 on newsletter verification with a non-newsletter token - #30281
Fixed 500 on newsletter verification with a non-newsletter token#302819larsons wants to merge 1 commit into
Conversation
ref https://linear.app/ghost/issue/PLA-375 Newsletter and settings email verification links are issued by two separate MagicLink services that share one single-use token store, so a support address token decodes cleanly on the newsletter side. Its payload is `{key, value}` rather than `{id, property, value}`, which left the model edit with an undefined id and knex throwing "Undefined binding(s) detected" as a 500. Validating the token payload up front mirrors the key allowlist the settings service already applies in verifyKeyUpdate, and turns the crash into a 400.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 3m 6s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 31s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 2s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 59s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 32s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 22s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 20s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 4s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-25 14:15:08 UTC
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (9)
🧰 Additional context used📓 Path-based instructions (7)Review new or changed service boundaries for explicit dependency ownership,⚙️ CodeRabbit configuration file Files:
Review whether tests prove changed behaviour, meaningful error/edge paths, and⚙️ CodeRabbit configuration file Files:
New source files must be TypeScript: flag new JS files as a required change⚙️ CodeRabbit configuration file Files:
Prioritise concrete correctness, security, data-integrity, compatibility,⚙️ CodeRabbit configuration file Files:
Boot owns service initialization; do not📄 CodeRabbit inference engine (AGENTS.md) Files:
New files are TypeScript: Fail if the PR adds a new .js/.jsx/.cjs/.mjs source file, unless it is: a DB📄 CodeRabbit inference engine (Custom checks) Files:
Always use `pnpm`, never npm or Yarn.📄 CodeRabbit inference engine (AGENTS.md) Files:
🔇 Additional comments (4)
WalkthroughThe newsletter service adds an invalid-token error message and allows token-based updates only for Merge Risk: ⚪ Minimal · up to The PR validates newsletter verification token payloads and returns a clean bad-request response instead of a raw 500 for invalid tokens. No actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Full details: Type-Safe BoundariesExplanation The PR adds a hand-written check for decoded token data instead of a Zod boundary schema. The endpoint receives Resolution Define a Zod schema for the newsletter verification token payload. Validate the result of Full details: New Files Are TypescriptExplanation The pull request adds no files. The exact parent-to-HEAD diff contains only two modified ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30281 +/- ##
==========================================
- Coverage 75.48% 75.44% -0.05%
==========================================
Files 1641 1641
Lines 155761 155776 +15
Branches 18772 18757 -15
==========================================
- Hits 117577 117519 -58
- Misses 37176 37274 +98
+ Partials 1008 983 -25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
ref https://linear.app/ghost/issue/PLA-375
Why
The newsletters service and the settings BREAD service each construct their own
MagicLinkwith a separateSingleUseTokenProvider, but both providers wrap the sameSingleUseTokenmodel. There's no type discriminator on the token, so a support address verification token validates cleanly against the newsletter verification endpoint.The payloads differ: settings tokens carry
{key, value}, whileverifyPropertyUpdatedestructures{id, property, value}. A support token therefore leavesidundefined, which reachesNewsletter.edit(attrs, {id: undefined})and makes knex throw at query-compile time:That's a raw error rather than a Ghost API error, so it surfaces as a 500. It never degrades into a clean 404, because the throw happens before the row lookup runs.
What changed
verifyPropertyUpdatenow validates the token payload before touching the model:idmust be present, andpropertymust be one ofsender_email/sender_reply_to. Anything else gets aBadRequestError.This mirrors the guard the settings service already applies in
verifyKeyUpdate, which rejects keys outside itsEMAIL_KEYSallowlist for the same reason — tokens issued for another flow, or by an older version.Notes for reviewers
BadRequestErrorand that the model is never touched.test/unit/server/services/newsletters/service.test.jspasses (17 tests), eslint and oxfmt clean.