[steps] Fix replaceAll() hanging and replacement patterns being expanded in interpolated values - #4438
Open
tahakocal wants to merge 2 commits into
Open
[steps] Fix replaceAll() hanging and replacement patterns being expanded in interpolated values#4438tahakocal wants to merge 2 commits into
tahakocal wants to merge 2 commits into
Conversation
…ded in interpolated values
|
Subscribed to pull request
Generated by CodeMention Warning: The preamble and epilogue options in commentConfiguration are deprecated. Use template instead. |
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.
Why
Two bugs in how
@expo/stepsusesString.prototype.replace.1.
replaceAll()never returns when the replacement contains the string it replaces.BuildStepContext.getInterpolationContext()implements it aswhile (input.includes(s)) input = input.replace(s, r), so the condition stays true forever. The natural uses of the helper hit this:${{ replaceAll(github.ref, '/', '//') }}${{ replaceAll(env.MSG, "'", "'\\''") }}The loop is synchronous, so the step burns a CPU until the job hits its timeout (or the builder runs out of memory, since the string grows by one character per iteration), with no error message. This runs on the build worker and in
eas build --localfor every${{ }}expression andif:condition of custom build configs and workflows.2.
$patterns in interpolated values are expanded as replacement patterns.interpolate()inutils/template.tspasses the value straight toString.prototype.replace, which treats$&,$$,$`and$'specially. Values are step inputs, step outputs and theeas.*context, and the result is used for step inputs and therun:script body, so a secret is silently corrupted:pa$$wordpa$wordpa$$worda$&ba${ eas.job.secrets.token }ba$&b$'$'$`$`The
$&case is the worst: the expression's own text is spliced into the middle of the value, so the credential is both wrong and partly replaced by the context path it came from.How
replaceAllnow callsString.prototype.replaceAllwith a function replacement, so it terminates and takes the replacement literally.interpolatepasses the value as a function replacement for the same reason..replace(regex, value)issue inintegrations:convex:connect, which writesCONVEX_DEPLOY_KEYto.env.local, matching its PostHog counterpart which already uses a function.build-toolstest mock; it now matches the implementation.Two intentional, user-visible behaviour changes beyond the bugs:
$in areplaceAllreplacement is now literal. Before,replaceAll(x, '-', '$$')produced$.String.prototype.replaceAll, instead of rescanning from the start after each replacement. That only differs when a replacement creates a new match, e.g.replaceAll('aabb', 'ab', 'a')is nowaabinstead ofaa.I left
build-tools'download.tsandios/pod.tsalone; they have the same pattern but their replacement comes fromEAS_BUILD_COCOAPODS_CACHE_URL.Test Plan
replaceAll(replacement containing the replaced string, empty string to replace,$patterns) and forinterpolateWithInputs,interpolateWithOutputsandinterpolateWithGlobalContextwith values containing$&,$$and$'.packages/steps: 575 tests pass.build-toolsunit tests andeas-clisrc/commands/integrationstests pass.oxlint,oxfmt --checkandtscare clean.replaceAllones cannot fail there: the loop is synchronous, so Jest's test timeout cannot interrupt it and the run hangs instead. I verified those cases by evaluating real expressions with the old implementation in a separate process with a wall-clock kill:${{ replaceAll(github.ref, '/', '//') }}refs//heads//feat//my-branch${{ replaceAll(env.MSG, "'", "'\\''") }}it'\''s fine${{ replaceAll(github.ref_name, '', '-') }}-f-e-a-t-/-m-y---b-r-a-n-c-h-${{ replaceAll(github.ref_name, '/', '-') }}feat-my-branchfeat-my-branch${{ replaceAll(github.ref, 'refs/heads/', '') }}feat/my-branchfeat/my-branchBuildStepGlobalContextwith a secret containing$&, and compared every case against the current implementation to confirm nothing else changes.