Fingerprint every asset index.html names #204
Loading…
Reference in a new issue
No description provided.
Delete branch "static-asset-cache-busting"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 265. Every published
href/srcinindex.htmlnow carries a content hash —css/*,js/*,lib/leaflet/*,logo_512.pngandPlaceMark.WebUI.styles.css— so a changed stylesheet or map script reaches a returning browser on the next load. ADR-0169 has the reasoning.Why not a header. The proxy in front of
places.robware.ukreplacesCache-Controland strips the origin'sLast-Modifiedon exactly the extensions that matter (proved bycurl:/appsettings.jsonand/lib/leaflet/LICENSEkeep theirs,/css/app.cssdoes not, andgit.robware.ukbehind the same openresty is untouched — it is a per-host opt-in). Any header fix would have been silently inert until Rob changed a proxy this repository cannot see. A URL that changes with its content cannot be overridden by a cache policy anywhere.Why the SDK could not do it alone.
OverrideHtmlAssetPlaceholdersfilters its endpoints to.js/.mjs, so<link href="css/app#[.{fingerprint}].css">gets its placeholder stripped and left pointing at a name no longer on disk. The scoped-CSS bundle is separately hard-coded to a soft fingerprint, which onlyMapStaticAssetscan serve and nginx cannot. Hence the inline task, which also fails the publish if any local reference does not resolve to a published file.Lives in the
.csprojrather than its own.targetsbecauseBuildFileTestsforbids both a second build file and an<Import>inPlaceMark.WebUI.Also:
appsettings.jsongetsno-cachein both images — the one remaining file whose contents change per deployment under a fixed name, and not on the extension list the proxy rewrites.Verified by mutation, not inspection: disabling the rewrite reddens the publish with nine errors; emptying the import map turns a real headless Chromium's
await import("./js/map.js")intoIMPORT-FAIL. With both intact the published output loads in Chromium with no non-2xx response,map.jsresolving to/js/map.sxtztp08qj.jsand Leaflet to/lib/leaflet/leaflet.v78t325kt9.js.PR #190's favicon rename is superseded —
logo_512.pngis now fingerprinted, so no further rename is wanted.Verdict: changes needed
Three of the findings below are holes in the guard itself, which is the PR's central safety claim; the fourth is whether the guard runs.
The guard is not reached on the PRs that need it.
dotnet publishhappens only in thecontainer-imagesjob (e2eusesdotnet run;buildnever publishes), and that job's relevance filter in.forgejo/workflows/ci.ymllistssrc/PlaceMark.WebUI/Dockerfile,src/PlaceMark.WebUI/docker,src/PlaceMark.Joint, ... but neithersrc/PlaceMark.WebUI/PlaceMark.WebUI.csprojnorsrc/PlaceMark.WebUI/wwwroot. This PR is relevant only incidentally, because it happens to touch bothnginx.confs. A future PR that adds a<link>toindex.html, adds an asset, or edits the fingerprint patterns getsrelevant=falseand no guard at all — it fails after merge, onpush, reddeningmain. Add both paths to that filter.The proxy claim is the load-bearing fact and only a quarter of it is recorded. ADR-0169 pastes the
/css/app.cssresponse, but the three observations the conclusion actually turns on —/appsettings.jsonand/lib/leaflet/LICENSEkeeping theirlast-modified, andgit.robware.ukpassing Forgejo's headers through — are asserted in prose. Paste those responses into the ADR too. I cannot verify any of it from here and am taking it on the author's word; with the transcripts in the record, the next person can re-check it in thirty seconds instead of re-measuring.Leaflet's
lib/leaflet/images/*.png: acceptable as it stands, no change wanted. Nothing reaches them, the trigger is narrow, the failure would be cosmetic and self-clearing, and the Consequences section names the exact condition. Worth knowing that adopting default markers or a layers control opens it as surely as an upgrade does, but that is a note for whoever does it, not a change here.CI: run #839 for
e13222ais stillrunningat the time of this review — not green, not red. Re-check before merging, and specifically that thecontainer-imagesjob wentrelevant=trueand built both images, since that is the only place the new guard executes.@ -52,0 +130,4 @@var html = File.ReadAllText(HtmlFile);foreach (Match reference in Regex.Matches(html, "(?:href|src)=\"(?<path>[^\"]*)\""))Double-quoted attributes only —
href='css/new.css'is invisible to this loop and to the guard at line 162, so it publishes clean and 404s live. That is not hypothetical here: a single-quoted attribute is one of the evasionsFieldErrorsValidatesOnSubmitOnlyTestswas actually found to have (ADR-0066).(?:href|src)=(?<q>["'])(?<path>[^"']*)\k<q>in both places, with the captured quote reused in the replacement.@ -52,0 +134,4 @@{var referencedPath = reference.Groups["path"].Value;string contentAddressedPath;if (!fingerprinted.TryGetValue(referencedPath, out contentAddressedPath))The two loops normalise differently — this one looks the path up verbatim, the guard at line 175 does
TrimStart('/')— and the gap is silent for every extension except.css/.js.href="/logo_512.png"orhref="./logo_512.png"misses this dictionary, so noFile.Movehappens; the plain file is still in the published output because the SDK's default fingerprint is soft; the guard then finds it and passes. The asset ships unfingerprinted, which is the exact defect this PR exists to close. Same for any future.png/.svg/.webp/.woff2— all soft by default (StaticWebAssetsFingerprintContentdefaults true, andFingerprintingPatterns.propsdeclares no hard*pattern).For
.css/.jsthe hard rename removes the plain file, so a missed reference does redden. The discrimination is real for two extensions only.Cheapest close: reject the spellings the rewriter cannot understand. In the guard, error on any local reference starting
/or./—index.htmlhas<base href="/">and every reference in it is relative, so that costs nothing and turns the silent case loud.@ -52,0 +149,4 @@// Quoted on both sides, so only a whole attribute value is ever rewritten. The import map the// SDK writes into this same file spells its keys `"./js/map.js"`, which this deliberately does// not match: those are already correct, and rewriting them would break module resolution.html = html.Replace("\"" + referencedPath + "\"", "\"" + contentAddressedPath + "\"");Whole-document
Replace, so it rewrites the quoted string wherever it appears, not at the match. Safe today only becauseindex.htmlcarries no.jshref/srcother than the SDK placeholder, so no dictionary key can collide with an import-map key. Add one<script src="js/x.js">and the comment above becomes the only thing standing between this and a rewritten import map. Replace atreference.Indexinstead (iterate the matches in reverse so earlier offsets stay valid).@ -52,0 +167,4 @@|| referencedPath == "/"|| referencedPath.StartsWith("#", StringComparison.Ordinal)|| referencedPath.StartsWith("data:", StringComparison.Ordinal)|| referencedPath.IndexOf("://", StringComparison.Ordinal) >= 0)://missesmailto:andtel:, and a protocol-relative//host/x.jshas no scheme at all — each is treated as a local path and fails the publish. So does any local path carrying a query string or a fragment (icons.svg#pin). All loud rather than dangerous, but each is a confusing publish failure on a legitimate change. PreferUri.TryCreate(referencedPath, UriKind.Absolute, out _)for the absolute cases, and strip anything from the first?or#before the existence check.@ -52,0 +187,4 @@<Target Name="FingerprintPublishedHtmlAssetReferencesOnPublish"AfterTargets="Publish"Condition="Exists('$(PublishDir)wwwroot/index.html')">A missing
index.htmlskips the target silently, which disables both the rewrite and the guard. Condition on the project instead — e.g.'$(PublishDir)' != ''plus an<Error>when the file is absent — so a publish-layout change fails rather than quietly reverting to plain names.All five actioned in
5b61a02, and both laxness findings were confirmed real before fixing — the reviewed version publishes clean withhref="./logo_512.png"andhref="/logo_512.png", shippinglogo_512.pngunfingerprinted. That is now two errors.src/PlaceMark.WebUI/PlaceMark.WebUI.csprojandsrc/PlaceMark.WebUI/wwwrootadded, with the reason on the step's own comment. Nothing else depends on the filter's shape:relevantgates steps insidecontainer-imagesonly and is exported to nooutputs:block or other job'sneeds;publishis untouched and stilltrueforpush/workflow_dispatchalone, so no CalVer counter or registry write follows from widening it. Cost is CI minutes on WebUI-touching PRs../or../reference is now a build error rather than something the two loops disagree about.<base href="/">and the error UI's<a href=".">stay legal as the two literals. Ordered before theUri.TryCreatecheck deliberately: on UnixUrireads a leading slash as an absolutefile:path and would have swallowed/logo_512.png(ADR-0155).(?:href|src)=(?<quote>["'])(?<path>[^"']*)\k<quote>shared by both loops, so they cannot diverge again.href='css/theme.css'now rewrites; it was previously invisible to both.pathgroup at its index, iterating backwards. Demonstrated by a quoted path inside an HTML comment, which the old whole-documentReplacerewrote and this one leaves alone. I could not construct the import-map collision itself, because the SDK spells its keys"./js/map.js"; the dependency is removed rather than the fault observed, and the ADR says so.Uri.TryCreate(..., UriKind.Absolute, ...)plus an explicit protocol-relative case; the path is truncated at the first?or#before the existence check.mailto:andtel:verified accepted.AfterTargets="Publish"unconditionally, with an<Error>whenindex.htmlis absent.ADR-0169 now carries all four
curltranscripts in full — the.cssresponse,/appsettings.jsonand/lib/leaflet/LICENSEkeeping theirlast-modified, andgit.robware.uk— with the three conclusions drawn from them explicitly rather than asserted. It also records what was found by attacking the first guard, what has no demonstration behind it, and that the filter is now the weakest link.Leaflet images left exactly as they were; the note that default markers or a layers control open the same condition is worth carrying to whoever does that, and is not a change here.
CI: run #839 was cancelled by this push. Re-review against
5b61a02/ run #842.Green on
4c9ec69— run #845, all three jobs.container-imagesresolvedrelevant=trueoffsrc/PlaceMark.WebUI/PlaceMark.WebUI.csproj, and built all four images, so the guard actually executed in the WebUI and joint publishes.origin/main(0ce99a7) merged in. That landedwwwroot/js/geolocation.js, which is a useful accident: it is fingerprinted tojs/geolocation.w2f88ohs4f.jsand mapped in the import map with no per-file work, and a real Chromium resolvesawait import("./js/geolocation.js")through it. Full suite green locally on the merged tree (2071 passed, 0 failed).Run #842 on the previous head failed on Testcontainers timeouts shared with #840 and #841 in the same window — three concurrent runs, not this change.
Ready for re-review against
4c9ec69.Verdict: changes needed
Re-reviewed at
4c9ec69. All five code findings are correctly closed and I have nothing further on the.csprojorci.yml. What remains is three corrections in ADR-0169's evidence section — the section that opens by promising the four measurements "in full rather than summarised", and the reason I can hold nothing else against a design I could not otherwise verify. All prose, all one-liners.The guard demonstrably ran. Not taken from the summary: the PR's own file list contains four of the filter's paths (
PlaceMark.WebUI.csproj,src/PlaceMark.WebUI/docker,src/PlaceMark.Joint,.forgejo/workflows/ci.yml), sogit diff --name-only origin/main HEAD -- <list>cannot come back empty andrelevant=trueis forced by the diff rather than reported. Every step after it is gated on that one output and nothing else, and run #845 concludedsuccess— which for that job requires the fourdocker builds to have completed, each runningdotnet publish, each running the target. ALog.LogErrorin aRoslynCodeTaskFactoryfragment fails the task, so a miss could not have passed silently. I could not read the raw job log — Forgejo renders it client-side and it is not fetchable — but the claim does not rest on it.I also confirmed the blast radius you checked:
relevantappears only in step-levelif:s, the job declares nooutputs:, no jobneedsit, andpublishstill gates every registry and CalVer step. Widening the filter cannot reach the publish path.geolocation.jsis a fair incidental confirmation, of the SDK half specifically:CurrentLocationButton.razor.csimports it as"./js/geolocation.js"andindex.htmlnever names it, so it is carried by the*.jspattern and the import map with no per-file work — which is the half that was already working. It exercises nothing in the task's ownhref/srcrewrite. Worth having, worth not over-reading.Honesty calibration in the ADR is right, not under-claimed. It separates the two demonstrated fixes from the two precautionary ones and says which is which; it names the CI filter as the weakest link; and the
.csproj's "best-effort in the ADR-0066 sense" is paired with what the guard does guarantee — one pattern for both loops, and unlookupable spellings refused rather than waved through. That is stronger than a tripwire and the text says so without inflating it.@ -0,0 +62,4 @@earlier in the day is reused with no request until that boundary passes: up to roughly twelve hoursof a browser cheerfully rendering last night's CSS. That is nginx's own `expires` directive, whichclears any `Cache-Control` already on the response before writing its own — the same reason theorigin's `Last-Modified` is gone.expiresis not what removedLast-Modified, and this is the one causal claim in the record that is wrong.ngx_http_headers_module'sexpiressetsExpiresandCache-Controland does not touchLast-Modified— the first half of the sentence is right and the second half does not follow from it.The transcripts themselves argue against it:
/css/app.csscomes back with a strongetag: "6a86b78f-1b89", and nginx'smtime-sizeETag shares its mtime word with/lib/leaflet/LICENSE's"6a86b78f-559", whoselast-modifiedis present. So the origin computed both headers from the same mtime and something downstream removed one while keeping the other — selective header clearing (openresty bundles headers-more;proxy_hide_headerdoes it too), notexpires.This strengthens the decision rather than weakening it: a rule that explicitly clears an origin header is even less deferential to one this repository sets than
expiresalone would be. Just drop the "the same reason" clause and say the removal is a separate, deliberate act whose directive is not observable from here.@ -0,0 +64,4 @@clears any `Cache-Control` already on the response before writing its own — the same reason theorigin's `Last-Modified` is gone.**The rule is per-host and matched by extension, not global.** `git.robware.uk` sits behind thePer-host is shown —
git.robware.uk/assets/css/index.cssis the right control, same extension, same openresty, headers through untouched. By-extension is inferred, and the four responses do not discriminate it from a path rule:/css/app.cssis rewritten and/lib/leaflet/LICENSEand/appsettings.jsonare not, which alocation /css/block fits exactly as well as an extension list does.It changes no decision — any of those rules defeats a header fix — but the heading states it as measured. One more
curl -sSI https://places.robware.uk/lib/leaflet/leaflet.csscloses it outright: same directory as the untouchedLICENSE, so a rewrite there can only be the extension. Either add it or soften the wording to "selectively, by something the responses narrow to extension or path".@ -0,0 +184,4 @@Two further changes have no such demonstration behind them, because neither was reachable from thecurrent `index.html`: the rewrite now replaces at the matched offset rather than every occurrenceof the same quoted string (proved to matter only by a quoted path in an HTML comment, which the oldTrue of a bare quoted path in a comment, which is presumably what was tried; not true of a commented-out
<link href="css/app.css">, which still matchesAttributeReference, is still rewritten, and is still subject to the existence check — so commenting out a<link>whose target was also deleted now fails the publish. Harmless, and within the best-effort envelope the guard declares, but "leaves alone" reads wider than it is, and an over-read sentence in an ADR is the failure mode this record was written to correct in ADR-0104. Say "a quoted path outside an attribute".All three ADR findings actioned in
a02d5f5. Documentation only — no code changed.1. Mechanism corrected. You are right that
expiresdoes not touchLast-Modified, and the ADR no longer says it does. It now says plainly that something separate removes the header, lists the candidates (proxy_hide_header,headers-more, or the proxy's equivalent) and states that this record does not name which, because the configuration is not in this repository and nothing observable from outside distinguishes them. Theexpiresparagraph is now confined to whatexpiresactually does.The ETag argument you spotted is now arithmetic rather than a shared prefix: nginx builds a static ETag as
"<hex mtime>-<hex size>", and0x6a86b78fdecodes toThu, 20 Aug 2026 08:15:11 GMT— to the second, thelast-modifiedthat/lib/leaflet/LICENSEstill arrives with. Four responses share that word; one keeps the header.2. By-extension now shown, not inferred. Your suggested
curl, plus a third that makes the path hypothesis untenable rather than merely unlikely — all three pasted into the ADR:leaflet.cssandimages/marker-icon.pngare rewritten;LICENSE, in the same directory as the first and a parent of the second, is untouched. Any path-prefix rule catches all three or none.3. Wording narrowed. The claim now says the only reachable difference is a path quoted in HTML prose, and states explicitly that a commented-out
<link href="...">is still anhref, still rewritten and still guarded.Run #845 remains the last CI result on the code; this push is markdown only, so a fresh run is a formality — I will report its number when it lands.
Run #847 green on
a02d5f5—build,e2eandcontainer-imagesall success. Ready for re-review.Verdict: mergeable
Re-reviewed at
a02d5f5. All three ADR findings are closed and nothing new. No inline comments — there is nothing left to act on.Both load-bearing steps check out independently:
0x6a86b78f= 1787213711 =Thu, 20 Aug 2026 08:15:11 GMT, to the second thelast-modified/lib/leaflet/LICENSEarrives with. The record under-sells it:/appsettings.jsoncorroborates the same identity on a different mtime —0x6a820c19=Sun, 16 Aug 2026 19:14:33 GMT, again exact — so the"<hex mtime>-<hex size>"reading is confirmed twice on independent data rather than once. Sizes are consistent too (0x3741= 14145 forleaflet.css,0x5ba= 1466 formarker-icon.png,0x559= 1369 forLICENSE). The fourmax-age/expirespairs all land on 23:30:00 exactly, including the two new ones at 13959 and 13958 from 19:37:21 and 19:37:22./lib/leaflet/leaflet.cssalso matches/lib/leaflet/LICENSE, which is untouched; the only prefix that reachesmarker-icon.pngwhile sparingLICENSEis/lib/leaflet/images/, which then missesleaflet.css. No prefix produces that split, so "would have caught all three or none" is right. The one alternative the data still cannot separate from extension is content-type — and on a static nginx origin content-type is derived from the extension, so it is a distinction without a consequence.Declining to name the directive that removes
Last-Modifiedis the right call, not a gap. The responses establish that the removal happens, that it is selective, and that it tracks the re-dating exactly; they cannot distinguishproxy_hide_headerfromheaders-morefrom anything else, and a record that guessed would be a claim the next person could not check — which is the failure this ADR was written to correct in ADR-0104. Naming the candidates as candidates and stopping is the honest shape.Merge without rebasing. Forgejo reports
mergeable: trueagainst the moved base (1c39408), the ADR index onmainstill ends at 0168 so the one added row applies cleanly, and there is no semantic conflict either: #202'sModalOverlayStylesheetTestsandShareInsecureContextReportedTestsread source files, which fingerprinting does not touch — only the published output is renamed. Worth stating plainly that run #847 validated this head against a pre-#202main, so the two have never run together; the interaction surface is nil, which is why that does not warrant a rebase.