From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.ffmpeg.org (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTPS id 7BA644C734 for ; Tue, 5 Aug 2025 03:06:35 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTP id 5255A68BA5E; Tue, 5 Aug 2025 06:06:31 +0300 (EEST) Received: from mail-ed1-f44.google.com (mail-ed1-f44.google.com [209.85.208.44]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTPS id 58EA9687BF3 for ; Tue, 5 Aug 2025 06:06:24 +0300 (EEST) Received: by mail-ed1-f44.google.com with SMTP id 4fb4d7f45d1cf-604bff84741so8957229a12.2 for ; Mon, 04 Aug 2025 20:06:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1754363181; x=1754967981; darn=ffmpeg.org; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=7ejd4QV2k/f8810yxtyB6lMyjfT+rgQFXY/ZK1R1roo=; b=H/w3lNCwnDDcgeeZX7g18y9ZO800ftf0nfgKx/vK6Gd5QyaoGoMbjVI96jlLNnba4X gScP8MElQB3I6kDqm5aYpxG1xckFnjYsSQwx6gN0+KRwNfkhuiSA/mO71cLxQl/0BSs6 eScDwhqIR9c+hsiLEbE9eTqbjAnKVtlk6iLX1F9lvp6N+ttAuR8F94En+wbUVWKMzIXd TxS74Wkfr9muNlrVxmgqUHPIDMP16NEkJMueHO6uv26V5hQK65v81LCsNHH4CMyRkPgu 0ohhmFhiO3gs37rDDNujLimQ7i7fIZFcCji6qxAKm9qQok8QSvkSl2q2D3nVj3klxoDZ OIqg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1754363181; x=1754967981; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=7ejd4QV2k/f8810yxtyB6lMyjfT+rgQFXY/ZK1R1roo=; b=ryZrkCc6gs5tUTD2VnhiL39HNhbnml3ujFppXOGcYpQYVzOzOcLcHiN06SDyxI/T45 DyPb/Er4aW/enPZny7WkLc0JvLccv3nTMTFHCRTEz9MEVxnrD5euETNiQzS9spijwjM/ qg8IJCvdCJhA/MiDfctQ68zD9IHxi0Y7TNa3F9MrceCwMNzJPu4vGTqyEc+KMpGEM36O i9nECm7W2W6ZvZY+6LJghu7+iyE8pdwTeudCVIw7QVfOfEtlvfnFQ6dev/xGs+iqZNSG RsncfmZl6f7ushK4saKI5R4XvyvX/3uB38TXnleICUcWA32aPQpdJZSPImxULWSyNPbD saLQ== X-Gm-Message-State: AOJu0YzPxtcdC6YOd8J4iDgS3YiyQ4eotdZkKuuSSjiNemnrh3Hpeo8z OnkivlqaCPucbNkeQlyo4pnmT5re79usiCllYLXyoHoPsoE5zPYiFEh15oFYiQnDefC6TIJpaZ5 K7c9QWBMfbxzl9RKXQBb+k9WX58H4dDt5Og== X-Gm-Gg: ASbGncuPg1IhmPxu67hTUAt82JnFSFjWTYiojJcNdjgrk2txYrOaYcS8ojcczOGPaq1 coZPBpeaYbj2UZ07APoYihcfKPi1k5JSpCoZ5ThMhcUyOxsWs3GNkQi67Pa3GpUGpE+63ND9r22 QRNibsWkj6ogqkZpjoXogcQ8e7cIg50y6q+1BipBMwAHwK3u2BmvVQp5RdmkGaBrRLRokZvKk/Q 6bO X-Google-Smtp-Source: AGHT+IHnfsxQJi9CsM6Dg6kqDrGLubF/WTaTiXFYhsH3GFZcmdlQQEWHfauH4KKGcKFBot35sRiq/xZ6ZcskWyhf9Pg= X-Received: by 2002:a05:6402:3546:b0:615:a7f4:da26 with SMTP id 4fb4d7f45d1cf-615e6ee9589mr8991545a12.12.1754363181339; Mon, 04 Aug 2025 20:06:21 -0700 (PDT) MIME-Version: 1.0 References: <20250803153139.GC29660@pb2> <20250803190234.GE29660@pb2> In-Reply-To: From: Kacper Michajlow Date: Tue, 5 Aug 2025 05:06:08 +0200 X-Gm-Features: Ac12FXxKldAlgQ_YuRln74whKJ__e4V6QR_ULB-4Wjv4B_4ZsDumI_YLW1PPOGo Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] rebasing security X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Mon, 4 Aug 2025 at 23:38, Marton Balint 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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".