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
Introduced in 2019 with 4c8dac4dc6. Redundant since 2020 with
commit c6c62e2c8f in MediaWiki core.
Bug: T139216
Change-Id: I51e9fc3899cf5505917d7899a395350dd86f5c0b
And more db clean ups:
- Use QueryBuilders
- Stop relying on actor migration to simplify query building
- Using expression builder in one case.
- Change the default actor migration stage to read new and write both.
Bug: T354194
Depends-On: I7c116cab0c748707d9a9fd17feeffe26e7d188ec
Depends-On: I74002911749335f4323a03fb430d02f936771b7e
Change-Id: Id84d1db7a2991f3cccc2f4f1502ba77643ddef24
The following sniffs are failing and were disabled:
* MediaWiki.Commenting.FunctionComment.MissingDocumentationPublic
Change-Id: I6075c76d53a899aac56af027f9a956a6b9e6a667
Why:
* An AbuseFilter variable is needed that allows filters to determine
what type the current user is. That is, whether the user is an
IP address, temporary account, named user or external user.
* Currently filters implement this by inspecting the value in
the 'user_name' variable, but this is likely to break when
temporary accounts are enabled as IPs would be hidden.
* Giving a dedicated variable that indicates the type of the user
allows filters to work out this information without having to
know the specific username of the user before performing the
check.
What:
* Add the 'user_type' variable which is lazily computed. It can have
the value 'named', 'temp', 'ip' or 'external' depending on the
type of the user. If the user does not match any of these, then
the value is 'unknown'.
* Replace call to deprecated User::newFromIdentity with a use of the
UserFactory service that is dependency injected.
* Add and update tests to ensure consistent test coverage.
Bug: T357615
Change-Id: Ifffa891879e7e49d2430a0330116b34c5a03049d
This patch solves a pending TODO which is to remove the ::factory()
method from the AutoPromoteGroupsHandler class. If the cache instance
is injected, we'll use it otherwise we'll default to a HashBagOStuff.
Bug: T358346
Change-Id: I2bc414da8733431d1d11025e954282fc7c73aa80
Why:
* In f3c87749b8, the sending of logs
to CheckUser when using a temporary account was fixed. As part
of that change, it was suggested to add a test to verify that
the call to `Hooks::updateCheckUserData` actually causes an
insert into the relevant CheckUser result table.
* This change adds this test as a follow-up.
What:
* Create a test for the AbuseLogger that verifies that an event is
sent to CheckUser and is saved into the DB for an abuse filter
hit. This test is only run if the CheckUser extension is installed.
Bug: T358632
Change-Id: I33ed0810db13e38eacf4e682eb54d4ffcd583084
Why:
* When CheckUser asks the AbuseFilter extension for modifications
to rows inserted into the CheckUser tables, the AbuseFilter
extension attempts to get the Filter user via User::newSystemUser
* User::newSystemUser can deadlock if multiple requests to create
the system user are being made at once.
* The CheckUserHander does not need to create the abuse filter system
and instead only needs to know if a given $user is the equal to
the FilterUser.
* As such the FilterUser service needs to provide a way to check if
a given $user is equal without creating the FilterUser.
What:
* Add FilterUser::isUserSameAs which returns a boolean value
indicating whether the Abuse Filter system user is the equal
to a given UserIdentity in the same way that UserIdentity::equals
is implemented.
* Refactor ::getUser to get the username for the filter user in
a separate method, so that the ::isUserSameAs method can also
use this method. Name this new method ::getFilterUserName.
* Add a test for the FilterUser service to ensure consistent test
coverage
* Convert the @covers and @coversDefaultClass annotations to be
a @covers for the class. This is because PHPUnit recommends this in
https://docs.phpunit.de/en/9.6/annotations.html#appendixes-annotations-covers-tables-annotations
Bug: T356275
Bug: T346967
Change-Id: I8a101781bb47612deabb0f2a06a398ac13e860e6
We are getting rid of the schema of implementing this interface and
calling self::READ_* constants, it's confusing, inconsistent, prone to
clashes and isn't really useful for non-ORM systems (which we are not)
Bug: T354194
Change-Id: I5d7a2c91a49311a6bdf6e56053c08610d4d6d110
Deprecated in T342301 in v1.41. This is now tracked
automatically. The variable can be safely deleted.
Change-Id: I7f42f3bfc58508421f4758089482fd1ed68c42c2
This requires 1.42 for some new names
Changes to the use statements done automatically via script
Addition of missing use statements and changes to docs done manually
Change-Id: Ic1e2c9a0c891382744e4792bba1effece48e53f3
The BadMethodCallException is documented as "thrown if a callback
refers to an undefined method or if some arguments are missing".
This is not what happens in these places.
Change-Id: Ic95b67acc2e17eea1dd0fa1d72f9ac94a86bcf17
Turns out the MediaWikiIntegrationTestCase base class comes with a
bunch of convenience methods for this. We can even remove the custom
messages because these methods already print a lot of debug info in
case of a failure.
Change-Id: I61fd86f1560c8e3bcf39a30b09fecdb063424613
Reverts part of Id9056528a433faf0, to switch to DeferredUpdates in
CirrusSearch back from DeferredUpdatesManager.
Bug: T265749
Change-Id: I8126cc76440724753c356c48ba4e0fcc9be5b41a
Most notably, make it not use additional DB tables to test global
filters. Instead, just pretend that the local database is not local (via
config) and "hide" local filters with a simple test-only flag in
FilterLookup.
Change-Id: Ib431dbf6c9d84978ee84e7f0671cfcbf8a54d7a2
Echo will at some point try to load the user with the given ID, and will
throw an exception if it doesn't exist. The test is currently passing
just because we're not properly cleaning DB tables, and the user with ID
1 happens to exist at that point, but it will fail with core change
Ie2f1809d.
Change-Id: Ie686f4d5c2842e45a6ed564b311bb5d9b0265091
These tests were accessing the Database, for mainly 3 reasons:
- User::newSystemUser
- Static methods in ChangeTags
- Echo's Event class
There isn't much we can do about them, so add tests to the Database
group where needed. In some cases, there are already comments that these
tests should be made unit tests once possible.
Bug: T155147
Change-Id: I8a0d52e0a4cae8a4059b62867853a73e60c878a1
TestUser requires a DB connection, so avoid using it in database-less
tests. Add to the Database group tests that are making DB writes (e.g.,
for log entries).
Change-Id: I211cb60296e5c2446128fcdf2caaadc728a8c272
I'm planning to add support for bypass and regex-based blocking which
means it'll grow a bit. So let's give it a dedicated class.
Bug: T337431
Change-Id: I5a6fe2fd2f1efdebd8cada0ba6c481341f830e27
The handler class uses hook interfaces from the CheckUser extension, so
it can't run if CheckUser is not installed.
Change-Id: I5f40366f27cc885e95e1bb93ec421b09c7caa9a6
This method actually consists of two: add derived vars, and initialize
content vars. The former part depends on no parameters of this method.
On the other hand, the latter part combines multiple implementations
for some of the content variables using branching.
The branching is a dirty workaround and inferior to the GRASP principle:
"When related alternatives or behaviors vary by type, assign
responsibility for the behavior to the types for which the behavior
varies."
In other words, the callers (extensions) should be responsible for
choosing the initialization strategy themselves, instead of letting
VariableGenerator figure it out.
As the first step, split the former part to a separate method.
For now, it will be implicitly called by ::addEditVars.
Change-Id: I5ff00dbdbf29ec54eabfd95c44a4fd7f713969f5
Service wiring should only depend on config, not on request state.
Creating a session object during service wiring causes issues with entry
points such as opensearch_desc.php that disable the session.
Bug: T340113
Change-Id: I2450b0b6821ff0b097e283ff660a0b8aeea9590a
Protected effectively means "public to subclasses" and should be
avoided for the same reasons as marking everything as public should
be avoided.
Change-Id: Iba674b486ce53fd1f94f70163d47824e969abb77
Unlike what the 20-year old source comments in UrlUtils.php would
have you believe, parse_url() works fine nowadays, including for
protocol-relative URLs and indeed lots of prod code uses it directly.
The class still has some convenience value for case where you need to
expand or manipulate URLs, but for the common case of extracting a part
of it, you really don't need it.
Test plan:
$ php phpunit.php ../../extensions/AbuseFilter/tests/phpunit/integration/FilteredActionsHandlerTest.php
Bug: T337431
Change-Id: I1e76d2f5aef65365743214530faba656325b965a
Or at least one step forward to better isolation.
Some providers were replaced with individual test cases.
Bug: T337144
Change-Id: I5cd8c5e79993260f18c3a17c40b8501a4da3c17f
