palantir / policy-bot

A GitHub App that enforces approval policies on pull requests
Apache License 2.0
780 stars 108 forks source link

Status check clarification #753

Closed bholland-bh closed 7 months ago

bholland-bh commented 7 months ago

We're using policy bot in our org, and when I'm adding policy bot as a required status check, I'm given two options policy bot and policy bot: main

My understanding is that policy bot evaluates the current branch policy, whereas policy bot: main evaluates the default branch policy. That way people can't modify the policy on a PR and merge code in.

However, when I click on the details for each on a PR, it takes me to the same policy evaluation, even when the policies are different.

Is my understanding of the two status checks correct?

bluekeyes commented 7 months ago

I recommend requiring the policy-bot: main status. By default, this is the only status Policy Bot will post. Since you're also seeing the plain policy-bot status, it sounds like your organization enabled the post_insecure_status_checks option in the server configuration.

Policy Bot always reads the policy file from the target branch of the pull request, so the two statuses will always have the same value. Unfortunately, reading from the target branch causes a problem when using the status check without the branch name: a user can generate a passing policy-bot status by manipulating the target branch of their commit. Suppose you have a branch called bypass-policy-bot that you want to get a passing check on:

  1. Create a branch called no-approval with a modified policy file that requires no approval
  2. Make a pull request from bypass-policy-bot to no-approval
  3. Policy Bot posts a passing policy-bot status to the head commit of bypass-policy because the policy of the target branch is satisfied
  4. Close that PR and create a new PR from bypass-policy-bot to main
  5. Because the head commit has a passing policy-bot check, you can potentially merge the PR before Policy Bot can respond to webhooks and update the status using the real policy from the main branch

Including the target branch name in the status check context was our way to fix this issue, as a passing policy-bot: no-approval status does not satisfy a requirement for the policy-bot: main status.

That said, some users do not care about this flaw and prefer the simplicity of a single check that is the same on all branches. In that case, the post_insecure_status_checks option restores the original behavior but does not disable what we consider the correct behavior (having the branch name in the check context), leading to the two checks you observed.

bholland-bh commented 7 months ago

I recommend requiring the policy-bot: main status. By default, this is the only status Policy Bot will post. Since you're also seeing the plain policy-bot status, it sounds like your organization enabled the post_insecure_status_checks option in the server configuration.

Policy Bot always reads the policy file from the target branch of the pull request, so the two statuses will always have the same value. Unfortunately, reading from the target branch causes a problem when using the status check without the branch name: a user can generate a passing policy-bot status by manipulating the target branch of their commit. Suppose you have a branch called bypass-policy-bot that you want to get a passing check on:

  1. Create a branch called no-approval with a modified policy file that requires no approval

  2. Make a pull request from bypass-policy-bot to no-approval

  3. Policy Bot posts a passing policy-bot status to the head commit of bypass-policy because the policy of the target branch is satisfied

  4. Close that PR and create a new PR from bypass-policy-bot to main

  5. Because the head commit has a passing policy-bot check, you can potentially merge the PR before Policy Bot can respond to webhooks and update the status using the real policy from the main branch

Including the target branch name in the status check context was our way to fix this issue, as a passing policy-bot: no-approval status does not satisfy a requirement for the policy-bot: main status.

That said, some users do not care about this flaw and prefer the simplicity of a single check that is the same on all branches. In that case, the post_insecure_status_checks option restores the original behavior but does not disable what we consider the correct behavior (having the branch name in the check context), leading to the two checks you observed.

Thank you so much for the detailed explanation.

This is very helpful.