Skip to content

Correctly handle expansion of job file variables, and variables that reference file variables

Axel von Bertoldi requested to merge avonbertoldi/29128/refactor-tmp-file-path-creation-bash into main

Status

This MR closes both gitlab#29407 (closed) and #29128 (closed)

What does this MR do?

This MR changes job variable expansion so that variables that reference/alias file variables are expanded to the file variable's file name/path instead of its value (i.e. the content of the file).

Note that while #29128 (closed) only calls for refactoring of the creation of the tmp file path, doing so in isolation in a meaningful way turned out to be difficult, and to meaningfully test the changes I ended up spilling over into actually fixing the original/root issue gitlab#29407 (comment 935276800).

So, this MR actually addresses gitlab#29407 (comment 935276800) more than it does #29128 (closed), but makes #29128 (closed) redundant. Note also that because tmp file path creation is inextricably tied the the job's shell/OS, it's not possible to simply move the tmp file creation logic from all shells into the Build object, nor is it sufficient to have a simple tmp file creation method in Build that will work with all shells. As a result, in discussion with @ajwalker, we arrived at a solution where a canonical or approximate tmp file path will be created at variable expansion time, and then be corrected by each shell writer at script generation time. This MR implements the latter part only for the Bash shell writer, to vet the approach before investing more effort for the other two shells. If the approach is accepted, I will implement the changes for the other two shells. Implementations for the cmd and poweshell writers has been added to this MR.

Note: best reviewed commit-at-a-time

Why was this MR needed?

Previously, variables that reference/alias file variables were expanded to the value of the file variable (i.e. its content). Not only is this unexpected in that it does not respect the typical shell variable expansion rules, but if such a variable was printed (e.g. echoed) as part of the jobs script, the contents of the associated file variable would be printed, potentially leaking secrets or sensitive information. gitlab#29407 (closed) Tracks the fulls story, which also required changes on the rails side (gitlab#365859 (closed)).

What's the best way to test this MR?

First create a file variable via the GitLab UI, For example: A_FILE_VAR with content this is some super secret content. Then create a job like this:

work:
   stage: test
   variables:
     REF_FILE_VAR: $A_FILE_VAR
   script:
     - echo $A_FILE_VAR
     - cat $A_FILE_VAR
     - echo $REF_FILE_VAR
     - cat $REF_FILE_VAR

Results

Before

$ echo $A_FILE_VAR
/builds/avonbertoldi/test-project.tmp/A_FILE_VAR
$ cat $A_FILE_VAR
this is some super secret content
$ echo $REF_FILE_VAR
this is some super secret content
$ cat $REF_FILE_VAR
cat: can't open 'this': No such file or directory
cat: can't open 'is': No such file or directory
cat: can't open 'some': No such file or directory
cat: can't open 'super': No such file or directory
cat: can't open 'secret': No such file or directory
cat: can't open 'content': No such file or directory

After

$ echo $A_FILE_VAR
/builds/avonbertoldi/test-project.tmp/A_FILE_VAR
$ cat $A_FILE_VAR
this is some super secret content
$ echo $REF_FILE_VAR
/builds/avonbertoldi/test-project.tmp/A_FILE_VAR
$ cat $REF_FILE_VAR
this is some super secret content

What are the relevant issue numbers?

NOTE: Best reviewed commit-at-a-time

Edited by Axel von Bertoldi

Merge request reports