diff --git a/fjs/web/todo/missing-index-message.md b/fjs/web/todo/missing-index-message.md new file mode 100644 index 0000000000..403d5fab9e --- /dev/null +++ b/fjs/web/todo/missing-index-message.md @@ -0,0 +1,197 @@ +## missing-index-message. A directory with no index reads as a bad URL + +**Priority:** P3 +**Status:** open + +### Problem + +`fjs web` answers `/` with `not found` when the served root has no +`index.html`, which describes the file the server looked for and not the +request the client made. The URL was right, the root was right, and the one +thing missing is a file the operator generates — but the sentence names none of +that, so the first reading is that the address is wrong. + +It is reachable from a clean checkout. `index.html` is a build artifact and is +gitignored, so `fjs web` in a fresh tree answers `not found` at `/` until +`npm run index-html` has run. Nothing in the response points at that. + +The same sentence covers every other refusal: + +| request | today | +|---|---| +| `/` with no `index.html` in the root | `404 not found` | +| `/fjs/` — a directory that exists, with no index | `404 not found` | +| `/no-such-dir/` | `404 not found` | +| `/fjs/web/nope.md` | `404 not found` | +| `/.git/` | `404 not found` | +| `/README.md/` — a trailing slash onto a regular file | `500 io error: ENOTDIR` on POSIX, `404 not found` on Windows | + +Byte-identical, the first five. That is not an oversight for `/.git/`: refusing +a dot-prefixed path as *absent* is what keeps its existence undisclosed, per +`resolve`'s note and "Deliberately absent" in [`../README.md`](../README.md). +So the fix is not "say more when a file is missing" — it is to say more about +**what was asked for** without saying more about what is on disk. + +The last row is the exception, and it sits in exactly the shape this issue +triggers on: a directory-form request whose status already tells an existing +regular file from nothing. It is filed separately as +[notdir-status](./notdir-status.md), because the message is not what is wrong +with it. + +### Proposal + +Give a directory-form request its own sentence, decided by the shape of the +URL rather than by the file system. Such a request is one the server answered +by appending `index.html`, and saying so discloses nothing: the client sent +that path and already knows its shape. + +``` +no index.html in /fjs/ +``` + +**Directory-form is not "ends in a slash".** `resolve` asks +`segments.length === 0 || decoded.endsWith('/')`, so a path that *parses* to +nothing qualifies too — `/.`, `/%2E`, `/a/..` and `/fjs/..` all get +`index.html` appended and none ends in a slash. Verified: each answers `200` +against a root that has one. A trailing slash is sufficient, not necessary, and +an implementation keying off it alone would leave these on the old sentence. + +**A hidden path keeps `not found`, though it is directory-form too.** +`resolve` refuses a dot-prefixed segment *before* computing `isDirectory`, so +`/.git/` never reaches the branch that appends `index.html` — and that +ordering is right rather than an obstacle to route around. The new sentence is +a claim about having looked; for a hidden path the server refused without +touching the disk, so `no index.html in /.git/` would describe work it did not +do. Non-disclosure is unaffected either way, which is worth stating plainly: +the hidden refusal never consults the disk, so `/.git/` and `/.nonexistent/` +answer identically whichever sentence they carry — verified. What separates +them from `/foo/` is that the client wrote a dot, which the client already +knows. So the question is which message is true, and only one of them is. + +Three constraints on the wording: + +- **Name the URL path, not the resolved path.** `fileResponse` already + prefers `errorSummary` to `errorMessage` so a client is not handed the + server's filesystem layout; a message reading `no index.html in + /home/…/fjs/` would give back exactly that. +- **Do not distinguish an existing directory from a missing one.** Telling + `/fjs/` apart from `/no-such-dir/` requires a second `stat` and turns the + response into a directory-existence oracle — the enumeration the identical + `404`s are written to deny, per the dot-prefixed-existence note in `resolve` + and "Deliberately absent" in [`../README.md`](../README.md). (Not the + loopback and `Host` checks: `respond` answers `403 host not served` before + `resolve` runs, so a rebound origin never reaches a `404` at all. Those + defend the socket; this defends what an answer says.) Both should keep + answering the same sentence. +- **Echo the percent-encoded spelling, never the decoded one.** The candidates + disagree — the raw target, the decoded path and the re-joined segments + differ for `/fjs%2F` (decodes to `/fjs/`) and `/fjs/./` (re-joins to `fjs`) + — but the choice is not only cosmetic. Nothing on the way in excludes a + control character: `percentDecode` rejects a malformed escape and invalid + UTF-8, `resolve` rejects NUL separately, and `/%1B%5B31m/` and `/a%0Ab/` are + none of those — they decode to real control characters. A body is not only + read by browsers: `curl` and `wget` write + it to a terminal, where an ANSI or OSC sequence is acted on rather than + displayed. `text/plain` and `nosniff` bound what a *browser* does with the + bytes and say nothing about that, so calling them harmless was wrong. + + So the message percent-encodes what it echoes, rather than trusting what it + was handed. Over HTTP a raw control byte does not reach this module — Node's + parser answers `400 Bad Request` on the request line before the listener + runs, verified — but that is a property of one caller, not of `respond`. + `respond` is exported, `IncomingMessage.url` is an unrestricted `string`, + and `proof.f.mjs` already calls it directly through the virtual runner with + whatever URL a case names; a non-Node runner is the same story. Relying on + the parser would put the invariant outside the function that depends on it, + which is the arrangement that quietly stops holding. + + The pass has to be spelled out, because the obvious one is wrong: a + general-purpose URI encoder escapes the `%` too, turning `/%1B%5B31m/` into + `/%251B%255B31m/` — unreadable, and not what the proof below asks for. And + preserving every `%` is not a rule either, since a lone one is not an + escape. So: + + - a `%XX` triplet — `isEscape`, the predicate `percentDecode` already uses + — passes through verbatim; + - unreserved characters and `/` pass through; + - every other byte is percent-encoded, which is what catches a raw control + character. + + Preserving triplets is total rather than a special case: by the time this + message is built `resolve` has accepted the path, and `percentDecode` + rejects a malformed escape, so every `%` present already begins a valid + triplet. A direct call to `respond` cannot smuggle a lone `%` past that — + it takes the same route — while it *can* carry a raw control byte, which + the third rule encodes. + + So `%1B` survives as `%1B` whether it arrived encoded or raw, and the + raw-versus-normalized choice stays free of safety consequences, since either + is encoded on the way out. What must not happen is quoting `percentDecode`'s + output — or the target's — into the body unprocessed. + + Nothing echoes anything today — the current answer is the constant `not + found` — so this is a property to build in, not a bug to fix. And nothing + needs plumbing for it: `respond` already binds `parseTarget(url)` for the + host check, so `target.path` is in scope where the message would be built. + Only the `isDirectory` fact below has to travel. + +The obstacle is that the distinction is gone by the time it is needed. +`resolve` computes `isDirectory` and then returns `Result` — +a path and nothing else — so `respond` cannot tell an appended `index.html` +from a requested one, and `fileResponse` sees only a `stat` failure. Options: + +1. Widen what `resolve` returns, so the routing fact travels with the path. + Keeps every routing decision in the one pure function that owns them, at + the cost of a change to `Resolve` and its proofs. +2. Re-derive it in `respond` from the URL. No type change, but it puts a + second copy of "what counts as a directory request" outside `resolve`, + which is the split the module header exists to state. +3. Key off the resolved path ending in `index.html`. Cheapest and wrong: it + cannot tell `/index.html` — a request naming the file — from `/`. + +(1) is the one that matches how the module is factored. + +Two spellings are open. A literal `/index.html` should perhaps get the new +sentence too — the message would be true, and treating it as a directory +request would not be. And `/fjs/..` is directory-form without looking it, so +`no index.html in /fjs/..` names a path that reads as a file's neighbour; +echoing the parsed path instead would name a directory the client never wrote. +Either answer satisfies the encoding constraint above — a normalized path is +re-encoded on the way out — so that choice stays open on its own merits. + +### Tasks + +- [ ] Decide how the directory-form fact reaches `respond` — (1) above unless + something argues otherwise. +- [ ] Answer a directory-form `404` with a sentence naming `index.html` and + the URL path, leaving every other `404` as it is. Key it off `resolve`'s + own predicate, not off a trailing slash — and off the branch that + appends `index.html`, so a hidden path keeps `not found`. +- [ ] Prove `/.git/` still answers `not found`, and answers it identically to + `/.nonexistent/`, so the refusal stays ahead of the new sentence. +- [ ] Prove `/%1B%5B31m/` echoes `%1B%5B31m` and not the escape it names, so + no answer this server writes can drive a terminal — and prove it for a + *raw* control character too, by calling `respond` directly the way + `proof.f.mjs` already does, since that path has no HTTP parser in front + of it. +- [ ] Prove that `/fjs/` and `/no-such-dir/` still answer identically — and + `/README.md/` with them, which needs + [notdir-status](./notdir-status.md) first. Without it the proof passes + while the directory-form shape still leaks. It will pass anyway for + `/locked/` and `/loop1/`, which that issue scopes out on purpose, so + state what the proof covers rather than letting it read as "no + directory-form request discloses". +- [ ] Update the response table in `module.f.mjs` and the prose in + [`../README.md`](../README.md). + +### Related + +- [`fjs/web`](../README.md) — "Deliberately absent", where the missing + directory listing and the `/docs` vs `/docs/` split are settled. +- [notdir-status](./notdir-status.md) — a directory-form request whose status + already discloses, which this issue's proof depends on. Not the only one: it + scopes itself to `ENOTDIR` and leaves `EACCES` and `ELOOP` at `500` + deliberately, so directory-form requests still do not answer uniformly on a + POSIX host. +- [`fjs/website`](../../website/) — writes the `index.html` whose absence this + is about. diff --git a/fjs/web/todo/notdir-status.md b/fjs/web/todo/notdir-status.md new file mode 100644 index 0000000000..3177a5d757 --- /dev/null +++ b/fjs/web/todo/notdir-status.md @@ -0,0 +1,248 @@ +## notdir-status. A path through a regular file answers `500`, and only on POSIX + +**Priority:** P3 +**Status:** open + +### Problem + +`GET /README.md/` answers `500 io error: ENOTDIR` on Linux and macOS. The +request is client-caused — a regular file is not a directory, and nothing under +it can exist — so by this module's own doctrine it belongs with the `404` +answers rather than in the channel reserved for the host failing at something it +should have managed. The same argument is already made about `%00`: *"a NUL is a +malformed URL, not a host error"*, and again in +[name-too-long-status](./name-too-long-status.md) for `ENAMETOOLONG`. + +`isNotFound` (`fjs/effects/node/module.f.mjs`) tests `ENOENT` and nothing else, +so `fileResponse` falls past its `404` branch to the `500`. + +**It is also a disclosure, which `ENAMETOOLONG` is not.** The status separates +a path that runs through an existing regular file from one that runs through +nothing: + +| request | POSIX | Windows | +|---|---|---| +| `/nope.md/` — nothing there | `404 not found` | `404 not found` | +| `/README.md/` — an existing regular file | `500 io error: ENOTDIR` | `404 not found` | + +So on POSIX a trailing slash answers "is there a regular file at this name?", +which is the enumeration the identical-`404` answers elsewhere are written to +deny — see the dot-prefixed-existence note in `resolve` and "Deliberately +absent" in [`../README.md`](../README.md). + +**And the status is platform-dependent**, which is the part that makes it worth +filing beyond its sibling. Windows returns `ENOENT` where POSIX returns +`ENOTDIR`, so the same request is `404` on one host and `500` on another, and a +proof written on one platform cannot see the other's answer. Verified directly: +`statSync('README.md/index.html')` reports `ENOENT` on `win32`. + +Reported on +[#1714](https://github.com/functionalscript/functionalscript/pull/1714), where +it contradicted a claim that the `404` was uniform. + +### Proposal + +Answer `404`: a path that descends through a regular file names nothing, and +whether the file it descends through exists is not a distinction worth +publishing. + +**Test it in `fileResponse`, not in `isNotFound`.** Widening the shared +predicate is the tempting reading — `ENOENT` and `ENOTDIR` are one fact +wearing two names, and which one a host says is not a distinction this module +wants. But `isNotFound` has two other callers, and both document, in prose, an +intent that widening would violate: + +- `fjs/cas`'s `list` answers `ok([])` for an absent store and surfaces + everything else, because *"a `.cas` that exists but cannot be read + (permissions, corruption) is a genuine storage error and is surfaced, not + masked as 'no hashes'"*. A store path with a regular file among its + components would start reporting as an **empty store**. +- `fjs/cas/evo`'s `decodeReadRevision` splits `revision not found` from + `failed to read revision`, because *"calling any of those 'not found' would + deny a stored revision exists"*. It would start denying one. + +Both would fail quietly, in the direction that loses data rather than the one +that raises an error, and neither is a trade this issue is entitled to make on +their behalf. A predicate named for one errno is the wrong place to put a +second one that only some of its callers want. + +So the answer is local: this is the same kind of test as `fileResponse`'s +existing `notRegular` and `tooLarge` cases — what *this server* will answer as +absent. If a later caller wants the same reading, the thing to share is a +named predicate that says so, not a broader `isNotFound`. + +**The branch itself goes in `respond`, not in `fileResponse`.** `fileResponse` +is `(path) => (Result) => ServerResponse`: pure, and holding neither `root` +nor any way to run a `stat`, so the re-check below cannot live there. `respond` +has both, and already ends in `resultMapStep(bytes, r => ok(fileResponse(path)(r)))` +— where `resultMapStep` is by definition `resultStep` over a *pure* function. +Dropping to `resultStep` is the whole change: the `ENOTDIR` case becomes a +`step(stat(served(root)), …)` deciding `404` or `500`, every other case stays +`fileResponse(path)(r)` as today, and `fileResponse` keeps its signature. + +**But validate the root first, or the mapping swallows an operator error.** +`ENOTDIR` does not only arise below a valid root. `fjs web README.md` serves a +regular file as its root, so `join` produces `README.md/index.html` and *every* +request stats a path descending through a file — the same errno, and nothing +inside `fileResponse` can tell it from `/README.md/` under a good root. A +blanket mapping would answer `404 not found` to every request against a +misconfigured server, which is the one case where `500` was telling the +operator something true. + +So the root is checked in `main`, before `listen`: if it is not a directory, +`errorExit` the way an out-of-range port already does. **Both checks stat +`served(root)`, never the argument as written** — `served` maps `''` to `.`, +and `fjs web ''` is a supported invocation with proofs of its own +(`emptyRoot`, twice). Statting the raw argument would make `stat('')` fail +`ENOENT` and reject it at startup, and would misjudge the re-check below under +`respond('')`. That is better than a +per-request comparison of the offending component against the root — it needs +no extra `stat` on the serving path, and it fails at the moment the mistake +was made rather than on someone else's request. + +**But a startup check alone does not establish the invariant the mapping +needs.** Rename the root, or replace it with a regular file, and every later +`stat(root/…)` is `ENOTDIR` from the root itself — which `fileResponse` would +then report as a client-caused `404` for the rest of the process's life. That +is not the request-local window +[stat-then-read](./stat-then-read.md) describes, where two calls race +microseconds apart; this one opens once and stays open, and it turns the +operator's mistake into a lie told to every visitor. An earlier draft of this +file claimed the two windows were the same size. They are not. + +So the mapping re-checks: on `ENOTDIR`, `stat` `served(root)`, and answer +`404` only if it is still a directory — otherwise `500`, which is again the +true answer. +The cost sits where it belongs, since `ENOTDIR` is the rare path and the +serving path is untouched. What remains is a genuine race, between that +re-check and the `stat` that produced the error, and it is the request-local +kind that `stat-then-read` already covers — a wrong status in a vanishing +window rather than a wrong status forever. + +Keep the startup check as well. It is what turns the common case — a mistyped +root — into immediate feedback instead of a `500` that waits for a visitor. + +**A root that is deleted rather than replaced is left as it is, and that is a +cost decision.** Renaming or removing the root makes every later `stat` fail +`ENOENT`, not `ENOTDIR`, so it takes the existing `isNotFound` branch and +answers `404` — the same permanent operator failure reported as client-caused +absence, and no re-check catches it. The symmetric fix would be to validate +the root before accepting any `ENOENT` too, and that is declined here: an +`ENOENT` `404` is the most common answer a static server gives, so this would +put a second `stat` on the hot path to improve a diagnostic, where the +`ENOTDIR` re-check pays nothing on it. That is a trade rather than a +principle, and it should be stated as one. + +Note also that `404` is not *false* in either case — with the root gone or a +file, nothing under it exists. What the `500` buys is telling the operator +which mistake they made, so what is lost by the asymmetry is diagnostic reach, +not correctness. + +The version that answers both, and needs no re-check at all, is holding the +root **open** and resolving beneath the handle, so it cannot be swapped +underneath the server at any point. That is the effect +[stat-then-read](./stat-then-read.md) is already blocked on, and this is a +second reason to want it. + +Worth noticing that this is already the answer on Windows, silently: `stat` +there reports `ENOENT`, so `fjs web README.md` starts happily and answers +`404` to everything — verified. The check makes both hosts say the same true +thing at startup instead of two different misleading things per request. + +**The check needs an operation the effect layer does not have.** `FileStat` is +`{ size, isFile }`, so `isFile === false` is not "is a directory" — it also +covers a FIFO, a device, a socket, and the virtual runner's `JsModule`, whose +`_Entity` is `readonly Vec[] | Dir | JsModule`. Serving any of those as a root +is the same operator error as serving a regular file, so the check has to name +what it wants: **add `isDirectory` to `FileStat`**, in the node runner and the +virtual one together, and reject a root that is not one — including the case +where both flags are false. + +Not `readdir(root)`, which needs no new API and is the obvious alternative. It +answers a different question: a directory may be traversable without being +listable — mode `--x` permits opening a known path under it while `readdir` +fails `EACCES` — so a root that this server can serve perfectly well would be +refused at startup. Reading a whole directory to discard it is the smaller +objection. + +Failure handling is the same `errorExit` either way, and covers two cases: +`stat` failing at all — a root that does not exist — and a root that exists +and is not a directory. Both are the command line being wrong, which is what +`main` already reports that way for a port. + +**Scope: `ENOTDIR` only, and the other two stay at `500` deliberately.** Two +more `stat` failures reach the same directory-form shape and disclose the same +way, on POSIX: + +| request | POSIX | +|---|---| +| `/locked/` — a directory with an `index.html`, mode `000` | `500 io error: EACCES` | +| `/loop1/` — a symlink cycle | `500 io error: ELOOP` | + +Both are left as they are, because the doctrine that makes `ENOTDIR` a `404` +does not reach them. `ENOTDIR` fires on any ordinary file — every served tree +has thousands, so any client can ask — which is what makes it client-caused. A +mode-`000` directory or a symlink cycle is an entry an operator placed, and a +`500` saying the host could not read what it was pointed at is not obviously +the wrong answer. Reopen them on their own evidence, not as a corollary of +this. + +`EISDIR` needs no entry: `stat` succeeds on a directory and `isFile` is false, +so it is already `notRegular` → `404`. + +**All three are POSIX-only.** Windows has none of them — `ENOTDIR` arrives as +`ENOENT` (see the table above), mode `000` does not stop traversal, so +`stat('locked/index.html')` simply succeeds, and a symlink cycle reports +`ENOENT` rather than `ELOOP`. So the oracle is a property of POSIX hosts, and +a proof of its absence has to run on one. + +The obstacle is the same as its sibling's: the virtual file system never +reports `ENOTDIR`, so the branch would be unreachable, which the coverage gate +rejects and `fjs/AGENTS.md` §1.2 says to restructure away rather than leave +uncovered. So this is two changes — teach the virtual file system to refuse a +path that descends through a regular file, then map the error. + +### Tasks + +- [ ] Report `ENOTDIR` from the virtual file system for a path descending + through a regular file. +- [ ] Add `isDirectory` to `FileStat`, in the node runner and the virtual one. +- [ ] Reject a non-directory root in `main`, before `listen` — including a + root that does not exist, and one that is neither file nor directory. + Stat `served(root)`, so `fjs web ''` keeps working; `emptyRoot` in + `proof.f.mjs` pins it. +- [ ] Answer `404` for it from `respond` — `resultStep` in place of + `resultMapStep`, leaving `fileResponse` and `isNotFound` alone — and + only after re-checking that the root is still a directory, so a root + replaced after startup keeps answering `500`. +- [ ] Prove `/README.md/` and `/nope.md/` answer identically, through the + virtual runner — `proof.f.mjs` already drives `respond` that way, so + once the virtual file system reports `ENOTDIR` the proof runs anywhere + and covers the branch on every host. It must not be conditioned on the + host's `stat`, which would leave the new branch uncovered on Windows. + The proof covers `ENOTDIR` and says so: `/locked/` and `/loop1/` stay at + `500` by the scoping above, so it must not claim directory-form requests + disclose nothing in general. +- [ ] Check the real answer on a POSIX host once, separately — the virtual + file system models what the host does, and this is the issue where that + model was wrong on two platforms at once. +- [ ] Update the response table in `module.f.mjs` and + [`../README.md`](../README.md). + +### Related + +- [name-too-long-status](./name-too-long-status.md) — the same shape for + `ENAMETOOLONG`, without the disclosure or the platform split. +- [missing-index-message](./missing-index-message.md) — triggers on the + directory-form request this leaks through. +- `fjs/effects/node/module.f.mjs` — `isNotFound`, the `ENOENT`-only test this + deliberately leaves alone. +- `fjs/effects/node/types.ts` — `FileStat`, which grows `isDirectory` for the + root check. +- `fjs/cas/module.f.mjs` and `fjs/cas/evo/module.f.mjs` — its other two + callers, whose documented readings settle that question. +- `fjs/effects/node/virtual/module.f.mjs` — the file system that would grow the + error, and the one `proof.f.mjs` already drives `respond` through. +- [stat-then-read](./stat-then-read.md) — the request-local replace-underneath + race, which is what the `ENOTDIR` re-check degrades to, and which a startup + check on its own would have been much worse than.