Closed njsmith closed 4 years ago
webknjaz
commented
5 years ago I'd not rely on [bot]. The webhook payload has user or sender['type'] == 'Bot': https://github.com/sanitizers/chronographer-github-app/blob/7db46ab/chronographer/event_handlers.py#L314
njsmith
commented
5 years ago Oh, good point. I tracked down one of the webhook payloads in the github ui and it looks like:
And if you squint, buried in there it looks like there's a pull_request.user.type field that's set to "Bot".
So I guess the check in snekomatic.app.pull_request_merged would be something like:
if glom(event.payload, "pull_request.user.type").lower() == "bot":
print("This user is a bot -> not inviting")
returnNote: for regular users, the pull_request.user.type is "User"
webknjaz
commented
5 years ago Yep, and there's bots which use regular user accounts. That's why my bot has a setting for blacklisting human accounts as well: https://github.com/sanitizers/chronographer-github-app/blob/7db46ab032476670dfef7a09e756c06cbf60751d/chronographer/event_handlers.py#L321
njsmith
commented
5 years ago Eh, I don't think we need to care about that too much though. We don't generally get PRs from bots-masquerading-as-users (at least, as far as I know!), and even if we did invite one to join the org once it would be fine. With dependabot though it keeps crashing when trying to send the invitation, so we don't mark it down as previously-invited, so we try again the next time, and the text...
webknjaz
commented
5 years ago even if we did invite one to join the org once it would be fine
I'd still consider this a security breach I guess...
njsmith
commented
5 years ago Oh yeah, for most projects it would be. But we have a pretty open membership policy (as you've seen). Our basic security approach is to assume that almost everyone is trustworthy, and then make sure that if we do end up with a malicious member, they can't do too much harm before being caught. (So e.g. we have branch protection so changes have to go through public review, and we don't put important credentials in our repos, even encrypted.)
This is sort of funny: every time one of dependabot's PRs are merged, trio-bot dutifully notices that the user
dependabot-preview[bot]is not a member of the org and has never been invited, so it tries to invite them to join.Fortunately (?) this fails; GitHub gives a 500 error if you try to invite a bot. But we should probably stop doing this anyway.
I think the solution is just to check if the user's name ends in
[bot], and if so then we skip inviting them. We'll also want a test – I guess by tweaking theScenariointest_app.pyto include the user name as a field, instead of using a hard-coded constant.Marking this a good first issue for anyone who's interested in getting involved here.