codecov bash uploader security update

[jeromedockes]

https://codecov.io/disclosure

• the github actions test workflow uses the codecov action, so that may have downloaded the altered script, but AFAIK it doesn't have access to any secrets
• azure pipelines uses the codecov package, not the bash script so it is probably not affected. in any case its only secret is the codecov token and I just changed it (note in any case we don't really consider it to be a secret; it's available to builds of forks)

still if someone else eg @NicolasGensollen could have a look at this that would be great!

[NicolasGensollen]

Thanks for opening this @jeromedockes ! 👍
I'll have a look.

[NicolasGensollen]

It looks like the codecov action will soon compute the checksum of the script and make sure it is not corrupted, see codecov/codecov-action#281

[NicolasGensollen]

@jeromedockes after looking into this, I think the only potential issue could come from the codecov action which indeed uses the uploader script. AFAICT, the workflow in which this action is used doesn't have access to any secret.
There are ongoing discussions (see jupyterhub/team-compass#398 for example) to find out whether actions from one workflow could potentially affect or get information from other workflows. I cannot find a precise answer for this question at the moment.
Note that we have one workflow (without codecov action in it) using a secret:

nilearn/.github/workflows/main.yml

Line 13 in 82f4075

repo-token: ${{ secrets.GITHUB_TOKEN }}

[jeromedockes]

@jeromedockes after looking into this, I think the only potential issue could come from the codecov action which indeed uses the uploader script. AFAICT, the workflow in which this action is used doesn't have access to any secret. There are ongoing discussions (see jupyterhub/team-compass#398 for example) to find out whether actions from one workflow could potentially affect or get information from other workflows. I cannot find a precise answer for this question at the moment. Note that we have one workflow (without codecov action in it) using a secret: https://github.com/nilearn/nilearn/blob/82f4075d8a8ea9aec25e66bd87ebb79a6be6d32f/.github/workflows/main.yml#L13

thanks for looking into it! it is used to upload the doc? generating a new token is easier than figuring out if we need to, so I suggest we change it

[NicolasGensollen]

@jeromedockes I'm wondering whether we shouldn't just remove the codecov action from the workflow, and use a script instead (similar to what is done for the azure pipeline)?
In any case, we should indeed change the token as a precaution.

[jeromedockes]

@jeromedockes I'm wondering whether we shouldn't just remove the codecov action from the workflow, and use a script instead > (similar to what is done for the azure pipeline)?

in case the same thing happens again with the github action in the future? I don't have a strong opinion, let's do what you think is best! using the codecov package instead of the bash uploader shouldn't be hard to do if you think it is more suited

In any case, we should indeed change the token as a precaution.

actually I think I read your previous comment a bit too fast this morning; IIUC the token in question is the GITHUB_TOKEN, which expires at the end of the workflow, is that correct?

[NicolasGensollen]

It seems like codecov merged their checksum verification PR a couple days ago: codecov/codecov-action#282 and bumped to v1.4.0, so I suppose we can keep the codecov action in the workflow.

Moreover, we are not using any secret in the "compromised" workflow. The github token is used in another workflow, and therefore shouldn't be accessible (see discussion linked in my previous comment).

actually I think I read your previous comment a bit too fast this morning; IIUC the token in question is the GITHUB_TOKEN, which expires at the end of the workflow, is that correct?

Absolutely, you are right. I had forgotten about that when I replied. See github doc page here.

Based on these points, I think we can close this without any action from our part. WDYT?

[jeromedockes]

Based on these points, I think we can close this without any action from our part. WDYT?

Yes I agree, thanks for making sure!