Cap $pad string allocation at 1e7 to prevent unbounded memory use (D2016), mirroring the range operator - #832
Open
youdie006 wants to merge 1 commit into
Open
Conversation
$pad puts no upper bound on the string it builds from its width argument.
Since width comes straight from the expression, a short expression drives an
arbitrarily large allocation: $pad("x", 530000000) builds a ~530MB string, and
above V8's maximum string length (~536,870,888) it throws RangeError. This is
reachable from any untrusted expression, and timeboxExpression does not catch it
(the allocation happens inside a single evaluation step).
Bound the padding length before allocating and throw a descriptive error,
mirroring the range operator (..) which already caps its allocation at 1e7 and
throws D2014 ($toMillis was similarly hardened - CVE-2026-52746, jsonata-js#825). Add a new
D2016 in the same allocation-limit family with the same 1e7 cap and message
shape. Runtime behaviour is unchanged for every valid use (padLength <= 1e7).
Add test/test-suite/groups/function-pad cases: $pad("x", 20000000) now throws
D2016, and a $pad("x", 5) regression.
Fixes jsonata-js#828.
Signed-off-by: manon <youdie006@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
$padputs no upper bound on the string it builds from itswidthargument. Sincewidthcomes straight from the expression, a short expression drives an arbitrarily large allocation:new Array(padLength + 1).join(char)insrc/functions.js(pad) allocates with no cap. Materializing the result (serializing into a response, logging, concatenating, scanning -- the normal path for a transform's output) costs ~530MB per call; above V8's maximum string length (~536,870,888) it throws aRangeError: Invalid string length. A handful of concurrent evaluations is enough to push a normal Node heap over.This is reachable straight from any untrusted expression (query builders, low-code/config tooling, anything that lets a user supply the JSONata). The
timeboxExpressionguardrail does not catch it: the allocation happens inside a single function evaluation step, so the time/depth checks never run during it.Fix
Bound the padding length before allocating and throw a descriptive error, mirroring the existing guardrail on the range operator. The range operator (
..) already caps its allocation at1e7and throwsD2014;$padsimply never got that treatment ($toMilliswas similarly hardened for a resource reason -- CVE-2026-52746, backported in #825). This adds a newD2016in the same allocation-limit family, using the same1e7hard cap and the same message shape asD2014, so the two stay consistent:Runtime behaviour is unchanged for every valid use (
padLength <= 1e7).Tests
Added to
test/test-suite/groups/function-pad/:$pad("x", 20000000)now throwsD2016(previously returned a ~20M-char string).$pad("x", 5)->"x "regression.Existing pad cases (
$pad("foo", 5)->"foo ",$pad("foo", -5)->" foo", etc.) still pass. Full suite green; 100% coverage maintained. Verified red/green: reverting only the src change makes the over-limit case fail (returns the huge string instead of the clean error).Reported by @EchoSkorJjj (#828).
This contribution was prepared with AI assistance and reviewed by the author.
Signed-off-by: manon youdie006@users.noreply.github.com