Why:
T367390 hid protected filters from users who didn't have the right to see them
because users with access to private filters also had access to search against
filters and were inadvertantly allowed to search against protected filters regardless
of whether or not they had the right to see them.
Now, in order to be consistent with how AbuseFilter displays private filters, reveal
protected filters to all users but continue to restrict access to their details.
What:
- Only hide protected filters when a user runs a search and doesn't have permission
to access them.
Bug: T381470
Change-Id: I2acb7e066885f6da18b29876c21c5d7d199b9886
Why:
* Protected variables were introduced to support temporary accounts
so that temporary users could be filtered based on their IP address.
* Filters that use protected variables are protected in order to
preserve privacy. This can't be undone.
* The current workflow for protecting filters is to have a 'protect'
checkbox that can be checked for any filter, even one without
protected variables, to the discretion of the editor. This has
led to mistakes (see T37765).
What:
* Do not show the 'protect' checkbox on the form by default. Instead,
show it when saving a filter with protected variables, after form
submission. The user must check it to save the filter.
* Still show the checkbox in a disabled state with a warning message
if a filter is already protected, so that the editor can easily see
it is protected.
Bug: T377765
Change-Id: Ie5c94ac1399860ccdca4482508dd37ff07309764
Why:
* For improvements to the protected filters workflow (T377765),
a checkbox needs to be displayed conditionally on the status
of a previously submitted form.
What:
* Pass a status object into ::buildFilterEditor, and build the
HTML for any warnings or errors in that function, instead of
first building the HTML and passing it.
* This also allows more than one error and/or warning to be
shown if present, though this may not happen in practice yet.
Change-Id: I37da49711e7fc192856f013cd931c05379f8e8c6
Why:
* The ProtectedVarsAccessLogger::logViewProtectedVariableValue
method creates a debounced log entry for accessing protected
variable values.
* This log can be created when a user calls the 'abuselog' query
API. This API query is made using a GET request.
* Because the ProtectedVarsAccessLogger creates the log
when it is called, the TransactionProfiler raises a warning
about writing to the primary DB on a GET request.
* We should address this warning by creating the log entry on
POSTSEND, in a similar way to how the CheckUser extension
creates CheckUser log entries.
What:
* Wrap the call to ProtectedVarsAccessLogger::log in
::logViewProtectedVariableValue with a DeferredUpdate callback
that is run POSTSEND.
** As part of this, the expectations that the code only uses the
replica DBs are silenced inside the DeferredUpdate as the
PostSend-GET expectations do not usually allow for writes.
The only other method would to be create the log via a job,
but we want the log creation to occur quickly and reliably.
Bug: T379083
Change-Id: If14e65b27b0bdd4fd0b99319024ffa281bf44656
Conditional set of variable is not easy to read.
Instead set the variable to null before try/catch
Reported by a new phan plugin (2efea9f989)
This bypass a false positive from phan (T378271)
Change-Id: I037efe8465747b8c915405f38546fc1ea4405a03
Implicitly marking parameter $... as nullable is deprecated in PHP
8.4. The explicit nullable type must be used instead.
Bug: T376276
Change-Id: I303342cf1a002d5f0afc77ce147ce9453ea5282e
Parent class constructor gets type-declaration in 1145328459
Remove simple doc-blocks without further information
Change-Id: I5d2179af0c7b826ca48df239152412205702cd77
Why:
- For account creations and account autocreations, the user_name
property is deliberately unset, to avoid displaying the IP address of
an unregistered user. Instead, `accountname` is set with the newly
created account name
- For logging that someone has seen a protected variable value, we need
to record the username that was seen
What:
- Use `accountname` as a fallback in case `user_name` is not set, when
logging protected variable access
- Update tests to cover this case.
Bug: T376885
Change-Id: I688a3529fac0ad8455977a0cfdb950f0105f550d
Changes to the use statements done automatically via script
Addition of missing use statement done manually
Change-Id: If80031678a474157e4cc78a3d3621dab53aded67
Like CheckUser, AbuseFilter should also log when specific protected
logs are viewed.
- Add support for debouncing logs to reduce log spam
- Log when AbuseFilterViewExamine with protected variables available
is accessed
- Log when SpecialAbuseLog with protected variables available is
accessed
- Log when QueryAbuseLog with protected variables available is accessed
Bug: T365743
Change-Id: If31a71ea5c7e2dd7c5d26ad37dc474787a7d5b1a
CVE-2024-PENDING
Why:
* The 'abusefiltercheckmatch' API allows callers to match
arbitary filter conditions against existing AbuseFilter logs
* The API does not check if the performer has the ability to
see the log details for the given filter, so can allow a user
to bypass hidden and protected visibility settings.
What:
* Call AbuseFilterPermissionManager::canSeeLogDetailsForFilter
before attempting to match a filter against a given AbuseFilter
log.
* Add a test to verify that this security fix works.
Bug: T372998
Change-Id: I4a2467dc4e0d1f8401d5428a89c7f6d6ebcdfa70
- Fix an issue where if a user didn't have view permissions they could
get the preference check error (a preference they wouldn't have) on
SpecialAbuseLog
- Fix an issue where the `change-access` hadn't been updated to the used
disabled/enabled log types
- Fix an issue where a ProtectedVarsAccessLoggerTest test wasn't
correctly using the data provider data
- Improve naming since ProtectedVarsAccessLogger exists in its own test
file instead of being a subset of tests on AbuseLoggerTest
Bug: T371798
Change-Id: I53f22855e63d9e1339361a5c9ee7886e0f74714a
Write logs related to temporary accounts to CheckUser if the extension
is available so that logs are topically centralized.
Bug: T373525
Depends-On: I35d50df7cd6754e29d964cc716fb3c42406272df
Change-Id: Ic95f211f4db7ce6dc2d769d2f3af206f4a3935e4
Similar to how CheckUser logs access to IP information about temporary
accounts, AbuseFilter needs to log whenever protected variables are
accessed.
- Implement ProtectedVarsAccessLogger which handles access logging
- Log whenever a user changes their ability to access protected
variables via Special:Preferences
Bug: T371798
Change-Id: Ic7024d9c5f369eb33c4198a59638de9a1d58b04b
Users need to enable a preference before gaining access to the IPs
from `user_unnamed_ip`, a protected variable.
- Add a preference that the user can check to toggle their access
- Check for the preference and the view right for logs that reveal
protected variables on:
+ AbuseFilterViewExamine
+ SpecialAbuseLog
+ QueryAbuseLog
Bug: T371798
Change-Id: I5363380d999118982b216585ea73ee4274a6eac1
AF rules don't support associative arrays, so the named capturing groups are provided in the array only by their numeric keys.
Bug: T374294
Change-Id: I53b39917e6677f3a5b8f68bcf0faebf48668ea27
Session providers can provide a `canAlwaysAutocreate` flag which
indicates account creation is exempt from autocreate permission
checks. This is used, for example, for providers that provide
users for supporting applications in a wiki farm.
Check the flag and exempt the auto creation from abuse filter
checks as well.
Bug: T373778
Change-Id: Id89358930b92cb8dd05c2b031e764412ee641269
Make the `reason` parameter default to the empty string, so that we
don't end up passing null to ManualLogEntry::setComment.
Bug: T373010
Change-Id: Ifca828401628368bdddae14df2bbeb7391b2c02d
get_debug_type() does the same thing but better (spelling type names
in the same way as in type declarations, and including names of
object classes and resource types). It was added in PHP 8, but the
symfony/polyfill-php80 package provides it while we still support 7.4.
Also remove uses of get_class() where the new method already provides
the same information.
For reference:
https://www.php.net/manual/en/function.get-debug-type.phphttps://www.php.net/manual/en/function.gettype.php
Change-Id: I5e65a0759df7fa0c10bfa26ebc3cda436630f456
Per discussion, as a compromise (that I’m fine with), I’m leaving these IDs localized (i.e. the digits may be other than arabic), and I’m only removing the thousands separators.
Bug: T348717
Change-Id: I77b484fec2071267c53a139104c23755a13f0129
In my recent change c458651370, which used Status::getMessages()
in FilteredActionsHandler, I overlooked the fact that it returns
MessageSpecifier objects instead of Message objects, and the return
value of MessageSpecifier::getParams() is not exactly specified
(the docs only promise that it's an array).
Now I'm working on a MediaWiki core change (I625a48a6ec) that
causes a different MessageSpecifier to be used, which stores
parameters in a different format, and would break that code.
To avoid problems, ConsequencesExecutor now stores Message objects
in the Status, which guarantees that FilteredActionsHandler will
get the same objects back.
Change-Id: I2c1bc8dde9a078d03badecf6d89443b65eeb92c5
A filter using a protected variable can be loaded via filter id
using testing tools even though the user might not have the right
to view protected variables. This can potentially leak PII and as
such, testing tools should check for the right before allowing
protected filters to be seen.
- Unload a filter asap if it uses protected variables and the
requestor doesn't have viewing rights. This:
+ disallows loading of existing protected filters on page load
+ disallows testing against rules that use protected variables
+ disallows subsequent requests for protected filters (via API)
There is a known bug (see T369620) where no user feedback is
provided if an API request for a filter returns no result (typically
when no filter matches the requested id). This commit adds another
pathway to that bug (the filter exists but is protected and not
returned by the API) but does not update this UI/UX.
Bug: T364834
Change-Id: I6a572790edd743596d70c9c4a2ee52b4561e25f3
