Changed servePublicFile from middleware to router
#25624
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. WalkthroughUpdated a nested submodule pointer. Removed Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
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.
rmgpinto
force-pushed
the
serve-public-file-router
branch
from
f8eceda to
a9c8b59
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: 0
🧹 Nitpick comments (1)
ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js (1)
4-4: Import path updated correctly; consider relocating test file.The import path change correctly reflects the module move from middleware to routers. However, this test file remains in
test/unit/frontend/web/middleware/while testing code that now lives inrouters/. Consider moving this test totest/unit/frontend/web/routers/serve-public-file.test.jsfor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ghost/core/content/themes/source(1 hunks)ghost/core/core/frontend/web/middleware/index.js(0 hunks)ghost/core/core/frontend/web/routers/serve-public-file.js(2 hunks)ghost/core/core/frontend/web/site.js(2 hunks)ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- ghost/core/core/frontend/web/middleware/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/content/themes/source
🧰 Additional context used
🧠 Learnings (9)
📓 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-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: Admin-x React apps build to `apps/*/dist` using Vite, which are then copied by `ghost/admin/lib/asset-delivery` to `ghost/core/core/built/admin/assets/*`
Applied to files:
ghost/core/core/frontend/web/site.jsghost/core/core/frontend/web/routers/serve-public-file.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.jsghost/core/core/frontend/web/routers/serve-public-file.jsghost/core/test/unit/frontend/web/middleware/serve-public-file.test.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: Applies to ghost/i18n/locales/en/{portal,comments,signup-form,search}.json : Add UI translations to namespace-specific files in `ghost/i18n/locales/en/` for public UI apps (portal.json for Portal, comments.json for Comments UI, etc.)
Applied to files:
ghost/core/core/frontend/web/site.jsghost/core/core/frontend/web/routers/serve-public-file.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.jsghost/core/core/frontend/web/routers/serve-public-file.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: API routes should be defined in `ghost/core/core/server/api/`
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: Public app configuration should be passed via data attributes in `<script>` tags injected by `{{ghost_head}}`
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: Public UI apps (Portal, Comments UI, Signup Form, Sodo Search, Announcement Bar) should be built as UMD bundles for CDN distribution
Applied to files:
ghost/core/core/frontend/web/site.js
🧬 Code graph analysis (2)
ghost/core/core/frontend/web/site.js (2)
ghost/core/core/frontend/web/routers/serve-public-file.js (1)
require(8-8)ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js (1)
require(4-4)
ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js (1)
ghost/core/core/frontend/web/routers/serve-public-file.js (1)
require(8-8)
⏰ 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: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push
🔇 Additional comments (5)
ghost/core/core/frontend/web/site.js (1)
13-13: Clean centralization of public file serving.The refactor correctly consolidates scattered public asset route registrations into a single
servePublicFiles(siteApp)call. The import path and invocation are properly aligned with the new router module.Regarding the previously flagged robots.txt ordering concern: the new implementation in
serve-public-file.jsexplicitly checks for a theme-providedrobots.txtand callsnext()to defer tostaticThememiddleware when found, which should preserve the intended fallback behavior.Also applies to: 68-69
ghost/core/core/frontend/web/routers/serve-public-file.js (4)
8-10: New dependencies added for theme-aware robots.txt serving.The imports for
cardAssets,themeEngine, andsettingsCacheare necessary for the centralized route handling and theme-aware robots.txt logic.
119-142: Route registrations are well-structured.The explicit GET route registrations achieve the PR objective of avoiding path comparisons on every request. The card asset routes correctly chain
cardAssets.serveMiddleware()to ensure on-the-fly building, andrecommendations.jsonappropriately disables server caching for dynamic content.
143-165: Robots.txt theme fallback logic correctly implemented.The implementation addresses the route ordering concern by:
- Checking
is_privateand deferring tofilterPrivateRoutes- Using
fs.accessto detect theme-providedrobots.txt- Calling
next()when theme has its own file, allowingstaticThememiddleware (registered later insite.js) to serve itOne minor note:
fs.accessis asynchronous, which is appropriate here. The error-first callback pattern is correctly handled.
168-170: Export structure maintains backward compatibility.The module correctly exports
servePublicFilesas the default while preservingservePublicFileandcreatePublicFileMiddlewareas named exports. This allows the test file to continue importingservePublicFilevia destructuring.
rmgpinto
force-pushed
the
serve-public-file-router
branch
from
a9c8b59 to
ad7cad3
Compare
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19936713309 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19936713309 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19936713309 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 19936713309 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
ref https://linear.app/ghost/issue/BER-3095 - Extracted the public files code from the middleware to the router. This avoids route comparison on every requests Ghost receives.
rmgpinto
force-pushed
the
serve-public-file-router
branch
from
ad7cad3 to
c3c22a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ghost/core/core/frontend/web/routers/serve-public-file.js (2)
119-137: Theme fallback for sitemap.xsl is correctly implemented.The handler properly checks for a theme-provided
sitemap.xslusingfs.accessand callsnext()to delegate tostaticThememiddleware when the file exists. This addresses the concern from the past review comment—the implementation does support theme overrides.
159-181: Theme and privacy handling for robots.txt is correctly implemented.The handler correctly prioritizes privacy settings (
is_private) and then checks for theme-providedrobots.txt, callingnext()to delegate to downstream middleware when appropriate. This addresses the concern from the past review comment—theme overrides are properly supported.
🧹 Nitpick comments (1)
ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js (1)
4-4: Import path correctly updated.The destructured import from the new router module location is correct. Consider whether this test file should be relocated from
middleware/torouters/to better reflect the new module structure, though this is a minor organizational detail that can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ghost/core/content/themes/source(1 hunks)ghost/core/core/frontend/web/middleware/index.js(0 hunks)ghost/core/core/frontend/web/routers/serve-public-file.js(2 hunks)ghost/core/core/frontend/web/site.js(2 hunks)ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- ghost/core/core/frontend/web/middleware/index.js
🧰 Additional context used
🧠 Learnings (10)
📓 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-05-29T07:45:35.714Z
Learnt from: ErisDS
Repo: TryGhost/Ghost PR: 23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
Applied to files:
ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.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/test/unit/frontend/web/middleware/serve-public-file.test.jsghost/core/core/frontend/web/routers/serve-public-file.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: Admin-x React apps build to `apps/*/dist` using Vite, which are then copied by `ghost/admin/lib/asset-delivery` to `ghost/core/core/built/admin/assets/*`
Applied to files:
ghost/core/core/frontend/web/routers/serve-public-file.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: Ghost admin serves admin-x apps from `/ghost/assets/{app-name}/{app-name}.js` URLs
Applied to files:
ghost/core/core/frontend/web/routers/serve-public-file.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: Applies to ghost/i18n/locales/en/{portal,comments,signup-form,search}.json : Add UI translations to namespace-specific files in `ghost/i18n/locales/en/` for public UI apps (portal.json for Portal, comments.json for Comments UI, etc.)
Applied to files:
ghost/core/core/frontend/web/routers/serve-public-file.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-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: API routes should be defined in `ghost/core/core/server/api/`
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: Public app configuration should be passed via data attributes in `<script>` tags injected by `{{ghost_head}}`
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: Public UI apps (Portal, Comments UI, Signup Form, Sodo Search, Announcement Bar) should be built as UMD bundles for CDN distribution
Applied to files:
ghost/core/core/frontend/web/site.js
🧬 Code graph analysis (2)
ghost/core/core/frontend/web/routers/serve-public-file.js (1)
ghost/core/core/frontend/web/site.js (3)
require(5-5)themeEngine(14-14)servePublicFiles(13-13)
ghost/core/core/frontend/web/site.js (3)
ghost/core/core/frontend/web/routers/serve-public-file.js (1)
require(8-8)ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js (1)
require(4-4)ghost/core/core/frontend/helpers/ghost_head.js (5)
require(5-5)require(6-6)require(7-7)require(10-10)require(16-16)
⏰ 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). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: Setup
- GitHub Check: Setup
- GitHub Check: Build & Push
🔇 Additional comments (6)
ghost/core/content/themes/source (1)
1-1: Submodule pointer update looks fineThis file only updates the tracked commit for the
sourcesubmodule. There’s no executable code to review here; as long as this SHA corresponds to the intended theme revision and CI passes with it, this change is good to go.ghost/core/core/frontend/web/site.js (1)
13-13: LGTM - Clean consolidation of public file serving.The refactor from individual middleware calls to a centralized
servePublicFiles(siteApp)function call is well-structured. The new router module handles theme fallback logic internally, so the registration order beforestaticTheme()is intentional and correct—the route handlers check for theme-provided files and callnext()when appropriate to defer to staticTheme.Also applies to: 68-69
ghost/core/core/frontend/web/routers/serve-public-file.js (4)
8-10: Appropriate imports for the new functionality.The new imports are correctly scoped for the theme-aware and privacy-aware routing logic.
140-155: Well-structured public asset route registrations.The explicit route registrations for static assets are clear and follow a consistent pattern. The card assets correctly chain
cardAssets.serveMiddleware()to ensure on-the-fly building before serving.
156-157: Correct cache configuration for recommendations.json.Using
disableServerCache: trueis appropriate for this endpoint since recommendations can change dynamically.
184-186: Export structure supports both usage patterns.The default export for
servePublicFilesenables clean imports insite.js, while named exports forservePublicFileandcreatePublicFileMiddlewaremaintain backward compatibility for existing tests and potential internal usage.
ref https://linear.app/ghost/issue/BER-3095
Note
Switches public file handling to explicit router endpoints, adding theme-aware sitemap/robots fallbacks and updating site wiring.
servePublicFiles(siteApp)incore/frontend/web/routers/serve-public-file.jsto register explicit routes forsitemap.xsl,robots.txt,public/ghost*.css,public/ghost-stats.min.js,public/cards.min.{css,js},public/comment-counts.min.js,public/member-attribution.min.js, and/.well-known/recommendations.jsonwith caching and card asset middleware.sitemap.xslandrobots.txt(checks active theme; respectsis_privatefor robots).servePublicFileandcreatePublicFileMiddlewarehelpers.core/frontend/web/site.js):mw.servePublicFile(...)usages withservePublicFiles(siteApp); removes directcardAssetsusage.servePublicFileexport.servePublicFile.Written by Cursor Bugbot for commit c3c22a7. This will update automatically on new commits. Configure here.