From 91d3fd7d034e24bcd7bf3fc3e27788fcf21f8684 Mon Sep 17 00:00:00 2001 From: Sean <22557931+sean0407@users.noreply.github.com> Date: Tue, 13 Oct 2026 15:01:37 +0800 Subject: [PATCH 1/2] fix(odf): fail validation when required parts are unreadable --- scripts/odf-validation.mjs | 55 ++++++++++++++++++++ scripts/validate-odf.mjs | 56 ++++++++------------- test/odf-validation.test.mjs | 98 ++++++++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 35 deletions(-) create mode 100644 scripts/odf-validation.mjs create mode 100644 test/odf-validation.test.mjs diff --git a/scripts/odf-validation.mjs b/scripts/odf-validation.mjs new file mode 100644 index 00000000..15896b5d --- /dev/null +++ b/scripts/odf-validation.mjs @@ -0,0 +1,55 @@ +import { mkdirSync, writeFileSync } from "node:fs" +import { dirname, join } from "node:path" + +export async function validateOdfParts({ + documents, + parts, + schema, + manifestSchema, + work, + extract, + validate, + log, +}) { + let failures = 0 + let skipped = 0 + + for (const [label, bytes] of documents) { + log(`\n${label} (${bytes.length} bytes)`) + + const checks = parts.map((part) => [part, schema]) + if (manifestSchema) { + checks.push(["META-INF/manifest.xml", manifestSchema]) + } else { + skipped++ + log(" META-INF/manifest.xml SKIPPED (no manifest schema supplied)") + } + + for (const [part, rng] of checks) { + let xml + try { + xml = await extract(bytes, part) + } catch { + // A required part that could not be extracted is not a successful check. + failures++ + log(` ${part.padEnd(24)} MISSING/UNREADABLE`) + continue + } + + const file = join(work, part.replace(/\//g, "_")) + mkdirSync(dirname(file), { recursive: true }) + writeFileSync(file, xml) + + const errors = validate(file, rng) + if (errors) { + failures++ + log(` ${part.padEnd(24)} INVALID`) + for (const line of errors.split("\n").slice(0, 4)) log(` ${line}`) + } else { + log(` ${part.padEnd(24)} valid`) + } + } + } + + return { failures, skipped } +} diff --git a/scripts/validate-odf.mjs b/scripts/validate-odf.mjs index 7911c993..93dff886 100644 --- a/scripts/validate-odf.mjs +++ b/scripts/validate-odf.mjs @@ -36,10 +36,11 @@ // under its ID/IDREF checking, which is a property of the schema rather // than of any document. -import { mkdtempSync, writeFileSync, mkdirSync } from "node:fs" +import { mkdtempSync, writeFileSync } from "node:fs" import { execFileSync } from "node:child_process" import { tmpdir } from "node:os" -import { join, dirname } from "node:path" +import { join } from "node:path" +import { validateOdfParts } from "./odf-validation.mjs" const args = new Map() for (let i = 2; i < process.argv.length; i += 2) { @@ -193,37 +194,22 @@ function validate(file, rng) { } const work = mkdtempSync(join(tmpdir(), "hucre-odf-val-")) -let failures = 0 - -for (const [label, bytes] of await documents()) { - console.log(`\n${label} (${bytes.length} bytes)`) - - const checks = [...PARTS.map((p) => [p, schema])] - if (manifestSchema) checks.push(["META-INF/manifest.xml", manifestSchema]) - - for (const [part, rng] of checks) { - let xml - try { - xml = await extract(bytes, part) - } catch { - console.log(` ${part.padEnd(24)} (absent)`) - continue - } - - const file = join(work, part.replace(/\//g, "_")) - mkdirSync(dirname(file), { recursive: true }) - writeFileSync(file, xml) - - const errors = validate(file, rng) - if (errors) { - failures++ - console.log(` ${part.padEnd(24)} INVALID`) - for (const line of errors.split("\n").slice(0, 4)) console.log(` ${line}`) - } else { - console.log(` ${part.padEnd(24)} valid`) - } - } -} - -console.log(failures === 0 ? "\nAll parts valid." : `\n${failures} invalid part(s).`) +const { failures, skipped } = await validateOdfParts({ + documents: await documents(), + parts: PARTS, + schema, + manifestSchema, + work, + extract, + validate, + log: console.log, +}) + +const summary = + failures > 0 + ? `\n${failures} invalid or unreadable part(s).` + : skipped > 0 + ? `\nAll checked parts valid; ${skipped} manifest check(s) skipped (no manifest schema supplied).` + : "\nAll parts valid." +console.log(summary) process.exit(failures === 0 ? 0 : 1) diff --git a/test/odf-validation.test.mjs b/test/odf-validation.test.mjs new file mode 100644 index 00000000..125ac668 --- /dev/null +++ b/test/odf-validation.test.mjs @@ -0,0 +1,98 @@ +import { afterEach, describe, expect, it } from "vitest" +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs" +import { tmpdir } from "node:os" +import { delimiter, join } from "node:path" +import { fileURLToPath } from "node:url" +import { spawnSync } from "node:child_process" + +let work + +afterEach(() => { + if (work) rmSync(work, { recursive: true, force: true }) + work = undefined +}) + +function runValidator(unzipMode) { + work = mkdtempSync(join(tmpdir(), "hucre-odf-validation-test-")) + const bin = join(work, "bin") + mkdirSync(bin) + + const dist = join(work, "fake-dist.mjs") + writeFileSync( + dist, + `export async function writeOds() { return new Uint8Array([1]) } +export function writeOdsStream() { + let read = false + return { getReader: () => ({ async read() { + if (read) return { done: true } + read = true + return { done: false, value: new Uint8Array([1]) } + } }) } +} +export class OdsStreamWriter { + addRow() {} + async finish() { return new Uint8Array([1]) } +} +`, + ) + + writeFileSync( + join(bin, "unzip"), + `#!/bin/sh +if [ "$FAKE_UNZIP_MODE" = "missing-settings" ] && [ "$3" != "settings.xml" ]; then + printf '' + exit 0 +fi +exit 1 +`, + { mode: 0o755 }, + ) + writeFileSync(join(bin, "java"), "#!/bin/sh\nexit 0\n", { mode: 0o755 }) + + return spawnSync( + process.execPath, + [ + fileURLToPath(new URL("../scripts/validate-odf.mjs", import.meta.url)), + "--schema", + "unused.rng", + "--jing", + "unused.jar", + "--dist", + dist, + ], + { + cwd: work, + encoding: "utf8", + env: { + ...process.env, + PATH: `${bin}${delimiter}${process.env.PATH}`, + FAKE_UNZIP_MODE: unzipMode, + }, + }, + ) +} + +describe("ODF validator CLI", () => { + it("fails when a required package part is missing", () => { + const result = runValidator("missing-settings") + + expect(result.status).toBe(1) + expect(result.stdout).toContain("settings.xml MISSING/UNREADABLE") + expect(result.stdout).toContain("invalid or unreadable part(s)") + expect(result.stdout).not.toContain("All parts valid.") + }) + + it("fails when the package cannot be read", () => { + const result = runValidator("unreadable") + + expect(result.status).toBe(1) + expect(result.stdout).toContain("MISSING/UNREADABLE") + expect(result.stdout).not.toContain("All parts valid.") + }) + + it("labels manifest validation as skipped when no schema is supplied", () => { + const result = runValidator("unreadable") + + expect(result.stdout).toContain("SKIPPED (no manifest schema supplied)") + }) +}) From 91c7b9c831b721056bb20827857b1a4ccfb471af Mon Sep 17 00:00:00 2001 From: productdevbook Date: Wed, 14 Oct 2026 09:15:09 +0300 Subject: [PATCH 2/2] fix(odf): keep the validator in one file and its test inside the gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validateOdfParts` had a single caller: the test drives the CLI through `spawnSync` and never imports the module, so the extraction bought indirection — eight injected parameters, including `log` — and nothing else. The counting change is back in the loop it belongs to. The test was a `.mjs` file, which no tsconfig project looks at, so it sat outside `bun run test`'s typecheck. It is `.test.ts` now, listed where CONTRIBUTING says a test needing node builtins goes. And `extract` resolves the ZIP reader relative to `--dist` instead of whatever sits in ../dist, which is what makes the stub bundle force the `unzip` fallback: the missing-part run can now assert that the three parts which *did* come out are still reported valid. Co-Authored-By: Claude Opus 5 --- scripts/odf-validation.mjs | 55 --------------- scripts/validate-odf.mjs | 82 ++++++++++++++++------ test/odf-validation.test.mjs | 98 --------------------------- test/odf-validation.test.ts | 128 +++++++++++++++++++++++++++++++++++ tsconfig.cli.json | 1 + tsconfig.json | 3 + 6 files changed, 193 insertions(+), 174 deletions(-) delete mode 100644 scripts/odf-validation.mjs delete mode 100644 test/odf-validation.test.mjs create mode 100644 test/odf-validation.test.ts diff --git a/scripts/odf-validation.mjs b/scripts/odf-validation.mjs deleted file mode 100644 index 15896b5d..00000000 --- a/scripts/odf-validation.mjs +++ /dev/null @@ -1,55 +0,0 @@ -import { mkdirSync, writeFileSync } from "node:fs" -import { dirname, join } from "node:path" - -export async function validateOdfParts({ - documents, - parts, - schema, - manifestSchema, - work, - extract, - validate, - log, -}) { - let failures = 0 - let skipped = 0 - - for (const [label, bytes] of documents) { - log(`\n${label} (${bytes.length} bytes)`) - - const checks = parts.map((part) => [part, schema]) - if (manifestSchema) { - checks.push(["META-INF/manifest.xml", manifestSchema]) - } else { - skipped++ - log(" META-INF/manifest.xml SKIPPED (no manifest schema supplied)") - } - - for (const [part, rng] of checks) { - let xml - try { - xml = await extract(bytes, part) - } catch { - // A required part that could not be extracted is not a successful check. - failures++ - log(` ${part.padEnd(24)} MISSING/UNREADABLE`) - continue - } - - const file = join(work, part.replace(/\//g, "_")) - mkdirSync(dirname(file), { recursive: true }) - writeFileSync(file, xml) - - const errors = validate(file, rng) - if (errors) { - failures++ - log(` ${part.padEnd(24)} INVALID`) - for (const line of errors.split("\n").slice(0, 4)) log(` ${line}`) - } else { - log(` ${part.padEnd(24)} valid`) - } - } - } - - return { failures, skipped } -} diff --git a/scripts/validate-odf.mjs b/scripts/validate-odf.mjs index 93dff886..e02589f7 100644 --- a/scripts/validate-odf.mjs +++ b/scripts/validate-odf.mjs @@ -36,11 +36,11 @@ // under its ID/IDREF checking, which is a property of the schema rather // than of any document. -import { mkdtempSync, writeFileSync } from "node:fs" +import { mkdtempSync, writeFileSync, mkdirSync } from "node:fs" import { execFileSync } from "node:child_process" import { tmpdir } from "node:os" -import { join } from "node:path" -import { validateOdfParts } from "./odf-validation.mjs" +import { join, dirname, resolve } from "node:path" +import { pathToFileURL } from "node:url" const args = new Map() for (let i = 2; i < process.argv.length; i += 2) { @@ -49,7 +49,13 @@ for (let i = 2; i < process.argv.length; i += 2) { const schema = args.get("schema") const manifestSchema = args.get("manifest-schema") const jing = args.get("jing") -const dist = args.get("dist") ?? new URL("../dist/index.mjs", import.meta.url).href +// A URL rather than the path as typed: `extract` resolves the ZIP reader +// relative to it, so `--dist` points at one bundle instead of the bundle +// for the library and whatever happens to sit in ../dist for the reader. +const distArg = args.get("dist") +const dist = distArg + ? pathToFileURL(resolve(distArg)).href + : new URL("../dist/index.mjs", import.meta.url).href if (!schema || !jing) { console.error("usage: node scripts/validate-odf.mjs --schema --jing ") @@ -169,9 +175,7 @@ const PARTS = ["content.xml", "styles.xml", "meta.xml", "settings.xml"] /** Pull one entry out of an ODS with hucre's own ZIP reader. */ async function extract(bytes, path) { - const { ZipReader } = await import(new URL("../dist/zip/reader.mjs", import.meta.url).href).catch( - () => ({}), - ) + const { ZipReader } = await import(new URL("zip/reader.mjs", dist).href).catch(() => ({})) if (ZipReader) return new TextDecoder().decode(await new ZipReader(bytes).extract(path)) // The bundle does not export it; read the workbook back through the // public API is not enough, so fall back to `unzip -p`. @@ -194,22 +198,58 @@ function validate(file, rng) { } const work = mkdtempSync(join(tmpdir(), "hucre-odf-val-")) -const { failures, skipped } = await validateOdfParts({ - documents: await documents(), - parts: PARTS, - schema, - manifestSchema, - work, - extract, - validate, - log: console.log, -}) - -const summary = +let failures = 0 +let skipped = 0 + +for (const [label, bytes] of await documents()) { + console.log(`\n${label} (${bytes.length} bytes)`) + + const checks = PARTS.map((p) => [p, schema]) + if (manifestSchema) { + checks.push(["META-INF/manifest.xml", manifestSchema]) + } else { + // A check that did not run is not a check that passed. Saying so is + // the difference between "valid" and "nobody looked" (#566). + skipped++ + console.log(` ${"META-INF/manifest.xml".padEnd(24)} SKIPPED (no manifest schema supplied)`) + } + + for (const [part, rng] of checks) { + let xml + try { + xml = await extract(bytes, part) + } catch { + // Every part in PARTS is written by all three ODS writers, so one + // that will not come out of the package is a defect rather than an + // absence — and a package that cannot be opened at all lands every + // part here. Counting these as checks that passed is what let the + // script print "All parts valid." for a document it never read + // (#566). + failures++ + console.log(` ${part.padEnd(24)} MISSING/UNREADABLE`) + continue + } + + const file = join(work, part.replace(/\//g, "_")) + mkdirSync(dirname(file), { recursive: true }) + writeFileSync(file, xml) + + const errors = validate(file, rng) + if (errors) { + failures++ + console.log(` ${part.padEnd(24)} INVALID`) + for (const line of errors.split("\n").slice(0, 4)) console.log(` ${line}`) + } else { + console.log(` ${part.padEnd(24)} valid`) + } + } +} + +console.log( failures > 0 ? `\n${failures} invalid or unreadable part(s).` : skipped > 0 ? `\nAll checked parts valid; ${skipped} manifest check(s) skipped (no manifest schema supplied).` - : "\nAll parts valid." -console.log(summary) + : "\nAll parts valid.", +) process.exit(failures === 0 ? 0 : 1) diff --git a/test/odf-validation.test.mjs b/test/odf-validation.test.mjs deleted file mode 100644 index 125ac668..00000000 --- a/test/odf-validation.test.mjs +++ /dev/null @@ -1,98 +0,0 @@ -import { afterEach, describe, expect, it } from "vitest" -import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs" -import { tmpdir } from "node:os" -import { delimiter, join } from "node:path" -import { fileURLToPath } from "node:url" -import { spawnSync } from "node:child_process" - -let work - -afterEach(() => { - if (work) rmSync(work, { recursive: true, force: true }) - work = undefined -}) - -function runValidator(unzipMode) { - work = mkdtempSync(join(tmpdir(), "hucre-odf-validation-test-")) - const bin = join(work, "bin") - mkdirSync(bin) - - const dist = join(work, "fake-dist.mjs") - writeFileSync( - dist, - `export async function writeOds() { return new Uint8Array([1]) } -export function writeOdsStream() { - let read = false - return { getReader: () => ({ async read() { - if (read) return { done: true } - read = true - return { done: false, value: new Uint8Array([1]) } - } }) } -} -export class OdsStreamWriter { - addRow() {} - async finish() { return new Uint8Array([1]) } -} -`, - ) - - writeFileSync( - join(bin, "unzip"), - `#!/bin/sh -if [ "$FAKE_UNZIP_MODE" = "missing-settings" ] && [ "$3" != "settings.xml" ]; then - printf '' - exit 0 -fi -exit 1 -`, - { mode: 0o755 }, - ) - writeFileSync(join(bin, "java"), "#!/bin/sh\nexit 0\n", { mode: 0o755 }) - - return spawnSync( - process.execPath, - [ - fileURLToPath(new URL("../scripts/validate-odf.mjs", import.meta.url)), - "--schema", - "unused.rng", - "--jing", - "unused.jar", - "--dist", - dist, - ], - { - cwd: work, - encoding: "utf8", - env: { - ...process.env, - PATH: `${bin}${delimiter}${process.env.PATH}`, - FAKE_UNZIP_MODE: unzipMode, - }, - }, - ) -} - -describe("ODF validator CLI", () => { - it("fails when a required package part is missing", () => { - const result = runValidator("missing-settings") - - expect(result.status).toBe(1) - expect(result.stdout).toContain("settings.xml MISSING/UNREADABLE") - expect(result.stdout).toContain("invalid or unreadable part(s)") - expect(result.stdout).not.toContain("All parts valid.") - }) - - it("fails when the package cannot be read", () => { - const result = runValidator("unreadable") - - expect(result.status).toBe(1) - expect(result.stdout).toContain("MISSING/UNREADABLE") - expect(result.stdout).not.toContain("All parts valid.") - }) - - it("labels manifest validation as skipped when no schema is supplied", () => { - const result = runValidator("unreadable") - - expect(result.stdout).toContain("SKIPPED (no manifest schema supplied)") - }) -}) diff --git a/test/odf-validation.test.ts b/test/odf-validation.test.ts new file mode 100644 index 00000000..19ab691d --- /dev/null +++ b/test/odf-validation.test.ts @@ -0,0 +1,128 @@ +import { afterEach, describe, expect, it } from "vitest" +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs" +import { tmpdir } from "node:os" +import { delimiter, join } from "node:path" +import { fileURLToPath } from "node:url" +import { spawnSync } from "node:child_process" +import type { SpawnSyncReturns } from "node:child_process" + +// The validator is a developer script, not library code: it needs `jing`, +// a 40 MB OASIS grammar and a JVM, so the suite cannot run it for real. +// What it can run is the script's own bookkeeping — which parts counted +// as checked — against stub `unzip` and `java` on PATH and a stub bundle +// behind `--dist`. That is the part #566 got wrong. + +let work: string | undefined + +afterEach(() => { + if (work) rmSync(work, { recursive: true, force: true }) + work = undefined +}) + +/** The line the script prints for one part, in its own column layout. */ +function line(part: string, status: string): string { + return ` ${part.padEnd(24)} ${status}` +} + +function runValidator(unzipMode: "missing-settings" | "unreadable"): SpawnSyncReturns { + work = mkdtempSync(join(tmpdir(), "hucre-odf-validation-test-")) + const bin = join(work, "bin") + mkdirSync(bin) + + // A stub for every export `documents()` reaches for. Add to the script + // and this list grows with it — the alternative is building dist to + // test the counting. + const dist = join(work, "fake-dist.mjs") + writeFileSync( + dist, + `export async function writeOds() { return new Uint8Array([1]) } +export function writeOdsStream() { + let read = false + return { getReader: () => ({ async read() { + if (read) return { done: true } + read = true + return { done: false, value: new Uint8Array([1]) } + } }) } +} +export class OdsStreamWriter { + addRow() {} + async finish() { return new Uint8Array([1]) } +} +export function createCellStore() { return {} } +export function setCell() {} +`, + ) + + // `unzip -p `: one part refuses to come out, or the whole + // package does. + writeFileSync( + join(bin, "unzip"), + `#!/bin/sh +if [ "$FAKE_UNZIP_MODE" = "missing-settings" ] && [ "$3" != "settings.xml" ]; then + printf '' + exit 0 +fi +exit 1 +`, + { mode: 0o755 }, + ) + // jing, accepting everything it is handed. + writeFileSync(join(bin, "java"), "#!/bin/sh\nexit 0\n", { mode: 0o755 }) + + return spawnSync( + process.execPath, + [ + fileURLToPath(new URL("../scripts/validate-odf.mjs", import.meta.url)), + "--schema", + "unused.rng", + "--jing", + "unused.jar", + "--dist", + dist, + ], + { + cwd: work, + encoding: "utf8", + env: { + ...process.env, + PATH: `${bin}${delimiter}${process.env["PATH"]}`, + FAKE_UNZIP_MODE: unzipMode, + }, + }, + ) +} + +describe("ODF validator CLI", () => { + it("fails when a required package part is missing (#566)", () => { + const result = runValidator("missing-settings") + + expect(result.status).toBe(1) + expect(result.stdout).toContain(line("settings.xml", "MISSING/UNREADABLE")) + // The parts that did come out are still reported as checked, so the + // failure is the missing one rather than a package-wide collapse. + expect(result.stdout).toContain(line("content.xml", "valid")) + expect(result.stdout).toContain(line("styles.xml", "valid")) + expect(result.stdout).toContain(line("meta.xml", "valid")) + // One part × the three documents the script writes. + expect(result.stdout).toContain("3 invalid or unreadable part(s).") + expect(result.stdout).not.toContain("All parts valid.") + }) + + it("fails when the package cannot be read at all (#566)", () => { + const result = runValidator("unreadable") + + expect(result.status).toBe(1) + expect(result.stdout).toContain(line("content.xml", "MISSING/UNREADABLE")) + expect(result.stdout).not.toContain(line("content.xml", "valid")) + // Four parts × three documents, none of them checked. + expect(result.stdout).toContain("12 invalid or unreadable part(s).") + }) + + it("names the manifest check as skipped when no schema is supplied (#566)", () => { + const result = runValidator("unreadable") + + expect(result.stdout).toContain( + line("META-INF/manifest.xml", "SKIPPED (no manifest schema supplied)"), + ) + }) +}) diff --git a/tsconfig.cli.json b/tsconfig.cli.json index 282ec600..88cedea4 100644 --- a/tsconfig.cli.json +++ b/tsconfig.cli.json @@ -22,6 +22,7 @@ "test/cli.test.ts", "test/threaded-comments-write.test.ts", "test/ods-fidelity.test.ts", + "test/odf-validation.test.ts", "test/parity-statement.test.ts", "test/real-files.test.ts", "test/ods-third-party.test.ts", diff --git a/tsconfig.json b/tsconfig.json index c3bd9b42..b8f1d587 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -42,6 +42,9 @@ // reason the file-reading tests above are here. See #474. "test/_reflect.ts", "test/reflect.test.ts", + // Spawns `scripts/validate-odf.mjs` with stub tools on PATH, so it + // needs `node:child_process` and `node:fs`. + "test/odf-validation.test.ts", // Reads the fixture corpus off disk, so it needs `node:fs`. "test/third-party-fixtures.test.ts", "test/ods-third-party.test.ts",