Changed link redirects from middleware to router #25728
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change extracts link redirect route registration into a new frontend router module exposing Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![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/core/server/services/link-redirection/LinkRedirectsService.js (1)
80-86: Well-designed public API for route registration.The method correctly combines the baseURL pathname with the redirect prefix.
Minor: Consider renaming
fullURLWithRedirectPrefixtoredirectPathsince it's a path segment, not a full URL.redirectPrefix() { - const fullURLWithRedirectPrefix = `${this.#baseURL.pathname}${this.#redirectURLPrefix}`; - return fullURLWithRedirectPrefix; + return `${this.#baseURL.pathname}${this.#redirectURLPrefix}`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/core/core/frontend/web/routers/link-redirects.js(1 hunks)ghost/core/core/frontend/web/site.js(2 hunks)ghost/core/core/server/services/link-redirection/LinkRedirectsService.js(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/core/server/services/members/members-api/controllers/RouterController.js:34-51
Timestamp: 2025-09-03T12:28:11.174Z
Learning: The extractRefererOrRedirect function in Ghost's RouterController (ghost/core/core/server/services/members/members-api/controllers/RouterController.js) is working as intended according to maintainer kevinansfield. The current handling of autoRedirect, redirect parameter parsing, and referrer logic does not need the security-related changes suggested around autoRedirect coercion, relative URL handling, or same-origin checks.
📚 Learning: 2025-09-03T12:28:11.174Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/core/server/services/members/members-api/controllers/RouterController.js:34-51
Timestamp: 2025-09-03T12:28:11.174Z
Learning: The extractRefererOrRedirect function in Ghost's RouterController (ghost/core/core/server/services/members/members-api/controllers/RouterController.js) is working as intended according to maintainer kevinansfield. The current handling of autoRedirect, redirect parameter parsing, and referrer logic does not need the security-related changes suggested around autoRedirect coercion, relative URL handling, or same-origin checks.
Applied to files:
ghost/core/core/frontend/web/routers/link-redirects.jsghost/core/core/frontend/web/site.jsghost/core/core/server/services/link-redirection/LinkRedirectsService.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Ghost admin serves admin-x apps from `/ghost/assets/{app-name}/{app-name}.js` URLs
Applied to files:
ghost/core/core/frontend/web/site.js
⏰ 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). (7)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (3)
ghost/core/core/server/services/link-redirection/LinkRedirectsService.js (1)
95-116: Handler logic is correct for the router-based approach.The method properly handles the redirect flow: lookup → dispatch event → redirect (or pass to next middleware if not found). The
next()call on line 101 when no link is found will correctly result in a 404 via downstream error handlers.ghost/core/core/frontend/web/site.js (1)
54-54: Correct placement in the middleware/router stack.The handler registration maintains the same position as the original middleware, ensuring link redirects are processed early before custom redirects and other route handlers.
ghost/core/core/frontend/web/routers/link-redirects.js (1)
1-5: Clean router implementation that achieves the PR objective.The explicit GET route registration with
redirectPrefix() + '*'ensures this handler only runs for redirect URLs, avoiding the middleware overhead on every request.
rmgpinto
force-pushed
the
link-redirects-router
branch
from
e29c039 to
5369c84
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 3
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/core/core/frontend/web/routers/link-redirects.js(1 hunks)ghost/core/core/frontend/web/site.js(2 hunks)ghost/core/core/server/services/link-redirection/LinkRedirectsService.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/frontend/web/routers/link-redirects.js
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/core/server/services/members/members-api/controllers/RouterController.js:34-51
Timestamp: 2025-09-03T12:28:11.174Z
Learning: The extractRefererOrRedirect function in Ghost's RouterController (ghost/core/core/server/services/members/members-api/controllers/RouterController.js) is working as intended according to maintainer kevinansfield. The current handling of autoRedirect, redirect parameter parsing, and referrer logic does not need the security-related changes suggested around autoRedirect coercion, relative URL handling, or same-origin checks.
📚 Learning: 2025-09-03T12:28:11.174Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/core/server/services/members/members-api/controllers/RouterController.js:34-51
Timestamp: 2025-09-03T12:28:11.174Z
Learning: The extractRefererOrRedirect function in Ghost's RouterController (ghost/core/core/server/services/members/members-api/controllers/RouterController.js) is working as intended according to maintainer kevinansfield. The current handling of autoRedirect, redirect parameter parsing, and referrer logic does not need the security-related changes suggested around autoRedirect coercion, relative URL handling, or same-origin checks.
Applied to files:
ghost/core/core/server/services/link-redirection/LinkRedirectsService.jsghost/core/core/frontend/web/site.js
📚 Learning: 2025-02-06T10:30:53.440Z
Learnt from: allouis
Repo: TryGhost/Ghost PR: 22127
File: .github/scripts/release-apps.js:72-74
Timestamp: 2025-02-06T10:30:53.440Z
Learning: In the Ghost repository, the path to defaults.json is correctly structured as 'ghost/core/core/shared/config/defaults.json' with an intentional double 'core' in the path.
Applied to files:
ghost/core/core/frontend/web/site.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Core Ghost backend logic should be placed in `ghost/core/core/server/` directory structure organized by purpose (api, services, models, data/schema, etc.)
Applied to files:
ghost/core/core/frontend/web/site.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Ghost admin serves admin-x apps from `/ghost/assets/{app-name}/{app-name}.js` URLs
Applied to files:
ghost/core/core/frontend/web/site.js
⏰ 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). (2)
- GitHub Check: Build & Push Docker Image
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
ghost/core/core/server/services/link-redirection/LinkRedirectsService.js (2)
80-86: LGTM!The
redirectPrefix()method correctly constructs and returns the full redirect path prefix by combining the base URL pathname with the redirect URL prefix.
95-116: Router correctly filters requests by prefix—code change is safe.The router at
ghost/core/core/frontend/web/routers/link-redirects.jsproperly mounts the handler onlinkRedirects.service.redirectPrefix() + '*', ensuring only redirect-prefixed requests reachhandleRequest. This satisfies the requirement for prefix filtering without the inline check. The change improves maintainability by delegating path filtering to the router layer as intended.
rmgpinto
force-pushed
the
link-redirects-router
branch
from
5369c84 to
bbfd884
Compare
rmgpinto
force-pushed
the
link-redirects-router
branch
from
bbfd884 to
88cd61d
Compare
ghost/core/test/unit/server/services/link-redirection/LinkRedirectsService.test.js
Show resolved
Hide resolved
rmgpinto
force-pushed
the
link-redirects-router
branch
from
88cd61d to
38d312c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/link-redirection/LinkRedirectsService.test.js (1)
68-88: Add test coverage for theroutePath()method.The test suite adds coverage for
redirectPrefix()but the service also introduces aroutePath()method (line 95-97 in LinkRedirectsService.js) that has no corresponding tests. Since this is a new public API, it should have test coverage to verify it returns the expected value (/r/).Add a test case for
routePath():}); + + describe('routePath', function () { + it('returns the Express route path', function () { + const instance = new LinkRedirectsService({ + linkRedirectRepository: {}, + config: { + baseURL: new URL('https://localhost:2368/blog/') + } + }); + assert.equal(instance.routePath(), '/r/'); + }); + }); describe('handleRequest', function () {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/core/core/frontend/web/routers/link-redirects.js(1 hunks)ghost/core/core/frontend/web/site.js(2 hunks)ghost/core/core/server/services/link-redirection/LinkRedirectsService.js(1 hunks)ghost/core/test/unit/server/services/link-redirection/LinkRedirectsService.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/frontend/web/routers/link-redirects.js
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/core/server/services/members/members-api/controllers/RouterController.js:34-51
Timestamp: 2025-09-03T12:28:11.174Z
Learning: The extractRefererOrRedirect function in Ghost's RouterController (ghost/core/core/server/services/members/members-api/controllers/RouterController.js) is working as intended according to maintainer kevinansfield. The current handling of autoRedirect, redirect parameter parsing, and referrer logic does not need the security-related changes suggested around autoRedirect coercion, relative URL handling, or same-origin checks.
📚 Learning: 2025-09-03T12:28:11.174Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/core/server/services/members/members-api/controllers/RouterController.js:34-51
Timestamp: 2025-09-03T12:28:11.174Z
Learning: The extractRefererOrRedirect function in Ghost's RouterController (ghost/core/core/server/services/members/members-api/controllers/RouterController.js) is working as intended according to maintainer kevinansfield. The current handling of autoRedirect, redirect parameter parsing, and referrer logic does not need the security-related changes suggested around autoRedirect coercion, relative URL handling, or same-origin checks.
Applied to files:
ghost/core/test/unit/server/services/link-redirection/LinkRedirectsService.test.jsghost/core/core/server/services/link-redirection/LinkRedirectsService.jsghost/core/core/frontend/web/site.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Core Ghost backend logic should be placed in `ghost/core/core/server/` directory structure organized by purpose (api, services, models, data/schema, etc.)
Applied to files:
ghost/core/core/frontend/web/site.js
📚 Learning: 2025-02-06T10:30:53.440Z
Learnt from: allouis
Repo: TryGhost/Ghost PR: 22127
File: .github/scripts/release-apps.js:72-74
Timestamp: 2025-02-06T10:30:53.440Z
Learning: In the Ghost repository, the path to defaults.json is correctly structured as 'ghost/core/core/shared/config/defaults.json' with an intentional double 'core' in the path.
Applied to files:
ghost/core/core/frontend/web/site.js
📚 Learning: 2025-04-23T15:44:52.549Z
Learnt from: 9larsons
Repo: TryGhost/Ghost PR: 21866
File: ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js:10-19
Timestamp: 2025-04-23T15:44:52.549Z
Learning: The existing implementation in `ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js` using `path.parse(req.url).base` is secure against path traversal attacks as it properly extracts only the filename component without any directory parts.
Applied to files:
ghost/core/core/frontend/web/site.js
📚 Learning: 2025-08-11T19:39:00.428Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24651
File: ghost/core/test/utils/urlUtils.js:53-57
Timestamp: 2025-08-11T19:39:00.428Z
Learning: In Ghost's test utilities, when fixing specific issues like async behavior, it's preferred to maintain existing error handling patterns (even if suboptimal) to keep PRs focused on their primary objective. Error handling improvements can be addressed in separate, dedicated PRs.
Applied to files:
ghost/core/core/frontend/web/site.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Ghost admin serves admin-x apps from `/ghost/assets/{app-name}/{app-name}.js` URLs
Applied to files:
ghost/core/core/frontend/web/site.js
⏰ 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). (3)
- GitHub Check: Build & Push Docker Image
- GitHub Check: Setup
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
ghost/core/core/server/services/link-redirection/LinkRedirectsService.js (2)
80-97: LGTM! Well-structured methods for router integration.The
redirectPrefix()method correctly composes the full path including subdirectory, which is used by the router for route registration. The implementation properly leverages the baseURL pathname that was normalized in the constructor.
106-127: LGTM! Clean handler implementation for route-mounted usage.The
handleRequest()method correctly delegates prefix filtering to the router level (via route pattern matching) and focuses on redirect resolution and execution. The URL construction and error handling are appropriate.ghost/core/core/frontend/web/site.js (1)
18-18: Router implementation correctly exports and registers link redirect routes.The integration at lines 18 and 54 of site.js properly imports and invokes the link-redirects router. The router correctly exports a function that accepts
siteApp, registers a GET route at the redirect prefix, and invokes the link redirection service handler. The change successfully replaces the previous global middleware approach with a router-based handler.
ghost/core/core/server/services/link-redirection/LinkRedirectsService.js
Outdated
Show resolved
Hide resolved
rmgpinto
force-pushed
the
link-redirects-router
branch
from
38d312c to
c8861c0
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
rmgpinto
force-pushed
the
link-redirects-router
branch
4 times, most recently
from
65713ab to
086cbcf
Compare
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 3
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
ghost/core/test/unit/server/services/link-redirection/link-redirects-service.test.js
Outdated
Show resolved
Hide resolved
ghost/core/core/server/services/link-redirection/link-redirects-service.js
Show resolved
Hide resolved
rmgpinto
force-pushed
the
link-redirects-router
branch
2 times, most recently
from
2921378 to
8a2ac85
Compare
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
I'll need to look deeper into this to understand the original behavior/intent. Could you resolve the conflict so we can see if the cursor bot comment is valid? More tests may need updating.
Is it actually cleaner to use this router? I'm finding that a confusing abstraction vs. using siteApp.get with the redirects service directly in site.js.
rmgpinto
force-pushed
the
link-redirects-router
branch
2 times, most recently
from
23da93b to
8a2ac85
Compare
ref https://linear.app/ghost/issue/BER-3095 Extracted the link redirects code from the middleware to the router. This avoids route comparison on every requests Ghost receives.
rmgpinto
force-pushed
the
link-redirects-router
branch
from
8a2ac85 to
8c59cd2
Compare
These tests do not check anything substantive after the changes made to remove `req.originalUrl.startsWith(fullURLWithRedirectPrefix)`.
|
Removed tests that are no longer relevant; We should test this on staging before merging, however I'm in favor of this change overall. We should see better performance with the |
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
Approved but would like to see tested on staging, especially with a subdirectory setup.
|
Deployed to staging with ID: |
ref https://linear.app/ghost/issue/BER-3095
Extracted the link redirects code from the middleware to the router. This avoids route comparison on every requests Ghost receives.
Note
Moves link redirect handling out of global middleware and into a dedicated router for clearer routing and fewer per-request checks.
routers/link-redirects.jsregistersGEThandler atlinkRedirects.service.relativeRedirectPrefix() + '*';site.jsmounts this routerLinkRedirectsService: addsrelativeRedirectPrefix()and removes in-method prefix checks;handleRequestnow assumes routing filtered pathsrelativeRedirectPrefix, update redirect behavior tests, and drop non-matching-prefix casesWritten by Cursor Bugbot for commit 93fa62f. This will update automatically on new commits. Configure here.