search

gentoo-audio / audio-overlay

Gentoo overlay for music production
https://gentoo-audio.github.io/audio-overlay
GNU General Public License v2.0
41 stars 22 forks source link

ci: don't post emerge QA messages as GitHub comments #522

Closed NexAdn closed 1 year ago

NexAdn commented 1 year ago

This requires setting a GitHub token in a public CI pipeline, allowing attackers to retrieve the token.

simonvanderveldt commented 1 year ago

@NexAdn But now there's no text/directly actionable feedback in PRs, right? Would be good to keep that.

NexAdn commented 1 year ago

@NexAdn But now there's no text/directly actionable feedback in PRs, right? Would be good to keep that.

The CI output is still available and publicly readable. Thus copying the output from the CI as a PR comment is on the one hand a bit redundant and on the other hand having a publicly readable CI output opens up a way to extract a private token from the CI pipeline by a malicious actor. Unless the GitHub/CircleCI combination allows for pipeline-specific tokens which don't cause any harm if leaked, I would strongly advise against putting a token in PR pipelines.

simonvanderveldt commented 1 year ago

@NexAdn AFAIK anyone not in the org/not logged in to CircleCI cannot access the logs. This was at least how it worked before, it might've changed in the meantime. I never understood why this was the case.

The secret was created as a secret on CircleCI, so PRs from third parties could not use it. This was resulting in failures for PRs from third parties, so not ideal either. Guess we could've skipped posting the comment in case of PRs from third parties.

I think the only way around this is with a GitHub App, where the App takes care of the (checks) integration. In case of CircleCI that would be https://circleci.com/docs/enable-checks/. This didn't exist when we started. I haven't tried this yet though, but looks like it should work :) If you don't care about it that's fine as well, we can just stick with the status feedback in the PRs that's there now.

NexAdn commented 1 year ago

@NexAdn AFAIK anyone not in the org/not logged in to CircleCI cannot access the logs. This was at least how it worked before, it might've changed in the meantime. I never understood why this was the case.

I was able to read the CI output before I had access to the GitHub org (you can check my PRs from before I became org member – I was able to explain why the pipeline failed).

The secret was created as a secret on CircleCI, so PRs from third parties could not use it. This was resulting in failures for PRs from third parties, so not ideal either. Guess we could've skipped posting the comment in case of PRs from third parties.

This is actually acceptable. Failing the pipeline just because it is unable to send the comment to the PR is not an ideal solution in any case, since it falsely marks the pipeline as failed although the actual commands succeeded.

I think the only way around this is with a GitHub App, where the App takes care of the (checks) integration. In case of CircleCI that would be https://circleci.com/docs/enable-checks/. This didn't exist when we started. I haven't tried this yet though, but looks like it should work :) If you don't care about it that's fine as well, we can just stick with the status feedback in the PRs that's there now.

Well, the pkgcheck output is color coded, so I'd ignore that colorless GitHub comment either way. Posting the QA messages might come in handy, but mostly for PRs from other users, since I always emerge the ebuilds locally before submission to check for stuff like changed SONAME versions and wrongly installed or missing files. Maybe @audiodef has an application for this?

simonvanderveldt commented 1 year ago

I was able to read the CI output before I had access to the GitHub org (you can check my PRs from before I became org member – I was able to explain why the pipeline failed).

OK, I guess they finally fixed this then at some point :)

Well, the pkgcheck output is color coded, so I'd ignore that colorless GitHub comment either way. Posting the QA messages might come in handy, but mostly for PRs from other users, since I always emerge the ebuilds locally before submission to check for stuff like changed SONAME versions and wrongly installed or missing files. Maybe @audiodef has an application for this?

Yeah, it'd definitely be useful for third party PRs, not everyone is familiar with all CI services and getting redirected to a different site with different UX isn't great. But it's not a big use-case for us, we get a few drive-by PRs each year max.

In any case, we can also just stick with the status feedback for now. I like/prefer the in PR stream of confirmation of success/failure, but if you and @audiodef don't that's fine as well. As long as the status checks pass it should be fine.