Page MenuHomePhabricator

Ensure testing tools that use `user_unnamed_ip` don't reveal it to users without the view right
Open, Needs TriagePublic

Description

The following AbuseFilter pages can be used to test out/examine filters against existing revisions. Ensure these pages don't accidentally reveal user_ip (eg. if /test uses user_ip in a filter, ensure it fails if the user shouldn't be able to use that variable.

  • /test
  • /tools
  • /examine

Event Timeline

I ran the following tests using accounts with no permissions (logged out), view private only, and view private + protected:

on /test (no change needed):

  • the page is not available to anyone who doesn't have access to private filters
  • with access to private filters but not protected variables, the specific log is also not accessible if the filter is protected

on /examine (no change needed):

  • user_unnamed_ip isn't loaded when comparing against recent changes
  • tested this against an edit that was warned/blocked/logged only from a temporary account, examined the RC (had to use action = 'edit' to even see an RC) to confirm that even if the original RC had been triggered via IP match it didn't show up here

on /tools (no change needed):

  • this page seems to only check filter syntax, no comparisons to revisions are available

on filter pages (no change needed):

  • the page is gated behind the protected variable view right
  • the filter can be tested, which leads to the /test page, which as noted doesn't need changes

As part of due diligence, I also grepped for other pages in case they were also possible leaks. Most of these were covered by the patch that implemented the protected variable concept but I'll note them here just so it's somewhere:

$ find . | grep AbuseFilterView

./includes/View/AbuseFilterView.php // Base class
./includes/View/AbuseFilterViewTestBatch.php // /test page, see above
./includes/View/AbuseFilterViewList.php // Main page, shows protected filters, hides hits as expected
./includes/View/AbuseFilterViewHistory.php // List of filter changes, hides protected filters as expected
./includes/View/AbuseFilterViewTools.php // /tools, see above
./includes/View/AbuseFilterViewDiff.php // Diff in filter changes, hidden from unprivileged user
./includes/View/AbuseFilterViewExamine.php // /examine, see above
./includes/View/AbuseFilterViewEdit.php // Filter edit page, hidden as expected
./includes/View/AbuseFilterViewImport.php // Import filter page, no revisions
./includes/View/AbuseFilterViewRevert.php // Filter action revert, that the filter has been tripped can be seen (assuming you have revert rights) even if it uses protected vars but this matches the existing private/hidden behavior

@STran I find that if I go to Special:AbuseFilter/test/<id> I can see the rules for the filter even if I do not have permissions to see them going to Special:AbuseFilter/<id>.

Change #1042228 had a related patch set uploaded (by STran; author: STran):

[mediawiki/extensions/AbuseFilter@master] Add protected variable view permission rights

https://gerrit.wikimedia.org/r/1042228

Tchanders renamed this task from Ensure testing tools that use `user_ip` don't reveal it to users without the view right to Ensure testing tools that use `user_unnamed_ip` don't reveal it to users without the view right.Jun 13 2024, 9:58 AM

Change #1052753 had a related patch set uploaded (by STran; author: STran):

[mediawiki/extensions/AbuseFilter@master] Disallow protected variable access on AbuseFilterViewTestBatch

https://gerrit.wikimedia.org/r/1052753

Change #1052753 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Disallow protected variable access on AbuseFilterViewTestBatch

https://gerrit.wikimedia.org/r/1052753