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. WalkthroughAdds a new frontend router module that registers a GET route using the link-redirection service’s redirectPrefix() and handleRequest callback. Updates site initialization to import and invoke this router handler instead of mounting the previous middleware. On the service layer, adds redirectPrefix() and routePath() methods and removes the inline prefix pre-check from handleRequest so requests are processed without that early short-circuit. Unit tests were added to cover redirectPrefix() behavior with and without a subdirectory. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://cdn.statically.io/img/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://cdn.statically.io/img/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
| const linkRedirects = require('../../../server/services/link-redirection'); | ||
|
|
||
| module.exports = function handleRedirects(siteApp) { | ||
| siteApp.get(linkRedirects.service.redirectPrefix() + '*', linkRedirects.service.handleRequest); |
There was a problem hiding this comment.
Bug: Link redirects fail on subdirectory installations
The route uses redirectPrefix() which returns the full path including the subdirectory (e.g., /blog/r/ for subdirectory installations). However, Express route matching in the siteApp operates on paths without the subdirectory prefix, as evidenced by other routes like sitemap (/sitemap.xml), favicon (/favicon.ico), and frontend-caching checks (req.path.startsWith('/p/')). The old middleware approach worked because it checked req.originalUrl which includes the subdirectory, but Express routing uses req.path which doesn't. For subdirectory installations, the route /blog/r/* won't match requests where req.path is /r/abc.
Additional Locations (1)
rmgpinto
force-pushed
the
link-redirects-router
branch
from
bbfd884 to
88cd61d
Compare
ghost/core/test/unit/server/services/link-redirection/LinkRedirectsService.test.js
Outdated
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.
| /** | ||
| * Returns the route path for Express routing (always /r/) | ||
| * Express routing uses req.path which excludes subdirectory | ||
| * @return {string} | ||
| **/ | ||
| routePath() { | ||
| return `/${this.#redirectURLPrefix}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to routePath() method
echo "=== Searching for routePath() calls ==="
rg -n --type js '\.routePath\s*\('
echo ""
echo "=== Searching for routePath reference ==="
rg -n --type js 'routePath'
echo ""
echo "=== Checking link-redirects router file ==="
if [ -f "ghost/core/core/frontend/web/routers/link-redirects.js" ]; then
echo "File found, showing content:"
cat -n ghost/core/core/frontend/web/routers/link-redirects.js
else
echo "Router file not found at expected location"
# Try to find any link-redirects router file
echo ""
echo "Searching for link-redirects router file..."
fd -t f "link-redirects" --type f 2>/dev/null | head -20
fiRepository: TryGhost/Ghost
Length of output: 576
Remove unused routePath() method or document its purpose.
The routePath() method (lines 90-97) is defined but never called anywhere in the codebase. The router implementation uses redirectPrefix() instead. If this method is not intended for future use, remove it; otherwise, add a comment explaining why it exists.
🤖 Prompt for AI Agents
In ghost/core/core/server/services/link-redirection/LinkRedirectsService.js
around lines 90 to 97, the routePath() method is defined but unused; either
remove the method entirely to avoid dead code or keep it and add a clear comment
explaining its purpose and why it differs from redirectPrefix() (e.g., reserved
for Express routing that expects a path excluding subdirectory) so future
readers know it’s intentional; update tests or references if any rely on this
API and run linting to ensure no unused-private warnings remain.
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
38d312c to
c8861c0
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.
Note
Moves link redirect handling from middleware to a dedicated router using a new redirectPrefix helper, updating site wiring and tests.
web/routers/link-redirects.jsmountingGETonlinkRedirects.service.redirectPrefix() + '*'.web/site.jsto use the new router instead oflinkRedirects.service.handleRequestmiddleware.LinkRedirectsService.redirectPrefix()to return full path prefix (handles subdirectory), and simplifyhandleRequestby removing prefix checks.redirectPrefix()and keep redirect behavior tests.Written by Cursor Bugbot for commit c8861c0. This will update automatically on new commits. Configure here.