Add CWD to Git's safe.directory with cml ci

[0x2b3bfa0]

Fixes #970 in a rather awkward way. See this upstream pull request for the rationale for not fixing this on actions/checkout by writing the global configuration:

• Enforce safe directory actions/checkout#762 (comment)

⚠️ This pull request modifies the Git global configuration. 🙊

[dacbd]

I feel this is a pretty safe fix considering how the tools are used. (talking about the linked rationale for the actions/checkout fix) cml and more so cml ci are run in ci/cd envs or its own created instance (cml runner) where writing an option to the global config should not be an issue.

[0x2b3bfa0]

Agreed. The only other alternative I can think of would be creating an ephemeral home directory with a copy of the global Git configuration, like in actions/checkout#762. 🙃

[0x2b3bfa0]

Not worth delaying the fix seeking perfection, but it's far from ideal. Mainly because the CML constructor already does many things it shouldn't.

[DavidGOrtega]

Mainly because the CML constructor already does many things it shouldn't.

Should we enumerate them and fix them?

[dacbd]

I haven't tried testing this within the GitHub Actions context, but is

error: Command failed: git config --get remote.origin.url {"output":[null,{"data":[],"type":"Buffer"},{"data":[],"type":"Buffer"}],"pid":16,"signal":null,"status":1,"stderr":{"data":[],"type":"Buffer"},"stdout":{"data":[],"type":"Buffer"}}

the expected output from a local test, see:

#!/bin/bash
git clone git@github.com:iterative/cml.git
pushd cml
git checkout git-safe-directory
npm install
docker build -t cml-local:latest .
popd
docker run --rm -it -v $PWD/cml:/home/runner cml-local:latest node ./bin/cml.js ci
rm -rf ./cml

[DavidGOrtega]

Yes, we can do a factory to solve that error or less formal enforce to call an init method, I never liked the last idea

[0x2b3bfa0]

Or, alternatively, treat repo as an asynchronous method (what it should be) instead of a class property. 🤔

[shcheklein]

thanks @0x2b3bfa0 for addressing this! Looks go to me it that works as a temp fix :) Let's merge (comments are not critical, esp if it's a short term fix).

Mid term:

• do we know what are the security implications of this? What was the angle of a possible attack in the first place, can it be applied in this case with some carefully crafted PR or something?
• do we need tests for this? or do we wait for checkout action to fix it on their end? Put a comment or create a ticket for this?

[dacbd]

I agree with the getOrSet comment, but my test script ran as I would expect.

[0x2b3bfa0]

do we need tests for this? or do we wait for checkout action to fix it on their end? Put a comment or create a ticket for this?

I would wait for GitHub Actions to fix it. Let's track any further feedback on #984.