diff --git a/CHANGELOG.md b/CHANGELOG.md index 945b19b..d6862ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,20 @@ All notable changes to bestASR are documented here. The format follows ### Fixed +- **Baseline field validation extracted to a tested lib, and `model` validated + (#116, #115)**: the gate parsed `baseline.json` in two independent inline + heredocs — the worklist stage (`corpus` + `language`) and the model stage + (`entries[0].model`, validated nowhere). Neither was reachable by a test + without running the whole gate, while the compare stage has been a tested lib + since #34. `scripts/lib/baseline-worklist.py` now owns the validation + (`--emit worklist|model`), and **both modes run the complete field pass** — + validating only the emitted field would recreate the divergence that let + `model` go unchecked. `model` gains a whitelist: a single path component + starting alphanumeric, no `/`, `..` rejected outright. It reaches a `cd` + target and a CLI arg, so #112 had not removed the traversal capability — only + moved the payload one field over. The extracted worklist output is + byte-identical to the heredoc's (pinned by sha in the test). + - **Baseline `language` field validated before the worklist TSV (#112)**: `scripts/regression-gate.sh` whitelisted the `corpus` field but wrote `language` into the same line-oriented TSV unchecked, so an embedded newline diff --git a/Tests/BestASRKitTests/RegressionWorklistTests.swift b/Tests/BestASRKitTests/RegressionWorklistTests.swift new file mode 100644 index 0000000..48d9748 --- /dev/null +++ b/Tests/BestASRKitTests/RegressionWorklistTests.swift @@ -0,0 +1,245 @@ +import Foundation +import Testing + +@testable import BestASRKit + +/// Baseline field-validation contracts (#116 extraction, #115 model whitelist). +/// +/// `scripts/lib/baseline-worklist.py` is the single implementation of the +/// gate's baseline-field validation; these tests exercise the REAL script via +/// Process (same discipline as `RegressionBaselineTests` does for +/// `baseline-compare.py`) rather than re-implementing the rules in Swift where +/// they could drift from what `scripts/regression-gate.sh` actually runs. +/// +/// The load-bearing contract here is CROSS-MODE: both `--emit worklist` and +/// `--emit model` run the COMPLETE validation pass. Validating only the fields +/// a given mode emits is exactly the two-heredoc divergence that let `model` +/// reach a path sink (`MODEL_DIR` → `cd`) and a CLI arg (`--models`) unchecked +/// for the whole of #112's lifetime. +struct RegressionWorklistTests { + static let repoRoot = URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() // BestASRKitTests + .deletingLastPathComponent() // Tests + .deletingLastPathComponent() // repo root + + static let script = repoRoot.appendingPathComponent("scripts/lib/baseline-worklist.py") + + /// Runs the real lib against a baseline file, keeping stdout and stderr + /// separate — a negative test must be able to assert that a rejected value + /// never reached stdout (where the gate would consume it). + private func run(emit: String, baselinePath: String) throws -> ( + exit: Int32, stdout: String, stderr: String + ) { + let p = Process() + p.executableURL = URL(fileURLWithPath: "/usr/bin/python3") + p.arguments = [Self.script.path, "--emit", emit, baselinePath] + let outPipe = Pipe() + let errPipe = Pipe() + p.standardOutput = outPipe + p.standardError = errPipe + try p.run() + let out = outPipe.fileHandleForReading.readDataToEndOfFile() + let err = errPipe.fileHandleForReading.readDataToEndOfFile() + p.waitUntilExit() + return ( + p.terminationStatus, + String(data: out, encoding: .utf8) ?? "", + String(data: err, encoding: .utf8) ?? "" + ) + } + + /// Same, for a synthetic baseline written to a temp file. + private func run(emit: String, baseline: Any) throws -> ( + exit: Int32, stdout: String, stderr: String + ) { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("bestasr-worklist-\(UUID().uuidString).json") + try JSONSerialization.data(withJSONObject: baseline).write(to: url) + defer { try? FileManager.default.removeItem(at: url) } + return try run(emit: emit, baselinePath: url.path) + } + + private func entry( + corpus: String = "c1", language: String = "zh", model: String = "large-v3-turbo" + ) -> [String: Any] { + [ + "corpus": corpus, "language": language, "model": model, + "metric": "cer", "golden": 0.10, "tolerance": 0.02, + ] + } + + // MARK: - regression lock: extraction must not change a single byte (#116) + + /// The pre-extraction inline heredoc (regression-gate.sh L40-63) emitted + /// exactly these bytes for benchmarks/baseline.json — captured before the + /// refactor, sha256 e3f18b26660a8132b738d2ffd62c80427d18fa2f7b6eb18194243182ee646d85. + static let pinnedWorklist = """ + jfk\ten + osr-harvard-1\ten + osr-harvard-2\ten + osr-harvard-3\ten + fleurs-ja-1\tja + fleurs-ja-2\tja + fleurs-ja-3\tja + fleurs-ja-4\tja + cv-zhtw-1\tzh + cv-zhtw-2\tzh + cv-zhtw-3\tzh + cv-zhtw-4\tzh + + """ + + @Test func `worklist emit on the real baseline is byte-identical to pre-extraction`() + throws + { + let baseline = Self.repoRoot.appendingPathComponent("benchmarks/baseline.json") + let r = try run(emit: "worklist", baselinePath: baseline.path) + #expect(r.exit == 0) + #expect(r.stdout == Self.pinnedWorklist) + #expect(r.stdout.split(separator: "\n", omittingEmptySubsequences: false).count == 13) + for line in r.stdout.split(separator: "\n") { + #expect(line.split(separator: "\t").count == 2, "not corpus\\tlanguage: \(line)") + } + } + + @Test func `model emit on the real baseline is the reference model`() throws { + let baseline = Self.repoRoot.appendingPathComponent("benchmarks/baseline.json") + let r = try run(emit: "model", baselinePath: baseline.path) + #expect(r.exit == 0) + #expect(r.stdout == "large-v3-turbo\n") + } + + // MARK: - model whitelist (#115) + + /// The empirically reproduced payload: `model` reaches MODEL_DIR (a `cd` + /// target) and `--models` (a CLI arg), so a traversal segment must never + /// leave the validator. + @Test func `a traversal model is rejected and never reaches stdout`() throws { + let payload = "large-v3-turbo/../../../../../../etc" + let r = try run(emit: "model", baseline: [entry(model: payload)]) + #expect(r.exit != 0) + #expect(r.stdout.isEmpty, "forged model leaked to stdout: \(r.stdout)") + #expect(r.stderr.contains("unsafe model in baseline")) + } + + @Test func `path-shaped and metacharacter models are rejected`() throws { + let hostile = [ + "/etc/passwd", // absolute path + "..", // bare traversal + "../large-v3-turbo", // leading traversal + "a/../b", // internal traversal + "large-v3-turbo\nlarge-v2", // newline (record forgery) + "large-v3-turbo\tx", // tab + "large-v3-turbo; rm -rf /", // shell metacharacters + "large-v3-turbo$(whoami)", // command substitution + "large-v3-turbo|cat", // pipe + "large-v3-turbo&", // background + "owner/repo/extra", // more than one path segment + "owner/repo", // ANY slash — model is a single path component (#126 verify) + ".hidden", // leading dot + "", // empty + // These two pass the charset+start rules and are caught ONLY by the + // `".." in model` substring guard. Every other hostile entry above + // fails on some other rule, so without these the guard is unlocked — + // deleting that line would leave the suite green (#126 verify, Codex). + "a..b", + "large-v3..turbo", + ] + for model in hostile { + let r = try run(emit: "model", baseline: [entry(model: model)]) + #expect(r.exit != 0, "accepted hostile model: \(model.debugDescription)") + #expect(r.stdout.isEmpty, "leaked to stdout: \(model.debugDescription)") + // Assert the specific rejection, not merely a non-zero exit — a + // crash or a missing script would otherwise pass this vacuously. + #expect( + r.stderr.contains("unsafe model in baseline"), + "wrong failure for \(model.debugDescription): \(r.stderr)") + } + } + + @Test func `legitimate model names are accepted`() throws { + // Single path components only. `owner/repo` is deliberately NOT here: + // that shape lives in ModelGrid's `hfRepo`, not the baseline `model` + // field, and neither sink accepts a slash (#126 verify). + for model in ["large-v3-turbo", "distil-whisper_v3.5", "tiny.en"] { + let r = try run(emit: "model", baseline: [entry(model: model)]) + #expect(r.exit == 0, "rejected legitimate model: \(model) — \(r.stderr)") + #expect(r.stdout == model + "\n") + } + } + + // MARK: - corpus / language locks carried over from the heredoc + + @Test func `an unsafe corpus name still fails loud`() throws { + let r = try run(emit: "worklist", baseline: [entry(corpus: "../../etc/passwd")]) + #expect(r.exit != 0) + #expect(r.stdout.isEmpty) + #expect(r.stderr.contains("unsafe corpus name in baseline")) + } + + @Test func `a duplicate corpus still fails loud`() throws { + let r = try run(emit: "worklist", baseline: [entry(), entry()]) + #expect(r.exit != 0) + #expect(r.stderr.contains("duplicate corpus in baseline")) + } + + /// #112 regression lock: an embedded newline in `language` forges a second + /// worklist record whose corpus never passed the charset check. + @Test func `a newline-injecting language still fails loud`() throws { + let r = try run( + emit: "worklist", baseline: [entry(language: "zh\n../../etc/passwd\ten")]) + #expect(r.exit != 0) + #expect(r.stdout.isEmpty, "forged record leaked to stdout: \(r.stdout)") + #expect(r.stderr.contains("unsafe language in baseline")) + } + + @Test func `an empty baseline fails loud`() throws { + let r = try run(emit: "worklist", baseline: []) + #expect(r.exit != 0) + #expect(r.stderr.contains("baseline is empty")) + let m = try run(emit: "model", baseline: []) + #expect(m.exit != 0) + #expect(m.stderr.contains("baseline is empty")) + } + + // MARK: - THE contract: both modes run the COMPLETE validation pass + + /// This is the reason #116 and #115 ship together. Under the old two-heredoc + /// layout the model stage read entries[0]["model"] with no validation at + /// all — each stage only looked at the fields it emitted. Emitting the model + /// must therefore still fail on a bad language or a bad corpus, and emitting + /// the worklist must still fail on a bad model. + @Test func `model emit fails on a bad language`() throws { + let r = try run(emit: "model", baseline: [entry(language: "zh\nforged\ten")]) + #expect(r.exit != 0, "model mode skipped language validation — divergence is back") + #expect(r.stdout.isEmpty) + #expect(r.stderr.contains("unsafe language in baseline")) + } + + @Test func `model emit fails on a bad corpus`() throws { + let r = try run(emit: "model", baseline: [entry(corpus: "a b; rm -rf /")]) + #expect(r.exit != 0, "model mode skipped corpus validation — divergence is back") + #expect(r.stdout.isEmpty) + #expect(r.stderr.contains("unsafe corpus name in baseline")) + } + + @Test func `worklist emit fails on a bad model`() throws { + let r = try run( + emit: "worklist", baseline: [entry(model: "large-v3-turbo/../../../etc")]) + #expect(r.exit != 0, "worklist mode skipped model validation — divergence is back") + #expect(r.stdout.isEmpty) + #expect(r.stderr.contains("unsafe model in baseline")) + } + + /// The validation pass covers every entry, not just entries[0] (the one the + /// gate happens to read the reference model from today). + @Test func `a bad field in a later entry fails both modes`() throws { + let baseline: [Any] = [entry(corpus: "c1"), entry(corpus: "c2", model: "../escape")] + for mode in ["worklist", "model"] { + let r = try run(emit: mode, baseline: baseline) + #expect(r.exit != 0, "\(mode) mode validated only the first entry") + #expect(r.stdout.isEmpty) + #expect(r.stderr.contains("unsafe model in baseline")) + } + } +} diff --git a/scripts/lib/baseline-worklist.py b/scripts/lib/baseline-worklist.py new file mode 100755 index 0000000..9cf4629 --- /dev/null +++ b/scripts/lib/baseline-worklist.py @@ -0,0 +1,134 @@ +#!/usr/bin/python3 +"""Regression-gate baseline field-validation + work-list stage (#116, #115). + + baseline-worklist.py --emit worklist|model + + --emit worklist → stdout: one `corpus\\tlanguage` TSV record per entry + --emit model → stdout: the reference model name (entries[0]["model"]) + +CRITICAL CONTRACT — both emit modes run the COMPLETE validation pass. + +This is the whole point of extracting the stage. The gate used to validate +these fields in TWO separate inline heredocs: the work-list heredoc checked +`corpus` and `language`, while the model heredoc read `entries[0]["model"]` +and printed it with NO validation at all — each stage looked only at the +fields it emitted. `model` then flowed into a path sink (MODEL_DIR → `cd`) +and into a CLI arg (`--models`), so a baseline carrying +`large-v3-turbo/../../../../../../etc` escaped the model cache root +unchallenged (#115). Validating per-mode would recreate exactly that +divergence, so validation is one pass over every field of every entry and +runs identically whichever mode is asked for. + +Validation is also complete BEFORE anything is emitted: a bad field in a +later entry must not leave earlier records on stdout. + +This file is the single implementation of these rules — scripts/regression-gate.sh +calls it for both stages and RegressionWorklistTests exercises it via Process, +so the tested behavior is exactly the behavior the gate runs. + +Prereqs: stdlib only, /usr/bin/python3 (Xcode CLT). +""" +import argparse +import json +import re +import sys + +# corpus flows into filesystem paths ($DEST/$corpus.wav) — filesystem-safe +# charset, and a leading dot is rejected so it can never name a dotfile or a +# traversal segment. +CORPUS_RE = re.compile(r"[A-Za-z0-9._-]+") + +# language flows into the TSV alongside corpus (line-oriented `read` split +# downstream) AND into `--language "$language"` as a CLI arg. Without a charset +# whitelist an embedded \n forges a worklist record whose corpus field never +# passed the check above (#112). Same fullmatch discipline as corpus: a +# language tag is [a-z]{2,3} with an optional region subtag. +LANGUAGE_RE = re.compile(r"[a-z]{2,3}(-[A-Za-z0-9]+)?") + +# model reaches BOTH a path sink (MODEL_DIR = "$CACHE_ROOT/openai_whisper-…", +# used as a `cd` target) and a CLI arg (`--models "$MODEL"`), so it gets the +# strictest treatment of the three (#115). +# +# The value must START with an alphanumeric, which rejects `.`, `..` and +# dotfiles outright, and contains NO `/` at all — a single path component. `..` +# is additionally rejected as a substring: the start rule already blocks `..` as +# a whole value, the substring rule additionally blocks `a..b`. Everything +# outside the charset — newline, tab, space, `;`, `|`, `&`, `$`, backtick, +# quotes, redirects, glob — is rejected by the charset alone, and +# `re.fullmatch` (unlike `$`) does not tolerate a trailing newline. +# +# No `/` branch on purpose (#126 verify): HF-style `owner/repo` names live in +# ModelGrid's `hfRepo` field, NOT in the baseline `model` field this validates. +# Both sinks reject slashes anyway — `--models` takes `family/size` (whisper/…, +# see BenchmarkRunner), and a slash in MODEL_DIR yields the unusable +# `…/openai_whisper-owner/repo`. Allowing `/` bought an empty set of working +# values while doubling reachable path depth. +# +# Net effect on the path sink: the value names a single component directly under +# the model cache root, and cannot be absolute (a leading `/` fails the +# alphanumeric-start rule). This is LEXICAL containment — it assumes the cache +# tree itself holds no hostile symlinks, since `cd` resolves those (see #129). +MODEL_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._-]*") + + +def validate(entries): + """Run the complete field-validation pass over every entry. + + Exits non-zero (loud, on stderr) on the first offending field. Returns + (rows, models) where rows is the list of validated (corpus, language) + pairs in baseline order and models the list of validated model names. + """ + if not entries: + sys.exit("✗ gate error: baseline is empty — nothing to gate") + rows = [] + models = [] + seen = set() + for e in entries: + corpus = e["corpus"] + if not CORPUS_RE.fullmatch(corpus) or corpus.startswith("."): + sys.exit(f"✗ gate error: unsafe corpus name in baseline: {corpus!r}") + if corpus in seen: + sys.exit(f"✗ gate error: duplicate corpus in baseline: {corpus}") + seen.add(corpus) + language = e["language"] + if not LANGUAGE_RE.fullmatch(language): + sys.exit(f"✗ gate error: unsafe language in baseline: {language!r}") + model = e.get("model") + if not isinstance(model, str) or ".." in model or not MODEL_RE.fullmatch(model): + sys.exit(f"✗ gate error: unsafe model in baseline: {model!r}") + rows.append((corpus, language)) + models.append(model) + return rows, models + + +def main() -> int: + parser = argparse.ArgumentParser( + description="Validate a regression baseline and emit one of its stages.") + parser.add_argument( + "--emit", required=True, choices=("worklist", "model"), + help="worklist: corpus\\tlanguage TSV; model: the reference model name") + parser.add_argument("baseline", help="path to benchmarks/baseline.json") + # Callers MUST pass the baseline after a `--` terminator (the gate does). + # Without it a path beginning with `-` lands in argparse's option slot, and + # `--help` there exits 0 having validated NOTHING — a validator must have no + # rc=0 path that skips validation (#126 verify, security lens). + args = parser.parse_args() + + with open(args.baseline) as fh: + entries = json.load(fh) + + # One pass, both modes, before any emission (see module docstring). + rows, models = validate(entries) + + if args.emit == "worklist": + for corpus, language in rows: + print(f"{corpus}\t{language}") + else: + # Single fixed canary (design D2): the reference model is the first + # entry's — every entry's model was validated above regardless. + print(models[0]) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/regression-gate.sh b/scripts/regression-gate.sh index e05bad4..99390d7 100755 --- a/scripts/regression-gate.sh +++ b/scripts/regression-gate.sh @@ -24,9 +24,11 @@ BIN="${BESTASR_BIN:-bestasr}" DEST="${BESTASR_CORPORA_DIR:-$HOME/.bestasr/corpora}" BASELINE="${BESTASR_BASELINE:-$(cd "$(dirname "$0")/.." && pwd)/benchmarks/baseline.json}" COMPARE="$(cd "$(dirname "$0")" && pwd)/lib/baseline-compare.py" +WORKLIST="$(cd "$(dirname "$0")" && pwd)/lib/baseline-worklist.py" [ -f "$BASELINE" ] || { echo "✗ baseline not found: $BASELINE" >&2; exit 1; } [ -f "$COMPARE" ] || { echo "✗ compare stage missing: $COMPARE" >&2; exit 1; } +[ -f "$WORKLIST" ] || { echo "✗ worklist stage missing: $WORKLIST" >&2; exit 1; } /usr/bin/python3 -c "import json" >/dev/null 2>&1 \ || { echo "✗ working /usr/bin/python3 required (install Xcode CLT)" >&2; exit 1; } @@ -35,38 +37,16 @@ trap 'rm -rf "$TMP"' EXIT mkdir -p "$TMP/results" # Work list + reference model from the baseline itself (single fixed canary, -# D2). Validates up front: non-empty baseline, unique corpus names, and -# filesystem-safe names (corpus values flow into paths below). -/usr/bin/python3 - "$BASELINE" > "$TMP/worklist.tsv" <<'PY' -import json, re, sys +# D2). Both come from lib/baseline-worklist.py, which runs ONE complete field +# validation pass (non-empty baseline; unique, filesystem-safe corpus names; +# whitelisted language tags; whitelisted model names) regardless of which +# stage is being emitted. These were two separate inline heredocs until #116, +# and each validated only the fields it emitted — which is how `model` reached +# MODEL_DIR (a `cd` target) and `--models` unchecked (#115). set -euo pipefail +# turns any gate error below into an abort before the value is ever used. +/usr/bin/python3 "$WORKLIST" --emit worklist -- "$BASELINE" > "$TMP/worklist.tsv" -entries = json.load(open(sys.argv[1])) -if not entries: - sys.exit("✗ gate error: baseline is empty — nothing to gate") -seen = set() -for e in entries: - corpus = e["corpus"] - if not re.fullmatch(r"[A-Za-z0-9._-]+", corpus) or corpus.startswith("."): - sys.exit(f"✗ gate error: unsafe corpus name in baseline: {corpus!r}") - if corpus in seen: - sys.exit(f"✗ gate error: duplicate corpus in baseline: {corpus}") - seen.add(corpus) - # language flows into the TSV alongside corpus (line-oriented `read` split - # downstream) AND into `--language "$language"` as a CLI arg. Without a - # charset whitelist an embedded \n forges a worklist record whose corpus - # field never passed the check above (#112). Same fullmatch discipline as - # corpus: a language tag is [a-z]{2,3} with an optional region subtag. - language = e["language"] - if not re.fullmatch(r"[a-z]{2,3}(-[A-Za-z0-9]+)?", language): - sys.exit(f"✗ gate error: unsafe language in baseline: {language!r}") - print(f"{corpus}\t{language}") -PY - -MODEL=$(/usr/bin/python3 - "$BASELINE" <<'PY' -import json, sys -print(json.load(open(sys.argv[1]))[0]["model"]) -PY -) +MODEL=$(/usr/bin/python3 "$WORKLIST" --emit model -- "$BASELINE") echo "regression gate: reference model = whisperkit/$MODEL, baseline = $BASELINE" # Model-artifact pin verification (#48): the corpora are digest-pinned