-
Notifications
You must be signed in to change notification settings - Fork 21
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
base: master
Are you sure you want to change the base?
Changes from all commits
8b4b33d
942e141
3c40ac7
e6e1cc4
249a0f1
9ffca78
9387996
9ff4764
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
# ADR 367: Enforce Branch Protection by Default Across All Repositories | ||
|
||
## 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, We can do it in a separate ADR? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
UlisesGascon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Security fixes that require immediate deployment | ||
- Emergency production incidents | ||
UlisesGascon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- When CI is clearly broken for unrelated reasons | ||
- To fix history when necessary | ||
- When a reasonable case is made where the exception makes sense | ||
UlisesGascon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
### 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we split this off into a separate decision? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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
?