Various smaller fixes in one commit - #561
Merged
Merged
Conversation
Contributor
PR Summary by QodoHarden redirects cache refresher and add configurable sitemap index routing
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
8 rules 1. Trailing-slash XML bypass
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+41
to
45
| if (context.Request.Path.Value?.EndsWith(".xml", StringComparison.OrdinalIgnoreCase) != true) | ||
| { | ||
| await _next.Invoke(context); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
2. Trailing-slash xml bypass 🐞 Bug ≡ Correctness
SitemapMiddleware rejects requests like "/sitemap-index.xml/" or "/sitemap-index/sitemap.xml/"
because the new pre-guard checks EndsWith(".xml") without trimming "/", even though
IsSitemapIndexRequest later trims trailing slashes for matching. This makes trailing-slash variants
of sitemap URLs fall through to _next unexpectedly.
Agent Prompt
## Issue description
`SitemapMiddleware` uses `EndsWith(".xml")` as a cheap pre-guard, but it does not normalize trailing slashes. As a result, requests that are effectively XML endpoints but end with `/` never reach the later matching logic (which *does* trim `/`).
## Issue Context
`IsSitemapIndexRequest(...)` already compares using `requestPath.Value?.TrimEnd('/')`, indicating the intention is to treat trailing slashes as equivalent.
## Fix Focus Areas
- src/SeoToolkit.Umbraco.Sitemap.Core/Middleware/SitemapMiddleware.cs[41-57]
- src/SeoToolkit.Umbraco.Sitemap.Core/Middleware/SitemapMiddleware.cs[110-119]
## Suggested change
Normalize once at the start (e.g., `var path = context.Request.Path.Value?.TrimEnd('/')`) and use `path` for:
- the initial `.xml` pre-guard
- `isSitemapRequest`
- `IsSitemapIndexRequest` comparisons (or pass the normalized PathString/value)
## Tests
Add/extend tests to cover trailing-slash variants for both:
- configured index filename (`/sitemap-index.xml/`)
- configured index folder (`/sitemap-index/sitemap.xml/`)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+41
to
+57
| if (context.Request.Path.Value?.EndsWith(".xml", StringComparison.OrdinalIgnoreCase) != true) | ||
| { | ||
| await _next.Invoke(context); | ||
| return; | ||
| } | ||
|
|
||
| var settings = _sitemapConfigurationService.GetSettings(); | ||
|
|
||
| var isSitemapRequest = context.Request.Path.Value.EndsWith("/sitemap.xml", StringComparison.OrdinalIgnoreCase); | ||
| var isConfiguredIndexRequest = !string.IsNullOrWhiteSpace(settings.SitemapIndexPath) | ||
| && IsSitemapIndexRequest(context.Request.Path, settings.SitemapIndexPath); | ||
|
|
||
| if (!isSitemapRequest && !isConfiguredIndexRequest) | ||
| { | ||
| await _next.Invoke(context); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
3. Settings read on any xml 🐞 Bug ➹ Performance
SitemapMiddleware now calls GetSettings() for every request ending in ".xml" even when it later determines the request is neither a sitemap.xml nor the configured index path and forwards to _next. This introduces extra configuration resolution work on unrelated XML endpoints compared to the previous sitemap-only guard.
Agent Prompt
## Issue description
`GetSettings()` is invoked before confirming the request is sitemap-related, so all `.xml` requests incur the settings lookup.
## Issue Context
The broadened `.xml` pre-guard is needed to support an index filename like `sitemap-index.xml`, but settings lookup can often be avoided for unrelated `.xml` requests.
## Fix Focus Areas
- src/SeoToolkit.Umbraco.Sitemap.Core/Middleware/SitemapMiddleware.cs[41-57]
## Suggested change
Consider a two-phase guard:
1) Normalize path and check if it ends with `/sitemap.xml`; if not, only then consider the configured index filename/path.
2) Only call `GetSettings()` when the request is either:
- a `/sitemap.xml` request, or
- a candidate for the configured index (e.g., ends with `.xml` and could match the configured index filename).
If `GetSettings()` is cheap/cached this is optional; otherwise it reduces unnecessary work on non-sitemap XML endpoints.
## Tests
Add a test for an arbitrary `.xml` path (e.g. `/rss.xml`) verifying `_next` is called and (optionally) settings lookup is not performed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+33
to
+35
| if (!rootNode.IsPublished(domain.Culture)) | ||
| continue; | ||
|
|
Contributor
There was a problem hiding this comment.
4. Un-guarded culture ispublished 🐞 Bug ☼ Reliability
SitemapIndexGenerator unconditionally calls rootNode.IsPublished(domain.Culture) without checking for null/whitespace culture, unlike SitemapGenerator which guards IsPublished(culture) behind a string.IsNullOrWhiteSpace check. This creates inconsistent behavior for empty/unspecified cultures and relies on IsPublished handling such values safely.
Agent Prompt
## Issue description
`SitemapIndexGenerator.Generate()` calls `rootNode.IsPublished(domain.Culture)` without guarding `domain.Culture` for null/empty/whitespace.
## Issue Context
Elsewhere (SitemapGenerator) the code explicitly avoids calling `IsPublished(culture)` when the culture string is empty, indicating empty cultures are a possibility that should be handled.
## Fix Focus Areas
- src/SeoToolkit.Umbraco.Sitemap.Core/Common/SitemapIndexGenerator/SitemapIndexGenerator.cs[27-39]
- src/SeoToolkit.Umbraco.Sitemap.Core/Common/SitemapGenerators/SitemapGenerator.cs[132-136]
## Suggested change
Update the check to something like:
- `if (!string.IsNullOrWhiteSpace(domain.Culture)) { if (!rootNode.IsPublished(domain.Culture)) continue; }`
- otherwise decide on intended invariant behavior (e.g., `if (!rootNode.IsPublished()) continue;`)
## Tests
Add a unit/integration test that covers a domain with an empty/unspecified culture and verifies the generator does not throw and produces expected output.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
133
to
+135
| var hideFromSitemap = contentOverride?.ExcludeFromSitemap == true | ||
| || (docTypeSettings?.HideFromSitemap ?? false); | ||
| || (docTypeSettings?.HideFromSitemap ?? false) | ||
| || (!string.IsNullOrWhiteSpace(culture) && !content.IsPublished(culture)); |
Contributor
There was a problem hiding this comment.
5. Url computed when excluded 🐞 Bug ➹ Performance
SitemapGenerator now flags items as HideFromSitemap when !content.IsPublished(culture), but it still calls content.Url(culture, UrlMode.Absolute) before the hideFromSitemap check prevents adding the node. This defeats the intent of excluding unpublished variants early and performs URL resolution work even when the node will be discarded.
Agent Prompt
## Issue description
Even when `hideFromSitemap` is true (including the newly added `!content.IsPublished(culture)` case), the code still constructs `SitemapNodeItem` with `content.Url(culture, UrlMode.Absolute)`.
## Issue Context
The item is only added inside `if (!hideFromSitemap)`, so URL resolution and object creation for hidden nodes is unnecessary.
## Fix Focus Areas
- src/SeoToolkit.Umbraco.Sitemap.Core/Common/SitemapGenerators/SitemapGenerator.cs[132-187]
## Suggested change
Move URL resolution and `SitemapNodeItem` construction inside the `if (!hideFromSitemap)` block, e.g.:
- compute `hideFromSitemap`
- `if (hideFromSitemap) { /* optionally skip recursion depending on desired behavior */ } else { var url = content.Url(...); var item = new SitemapNodeItem(url) { ... }; items.Add(item); }`
## Tests
Add a test case for a culture where a node is not published ensuring the generator does not attempt URL generation for that node (if test harness can observe calls/behavior).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
patrickdemooij9
added a commit
that referenced
this pull request
Aug 15, 2026
Co-authored-by: patrickdemooij9 <11466511+patrickdemooij9@users.noreply.github.com>
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
1 participant
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.
Fixes #554
Fixes #553
Fixes #552
Fixes #550