🎨 Fixed infinite wait when auth code email fails to send in Direct mode #23610
Conversation
fixes TryGhost#23050 When email is configured to use Direct mode, it can take over 15 minutes for the email to actually fail to send. When a user attempts to sign in on a new device in this situation, the login screen will appear to be stuck with no feedback. This change adds a timeout in Direct mode to the sendAuthCodeToUser function. If the email has not been sent after 15 seconds, an error is thrown, which will then surface the 'Failed to send email. Please check your site configuration and try again.', which will will then lead them to either setting up email correctly or disabling the email 2FA.
WalkthroughThe changes update the Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/auth/session/session-service.test.js (1)
484-489: Fix code formatting in promise callbacks.The promise callbacks have formatting issues that violate ESLint rules.
Apply this diff to fix the formatting:
- const mailer = { - send: sinon.stub().returns(new Promise((resolve) => { setTimeout(() => resolve(), 20000) })), - state: { - usingDirect: true - } - }; + const mailer = { + send: sinon.stub().returns(new Promise((resolve) => { + setTimeout(() => resolve(), 20000); + })), + state: { + usingDirect: true + } + };And similarly for line 552:
- const mailer = { - send: sinon.stub().returns(new Promise((resolve) => { setTimeout(() => resolve(), 20000) })), - state: { - usingDirect: false - } - }; + const mailer = { + send: sinon.stub().returns(new Promise((resolve) => { + setTimeout(() => resolve(), 20000); + })), + state: { + usingDirect: false + } + };Also applies to: 551-556
🧰 Tools
🪛 ESLint
[error] 485-485: Statement inside of curly braces should be on next line.
(brace-style)
[error] 485-485: Missing semicolon.
(semi)
[error] 485-485: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/services/auth/session/session-service.js(1 hunks)ghost/core/test/unit/server/services/auth/session/session-service.test.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ghost/core/core/server/services/auth/session/session-service.js (3)
ghost/core/test/unit/server/services/auth/session/session-service.test.js (5)
mailer(381-383)mailer(431-433)mailer(484-489)mailer(551-556)mailer(653-655)ghost/core/core/server/services/auth/session/index.js (1)
mailer(39-39)ghost/core/test/e2e-api/admin/session.test.js (2)
token(155-155)
ghost/core/test/unit/server/services/auth/session/session-service.test.js (2)
ghost/core/test/e2e-api/admin/sso.test.js (1)
sinon(4-4)ghost/core/core/server/services/auth/session/index.js (3)
mailer(39-39)urlUtils(12-12)sessionService(41-61)
🪛 ESLint
ghost/core/core/server/services/auth/session/session-service.js
[error] 282-283: Missing semicolon.
(semi)
[error] 286-286: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
ghost/core/test/unit/server/services/auth/session/session-service.test.js
[error] 485-485: Statement inside of curly braces should be on next line.
(brace-style)
[error] 485-485: Missing semicolon.
(semi)
[error] 485-485: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
[error] 552-552: Statement inside of curly braces should be on next line.
(brace-style)
[error] 552-552: Missing semicolon.
(semi)
[error] 552-552: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
🔇 Additional comments (3)
ghost/core/test/unit/server/services/auth/session/session-service.test.js (3)
16-18: Good addition for test isolation!Adding
sinon.restore()in the afterEach hook ensures proper cleanup between tests.
461-527: Well-implemented test for Direct mode timeout!The test correctly verifies that the 15-second timeout is enforced when using Direct mode. Good use of fake timers and proper promise handling with
setImmediate.🧰 Tools
🪛 ESLint
[error] 485-485: Statement inside of curly braces should be on next line.
(brace-style)
[error] 485-485: Missing semicolon.
(semi)
[error] 485-485: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
529-592: Good test coverage for non-Direct mode!The test correctly verifies that the timeout is not applied when not using Direct mode, ensuring the email can take longer than 15 seconds to send.
🧰 Tools
🪛 ESLint
[error] 552-552: Statement inside of curly braces should be on next line.
(brace-style)
[error] 552-552: Missing semicolon.
(semi)
[error] 552-552: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
| let sendPromise = mailer.send({ | ||
| to: recipient, | ||
| subject: `${token} is your Ghost sign in verification code`, | ||
| html: email | ||
| }); | ||
| }) | ||
| // In Direct mode, it can take 15+ minutes for the mailer to actually fail to send. | ||
| // Compromise by giving up after 15 seconds. | ||
| if (mailer.state?.usingDirect) { | ||
| sendPromise = Promise.race([sendPromise, new Promise((_resolve, reject) => setTimeout(reject, 15000))]); | ||
| } | ||
| await sendPromise; |
There was a problem hiding this comment.
Fix the promise executor return value issue.
The implementation correctly adds a 15-second timeout for Direct mode. However, the promise executor should not return the setTimeout value.
Apply this diff to fix the ESLint warning:
- if (mailer.state?.usingDirect) {
- sendPromise = Promise.race([sendPromise, new Promise((_resolve, reject) => setTimeout(reject, 15000))]);
- }
+ if (mailer.state?.usingDirect) {
+ sendPromise = Promise.race([sendPromise, new Promise((_resolve, reject) => {
+ setTimeout(reject, 15000);
+ })]);
+ }📝 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.
| let sendPromise = mailer.send({ | |
| to: recipient, | |
| subject: `${token} is your Ghost sign in verification code`, | |
| html: email | |
| }); | |
| }) | |
| // In Direct mode, it can take 15+ minutes for the mailer to actually fail to send. | |
| // Compromise by giving up after 15 seconds. | |
| if (mailer.state?.usingDirect) { | |
| sendPromise = Promise.race([sendPromise, new Promise((_resolve, reject) => setTimeout(reject, 15000))]); | |
| } | |
| await sendPromise; | |
| let sendPromise = mailer.send({ | |
| to: recipient, | |
| subject: `${token} is your Ghost sign in verification code`, | |
| html: email | |
| }) | |
| // In Direct mode, it can take 15+ minutes for the mailer to actually fail to send. | |
| // Compromise by giving up after 15 seconds. | |
| if (mailer.state?.usingDirect) { | |
| sendPromise = Promise.race([sendPromise, new Promise((_resolve, reject) => { | |
| setTimeout(reject, 15000); | |
| })]); | |
| } | |
| await sendPromise; |
🧰 Tools
🪛 ESLint
[error] 282-283: Missing semicolon.
(semi)
[error] 286-286: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
🤖 Prompt for AI Agents
In ghost/core/core/server/services/auth/session/session-service.js around lines
278 to 288, the promise executor function passed to the new Promise incorrectly
returns the setTimeout ID, causing an ESLint warning. To fix this, modify the
executor function so it does not return the setTimeout call; instead, just call
setTimeout inside the executor without returning its value.
fixes #23050
When email is configured to use Direct mode, it can take over 15 minutes for the email to actually fail to send. When a user attempts to sign in on a new device in this situation, the login screen will appear to be stuck with no feedback.
This is especially problematic because the Docker installation sets up Direct mode for you automatically, and most hosts / ISPs block outbound port 25. This likely means almost any self hoster that has not taken the step of explicitly setting up email will unknowingly be unable to log in to from a new device, and will have difficulty even discovering why.
This change adds a timeout in Direct mode to the sendAuthCodeToUser function. If the email has not been sent after 15 seconds, an error is thrown, which will then surface the 'Failed to send email. Please check your site configuration and try again.', which will will then lead them to either setting up email correctly or disabling the email 2FA.
Please check your PR against these items:
We appreciate your contribution! 🙏