From f84e1a63fd1b69535ee5b9689a8f99aefb56ec8a Mon Sep 17 00:00:00 2001 From: erseco Date: Wed, 5 Aug 2026 13:44:45 +0200 Subject: [PATCH 1/2] fix(elpx): converge the three entry-path normalizers on validate-never-rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PHP, TypeScript and Service Worker implementations of the entry-path rule disagreed. PHP rejected any `.`/`..` segment and kept the empty segment from a doubled slash; TS and the SW mirror resolved dot segments and collapsed doubled slashes. Never a traversal hole — all three rejected `../escape` — but a package containing `a/b/../c` rendered in the browser and 404'd from the PHP asset route and the preview provider, and the docblock on `normalizeEntryPath` claimed the two matched. All three now validate without rewriting: an entry path is accepted only when it is already canonical, and it comes back byte-identical. Rejected: the empty string, NUL bytes, backslashes, and any empty, `.` or `..` segment. Rewriting cannot be made consistent. Entry names are looked up verbatim — `ZipArchive::statName()` matches central-directory names byte-for-byte, and the browser keys its map by the stored name — so resolving `a/b/../c` to `a/c` addresses a different entry than the one requested, and an archive can contain both. Dot segments are unreachable anyway: URL parsers apply RFC 3986 §5.2.4 dot-segment removal before a request is dispatched. `resolveRelativeEntry` still resolves `./` and `../`, because an href inside package HTML may legitimately contain them; it resolves first and validates the result. A leading slash in an href now addresses the package root. One shared table, `tests/fixtures/entry-path-vectors.json`, is loaded by both suites. The Vitest side also evaluates the shipped Service Worker file in a `node:vm` context so the mirror is tested rather than transcribed. --- .agents/skills/elpx-package-safety/SKILL.md | 87 ++++++++------ AGENTS.md | 8 +- lib/Service/ZipEntryService.php | 34 ++++-- src/elpx/paths.ts | 83 ++++++++++---- src/elpx/zip-reader.ts | 18 +-- src/sw/exelearning-sw.js | 31 ++--- tests/Unit/Service/ZipEntryServiceTest.php | 81 ++++++++++--- tests/fixtures/entry-path-vectors.json | 31 +++++ tests/js/paths.test.ts | 121 ++++++++++++++++---- 9 files changed, 374 insertions(+), 120 deletions(-) create mode 100644 tests/fixtures/entry-path-vectors.json diff --git a/.agents/skills/elpx-package-safety/SKILL.md b/.agents/skills/elpx-package-safety/SKILL.md index 99d825b..57d2b5e 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-XXXX-01`](../../../docs/architecture/adr/ADR-XXXX-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-XXXX-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-XXXX-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..fd64e7a 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-XXXX-01`). ## Architecture decision records diff --git a/lib/Service/ZipEntryService.php b/lib/Service/ZipEntryService.php index 93ceec8..7fa2165 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-XXXX-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..4bf172c 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-XXXX-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..e21118d 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-XXXX-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..3e2bf3f --- /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-XXXX-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', () => { From a1162a58b3916ea2c240bc490c0e44bf9234ab58 Mon Sep 17 00:00:00 2001 From: erseco Date: Wed, 5 Aug 2026 13:48:14 +0200 Subject: [PATCH 2/2] docs(architecture): record the entry-path rule as ADR-96-01 Names the record after this pull request, which is the tracking number here because issues are disabled on the repository, and pins the identifier in the three implementations, the shared vector table, AGENTS.md and the elpx-package-safety skill. The skill documented the divergence as an open defect; it now documents the converged rule, why validation cannot be a rewrite, and that the Service Worker copy stays an inline mirror. --- .agents/skills/elpx-package-safety/SKILL.md | 6 +- AGENTS.md | 4 +- ...e-entry-paths-instead-of-rewriting-them.md | 290 ++++++++++++++++++ lib/Service/ZipEntryService.php | 2 +- src/elpx/paths.ts | 2 +- src/sw/exelearning-sw.js | 2 +- tests/fixtures/entry-path-vectors.json | 2 +- 7 files changed, 299 insertions(+), 9 deletions(-) create mode 100644 docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md diff --git a/.agents/skills/elpx-package-safety/SKILL.md b/.agents/skills/elpx-package-safety/SKILL.md index 57d2b5e..ad021c4 100644 --- a/.agents/skills/elpx-package-safety/SKILL.md +++ b/.agents/skills/elpx-package-safety/SKILL.md @@ -45,7 +45,7 @@ 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-XXXX-01`](../../../docs/architecture/adr/ADR-XXXX-01-validate-entry-paths-instead-of-rewriting-them.md). +[`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 @@ -59,7 +59,7 @@ The SW copy is an inline mirror of the TS one — deliberately, because the SW i loaded out-of-band by the browser and cannot import bundled application code. **Keep it inline. Do not make it import anything.** -They diverged once, before `ADR-XXXX-01`: PHP rejected `.`/`..` segments while +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 @@ -72,7 +72,7 @@ PHP asset controller and the preview provider, and the docblock on - 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-XXXX-01` rather than editing it, and do it in its own PR. + supersede `ADR-96-01` rather than editing it, and do it in its own PR. ### Resolution is a separate concern diff --git a/AGENTS.md b/AGENTS.md index fd64e7a..5f94bea 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -67,7 +67,7 @@ protocol, the iframe boot HTML pattern). Remove everything Drive-specific. 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-XXXX-01`). + `tests/fixtures/entry-path-vectors.json` (`ADR-96-01`). ## Architecture decision records @@ -110,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 7fa2165..8a42797 100644 --- a/lib/Service/ZipEntryService.php +++ b/lib/Service/ZipEntryService.php @@ -143,7 +143,7 @@ public function listEntries(File $file): array { * `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-XXXX-01-validate-entry-paths-instead-of-rewriting-them.md`. + * `docs/architecture/adr/ADR-96-01-validate-entry-paths-instead-of-rewriting-them.md`. */ public function normalizeEntry(string $entry): ?string { if ($entry === '' || str_contains($entry, "\0") || str_contains($entry, '\\')) { diff --git a/src/elpx/paths.ts b/src/elpx/paths.ts index 4bf172c..5381890 100644 --- a/src/elpx/paths.ts +++ b/src/elpx/paths.ts @@ -31,7 +31,7 @@ const PROTOCOL_LIKE = /^[a-zA-Z][a-zA-Z0-9+.-]*:/ * `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-XXXX-01-validate-entry-paths-instead-of-rewriting-them.md`. + * `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 { diff --git a/src/sw/exelearning-sw.js b/src/sw/exelearning-sw.js index e21118d..8858fe5 100644 --- a/src/sw/exelearning-sw.js +++ b/src/sw/exelearning-sw.js @@ -194,7 +194,7 @@ function safeDecode(value) { * `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-XXXX-01-validate-entry-paths-instead-of-rewriting-them.md`. + * `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) { diff --git a/tests/fixtures/entry-path-vectors.json b/tests/fixtures/entry-path-vectors.json index 3e2bf3f..d3f09a3 100644 --- a/tests/fixtures/entry-path-vectors.json +++ b/tests/fixtures/entry-path-vectors.json @@ -1,7 +1,7 @@ { "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-XXXX-01-validate-entry-paths-instead-of-rewriting-them.md", + "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" },