mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-10 22:20:14 +00:00
fix(core,producer,skills): unicode paths, non-Error rejections, shell callers
Three R3 findings.
The redactor's segment classes were ASCII `\w`, so `/数据/客户/秘密视频.mp4` and
`/data/客户/secret.mp4` went out verbatim — and the generic redactor also feeds
CLI telemetry and producer observation messages, where no known-path list
compensates. Segments are now defined by their delimiters instead of an
alphabet, which is correct for every script by construction rather than
requiring Unicode classes to be kept correct. The bare-relative lookbehind had
the same ASCII assumption and let a match start mid-token, redacting
`客户/秘密/视频.mp4` to `客户[path]`; it is now a token boundary, and
bare-relative runs before absolute so it claims the whole token.
sanitizeProbeFailure cast the rejection reason to Error and read `.message`.
An injected probe can reject with anything, so `Promise.reject("failed")` gave
`undefined` and threw inside the redactor — converting a returned failure
result into a rejected promise. Normalized at the boundary, and
redactKnownPaths no longer throws on a non-string.
The contract only admitted .ts/.js/.mjs/.cjs, so it missed shipped shell and
Python callers. frame_strip.sh passed a user-controlled path as ffprobe's last
positional with no terminator; render-and-composite.sh had four more. Both
fixed, and the sweep now covers .py/.sh. Python list argvs are bracket
literals so they get the same position check; shell command lines get a
separate presence check, because checking position there needs a shell parser
— stated as the weaker guarantee it is rather than implied to be equal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
6c5403f7cd
commit
1664fe6ad7
@@ -337,4 +337,45 @@ describe("PadTrimAudioResult.error never carries the input path", () => {
|
||||
expect(result.error ?? "").not.toContain("acme-secret");
|
||||
expect(result.error ?? "").toContain("failed to probe audio");
|
||||
});
|
||||
|
||||
// An injected probe can reject with anything. Casting the reason to Error and
|
||||
// reading `.message` yielded undefined, which threw inside the redactor and
|
||||
// turned a returned failure result into a rejected promise.
|
||||
describe("a probe that rejects with a non-Error value", () => {
|
||||
const nonErrors: Array<[string, unknown]> = [
|
||||
["a string", "probe failed"],
|
||||
["undefined", undefined],
|
||||
["null", null],
|
||||
["a number", 42],
|
||||
["a plain object", { code: "ENOENT" }],
|
||||
];
|
||||
|
||||
for (const [label, reason] of nonErrors) {
|
||||
it(`still returns a failed result when the video probe rejects with ${label}`, async () => {
|
||||
const result = await padOrTrimAudioToVideoFrameCount({
|
||||
videoPath: "/data/acme-secret/video.mp4",
|
||||
audioPath: "/tmp/audio.aac",
|
||||
outputPath: "/tmp/out.aac",
|
||||
probeVideoFrameInfo: () => Promise.reject(reason),
|
||||
probeAudioInfo: () => Promise.resolve({ durationSeconds: 1 }),
|
||||
runFfmpeg: () => Promise.resolve({ success: true }),
|
||||
});
|
||||
expect(result.success).toBe(false);
|
||||
expect(result.error ?? "").toContain("failed to probe video");
|
||||
});
|
||||
|
||||
it(`still returns a failed result when the audio probe rejects with ${label}`, async () => {
|
||||
const result = await padOrTrimAudioToVideoFrameCount({
|
||||
videoPath: "/tmp/v.mp4",
|
||||
audioPath: "/data/acme-secret/audio.aac",
|
||||
outputPath: "/tmp/out.aac",
|
||||
probeVideoFrameInfo: () => Promise.resolve({ frameCount: 30, fpsNum: 30, fpsDen: 1 }),
|
||||
probeAudioInfo: () => Promise.reject(reason),
|
||||
runFfmpeg: () => Promise.resolve({ success: true }),
|
||||
});
|
||||
expect(result.success).toBe(false);
|
||||
expect(result.error ?? "").toContain("failed to probe audio");
|
||||
});
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -230,7 +230,13 @@ function formatSeconds(sec: number): string {
|
||||
* recognise them), then the generic shape-based scrub for anything the message
|
||||
* picked up elsewhere.
|
||||
*/
|
||||
function sanitizeProbeFailure(message: string, paths: readonly string[]): string {
|
||||
function sanitizeProbeFailure(reason: unknown, paths: readonly string[]): string {
|
||||
// Normalized here, not at the call sites. A caller-supplied probe can reject
|
||||
// with anything — `Promise.reject("probe failed")` has no `.message`, so
|
||||
// casting to Error yielded `undefined` and threw inside the redactor. That
|
||||
// turned a returned failure result into a rejected promise, which is a
|
||||
// behaviour regression the cast introduced.
|
||||
const message = reason instanceof Error ? reason.message : String(reason);
|
||||
return redactTelemetryString(redactKnownPaths(message, paths));
|
||||
}
|
||||
|
||||
@@ -261,7 +267,7 @@ export async function padOrTrimAudioToVideoFrameCount(
|
||||
0,
|
||||
audioResult.status === "fulfilled" ? audioResult.value.durationSeconds : 0,
|
||||
`audioPadTrim: failed to probe video: ${sanitizeProbeFailure(
|
||||
(videoResult.reason as Error).message,
|
||||
videoResult.reason,
|
||||
probePaths,
|
||||
)}`,
|
||||
);
|
||||
@@ -272,7 +278,7 @@ export async function padOrTrimAudioToVideoFrameCount(
|
||||
0,
|
||||
0,
|
||||
`audioPadTrim: failed to probe audio: ${sanitizeProbeFailure(
|
||||
(audioResult.reason as Error).message,
|
||||
audioResult.reason,
|
||||
probePaths,
|
||||
)}`,
|
||||
);
|
||||
|
||||
@@ -32,7 +32,20 @@ const REPO_ROOT = join(import.meta.dirname, "..", "..", "..", "..");
|
||||
const SWEEP_ROOTS = ["packages", "skills", "scripts"];
|
||||
|
||||
/** `.mjs`/`.cjs` are first-class here — the skill scripts are not TypeScript. */
|
||||
const SOURCE_EXT = /\.(?:ts|mjs|cjs|js)$/;
|
||||
const SOURCE_EXT = /\.(?:ts|mjs|cjs|js|py|sh)$/;
|
||||
|
||||
/**
|
||||
* Shell-syntax invocations, checked separately and more weakly.
|
||||
*
|
||||
* A JS/Python argv is a bracketed literal, so the parser above can check that
|
||||
* `--` is the PENULTIMATE entry. A shell command line is not a literal —
|
||||
* `ffprobe -v error ... "$BG" 2>/dev/null | tr -dc '0-9.'` has redirections,
|
||||
* pipes and substitutions after the input — so checking position would need a
|
||||
* shell parser. This asserts the terminator is PRESENT on any ffprobe command
|
||||
* line, which is weaker but is the part that was missing, and it is honest
|
||||
* about being weaker rather than implying the same guarantee.
|
||||
*/
|
||||
const SHELL_EXT = /\.sh$/;
|
||||
|
||||
/**
|
||||
* The caller set as of the sweep that introduced this contract.
|
||||
@@ -139,11 +152,15 @@ function isSourceFile(entry: string): boolean {
|
||||
return true;
|
||||
}
|
||||
|
||||
function discoverCallers(): { found: string[]; unclassified: string[] } {
|
||||
function discoverCallers(): { found: string[]; unclassified: string[]; shell: string[] } {
|
||||
const found: string[] = [];
|
||||
const unclassified: string[] = [];
|
||||
const shell: string[] = [];
|
||||
const classify = (abs: string): void => {
|
||||
const src = readFileSync(abs, "utf8");
|
||||
if (SHELL_EXT.test(abs) && /(?:^|[^\w-])ffprobe\s+-/m.test(src)) {
|
||||
shell.push(relative(REPO_ROOT, abs));
|
||||
}
|
||||
// Discovery is ARGV-shaped, not call-shaped. Matching on spawn/execFile
|
||||
// misses a dependency-injected runner — `runner("ffprobe", [...])` in
|
||||
// studio-server's mediaValidation.ts is exactly that, and a call-shaped
|
||||
@@ -168,7 +185,7 @@ function discoverCallers(): { found: string[]; unclassified: string[] } {
|
||||
/* root absent in a partial checkout */
|
||||
}
|
||||
}
|
||||
return { found: found.sort(), unclassified: unclassified.sort() };
|
||||
return { found: found.sort(), unclassified: unclassified.sort(), shell: shell.sort() };
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -211,6 +228,32 @@ function argvTails(source: string): Array<{ snippet: string; tail: string[] }> {
|
||||
return tails;
|
||||
}
|
||||
|
||||
describe("shell ffprobe invocations terminate their options", () => {
|
||||
const shellFiles = discoverCallers().shell;
|
||||
|
||||
it("finds the shell callers", () => {
|
||||
// Guards the guard: `frame_strip.sh` and `render-and-composite.sh` both
|
||||
// shipped un-terminated while the JS-only sweep reported the class closed.
|
||||
expect(shellFiles.length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it.each(shellFiles)("%s passes -- on every ffprobe command line", (relPath) => {
|
||||
const source = readFileSync(join(REPO_ROOT, relPath), "utf8");
|
||||
const offenders = source
|
||||
.split("\n")
|
||||
.map((line, index) => ({ line: line.trim(), number: index + 1 }))
|
||||
// An invocation passes flags. `command -v ffprobe >/dev/null` is a PATH
|
||||
// check and `echo "ffmpeg/ffprobe not on PATH"` is a message; neither
|
||||
// takes an input, and both were reported before this narrowed.
|
||||
.filter(({ line }) => /(?:^|[^\w-])ffprobe\s+-/.test(line) && !line.startsWith("#"))
|
||||
.filter(({ line }) => !/\b(?:command\s+-v|which|type)\s+ffprobe/.test(line))
|
||||
.filter(({ line }) => !/\s--\s/.test(line))
|
||||
.map(({ line, number }) => `${number}: ${line.slice(0, 80)}`);
|
||||
|
||||
expect(offenders, `${relPath}: ffprobe command lines missing "--"`).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("ffprobe argv contract", () => {
|
||||
const { found: callers, unclassified } = discoverCallers();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user