Hacker News Reader

Comment by Buetol | Hacker News Reader

Forcing any commit to be a PR merged by another dev would have solved it

While I agree this is a good practice, it wouldn't have helped much. He had organization-level admin access and could've easily added a second dev account to accept them by himself.

This part of the discussion started thinking about an attacker with a goal of the malicious code being undetected.

I dunno about "solved", but helped sure. I bet you could manage to get a commit in soon enough before someone else merged that they wouldn't notice the extra commit. Or even a history rewrite adding your code to the last real commit.

Do you know of any companies that do that? That seems like a big hassle.

This is standard practice where I work due to SOX compliance. (No pushing directly to master, all PRs need at least one other person's approval). In practice it's not an issue, since PRs are good practice anyway.

Yes but you can push to PRs after approval and then merge them.

You could revoke push permissions on that branch after the PR is requested though - there are probably tools that do that already.

For mine every feat/fix/refactor is a new branch which then requires 2 approvals to get merged, among other things (style guide enforced as linting, minimum test coverage threshold, etc ).

Tbh it's cool, coming from a previous job at a startup where version control meant just zipping the project from time to time.

This is what happens where I work as well.

Same here. All code goes in a Pull Request, and requires at least 1 approval.

Since Pull Request review is a high priority activity there's no "bottleneck", and you double the bus factor for free + prevent bad things from happening.

Yeah but you can still push commits to a PR that has received approval, and the approval is not revoked.

I don't know of company-wide policies like that (though I'm sure that they exist -- and I do know of individual teams that have such policies), but I do know of many projects which have such policies (for instance, all of the Open Container Initiative projects require two approvals from maintainers other than the author).

The company I am working for does that at least for the "junior" members on their core project. There is a bot that checks the tests pass and a senior dev has to approve the review.

Google.

Reply on news.ycombinator.com