Skip to content

[steps] Fix replaceAll() hanging and replacement patterns being expanded in interpolated values - #4438

Open
tahakocal wants to merge 2 commits into
expo:mainfrom
tahakocal:fix/steps-replace-all-hang
Open

tahakocal wants to merge 2 commits into
expo:mainfrom
tahakocal:fix/steps-replace-all-hang

Conversation

@tahakocal

Copy link
Copy Markdown

Why

Two bugs in how @expo/steps uses String.prototype.replace.

1. replaceAll() never returns when the replacement contains the string it replaces. BuildStepContext.getInterpolationContext() implements it as while (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, '/', '//') }}
  • POSIX shell quoting: ${{ replaceAll(env.MSG, "'", "'\\''") }}
  • an empty string to replace

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 --local for every ${{ }} expression and if: condition of custom build configs and workflows.

2. $ patterns in interpolated values are expanded as replacement patterns. interpolate() in utils/template.ts passes the value straight to String.prototype.replace, which treats $&, $$, $` and $' specially. Values are step inputs, step outputs and the eas.* context, and the result is used for step inputs and the run: script body, so a secret is silently corrupted:

value before after
pa$$word pa$word pa$$word
a$&b a${ eas.job.secrets.token }b a$&b
$' (empty) $'
$` (the text before the placeholder, repeated) $`

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

  • replaceAll now calls String.prototype.replaceAll with a function replacement, so it terminates and takes the replacement literally.
  • interpolate passes the value as a function replacement for the same reason.
  • The same one-line .replace(regex, value) issue in integrations:convex:connect, which writes CONVEX_DEPLOY_KEY to .env.local, matching its PostHog counterpart which already uses a function.
  • A copy of the old loop lived in a build-tools test mock; it now matches the implementation.

Two intentional, user-visible behaviour changes beyond the bugs:

  • $ in a replaceAll replacement is now literal. Before, replaceAll(x, '-', '$$') produced $.
  • Replacement is now a single left-to-right pass, like 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 now aab instead of aa.

I left build-tools' download.ts and ios/pod.ts alone; they have the same pattern but their replacement comes from EAS_BUILD_COCOAPODS_CACHE_URL.

Test Plan

  • Added tests for replaceAll (replacement containing the replaced string, empty string to replace, $ patterns) and for interpolateWithInputs, interpolateWithOutputs and interpolateWithGlobalContext with values containing $&, $$ and $'.
  • packages/steps: 575 tests pass. build-tools unit tests and eas-cli src/commands/integrations tests pass. oxlint, oxfmt --check and tsc are clean.
  • The interpolation tests fail without the fix. The replaceAll ones 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:
expression before after
${{ replaceAll(github.ref, '/', '//') }} killed after 8s, still looping refs//heads//feat//my-branch
${{ replaceAll(env.MSG, "'", "'\\''") }} killed after 8s it'\''s fine
${{ replaceAll(github.ref_name, '', '-') }} out of memory after 2.8s -f-e-a-t-/-m-y---b-r-a-n-c-h-
${{ replaceAll(github.ref_name, '/', '-') }} feat-my-branch feat-my-branch
${{ replaceAll(github.ref, 'refs/heads/', '') }} feat/my-branch feat/my-branch
  • I also ran both interpolation paths end-to-end through BuildStepGlobalContext with a secret containing $&, and compared every case against the current implementation to confirm nothing else changes.

@github-actions

Copy link
Copy Markdown

Subscribed to pull request

File Patterns Mentions
packages/eas-cli/** @douglowder

Generated by CodeMention

Warning: The preamble and epilogue options in commentConfiguration are deprecated. Use template instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant