diff --git a/.agents/skills/elpx-package-safety/SKILL.md b/.agents/skills/elpx-package-safety/SKILL.md index 99d825b..ad021c4 100644 --- a/.agents/skills/elpx-package-safety/SKILL.md +++ b/.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. diff --git a/AGENTS.md b/AGENTS.md index 52b676e..5f94bea 100644 --- a/AGENTS.md +++ b/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 diff --git a/docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md b/docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md new file mode 100644 index 0000000..51fc02b --- /dev/null +++ b/docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md @@ -0,0 +1,290 @@ +--- +id: ADR-96-01 +title: "Validate .elpx entry paths instead of rewriting them" +status: Proposed +date: 2026-08-05 +tracking_issue: 96 +deciders: + - "@erseco" +reviewers: + - "@erseco" +related: + prs: [96] + changes: [] + adrs: [ADR-93-01] +supersedes: [] +superseded_by: [] +ai_assistance: + tool: "Claude Code" + model: "claude-opus-5" +--- + +# ADR-96-01: Validate .elpx entry paths instead of rewriting them + +## Context + +A `.elpx` package is a ZIP whose entry names are attacker-controlled: anyone who +can upload a file to Nextcloud can craft one. Turning such a name into something +the app will read or serve is this app's security boundary, and it has three +implementations — one per runtime that has to answer the question: + +| Implementation | Location | Reached by | +|---|---|---| +| PHP | `lib/Service/ZipEntryService.php::normalizeEntry` | `AssetController`, `ThumbnailController`, `ElpxPreviewProvider` | +| TypeScript | `src/elpx/paths.ts::normalizeEntryPath` | `zip-reader`, URL builders, `parseRuntimeUrl` | +| Service Worker | `src/sw/exelearning-sw.js::normalizeEntry` | session registration, request matching | + +The Service Worker copy is an inline mirror of the TypeScript one and must stay +inline: the browser loads the worker out-of-band, so it cannot import bundled +application code. + +The three did not agree, and a comment asserted that they did. + +## Problem + +The three implementations must answer identically, because a package is served +through more than one of them: the browser renders it from the Service Worker, +while the server-side `AssetController` fallback and the preview provider read +the same archive in PHP. Which single rule should all three implement? + +## Decision drivers + +- **Exact agreement.** Any input must produce the same answer in all three, or a + package renders through one path and 404s through another. +- **Reviewability.** This is security code. A rule a reviewer can hold in their + head is worth more than a rule that is merely permissive. +- **No surprises for existing content.** Packages that work today should keep + working. +- **Soundness of the lookup.** Whatever the rule returns is used to *find* an + entry. It must not find a different one. + +## Options considered + +### Option 1: Resolve `.` and `..` (adopt the TypeScript behaviour in PHP) + +Keep the browser behaviour and teach PHP to resolve dot segments and collapse +doubled slashes, rejecting only an attempt to climb above the package root. + +- **Pros:** more permissive; accepts sloppily written archives; no change to the + side that renders content today. +- **Cons:** does not actually converge anything — see the evidence below. PHP + looks entries up by their stored name, so a resolved name finds a *different* + entry or none at all. It also cannot be exercised over the runtime URL scheme, + because URL parsers strip dot segments before the request is dispatched. + +### Option 2: Reject `.` and `..` and rewrite nothing (chosen) + +An entry path is accepted only when it is already canonical, and it is returned +unchanged. Rejected: the empty string, any NUL byte, any backslash, and any +empty, `.` or `..` segment — which covers leading, doubled and trailing slashes. + +- **Pros:** `normalizeEntry(x)` is either `x` or `null`, so the three + implementations agree by construction and no lookup can be redirected. It is + one sentence to review. +- **Cons:** stricter than what the browser accepted before, so a package with a + non-canonical entry name now fails to open at all rather than rendering. It + also refuses entry names that are legal on POSIX but not in the ZIP format, + such as a filename containing a backslash. + +### Option 3: Resolve in PHP *and* canonicalize the archive side + +Make PHP enumerate the central directory, normalize each stored name, and match +requests against that map, so a resolved request finds the entry it came from. + +- **Pros:** would make Option 1 genuinely consistent. +- **Cons:** an enumeration per request; two distinct stored names can normalize + to the same key, so it introduces a shadowing decision (which entry wins?) at + the security boundary; and it still cannot help dot-segment names, which never + survive URL parsing. Rejected as more machinery for a worse invariant. + +## Evidence + +All measurements below were reproduced against `origin/main` at commit +`7520972`, on PHP 8.5.9 and Node 26.6.0. + +### The measured divergence + +| 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` | + +This is **not** a traversal vulnerability: every implementation rejected +`../escape`, and none escaped the package root. It was a consistency defect. The +docblock on `normalizeEntryPath` claimed the helper "matches the rule used by the +PHP-side `ZipEntryService`", which was false. + +### ZIP entries are looked up by their stored name + +`ZipArchive::statName()` matches central-directory names verbatim. Against an +archive built with four literal entry names: + +```console +$ php statname.php +central directory names: + [0] "a/b/../c" + [1] "a/c" + [2] "a/./d" + [3] "a//e" +lookups: + statName("a/b/../c") => found, content=DOTSEG + statName("a/c" ) => found, content=PLAIN + statName("a/./d" ) => found, content=DOTCUR + statName("a/d" ) => FALSE + statName("a//e" ) => found, content=DBLSLASH + statName("a/e" ) => FALSE +``` + +`a/b/../c` and `a/c` are two different entries with different contents in the +same archive. A resolving normalizer asked for the first would serve the second. +The browser side has the same property for the opposite reason: `zip-reader` +keys its map by the name it stores, and the Service Worker looks requests up in +that map. + +So Option 1 does not fix the reported symptom. A package whose stored name is +`a/b/../c` would still 404 from the asset route after resolving, because the +route would look up `a/c`. + +### Dot-segment entries are unaddressable over a URL + +URL parsers apply RFC 3986 §5.2.4 dot-segment removal before a request is ever +dispatched, and percent-encoding does not evade it: + +```console +$ node -e '...' +"/base/a/./d" -> pathname "/base/a/d" +"/base/a/b/../c" -> pathname "/base/a/c" +"/base/a//e" -> pathname "/base/a//e" +"/base/a/%2E/d" -> pathname "/base/a/d" +"/base/a/%2E%2E/c" -> pathname "/base/c" +encodeURIComponent(".") = "." encodeURIComponent("..") = ".." +``` + +Both the Service Worker route (`RUNTIME_PREFIX`) and the server-side asset route +(`ASSET_PREFIX`) address entries through a URL path. An entry whose stored name +contains a dot segment therefore cannot be requested at all, whatever the +normalizer decides. Rejecting it states that plainly; resolving it pretends +otherwise. Empty segments do survive URL parsing, which is why `a//b` is a +deliberate choice rather than a forced one — it is rejected for consistency with +the rest of the rule, not because it is unreachable. + +### Real packages are unaffected + +Every `.elp` and `.elpx` file available locally — this repository's fixtures plus +the eXeLearning editor's own test corpus — was scanned for entry names with a +leading slash, a backslash, a `.` or `..` segment, an empty segment, or a NUL +byte: + +```console +scanned 224 packages, 43539 entries +non-canonical entry names: 0 +``` + +The producers of these packages (JSZip and Archiver in the editor; Python's +`zipfile` for the legacy v2 corpus) all emit forward-slash-separated relative +names. The ZIP specification agrees: APPNOTE §4.4.17.1 requires that a stored +name "MUST not contain a drive or device letter, or a leading slash", and that +"all slashes MUST be forward slashes". + +## Decision + +We will make entry-path handling a **validation**, not a transformation. All +three implementations accept a path only when it is already canonical, and +return it unchanged: + +1. Reject the empty string. +2. Reject any path containing a NUL byte or a backslash. +3. Split on `/` and reject the path if any segment is empty, `.` or `..`. +4. Otherwise return the input, byte-identical. + +`normalizeEntry(x)` is therefore `x` or `null`, never a third string. + +Two supporting rules: + +- **Resolution stays separate from validation.** `resolveRelativeEntry` still + applies RFC 3986 dot-segment removal, because an href written inside package + HTML may legitimately contain `./` and `../`. It resolves first and validates + the result. Hrefs get resolved; stored entry names do not. +- **The rule is pinned by one shared table.** + `tests/fixtures/entry-path-vectors.json` is loaded by + `tests/js/paths.test.ts` and by `tests/Unit/Service/ZipEntryServiceTest.php`. + The JavaScript suite additionally evaluates the shipped Service Worker file in + a `node:vm` context with a stub `self`, so the mirror is tested rather than + transcribed. + +We do not attempt to validate an entry name as a *filesystem* path beyond the +above. The name is never used as one — it is a lookup key for `ZipArchive` and +for an in-memory `Map` — so rules about drive letters or reserved device names +would be scope this app cannot justify. + +## Consequences + +### Positive + +- The three implementations agree by construction: the accepted set is defined + by a predicate, and the returned value is the input. +- No request can be redirected onto a different archive entry, because no + request is ever rewritten. +- The rule fits in one sentence, which is what a security reviewer needs. +- A divergence now fails a test: one JSON table drives both suites, and the + Service Worker is executed from its shipped source. +- The false docblock on `normalizeEntryPath` is gone. + +### Negative + +- Stricter than the browser was. A package containing a non-canonical entry name + now fails to open entirely (`ZipReadError` with code `UNSAFE_ENTRY`) where it + previously rendered. Measured impact on 224 real packages: none. +- Entry names that are legal on POSIX but not in the ZIP format — a filename + containing a backslash, most plausibly — are refused. This is a deliberate + trade: refusing is safe, mis-serving is not. +- `readEntry()` no longer tolerates a leading slash on a caller-supplied name. + All in-tree callers pass canonical names. + +### Neutral + +- `resolveRelativeEntry` gained a documented behaviour: a leading slash in an + href addresses the package root. Previously it produced a doubled slash that + the old normalizer silently collapsed. The helper has no production caller + today; it is exercised by tests and kept for the iframe renderer. +- The word "normalize" is kept in both function names. Renaming them would touch + `AGENTS.md`, the skill and every call site for no behavioural gain; the + docblocks now state that the functions validate. + +## Risks + +- **A package in the wild that we have not seen.** The corpus is 224 packages + from this project's own ecosystem, not a survey of everything a third-party + tool might produce. If such a package appears, the failure is loud (the viewer + refuses to open it with a specific error) rather than silent, which is the + right direction, but it is a regression for that user. Superseding this ADR + with a narrower rule is the remedy, not a local patch to one of the three. +- **Drift.** Three implementations of one rule is inherently fragile. The shared + table mitigates it but does not eliminate it: someone could add a case to the + JSON and fix only two implementations. Both suites run in CI, so that fails. + +## Validation + +- `tests/fixtures/entry-path-vectors.json` runs green in both suites, including + the Service Worker mirror loaded from its shipped source. +- Deliberately reverting the empty-segment check in + `src/sw/exelearning-sw.js` fails 6 vector tests, confirming the guardrail + detects a real divergence rather than a transcribed one. +- An accepted path is asserted byte-identical to its input on both sides. + +## Follow-up work + +- Record the Service Worker scope and the iframe sandbox as ADRs; they are still + only prose in `AGENTS.md` and in the `elpx-package-safety` skill. + +## References + +- Skill: [`.agents/skills/elpx-package-safety/SKILL.md`](../../../.agents/skills/elpx-package-safety/SKILL.md) +- Shared vectors: [`tests/fixtures/entry-path-vectors.json`](../../../tests/fixtures/entry-path-vectors.json) +- Prior record noting the divergence as follow-up work: [`ADR-93-01`](ADR-93-01-identify-records-by-github-tracking-number.md) +- RFC 3986 §5.2.4, "Remove Dot Segments" — https://www.rfc-editor.org/rfc/rfc3986#section-5.2.4 +- PKWARE APPNOTE.TXT §4.4.17.1, file name field — https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT +- PHP `ZipArchive::statName()` — https://www.php.net/manual/en/ziparchive.statname.php diff --git a/lib/Service/ZipEntryService.php b/lib/Service/ZipEntryService.php index 93ceec8..8a42797 100644 --- a/lib/Service/ZipEntryService.php +++ b/lib/Service/ZipEntryService.php @@ -33,7 +33,9 @@ public function __construct( * Reads a single entry from the package. Returns the raw bytes or null if * the entry is not present. * - * Path traversal entries (`..`, absolute paths) are rejected. + * Entry names that are not already canonical — traversal, absolute paths, + * dot segments, doubled slashes, backslashes — are rejected outright by + * {@see self::normalizeEntry()} rather than repaired. */ public function readEntry(File $file, string $entry): ?string { $normalized = $this->normalizeEntry($entry); @@ -123,18 +125,32 @@ public function listEntries(File $file): array { } /** - * Returns the canonical form of an entry name or null if the entry is - * unsafe (path traversal, absolute path). + * Validates an entry name and returns it **unchanged**, or null when it is + * not already a canonical path inside the package. + * + * This method never rewrites its input: an accepted name is byte-identical + * to the one passed in. A name is accepted only when it is non-empty, free + * of NUL bytes and backslashes, and made exclusively of segments that are + * non-empty and are neither `.` nor `..`. That rejects leading, doubled and + * trailing slashes, dot segments, and Windows separators. + * + * Rewriting would be unsound here: `readEntry()` hands the result straight + * to `ZipArchive::statName()`, which matches central-directory names + * verbatim, so resolving `a/b/../c` into `a/c` would serve a *different* + * entry than the one that was asked for. + * + * `normalizeEntryPath` in `src/elpx/paths.ts` and its inline mirror in + * `src/sw/exelearning-sw.js` implement exactly this rule. All three are + * tested against the shared table in + * `tests/fixtures/entry-path-vectors.json`; see + * `docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md`. */ public function normalizeEntry(string $entry): ?string { - $entry = ltrim($entry, '/\\'); - $entry = str_replace('\\', '/', $entry); - if ($entry === '' || str_contains($entry, "\0")) { + if ($entry === '' || str_contains($entry, "\0") || str_contains($entry, '\\')) { return null; } - $parts = explode('/', $entry); - foreach ($parts as $part) { - if ($part === '..' || $part === '.') { + foreach (explode('/', $entry) as $segment) { + if ($segment === '' || $segment === '.' || $segment === '..') { return null; } } diff --git a/src/elpx/paths.ts b/src/elpx/paths.ts index fd6efe4..5381890 100644 --- a/src/elpx/paths.ts +++ b/src/elpx/paths.ts @@ -2,7 +2,7 @@ * Path helpers shared between the loader, the service worker client, the * iframe renderer, and the package validator. Everything that touches a ZIP * entry path or builds a virtual runtime URL goes through this module so the - * normalization rules live in one place. + * entry-path rule lives in one place. */ export const RUNTIME_PREFIX = '/apps/exelearning/runtime' @@ -11,38 +11,68 @@ export const ASSET_PREFIX = '/apps/exelearning/asset' const PROTOCOL_LIKE = /^[a-zA-Z][a-zA-Z0-9+.-]*:/ /** - * Returns a canonical, slash-separated path with no `.`/`..` segments and no - * leading slash, or null if the input is not safe (path traversal, absolute - * file URL, NUL byte, ...). + * Validates a ZIP entry path and returns it **unchanged**, or null when it is + * not already a canonical path inside the package. * - * This matches the rule used by the PHP-side {@see ZipEntryService}. + * This function never rewrites its input: an accepted path is byte-identical + * to the one passed in. A path is accepted only when it is non-empty, free of + * NUL bytes and backslashes, and made exclusively of segments that are + * non-empty and are neither `.` nor `..`. That rejects leading, doubled and + * trailing slashes, dot segments, and Windows separators. + * + * Rewriting would be unsound here. Entry names are looked up verbatim in the + * archive (`ZipArchive::statName()` on the PHP side, a `Map` keyed by the + * stored name in the browser), so turning `a/b/../c` into `a/c` addresses a + * different entry than the one that was requested. URL parsers also strip dot + * segments before a request is ever dispatched (RFC 3986 §5.2.4), which means + * an entry whose stored name contains one cannot be addressed over the + * runtime URL scheme at all. + * + * `lib/Service/ZipEntryService.php::normalizeEntry` and the inline mirror in + * `src/sw/exelearning-sw.js` implement exactly this rule. All three are tested + * against the shared table in `tests/fixtures/entry-path-vectors.json`; see + * `docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md`. * @param input Raw entry path as it appears in the archive or in a request. */ export function normalizeEntryPath(input: string): string | null { - if (input.length === 0 || input.includes('\0')) { + if (input.length === 0 || input.includes('\0') || input.includes('\\')) { return null } - const replaced = input.replace(/\\/g, '/').replace(/^\/+/, '') - const parts = replaced.split('/') + for (const segment of input.split('/')) { + if (segment === '' || segment === '.' || segment === '..') { + return null + } + } + return input +} + +/** + * Applies RFC 3986 §5.2.4 dot-segment removal to a package-relative path. + * Returns null when a `..` segment would climb above the package root. + * + * Resolution is deliberately separate from {@link normalizeEntryPath}: an + * href written inside package HTML may legitimately contain `./` and `../`, + * while a *stored entry name* may not. Only hrefs go through here, and the + * result is still validated before it is used. + * @param path Package-relative path that may contain `.`/`..` segments. + */ +function removeDotSegments(path: string): string | null { const stack: string[] = [] - for (const part of parts) { - if (part === '' || part === '.') { + for (const segment of path.split('/')) { + if (segment === '.') { continue } - if (part === '..') { + if (segment === '..') { // `..` is only legitimate when it cancels a directory we have // already entered. An attempt to escape the package root returns - // null so callers can reject the path outright. + // null so callers can reject the href outright. if (stack.length === 0) { return null } stack.pop() continue } - stack.push(part) - } - if (stack.length === 0) { - return null + stack.push(segment) } return stack.join('/') } @@ -50,7 +80,7 @@ export function normalizeEntryPath(input: string): string | null { /** * Resolves a relative resource href against a base entry path (e.g. when the * iframe-loaded page navigates to `./html/page.html`). Returns null if the - * resolution escapes the package root. + * resolution escapes the package root or does not land on a valid entry. * @param baseEntry Entry path of the page that contains the link. * @param href Relative href from inside the package HTML (`./foo`, `../bar`, …). */ @@ -58,11 +88,24 @@ export function resolveRelativeEntry(baseEntry: string, href: string): string | if (isExternalUrl(href)) { return null } - const baseDir = baseEntry.includes('/') + // A leading slash addresses the package root rather than the directory + // containing the page, which is how a browser resolves it against the + // package's own base URL. + const fromRoot = href.startsWith('/') + const baseDir = !fromRoot && baseEntry.includes('/') ? baseEntry.slice(0, baseEntry.lastIndexOf('/')) : '' - const combined = baseDir ? `${baseDir}/${href}` : href - return normalizeEntryPath(combined) + let combined: string + if (fromRoot) { + combined = href.slice(1) + } else { + combined = baseDir ? `${baseDir}/${href}` : href + } + const resolved = removeDotSegments(combined) + if (resolved === null) { + return null + } + return normalizeEntryPath(resolved) } /** diff --git a/src/elpx/zip-reader.ts b/src/elpx/zip-reader.ts index 5a42588..54b2fa3 100644 --- a/src/elpx/zip-reader.ts +++ b/src/elpx/zip-reader.ts @@ -1,7 +1,8 @@ /** * Reads an `.elpx` (ZIP) archive in the browser using fflate. The output is - * keyed by normalized entry path; binary entries stay as `Uint8Array` so the - * Service Worker can hand them back as bytes without re-decoding. + * keyed by the entry's stored name, which `normalizeEntryPath` has validated + * and returned unchanged; binary entries stay as `Uint8Array` so the Service + * Worker can hand them back as bytes without re-decoding. * * Limits are enforced both on the number of entries and on the total * decompressed size to prevent ZIP bombs. @@ -60,10 +61,10 @@ export function looksLikeZip(buffer: ArrayBuffer): boolean { } /** - * Decompresses an `.elpx` archive into a map of normalised entry paths to - * their bytes. Throws {@link ZipReadError} with a typed `code` for any - * limit violation (size, entry count, traversal) or corruption — the - * caller surfaces those as user-facing errors. + * Decompresses an `.elpx` archive into a map of entry paths to their bytes. + * Throws {@link ZipReadError} with a typed `code` for any limit violation + * (size, entry count), corruption, or an entry whose stored name is not a + * canonical package path — the caller surfaces those as user-facing errors. * @param buffer Raw `.elpx` bytes (must already be in memory). * @param limits Optional override of {@link DEFAULT_LIMITS} for tests. */ @@ -114,9 +115,12 @@ export async function readPackage( if (count > limits.maxEntries) { throw new ZipReadError(`Package has more than ${limits.maxEntries} entries`, 'TOO_MANY_ENTRIES') } + // `normalizeEntryPath` validates without rewriting, so `normalized` is + // `rawName` itself. A package is refused whole rather than served with + // some entries silently missing. const normalized = normalizeEntryPath(rawName) if (normalized === null) { - throw new ZipReadError(`Unsafe entry path: ${rawName}`, 'UNSAFE_ENTRY') + throw new ZipReadError(`Entry path is not a canonical package path: ${rawName}`, 'UNSAFE_ENTRY') } totalUncompressed += data.byteLength if (totalUncompressed > limits.maxUncompressedBytes) { diff --git a/src/sw/exelearning-sw.js b/src/sw/exelearning-sw.js index c55402e..8858fe5 100644 --- a/src/sw/exelearning-sw.js +++ b/src/sw/exelearning-sw.js @@ -182,23 +182,28 @@ function safeDecode(value) { * SW-side mirror of `normalizeEntryPath` from src/elpx/paths.ts. Kept * inline because the SW must not import from the bundled application * code (it is loaded out-of-band by the browser, not by webpack). + * + * Validates and returns the input unchanged, or null. A path is accepted + * only when it is non-empty, free of NUL bytes and backslashes, and made + * exclusively of segments that are non-empty and are neither `.` nor `..`. + * Nothing is rewritten: entries are looked up by their stored name, so + * turning `a/b/../c` into `a/c` would serve a different file than the one + * requested. + * + * The same rule lives in `src/elpx/paths.ts` and in + * `lib/Service/ZipEntryService.php`. All three are tested against + * `tests/fixtures/entry-path-vectors.json` — change one and the shared + * table fails until the other two follow. See + * `docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md`. * @param {unknown} input Untrusted entry value coming from a request URL. */ function normalizeEntry(input) { - if (typeof input !== 'string' || input.length === 0 || input.indexOf('\0') >= 0) { + if (typeof input !== 'string' || input.length === 0 + || input.indexOf('\0') >= 0 || input.indexOf('\\') >= 0) { return null } - const cleaned = input.replace(/\\/g, '/').replace(/^\/+/, '') - const parts = cleaned.split('/') - const stack = [] - for (const part of parts) { - if (part === '' || part === '.') continue - if (part === '..') { - if (stack.length === 0) return null - stack.pop() - continue - } - stack.push(part) + for (const segment of input.split('/')) { + if (segment === '' || segment === '.' || segment === '..') return null } - return stack.length === 0 ? null : stack.join('/') + return input } diff --git a/tests/Unit/Service/ZipEntryServiceTest.php b/tests/Unit/Service/ZipEntryServiceTest.php index 6522014..078815c 100644 --- a/tests/Unit/Service/ZipEntryServiceTest.php +++ b/tests/Unit/Service/ZipEntryServiceTest.php @@ -5,6 +5,7 @@ namespace OCA\ExeLearning\Tests\Unit\Service; use OCA\ExeLearning\Service\ZipEntryService; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use RuntimeException; @@ -21,30 +22,80 @@ protected function setUp(): void { $this->service = new ZipEntryService(); } - public function testNormalizesCanonicalEntries(): void { - self::assertSame('index.html', $this->service->normalizeEntry('index.html')); - self::assertSame('html/page.html', $this->service->normalizeEntry('html/page.html')); + /** + * The shared vector table is the contract between the three + * implementations of the entry-path rule: this one, `normalizeEntryPath` + * in `src/elpx/paths.ts`, and the inline Service Worker mirror in + * `src/sw/exelearning-sw.js`. The JavaScript side runs the same file from + * `tests/js/paths.test.ts`. They diverged once — a package with an + * `a/b/../c` entry rendered in the browser but 404'd from the PHP asset + * route — so the table exists to make a divergence fail a test instead of + * shipping. + * + * @return iterable + */ + public static function entryPathVectors(): iterable { + $path = __DIR__ . '/../../fixtures/entry-path-vectors.json'; + $raw = file_get_contents($path); + if (!is_string($raw)) { + throw new RuntimeException("Could not read the shared vector table at {$path}"); + } + $table = json_decode($raw, true, 512, JSON_THROW_ON_ERROR); + if (!is_array($table) || !is_array($table['vectors'] ?? null)) { + throw new RuntimeException('The shared vector table has no "vectors" list'); + } + + foreach ($table['vectors'] as $vector) { + $name = sprintf( + '%s => %s (%s)', + json_encode($vector['input'], JSON_UNESCAPED_UNICODE), + json_encode($vector['expected'], JSON_UNESCAPED_UNICODE), + $vector['why'], + ); + yield $name => [$vector['input'], $vector['expected']]; + } } - public function testStripsLeadingSlashesAndBackslashes(): void { - self::assertSame('html/page.html', $this->service->normalizeEntry('/html/page.html')); - self::assertSame('html/page.html', $this->service->normalizeEntry('html\\page.html')); + #[DataProvider('entryPathVectors')] + public function testAgreesWithTheSharedEntryPathVectors(string $input, ?string $expected): void { + self::assertSame($expected, $this->service->normalizeEntry($input)); } - public function testRejectsParentTraversal(): void { - self::assertNull($this->service->normalizeEntry('../etc/passwd')); - self::assertNull($this->service->normalizeEntry('html/../../etc')); + public function testNeverRewritesAnAcceptedEntry(): void { + // The rule validates, it does not repair: whatever comes back is the + // name that gets looked up verbatim in the archive. + $accepted = 0; + foreach (self::entryPathVectors() as [$input, $expected]) { + if ($expected === null) { + continue; + } + $accepted++; + self::assertSame($input, $expected, 'A vector claims an entry is rewritten on accept'); + self::assertSame($input, $this->service->normalizeEntry($input)); + } + self::assertGreaterThan(0, $accepted, 'The shared table accepts nothing at all'); } - public function testRejectsCurrentDirSegmentToBeStrict(): void { - // `.` segments are intentionally rejected — eXeLearning packages - // never include them and they often indicate a sloppy ZIP writer. + public function testRejectsNearMissEntriesInsteadOfRepairingThem(): void { + // Before the three implementations were converged, this method + // answered 'html/page.html' to all three inputs. Repairing them + // addresses an entry other than the one stored in the archive — + // ZipArchive matches central-directory names verbatim — so they are + // refused instead. + self::assertNull($this->service->normalizeEntry('/html/page.html')); self::assertNull($this->service->normalizeEntry('html/./page.html')); + self::assertNull($this->service->normalizeEntry('html\\page.html')); } - public function testRejectsEmptyAndNulTaintedPaths(): void { - self::assertNull($this->service->normalizeEntry('')); - self::assertNull($this->service->normalizeEntry("a\0b")); + public function testReadEntryRefusesANonCanonicalName(): void { + // The refusal happens before the archive is opened, so a near-miss + // name can never read a different entry than the caller asked for. + $archive = $this->createTestArchive('index.html', '

Playground

'); + $file = $this->createFakeFile($archive, ''); + + self::assertNull($this->service->readEntry($file, '/index.html')); + self::assertNull($this->service->readEntry($file, './index.html')); + self::assertNull($this->service->readEntry($file, 'html/../index.html')); } public function testReadEntryFallsBackToStreamWhenLocalPathCannotBeOpened(): void { diff --git a/tests/fixtures/entry-path-vectors.json b/tests/fixtures/entry-path-vectors.json new file mode 100644 index 0000000..d3f09a3 --- /dev/null +++ b/tests/fixtures/entry-path-vectors.json @@ -0,0 +1,31 @@ +{ + "description": "Shared test vectors for the .elpx entry-path rule. Loaded verbatim by tests/js/paths.test.ts (the TypeScript helper and the Service Worker mirror) and by tests/Unit/Service/ZipEntryServiceTest.php (PHP). One file, three implementations: a divergence between them fails a test instead of shipping.", + "rule": "An entry path is accepted only when it is already canonical, and it is then returned unchanged. Rejected: the empty string, any NUL byte, any backslash, and any empty, '.' or '..' segment (which covers leading, doubled and trailing slashes). Nothing is ever rewritten.", + "adr": "docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md", + "vectors": [ + { "input": "index.html", "expected": "index.html", "why": "canonical root entry" }, + { "input": "a/b/c", "expected": "a/b/c", "why": "canonical nested entry" }, + { "input": "html/page-1.html", "expected": "html/page-1.html", "why": "canonical entry with punctuation" }, + { "input": "content/Página 1.html", "expected": "content/Página 1.html", "why": "non-ASCII and spaces are ordinary name characters" }, + { "input": "..../x", "expected": "..../x", "why": "four dots is a name, not a dot segment" }, + { "input": "../escape", "expected": null, "why": "climbs above the package root" }, + { "input": "html/../../etc", "expected": null, "why": "climbs above the package root after one level" }, + { "input": "a/b/../c", "expected": null, "why": "interior '..' segment: resolving it would address a different archive entry" }, + { "input": "a/b/..", "expected": null, "why": "trailing '..' segment" }, + { "input": "a/./b", "expected": null, "why": "interior '.' segment" }, + { "input": "./a", "expected": null, "why": "leading '.' segment" }, + { "input": ".", "expected": null, "why": "bare '.' is not an entry" }, + { "input": "..", "expected": null, "why": "bare '..' is not an entry" }, + { "input": "a//b", "expected": null, "why": "empty interior segment (doubled slash)" }, + { "input": "....//x", "expected": null, "why": "empty interior segment behind a four-dot name" }, + { "input": "/leading", "expected": null, "why": "absolute path: the ZIP format forbids a leading slash in a stored name" }, + { "input": "/", "expected": null, "why": "nothing but a separator" }, + { "input": "//", "expected": null, "why": "nothing but separators" }, + { "input": "trailing/", "expected": null, "why": "directory entry, not a file entry" }, + { "input": "a\\b", "expected": null, "why": "backslash: the ZIP format requires forward slashes" }, + { "input": "\\leading", "expected": null, "why": "Windows-style absolute path" }, + { "input": "", "expected": null, "why": "empty string" }, + { "input": "a\u0000b", "expected": null, "why": "NUL byte inside an otherwise ordinary name" }, + { "input": "\u0000", "expected": null, "why": "bare NUL byte" } + ] +} diff --git a/tests/js/paths.test.ts b/tests/js/paths.test.ts index 709f439..7951a43 100644 --- a/tests/js/paths.test.ts +++ b/tests/js/paths.test.ts @@ -1,3 +1,6 @@ +import { readFileSync } from 'node:fs' +import { resolve } from 'node:path' +import { createContext, runInContext } from 'node:vm' import { describe, expect, it } from 'vitest' import { buildAssetUrl, @@ -8,30 +11,90 @@ import { resolveRelativeEntry, RUNTIME_PREFIX, } from '../../src/elpx/paths' - -describe('normalizeEntryPath', () => { - it('returns paths unchanged when they are already canonical', () => { - expect(normalizeEntryPath('index.html')).toBe('index.html') - expect(normalizeEntryPath('html/page-1.html')).toBe('html/page-1.html') - }) - - it('strips leading slashes and resolves dots', () => { - expect(normalizeEntryPath('/html/./page.html')).toBe('html/page.html') - expect(normalizeEntryPath('content//x.png')).toBe('content/x.png') - }) - - it('rejects parent-traversal paths', () => { - expect(normalizeEntryPath('../etc/passwd')).toBeNull() - expect(normalizeEntryPath('html/../../etc')).toBeNull() - }) - - it('rejects empty and NUL-tainted paths', () => { - expect(normalizeEntryPath('')).toBeNull() - expect(normalizeEntryPath('a\0b')).toBeNull() +import vectorTable from '../fixtures/entry-path-vectors.json' + +const VECTORS = vectorTable.vectors + +/** + * Loads the Service Worker's inline `normalizeEntry` mirror. + * + * The worker is deliberately not importable — it ships as a classic script + * loaded out-of-band by the browser, so it cannot pull in bundled application + * code (see `src/sw/exelearning-sw.js`). Evaluating the real file in a VM + * context with a stub `self` is therefore the only way to test the shipped + * copy rather than a transcription of it. Top-level function declarations + * become properties of the VM's global object, which is how the mirror is + * reached. + * + * The path is resolved from the Vitest root (`process.cwd()`) because under + * the happy-dom environment `import.meta.url` is not a `file:` URL. + */ +function loadServiceWorkerNormalizer(): (input: unknown) => string | null { + const workerPath = resolve(process.cwd(), 'src/sw/exelearning-sw.js') + const source = readFileSync(workerPath, 'utf8') + const context = createContext({ + self: { + addEventListener: () => {}, + skipWaiting: () => {}, + clients: { claim: () => {} }, + }, + }) as Record + runInContext(source, context, { filename: workerPath }) + const mirror = context.normalizeEntry + if (typeof mirror !== 'function') { + throw new Error('normalizeEntry was not found in the Service Worker source') + } + return mirror as (input: unknown) => string | null +} + +const normalizeEntryInServiceWorker = loadServiceWorkerNormalizer() + +/** + * The shared vector table is the contract between the three implementations + * of the entry-path rule: this TypeScript helper, the Service Worker mirror + * above, and `ZipEntryService::normalizeEntry` in PHP (which runs the same + * table from `tests/Unit/Service/ZipEntryServiceTest.php`). They diverged + * once — a package with an `a/b/../c` entry rendered in the browser but 404'd + * from the PHP asset route — so the table exists to make a divergence fail a + * test instead of shipping. + */ +describe('entry-path rule (shared vectors)', () => { + it('covers every case the rule has to answer', () => { + expect(VECTORS.length).toBeGreaterThanOrEqual(20) + expect(VECTORS.some((vector) => vector.expected !== null)).toBe(true) + }) + + for (const { input, expected, why } of VECTORS) { + const label = `${JSON.stringify(input)} → ${JSON.stringify(expected)} (${why})` + + it(`normalizeEntryPath: ${label}`, () => { + expect(normalizeEntryPath(input)).toBe(expected) + }) + + it(`service worker mirror: ${label}`, () => { + expect(normalizeEntryInServiceWorker(input)).toBe(expected) + }) + } + + it('never rewrites: an accepted path comes back byte-identical', () => { + for (const { input, expected } of VECTORS) { + if (expected !== null) { + expect(expected).toBe(input) + expect(normalizeEntryPath(input)).toBe(input) + expect(normalizeEntryInServiceWorker(input)).toBe(input) + } + } }) +}) - it('normalizes backslashes to forward slashes', () => { - expect(normalizeEntryPath('html\\page.html')).toBe('html/page.html') +describe('normalizeEntryPath', () => { + it('rejects near-miss paths instead of repairing them', () => { + // The pre-convergence helper answered 'html/page.html' to all three. + // Repairing them addresses an entry other than the one stored in the + // archive, so they are refused now. + expect(normalizeEntryPath('/html/page.html')).toBeNull() + expect(normalizeEntryPath('html/./page.html')).toBeNull() + expect(normalizeEntryPath('html\\page.html')).toBeNull() }) }) @@ -44,8 +107,22 @@ describe('resolveRelativeEntry', () => { expect(resolveRelativeEntry('html/sub/page.html', '../image.png')).toBe('html/image.png') }) + it('resolves current-directory references', () => { + expect(resolveRelativeEntry('html/page.html', './image.png')).toBe('html/image.png') + }) + + it('resolves root-relative hrefs against the package root', () => { + expect(resolveRelativeEntry('html/sub/page.html', '/theme/style.css')).toBe('theme/style.css') + }) + it('rejects escapes from the package root', () => { expect(resolveRelativeEntry('html/page.html', '../../etc/passwd')).toBeNull() + expect(resolveRelativeEntry('page.html', '../etc/passwd')).toBeNull() + }) + + it('rejects hrefs that resolve onto a non-canonical entry', () => { + expect(resolveRelativeEntry('html/page.html', 'sub//image.png')).toBeNull() + expect(resolveRelativeEntry('html/page.html', 'sub/')).toBeNull() }) it('refuses to resolve external URLs', () => {