🐛 Fixed deep-linking to Admin screens whilst logged out#27316
🐛 Fixed deep-linking to Admin screens whilst logged out#27316kevinansfield merged 3 commits intomainfrom
Conversation
ref https://linear.app/ghost/issue/BER-3337/ When deep-linking to an admin route (e.g. /ghost/#/tags) while logged out, the page rendered blank instead of showing the signin screen. This was caused by the react-fallback catch-all route lacking the authentication check that was previously on the now-removed Ember route files. - Added react-fallback route extending AuthenticatedRoute - Persisted intended URL in sessionStorage before the auth redirect so the user is returned to their original destination after signing in - Made LoginPage.logout() wait for the signin page to fully load before resolving to avoid race conditions with subsequent navigations - Added e2e tests covering signin redirect for both React and Ember routes
kevinansfield
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAuthenticatedRoute.beforeModel now stores the attempted URL in window.sessionStorage under 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
kevinansfield
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/helpers/pages/admin/posts/posts-page.ts (1)
47-47: Consider an explicit return type for consistency.
waitForPageToFullyLoad()could be declared asasync waitForPageToFullyLoad(): Promise<void>to match the surrounding method style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/posts/posts-page.ts` at line 47, The method waitForPageToFullyLoad is missing an explicit return type; update its signature to include Promise<void> (declare async waitForPageToFullyLoad(): Promise<void>) to match surrounding methods and improve type consistency, ensuring any callers still work and no return values are added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/helpers/pages/admin/posts/posts-page.ts`:
- Line 47: The method waitForPageToFullyLoad is missing an explicit return type;
update its signature to include Promise<void> (declare async
waitForPageToFullyLoad(): Promise<void>) to match surrounding methods and
improve type consistency, ensuring any callers still work and no return values
are added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bcfdaed-15ae-4628-9593-71f1f4c70ff9
📒 Files selected for processing (6)
e2e/helpers/pages/admin/login-page.tse2e/helpers/pages/admin/posts/posts-page.tse2e/tests/admin/signin.test.tsghost/admin/app/routes/authenticated.jsghost/admin/app/routes/react-fallback.jsghost/admin/app/services/session.js
- Always clear ghost-signin-redirect from sessionStorage after reading - Filter out /signin, /signup, /setup URLs from redirect targets - Updated authentication test expectation for react-fallback auth check
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/admin/app/services/session.js`:
- Around line 91-96: The early return bypasses ember-simple-auth's cleanup; call
super.handleAuthentication('home') before performing the custom redirect so ESA
can clear attemptedTransition and redirect targets. Concretely: after reading
and removing 'ghost-signin-redirect' (redirectUrl and
window.sessionStorage.removeItem) but before router.transitionTo(redirectUrl)
and return, invoke and await super.handleAuthentication('home') (or call
super.handleAuthentication if not async) so ESA's internal cleanup runs, then
proceed with this.router.transitionTo(redirectUrl) and return.
🪄 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: 87030c2f-45b0-44e5-81a5-528dcb20a645
📒 Files selected for processing (2)
ghost/admin/app/services/session.jsghost/admin/tests/acceptance/authentication-test.js
| const redirectUrl = window.sessionStorage.getItem('ghost-signin-redirect'); | ||
| window.sessionStorage.removeItem('ghost-signin-redirect'); | ||
| if (redirectUrl && !redirectUrl.startsWith('/signin') && !redirectUrl.startsWith('/signup') && !redirectUrl.startsWith('/setup')) { | ||
| this.router.transitionTo(redirectUrl); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the ember-simple-auth version used by Ghost admin, does SessionService.handleAuthentication clear attemptedTransition (or equivalent state), and what side effects are skipped if an override returns before calling super.handleAuthentication()?
💡 Result:
Ghost Admin pins ember-simple-auth to 5.0.0 in ghost/admin/package.json. [1]
Does SessionService.handleAuthentication clear attemptedTransition (or equivalent)?
In ember-simple-auth, SessionService.handleAuthentication(routeAfterAuthentication) does not directly clear state itself—it delegates to an internal helper (handleSessionAuthenticated(...)). [2]
That helper:
- retries
attemptedTransitionif present, then clears it viasessionService.set('attemptedTransition', null), or - if there’s no
attemptedTransitionbut there is a persisted redirect target, it transitions there and clears the redirect target, otherwise - transitions to
routeAfterAuthentication. [3]
So: yes, the “attempted transition” is cleared, but it’s cleared inside the internal handleSessionAuthenticated flow that handleAuthentication calls. [2][3]
If an override returns before calling super.handleAuthentication(), what side effects are skipped?
You skip all of the default post-login routing/cleanup behavior, specifically: [2][3]
- No
attemptedTransition.retry() - No
attemptedTransitionreset tonull(so it can remain stale in memory) - No redirect to the persisted redirect target (if set)
- No
clearRedirectTarget()(so the stored redirect target can persist and affect later navigations) - No fallback transition to
routeAfterAuthentication(so you may remain on the login route unless you handle navigation yourself)
Bottom line: returning early means you must perform the retry/redirect + cleanup yourself, or you risk stale redirect state and “wrong” redirects later. [3]
Sources
[1] Ghost Admin package.json (Ghost v5.84.2) showing ember-simple-auth: "5.0.0". (github.com)
[2] ember-simple-auth SessionService.handleAuthentication delegates to handleSessionAuthenticated. (ember-simple-auth.com)
[3] ember-simple-auth internal handleSessionAuthenticated retries & clears attemptedTransition, and clears redirect target when used. (github.com)
Citations:
- 1: https://github.com/TryGhost/Ghost/blob/v5.84.2/ghost/admin/package.json
- 2: https://ember-simple-auth.com/api/services_session.ts.html
- 3: https://github.com/mainmatter/ember-simple-auth/blob/8c813680/packages/ember-simple-auth/src/-internals/routing.js
🏁 Script executed:
# First, find the session.js file
find . -type f -name "session.js" | grep -E "ghost/admin"Repository: TryGhost/Ghost
Length of output: 1867
Preserve ESA transition-state cleanup when taking the custom redirect path.
Line 95 returns before super.handleAuthentication('home'). In ember-simple-auth v5.0.0, handleAuthentication() triggers internal cleanup that clears attemptedTransition and the redirect target. By returning early, this cleanup is bypassed, leaving stale transition state that can cause unexpected redirects on future auth events.
Suggested patch
const redirectUrl = window.sessionStorage.getItem('ghost-signin-redirect');
window.sessionStorage.removeItem('ghost-signin-redirect');
if (redirectUrl && !redirectUrl.startsWith('/signin') && !redirectUrl.startsWith('/signup') && !redirectUrl.startsWith('/setup')) {
+ // Avoid leaving stale ESA transition state when bypassing super.handleAuthentication()
+ this.attemptedTransition = null;
this.router.transitionTo(redirectUrl);
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/admin/app/services/session.js` around lines 91 - 96, The early return
bypasses ember-simple-auth's cleanup; call super.handleAuthentication('home')
before performing the custom redirect so ESA can clear attemptedTransition and
redirect targets. Concretely: after reading and removing 'ghost-signin-redirect'
(redirectUrl and window.sessionStorage.removeItem) but before
router.transitionTo(redirectUrl) and return, invoke and await
super.handleAuthentication('home') (or call super.handleAuthentication if not
async) so ESA's internal cleanup runs, then proceed with
this.router.transitionTo(redirectUrl) and return.
|
1 participant
ref https://linear.app/ghost/issue/ONC-1623/
When deep-linking to an admin route (e.g.
/ghost/#/tags) while logged out, the page rendered blank instead of showing the signin screen. This was caused by thereact-fallbackcatch-all route lacking the authentication check that was previously on the now-removed Ember route files.react-fallbackroute extendingAuthenticatedRouteto enforce signin for all React-rendered routessessionStoragebefore the auth redirect so the user is returned to their original destination after signing in (works for both React and Ember routes)LoginPage.logout()wait for the signin page to fully load before resolving to avoid race conditions with subsequent navigations in e2e tests/tags) and Ember (/posts) routes