GitHub native stacked PRs don't work with policy-bot's status name

[juzsal]

Hi folks,

GitHub's new native stacked PRs (the built-in feature currently in preview, not Graphite or similar) don't play nicely with policy-bot currently, so I wanted to check whether a fix would be welcome before I put more time into it.

As far as I understand, policy-bot names its status after each PR's base branch. In a native stack, an upper PR's base is the branch below it, so policy-bot posts policy-bot: <that branch>. But when GitHub merges a stack, it checks every PR against the branch the whole stack lands on, so on the upper PRs it's waiting for policy-bot: main. The names never match and the stack won't merge. I mentioned this on #1202, but that one was really about Graphite-style tools wanting a stable name for branch protection, which is kind of a different thing.

post_insecure_status_checks isn't an option for us. It drops the branch from the name, and you've already said it's insecure, so we can't hang a required check on it.

I had a look at fixing it properly, and the security points hinted at on #1202 do indeed seem like a sticking point. If you just relabel the status, someone can point a stack at a branch with a weaker policy and get that evaluated instead. The only approach I found that holds up is to evaluate the PR as if it actually targeted the destination, i.e. the destination's policy, run against the real diff into the destination. It looks like it would be a fairly involved change, and would also need to consider the stale status after a retarget that you mentioned too.

Would you be open to this as an opt-in feature? I've got a rough prototype on a fork and I'm happy to turn it into a PR, but I didn't want to open one if you'd rather approach it differently, or whether this isn't something you'd be comfortable accepting an external contribution for.

Thanks in advance for any feedback!

[bluekeyes]

Thanks for starting a discussion in the context of native stacked PRs. I'm hesitant to add something only for this use case, especially if it's complicated, since that doesn't help with any of the other situations where including the branch name in the check context causes problems.

The main alternative we're considering is having Policy Bot read the policy from the default branch of the repository instead of from the target branch of the pull request. Assuming the default branch is protected, this allows dropping the branch suffix and solves the core problem of people adding weaker policies on branches they control. It does have two potential issues:

• It doesn't work for repositories that legitimately have different policies on different branches. For some cases, you can work around this with the targets_branch predicate, but that may not work for all situations.
• Policies that use certain predicates (targets_branch, anything about PR content) are still vulnerable to target switching, as there is some delay before Policy Bot can update the status after a target switch. This is most severe when the policy allows auto-approval. The benefit is that the risk of target switching is now policy-specific rather than global.

Any change here also needs to be an opt-in server feature to avoid breaking current behavior.

In summary, my position right now is that I'd prefer to find a way to remove the branch suffix with acceptable security tradeoffs rather than add more logic to make the branch suffixes work in specific cases.

But if you have a prototype, I'm open to looking at a draft PR to see the approach you took and how complicated it is. It's also possible there are less complex things we could do to support the stacked use case while thinking more about the general problem, although it likely depends on how GitHub expose stack details in the API.

[juzsal]

@bluekeyes thanks for the quick and thorough response! 🙌

This is really helpful. I agree that a fix only for stacking isn't worth it. Reading the policy from the default branch is the better approach., it solves the general problem and fixes stacking at the same time.

I will link the stacking prototype here for posterity. I don't think it's the right direction now, so just putting it here to show how involved that route was: juzsal#1

On the default-branch approach, I had a go at a small opt-in version while thinking this through: juzsal#2. Since the security design is still open on your side, is this something you'd be open to considering external PRs for?

[samandmoore]

The approach of using the default-branch's policy in all cases would work for my org.

We're fortunate in that we don't have any requirement for policies that vary across branches.

[vafanassieff]

Would love to have an option to read policy from the default branch (opt in). We default to the current behaviour otherwise.

[juzsal]

@bluekeyes I've put this up as a draft on the repo too: #1350 (same change as the fork PR above). Keen to hear your thoughts 🙏