fix: improve in-memory sitemap.xml endpoint#1074
Open
BenjaminMichaelis wants to merge 11 commits intomainfrom
Open
fix: improve in-memory sitemap.xml endpoint#1074BenjaminMichaelis wants to merge 11 commits intomainfrom
BenjaminMichaelis wants to merge 11 commits intomainfrom
Conversation
Addresses multiple items from the OWASP Docker Security Cheat Sheet: - Switch base image to mcr.microsoft.com/dotnet/aspnet:10.0-noble-chiseled (smaller attack surface, no shell, fewer CVEs, non-root by default) - Add USER app directive so container runs as non-root (OWASP Rule 2) - Remove curl|sh pattern for Azure Artifact Credential Provider - Replace raw PAT secret with BuildKit --mount=type=secret file (nugetconfig) so credentials are never written to any image layer or build cache - Generate NuGet auth config in CI via heredoc; clean up with if: always() - Fix PR docker build step: add context and ACCESS_TO_NUGET_FEED=false - Downgrade Node from non-LTS v25 to Active LTS v24 (Krypton)
- Add required=false to BuildKit secret mount so PR/merge_group builds succeed without the nugetconfig secret (was silently working due to dockerfile:1.2 syntax but required=false makes the intent explicit and safe for future syntax bumps) - Use NuGet config layering instead of --configfile: copy credentials-only secret into ~/.nuget/config/credentials.config so dotnet restore reads both the repo nuget.config (sources + packageSourceMapping) and the credentials file automatically - CI-generated config is now credentials-only (no packageSources, no packageSourceMapping) making nuget.config the single source of truth - no more drift risk
The previous approach copied credentials to ~/.nuget/config/credentials.config, which is NOT a path the .NET SDK reads on Linux. The NuGet client only reads user-level config from ~/.nuget/NuGet/NuGet.Config. Replace the broken config-layering approach with 'dotnet nuget update source', which injects credentials into the repo's nuget.config in the build stage. This preserves packageSourceMapping and uses no external tooling. Also fixes: - Semicolon (;) after dotnet restore replaced with && to fail fast - CI now passes raw PAT as BuildKit secret instead of generating XML - Uses runner.temp instead of /tmp for the PAT file
Root causes of the 401 failures:
- Approach 1 (--configfile): the CI script wrote the literal string
\ into the XML file instead of the actual PAT
value (shell variable not expanded), so NuGet forwarded that literal
string to Azure DevOps 401. Also --configfile replaces all
config-file lookup, silently dropping packageSourceMapping.
- Approach 2 (config layering): credentials were copied to
~/.nuget/config/credentials.config, a path the .NET NuGet client
never scans. On Linux the user-level config is
~/.nuget/NuGet/NuGet.Config (single file). The credentials file was
ignored entirely 401.
Current approach (dotnet nuget update source) is correct; two fixes:
1. Prevent credentials from leaking into the Docker build-layer cache.
dotnet nuget update source writes the PAT into /src/nuget.config.
With cache-to: type=gha,mode=max that modified file is cached.
Fix: back up nuget.config before injection and restore it at the end
of the same RUN command so the committed layer contains only the
original credential-free file. The PAT itself never leaves the
BuildKit secret mount.
2. Enforce packageSourceMapping. Per NuGet docs, packageSourceMapping
is bypassed when the MSBuild RestoreSources property is set.
Removing RestoreSources from Directory.Packages.props lets
nuget.config (with its packageSourceMapping) be the single source of
truth:
- ContentFeedNuget private Azure DevOps feed only (no nuget.org
package with that name can be injected via dependency confusion)
- * nuget.org (including EssentialCSharp.Shared.Models, which is
a public package; confirmed by PR builds succeeding without private
feed credentials)
The AccessToNugetFeed condition on the ContentFeedNuget PackageVersion
and PackageReference ItemGroups continues to control whether the
private-feed package is part of the build.
The chiseled image's 'app' user (UID 1654) cannot write to /app/wwwroot/ for sitemap.xml generation at startup. Setting ownership on COPY resolves the UnauthorizedAccessException without requiring chmod or extra layers.
Replace startup file-write of sitemap.xml with a dynamic /sitemap.xml controller action. This eliminates the UnauthorizedAccessException caused by the non-root app user being unable to write to /app/wwwroot/sitemap.xml. - Add SitemapXml() action to HomeController using SitemapProvider.CreateSitemap - Inject IOptions<SiteSettings> for base URL, IRouteConfigurationService via [FromServices] - Remove GenerateAndSerializeSitemapXml (file-write) from SitemapXmlHelpers - Remove unused DirectoryInfo parameter from GenerateSitemapXml - Remove startup sitemap file-write from Program.cs; keep EnsureSitemapHealthy for fail-fast validation - Update tests to match new GenerateSitemapXml signature - Inline BuildKit secret in CI workflow (no temp PAT file) Serving from memory is also more secure: no write permissions needed, no stale file on disk, and the content is always fresh.
Follow-up addressing issues from dual AI review (GPT-5.5 + Opus 4.6). - Use [OutputCache(Duration=3600)] instead of [ResponseCache]; register AddOutputCache/UseOutputCache in Program.cs for real server-side caching - Remove [FromServices] workaround; expose protected RouteConfigurationService in BaseController to avoid CS9107 double-capture - Drop LastModificationDate on static controller routes (UtcNow as lastmod is an SEO anti-pattern) - Filter sitemap route from route discovery so /sitemap.xml does not list itself - Add [EnableRateLimiting] to SitemapXml action for consistency - Accept IEnumerable<SiteMapping> to avoid unnecessary .ToList() allocation - Fix startup log messages: generation -> validation
Member
Author
|
@copilot resolve the merge conflicts in this pull request - I think some commits merged into main already so maybe we just need to rebase on main? |
Co-authored-by: BenjaminMichaelis <22186029+BenjaminMichaelis@users.noreply.github.com>
Contributor
Resolved in ea2174a. Merged Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates sitemap generation in EssentialCSharp.Web by replacing the previous startup-time file generation with a dynamic /sitemap.xml controller action and enabling server-side output caching so the sitemap can be cached in-process.
Changes:
- Add ASP.NET Core Output Caching registration/middleware and apply
[OutputCache]to the new/sitemap.xmlaction. - Refactor
SitemapXmlHelpersto generate nodes in-memory (no file write) and exclude the sitemap route from self-listing. - Update sitemap helper tests to match the new helper signature (and remove temp-dir setup).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| EssentialCSharp.Web/Program.cs | Registers and enables Output Caching; switches startup sitemap logic from “generate file” to “validate mappings”. |
| EssentialCSharp.Web/Helpers/SitemapXmlHelpers.cs | Removes file serialization path; generates sitemap nodes in-memory and filters routes (including excluding sitemap). |
| EssentialCSharp.Web/Controllers/HomeController.cs | Adds /sitemap.xml action that generates and returns sitemap XML with output caching + rate limiting. |
| EssentialCSharp.Web/Controllers/BaseController.cs | Exposes RouteConfigurationService via a protected property for controller reuse. |
| EssentialCSharp.Web.Tests/SitemapXmlHelpersTests.cs | Updates tests to call the revised GenerateSitemapXml signature (no temp dir). |
| Directory.Packages.props | Removes conditional RestoreSources entries (feed/restore behavior change). |
Comment on lines
39
to
46
| var allRoutes = routeConfigurationService.GetStaticRoutes(); | ||
| var controllerRoutes = allRoutes | ||
| .Where(route => !route.Contains("error", StringComparison.OrdinalIgnoreCase)) // Skip Error actions for sitemap | ||
| .Where(route => !route.Contains("index", StringComparison.OrdinalIgnoreCase)) // Skip Index actions for sitemap | ||
| .Where(route => !route.Contains("identity", StringComparison.OrdinalIgnoreCase)) // Skip Identity actions for sitemap | ||
| // All routes should have leading slash | ||
| .Select(route => $"/{route}") // Add leading slash for sitemap URLs | ||
| .Where(route => !route.Contains("error", StringComparison.OrdinalIgnoreCase)) | ||
| .Where(route => !route.Contains("index", StringComparison.OrdinalIgnoreCase)) | ||
| .Where(route => !route.Contains("identity", StringComparison.OrdinalIgnoreCase)) | ||
| .Where(route => !route.Contains("sitemap", StringComparison.OrdinalIgnoreCase)) | ||
| .Select(route => $"/{route}") | ||
| .ToList(); |
Comment on lines
574
to
587
| // Validate sitemap data at startup to fail fast on bad content | ||
| var siteMappingService = app.Services.GetRequiredService<ISiteMappingService>(); | ||
| var logger = app.Services.GetRequiredService<ILogger<Program>>(); | ||
|
|
||
| // Extract base URL from configuration | ||
| var baseUrl = app.Services.GetRequiredService<IOptions<SiteSettings>>().Value.BaseUrl; | ||
|
|
||
| try | ||
| { | ||
| // Create a scope to resolve scoped services | ||
| var routeConfigurationService = app.Services.GetRequiredService<IRouteConfigurationService>(); | ||
|
|
||
| SitemapXmlHelpers.EnsureSitemapHealthy(siteMappingService.SiteMappings.ToList()); | ||
| SitemapXmlHelpers.GenerateAndSerializeSitemapXml(wwwrootDirectory, siteMappingService.SiteMappings.ToList(), initialLogger, routeConfigurationService, baseUrl); | ||
| LogSitemapGenerationSucceeded(logger); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| LogSitemapGenerationFailed(logger, ex); | ||
| // Continue startup even if sitemap generation fails | ||
| // Continue startup even if sitemap validation fails | ||
| } |
Comment on lines
2
to
7
| <PropertyGroup> | ||
| <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally> | ||
| <CentralPackageTransitivePinningEnabled>false</CentralPackageTransitivePinningEnabled> | ||
| <ToolingPackagesVersion>1.1.1.19025</ToolingPackagesVersion> | ||
| <AccessToNugetFeed Condition="'$(AccessToNugetFeed)' == ''">false</AccessToNugetFeed> | ||
| <RestoreSources> | ||
| https://api.nuget.org/v3/index.json; | ||
| </RestoreSources> | ||
| <RestoreSources Condition="$(AccessToNugetFeed)"> | ||
| $(RestoreSources); | ||
| https://pkgs.dev.azure.com/intelliTect/_packaging/EssentialCSharp/nuget/v3/index.json; | ||
| </RestoreSources> | ||
| </PropertyGroup> |
Comment on lines
+21
to
24
| public static void GenerateSitemapXml(IEnumerable<SiteMapping> siteMappings, IRouteConfigurationService routeConfigurationService, string baseUrl, out List<SitemapNode> nodes) | ||
| { | ||
| DateTime newDateTime = DateTime.UtcNow; | ||
|
|
- Remove LastModificationDate from root URL node (setting it to UtcNow on every cache expiry is an SEO anti-pattern; omitting it is cleaner than a stale value) - Fix misleading startup comment: validation errors are logged and startup continues, not fail-fast - Add test: GenerateSitemapXml_DoesNotIncludeSitemapRoute to prevent regression on sitemap self-listing filter
NuGet vulnerability audit queries all sources in nuget.config even when AccessToNugetFeed=false (which only affects RestoreSources via MSBuild). The private Azure DevOps feed is unreachable in CI without auth, causing NU1900 treated as warning-as-error. Add --ignore-failed-sources to dotnet restore so the unavailable private source is skipped gracefully.
Comment on lines
508
to
510
| app.UseRouting(); | ||
| app.UseOutputCache(); | ||
|
|
Comment on lines
+601
to
605
| [LoggerMessage(Level = LogLevel.Information, Message = "Sitemap validation completed successfully during application startup")] | ||
| private static partial void LogSitemapGenerationSucceeded(ILogger<Program> logger); | ||
|
|
||
| [LoggerMessage(Level = LogLevel.Error, Message = "Failed to generate sitemap.xml during application startup")] | ||
| [LoggerMessage(Level = LogLevel.Error, Message = "Failed to validate sitemap during application startup")] | ||
| private static partial void LogSitemapGenerationFailed(ILogger<Program> logger, Exception exception); |
| @@ -4,13 +4,6 @@ | |||
| <CentralPackageTransitivePinningEnabled>false</CentralPackageTransitivePinningEnabled> | |||
| <ToolingPackagesVersion>1.1.1.19025</ToolingPackagesVersion> | |||
| <AccessToNugetFeed Condition="'$(AccessToNugetFeed)' == ''">false</AccessToNugetFeed> | |||
Comment on lines
+206
to
+208
|
|
||
| // /sitemap.xml should not list itself | ||
| await Assert.That(allUrls).DoesNotContain(url => url.Contains("sitemap", StringComparison.OrdinalIgnoreCase)); |
NuGet vulnerability audit queries all sources in nuget.config (including the private Azure DevOps feed) regardless of RestoreSources. When credentials aren't available in CI, this produces NU1900 which TreatWarningsAsErrors escalates to an error. Disable NuGetAudit when AccessToNugetFeed!=true. Also reverts --ignore-failed-sources workaround which didn't help.
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
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
Follow-up to PR #1063. Replaces the startup file-write with a dynamic controller action, then addresses all issues identified in a dual AI code review (GPT-5.5 + Claude Opus 4.6).
Changes
Server-side caching — was a no-op before
[ResponseCache(Duration = 3600)]with[OutputCache(Duration = 3600)]AddOutputCache()andUseOutputCache()inProgram.cs[ResponseCache]only setCache-Controlheaders — every request still regenerated the sitemap XML. Now it's actually cached in-process.Correct DI — remove
[FromServices]workaroundprotected IRouteConfigurationService RouteConfigurationServiceinBaseController[FromServices] IRouteConfigurationServiceaction parameter fromSitemapXml()— this was resolving the same singleton twice and was a code smellSEO fix — stop claiming every page changed on every request
LastModificationDate = DateTime.UtcNowfrom static controller routeslastmodto "right now" on every request teaches crawlers to ignore the signal. Content page mappings still usesiteMapping.LastModifiedwhich is correct.Filter sitemap route from self-listing
sitemapto the route exclusion list inGenerateSitemapXmlso/sitemap.xmldoesn't appear in the sitemapMinor improvements
[EnableRateLimiting("content")]toSitemapXmlaction (consistent with other actions)IEnumerable<SiteMapping>to avoid unnecessary.ToList()allocation on every requestTesting
EssentialCSharp.WebandEssentialCSharp.Web.Testsbuild cleanly with 0 errors