Explicitly run coveralls step

[damccorm]

This will allow the token to be passed on PRs in without concern of it being stolen by a changed script. Without this, an attacker could change the npm run coveralls command in package.json in their PR so that it does something bad (e.g. sends them the token).

See #2299 (comment)

[phated]

@damccorm With this change, couldn't they change the npm run cover command and have access?

[damccorm]

Yes 🤦‍♂️ Updated appropriately, sorry about that

[phated]

I know I'm being a little paranoid here but what would happen if someone changed the package.json to point istanbul or istanbul-coveralls at some git repository that replaced the commands with something that dumped the env?

[damccorm]

Can someone do that? My understanding of how npm works is that it makes that sort of behavior impossible. They'd have to change the dependency name in package.json at which point it would no longer be added to the path as Istanbul or Istanbul-coveralls, it would get added as something else.

[phated]

I'm allowed to do something like:

{
    "devDependencies": {
        "istanbul": "github:phated/malicious-package"
    }
}

And as long as "malicious-package" contained an istanbul bin, it'd run.

[phated]

Maybe this step needs to forcefully install the packages from npm. We can probably achieve that by using npx --ignore-existing istanbul && npx --ignore-existing istanbul-coveralls - Thoughts?

[damccorm]

Ah, I see what you mean now - I guess that could be an issue. I think that solution should work in theory, I'm having a bit of trouble getting it to work though because it looks like npx doesn't work correctly on lower versions of npm or Node - see build here.

We can keep looking for a solution here, to be honest I'm not sure how necessary this is though. I don't think there's anything a malicious actor could do with this token outside of messing with your code coverage results - the work to do that seems to far outweighs the benefits, especially since you can just regen the token and delete the bad builds. In the codecov docs they don't even recommend keeping it as a secret variable even though this would affect all CI's outside of Travis and I think most repos that use this without Travis just keep it as plaintext.

If you want I can keep trying to tweak npx though, I'm happy to continue to work on this if we think its worthwhile. Just don't want to overcomplicate things if we don't need to.

[lirantal]

@damccorm this issue is referring only to azure pipelines integration, right? because Travis CI doesn't provide token access to forks.

[damccorm]

I'm not sure. I think they might actually just automatically inject the COVERALLS_REPO_TOKEN into their builds in which case it would be an issue, but they might have a more tightly integrated approach. I don't know exactly how this works on their end.

[phated]

@damccorm I'm having a really hard time getting coveralls to work correctly. They need a unique ID that represents the entire job and then individual job IDs to group under the top-level job. I don't see anything like that in https://docs.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml

[damccorm]

I think we can definitely use Build.BuildNumber for the unique Id across the build. For the individual job IDs, could we use the version of node as the id? That should be unique for each coveralls run per build.

[phated]

@damccorm I was able to get this to work with these environment variables:

gulp/.ci/.azure-pipelines-steps.yml

Lines 19 to 30 in b2c6c7e

	env:
	# Pretend to be AppVeyor for now
	APPVEYOR: true
	APPVEYOR_BUILD_NUMBER: $(Build.BuildNumber)
	APPVEYOR_BUILD_ID: $(Agent.OS)_$(node_version)
	APPVEYOR_REPO_COMMIT: $(Build.SourceVersion)
	APPVEYOR_REPO_BRANCH: $(Build.SourceBranchName)
	# Overwrite the GitLab Service Name
	COVERALLS_SERVICE_NAME: Azure Pipelines
	COVERALLS_REPO_TOKEN: $(COVERALLS_REPO_TOKEN_SECRET)
	COVERALLS_PARALLEL: true
	CI_PULL_REQUEST: $(System.PullRequest.PullRequestNumber)

and this extra step

gulp/.ci/.azure-pipelines.yml

Lines 72 to 84 in b2c6c7e

	- job: Notify_Coveralls
	displayName: Notify Coveralls that the parallel report is done
	pool:
	vmImage: "Ubuntu 16.04"
	dependsOn:
	- Test_Linux
	- Test_Windows
	- Test_MacOS
	steps:
	- script: curl -k https://coveralls.io/webhook?repo_token=$COVERALLS_REPO_TOKEN -d "payload[build_num]=$BUILD_NAME&payload[status]=done"
	env:
	COVERALLS_REPO_TOKEN: $(COVERALLS_REPO_TOKEN_SECRET)
	BUILD_NAME: $(Build.BuildNumber)

- which produces: https://coveralls.io/builds/22365839

Any ways to clean that up would be greatly appreciated!

[phated]

@damccorm it seems like we can't actually enable Azure Pipelines to run on Forks because of this little snippet I found in the docs:

By default with GitHub pipelines, secrets associated with your build pipeline are not made available to pull request builds of forks. These secrets are enabled by default with GitHub Enterprise Server pipelines. Secrets include:

• A security token with access to your GitHub repository.
• These items, if your build uses them:
  • Service connection credentials
  • Files from the secure files library
  • Build variables marked secret

That seems to indicate that "Make secrets available to builds of forks" is an all-or-nothing option. I'd either have to make the Coveralls token non-secret or expose our repo to attacks using it's security token. I really wish I could decide which secrets to allow to exposed forks. It'd also be really helpful if I could reject Azure Pipelines running if certain files were changed (like my package.json, etc) - it doesn't solve everything but it helps a lot.

I'm going to close this for now and just concede that we need to run Travis, AppVeyor & Azure Pipelines side-by-side until Azure fits all our use cases better.

[damccorm]

@phated where are you pulling that from? It seems pretty misleading to me and I'd love to clean the doc up.

To be clear, secrets are not scoped into tasks by default, regardless of whether secrets are enabled on forks. So unless you explicitly scope the security token into a task (which I definitely wouldn't recommend), forks will have no way of accessing that token.

So if you have the following YAML definition:

- task: task1
  env:
    MY_SECRET_1: $(MY_SECRET_1)

- task: task2
  env:
    MY_SECRET_2: $(MY_SECRET_2)

task1 has access to MY_SECRET_1 but not MY_SECRET_2 and vice versa. So unless you explicitly scope the security token with access to your repo in, no task will be able to use it.

Not sure if that changes how you view this or not, but just wanted to be clear on that. Several Microsoft repos enable secrets on forks, and that would be a big-time safety issue if people could attack the repos through that.

[phated]

@damccorm oh, that's good to know. I found those docs at https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/github?view=azure-devops#validate-contributions-from-forks

For the time being, I just enabled the /azp command thing that I can trigger after reviewing code.