Fixed lazy URL service handing out URLs for non-public resources - #28890
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughSerializer inputs for authors, pages, posts, and tags now request lazy URL columns through a shared helper. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 35s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 41s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 42s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 27s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 3s | View ↗ |
nx run-many -t lint -p ghost |
✅ Succeeded | 34s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 29s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 5s | 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-29 05:48:28 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
June 25, 2026 05:31
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
4 times, most recently
from
June 25, 2026 08:16
b4d2de8 to
9ba39b3
Compare
allouis
marked this pull request as ready for review
June 25, 2026 08:37
![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
allouis
force-pushed
the
lazy-url-non-public-resources
branch
from
June 25, 2026 12:03
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
allouis
force-pushed
the
lazy-url-non-public-resources
branch
from
June 29, 2026 04:55
44fd692 to
b84711e
Compare
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 257-267: `ownsResource()` in `lazy-url-service.ts` is missing the
thin-resource guard, so undefined fields can still fall through the
router/filter checks and be owned incorrectly. Add the same thin-resource
detection used elsewhere in the lazy URL flow before evaluating
`_baseFilterMatches()` or `filterMatches()`, and make `ownsResource()` raise
`LAZY_URL_THIN_RESOURCE` for incomplete records instead of returning
false/allowing fallback ownership. Use the existing `ownsResource`,
`_recordForFilter`, and `_baseFilterMatches` logic as the integration points so
the behavior matches the eager-path checks.
🪄 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: 0dc89045-b363-4208-a9a7-a587a5705d0f
📒 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
🚧 Files skipped from review as they are similar to previous changes (10)
- ghost/core/test/unit/server/services/url/url-service-facade.test.js
- ghost/core/core/server/api/endpoints/utils/serializers/input/tags.js
- ghost/core/core/server/services/url/url-service-facade.ts
- ghost/core/core/server/api/endpoints/utils/serializers/input/posts.js
- ghost/core/core/server/api/endpoints/utils/serializers/input/pages.js
- ghost/core/test/unit/api/canary/utils/serializers/input/utils/url.test.js
- ghost/core/core/server/api/endpoints/utils/serializers/input/authors.js
- ghost/core/core/server/api/endpoints/utils/serializers/input/utils/url.js
- ghost/core/test/integration/url-service-parity.test.js
- ghost/core/test/unit/server/services/url/lazy-url-service.test.js
ref https://linear.app/ghost/issue/HKG-1875 The lazy URL service runs alongside the eager service in compare mode, and the two diverged: lazy returned real URLs for internal tags and draft posts where eager returns /404/. Eager only builds URLs for resources that pass each type's resource-fetch filter — visibility:public for tags, status:published for posts and pages — applied once when it loads its cache. The lazy forward path only consulted the per-router filter, which is null for taxonomy routers, so any tag or post matched. This applies the same per-type base filter before any router runs. The filters live in an inline BASE_FILTERS constant, deliberately duplicated from the eager config for now: lazy is meant to replace eager, at which point the two consolidate into one source. A resource that reaches URL generation must carry the columns its filter reads and production callers always provide them, so a resource missing them is a programmer error that throws rather than silently returning a URL eager would 404. Authors have no base filter, since users.visibility is schema-pinned to public. The parity integration test fed eager's cached resources, which drop those columns after filtering, so it now re-adds them to exercise the shape lazy actually receives.
ref https://linear.app/ghost/issue/HKG-1875 Now that lazy gates URL generation on per-type base filters, callers that serialize a resource's URL need a way to learn which columns those filters read so they can ensure the columns are loaded. This exposes that set through getRequiredFields on both the lazy service and the facade, mirroring the existing getRequiredRelations. The facade returns an empty array under the eager service, so eager callers are unaffected.
ref https://linear.app/ghost/issue/HKG-1875 A Content API ?fields=url query narrows the selected columns, so a resource can reach URL generation without the columns the lazy service needs to decide whether it is routable — status for posts and pages, visibility for tags. The posts, pages and tags input serializers now force those columns back into the fetch whenever url is requested; the response is still trimmed to the requested fields afterwards. This is what guarantees an internal tag or draft post still carries its visibility or status so the base filter applies. It is a no-op under the eager service, where getRequiredFields returns nothing.
ref https://linear.app/ghost/issue/HKG-1875 Unlike eager, which looks URLs up by id, lazy builds them by substituting the resource's own fields into the router permalink template, so it also needs the scalar columns a permalink references — slug, id, and published_at for date tokens like :year/:month/:day. A ?fields=url query strips these too, which left lazy producing broken URLs such as /tag//. getRequiredFields now reports each type's permalink columns alongside its base-filter columns, and the authors serializer gains the same column-forcing hook, since authors have no base filter but their /author/:slug/ permalink still needs slug. Permalink relations such as primary_tag and primary_author remain covered by getRequiredRelations.
ref https://linear.app/ghost/issue/HKG-1875 A filtered collection router such as featured:true also needs its filter's own columns on the resource to choose the same router eager would. These were neither reported by getRequiredFields nor required on the forward path, so under ?fields=url a thin record let filterMatches see an undefined field and a featured post routed to /:slug/ instead of /featured/:slug/. The non-relation scalar fields a router filter references are now reported so the serializers force them, and required on the forward path so a thin resource is rejected rather than silently mis-routed. Field names are matched only at NQL expression boundaries, so colon-bearing values such as URLs or timestamps are not mistaken for fields; relation fields and the page/type discriminator stay excluded.
allouis
force-pushed
the
lazy-url-non-public-resources
branch
from
June 29, 2026 05:38
b84711e to
8b66e12
Compare
This was referenced Jul 6, 2026
This was referenced Jul 14, 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