fix: refuse a stray brace, bound value length, and drop a quadratic scan
Addresses Copilot's round-14 review on #2035. Three suppressed comments,
all real; each measured on 1c595e96 before acting.
1. A brace surviving in a literal is an unmatched one, and not a legal
literal character. The SDK's constructor rejects an unclosed `{` but
reads a stray `}` as text, and this parser did the same:
expandUriTemplateStrict("x://a}", {}) => "x://a}"
-- a "URI" carrying a brace, with the panel enabling the read on it.
Both halves are now refused the same way, so the withhold behavior
covers the whole class rather than the half the SDK happened to catch.
2. Replacing `UriTemplate.expand` also removed its 1,000,000-character
per-value ceiling. Nothing else bounded what reached the
allocation-heavy encoders, so a pasted or programmatically supplied
megabyte-plus value went straight through. Restored at the SDK's own
limit -- the guard it was, not a new policy -- and the error names the
offending variable.
3. `templateVariables` re-scanned an expression's whole varspec list for
every one of its names to compute `conforming`, which is quadratic in a
multi-name expression. Templates are server-controlled and the SDK
admits one up to 1 MB, so a single expression can carry thousands of
short comma-separated names, and this runs on the render thread.
Iterating the varspecs supplies the name and its conformance in one
pass.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
1c595e96ee
commit
c6b17c5241
1 changed files with 94 additions and 4 deletions
|
|
@ -801,6 +801,60 @@ describe("a value defined as the empty string", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("unmatched braces", () => {
|
||||
// The SDK's constructor rejects an unclosed `{` but reads a stray `}` as
|
||||
// literal text, and this parser does the same -- so `x://a}` used to expand
|
||||
// to itself, a "URI" carrying a brace, with the panel enabling the read.
|
||||
it.each([
|
||||
["a stray closing brace", "x://a}"],
|
||||
["a closing brace after an expression", "x://{a}}"],
|
||||
["an unclosed opening brace", "x://a/{oops"],
|
||||
])("strict refuses %s", (_label, template) => {
|
||||
// The stray `}` is ours to catch; the unclosed `{` the SDK's constructor
|
||||
// already rejects ("Unclosed template expression"). Either way the read is
|
||||
// withheld, which is the property under test.
|
||||
expect(() => expandUriTemplateStrict(template, { a: "1" })).toThrow(
|
||||
/Unmatched brace|Unclosed template expression/,
|
||||
);
|
||||
});
|
||||
|
||||
it("leaves such a template unexpanded on the display path", () => {
|
||||
expect(expandUriTemplate("x://a}", {})).toBe("x://a}");
|
||||
expect(expandUriTemplate("x://a/{oops", {})).toBe("x://a/{oops");
|
||||
});
|
||||
|
||||
it("still accepts a well-formed expression", () => {
|
||||
expect(expandUriTemplate("x://{a}/b", { a: "1" })).toBe("x://1/b");
|
||||
});
|
||||
});
|
||||
|
||||
describe("the per-value length ceiling", () => {
|
||||
// Replacing `UriTemplate.expand` dropped the SDK's own 1,000,000-character
|
||||
// guard, leaving the allocation-heavy encoders unbounded for a pasted or
|
||||
// programmatically supplied value.
|
||||
it("refuses a value past the limit", () => {
|
||||
const result = tryExpandUriTemplate("x://{v}", {
|
||||
v: "a".repeat(1_000_001),
|
||||
});
|
||||
expect(result.uri).toBeUndefined();
|
||||
expect(result.error).toMatch(/exceeds the 1000000-character limit/);
|
||||
});
|
||||
|
||||
it("accepts a value at the limit", () => {
|
||||
const value = "a".repeat(1_000_000);
|
||||
expect(tryExpandUriTemplate("x://{v}", { v: value }).uri).toBe(
|
||||
`x://${value}`,
|
||||
);
|
||||
});
|
||||
|
||||
it("names the offending variable", () => {
|
||||
expect(
|
||||
tryExpandUriTemplate("x://{a}/{b}", { a: "1", b: "b".repeat(1_000_001) })
|
||||
.error,
|
||||
).toMatch(/"b"/);
|
||||
});
|
||||
});
|
||||
|
||||
describe("tryExpandUriTemplate", () => {
|
||||
it("returns the URI and no error when the template expands", () => {
|
||||
expect(
|
||||
|
|
|
|||
Loading…
Reference in a new issue