Skip to content

Recursive Variable Expansion Logic

Cauhx Milloy requested to merge CauhxMilloy/gitlab:recursiveVariableExpansionLogic into master

What does this MR do?

This Merge Request adds support for recursive environment variable expansion for environment variables.
It is specifically trying to fix gitlab-foss#43406 (moved) -- now #21218 (closed).
This is a resubmission of gitlab-foss!32181 (closed), which was closed due to the recent gitlab repo migration. As per the discussion on the previous MR, this MR does not contain "default value" support, which will be submitted in a separate MR.

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).

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.

Cases for Strange/Unsupported Behavior

Self Loop Inconsistent Depending on Order

If an indirect self-loop exists, it will be expanded differently depending on which variable is first used in the loop and if the loop had prior expansion. Consider the following test cases. Note that different expansions happen depending on order and start of loop.

          "3-loop self reference expansion different starts": {
            value: '~${variable}~${variable2}~',
            result: '~abc123xyzzyx321cba~123xyzzyx321~',
            variables: [
              { key: 'variable', value: 'abc${variable2}cba' },
              { key: 'variable2', value: '123${variable3}321' },
              { key: 'variable3', value: 'xyz${variable}zyx' }
            ]
          },
          "3-loop self reference expansion different starts 2": {
            value: '~${variable2}~${variable}~',
            result: '~123xyzabccbazyx321~abccba~',
            variables: [
              { key: 'variable', value: 'abc${variable2}cba' },
              { key: 'variable2', value: '123${variable3}321' },
              { key: 'variable3', value: 'xyz${variable}zyx' }
            ]
          },

This is not truly an issue, as it is contained within the scope of self-reference loops. Loops have no expectation of expanding properly (as that is impossible), and perhaps may lead to CI config errors (part of discussion below, !18112 (comment 228429884)). However, documenting this strange behavior is valuable.

Mecha Shiva!

When expanding variables, the string is parsed/expanded depth-first. At a given level (at a given node, if thought of as a tree), each variable is fully expanded. No post-combined-expansion of the current node occurs. This means that environment variables cannot be created dynamically and expanded. Consider the following example (named Mecha Shiva):

          "mecha shiva": {
            value: '~${MECHA}_${SHIVA}~',
            result: '~Mecha Shiva!~',
            variables: [
              { key: 'MECHA', value: '${MECHA' },
              { key: 'SHIVA', value: 'SHIVA}' },
              { key: 'MECHA_SHIVA', value: 'Mecha Shiva!' }
            ]
          },

The actual expansion, of '~${MECHA}_${SHIVA}~', results in '~${MECHA_SHIVA}~' instead of its expanded form 'Mecha Shiva!'. The current algorithm does not do a post-processing expansion of the current node, so it does not detect the newly formed environment variable MECHA_SHIVA. Note that this is true anywhere in the expansion tree, not only the root. This actually does sorta violate the "fully expand guarantee" (::ExpandVariables.expand(::ExpandVariables.expand(value, all_variables), all_variables) should always be equal to ::ExpandVariables.expand(value, all_variables)) -- discussed in !18112 (comment 227769151). Mecha Shiva functionality could be added in... I just kinda chose to leave it out, since its kinda awkward behavior. Still wanted to document its absence.

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. It is also worth noting that the expansion algorithm employs memoization; if the same variable is encountered multiple times, subsequent expansions will be O(1) via a lookup hash.

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
Edited by Elliot Rushton

Merge request reports