🐛 Fixed ActivityPub /ghost/api/admin/site/ access with separated admin setup
#24630
Conversation
WalkthroughReplaces the adminRedirect factory with a direct handler handleAdminRedirect(req, res) that derives the admin target by stripping a leading /ghost from req.path (defaulting to /) and calls urlUtils.redirectToAdmin(301, res, adminPath). The middleware now exposes a single route matching /^/ghost(/.*)?/?$/ wired to that handler and exports handleAdminRedirect for testing. Adds unit tests for the handler and route matching, multiple end-to-end tests covering /ghost redirects under different admin:redirects and subdirectory/admin-host configurations, and updates legacy tests to expect redirect semantics for API vhost cases. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
kevinansfield
force-pushed
the
add-frontend-ghost-redirects
branch
from
9d0cf30 to
55f1bb1
Compare
![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 (5)
ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js (2)
6-9: Solid extraction of core redirect logic; consider normalizing double slashes in derived pathIf someone hits /ghost//..., adminPath becomes '//' which may propagate to the final URL. Normalize to avoid accidental double slashes.
-const handleAdminRedirect = function (req, res) { - const adminPath = req.path.replace(/^\/ghost/, '') || '/'; - return urlUtils.redirectToAdmin(301, res, adminPath); -}; +const handleAdminRedirect = function (req, res) { + const adminPathRaw = req.path.replace(/^\/ghost/, '') || '/'; + const adminPath = adminPathRaw.replace(/\/{2,}/g, '/'); + return urlUtils.redirectToAdmin(301, res, adminPath); +};
16-16: Optional: Also handle HEAD requests explicitlyExpress auto-handles HEAD for GET, but adding an explicit HEAD route can make intent clearer and future-proof.
- router.get(/^\/ghost(\/.*)?\/?$/, handleAdminRedirect); + router.get(/^\/ghost(\/.*)?\/?$/, handleAdminRedirect); + router.head(/^\/ghost(\/.*)?\/?$/, handleAdminRedirect);ghost/core/test/e2e-frontend/advanced_url_config.test.js (1)
114-118: Restore URL utils in teardown for explicit cleanupFollow our test hygiene: restore url-utils in teardown as well, not just in setup. This keeps suites isolated if dependencies change.
after(async function () { await configUtils.restore(); + await urlUtils.restore(); await testUtils.startGhost(); request = supertest.agent(configUtils.config.get('server:host') + ':' + configUtils.config.get('server:port')); });ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.js (2)
72-87: Add a negative match case for '/ghostly' to harden the route regex testThis ensures we don't accidentally match prefixes that merely start with 'ghost'.
// Test that it doesn't match unrelated paths route.regexp.test('/admin').should.be.false(); route.regexp.test('/.ghost/').should.be.false(); - route.regexp.test('/tag/ghost/').should.be.false(); + route.regexp.test('/tag/ghost/').should.be.false(); + route.regexp.test('/ghostly').should.be.false();
51-70: Minor: Prefer consistent config setting styleElsewhere we use configUtils.set('admin:redirects', boolean). Using the same style here removes any ambiguity with nested objects and colon-path resolution.
If you prefer, I can update these to:
- configUtils.set('admin:redirects', false)
- configUtils.set('admin:redirects', true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js(1 hunks)ghost/core/test/e2e-frontend/advanced_url_config.test.js(1 hunks)ghost/core/test/e2e-frontend/default_routes.test.js(1 hunks)ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
📚 Learning: 2025-08-11T19:37:41.008Z
Learnt from: kevinansfield
PR: TryGhost/Ghost#24651
File: ghost/core/test/integration/services/q-email-addresses.test.js:138-142
Timestamp: 2025-08-11T19:37:41.008Z
Learning: In Ghost's test suite, prefer explicit cleanup calls (like `configUtils.restore()`) in test teardown even when they might be called by other utilities (like `urlUtils.restore()`), as this improves test readability and prevents issues if test dependencies change in the future.
Applied to files:
ghost/core/test/e2e-frontend/advanced_url_config.test.js
📚 Learning: 2025-05-29T07:47:00.748Z
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
Applied to files:
ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.js
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.jsghost/core/test/e2e-frontend/default_routes.test.js
📚 Learning: 2025-08-11T14:18:31.729Z
Learnt from: niranjan-uma-shankar
PR: TryGhost/Ghost#24626
File: apps/portal/src/components/pages/AccountHomePage/components/AccountFooter.js:15-15
Timestamp: 2025-08-11T14:18:31.729Z
Learning: The /dashboard route in Ghost admin was retired in version 6.0.
Applied to files:
ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js
🧬 Code Graph Analysis (4)
ghost/core/test/e2e-frontend/advanced_url_config.test.js (5)
ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js (1)
urlUtils(3-3)ghost/core/core/frontend/meta/canonical-url.js (1)
urlUtils(2-2)ghost/core/core/frontend/meta/url.js (1)
urlUtils(1-1)ghost/core/test/utils/e2e-utils.js (1)
configUtils(17-17)ghost/core/test/e2e-frontend/preview_routes.test.js (1)
testUtils(10-10)
ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.js (1)
ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js (3)
urlUtils(3-3)handleAdminRedirect(6-9)router(13-13)
ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js (2)
ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.js (7)
req(9-9)res(10-10)urlUtils(6-6)redirectGhostToAdmin(3-3)router(55-55)router(65-65)router(75-75)ghost/core/test/e2e-frontend/advanced_url_config.test.js (1)
urlUtils(5-5)
ghost/core/test/e2e-frontend/default_routes.test.js (8)
ghost/core/test/e2e-frontend/advanced_url_config.test.js (4)
configUtils(4-4)testUtils(3-3)request(8-8)supertest(2-2)ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.js (1)
configUtils(5-5)ghost/core/test/legacy/mock-express-style/api-vs-frontend.test.js (2)
configUtils(6-6)testUtils(4-4)ghost/core/test/unit/frontend/helpers/ghost_head.test.js (3)
configUtils(8-8)testUtils(7-7)testUtils(52-52)ghost/core/test/legacy/site/frontend.test.js (4)
configUtils(11-11)testUtils(10-10)request(13-13)supertest(8-8)ghost/core/test/e2e-frontend/custom_routes.test.js (4)
configUtils(6-6)testUtils(5-5)request(16-16)supertest(2-2)ghost/core/test/e2e-frontend/middleware.test.js (4)
configUtils(4-4)testUtils(3-3)request(9-9)supertest(2-2)ghost/core/test/e2e-frontend/site_id_middleware.test.js (4)
configUtils(4-4)testUtils(3-3)request(7-7)supertest(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: E2E tests
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Unit tests (Node 22.13.1)
🔇 Additional comments (4)
ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js (1)
22-23: Good call exporting core logic for unit testsExporting handleAdminRedirect improves testability and clarity; aligns with our coverage expectations for middleware.
ghost/core/test/e2e-frontend/advanced_url_config.test.js (1)
104-112: Setup sequence looks correct for subdir + external adminResetting url-utils before stubbing and restarting Ghost for this context is sound. The expectations for Location headers under subdirectory config match the middleware behavior.
ghost/core/test/e2e-frontend/default_routes.test.js (1)
219-262: End-to-end redirect assertions cover both SPA and API; looks goodThe positive redirect tests for /ghost, /ghost/, and admin API paths with admin:redirects=true (and external admin host) correctly validate Location and headers.
ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.js (1)
37-49: Unit tests validate core mapping correctlyCoverage for '/ghost', '/ghost/', and a known admin API path to adminPath is accurate and isolates redirectToAdmin with a stub. Nice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ghost/core/test/legacy/mock-express-style/parent-app-vhosts.test.js (4)
194-206: Also assert Location header for API redirect (configured URL)Only asserting 301 leaves the target unchecked. Verifying the Location ensures we’re redirecting to the correct admin host and path.
Apply this diff:
return localUtils.mockExpress.invoke(app, req) .then(function (response) { - response.statusCode.should.eql(301); + response.statusCode.should.eql(301); + response.headers.location.should.eql(`https://admin.example.com${ADMIN_API_URL}/site/`); });
208-220: Also assert Location header for API redirect (localhost)Mirror the configured-URL test by checking the redirect target. This catches misconfigurations where the admin host/path might be wrong.
Apply this diff:
return localUtils.mockExpress.invoke(app, req) .then(function (response) { - response.statusCode.should.eql(301); + response.statusCode.should.eql(301); + response.headers.location.should.eql(`https://admin.example.com${ADMIN_API_URL}/site/`); });
317-329: Good addition; consider adding localhost variant for symmetryThis new test correctly asserts a 404 when admin redirects are disabled. For completeness, consider a matching test for localhost to prove we don’t accidentally redirect there either.
Suggested test to add nearby:
it('404s the api on localhost', function () { const req = { secure: false, method: 'GET', url: `${ADMIN_API_URL}/site/`, host: 'localhost' }; return localUtils.mockExpress.invoke(app, req) .then(function (response) { response.statusCode.should.eql(404); }); });
363-375: Test name mismatches its expectationThe description says “404s” but the assertion expects 200. Update the name for clarity.
-it('404s the front-end on configured url (https)', function () { +it('loads the front-end on configured url (https)', function () {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/test/legacy/mock-express-style/parent-app-vhosts.test.js(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ghost/core/test/legacy/mock-express-style/parent-app-vhosts.test.js (2)
ghost/core/test/legacy/mock-express-style/api-vs-frontend.test.js (19)
response(119-119)req(53-57)req(67-71)req(81-85)req(95-99)req(113-117)req(129-133)req(142-146)req(167-171)req(185-189)req(200-205)req(218-223)req(235-240)req(265-270)req(280-285)req(297-302)req(312-317)localUtils(5-5)app(13-13)ghost/core/test/legacy/mock-express-style/utils/setup.js (1)
app(44-47)
ref https://linear.app/ghost/issue/PROD-2429/ - fixes ActivityPub requests for self-hosters with separate front-end/admin domains because it assumes it can fetch `{site url}/ghost/api/admin/site/` which would 404 without these redirects in place
kevinansfield
force-pushed
the
add-frontend-ghost-redirects
branch
from
016e467 to
1108e2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/core/test/legacy/mock-express-style/parent-app-vhosts.test.js (1)
319-331: Add a localhost negative case when redirects are disabledThe configured host 404 case is covered. Consider adding a localhost test to ensure behavior is consistent across vhosts when admin redirects are disabled.
You can add a companion test:
it('404s the api on localhost', function () { const req = { secure: false, method: 'GET', url: `${ADMIN_API_URL}/site/`, host: 'localhost' }; return localUtils.mockExpress.invoke(app, req) .then(function (response) { response.statusCode.should.eql(404); }); });ghost/core/test/e2e-frontend/advanced_url_config.test.js (2)
113-118: Nit: Restore config before restoring URL utils for consistencyOther suites tend to restore config before urlUtils. Swapping these improves consistency and avoids any edge cases where urlUtils.restore() might rely on current config state.
Apply this diff:
after(async function () { - await urlUtils.restore(); - await configUtils.restore(); + await configUtils.restore(); + await urlUtils.restore(); await testUtils.startGhost(); request = supertest.agent(configUtils.config.get('server:host') + ':' + configUtils.config.get('server:port')); });
132-143: API redirect coverage is solid; consider adding no-trailing-slash and query-string casesThe current assertions cover trailing-slash API redirects. To harden coverage:
- Add a no-trailing-slash API test.
- Verify query strings are preserved on redirect (important for clients that send params).
Add the following tests below this block:
it('/blog/ghost/api/admin/site should redirect to external admin API route', async function () { await request.get('/blog/ghost/api/admin/site') .expect(301) .expect('Location', 'http://admin.localhost/blog/ghost/api/admin/site'); }); it('/blog/ghost/?ref=abc should preserve query string on redirect', async function () { await request.get('/blog/ghost/?ref=abc') .expect(301) .expect('Location', 'http://admin.localhost/blog/ghost/?ref=abc'); }); it('/blog/ghost/api/admin/posts/?page=2 should preserve query string on redirect', async function () { await request.get('/blog/ghost/api/admin/posts/?page=2') .expect(301) .expect('Location', 'http://admin.localhost/blog/ghost/api/admin/posts/?page=2'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js(1 hunks)ghost/core/test/e2e-frontend/advanced_url_config.test.js(2 hunks)ghost/core/test/e2e-frontend/default_routes.test.js(1 hunks)ghost/core/test/legacy/mock-express-style/parent-app-vhosts.test.js(4 hunks)ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/test/unit/frontend/web/middleware/redirect-ghost-to-admin.test.js
- ghost/core/test/e2e-frontend/default_routes.test.js
- ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T19:37:41.008Z
Learnt from: kevinansfield
PR: TryGhost/Ghost#24651
File: ghost/core/test/integration/services/q-email-addresses.test.js:138-142
Timestamp: 2025-08-11T19:37:41.008Z
Learning: In Ghost's test suite, prefer explicit cleanup calls (like `configUtils.restore()`) in test teardown even when they might be called by other utilities (like `urlUtils.restore()`), as this improves test readability and prevents issues if test dependencies change in the future.
Applied to files:
ghost/core/test/e2e-frontend/advanced_url_config.test.js
🧬 Code Graph Analysis (1)
ghost/core/test/e2e-frontend/advanced_url_config.test.js (2)
ghost/core/core/frontend/web/middleware/redirect-ghost-to-admin.js (1)
urlUtils(3-3)ghost/core/test/e2e-server/admin.test.js (1)
urlUtils(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: E2E tests
- GitHub Check: i18n
- GitHub Check: Lint
🔇 Additional comments (5)
ghost/core/test/legacy/mock-express-style/parent-app-vhosts.test.js (2)
194-207: Redirect expectation for admin API on configured URL looks correctThe 301 status and absolute Location to the external admin host match the new redirect semantics for separate admin hosts.
209-222: Redirect expectation for admin API on localhost looks correctGood parity with the configured URL test; asserting a 301 with absolute Location to the admin host covers the vhost behavior well.
ghost/core/test/e2e-frontend/advanced_url_config.test.js (3)
31-35: Good fix: await urlUtils.restore()urlUtils.restore() returning a Promise should be awaited; this aligns with the utility’s async behavior and prevents teardown races.
103-111: Setup looks minimal and focusedEnabling admin redirects with a separate admin host for a subdirectory site is configured correctly. Stubbing URL utils and restarting Ghost prior to requests ensures the new config is in effect.
120-131: Redirects for /blog/ghost and /blog/ghost/ are validated correctlyBoth variants exercise the route’s optional trailing slash handling and assert absolute Location to the external admin SPA.
kevinansfield
changed the title
/ghost/api/admin/site/ access with separate admin url
kevinansfield
changed the title
/ghost/api/admin/site/ access with separate admin url/ghost/api/admin/site/ access with separated admin setup
ref https://linear.app/ghost/issue/PROD-2429/
{site url}/ghost/api/admin/site/but for self-hosters with separated front-end/admin domain setups that endpoint would 404 because/ghost/*routes are only served by the admin app which lives at{admin url}/ghost/redirectGhostToAdminmiddleware used by the frontend app so it now redirects{site}/ghost/*instead of just{site}/ghost/, fixing the 404 that ActivityPub was receiving when hitting the front-end