fix: interpolate_only should substitute placeholders in a single pass#6320
fix: interpolate_only should substitute placeholders in a single pass#6320Osamaali313 wants to merge 1 commit into
Conversation
interpolate_only substituted each placeholder with result.replace() against
the running result, so a value substituted for an earlier {var} that itself
contained text like {another_var} was re-interpolated by a later iteration.
This produced incorrect output and let one input inject another input's value
(e.g. a secret). The docstring and existing tests describe single-pass
substitution.
Use re.sub with a replacer over the original string so each placeholder is
replaced exactly once and substituted text is never re-scanned. Existing
behavior (multiple occurrences, untouched JSON braces, preserved {123}/{!var},
missing-variable KeyError) is unchanged. Adds a regression test.
There was a problem hiding this comment.
Summary: This PR changes interpolate_only to perform single-pass placeholder substitution and adds a regression test, reducing the risk of unintended cross-input interpolation.
Risk: Low risk. The changes do not introduce new authentication, authorization, file, network, or data-access surfaces, and no exploitable security vulnerabilities were identified.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesSingle-pass interpolation behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
interpolate_only(crewai/utilities/string_utils.py) substitutes template placeholders by iterating over the discovered variables and callingresult.replace("{var}", value)against the runningresult:Because each
.replacere-scans the whole accumulated string, a value substituted for an earlier placeholder that itself contains text like{another_var}gets re-interpolated by a later iteration. The docstring and existing tests describe single-pass substitution (each{var}→ its value); nothing supports recursive expansion.Beyond producing incorrect output, this is an injection vector when inputs are user/LLM-provided — one input can pull in the value of another:
Fix
Substitute in a single pass over the original string with
re.sub(the missing-variableKeyErrorcheck still runs first):re.subreplaces each matched placeholder in the original template exactly once and never re-scans substituted text.Testing
Added
test_value_containing_placeholder_is_not_reinterpolatedtoTestInterpolateOnly. It fails onmain(the inner{secret}is expanded) and passes with the fix.All previously-covered behavior is preserved (verified against the existing
test_string_utils.pycases): multiple occurrences of the same variable, untouched JSON braces, preserved{123}/{!var}, underscore-prefixed names, empty/None input, and the missing-variableKeyError.Summary by CodeRabbit
Bug Fixes
{...}are not re-processed during interpolation.Tests