Skip to content

Improved Environment Variable Expansion Logic

Cauhx Milloy requested to merge CauhxMilloy/gitlab-ce:environmentVariableExpansionLogic into master

What does this MR do?

This Merge Request adds support for recursive environment variable expansion as well as default values for environment variables.
It is specifically trying to fix https://gitlab.com/gitlab-org/gitlab-ce/issues/43406 (and then also default value support).

Recursive variables Expansion Support

If $SOME_VAR is set to abc${SOME_OTHER_VAR}def (both within the variables sections of CI), all variables within variables will be expanded (no depth limit).

Default Value variables Expansion Support

If $SOME_VAR is set to ${SOME_OTHER_VAR:-default_value}, it will expand to the value of $SOME_OTHER_VAR if it is set, otherwise it will expand to default_value (in this example).

Infinite Recursion Protection

There is also infinite recursion protection. If an environment variable contains itself at some depth (e.g.: $SOME_VAR is set to ~${SOME_VAR}~, it will expand to '' upon subsequent expansion attempts (the given example will expand to ~~). Test cases were added checking for recursion, default values, and infinite recursion protection.

Unsupported Edge Case

One unsupported test case (which was not added) would be:

"dual removed recursive expansion": {
  value: '~${variable}~${variable3}~',
  result: '~abc123xyz890098zyx321cba~xyz890098zyx~',
  variables: [
    { key: 'variable', value: 'abc${variable2}cba' },
    { key: 'variable2', value: '123${variable3}321' },
    { key: 'variable3', value: 'xyz${variable4}zyx' },
    { key: 'variable4', value: '890${variable5}098' }
  ]
},

This is an unsupported edge case. With this Merge Request, the above example will expand to '~abc123321cba~xyz890098zyx~'. This is due to the fact that the algorithm used sees expansion of ${variable3} as already done, and refuses to expand further. It cannot differentiate between "dual removed" recursive expansion and infinite recursion. This is caused by the algorithm's the usage of value.gsub(variable_pattern) do ... end, which is breadth first traversal. A depth first traversal would be necessary to cover this use case.
That all said, this is an edge case. Further, it is also not supported without this Merge Request. Coverage of such expansion could be fixed in a follow up MR.

Screenshots

N/A

Does this MR meet the acceptance criteria?

Conformity

Performance and Testing

Potential Performance Impact

Now that environment variable expansion is done in a loop, it is possible for ExpandVariables.expand() to take longer than before (when only expanding a depth of 1 or 2). However, extending this loop requires manually adding unique recursive environment variables -- which is unlikely. The infinite recursion protection should prevent any non-linear recursion.

Additional Testing

As mentioned above, test cases we added in order to check for different forms of looping environment variable expansion. Since this is a back-end change, no extra cross-browser testing should be necessary.

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Merge request reports