[FFmpeg-devel] rebasing security

[Alexander Strasser]

On 2025-08-05 05:06 +0200, Kacper Michajlow wrote:
> On Mon, 4 Aug 2025 at 23:38, Marton Balint <cus at passwd.hu> wrote:
> >
> > On Mon, 4 Aug 2025, Alexander Strasser via ffmpeg-devel wrote:
[...]
> > >
> > > If I understand the original point you wanted to discuss correctly,
> > > than this is not a question of rebase or merge but one of letting
> > > **commits happen on the forge**. If it happens it bears the
> > > possibility of modification on the server the forge is running on.
> > >
> > > TL;DR: I think it's fine the way it's setup now.
> > >
> > > I'm not against letting rebase/merges happen on the server because
> > > otherwise we would lose a lot of advantages and comfort we get by
> > > using a forge for PRs.
> > >
> > > Only alternative I see is to do PRs on the forge and doing merging
> > > manually by the same person that ensures reviewed PR is not changed
> > > and pushes (after rebase or with a clean merge commit) from their
> > > machine.
> >
> > Two things came to my mind about the current forgejo workflow.
> >
> > - Previously it was pretty clear from git history who actually committed
> >    a change from the comitter field. With using forgejo the comitter
> >    field no longer shows the person who actually *committed* the change to
> >    the main repo, but it is inherited from the original pull request commit
> >    instead, so it simply shows the original author of the patch.
> 
> I don't think this is accurate. Committer field is set to the person
> who clicks the "merge" button. Same as they would manually git push
> the patches.
> 
> Slightly related, I don't like how simple the web ui commit log of
> forgejo is, it doesn't show commiter at all. For me this information
> is as important as the author. I'm keeping notes on forgejo usage and
> will share it when the time comes, it has some annoying limitations
> compared to other forges.
> 
> > - A pull request is writable both by the reviewer and the author up to
> >    the point when it is actually committed to the main repo. So force
> >    pushes from an author can happen anytime during this timeline:
> >      - reviewer reads changes
> >      - approves the changes
> >      - rebases the branch
> >      - sets it up to auto merge
> >      - CI actually runs
> >      - forgejo auto-merge
> >    A reviewer may not realize the new force push from the author. Maybe
> >    forgejo handle some force pushes in this timeline gracefully and aborts,
> >    or ignores them, I am not sure. It still looks a bit fragile, my
> >    expectation as a reviewer would be that what I saw when I finished the
> >    review and clicked on the Approve button will get comitted, when I later
> >    click on the merge button.
> 
> There is a timeline of events. If PR is approved, you can see if there
> were new events (comments/pushes) after that. This is close to the
> "merge" button and you should see "approve" as the last event in the
> timeline to be sure nothing changes.
> 
> Also if there were rebases after approval, the approved tick mark (in
> Reviewers list) becomes yellow instead of green.
> 
> It is also possible to configure to completely discard previous
> approval if any push happened.

Right, that is what should be enforced by our forgejo. I think other
ways are not good for our constellation of collaboration.

Anyway while this is an important point, the original danger I
mentioned is orthogonal to that and still there. Something could go
wrong inside forgejo and get pushed into our main git repo. So if
anybody is deeming this to be to risky, now would be the moment to
speak up.


  Alexander