Fixed lazy URL service handing out URLs for non-public resources#28890
Open
allouis wants to merge 5 commits into
Open
Fixed lazy URL service handing out URLs for non-public resources#28890allouis wants to merge 5 commits into
allouis wants to merge 5 commits into
Conversation
Contributor
WalkthroughSerializer inputs for authors, pages, posts, and tags now call a lazy URL column helper. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 39s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 30s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 50s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 13s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run-many -t lint -p ghost |
✅ Succeeded | 26s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 30s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 9s | View ↗ |
Additional runs (2) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-06-25 12:12:57 UTC
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 28148235613 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
allouis
force-pushed
the
lazy-url-non-public-resources
branch
from
0160c12 to
484df86
Compare
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 28149109638 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
allouis
force-pushed
the
lazy-url-non-public-resources
branch
3 times, most recently
from
20bd27f to
b4d2de8
Compare
- eager only builds URLs for resources passing each type's resource-fetch filter (visibility:public for tags, status:published for posts/pages); the lazy forward path only consulted the per-router filter (null for taxonomy routers), so it handed out URLs for internal tags and draft posts where eager returns /404/ - apply the same per-type base filter (inline BASE_FILTERS, duplicated from the eager config for now) in getUrlForResource/ownsResource - a resource that reaches URL generation must carry the columns its base filter reads (status/visibility); production callers always do (full models, or forced by the serializers), so a thin one throws LAZY_URL_THIN_RESOURCE rather than silently 404 a URL eager would produce - the parity integration test fed eager's *cached* resources, which drop those columns; updated it to re-add them (everything cached is published/public) so it exercises the shape lazy actually receives
- the lazy URL service needs each resource's base-filter columns (status for posts/pages, visibility for tags/authors) to decide routability; under a Content API ?fields=url query those columns get stripped before the URL is generated, so the service would throw LAZY_URL_THIN_RESOURCE - expose the per-type base-filter fields so the API serializers can force the columns to be fetched, mirroring getRequiredRelations for router relations - returns [] with no lazy backend, so eager callers are unaffected
- under a Content API ?fields=url query the selected columns are narrowed, so a resource reaches URL generation without the columns the lazy service reads (status for posts/pages, visibility for tags) and is rejected as thin - add forceUrlColumnsWhenLazy to the input url serializer util and call it from the posts/pages/tags input serializers so those columns are force-fetched when url is requested; the response is still trimmed to ?fields - no-op under the eager service (getRequiredFields returns [])
- the lazy URL service builds URLs by substituting the resource's fields into the router permalink template (slug, id, published_at for :year/:month/:day), so it needs those columns present; eager never does (it looks URLs up by id) - a Content API ?fields=url query strips them, so lazy produced broken URLs like /tag// — a divergence from eager - extend getRequiredFields to also report the scalar columns each type's router permalinks reference, so the existing serializer column-forcing loads them - add the authors serializer hook: authors have no base filter but their /author/:slug/ permalink still needs slug - permalink relations (primary_tag/primary_author) stay covered by getRequiredRelations
allouis
force-pushed
the
lazy-url-non-public-resources
branch
from
b4d2de8 to
9ba39b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ghost/core/core/server/services/url/lazy-url-service.ts`:
- Around line 153-173: getRequiredFields() currently only collects
permalink-derived fields and base filters, so router-specific scalar filter
fields like featured or page can be missing and cause lazy routing to
mis-evaluate. Update getRequiredFields() in lazy-url-service.ts to also inspect
each matching router config’s filter definitions and add any non-relation scalar
fields referenced by config.filter to the required field set, alongside
slug/id/published_at, so thin resources still include the values needed by
filterMatches() on the forward path.
🪄 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: c99cc864-7856-4ede-89ca-0075d004af53
📒 Files selected for processing (11)
ghost/core/core/server/api/endpoints/utils/serializers/input/authors.jsghost/core/core/server/api/endpoints/utils/serializers/input/pages.jsghost/core/core/server/api/endpoints/utils/serializers/input/posts.jsghost/core/core/server/api/endpoints/utils/serializers/input/tags.jsghost/core/core/server/api/endpoints/utils/serializers/input/utils/url.jsghost/core/core/server/services/url/lazy-url-service.tsghost/core/core/server/services/url/url-service-facade.tsghost/core/test/integration/url-service-parity.test.jsghost/core/test/unit/api/canary/utils/serializers/input/utils/url.test.jsghost/core/test/unit/server/services/url/lazy-url-service.test.jsghost/core/test/unit/server/services/url/url-service-facade.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/url/lazy-url-service.ts (1)
253-263: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply thin-resource checks before ownership matching.
ownsResource()now gates on base filters, but it still evaluates router filters on whatever fields happen to be present. A thin record missingfeatured,tags, etc. can skip an earlier filtered router and incorrectly let a later catch-all router claim ownership.Suggested fix
// router owns it. Mirrors the base-filter gate in getUrlForResource. const record = this._recordForFilter(resource); + if (this._hasRouterForType(routerType)) { + this._assertBaseFieldsPresent(routerType, resource); + } if (!this._baseFilterMatches(routerType, record)) { return false; } // Ownership is exclusive: only the first matching router of the type // owns the resource, matching eager's reservation. - const owner = this.routerConfigs.find( - c => c.resourceType === routerType && filterMatches(c.compiledFilter, record) - ); + const owner = this.routerConfigs.find((c) => { + if (c.resourceType !== routerType) { + return false; + } + this._assertNotThin(c, resource, routerType); + return filterMatches(c.compiledFilter, record); + });🤖 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 `@ghost/core/core/server/services/url/lazy-url-service.ts` around lines 253 - 263, Update ownsResource() in lazy-url-service.ts so thin records are rejected before router ownership matching: after _recordForFilter(resource) and the base filter check, add the same thin-resource validation used elsewhere before calling routerConfigs.find/filterMatches. This should ensure missing fields like featured or tags do not let a later catch-all router claim ownership. Reference ownsResource(), _recordForFilter(), _baseFilterMatches(), and routerConfigs.find() when making the change.
🤖 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 `@ghost/core/core/server/services/url/lazy-url-service.ts`:
- Around line 76-83: The field extraction logic in lazyUrlService’s filter
parsing is too broad because the regex scans the entire filter string and can
treat colon-containing values as fake fields. Update the extraction in the same
filter-handling path used by buildFilter to only match real filter-expression
field names, or otherwise anchor matches to expression boundaries before adding
to fields. Keep the existing FILTER_NON_SCALAR_FIELDS and _assertNotThin flow,
but ensure the matcher in the field-collection logic does not capture URLs,
timestamps, or other literal values.
---
Outside diff comments:
In `@ghost/core/core/server/services/url/lazy-url-service.ts`:
- Around line 253-263: Update ownsResource() in lazy-url-service.ts so thin
records are rejected before router ownership matching: after
_recordForFilter(resource) and the base filter check, add the same thin-resource
validation used elsewhere before calling routerConfigs.find/filterMatches. This
should ensure missing fields like featured or tags do not let a later catch-all
router claim ownership. Reference ownsResource(), _recordForFilter(),
_baseFilterMatches(), and routerConfigs.find() when making the change.
🪄 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: 738b22d3-35ab-40c5-ac92-1fbc33a7c287
📒 Files selected for processing (2)
ghost/core/core/server/services/url/lazy-url-service.tsghost/core/test/unit/server/services/url/lazy-url-service.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/url/lazy-url-service.test.js
- getRequiredFields and the forward thin-check only covered base-filter and permalink columns plus router-filter *relations*; a router filter's scalar fields (e.g. featured:true, a custom collection filter) were neither reported nor required - so under ?fields=url a thin record reached the service, filterMatches saw the field as undefined, and a featured post routed to /:slug/ instead of /featured/:slug/ (or 404'd) — a divergence from eager, whose cache keeps those columns - extract the non-relation scalar fields a router filter references; report them from getRequiredFields (so the serializers force them) and require them in _assertNotThin (relations and page/type discriminator stay excluded)
allouis
force-pushed
the
lazy-url-non-public-resources
branch
from
0d86527 to
44fd692
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/url/lazy-url-service.ts (1)
257-264: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the same thin-resource guard in ownership checks.
ownsResource()now evaluates the base filter, but unlikegetUrlForResource()it does not call_assertBaseFieldsPresent()first. If a thin post/page/tag lacksstatus,type, orvisibility,filterMatches()can returnfalseand make a valid resource look unowned instead of raisingLAZY_URL_THIN_RESOURCE.🐛 Proposed fix
// A resource failing its base filter is not in eager's map, so no // router owns it. Mirrors the base-filter gate in getUrlForResource. const record = this._recordForFilter(resource); + this._assertBaseFieldsPresent(routerType, resource); if (!this._baseFilterMatches(routerType, record)) { return false; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd6703fa-a871-4acc-bce7-e741548b3d79
📒 Files selected for processing (2)
ghost/core/core/server/services/url/lazy-url-service.tsghost/core/test/unit/server/services/url/lazy-url-service.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/url/lazy-url-service.test.js
vershwal
approved these changes
Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
While the lazy URL service runs alongside eager in compare mode, the lazy forward path handed out URLs for resources eager returns
/404/for — most visibly internal/private tags (the reported divergence), and also draft/scheduled/sent posts.Root cause
Eager only builds URLs for resources that pass each type's resource-fetch filter (
visibility:publicfor tags,status:published+type:*for posts/pages), applied at fetch time. The lazy forward path (getUrlForResource/ownsResource) only consulted the per-router filter, which isnullfor taxonomy routers, so any tag/post matched.Changes (5 atomic commits)
BASE_FILTERS, an explicit inline constant duplicated fromservices/url/config.jsfor now — lazy is meant to replace eager and consolidate) ingetUrlForResource/ownsResource. A resource that reaches URL generation must carry the columns its base filter reads (status/visibility); production callers always do (full models, or forced by the serializers), so a thin resource throwsLAZY_URL_THIN_RESOURCErather than silently 404 a URL eager would produce. Authors have no base filter (users.visibilityis schema-pinned to'public'). Theurl-service-parityintegration test fed eager's cached resources (which drop those columns after fetch-time filtering) — updated it to re-add them so it exercises the shape lazy actually receives.getRequiredRelations); returns[]under eager.?fields=urlthe selected columns are narrowed, so posts/pages/tags serializers force the base-filter columns back into the fetch. No-op under eager.?fields=url— lazy builds URLs by substituting resource fields into the permalink template (slug,id,published_atfor date tokens), which?fields=urlalso strips.getRequiredFieldsreports each type's permalink scalar columns; permalink relations stay covered bygetRequiredRelations.featured:true) were neither reported bygetRequiredFieldsnor required by the thin-check, so a thin record under?fields=urlcould route a featured post to/:slug/instead of/featured/:slug/. Now the non-relation scalar fields a router filter references are reported (so the serializers force them) and required on the forward path (relations and thepage/typediscriminator stay excluded).The eager
services/url/config.jsis not modified.Testing (run locally)
test/unit/api+test/unit/server+test/unit/frontend— all green.test/integrationsuite green (incl.url-service-parity.test.js).Known remaining divergence (follow-up, not in this PR)
shouldHavePosts: eager omits tags/authors with zero published posts from its forward map →/404/; lazy's forward path still returns a URL (the reverseresolveUrlpath already matches, viaTagPublic). Needs post-count data on the resource. Low impact: nothing branches on a forward tag/author/404/, and the archive page itself still 404s via the reverse path.🤖 Generated with Claude Code