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 35BA84C719 for ; Tue, 5 Aug 2025 03:19:09 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTP id 99AAF68038D; Tue, 5 Aug 2025 06:19:04 +0300 (EEST) Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTPS id 09EEB68A5E3 for ; Tue, 5 Aug 2025 06:18:58 +0300 (EEST) Received: by mail-ed1-f41.google.com with SMTP id 4fb4d7f45d1cf-61580eb7995so11238543a12.0 for ; Mon, 04 Aug 2025 20:18:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1754363936; x=1754968736; 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=cvVpBCxdW5qBnIY1dCdguImIwPAmbSHCkdBX+SOL8S0=; b=h2ZGwXLu7n4QcW4Kn0IBUOzeSuBoZpTVtv3gNXhOhxSlatPSq4JLS/D35qQXbbILfL 4XRYadN2X1DU7ulkcHm4nLnXWcX8sv/BL4ONBnhO8Xr0UkNGGvxjw8tcHEFLHCFy2M9+ B3sXoNhdkuevP57dlOuTXa97E/PdSPiwneHKs93FYPP6dNlt7vft/vqjZieTlhAvFxg6 TqfOviTr+lxmbgUqnHMZO2XLrY/ogNaqCrB+i3N5RDehI7ibLnnHdEKHAUr9R1KdQGlI Qr6Vz26pS6O4hHyCIWk4ZyDSOEKzlrZf3+eHhmvjeCdNhBwLXTJ1uyDklPeozo0cCggN HOKQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1754363936; x=1754968736; 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=cvVpBCxdW5qBnIY1dCdguImIwPAmbSHCkdBX+SOL8S0=; b=Y2L432u5cQBwcnMaWk8QDwqVZxeoKZ4SBarBfPC742kmoGo9A2Z69VZ+VavOKvvE3K t8zT2dJmZy8WEl1NtT6k5eovSNIipe7grePEW6uHdw3bl/G9jiQT6oQ88mwKBCjO3GFC g5XFAEePQLNpDGEo/sQ+kmsznUdNm5N/7PKOu5juQpnBKjOmwIvu0AauHe2Dg2Suf2Y4 0MeoJRM0cUMrvH8ZaOZ0ReW9xWu1yahtGLogAor3QBYsW2hTiPrPo0AtCONbMiCg4stc Qf7uukGKRyVoYH05XnleSxo8qrppFsU+OPCDfcKyDAWI2zGmR+ElQ5VuSL6V2H/BLlUs tzlg== X-Gm-Message-State: AOJu0YwdEjdIb+cWo7R3iIvmnXMLKyQ8m78WBlif3xyKIK3U4XE9GBJX HuY5uCRwBbzw8KaWOsCr+M7T/n0Y1cA+T/qgadGLZQNsettEwcM9rYj6g6N7Iz8TUzWcg+Qwoe7 p48lM1OK12x48hFLZgpZ0P8VK9o/z+SsxeBF9 X-Gm-Gg: ASbGncvzXHUwnQMx/7V5tMRvAxMv7Kmq8lIbp/Oe0drmzPRXAF/1bnZrM/2NKTydRKp DJME09n6HYY4GydahAwyG3dFweX8VeCUAopvi3CGU0Xa/L5FN+0HoVRaX2zdI4fGv9upSLOkOOs i1EuLT2KVAZ08cs+iafuxAU0rudoTyBepWodp586yT+tWrq6dRLrroTHpMJYm2bGoG5eybWqMoF hJatMdw4UAV35s= X-Google-Smtp-Source: AGHT+IEPSq7xJ7jh4U2XuuLZ7a9rtAkk5AegUioBTmAqq6F3Q/kPFfHfjAewHLi7qCisUxeO/CnviepJT10K2eU32bc= X-Received: by 2002:a05:6402:3509:b0:613:5257:6cad with SMTP id 4fb4d7f45d1cf-6177b04dfacmr1541748a12.11.1754363936454; Mon, 04 Aug 2025 20:18:56 -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:18:44 +0200 X-Gm-Features: Ac12FXztSHx_HdVz0Y9r1mUhsnaub5cZ0x_QKLVO4WjVKSAUv_-qrCS2J3Jdy8g 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 Tue, 5 Aug 2025 at 05:06, Kacper Michajlow wrote: > > 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 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 _______________________________________________ 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".