Skip to content

docs: add ADR for branch protection #391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions docs/adr/367-branch-protection-md
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
# ADR 367: Enforce Branch Protection by Default Across All Repositories
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be ADR 391?


## Status

Accepted

## Submitters

- @UlisesGascon

## Decision Owners

- @expressjs/express-tc

## Context

Branch protection rules in GitHub enforce key policies—such as required reviews, status checks, and linear history—that help safeguard critical branches like `main`, `master` or version branches/tags. Despite the clear benefits, adoption across repositories has been inconsistent, leading to:

- Risk of direct or accidental pushes to protected branches
- Increased likelihood of unreviewed or unsafe code merges
- Reduced ability to enforce standardized development workflows

GitHub allows fine-grained control over branch protections, including the ability for administrators (tc members, repo captains...) to bypass rules when needed (e.g., urgent hotfixes or security patches).

This decision aims to normalize a secure-by-default stance while maintaining flexibility for urgent, responsible exceptions.

## Decision

**Branch protection rules will be enabled by default on all repositories.**

Key rules to be applied:

- Protect release branches, this includes: `main`, `master`, and `v*` branches/tags
- Require pull request reviews before merging (at least 1 approval)
- Require all status checks to pass before merging

Optional rules:
- Enforce linear history (disable merge commits, allow squash/rebase)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big fan of this, and I think we also had earlier discussion about this where we all generally agreed on it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, We can do it in a separate ADR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could leave it here as optional and then open a PR to move it to required and discuss that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately tho there's issues with this; linear history is good, but github's rebasemerge/squashmerge is bad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

funny -- we just ran into this earlier today, where I had to do a git CLI commit undo, rebase, etc. I can go either way on the branch setting itself, but there will be times it will need to be disabled to avoid messy and risky rebase work

- Restrict who can push directly to the protected branches to people who can release (captains, TC, etc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me understand this optional rule and how it interacts with the exception below?

From my understanding enabling this would allow the people who can release to permanently bypass the branch protection rules, rather than temporarily. It'd be simpler to have everything follow the same process to avoid any confusion when it comes down to this.


### Exceptions:

- TC members and Repo Captains will retain the ability to **temporarily bypass branch protections** in critical scenarios:
- Security fixes that require immediate deployment
- Emergency production incidents
- When CI is clearly broken for unrelated reasons
- To fix history when necessary
- When a reasonable case is made where the exception makes sense
- These exceptions must be documented in the relevant PR or release communication

## Rationale

### Alternatives Considered:

- **Opt-in branch protection**: Rejected due to inconsistent adoption and higher risk of accidental pushes.
- **No admin bypass**: Rejected because it adds overhead in urgent situations and may block time-sensitive fixes.

### Pros and Cons:

**Pros:**

- Stronger default security posture
- Better alignment with compliance and audit requirements
- Reduced chance of unsafe or accidental changes

**Cons:**

- May slow down development for repos lacking automation (e.g., CI failures delaying merges)
- Risk of misuse of admin bypass unless monitored

**Why is this the best option?**

This decision strikes a balance between **security and agility**, using defaults that promote safe practices while preserving escape hatches for legitimate exceptions.

## Consequences

### Positive Impact:

- Improved consistency and safety across all codebases
- Reduced likelihood of accidental or unreviewed code merges
- Easier to audit and enforce development standards across org

### Negative Impact:

- Initial friction for projects with a single captain or no committers (at least 1 approval per PR)
- Possible misuse of admin bypass if not documented


## Implementation

### Phase 1: Rollout to Active Repos

- Identify all active repositories
- Notify repo maintainers of the change
- Apply default protection rules using GitHub API or organization policies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CleanShot 2025-07-13 at 11 20 40@2x

Can you document the API you use so people can re-use/update it over time? 🙏


### Phase 2: Harden Governance

- Periodic review of admin bypass usage using the Scorecard Monitor ([ref](https://github.com/expressjs/security-wg/pull/70))
- Enforce admin bypass documentation for emergency changes
Comment on lines +97 to +100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we split this off into a separate decision?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment about adding more valid reasons above, but seeing this comment I think I agree that we should make that an amendment onto this larger ADR. For a while our CI was broken and the only way I could merge things was bypassing the protections. I think it is unreasonable to limit ourselves like this until we have automated releases since we can always fix the history if a mistake is made.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. No problem to split... what do we want to split only the Implementation or just the bypass rules?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chris was speaking specifically about keeping the Branch Protection ADR as its own, and a score card ADR as a separate decision.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the other changes were made, and this one only says "enforce" the rules we document above. I think that means we can mark it resolve. I am going to and change to approve, but as it was @ctcpip's comment I am very much alright with re-opening it to continue discussion if necessary.


## References

- [GitHub Docs: About protected branches](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches)
- [OSSF Scorecard: Branch Protection](https://github.com/ossf/scorecard/blob/main/docs/checks.md#branch-protection)

## Changelog

- **[2025-06-03]**: @UlisesGascon – Initial proposal and adoption for org-wide branch protection default