Skip to content

🎨 Added warning log for signin attempts on unknown members#28068

Closed
leafwind wants to merge 1 commit into
TryGhost:mainfrom
leafwind:main
Closed

🎨 Added warning log for signin attempts on unknown members#28068
leafwind wants to merge 1 commit into
TryGhost:mainfrom
leafwind:main

Conversation

@leafwind
Copy link
Copy Markdown
Contributor

When a signin magic link is requested for an email with no associated member, Ghost silently returns 201 without sending an email. This makes it hard for admins to diagnose why users report not receiving magic links. The log only records the email domain to avoid storing personal data.

  • I've read and followed the Contributor Guide
  • I've explained my change
  • I've written an automated test to prove my change works

When a signin magic link is requested for an email with no associated
member, Ghost silently returns 201 without sending an email. This makes
it hard for admins to diagnose why users report not receiving magic
links. The log only records the email domain to avoid storing personal
data.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2081d242-af68-4d20-a971-36b38708af34

📥 Commits

Reviewing files that changed from the base of the PR and between 6be7a48 and 4b8c6fe.

📒 Files selected for processing (2)
  • ghost/core/core/server/services/members/members-api/controllers/router-controller.js
  • ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js

Walkthrough

This PR adds warning-level logging to the RouterController._handleSignin method when a signin attempt is made for a member that does not exist in the repository. The logged warning includes the email domain extracted from the normalized email address, providing observability without exposing the full email. Test coverage verifies the warning is only logged for unknown members and that the message does not contain the complete email value.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding a warning log for signin attempts on unknown members, which aligns with the actual code modifications.
Description check ✅ Passed The description is directly related to the changeset, explaining the motivation for logging warnings on failed signin attempts and clarifying privacy considerations.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

@9larsons
Copy link
Copy Markdown
Contributor

9larsons commented Jun 6, 2026

Hey @leafwind, thanks for the PR. I'm going to close this since I think there's a couple challenges, though you're welcome to reopen and discuss:

  1. Logging just the domain is of limited value for high-volume or common domains
  2. This could get quite verbose; it might be better to make this debug if you feel strongly about it
  3. If we're having trouble tracking a single person, it feels like we need the PII, which is a different question altogether

If users report not receiving magic links, I think you'll want to look into SMTP logs.

@9larsons 9larsons closed this Jun 6, 2026
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.

2 participants