diff --git a/changelog.d/fixes/13295-redaction-truncates-after-path.md b/changelog.d/fixes/13295-redaction-truncates-after-path.md new file mode 100644 index 0000000000..adeeb8a591 --- /dev/null +++ b/changelog.d/fixes/13295-redaction-truncates-after-path.md @@ -0,0 +1 @@ +- **fix(backend):** error messages are no longer truncated after a path — `redactErrorPaths` treated any slash-bearing span as an unequivocal filesystem path and swallowed the rest of the line, so the image-model 400 lost the `Use POST /v1/images/generations instead.` hint it exists to give, and a redacted diagnostic lost its ` with api_key='[REDACTED]'` tail. Only a Windows path, file URI or known POSIX root with no determinable end swallows the line now ([#13144](https://github.com/diegosouzapw/OmniRoute/issues/13144)) diff --git a/open-sse/utils/errorPathRedaction.ts b/open-sse/utils/errorPathRedaction.ts index 24cb50091a..2d29f0e575 100644 --- a/open-sse/utils/errorPathRedaction.ts +++ b/open-sse/utils/errorPathRedaction.ts @@ -465,10 +465,23 @@ function findUnquotedPathEnd( let hasUnresolvedFragments = false; const resolveEndpoint = (ignoreAmbiguity = false): number => { - if (hasUnresolvedFragments && !ignoreAmbiguity) { - return failClosedAmbiguity || hasFilesystemEvidence ? value.length : -1; - } + // A deterministic filename extension pins the endpoint exactly, so there is + // no ambiguity left to fail closed about -- the suffix cannot leak because we + // know where it ends. Checked BEFORE the ambiguity branch, which otherwise + // discarded a resolved endpoint the moment any prose followed it and swallowed + // the rest of the line (#13144: `... provider.ts:42:7 with api_key=...` lost + // its redacted-secret tail). if (resolvedExtensionEnd >= 0) return resolvedExtensionEnd; + if (hasUnresolvedFragments && !ignoreAmbiguity) { + // Only an *unequivocal* prefix — Windows, a file URI, or a known POSIX + // filesystem root — may swallow the rest of the line to avoid exposing a + // suffix like `Files\secret`. Separator evidence alone is not that: + // every API route carries slashes, so treating it as unequivocal made an + // ordinary `/v1/x/y` in prose truncate the message after it, which is + // exactly what this function documents it must not do (#13144). Such a + // span returns -1 and falls back to token-level handling instead. + return failClosedAmbiguity ? value.length : -1; + } if (hasFilesystemEvidence && lastPathTokenEnd >= 0) return lastPathTokenEnd; if ( acceptFirstTokenPunctuation && diff --git a/stryker.conf.json b/stryker.conf.json index a8e56ff1d7..270c7dd468 100644 --- a/stryker.conf.json +++ b/stryker.conf.json @@ -265,6 +265,7 @@ "tests/unit/embeddings-auth.test.ts", "tests/unit/error-classification.test.ts", "tests/unit/error-message-sanitization.test.ts", + "tests/unit/error-path-redaction-route-truncation-13144.test.ts", "tests/unit/error-sanitizer-sk-key-qv45.test.ts", "tests/unit/error-sensitive-redaction.test.ts", "tests/unit/execute-chat-resource-pressure-breaker.test.ts", diff --git a/tests/unit/error-path-redaction-route-truncation-13144.test.ts b/tests/unit/error-path-redaction-route-truncation-13144.test.ts new file mode 100644 index 0000000000..3f6ee8e934 --- /dev/null +++ b/tests/unit/error-path-redaction-route-truncation-13144.test.ts @@ -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 " + * 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 ``, 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(""), 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 " 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 and more"], + ["failed at /Users/alice/My Project/app.ts and more", "failed at and more"], + [ + "Provider failed in /srv/omniroute/src/private/provider.ts:42:7 with api_key='x'", + "Provider failed in 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 "); + 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 ''" + ); + assert.equal( + redactErrorPaths("loading file:///Users/alice/secret/mod.ts failed later"), + "loading failed later" + ); +});