mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-19 21:32:20 +03:00
fix(backend): stop redaction truncating the message after a path (#13295)
* fix(backend): stop redaction truncating the message after a path (#13144) `findUnquotedPathEnd` may swallow the rest of a line when it cannot tell where a path ends, so a Windows path with spaces cannot leak a `Files\secret` suffix. Two things made that fire far wider than the function documents at `:606`. **1. The licence was granted on separator evidence alone.** Every API route carries slashes, so an ordinary `/v1/x/y` in prose qualified as unequivocal and truncated everything after it. The image-model 400 lost the one sentence it exists to deliver: built ...cannot be used on /v1/chat/completions. Use POST /v1/images/generations instead. delivered ...cannot be used on <path> Now only a Windows path, a file URI, or a known POSIX filesystem root may swallow the line. @diegosouzapw's `/zz` vs `/etc` probe on the issue is why this is the condition and not the first-segment check I originally proposed: both truncated identically, so the root was never the driver. **2. The ambiguity branch ran before `resolvedExtensionEnd`.** A path whose end is pinned exactly by a known extension was still treated as ambiguous the moment any prose followed it, so the endpoint was discarded and the line swallowed. A determinable extension leaves nothing to fail closed about -- the whole path is still replaced, suffix included, and the tail survives: before Provider failed in <path> after Provider failed in <path> with api_key='[REDACTED]' Both halves are independently load-bearing: reverting (1) fails the two route tests, reverting (2) fails the extension test. Fail-closed is narrowed, not weakened. `/etc/shadow copy failed` has nothing to anchor an endpoint on and still collapses to `reading <path>`. Worth recording that the guard test for that cannot be killed by mutating either mechanism alone -- the two are mutually redundant, so it takes disabling both, which is also why this change cannot expose a suffix these shapes did not already hide. Test results against base: chat-rejects-image-only-model red -> GREEN dashboard-request-failed-redaction its delivered-log assertion now passes; the test still fails on a second, unrelated assertion (the *internal* log is redacted where it should stay raw) that base never reached tunnel-routes-error-sanitization unchanged, independent (a tripwire asserting the shared sanitizer does NOT cover a shape it now does) 836/840 pass across the sanitization, redaction and error suites; the remaining failures are the two above plus mcp-public-error-boundaries, which passes in isolation on base and on this branch and only flakes under --test-concurrency=8. * docs(changelog): fragment for the redaction truncation fix (#13144) changelog.d/README says a PR adds exactly one fragment rather than editing CHANGELOG.md, so the aggregation order stays deterministic and siblings cannot conflict. This one was missing. * test(redaction): narrow the headline case to the truncation it names Rebased onto a base that has moved 40 commits; resolveEndpoint gained an ignoreAmbiguity parameter and route-context callers in that window. The merge keeps both: base's parameter, plus this PR's two changes (check the resolved extension BEFORE the ambiguity branch, and drop hasFilesystemEvidence from the swallow licence). The headline assertion was that the whole message survives byte for byte. On the current base the tail survives but the route itself still becomes <path> in that particular message, because the quoted model slug earlier in the line carries separators. That is a narrower, separate question from the truncation this PR fixes, so the test now asserts the remediation sentence survives and records the <path> substitution explicitly rather than silently dropping the case. * test(stryker): register the redaction-truncation test for mutation runs errorPathRedaction.ts is mutation-tested, so a new covering test has to be in tap.testFiles or the Stryker sandbox never runs it and its mutants report as survived. Inserted in the alphabetical run beside the sibling error-sensitive-redaction.test.ts, following #13036's precedent. The list has a second, unsorted appended group; left that alone rather than re-sorting a file this PR only needed one line in. Requested in review by @diegosouzapw.
This commit is contained in:
@@ -0,0 +1,97 @@
|
||||
/**
|
||||
* #13144 — error messages were being truncated after a path.
|
||||
*
|
||||
* `findUnquotedPathEnd` may swallow the remainder of a line when it cannot tell
|
||||
* where a path ends, so a Windows path with spaces cannot leak a `Files\secret`
|
||||
* suffix. Two things made that fire far wider than intended:
|
||||
*
|
||||
* 1. The licence was granted on `hasFilesystemEvidence` alone, and every API
|
||||
* route carries slashes — so an ordinary `/v1/x/y` in prose qualified. The
|
||||
* image-model 400 lost the one sentence it exists to deliver:
|
||||
* built: "...cannot be used on /v1/chat/completions. Use POST
|
||||
* /v1/images/generations instead."
|
||||
* delivered: "...cannot be used on <path>"
|
||||
* 2. The ambiguity check ran *before* `resolvedExtensionEnd`, so a path whose
|
||||
* end was pinned exactly by a known extension was treated as ambiguous the
|
||||
* moment any prose followed it — costing the dashboard probe its
|
||||
* ` with api_key='[REDACTED]'` tail.
|
||||
*
|
||||
* Now only an unequivocal prefix with no determinable end swallows the line.
|
||||
* The last test is the other half of the fix: this narrows what fails closed
|
||||
* and must not weaken it.
|
||||
*/
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
const { redactErrorPaths } = await import("../../open-sse/utils/errorPathRedaction.ts");
|
||||
|
||||
test("an API route in prose does not truncate the rest of the message (#13144)", () => {
|
||||
const built =
|
||||
"Model 'huggingface/stabilityai/stable-diffusion-xl-base-1.0' is an image-generation " +
|
||||
"model and cannot be used on /v1/chat/completions. Use POST /v1/images/generations instead.";
|
||||
|
||||
const out = redactErrorPaths(built);
|
||||
|
||||
// The bug this issue names: everything after the route was swallowed, so the
|
||||
// one sentence the 400 exists to deliver never reached the caller.
|
||||
assert.match(out, /Use POST \/v1\/images\/generations instead\.$/);
|
||||
assert.equal(out.endsWith("instead."), true, "the remediation sentence must survive");
|
||||
|
||||
// Documented, not asserted as desirable: in THIS message the route itself is
|
||||
// still replaced with `<path>`, while the same route in isolation survives
|
||||
// verbatim (next test). The difference is the quoted model slug earlier in the
|
||||
// line — `huggingface/stabilityai/...` carries separators, so the route is read
|
||||
// with prior path context. Narrower than the truncation this PR fixes, and a
|
||||
// separate judgement call about how a bare API route should be treated, so it
|
||||
// is recorded here rather than quietly changed.
|
||||
assert.equal(out.includes("<path>"), true);
|
||||
});
|
||||
|
||||
test("separator evidence alone does not license swallowing the line", () => {
|
||||
// The mechanism isolated from the message that surfaced it: a route is
|
||||
// route-shaped, not filesystem-shaped, whatever its first segment is. All
|
||||
// three truncated to "on <path>" before the fix, including the known root.
|
||||
for (const route of ["/v1/chat/completions", "/zz/chat/completions", "/v1/chat"]) {
|
||||
const input = `on ${route}. Contact support.`;
|
||||
assert.equal(redactErrorPaths(input), input, `route ${route} must survive with its tail`);
|
||||
}
|
||||
});
|
||||
|
||||
test("a determinable extension pins the endpoint, so the tail survives", () => {
|
||||
// A known extension says exactly where the path stops, so there is no
|
||||
// ambiguity to fail closed about. The whole path is still replaced —
|
||||
// including its suffix — and the message keeps what followed it.
|
||||
const cases: readonly (readonly [string, string])[] = [
|
||||
["at C:\\Program Files\\secret\\a.ts and more", "at <path> and more"],
|
||||
["failed at /Users/alice/My Project/app.ts and more", "failed at <path> and more"],
|
||||
[
|
||||
"Provider failed in /srv/omniroute/src/private/provider.ts:42:7 with api_key='x'",
|
||||
"Provider failed in <path> with api_key='x'",
|
||||
],
|
||||
];
|
||||
for (const [input, expected] of cases) {
|
||||
const out = redactErrorPaths(input);
|
||||
assert.equal(out, expected);
|
||||
assert.ok(!/secret|alice|omniroute/.test(out), `must not expose a path suffix: ${out}`);
|
||||
}
|
||||
});
|
||||
|
||||
test("an unequivocal prefix with no determinable end still swallows the line", () => {
|
||||
// `/etc/shadow copy failed` has nothing to anchor the endpoint on, so
|
||||
// stopping at the space could publish `shadow copy`. This is the case the
|
||||
// branch exists for, and it must keep firing.
|
||||
const out = redactErrorPaths("reading /etc/shadow copy failed");
|
||||
assert.equal(out, "reading <path>");
|
||||
assert.ok(!/shadow/.test(out), "must not expose a suffix");
|
||||
});
|
||||
|
||||
test("quoted paths and file URIs are unaffected", () => {
|
||||
assert.equal(
|
||||
redactErrorPaths("ENOENT: no such file or directory, open '/Users/alice/secret/app.ts'"),
|
||||
"ENOENT: no such file or directory, open '<path>'"
|
||||
);
|
||||
assert.equal(
|
||||
redactErrorPaths("loading file:///Users/alice/secret/mod.ts failed later"),
|
||||
"loading <path> failed later"
|
||||
);
|
||||
});
|
||||
Reference in New Issue
Block a user