fix(elpx): converge the three entry-path normalizers on validate-never-rewrite

.agents/skills/elpx-package-safety/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: elpx-package-safety
-description: "Use when touching ZIP entry handling, entry-path normalization, the Service Worker, or the sandboxed iframe. Entry-path normalization is this app's security boundary and it currently has three implementations that do NOT agree."
+description: "Use when touching ZIP entry handling, entry-path validation, the Service Worker, or the sandboxed iframe. Entry-path validation is this app's security boundary and it has three implementations that must agree exactly, pinned by one shared vector table."
 compatibility: "PHP ZipEntryService, TypeScript src/elpx/paths.ts, and the hand-written Service Worker src/sw/exelearning-sw.js."
 ---
 
@@ -16,14 +16,38 @@ runtime URL: `lib/Service/ZipEntryService.php`, `src/elpx/*`,
 
 A `.elpx` package is a ZIP whose entry names are **attacker-controlled**. Anyone
 who can upload a file to Nextcloud can craft one. The only thing standing between
-a crafted entry name and reading outside the package is path normalization.
+a crafted entry name and reading outside the package is entry-path validation.
 
 `AGENTS.md` states the rule: any new helper that handles entries must call
 `normalizeEntryPath` (TS) or `ZipEntryService::normalizeEntry` (PHP). Follow it.
 Never inline your own check, never "just" `str_replace('..', '')` — that is
 defeated by `....//`.
 
-## There are three implementations, and they disagree
+## The rule: validate, never rewrite
+
+An entry path is accepted **only when it is already canonical**, and it is then
+returned **unchanged**. Rejected outright:
+
+- the empty string;
+- any NUL byte;
+- any backslash;
+- any empty, `.` or `..` segment — which covers leading, doubled and trailing
+  slashes as well as dot segments.
+
+Nothing is ever repaired. `normalizeEntry(x)` is either `x` or `null`.
+
+The reason is that entry names are looked up **verbatim**: `ZipArchive::statName()`
+matches central-directory names byte-for-byte, and the browser keys its in-memory
+map by the stored name. Rewriting `a/b/../c` to `a/c` would therefore hand back a
+*different* entry than the one asked for — an archive can legitimately contain
+both. Dot segments are also unreachable by construction: URL parsers apply
+RFC 3986 §5.2.4 dot-segment removal before a request is dispatched, so a stored
+name containing one can never be addressed over the runtime URL scheme.
+
+Full reasoning, options and evidence:
+[`ADR-96-01`](../../../docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md).
+
+## There are three implementations, and they must agree exactly
 
 | Implementation | Location |
 |---|---|
@@ -33,36 +57,29 @@ defeated by `....//`.
 
 The SW copy is an inline mirror of the TS one — deliberately, because the SW is
 loaded out-of-band by the browser and cannot import bundled application code.
-Those two agree. **The PHP one does not.** Measured behaviour:
-
-| Input | PHP | TS / SW |
-|---|---|---|
-| `a/b/c` | `a/b/c` | `a/b/c` |
-| `../escape` | `null` | `null` |
-| `a/b/../c` | **`null`** | **`a/c`** |
-| `a/./b` | **`null`** | **`a/b`** |
-| `a//b` | **`a//b`** | **`a/b`** |
+**Keep it inline. Do not make it import anything.**
 
-PHP rejects any `.` or `..` segment outright. TS/SW resolve them and only reject
-an attempt to escape the root. PHP also keeps an empty segment from a doubled
-slash; TS/SW collapse it.
+They diverged once, before `ADR-96-01`: PHP rejected `.`/`..` segments while
+TS/SW resolved them, and PHP kept the empty segment from a doubled slash while
+TS/SW collapsed it. It was never a traversal hole — both rejected `../escape` —
+but a package containing `a/b/../c` rendered in the browser and 404'd from the
+PHP asset controller and the preview provider, and the docblock on
+`normalizeEntryPath` claimed the two matched.
 
-**This is not a traversal hole.** Both reject `../escape`; neither escapes the
-package root. It is a *consistency* defect: a package containing `a/b/../c`
-renders in the browser but 404s from the PHP asset controller and the preview
-provider, and the difference is invisible until someone ships such a package.
+### What to do
 
-The docblock on `normalizeEntryPath` claims it "matches the rule used by the
-PHP-side `ZipEntryService`". **That comment is wrong**, which is the dangerous
-part: it tells the next maintainer the two agree.
+- If you change one, change all three in the same commit.
+- Add the case to `tests/fixtures/entry-path-vectors.json`. It is a single file
+  loaded by both test suites, so a divergence fails a test instead of shipping.
+- Loosening or tightening the rule is a behaviour change to a security boundary:
+  supersede `ADR-96-01` rather than editing it, and do it in its own PR.
 
-### What to do about it
+### Resolution is a separate concern
 
-- **Do not** treat the two as interchangeable when reasoning about behaviour.
-- If you change one, change all three, and add a shared test vector table so the
-  divergence cannot silently return.
-- Converging them is a behaviour change to a security boundary. That deserves an
-  ADR and its own PR — not a drive-by edit inside an unrelated change.
+`resolveRelativeEntry` in `src/elpx/paths.ts` *does* resolve `./` and `../`,
+because an href written inside package HTML may legitimately contain them. It
+resolves first and then validates the result with `normalizeEntryPath`. Keep
+that split: hrefs get resolved, stored entry names do not.
 
 ## Service Worker scope
 
@@ -91,9 +108,15 @@ Every deviation from this makes the app responsible for a format it does not own
 
 Entry-path handling is pure and has no excuse for being untested:
 
-- `tests/js/paths.test.ts` — the TS normalizer.
-- `tests/Unit/Service/ZipEntryServiceTest.php` — the PHP one.
+- `tests/fixtures/entry-path-vectors.json` — **the** table. One file, three
+  implementations.
+- `tests/js/paths.test.ts` — runs the table against the TS helper *and* against
+  the shipped Service Worker file, which it evaluates in a `node:vm` context
+  with a stub `self`. That tests the real worker rather than a transcription of
+  it.
+- `tests/Unit/Service/ZipEntryServiceTest.php` — runs the same table against the
+  PHP implementation through a `#[DataProvider]`.
 
 Any new case you reason about — a crafted separator, an empty segment, a NUL
-byte, a Windows path, a unicode look-alike — becomes a test case in both, or it
-is not covered.
+byte, a Windows path, a unicode look-alike — goes in the JSON table, where all
+three pick it up at once.

AGENTS.md

@@ -61,9 +61,13 @@ protocol, the iframe boot HTML pattern). Remove everything Drive-specific.
 - Tests live in `tests/js/**/*.test.ts` (Vitest) and `tests/Unit/**/*.php`
   (PHPUnit). Keep them fast and pure; integration is for CI against a real
   Nextcloud.
-- Path normalization is **the** security boundary for package assets. Any
+- Entry-path validation is **the** security boundary for package assets. Any
   new helper that handles entries must call `normalizeEntryPath` (TS) or
-  `ZipEntryService::normalizeEntry` (PHP).
+  `ZipEntryService::normalizeEntry` (PHP). Both **validate and never rewrite**:
+  an entry path is accepted only when it is already canonical, and it comes
+  back unchanged. The rule is identical in TS, PHP and the Service Worker
+  mirror, pinned by the shared table in
+  `tests/fixtures/entry-path-vectors.json` (`ADR-96-01`).
 
 ## Architecture decision records
 
@@ -106,7 +110,7 @@ that matches before starting:
 |---|---|
 | `architecture-records` | Writing or reviewing an ADR or a change document |
 | `nextcloud-app` | Touching `lib/` — controllers, services, DI, routes, preview |
-| `elpx-package-safety` | Touching ZIP entry handling, path normalization or the Service Worker |
+| `elpx-package-safety` | Touching ZIP entry handling, entry-path validation or the Service Worker |
 | `testing` | Adding or fixing tests in `tests/js/` or `tests/Unit/` |
 
 ## Documentation lookup