🎨 Fixed infinite wait when auth code email fails to send in Direct mode #23610
Conversation
|
""" 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)
ghost/core/core/server/services/auth/session/session-service.js
Outdated
Show resolved
Hide resolved
PaulBernhardt
force-pushed
the
23050-auth-code-direct-email-timeout
branch
from
855f4bf to
e458f4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
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)
🪛 ESLint
ghost/core/core/server/services/auth/session/session-service.js
[error] 282-283: Missing semicolon.
(semi)
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.The
afterEachhook properly restores Sinon stubs and mocks, ensuring test isolation and preventing potential test interference.
461-527: Comprehensive test for Direct mode timeout behavior.The test correctly verifies that the 15-second timeout is enforced in Direct mode using fake timers. The test setup with a 20-second delay and 15-second timeout advancement properly validates the timeout functionality.
🧰 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: Proper test for non-Direct mode behavior.This test correctly verifies that the timeout logic doesn't interfere when
usingDirectis false, ensuring the mailer can take longer than 15 seconds to complete in non-Direct modes.🧰 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)
ghost/core/core/server/services/auth/session/session-service.js
Outdated
Show resolved
Hide resolved
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.
PaulBernhardt
force-pushed
the
23050-auth-code-direct-email-timeout
branch
from
e458f4d to
b8bc077
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/test/unit/server/services/auth/session/session-service.test.js (2)
484-489: Minor ESLint violations & unnecessary verbosity inside the Promise factoryThe inline arrow function
{ setTimeout(() => resolve(), 20000) }trips the brace-style / semicolon rules and is a bit wordy. A terse one-liner keeps the linter and the reader happy.- send: sinon.stub().returns(new Promise((resolve) => { setTimeout(() => resolve(), 20000) })), + send: sinon.stub().returns(new Promise(resolve => setTimeout(resolve, 20_000))),Apply the same change to the duplicate stub in the second test block.
No behaviour change, just lint- & readability polish.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)
461-527: Optional: explicitly restore the faked clock to avoid nested timer stubs
sinon.restore()does restore the default sandbox clock, but being explicit makes the intent obvious and guards against future refactors where a custom sandbox might be introduced.@@ await should(resultPromise).... }); + clock.restore(); // be explicit – prevents nested fake timers when tests are reordered });Same advice applies to the non-direct-mode test below.
🧰 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/auth/session/session-service.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
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/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 (1)
ghost/core/test/unit/server/services/auth/session/session-service.test.js (1)
16-18: Good call adding a global cleanup hook
afterEach(() => sinon.restore())guarantees all spies/stubs are reset between tests and (since Sinon ≥ 9) also restores any fake timers – solid test-hygiene.
| // Advance the clock by the expected timeout of 15 seconds | ||
| clock.tick(20000); | ||
|
|
||
| // Give our promises a chance to resolve | ||
| await new Promise(setImmediate); | ||
|
|
||
| await should(resultPromise).resolved(); | ||
| }); |
There was a problem hiding this comment.
resolved() is not a Should.js assertion – use fulfilled() instead
The Should.js promise API exposes .fulfilled()/.resolvedWith() but not bare .resolved().
The current call will throw and fail the suite.
- await should(resultPromise).resolved();
+ await resultPromise.should.be.fulfilled();This mirrors the pattern used earlier in the file (.should.be.fulfilled()) and keeps the test green.
📝 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.
| // Advance the clock by the expected timeout of 15 seconds | |
| clock.tick(20000); | |
| // Give our promises a chance to resolve | |
| await new Promise(setImmediate); | |
| await should(resultPromise).resolved(); | |
| }); | |
| // Advance the clock by the expected timeout of 15 seconds | |
| clock.tick(20000); | |
| // Give our promises a chance to resolve | |
| await new Promise(setImmediate); | |
| await resultPromise.should.be.fulfilled(); | |
| }); |
🤖 Prompt for AI Agents
In ghost/core/test/unit/server/services/auth/session/session-service.test.js
around lines 585 to 592, the test uses the non-existent Should.js assertion
`.resolved()` on a promise, which causes the test to fail. Replace `.resolved()`
with `.fulfilled()` to correctly assert that the promise resolves successfully,
matching the existing pattern used earlier in the file and ensuring the test
passes.
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! 🙏