3 ms·

I am sympathetic to their need for manual intervention, but maintainers should be the last line of defense, not the first and only line they seem to have here.

by sramam 5y ago

I am sympathetic to their need for manual intervention, but maintainers should be the last line of defense, not the first and only line they seem to have here. Specifically: > If the pull request is updated to a new commit, a new approval will be required. What?! It'd be much more palatable if the manual approvals were more selective. For example: - only invoked if a job takes more than 1.5x or 2x the max recorded run-time for the job. - some notion of PR-author's karma being used to throttle manual approvals from maintainers. It also would be fair to allow maintainers that do not want this load to mark their repos as open-source but closed to contributions. [edit] formatting

hashhar 5y ago
There are valid reasons for requiring re-approval. A very good example is that a normal looking first commit gets approved. In the next commit the PR author exfiltrates GitHub secrets by base64ing them and logging in the workflow run (or any other way). Boom - now your CI infra AWS S3 testing bucket, Google BigQuery keys etc. are leaked. Runtime is also easy to work-around - repeatedly push commits with a execution time limit on the CI workflow to avoid triggering outlier detection. A trusted PR author today doesn't guarantee trust tomorrow. Also your trust on some other repo doesn't (and shouldn't) transfer across repos. > It also would be fair to allow maintainers that do not > want this load to mark their repos as open-source but > closed to contributions. This is already possible.
- mhils 5y ago
  > In the next commit the PR author exfiltrates GitHub secrets by base64ing them and logging in the workflow run (or any other way). Environment secrets are not exposed to PRs, so this does not work. This really only concerns DoS.
  - hashhar 5y ago
    IIRC I can look at the secret names in the workflow definition then pipe them through `base64 | zip` and have fun. I did this quite some time ago but they may have added better limits in place.
- gray_-_wolf 5y ago
  > This is already possible. How? I did not find it possible to turn off pull requests when I last looked. Is that something they've added recently?
  - __s 5y ago
    You're right, likely the OP thought PRs were included in settings where you can disable issues or wikis Discussion: https://github.com/dear-github/dear-github/issues/84 https://github.com/dear-github/dear-github/issues/84
  - hashhar 5y ago
    Thanks for correcting me. Looks like I got confused. Though ironically people have used GitHub Actions to auto-close all incoming pull-requests. XD
- sramam 5y ago
  Any review should be part of the code review process - not the CI build process. If the build is broken, a review is likely to be less reliable.
codeflo 5y ago
It's essentially an admission that the "mitigations" they claim to have implemented (second paragraph) aren't that effective, and they want to outsource the review effort to the community. Which is fine in my opinion, but I agree that the current tools might not be fine granular enough. There should also be clearer messaging about what happens when there's an honest review mistake and a crypto miner slips through.

by sramam 5y ago

hashhar 5y ago

There are valid reasons for requiring re-approval. A very good example is that a normal looking first commit gets approved. In the next commit the PR author exfiltrates GitHub secrets by base64ing them and logging in the workflow run (or any other way). Boom - now your CI infra AWS S3 testing bucket, Google BigQuery keys etc. are leaked. Runtime is also easy to work-around - repeatedly push commits with a execution time limit on the CI workflow to avoid triggering outlier detection. A trusted PR author today doesn't guarantee trust tomorrow. Also your trust on some other repo doesn't (and shouldn't) transfer across repos. > It also would be fair to allow maintainers that do not > want this load to mark their repos as open-source but > closed to contributions. This is already possible.

mhils 5y ago
> In the next commit the PR author exfiltrates GitHub secrets by base64ing them and logging in the workflow run (or any other way). Environment secrets are not exposed to PRs, so this does not work. This really only concerns DoS.
- hashhar 5y ago
  IIRC I can look at the secret names in the workflow definition then pipe them through `base64 | zip` and have fun. I did this quite some time ago but they may have added better limits in place.
gray_-_wolf 5y ago
> This is already possible. How? I did not find it possible to turn off pull requests when I last looked. Is that something they've added recently?
- __s 5y ago
  You're right, likely the OP thought PRs were included in settings where you can disable issues or wikis Discussion: https://github.com/dear-github/dear-github/issues/84 https://github.com/dear-github/dear-github/issues/84
- hashhar 5y ago
  Thanks for correcting me. Looks like I got confused. Though ironically people have used GitHub Actions to auto-close all incoming pull-requests. XD
sramam 5y ago
Any review should be part of the code review process - not the CI build process. If the build is broken, a review is likely to be less reliable.

codeflo 5y ago

It's essentially an admission that the "mitigations" they claim to have implemented (second paragraph) aren't that effective, and they want to outsource the review effort to the community. Which is fine in my opinion, but I agree that the current tools might not be fine granular enough. There should also be clearer messaging about what happens when there's an honest review mistake and a crypto miner slips through.