🐛 Fixed expired sessions leaving Admin stuck instead of returning to signin#29328
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe API response handler now throws 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin-x-settings:test:acceptance |
✅ Succeeded | 9m 7s | View ↗ |
nx run-many -t test:unit -p @tryghost/admin-x-f... |
✅ Succeeded | 4m 52s | View ↗ |
nx run ghost-admin:test |
✅ Succeeded | 3m 1s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 2m 45s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 19s | View ↗ |
nx run-many -t lint -p @tryghost/admin-x-framew... |
✅ Succeeded | 1m 35s | View ↗ |
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 1m 28s | View ↗ |
nx run @tryghost/activitypub:test:acceptance |
✅ Succeeded | 50s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-15 17:17:01 UTC
![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)
apps/admin-x-framework/src/hooks/use-handle-error.ts (1)
54-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider excluding session expirations from Sentry logging.
Session expiry is a normal and expected user event. Since
UnauthorizedErrorfalls underAPIError, it will still be caught by the Sentry logging block higher up in this function. Logging it will likely create unnecessary noise in your error tracker.Consider wrapping the Sentry capture logic to exclude unauthorized errors:
// Around line 41: if (!(error instanceof UnauthorizedError)) { Sentry.captureException(error); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/admin-x-framework/src/hooks/use-handle-error.ts` around lines 54 - 57, Update the Sentry capture logic in the error-handling function to skip errors that are instances of UnauthorizedError, while continuing to capture all other errors. Keep the existing unauthorized-error toast removal and redirect behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/admin-x-framework/src/utils/api/fetch-api.ts`:
- Around line 53-70: Update SESSION_API_REQUEST so its boundary matcher accepts
a slash, query string, fragment, or end of URL after the session path, such as
using ([/?#]|$). Keep redirectOnSessionExpiry unchanged so session requests with
query parameters or fragments are excluded from the expiry redirect.
---
Nitpick comments:
In `@apps/admin-x-framework/src/hooks/use-handle-error.ts`:
- Around line 54-57: Update the Sentry capture logic in the error-handling
function to skip errors that are instances of UnauthorizedError, while
continuing to capture all other errors. Keep the existing unauthorized-error
toast removal and redirect behavior unchanged.
🪄 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: 7f833ee4-33f3-494d-90ae-c7e1d9eb5149
📒 Files selected for processing (7)
apps/admin-x-framework/src/hooks/use-handle-error.tsapps/admin-x-framework/src/utils/api/fetch-api.tsapps/admin-x-framework/src/utils/api/handle-response.tsapps/admin-x-framework/src/utils/errors.tsapps/admin-x-framework/test/unit/hooks/use-handle-error.test.tsxapps/admin-x-framework/test/unit/utils/api/session-expiry.test.tsxapps/admin-x-framework/test/utils/mock-fetch.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29328 +/- ##
=======================================
Coverage 74.03% 74.03%
=======================================
Files 1577 1577
Lines 136821 136818 -3
Branches 16574 16574
=======================================
- Hits 101299 101298 -1
+ Misses 34516 34514 -2
Partials 1006 1006
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:
|
…signin ref https://linear.app/ghost/issue/PLA-188/add-401-session-expiry-handling-to-the-admin-x-framework-fetch-layer - the React admin fetch layer had no 401 handling, so an expired session surfaced as a generic error toast and left the page dead - a 401 on an Admin API request (excluding the session endpoint, so failed signins are unaffected) now triggers a single full-page replace to the admin root - the same mechanism Ember's ajax service uses - which reboots Admin unauthenticated and lands on signin; this works for every framework consumer without depending on the Ember bridge - handleResponse was returned un-awaited, so response errors skipped the fetch layer's catch entirely; awaiting it also makes the documented MaintenanceError (503) retry reachable
ref https://linear.app/ghost/issue/PLA-188/ Only redirected Admin API 401s should be hidden; other unauthorized responses still need visible error feedback. Session URL query and fragment boundaries must also remain inside the authentication flow.
ref https://linear.app/ghost/issue/PLA-188/ - Core returns 403 Authorization failed when an authenticated session expires - only that exact forbidden response should trigger the existing signin redirect
948f268 to
061dae5
Compare
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 29431898400 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
ref https://linear.app/ghost/issue/PLA-188/ - unauthenticated Admin routes legitimately receive authorization failures while booting - keep session-expiry suppression without replacing a page that is already returning to sign-in
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 29433506363 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
ref https://linear.app/ghost/issue/PLA-188/ - 401 bodies are parsed when JSON so consumers reading error.data keep the API's structured errors instead of an unparsed string - #/reset/:token is an unauthenticated Ember route, so it now skips the expiry redirect like signin/signup/setup - split the session-expiry check from the redirect side effect so the classification in fetch-api reads directly - covered the 503 retry path with a test, since awaiting handleResponse made it live
1 participant
ref https://linear.app/ghost/issue/PLA-188/
The React Admin fetch layer (
admin-x-framework) did not handle expired sessions. Core returns 403 +Authorization failedwhen a cookie session is no longer valid, so requests fell through asValidationError, displayed a generic toast, and left the page stuck. Ember's Ajax service already treats that response, as well as 401 responses, as an invalid session and returns to sign-in.What this does
handle-response.tsclassifies both 401 responses and the specific 403 +Authorization failedAdmin API response asUnauthorizedError.fetch-api.tsconverts those errors toSessionExpiredErrorfor Ghost API requests and, from authenticated Admin routes, performs a full-pagewindow.location.replaceto the Admin root. Admin then revalidates throughusers/meand lands on sign-in./ghost/api/. Session endpoints such as/session/and/session/verify/are excluded so failed sign-in and verification requests do not redirect, and external/Tinybird requests cannot log the user out.#/setup,#/signin,#/signup, and#/reset) still classify the response asSessionExpiredErrorfor toast/Sentry suppression, but do not replace the page. This lets Ember finish booting the authentication UI without a reload loop.useHandleErrorsuppresses toast and Sentry reporting specifically forSessionExpiredError, avoiding noise while the page unloads or authentication UI boots.ValidationErrorand keep their existing behavior.This works for all framework consumers (Admin, Admin X Settings, and ActivityPub) without relying on the Ember bridge, which exposes no session-invalidation method.
Latent bug fixed en route
fetch-api.tsreturnedhandleResponse(response)without awaiting it, so response-derived errors bypassed its retry and error-handlingcatch. Awaiting it makes the existingMaintenanceErrorretry path functional and allows session errors to be classified centrally. Two incomplete Response fakes intest/utils/mock-fetch.tswere completed as fallout.Behavioral changes / reviewer callouts
Authorization failedresponse when encountered on an authenticated route.UnauthorizedError, including excluded session endpoints and external requests; those requests do not redirect.UnauthorizedError; unrelated 403 responses are unchanged.handleResponsemakes the pre-existing 503 retry behavior live. With retries enabled in production, this applies to mutations as well as reads, so a mutation returning 503 may be submitted again for up to 15 seconds.Verification
admin-x-framework: 32 test files / 455 tests passing.