[FFmpeg-devel] rebasing security

[Kacper Michajlow]

On Tue, 5 Aug 2025 at 05:06, Kacper Michajlow <kasper93 at gmail.com> 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:
> >
> > > Hi Michael,
> > > hi all!
> > >
> > > I think it's a good time to bring stuff like this up for discussion.
> > >
> > > On 2025-08-03 21:02 +0200, Michael Niedermayer wrote:
> > >>
> > >> On Sun, Aug 03, 2025 at 05:31:39PM +0200, Michael Niedermayer wrote:
> > >> [...]
> > >>> The solutions are obvious:
> > >>> 1. ignore security and supply chain attacks
> > >>> 2. use merges not rebases on the server
> > >>> 3. rebase locally, use fast forward only
> > >>> 4. verify on server rebases
> > >>
> > >> Maybe not everyone understood the problem. So let me try a different
> > >> explanation. Without any signatures.
> > >>
> > >> In the ML workflow: (for simplicity we assume reviewer and commiter is the same person)
> > >> 1. someone posts a patch
> > >> 2. patch is locally applied or rebased
> > >> 3. commit is reviewed
> > >> 4. commit is tested
> > >> 5. commit is pushed
> > >>
> > >> Here the only way to get bad code in, is through the reviewer
> > >> If the reviewer doesnt miss anything and his setup is not compromised
> > >> then what he pushes is teh reviewed code
> > >>
> > >> if its manipulated after its pushed git should light up like a christmess tree
> > >> on the next "git pull --rebase"
> > >>
> > >>
> > >> With the rebase on webapp (gitlab or forgejo) workflow
> > >> 1. someone posts a pull request
> > >> 2. pr is reviewed
> > >> 3. pr is approved
> > >> 4. pr is rebased
> > >> 5. pr is tested
> > >> 6, pr is pushed
> > >>
> > >> now here of course the same reviewer trust or compromised scenarios exist
> > >> but we also have an extra one and that is the server
> > >> because the server strips the signatures during rebase it can modify the
> > >> commit. And this happens after review and because a rebase was litterally
> > >> requested by the reviewer its not likely to be noticed as something out of
> > >> place
> > >
> > > 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. But this hinders the workflow if there
> is trivial rebase done to run CI on the latest merge base. Reviewer
> would have to approve again. This works if the reviewer is the same
> person who will merge, but if both author and reviewer are
> maintainers, we should trust each other and respect approver to not
> include unwanted changes after this point.
>
> Saying that, I think (if possible) it should be configured to clear
> the approval status if the **user** (not contributor) pushes to this
> branch. This way the reviewer has to recheck it, before merge.
>
> - Kacper

Looking quickly at forgejo source, it seems to do a diff against merge
base, so it won't dismiss approvals on trivial rebases. Only if
something actually changes. In this case I think enabling the "dismiss
stale approvals" protection option is a good idea to ensure the
approved state is always valid.

Of course this would clear approve if someone fixes even a typo, but I
guess it is expected.

- Kacper