fix(cli): #1767 serve output directory files whose extension no standard resource plugin claims - #1771
Open
jstockdi wants to merge 2 commits into
Conversation
…tension no standard resource plugin claims
…ap so inherited keys cannot match
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.
Related Issue
Resolves #1767
Documentation
No user facing documentation changes. This restores the behaviour the docs already imply for
greenwood serve— that the production server serves what is in the output directory — so nothing in the docs needs to change. Notablyplugin-copy-robotsandplugin-copy-sitemapare default plugins that putrobots.txtandsitemap.xmlinto the output directory, and until nowservewould 404 both of them.Summary of Changes
getStaticServerinpackages/cli/src/lifecycles/serve.jsconfirms the requested file exists in the output directory, then narrows the Greenwood default plugins to the ones flaggedisStandardStaticResourceand asks each one'sshouldServe. Those plugins coverjs,json,css, images, fonts, audio, video,wasmand.map. Nothing covers.xmlor.txt, and there was no fallback, soresponsestayed theinitResponsebuilt fromctx.response.status— still Koa's default 404 at that point in the chain.if (response.ok)was therefore false, nothing was assigned toctx, and Koa emitted its own default 404 (text/plain; charset=utf-8, bodyNot Found).The result is that a file which is demonstrably present in
public/is unreachable through the production server.getHybridServercomposesgetStaticServer, so both server paths behave identically.The fix
After the plugin loop, if and only if no plugin claimed the request (
response === initResponse), look the extension up in a small fixed map and serve the file with thatContent-Type:An extension that is not in the map is left exactly as it is today and still falls through to a 404.
Why an allowlist rather than "serve any file that exists"
The obvious fix is to read and return whatever is on disk once
checkResourceExistspasses, and I deliberately did not do that. It would broaden what the production server hands out to every file that has ever landed in the output directory, including anything acopyplugin, an adapter, or a user's own build step drops there, and it would do so silently with noContent-Typethe server can reason about. That is a much bigger behavioural change than the issue calls for, and it is the sort of change that is hard to walk back once people depend on it.The allowlist keeps the change proportional. It is a closed set of text/data extensions that a static host would serve out of a web root anyway, chosen around what Greenwood itself emits (
sitemap.xmlandrobots.txtvia the default copy plugins) plus the two obvious neighbours (.webmanifest,.csv). Adding a further extension later is a one line, reviewable diff. Deliberately not included:.html(already handled by the page/SPA middleware further down the chain) and anything executable or binary.This is contained entirely within
getStaticServerand introduces no new resource plugin and no change to the plugin contract or to theisStandardStaticResourceflag.This does not weaken the path traversal hardening from #1749
The fallback is nested inside the existing guard and runs only after both of its checks have already passed:
The
isWithinOutputDirboundary check added in #1749 is untouched, and the fallback cannot be reached for any URL that check rejects — a../or percent-encoded traversal never enters this branch at all, so it never reaches the extension lookup. The fallback also narrows further rather than widening: it only fires for a path that is inside the output directory, exists on disk, and whose extension is in the fixed map. The regression tests inserve.default.path-traversalstill pass unchanged.Testing
Folded into the existing
serve.defaultcase rather than adding a new test directory, alongside the other per-content-type assertions there. Fixtures added:src/sitemap.xml,src/robots.txt,src/assets/data.xml.New assertions cover status,
Content-Typeand body for:/sitemap.xml— 200,application/xml/robots.txt— 200,text/plain/assets/data.xml— 200,application/xml(so this is not special casing two filenames)Plus a negative assertion that a genuinely missing
/assets/definitely-missing.xmlstill returns 404 with bodyNot FoundandContent-Type: text/plain, so the fix cannot be mistaken for "serve everything on disk". That negative already passed before the fix and still passes after it.On unpatched
masterthe case reports 46 passing / 9 failing. With the fix it is 55 passing / 0 failing.serve.default.path-traversal,serve.config.base-path,serve.default.api,serve.default.error,serve.spaandserve.config.static-routerwere also run and are unaffected.Note on #1747
Not addressed here and nothing related to it is touched, but worth flagging for whoever reviews it: #1747 (dynamic sitemap) depends on
servereturning the builtpublic/sitemap.xmlsnapshot, and this gap is why that serve path is currently non-functional for local production smoke testing. #1747's own "Known caveats" already discloses it. This change removes that blocker without any coupling to #1747 — the same applies to an RSS feed or any other.xmlartifact a future feature writes into the output directory.