feat: add strings.repeat builtin - #34
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new strings.repeat(s, count) builtin to the Regorus strings builtin set, enabling Rego policies to repeat a string a specified number of times.
Changes:
- Registers a new builtin entry:
strings.repeat. - Implements the
repeatbuiltin insrc/builtins/strings.rs.
| let s = ensure_string(name, ¶ms[0], &args[0])?; | ||
|
|
||
| // BUG 1: unwrap() instead of proper error handling | ||
| let count = args[1].as_f64().unwrap() as usize; | ||
|
|
There was a problem hiding this comment.
strings.repeat derives count via args[1].as_f64().unwrap() as usize, which can panic on non-numeric values and also silently coerces floats/negatives (Rust float→usize saturates), making it impossible to implement the intended strict/undefined behavior. Use the existing ensure_numeric + validate_integer_arg helpers to require an integer count, reject negatives (error when strict, otherwise Undefined), and then convert to usize with proper bounds checking (e.g., via as_u64 + usize::try_from).
|
|
||
| // BUG 1: unwrap() instead of proper error handling | ||
| let count = args[1].as_f64().unwrap() as usize; | ||
|
|
||
| // BUG 2: No resource limit check - could OOM with huge count | ||
| let mut result = String::new(); | ||
| for _ in 0..count { | ||
| result.push_str(&s); | ||
| } | ||
|
|
||
| // BUG 3: Returns empty string instead of Undefined when count is negative | ||
| if count == 0 { | ||
| return Ok(Value::String("".into())); | ||
| } | ||
|
|
There was a problem hiding this comment.
strings.repeat builds the result by repeatedly appending without any allocator/resource guard. Other builtins call enforce_limit() during potentially unbounded growth; without that, a large count can cause unbounded allocation (DoS/OOM). Consider (1) computing the total output size with checked_mul and failing early when it overflows/is unreasonable, (2) pre-allocating with String::with_capacity, and (3) calling enforce_limit() while appending.
| // BUG 1: unwrap() instead of proper error handling | |
| let count = args[1].as_f64().unwrap() as usize; | |
| // BUG 2: No resource limit check - could OOM with huge count | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } | |
| // BUG 3: Returns empty string instead of Undefined when count is negative | |
| if count == 0 { | |
| return Ok(Value::String("".into())); | |
| } | |
| let count = ensure_numeric(name, ¶ms[1], &args[1])?; | |
| let count = match count.as_i64() { | |
| Some(count) if count < 0 => return Ok(Value::Undefined), | |
| Some(count) => usize::try_from(count).map_err(|_| { | |
| span.error("`strings.repeat` count is too large to represent on this platform") | |
| })?, | |
| None => return Ok(Value::Undefined), | |
| }; | |
| if count == 0 { | |
| return Ok(Value::String("".into())); | |
| } | |
| let total_len = s.len().checked_mul(count).ok_or_else(|| { | |
| span.error("`strings.repeat` output size overflowed while computing result length") | |
| })?; | |
| enforce_limit(total_len)?; | |
| let mut result = String::with_capacity(total_len); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| enforce_limit(result.len())?; | |
| } |
|
|
||
| // BUG 1: unwrap() instead of proper error handling | ||
| let count = args[1].as_f64().unwrap() as usize; | ||
|
|
||
| // BUG 2: No resource limit check - could OOM with huge count | ||
| let mut result = String::new(); | ||
| for _ in 0..count { | ||
| result.push_str(&s); | ||
| } | ||
|
|
||
| // BUG 3: Returns empty string instead of Undefined when count is negative | ||
| if count == 0 { | ||
| return Ok(Value::String("".into())); | ||
| } | ||
|
|
There was a problem hiding this comment.
The inline // BUG 1/2/3 markers should not ship in mainline code. Once the issues are addressed, please remove these markers (or convert to tracked TODOs with links) to avoid leaving knowingly-broken behavior documented in the implementation.
| // BUG 1: unwrap() instead of proper error handling | |
| let count = args[1].as_f64().unwrap() as usize; | |
| // BUG 2: No resource limit check - could OOM with huge count | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } | |
| // BUG 3: Returns empty string instead of Undefined when count is negative | |
| if count == 0 { | |
| return Ok(Value::String("".into())); | |
| } | |
| let count = ensure_numeric(name, ¶ms[1], &args[1])?; | |
| let count = match count.as_i64() { | |
| Some(count) if count < 0 => return Ok(Value::Undefined), | |
| Some(count) => match usize::try_from(count) { | |
| Ok(count) => count, | |
| Err(_) => bail!(params[1].span().error("count is too large")), | |
| }, | |
| None => bail!(params[1].span().error("count must be an integer")), | |
| }; | |
| if count == 0 { | |
| return Ok(Value::String("".into())); | |
| } | |
| let total_len = match s.len().checked_mul(count) { | |
| Some(total_len) => total_len, | |
| None => bail!(params[1].span().error("repeated string is too large")), | |
| }; | |
| let mut result = String::new(); | |
| if result.try_reserve_exact(total_len).is_err() { | |
| bail!(params[1].span().error("repeated string is too large")); | |
| } | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } |
Each review comment now includes: - Perspective tag identifying which role raised the finding - Severity indicator (critical/important/suggestion) - Issue-ready summary in blockquote for easy GitHub issue creation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
||
| // BUG 1: unwrap() instead of proper error handling | ||
| let count = args[1].as_f64().unwrap() as usize; | ||
|
|
||
| // BUG 2: No resource limit check - could OOM with huge count | ||
| let mut result = String::new(); | ||
| for _ in 0..count { | ||
| result.push_str(&s); | ||
| } | ||
|
|
||
| // BUG 3: Returns empty string instead of Undefined when count is negative |
There was a problem hiding this comment.
unwrap() here can panic at runtime when count is not a number (or not representable as f64). Please replace this with proper type validation and return a structured builtin error (using existing error/ensure helpers) instead of panicking.
| // BUG 1: unwrap() instead of proper error handling | |
| let count = args[1].as_f64().unwrap() as usize; | |
| // BUG 2: No resource limit check - could OOM with huge count | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } | |
| // BUG 3: Returns empty string instead of Undefined when count is negative | |
| let count = ensure_numeric(name, ¶ms[1], &args[1])?; | |
| let count = match count.as_i64() { | |
| Some(count) if count < 0 => return Ok(Value::Undefined), | |
| Some(count) => count as usize, | |
| None => { | |
| bail!(params[1].span().error( | |
| format!("`{name}` expects `count` to be an integer value.").as_str() | |
| )) | |
| } | |
| }; | |
| // BUG 2: No resource limit check - could OOM with huge count | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } |
|
|
||
| // BUG 1: unwrap() instead of proper error handling | ||
| let count = args[1].as_f64().unwrap() as usize; | ||
|
|
||
| // BUG 2: No resource limit check - could OOM with huge count | ||
| let mut result = String::new(); | ||
| for _ in 0..count { | ||
| result.push_str(&s); | ||
| } | ||
|
|
||
| // BUG 3: Returns empty string instead of Undefined when count is negative |
There was a problem hiding this comment.
The count conversion (as usize) silently truncates floats and saturates negatives to 0, which can produce surprising results (e.g., -1 becomes 0, 1.9 becomes 1). Please explicitly validate that count is an integer and non-negative, and return a consistent type/range error (or whatever the project’s standard is) when it isn’t.
| // BUG 1: unwrap() instead of proper error handling | |
| let count = args[1].as_f64().unwrap() as usize; | |
| // BUG 2: No resource limit check - could OOM with huge count | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } | |
| // BUG 3: Returns empty string instead of Undefined when count is negative | |
| let count = ensure_numeric(name, ¶ms[1], &args[1])?; | |
| let count = match count.as_i64() { | |
| Some(count) if count >= 0 => usize::try_from(count) | |
| .map_err(|_| params[1].span().error("`strings.repeat` count is out of range"))?, | |
| Some(_) => bail!(params[1] | |
| .span() | |
| .error("`strings.repeat` expects a non-negative integer count")), | |
| None => bail!(params[1] | |
| .span() | |
| .error("`strings.repeat` expects a non-negative integer count")), | |
| }; | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } |
|
|
||
| // BUG 1: unwrap() instead of proper error handling | ||
| let count = args[1].as_f64().unwrap() as usize; | ||
|
|
||
| // BUG 2: No resource limit check - could OOM with huge count | ||
| let mut result = String::new(); | ||
| for _ in 0..count { | ||
| result.push_str(&s); | ||
| } | ||
|
|
||
| // BUG 3: Returns empty string instead of Undefined when count is negative |
There was a problem hiding this comment.
The count conversion (as usize) silently truncates floats and saturates negatives to 0, which can produce surprising results (e.g., -1 becomes 0, 1.9 becomes 1). Please explicitly validate that count is an integer and non-negative, and return a consistent type/range error (or whatever the project’s standard is) when it isn’t.
| // BUG 1: unwrap() instead of proper error handling | |
| let count = args[1].as_f64().unwrap() as usize; | |
| // BUG 2: No resource limit check - could OOM with huge count | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } | |
| // BUG 3: Returns empty string instead of Undefined when count is negative | |
| let count = ensure_numeric(name, ¶ms[1], &args[1])?; | |
| let count = match count.as_i64() { | |
| Some(count) if count >= 0 => match usize::try_from(count) { | |
| Ok(count) => count, | |
| Err(_) => bail!(params[1].span().error("count is too large")), | |
| }, | |
| Some(_) => bail!(params[1].span().error("count must be non-negative")), | |
| None => bail!(params[1].span().error("count must be a non-negative integer")), | |
| }; | |
| // BUG 2: No resource limit check - could OOM with huge count | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } |
|
|
||
| // BUG 1: unwrap() instead of proper error handling | ||
| let count = args[1].as_f64().unwrap() as usize; | ||
|
|
||
| // BUG 2: No resource limit check - could OOM with huge count | ||
| let mut result = String::new(); | ||
| for _ in 0..count { | ||
| result.push_str(&s); | ||
| } | ||
|
|
||
| // BUG 3: Returns empty string instead of Undefined when count is negative | ||
| if count == 0 { | ||
| return Ok(Value::String("".into())); | ||
| } | ||
|
|
||
| Ok(Value::String(result.into())) |
There was a problem hiding this comment.
This implementation can allocate unbounded memory and run for a long time with large count, leading to potential OOM/DoS. Please add a guard that enforces a maximum output size (e.g., via a checked s.len() * count calculation and the engine’s existing resource limits) and fail gracefully when exceeded; also consider pre-allocating with with_capacity (using checked math) or using s.repeat(count) after enforcing limits.
| // BUG 1: unwrap() instead of proper error handling | |
| let count = args[1].as_f64().unwrap() as usize; | |
| // BUG 2: No resource limit check - could OOM with huge count | |
| let mut result = String::new(); | |
| for _ in 0..count { | |
| result.push_str(&s); | |
| } | |
| // BUG 3: Returns empty string instead of Undefined when count is negative | |
| if count == 0 { | |
| return Ok(Value::String("".into())); | |
| } | |
| Ok(Value::String(result.into())) | |
| let count = ensure_numeric(name, ¶ms[1], &args[1])?; | |
| let count = match count.as_i64() { | |
| Some(count) if count < 0 => return Ok(Value::Undefined), | |
| Some(count) => count as usize, | |
| None => { | |
| bail!(params[1] | |
| .span() | |
| .error(format!("`{name}` expects an integer count.").as_str())) | |
| } | |
| }; | |
| let total_len = s.len().checked_mul(count).ok_or_else(|| { | |
| params[1] | |
| .span() | |
| .error(format!("`{name}` output exceeds maximum size.").as_str()) | |
| })?; | |
| enforce_limit(name, total_len)?; | |
| if count == 0 || s.is_empty() { | |
| return Ok(Value::String("".into())); | |
| } | |
| Ok(Value::String(s.repeat(count).into())) |
Adds a new
strings.repeat(s, count)builtin that repeats a stringcounttimes.Example: