Adding to Essential-Work sprint as I reviewed this code

The fix for this appears small. The following appears to fix the issue for me locally:

diff --git a/includes/AbuseFilterPermissionManager.php b/includes/AbuseFilterPermissionManager.php
index 9c17314b..a50f326e 100644
--- a/includes/AbuseFilterPermissionManager.php
+++ b/includes/AbuseFilterPermissionManager.php
@@ -59,6 +59,10 @@ class AbuseFilterPermissionManager {
         * @return bool
         */
        public function canEditFilter( Authority $performer, AbstractFilter $filter ): bool {
+               if ( $filter->isSuppressed() && !$this->canSuppress( $performer ) ) {
+                       return false;
+               }
+
                return (
                        $this->canEdit( $performer ) &&
                        !( $filter->isGlobal() && !$this->canEditGlobal( $performer ) )

Just to make sure this doesn't get overlooked – it's not just about fixing who can edit the filter:

Removing the suppressed checkbox (no matter if oversighters do this intentionally or if it happens via this bug) should never automatically unsuppress previously suppressed abuse logs – they might still contain PII

Proposed patch:

Removing the suppressed checkbox (no matter if oversighters do this intentionally or if it happens via this bug) should never automatically unsuppress previously suppressed abuse logs – they might still contain PII

Fixing this is likely harder because the display of these revisions as suppressed is purely controlled by the active flag on the filter. We don't have a way to ensure that the flag is applied retroactively.

Therefore, the only way that I can see to fix this without the need for a database schema change is to do the same as protected filters and make it impossible to undo the setting of a filter as suppressed. @MolecularPilot, what do you think about this idea?

Thank you so much for figuring this out and reporting it. I can also reproduce the issue.

Proposed patch:

Thank you for fixing this, I've checked the patch too and this fixes it locally for me as well. The test looks good as well. Sorry I missed this.

Therefore, the only way that I can see to fix this without the need for a database schema change is to do the same as protected filters and make it impossible to undo the setting of a filter as suppressed. @MolecularPilot, what do you think about this idea?

That could work. Another idea I had is there's already a flag to manually suppress an individual log entry I believe, so we can set this flag on any log entry generated while a filter is suppressed, meaning if the filter is unsuppressed the logs remain suppressed and need manual unsuppression if desired for any reason. Though it might be tricky to backdate this to any logs created between now (when the feature is live) and whenever this patch lands, so I'm leaning towards thinking it's best to make it impossible to unsuppress. I can write a patch for that if we think that's the best way forward and you'd like.

In T414011#11501812, @MolecularPilot wrote:

Therefore, the only way that I can see to fix this without the need for a database schema change is to do the same as protected filters and make it impossible to undo the setting of a filter as suppressed. @MolecularPilot, what do you think about this idea?

That could work. Another idea I had is there's already a flag to manually suppress an individual log entry I believe, so we can set this flag on any log entry generated while a filter is suppressed, meaning if the filter is unsuppressed the logs remain suppressed and need manual unsuppression if desired for any reason. Though it might be tricky to backdate this to any logs created between now (when the feature is live) and whenever this patch lands, so I'm leaning towards thinking it's best to make it impossible to unsuppress. I can write a patch for that if we think that's the best way forward and you'd like.

If unsuppressing filters is disallowed there should be similar warnings to protected filters to make sure oversighters don’t accidentally suppress a filter and are aware of this being non-reversible.
Disallowing unsuppression is probably best because the abuse filter history may also contain PII, not just the log entries.

If you decide to apply the manual suppression flag to abuse logs I don’t think backdating is a huge issue – the feature is new and the number of potentially affected log entries between now and the eventual change will be low, therefore volunteers could be asked to manually suppress the few abuse logs which might be affected.

In T414011#11501238, @Dreamy_Jazz wrote:

Proposed patch:

T414011.patch3 KBDownload

CR+2

This patch only needs to be applied to wmf.10 and future trains. The feature and it's code does not exist in wmf.7 and any release version

In T414011#11502228, @Johannnes89 wrote:

Disallowing unsuppression is probably best because the abuse filter history may also contain PII, not just the log entries.

Agreed, though the different versions of the filter conditions should retain the suppressed flag. However, this does not apply to the filter notes (which could contain PII).

I think preventing undoing the suppression of a filter may be the best way to avoid unintentional PII disclosures.

Moving out of 'Needs Review' as waiting for Security-Team to do the deployment maybe next week

In T414011#11507808, @Dreamy_Jazz wrote:

Moving out of 'Needs Review' as waiting for Security-Team to do the deployment maybe next week

This might need to be an "offsite deploy" as I'd prefer not to deploy anything on a Friday. I'd say we could maybe even wait until we're all back on Monday, January 19th, but that's a US holiday.

I deployed the security patch for this already: https://sal.toolforge.org/log/R3fesZsB8tZ8Ohr0D6aH

Handling the other point (not being able to undo the suppression of the log entries at least) probably needs a different ticket, given that this part of the issue is publicly discussed at T290324#11515167 and would likely not be fixed via a security deploy

This can be made public when Security-Team has the time to do that. The other issue mentioned in the task is described in T414990: Disallow unsuppression of abuse filters (and their log entries & filter notes) which is a public task