There is a try-catch block but the same call was also done
unconditionally after it, making it throw when global filters
are disabled.
Change-Id: Ic8032592799756521a59ee23c0e76cb03a510b94
$wgAbuseFilterActions shouldn't be used normally, as it excludes actions
registered by other extensions.
Note: mw:Extension:AbuseFilter#Integration_with_other_extensions should
be updated after merging.
Bug: T239348
Change-Id: I89b3f0228eacdf145e8f2dd2a5602d0c7ce75a86
This was NULL for old entries, because no default was added
in I758795f01eaf3ff56c5720d660cd989ef95764a7 (see T263324)
Bug: T269314
Change-Id: I5af8b0d3a9d7b6d2570cf79bbbe8b5b170ba1230
Also fix a bug in FilterProfiler. It would attempt to reset
stats for global filters but we do not record them (yet?).
Change-Id: I0228d8c85dab146deb877dfce506f1e8e7711a9f
* Move all SQL files into db_patches (or below)
** Remove db type from filename
* Remove a lot of duplicated code and simplify
Change-Id: If22f2a2c46a59ac24c89ce612c74d169f053ab26
Just moving code around. Without a unit test because DI
coverage of change tags in core isn't available yet.
Change-Id: Iac861e1e24dae13581b8d9173357a1d6c94be88a
It makes sense to look at this and Iedd7a5dca24 together,
as this patch itself doesn't really fix anything.
Change-Id: Ifef5266b1803d1a96489789b08d9beed044d908f
The consequence-taking logic is moved away from AbuseFilterRunner, to
dedicated classes. There's now one class per consequence, encapsulating
everything it needs to take the consequence.
Several interfaces allow customizing different types of consequences.
Every "special check" in AbuseFilter was generalized to use these
interfaces, rather than knowing how to handle each consequence.
Adding more consequences from other extensions will also be easier, and
it should happen via a hook (not a global), returning a class that
implements Consequence. The BCConsequence class was temporarily added
for legacy custom consequences.
A ConsequenceFactory class is added to instantiate consequences; this
would possibly benefit from using ObjectFactory, but it doesn't because
it would also reduce readability (although we might do that in the
future).
These classes are still not covered by unit tests, and this is left to
do for later. The new unit tests should mostly replace
AbuseFilterConsequencesTest. @covers tag were added to keep the status
quo (i.e. code that was considered covered while in AbuseFilterRunner
will still be considered covered), although we'll have to adjust them.
Change-Id: Ia1a9a8bbf55ddd875dfd5bbc55fcd612cff568ef
This will ease adding new watchers, for instance to send Echo
notifications (see T179495 and T100892).
For now, this is just boilerplate, and converting EmergencyWatcher to
the new interface.
Change-Id: I18d62aba53471202b709cdb19033b1729c5c25b4
This will avoid unneeded queries, in theory. In practice, it will
almost never happen to have more than one filter to throttle.
Change-Id: I5b8df51215463ce4464f6a2d0390f58612a5a213
-Exclude methods and classes that cannot be meaningfully covered
-Add a simple test for AbuseFilterServices
-Exclude ServiceWiring because there's no way to tell PHPUnit it's
covered
Change-Id: I4c67b0d3fea68c7a3b3cbe01b5608f87e1b492db
Needs the patch in ContentTranslation first.
Depends-On: I0b74db70ad4e9768e4dcb84b9decb9c737e942e5
Change-Id: Id186ea99fcf69aa4348e404677ce5da998d83170
The slow filter hits are logged for the target wiki, but
the fix would be on meta, so make it possible to filter
for those
Change-Id: I6e02866479e77d707f4fa951ec909c325b944158
The behaviour is:
- When assigning to an undefined offset, delete the whole array and turn
it into another DUNDEFINED
- When retrieving from an undefined offset, just return DUNDEFINED.
Bug: T237214
Change-Id: I621ee7a16c90bb86a57be04e7ce0a748ecdbfcc7
The main benefit of having a dedicated interface is that we can easily
change the output format. So we're now using a custom array without
references to the DB schema, thus making the import/export process
completely independent from the schema.
Change-Id: I4c0de41d914baf1e9a0e588bd31f95b3524a424b
Add a radio to select between "hide" and "show" instead of a single,
cryptic checkbox which doesn't really explain what it does.
Also wrap the list in a form which will later be used to mass-delete
entries.
Depends-On: I1bb45e47c3b42c01388b99778ce833e4e44419e1
Change-Id: Ie2d019fad5af7c626d722dc348f40eb0db21e527
Remove $title->exists() from the check, so we have the following
changes:
- The AbuseLog will add a diff link for page creations
- Searching the AbuseLog for impact:saved will include page creations
- We don't have to recreate the WikiPage again in RunVariableGenerator
Also remove an old reference to "bug 31656": that comment was added in
rEABFefecf8b2441ae2f31f924ff33103f5affe5d1d62, which changed
Article::getContent() to Article::getRevision()->getRawText(). Nowadays
we don't even use Article anymore, and that conditional isn't even for
retrieving the page content, so the comment is wrong.
Add logging for when the Title object cannot exist, as this should never
happen in the context of the EditFilterMergedContent hook, and always
create a WikiPage. Some signatures were changed to require a WikiPage
object now, and every caller updated to provide it.
Bug: T263104
Bug: T62179
Depends-On: Ic238eaa529ef6bfba06b4dd03924a8e0111d8259
Change-Id: Ibf3bf4f68328ba4a5616ab8f26a8b44d27a25cd7
This is a no-op, moving code around, introducing another distinction re
"filtering actions", which now happens in 2 steps:
- The first step only uses "generic" information available by looking
at enabled actions as a "group". This includes keeping only the
longest block, and removing 'disallow' if other blocking actions are
enabled.
- The second step uses information that is only available after having
"partly executed" (named "pre-checked") a consequence. For instance,
we need to pre-check 'throttle' to see if the throttle was hit, and
remove any other actions if not.
Change-Id: I7be5cfaa61e942a06f97ed52f50e9c8c70a120e8
This way we don't have special cases in executeFilterActions, and instead, we execute
all actions in the same place. In turn, this is going to ease the
transition to a new consequences system: next step is refactoring this
code into a service with proper DI etc.
Bug: T204447
Change-Id: I8134ecc41fbecdbed99faf406e9e3ca91b6123b9
The scope is still quite limited, but as noted in a todo, we might want
to make this completely independent from the database, and add the use
case of ViewDiff.
Change-Id: Ie980fff0983b3e86037265e85da04444c809a6e8
They've been replaced by getters in the Filter class.
Note, the Lookup is not injected in this patch because some places would
need careful thought, so it's left to do later.
Change-Id: I40b8c8452d9df741217d7fa090a5e746a2f46994
This moves a lot of things away from the AbuseFilter class. There's a
nasty static dependency on ChangeTags, but it's very limited anyway, and
it's going to be fixed once T245964 is resolved.
Change-Id: Ia7df4b4d3289c2722323f59ceecf3fdd38277785
Some pieces of code were updated to use Filter objects, while other
places are still to be updated. We also need to change the history part
to exclude actions somehow, cleanup the ViewEdit, reduce direct DB
access or anything mentioning DB fields outside of FilterLookup, etc.
Change-Id: I42b7ded685db76eddd45e4b1336f9828cba811ce
This requires adjusting some methods to work with Filter objects. Some
methods and tests are left in an inconsistent/suboptimal state, plus some todos
were added, but all of this is going to be remediated in another commit.
Change-Id: Id063ee73d97c7aef56323e1457d99704f77ab943
Phan is failing on master with
includes/Views/AbuseFilterViewEdit.php:506 PhanTypeMismatchArgument Argument 1 ($salt) is ['abusefilter',$filter] of type array{0:'abusefilter',1:?int} but \User::getEditToken() takes string|string[] defined at ../../includes/user/User.php:3735
due to a documentation change in core.
Change-Id: Ibc01332c67224e3efc7922d1be882615c2de5d9a
The only usage outside of AbuseFilter (in ContentTranslation) was fixed with
Ifc9ede277791398290786cdb6743137004b5c713.
Change-Id: I22cf9c76ef3b007502045a02c82255ba6c9fd0f2
This is just a start; next step is adding a factory/store method to
get/store these objects. And then use these value objects whenever
applicable.
Note: the actions-related code is still not fully implemented. This is
going to happen as part of the FilterLookup.
Change-Id: I5f33227887c035e301313bbe24d1c1fefb75bc6a
Rely on modern HTMLForm features instead of using a dedicated class
property. The form identifiers are necessary, because these forms are
GET forms, and HTMLForm will always think that the form was submitted,
if it doesn't have an identifier (see T238467 and related
tasks/patches).
Additionally, make the first form on ViewRevert a GET form, like on
Special:AbuseLog.
Bug: T263627
Change-Id: Ia6ca45896732742ef73e401b09663728b9e7dda2
The publish() method that it resembles is not a method defined in
the LogEntry class, and not even in LogEntryBase class. It is
defined in the ManualLogEntry class. Let's reference it correctly.
Change-Id: I60cfceac7c19047e299cf9f704dda8d8ef2f2ba6
TODO For the future: the final directory for Parser-related classes
should be "Parser", not "ParserNS". However, moving all classes now
would make it harder to rebase changes etc.
Change-Id: Ice335f4723e74f4e5fbe8dcc76ff8ea16310962c
Ordering is done by in IndexPager::buildQueryInfo. In fact,
this key is unconditionally overridden there and the query
is sorted by rc_id (specified in ::getIndexField). It would
probably deserve some performance analysis because
the ordering and filtering don't seem to use matching indices.
Change-Id: I9e73d44d868ddf5beba6dc6e4550e851a6df5119
This is a thin wrapper around LBFactory and the global variable, that
can be injected in classes requiring it (no real class right now, but
that's going to change soon).
Also, remove some DWIM-style returns which made the code harder to
understand.
Change-Id: I1d28ad4a67f914103f3a17cda5f61b28070c7f1c
Remove outdated/pointless comments, use already defined variables, etc.
Additionally, make it possible to disable throttling locally.
Change-Id: I98fd5f3eb47b32fc1013360e462a57d932174a95
This is still not very useful, but it's going to come up handy when
we'll be refactoring this code.
Additionally, fix a shortcircuit issue which caused additional throttle
types to not be processed if a type was already triggered.
Change-Id: Ied44d9300b3fa2ad00fe95c9c3da3c3f8faa650b
Make FilterProfiler::getFilterProfile return stats unchanged,
in a structured way. Move computations to AbuseFilterViewEdit,
as they are only useful there. Don't return false on cache
misses, return arrays with zero values instead.
Bug: T266531
Change-Id: I8718cc31a5004340bf742315c7075e10a61fcbfd
This commit splits this method into a version that doesn't need a
filter, and another version which requires one. This latter version has
a single mandatory parameter, $filterHidden, and it's up to the callers
to retrieve the value to pass in.
As mentioned in a TODO, this should eventually be changed to take a
Filter object (still under review as
I5f33227887c035e301313bbe24d1c1fefb75bc6a), which is also why
AbuseFilter::filterHidden is not being used here.
Change-Id: Id47a80131e12a5f7e1e93676299641dbf1e2b0ad
FilterProfiler::getFilterProfile returns data in a different
format than the data is really stored.
Bug: T266531
Change-Id: I0d961a1ae67769da61f841df2462d47f81849972
This deals with data inconsistencies in buildFilterEditor. Every
property of $row was tested in all 5 scenarios (also using Selenium) to
check when it's set. The result is in the normalizeRow method, which
aims to remove any inconsistencies, so that buildFilterEditor always
receives a "complete" row with all defaults set.
The code in buildFilterEditor is now cleaner (because there are no
isset() checks), and it gives us a unique place where we can set
defaults (rather than partly doing that in
loadRequest/loadFilterData/loadImport, and partly relying on isset).
This will be especially useful when introducing value objects to
represent filters, because now you just have to look at normalizeRow()
to tell which properties are allowed to be missing, and thus what "kind"
of filter object you need (see
I5f33227887c035e301313bbe24d1c1fefb75bc6a).
Additionally, reduce the properties that get passed around during
export/import, and make the selenium test try a roundtrip, rather than
relying on hardcoded data that may get outdated. A future patch will
refactor the import/export code.
Change-Id: Id52c466baaf6da18e2981f27a81ffdad3a509e78
Unfortunately, this isn't using DI completely, because of the
User::newSystemUser call. I'm not even sure if we really need to call it
or we can just stick to new UserIdentityValue, but leaving like this for
now.
Also, the types were weakened to UserIdentity, so the transition is
going to be easy anyway.
Change-Id: I08f8fae0fcc622ff0ac3f86771476d06d1c18549
The previous code would call getUserGroups again once creating the log
entry, but this was slightly flawed: we're updating groups on master,
but the read happens on a replica that might be outdated, hence
resulting in broken logging. Instead of reading from master, we can just
keep a list of the groups that were actually added, and use that
afterwards.
Change-Id: I7cc282e15561de3a3d3e183808a65991aa27d2bb
This service is responsible for the blockautopromote feature:
(un)block autopromotion and check status.
The patch mostly moves code from static methods to the new class
and relaxes type hints (e.g. from User to UserIdentity).
Change-Id: I79a72377881cf06717931cd09af12f3b8e5f3e3f
Previously, AbuseFilterHooks would proxy the data from a slower backend
(db-replicated) to a faster one (hash) reusing the same key. This change
makes it use a dedicated key, so that the "main" key can be kept
internal inside the upcoming BlockAutopromoteStore.
Change-Id: Id46a66991d0e994ee0a83b83b9c95e8951f3041c
- Add a helper method to output an unrecoverable error, comprising a
button to go back to the filters list;
- Move the token check to attemptSave, so to make the conditionals
easier to read, and group errors together
- Make buildFilterEditor take an HTML parameter for the error, so the
caller can specify whether it's error or warning
- Move the check for non-existing filters out of buildFilterEditor
- Add a bunch of typehints
- Don't set af_throttled and af_hit_count in the empty row template, but
set af_deleted (these are only used in buildFilterEditor)
- Make AbuseFilter::translateFromHistory consistently include the af_global
property (previously it would only be set for global filters; this error
was introduced when first implementing global filters)
- The only user-facing change is that, when trying to use a custom
warning/disallow message on a global filter, this is now considered a
non-fatal error, so we now show the editing interface (and not just an
unrecoverable error).
The next step is resolving the @todo in buildFilterEditor about null
checks.
Change-Id: I9d217dcac3f4cc0b26e53eca735cc327d5efc76d
This commit avoids direct queries on the DB, which is already an
improvement. It also adds some TODO comments for future improvements,
mostly things that depend on core changes.
Bug: T265224
Change-Id: I8eb76a0c463751976c2c5deedb3570305f1ab4f0
There's no point in repeating the action name, because it's already used
as key. We can then flatten the array and just keep the parameters in
the third nesting level.
Change-Id: I54abcc49322f432cedd361abeedb72e067d3de41
The schema changes hook was chosen because the handler is very long. The
test ones were chosen to keep test things away from actual code.
Bug: T261067
Change-Id: Ie06bf62399f6353e3e268cccb3fe4b41bbf951c5
Follows up Ib66c42ac220731f4e1da9ee6cfb5290759dd6494.
Apply DannyS712's suggestions from that patch.
Change-Id: Ib9f19969a888bd29f9f46e90fb52b49ce883c667
So that sysadmins can further customize the extension. It was also wrong
to use the same variable for many different things.
Note that there's no associated patch in wmf-config because we use the
defaults. However, before merging this patch, please recheck that
AbuseFilterRestrictions and AbuseFilterDisallowGlobalLocalBlocks aren't
used there (https://codesearch.wmflabs.org/operations/?q=AbuseFilterDisallowGlobalLocalBlocks%7CAbuseFilterRestrictions&i=nope&files=&repos=)
Bug: T175221
Change-Id: I7581b3ee6d9d11a6cf1599b8ff874e8c3d54adf4
This hook is called on every request, even for view actions, hence it's
a hot spot and a potential source of performance issues. We can slightly
optimize it by avoiding a cache lookup if blockautopromote is disabled.
Note: this won't really have an impact on WMF wikis since blockautopromote
is enabled almost everywhere.
Bug: T22487
Change-Id: I3743bfea9fe5865a3947cd23a07ae27e2dfa9301
The logic about action IDs and the persistent buffer is now encapsulated
inside a single service, which is a step towards getting rid of global
state in the AbuseFilter class, and reducing the responsibilities of the
Runner.
An important change made here is that we now require a LinkTarget rather
than a Title. This removes a dependency on the Title class (a monster
object), makes tests simpler, and denies the need to inject a
TitleFactory. This means living without some bits of context (e.g. we're
no longer using makeTitleSafe to ensure a valid title, and we have to
build a "prefixedtext" manually), but this shouldn't be a problem, given
that the titles are only used to create a cache key: invalid titles are
not a problem, and concatenating namespace + title should always be
sufficient.
Bug: T265370
Change-Id: Iff59cd3d889454a482a89c16691bfefcc5ec0a12
This patch addresses two issues observed in WMF production:
- Specifying a search mode without a search pattern would result in a
call to mb_stripos (in AbuseFilterPager) with an empty delimiter,
which triggers a PHP warning. Avoid this by checking that the search
pattern is not the empty string, and unset the search mode if that's
the case.
- Trying to use an invalid search mode would result in an unhandled
LogicException. We have some code in place to check the validity of
the URL parameter, but the relevant code didn't reset the search mode
to null, hence AbuseFilterPager would throw before we can show a
pretty error to the user.
Bug: T265994
Change-Id: Ib19d36d6265981097bbb551783fdac8bdaa98854
It doesn't make much sense to try to remove implicit groups like 'user'
and '*'. As a matter of fact, these groups are also excluded in
AbuseFilterViewRevert when undoing degroups.
Change-Id: I292499611ccfbd12df28b713d4244530db15c26d
This method was divided into multiple, shorter methods. We now have a
dedicated method for imports, and one for everything else, plus a method
for loading actions. Merged a conditional for when the token didn't
match. Avoid returning Status objects with data inside as it's too
difficult to properly infer types for those.
This is still not perfect, and another round of simplification might be
necessary before this class can be updated to use the upcoming Filter
value objects.
Change-Id: I2de1de1982105e5b9b817a893c357615ffb7db86
While this might seem a small change, it removes the last remaining
coupling between SpecialAbuseFilter and the *View classes, that were
forming a huge tangle.
Change-Id: I5a9d6516e3fa2d3efc4bb2e19b05379dc33cd84d
Don't create <a> tags without a href. Show a placeholder
message instead of nothing (alternatively, we could create
a new message for each existing one).
Bug: T174000
Change-Id: Id55b90881aacc620ff3c519ad6eedf212f36c4ed
The first one is UserGroupManager, used for the 'degroup' action. This
is a simple one-line replacement (repeated twice), and the current code
was already using this service under the hood.
The second one is BlockUser, which is not a one-line change (but still
quite simple). In particular, this allows us to avoid duplication with
core logic when constructing the log entry (this is now done by
BlockUser).
Bug: T248743
Change-Id: Ib7c1dc107a169b575f7021e64b6a8fee09529548
This code was simply caching the AbuseFilter::$tagsToSet property, but
this is not necessary. The only tag that can be buffered during edit
stashing is the conds limit tag. So we just save whether the conds limit
was hit, and apply the tag from a single point afterwards.
Also avoid checking whether 'tag' is enabled as an action, since this tag
should always be added when applicable.
Next step is creating some sort of Watcher service that will do
everything on its own: check whether the limit was hit, save this
information, and tag the action later.
Bug: T265370
Change-Id: I90319a658736fad7d564cb51152061709c230411
- Depend on a generic IContextSource rather than SpecialAbuseFilter
(lower coupling);
- Inject a LinkRenderer (IContextSource doesn't have a ::getLinkRenderer
method)
- Add a helper method in SpecialAbuseFilter to get the page title, that
can also be used elsewhere (and the name constant can be made private
now)
- Pull down the mFilter property (and rename it to just 'filter') to
classes that actually need it. Some classes didn't need this at all
and the types were different among subclasses
Now the only cause of coupling between the View classes and
SpecialAbuseFilter is the static call in getTitle.
Change-Id: I3df0c3a7621f0cc9a64a16b0a402a15aae2d5d73
This service should act as a mediator between the AF code and the
permission manager, and it should know what are the permissions required
by each action.
Change-Id: Ieb177d9992147b11fa7b8f05929da6c182cc2286
In particular, the interface shouldn't generate links to
"Special:AbuseFilter/history/0" (AbuseFilterHistoryPager::getTitle,
can be seen when visiting "Special:AbuseFilter/history").
Change-Id: Id3dc1bb4fc3c5e853603bf0ec04a6b1751f7d862
PHP is not strongly typed, so it's not a good idea to use scalars of
different types (here it's an integer vs the string 'new') to represent
different possibilities. This can have bad effects when type juggling
occurs, and it's also harder to figure out what the type of the
parameter can be (because a numeric ID might have been passed as a
string). Using integer vs null avoids all of this, and also allows us to
use nullable typehints.
These changes were partly copied from
If981cb35bf19a8469aa6c43c907e107cf8c65bc2 and should help with the
migration to the Filter value objects.
Change-Id: I8837d46c3c33761fea53f67b530b721dc7bd49b0
This feature didn't work and even if we fixed it as suggested
in the task, it would still be bogus. For deterministic paging,
the afh_user_text field should be in an index together with
another field(s). But currently it's indexed alone.
By the way, the indexes on abuse_filter_history should be fixed
anyway. Special:AbuseFilter/history also allows filtering by
filter/user which require index on the fields. They are present
but are not composite, so either the sorting is done
inefficiently without an index or there is a fullscan.
Also remove the getIndexField override. TablePager knows best
what value can be used there, we don't really have to override
it.
Bug: T204210
Change-Id: I7335f82c917a1d219fd7f0999da5b62433f14bd8
This was a means to bypass the limitation to filter by
triggered filter (for example, when a group contains
a single filter).
Change-Id: Icd7b0b64ff16b4ce26f4d52ad9d9abce62972e60
This patch removes the dependency of saveFilter on the ContextSource
kitchen sink. It also removes some unneded dependency, and adds
$originalRow/$originalActions as parameter, rather than hacky properties
in $newRow that are easy to forget. The related test can also be greatly
simplified.
This also introduces a behaviour change: checking $newRow instead of the Request allows us
to account for values normalization done in
AbuseFilterViewEdit::loadRequest, and to also work correctly for imports
(and generally speaking, it makes the method suitable for an
AbuseFilterEdit API module, too).
Next step is moving this method to a service. Some signatures,
indenting, name choices etc. are subpar, but this is just because these
methods are temporary anyway.
Bug: T213037
Change-Id: I235b928d7b9c2ef1c46ea0bf3e3ed212500b4161
The array_filter is likely meant to empty the array if the empty string
was exploded ( `explode( "\n", '' ) === [ '' ]` ). However, it can also
remove other stuff, e.g. the string '0'. An explicit comparison is
easier to read & interpret, marginally faster, and avoids rare but not
impossible edge cases.
Change-Id: Ie77d65b56319664a2ac370f32341dc72b619a635
Previously, the cached value would depend on the tags
parameter to be updated. The provided value may be
different for each call, so callers may receive
unexpected values.
For example, while core usually calls this with core-defined
hooks, our method AbuseFilter::isAllowedTag calls this
providing an empty array. If core's call happened shortly
after ours and hit cache, its array would be overwritten
with only AbuseFilter's tags, the rest would be lost.
Also do some clean up:
- only call array_filter on explode'd array
- call array_unique on the value, since it's usual that
multiple filters share the same tag
Noticed when thinking about moving this to a service.
Change-Id: I4f4322e80ec89e48458a3bf46a1146863bec8237
af_actions and af_hidden are treated in the same way, so avoid
duplicating that code. Some of the remaining cases are also quite
similar (although not identical), so we might want to merge them in the
future.
Change-Id: I1b48502e077e58eb9ff459326bba18bb1d127242
Copying my investigation from I8c93e2ae7e7bd4fc561c5e8490ed2feb1ef0edc2:
This code was introduced in 2009, see rEABF0f1eb8db78bfa83ddb93427f39aad619523d8f25:
$display = wfMsg( "abusefilter-action-$action" );
$display = wfEmptyMsg( "abusefilter-action-$action", $display ) ? $action : $display;
And wfEmptyMsg looked like this:
function wfEmptyMsg( $msg, $wfMsgOut ) {
return $wfMsgOut === htmlspecialchars( "<$msg>" );
}
so this made sense. But then, in 2010 (rMWae3ced88e535c7fd046f0ad6f0710cc87f0004ea) the function was changed:
function wfEmptyMsg( $key ) {
global $wgMessageCache;
return $wgMessageCache->get( $key ) === false;
}
without anyone removing the parameter from AbuseFilter.
Finally, in 2012 (rEABF176227e721c9475de2c2163d3b6e20ca4769c406) the usage of wfEmptyMsg was removed, and $display became a parameter to wfMessage().
Long story short, no need to pass that parameter.
Change-Id: Iad875f0c0ab5aaa06c795232638f52e9ca62786e
Ideally, this might live in MediaWikiIntegrationTestCase. For the
createaccount one, AuthManager should also provide a method to log the
creation, because currently we are forced to copypaste that code here.
- Add the missing tests for 'upload' in RCVariableGenerator, and adjust
the existing ones (delete file afterwards, more tablesUsed, use the
right extension).
- Exclude from the coverage report a couple of lines which should
theoretically be unreachable. Escalate logging to WARN level, where it's
more likely to be spotted.
- Remove an unused method (RCVariableGenerator::newFromID). This denies
the need to maintain and cover it. We also don't want this generator
to act as a factory.
Overall, this change brings the coverage for RCVariableGenerator to 100%
Bug: T201193
Change-Id: I425c3d9f6800f74eb6e4eda483b90cfb3bbbcb51
This was also long overdue. Also fix a bug that caused page creations to
not be shown when examining past edits (using rc_last_oldid doesn't work
for page creations).
Bug: T201193
Bug: T262903
Change-Id: I5f7a994add12332c950904146248c5de7c2beee5
- Make a separate method which determines the view
to be shown from subpage syntax and test it.
- Reduce circular dependency between SpecialAbuseFilter
and AbuseFilterView. Use params to transfer information
to views.
Change-Id: Ib9442ea5f9990a5c48f9b9e04055aa22bf7e456e
- Include an attempt to restore the dump in case the text table
contains a truncated dump (not 100% sure that this can really
happen, nor do I know the cause, but it shouldn't hurt)
- Remove a check for 'action'. The variable might be missing in case of
a corrupted dump. Having an array at that point can only mean "new
format".
- Don't assume that old_wikitext and new_wikitext are set when showing
past filter hits (again, might be unset due to data corruption).
Bug: T264513
Change-Id: I7510d28fc3f43f985a1283e23b413f07adfe7921
This is a simple change but with tons of benefits:
- Easier to track usages for IDEs
- Easier to understand in static analysis (phan)
- Can be analyzed by phpda
- Ensures no typos
- These classes can be namespaced without affecting readability here
Change-Id: Ic04d19dfbe9184baf2ef4bac53011521e2e44953
- Use null instead of empty strings
- Check the mode, and not the pattern, to decide whether the user
searched for something
- The call to parent::__construct can now be moved up
- Note in a comment how this code is problematic due to "smart"
constructors
- Avoid caching the headers, as that's not going to work anymore.
Change-Id: I420ab0215d53354a67d9d130ebd8d85dfbd2778b
These have been saved in the parent class for quite some time.
Refactor accessors in method overrides.
Change-Id: I9819caa5ab87ac3a8e47efb32b00d89c3e2a61af
The current code was more of a subpar, temporary solution. However, we
need a stable solution in case more variables will be deprecated in the
future (T213006 fixes the problem for the past deprecation round). So,
instead of setting a hacky property, directly translate all variables
when loading the var dump. This is not only stable, but has a couple
micro-performance advantages:
- Calling getDeprecatedVariables happens only once when loading the
dump, and not every time a variable is accessed
- No checks are needed when retrieving a variable,
because names can always assumed to be new
Some simple benchmarks reveals a runtime reduction of 8-15% compared to
the old code (8% when it had varsVersion = 2, 15% for varsVersion = 1),
which comes at no cost together with increased readability and
stability. It ain't much, but it's honest work.
Change-Id: Ib32a92c4ad939790633aa63eb3ef8d4629488bea
When the user selects to see global rules and it's a remote wiki, hide the rule search field. (Note that the list of search modes needs to listen to this setting as well.)
This was discussed during reviewing I0771fa048.
Also move local/global filters setting to the top as it's more important than that for disabled and deleted filters (which will both stay together).
Change-Id: I0912aa1f5d7a5d75e6ae5a2a3362b8d38260c611
This will decouple a bit the huge and chaotic tangle of AF classes. Some
boilerplate code for AbuseFilter services is also added with this patch.
Note that this requires injecting a KeywordsManager in
AbuseFilterVariableHolder, or unit tests would fail. This is still
incomplete, and the Manager is only injected in tests, because
VariableHolder still has to be refactored.
The test for the UpdateVarDumps script had to be updated, because
serializing VHs in there was a bad choice. As pointed out in a comment,
the test is likely going to break again once we remove the BC code, but
I hope that we'll be able to remove the test at that point.
Change-Id: I12a656a310adb8c5f75cab63f6db9e121e109717
These methods had no reals reason to be static and belong to the
AbuseFilter class. Most of them were moved to Parser class as common
variations of the existing entry points. One was specific to the
EvalExpression API module and was moved there.
This change comes at no cost, and will make it possible to inject a
parser where needed.
Change-Id: Ifd169cfc99df8a5eb4ca94ac330f301ca28a2442
This adds some coverage for the *VariableGenerator classes. It's still
not perfect, but something to start with in sight of future
refactorings.
Bug: T201193
Change-Id: Iafa85fb8623ea278ce6e42118df72751806382c2
This reverts commit 6a268e7339.
Reason for revert: Ic1252efe9f96743d9402fa31a7b2dca1f57ff6ae ended up not renaming the index, so this patch removed an index that was still in use.
Change-Id: Ide4a600a57bcfa4da0c7354b972cc89709ccd660
This fixes the abuse_filter_log patch-afl_change_deleted_patrolled
not being applied. The patch is provided for (and should work with) all
the supported DBMS.
Additionally, fix the base table files, which would report
afl_patrolled_by as 'NULL', whereas on the WMF cluster it's 'NOT NULL
DEFAULT 0'. The schema patch takes care of converting that column as
well.
Note that this schema change needs not be applied on the WMF cluster, as
that's already up-to-date.
Finally, note that this patch must be backported to 1.33 and 1.34 (and
it might be fairly hard due to the recent schema changes on the
abuse_filter_log table).
Bug: T240895
Change-Id: Ibdbc9b50c25b9e871ebdeae93a54d10877b585f8
The <pre> element is now hidden with CSS, and is only shown after the
user clicks the "Eval" button.
Moreover, make the button primary and progressive, as to indicate that
it activates the primary function of that page.
Bug: T253492
Change-Id: I300ce6ec0a84ea73025a5af9173024df7c291e03
We have many topnav links, and future patches may add others (e.g.
Ia5fd4f0b35fcabf045a7b49fa40fa85b72c92544). The "import" feature is
probably the less used, and is also pretty similar to creating a new
filter.
Thus, remove its link from the topbar and move it to a button next to
the "Create a new filter" button.
Note that the old message is reusable, and thus it should be moved on
translatewiki after merge.
Change-Id: I52042d62b2bab7e4a1e9bbc027e7de5addec8157
While checking a filter, if a variable is not set (e.g. added_lines for
an account creation), the VariableHolder will return a DNULL, rather
than a DUNDEFINED. This means that some filters will resume working, and
the WMF servers will stop getting AF warnings at a rate of 4 millions per
day. This also requires adjusting some tests to reflect the new
behaviour (which is actually the OLD behaviour, that filters had until
last year when we introduced the DUNDEFINED data type). It also requires
adjusting a check in the old parser, but that's not really relevant
because the plan is to remove the old parser before 1.36 is released
(see I0e75f334c7e0dfc1239f2e5f5f7d7452b0bbf29e).
Bug: T230256
Change-Id: I4d06303047397674c1edbfc32628f1bc83ac3340
Rather than always using 5 days, the length (in days) can be configured by setting
`AbuseFilterBlockAutopromoteDuration` to the desired length.
Bug: T231756
Change-Id: I996e08a9099ab59657fe511ec2934d26edfa5c7b
This is an intermediate step for better "diff" links
on abuse log. With this first change, only links
to existing revisions are shown.
Change-Id: Ib420d46fd34dc38d8c7fd3d511a905738e49db0b
For history action, the link would be already added by
HistoryPageToolLinks hook, so it should not be duplicated by this hook.
See images on https://phabricator.wikimedia.org/T261087#6430172
Change-Id: Ia8dd5be49d3ffb48f298ea287e0b2f98c3052015
This shouldn't happen before the script has been tested thoroughly on
WMF wikis with --dry-run.
Bug: T213006
Change-Id: I51425c85bd6932a5c60eb870b02195aae1c24117
This can be different from the User set inside the $context object, as
seen e.g. in Wikibase jobs. Given that the hook provides a $user param,
it makes more sense to use that, rather than extracting it from the
ContextSource kitchen sink.
Bug: T258717
Change-Id: Ib5961068d3df6ae2bfc3f9c6a7b9e555d248b332
Some AbuseLog entries from 2009 are missing the 'timestamp' parameter
used to compute the old wikitext of the page. This was only used for a
short amount of time before
https://phabricator.wikimedia.org/rEABFd1d27eede6536067c5180b2515ea937d71525d4d.
Nowadays, it's causing a fatal error when we try to migrate the affected
entries, see T246539#6388362.
Since we only have a Title available, we cannot rebuild what the old
wikitext would look like, so a placeholder text is used (this should
hopefully be clearer than showing an empty string).
Bug: T246539
Change-Id: I5230f2fdc84da121728a5a75da458f1a4ef1ecd3
ParserOptions::setTidy() was already a no-op in MW 1.35, and
AbuseFilter already requires MW >= 1.35 in extension.json.
ParserOptions::setTidy() was deprecated in MW 1.35 and will be removed
in a future release.
Bug: T198214
Change-Id: I269e829cf1f33e233bfcf7f95388e041180c2556
These render string arguments with potentially sensitive information
that we don't want to store in debug logs. Use the standard
'exception' field instead per T233342, letting the central
logic be in charge of creating 'exception.trace' with the normalised
trace rendering and filtering logic we have there.
Bug: T233342
Change-Id: I4620f36229fd5076b4370d20149c890030bf4c64
Any should always be the first choice. Other/None should always be
the last choice. The rest of the choices come in between and should
be sorted alphabetically.
Also capitalize the first letter of "None" for filtering logs down
to those in which no action taken. This makes the options uniform.
Bug: T255533
Change-Id: Id106bbc352531437af95a303b7dcf32e44383f95
For MySQL it was renamed Ic1252efe9f96743d9402fa31a7b2dca1f57ff6ae, but
the old index isn't being deleted, hence creating a duplicate.
Change-Id: I09b9f64759f6a897c393caa77458d63995d5713b
When using RecentChange::getQueryInfo() it should be used to instance a
RecentChange class
RCDatabaseLogEntry is only useful in context of LogFormatter
This is a breaking change for the hook variable,
but "RC entry" refers more to the RecentChange class than to the
RCDatabaseLogEntry class
Change-Id: I3af1e42594f8235be815ce38e3411c762ae01092
Additional changes:
* Removed phan-taint-check-plugin from extra, now inherited from mediawiki-phan-config.
Change-Id: Ib63be75df4bfdbd2c5b97de5f80dbec715108c01
The form is now collapsed by default, as that seems to be the most
common way to do that.
Bug: T252584
Change-Id: Ie3fa3d2858519e6bc03854a12f90f76a684e7648
The problem is explained at T250570#6068702; basically, the previous
check didn't account for DUNDEFINED nested deep inside arrays.
Bug: T250570
Change-Id: Iacee2db54ca00108de6339bb3dae70af7e2eeb56
Using var_export for better visual effect, especially for arrays.
The result from /tools is much clearer and the 'wrong syntax' message is
a bit more explicative than before.
Bug: T190653
Bug: T239972
Change-Id: I79a17305c7f19f7900f896f895e9365bb5f2fd58
- Increase batch size to 500
- Add an option to print progress markers
- Fix some bad logic which caused some JSONified data to be stored in
the text table without checking (and respecting) old_flags. This caused
some errors on the beta cluster.
Additionally, add a return typehint to AbuseFilter::loadVarDump to make
sure that errors are caught asap. Not only there's no apparent way that
loadVarDump can return an array, but most code is already using the
result as a VariableHolder, unconditionally. This is probably another
leftover from the past.
Bug: T213006
Bug: T246539
Change-Id: Iaebd28badb70d27693fa809cad4db956881e3e5e
This was used to dynamically generate *_restriction_* variables.
However, it had two big problems:
- We only have i18n for 'create', 'move', 'edit', and 'upload' (the
default value of the global); other restrictions would show missing
messages in various pages.
- We had to access the global state in various points.
This change also makes some code in AbuseFilterVariableHolder simpler,
and also allows us to make AbuseFilterTest a unit test.
Change-Id: I321ad6e07f8243200af67a581b6e485970efd3ce
Complete WikiPage/Article split and deprecate Page interface
Using actual WikiPage/Article contract
Bug: T239975
Change-Id: I343c3ca2e30715656950cab49c6470061c72b9a0
At the moment there's no validation for import data, so it's totally
possible to insert rubbish in the field, and the code will produce other
rubbish. For instance, it's not so uncommon to see lots of PHP notices
on logstash for ViewEdit code trying to access members of the imported
data as if it were an object.
Change-Id: If9d783f0f9242d3d1bc297572471e62f51ee0e40
Follow-up Ie9aae938cca06e38a7a834a3f74f3e8735ab01ee.
Some fields are actually necessary when the filter isn't saved. This
would cause PHP notices when showing the editor again.
Change-Id: I2b9e0f04b3e8ad4eea8e334e16ee422bb40f0eb5
In T43172 it was told that adding the site name could increase the risk of
attracting more spam, but I don't see how this variable could cause that.
Bug: T240948
Bug: T97933
Change-Id: I1d2aeabaf008ac06798b8d7e4af7d61ae1702776
This code was introduced with Iba59fe8d190dd338ecc8cfd682205bce33c9738b
and is unused since then. The name should highlight that those variables are not
supposed to be "static", i.e. immutable. Examples are: timestamp, spam
blacklist, site name, site language. These are not immutable, but rather
"generic", and they're known even without an ongoing action.
Also add an RC row param and update docs.
Change-Id: I402f04585e9154059fc413e527e39dcb8e6b3d7c
Follow-up Iabd0ae5b18571f8cad44ef2d86bcf2519e7f95ba.
This patch:
- Moves some save-related code to a separate method
- Reduces conditionals nesting
- Fixes an edge case where the content of the form would be
wiped in case the token didn't match.
- Adds another (basic) selenium test
- Standardizes return types
- Moves data load outside of buildFilterEditor
Change-Id: I89444b59f04c495c9ab59244151c8ed5d38cf0fe
This is another step needed to reduce the size of the gigantic
AbuseFilter and AbuseFilterHooks classes. It also makes many methods
non-static, for more testability.
Note, this layout is still not final. We should somehow merge the
functionality of VariableGenerator and AFComputedVariable, for which
I already have plans.
Change-Id: I366d598b69ad866496b7cb0059e0835c02e54041
RunVariableGenerator is for generating variables based on the current
action;
RowVariableGenerator is for RC entries;
VariableGenerator is the generic one.
This patch only moves the methods to the new classes, to keep the diff
easier to read, and facilitate conflict resolution. These classes will
then be revamped in I366d598b69ad866496b7cb0059e0835c02e54041.
Note that these classes are now namespaced.
One method, AbuseFilter::getEditVars, was renamed to
AbuseFilterVariableGenerator::generateEditVars, because it would
otherwise conflict with an incompatible method in RunVariableGenerator.
Change-Id: Iff412e5492873d4fae55402939a51609e64d55a8
This provides various shortcuts for user, target, comment, etc.,
avoiding direct access to the row, and thus a dependency on the
schema.
Change-Id: I250f94e0ac6cade33441a31ae8a27093a4d937a0
Also fix a couple of broken tests in Consequences:
- For createaccount, $user->addToDatabase must be called before
testForAccountCreation, or it will throw a CannotCreateActorException.
- In testThrottleLimit, also set wgAbuseFilterEmergencyDisableThreshold
to avoid relying on the local config.
Bug: T201193
Change-Id: If1a50b0a729e4d554485f2e2225d5877510966b6
Most of them are overwritten either in ViewEdit::loadRequest or
AbuseFilter::saveFilter. af_hit_count and af_throttled are actually
relevant for the old version, so list them explicitly. And also add
default af_group and af_global, which are later read, for import action.
Depends-On: Iabd0ae5b18571f8cad44ef2d86bcf2519e7f95ba
Change-Id: Ie9aae938cca06e38a7a834a3f74f3e8735ab01ee
Before the phan upgrade, this was silently choking on null as so falling
back to age since 1970-01-01 (~50 years); since the upgrade, the code is
breaking filters by responding with 0. The approximation of using 2008's
Wikipedia Day is less wrong and more fun (credit to Roan for making this
suggestion).
Bug: T243469
Change-Id: Ibc25ab09ecd0bf0b2292425c2768b1dc911b9974
Instead of having a single loadRequest method (which could end up
loading from the DB...), split it in a DB-only method and a request-only
one. Simplify the logic used to show the filter editor. Show the page
without changes or warnings if the user lost editing rights in the
meanwhile. Avoid two static properties, and pass them in when relevant
instead. Bonus: optimize a query to sort by afh_id instead of afh_timestamp to avoid filesort.
This will allow a subsequent patch to clean the $row object in
loadRequest.
Change-Id: Iabd0ae5b18571f8cad44ef2d86bcf2519e7f95ba
composer:
* mediawiki/mediawiki-codesniffer: 28.0.0 → 29.0.0
The following sniffs are failing and were disabled:
* MediaWiki.Commenting.FunctionComment.MissingParamTag
* MediaWiki.Commenting.FunctionComment.ParamNameNoMatch
npm:
* eslint-config-wikimedia: 0.13.1 → 0.15.0
* grunt-stylelint: 0.11.1 → 0.13.0
* stylelint-config-wikimedia: 0.6.0 → 0.8.0
Additional changes:
* Remove direct "stylelint" dependency in favor of "grunt-stylelint".
* Also sorted "composer fix" command to run phpcbf last.
* Removing manual reportUnusedDisableDirectives for eslint.
Change-Id: I8f73202db1333fbc36ccf556b3bb05b1e8c279cb
This is a preparatory step for T234427 (although not strictly related),
and in the future it will enable us not to use the DB in several tests.
Change-Id: Id069f6e74f9c4df43b3a602d4224473d5ca68ed1
-new_html: also strip the "Transclusion limit" comment if present, and
anyway take it into account (as well as a "</div>"), which right now
prevent the PP limit report from being stripped as well.
-new_text: trim extra whitespace on the right, which is created when
stripping the aforementioned comments.
Also simplify the test for getEditVars, make it not blindly copy what
AFComputedVariable does.
Extra: kill a temporary variable.
These changes are partly taken from
I96785c6c5fdf381c21d5f8930ee12e706abb7f3f.
Change-Id: I2b4c84a3d9d0d17ce229088197b75781d5181b4f
This patch is mostly replacing Revision::* constants,
Wikimedia\(restore|suppress)Warnings, and wfWikiId.
Change-Id: I13544cc3e12955a9376ccce3c120e2cee1f2ee2e
Even if the array is DUNDEFINED, we need to check the offset to ensure
that it's valid.
Bug: T237351
Change-Id: Ibfa360c4ae1d80abe14d9fdf66991b76cb5954df
For the new parser, xhgui shows that AbuseFilterParser::getVarValue is
taking up a lot of time; in turn, most of the time spent inside
getVarValue is used to log the use of deprecated variables. Hence, given
that:
- We should keep the new parser performant
- There are tons of deprecated variables out there and they likely
won't be replaced
- Having gazillions of debugLog entries doesn't help
log them only in the cached phase.
Bug: T234427
Change-Id: I2bfc692c829c3cbe889e5076f5205e2c99097087
In other View* classes, AbuseFilterView::mFilter contains the ID of a
filter, e.g. the filter being edited in ViewEdit. In ViewTestBatch,
however, it is a string containing some filter text. Hence, use a new
private property instead (without the legacy "m" prefix).
Change-Id: Ib22ce238aff4ca5ed57ba725ee9bff7f8c3d153b
Thinking about it again, all messages on ViewEdit start with
abusefilter-edit. Also add a reference to the other message to
facilitate translations.
Follow-up: I3717d06d4a757684fe6622961391ae06b5bd3c38
Bug: T235590
Change-Id: I4cbaa2e92d22296f55a4b5ef0c633fe959fe9ea3
Even if the Content objects are different, the normalized text contents
may be identical.
Also, stop misattributing null edits by adding the last revision of the
page as afl_rev_id.
Bug: T240115
Change-Id: I3fb7b36ab38ca1544889a4c233b8ffdfc6c80936
This is identical to I8a3c31e7385283d95b4712d457784016239a0b3b, except
for the array append case.
Bug: T236870
Change-Id: Iac033ba467232f6ff110d575920e968759ce0e15
Otherwise deleted and disabled filters would be mixed. Needs dependency
in core, otherwise we'd use af_deleted as secondary sort for every other
sortable field.
Bug: T191694
Depends-On: I0e695f96f18c7a9229753b1225dd473feb936a31
Change-Id: I979849e66bdcc158b7a3d0793ee3196e20db37b6
This is consistent with the "anti-DoS" measures on other API modules.
Although this may not be a serious DoS vector, it makes sense to
restrict this module. Moreover, it's also consistent with
Special:AbuseFilter/tools (which is the corresponding web interface),
which requires the same user rights.
Bug: T238451
Change-Id: Id09fd57195d71884674ac0470f137ca30c56e13c
AbuseFilterViewEdit does privilege checks based on filter ID,
and displays what is hidden under given history ID, but doesn't
make sure those two IDs actually belong to one filter.
That means user can easily change filter ID to a public
filter and view old versions of nowadays private filters.
Bug: T237887
Change-Id: Ic12790bd33982473f77551bde9599ed083a3e1f1
This will allow people to switch their filters to the new syntax. The
deprecation warning is now more exhaustive, and the info() warning is
kept to ensure that everything proceeds smoothly.
The regex v2 has also been fixed to:
- Consume all the digits/letters on the right (*)
- Have named groups
- Be created dynamically with other constants
(*) The previous version of v2 could complete the match and leave
digits/letters on the right when encountering numbers with the old
syntax, hence dropping support too early. We also cannot use a word
boundary (\b) because that would prevent matching numbers with trailing
dots (e.g. "5.").
Bug: T212730
Change-Id: Ibf6ac571f6b5c09149d69a19c38240ce6b024dff
This bumps the level to WARN, and makes it very clear that people should
fix the affected filters. It also removes the calling method, which was
mostly meant for debugging purposes, and changes the type to 'op_type'
to avoid conflicting with type:mediawiki in logstash.
Bug: T156096
Change-Id: Ie73f1604e8ed82bc2e1be9fc90fa065be37889a3
Currently, `abusefilter-edit-oldwarning` is shown to all users, but not all users are able to edit the filters, and thus the warning about editing isn't applicable to them.
Bug: T235590
Change-Id: I3717d06d4a757684fe6622961391ae06b5bd3c38
Always run the keyword/function handler, even if there are DUNDEFINED
arguments, so that the handler can perform further validation on the
input and report any error to the user. However, replace DUNDEFINED with
DNULL before running the handler, to avoid special-casing DUNDEFINED in
every handler. If any argument was a DUNDEFINED, we will return
DUNDEFINED anyway.
Also centralize the keyword handling logic to a new method, like it
happens for functions.
Bug: T234339
Change-Id: I875cb77418a39790e91fe5867c49917bfe406ed4
This allows sharing the code between cachingparser and the old parser
(for DRY-ness), and even when the old parser will be killed, having the
logic outside of the generic parse method seems saner.
This copies what I446a307e5395ea8cc8ec5ca5d5390b074bea2f24 did for
functions.
Change-Id: Ie6290243a6c78661510a9b4cb713d6e7b2778248
This emits its own error because:
1- It's clearer to understand
2- It's easier to find where we're dealing with negative offsets, if
we'll ever want to allow that.
Note that trying to use a negative index already results in a hard PHP
error being thrown.
Bug: T237219
Change-Id: Ib11eaaca5e21f740269141c75e62bac48093e8d0
As the code comment says, and as it was suggested in
Iafe54285384bc28b3e8812b495166f2682d4571c, we were validating the
provided regexp as PCRE, but using it in SQL, which only supports POSIX.
Furthermore, we won't have to worry about cross-DBMS compat anymore.
Bug: T193068
Change-Id: If6d8717795b6c1dcf619a23363eb6144902cfaed
Instead of checking if the filter is currently hidden, check the
visibility for each version and, if the user cannot see private filters,
only show the diff if none of the revision is hidden.
Also avoid showing a "diff" link if the user cannot see it.
Bug: T104807
Change-Id: Ie23e8234ae550273bf3f6f9c5ac45b7fc54eec2a
In Ib7427e15f673a575738489476e604c387f449ddd, I thought that $parameters could've only been null if $action wasn't
enabled, but actually, they're null even if the action is just not set.
Which is true for all actions when creating a new filter, and all
non-set actions when editing an existing one.
Hence, revert the part that touched ViewEdit.
Also add a selenium test to ensure that warn parameters are visible.
Bug: T236286
Change-Id: I8150baa077208eb1fc54ebc1d8415a243d0f3bd3
The method, which simply duplicates an AFPData instance, is only used
when casting types, to return a different instance when the object
already has the desired type.
However, nothing is assuming that, so we can just return the original
instance and save some time.
Bug: T234427
Change-Id: Id8067b418a00260ceead35f234e55268390699ab
I just realized that the parser is already throwing if it finds a
disabled variable. Hence, all calls to getVar with a disabled var are
from old entries and the like, and we don't care.
Bug: T234048
Change-Id: I39429d286575df91108a4119177a0d3aef181d0b
This is a micro-optimization, but IMHO it's necessary. The AF parser
code is executed for every active filter, for every
edit/move/deletion/accountcreation. In PHP, foreach is usually faster
than array_map. Especially in the case of variadic functions potentially
taking hundreds of strings, foreach will consume less time.
Bug: T234427
Change-Id: I1beedf419a6637a9a3dd668635645df950ceda21
This follows-up 8587576655 (AF) and efbfa0a727 (core). The
method was recently introduced within the 1.34 cycle but
renamed following late CR feedback.
Change-Id: I9986deb080791c6266c6c60cc91022266ad9b5e5
This also includes the filter ID. If the filter ID is not available, it
means that the user is using stuff like /tools, and they'll immediately
see the error.
Bug: T234048
Change-Id: I44a37d98c80df910b0c466fbd464e69042770c0c
$summary and $user are always guaranteed to be passed, and $user is
guaranteed to be a User object. Hence, update the hook handler to
reflect that.
Change-Id: I3a7fcb074b460b77210de5a6bad43f500aff3249
Deleted/suppressed usernames and summaries leak through AbuseLog.
Temporarily hide all non-public revision from AbuseLog, until we can
properly fix the issue.
Bug: T224203
Change-Id: If3d3256404d0f3dbde171831937d1a816b3e2734
This allows us to:
- Defer handling of the block to the main module
- Choose the right message depending on the block type
- Avoid directly using the apierror-blocked message, which could change
in the future.
Change-Id: If2e32bd2ccf5e314aa51203afd1522b8481377e0
Follows-up: I35f2c6e701a24dccb6e26e3f3c578fd44f68127d
This is similar to the old parser: when discarding a node, actually
evaluate it if short-circuit is not allowed.
Add a whole lot of tests for all possible exceptions.
Move the logic to extract a message from an AFPUserVisibleException away
from the parser, to keep unit tests working.
Bug: T232498
Change-Id: I31ee4e255c6a87dd693b9bcd582539fdf57acd45
This implements T230982#5475400, and it should speed up the CachingParser by roughly 40%.
Bug: T230982
Change-Id: I803cc58637d50eb90e57decf243f5ca78075d63d
Setting 'apiHookResult' results in a "successful" response; if we want
to report an error, we need to use ApiMessage. We already were doing
this for action=upload. Now our action=edit API responses will be
consistent with MediaWiki and other extensions, and will be able to
take advantage of errorformat=html.
Since this breaks compatibility anyway, also remove some redundant
backwards-compatibility values from the output.
To avoid user interface regressions in VisualEditor, the changes
I3b9c4fef (in VE) and I106dbd3c (in MediaWiki) should be merged first.
Before:
{
"edit": {
"code": "abusefilter-disallowed",
"message": {
"key": "abusefilter-disallowed",
"params": [ ... ]
},
"abusefilter": { ... },
"info": "Hit AbuseFilter: Test filter disallow",
"warning": "This action has been automatically identified ...",
"result": "Failure"
}
}
After:
{
"errors": [
{
"code": "abusefilter-disallowed",
"data": {
"abusefilter": { ... },
},
"module": "edit",
"*": "This action has been automatically identified ..."
}
],
"*": "See http://localhost:3080/w/api.php for API usage. ..."
}
For comparison, a 'readonly' error:
{
"errors": [
{
"code": "readonly",
"data": {
"readonlyreason": "foo bar"
},
"module": "main",
"*": "The wiki is currently in read-only mode."
}
],
"*": "See http://localhost:3080/w/api.php for API usage. ..."
}
Bug: T229539
Depends-On: I106dbd3cbdbf7082b1d1f1c1106ece6b19c22a86
Depends-On: I3b9c4fefc0869ef7999c21cef754434febd852ec
Change-Id: I5424de387cbbcc9c85026b8cfeaf01635eee34a0
It was executed on WMF wikis, and since they were the only affected
wikis we can remove the script.
Also remove a temporary back-compat check in the log formatter.
Bug: T231131
Change-Id: I534acd9c86894eb1bdd96331e9fa85afc7502f88
SpecialPage::setHeaders already handles page title, robot policy and
articleRelated. Moreover, avoid having different messages for the H1
title on the special page and the description shown elsewhere, just like
the base SpecialPage class suggests doing. The deleted messages have
been moved to the default message used by SpecialPage::getDescription.
Change-Id: Iab6beaf64b142e30469afd798c569ef40182153e
This is because there are many filters using this feature. Moreover, it
could make it a little easier to add new arguments, just like dangling
commas in PHP arrays do.
Also re-align the CachingParser code of doLevelFunctions to the one in
the old Parser.
Bug: T153251
Change-Id: Ie4325159f47310788da57415a5e36e62aa4efad0
This will help mitigating problems like T230256 by enforcing that the
requested variables must exist. For now, it will only log bad usages,
thus providing a way to identify affected filters and fix them.
Bug: T230256
Change-Id: I7a61916576e444a56f0e07da7b6e5033346226bd
Using `new LanguageEn()` involved a global, so use a MockObject instead.
Also fix LoggerFactory usage in Tokenizer to use DI instead.
Change-Id: I94d03f9459ab6444e239386eb96a0c2434bfe3dc
PHP7 throws an Error, not a BadMethodCallException. We don't want to
clog the logs with fatals, now that PHP7 is closer.
Bug: T187153
Change-Id: I5a9e581ee0418ae41dd911de02a64d18e4670cd4
We didn't check if the provided ID was valid. While editing an existing
filter (or creating a new one), we check the ID in SpecialAbuseFilter,
so it's guaranteed to get an integer in ViewEdit, and the case of a
non-existing filter is handled later, in buildFilterEditor.
But for links like Special:AbuseFilter/history/foobarbaz/item/1 (where
"foobarbaz" should be the filter ID), no validation was performed. This
caused a useless query to be carried out on the abuse_filter_history table (which would likely return false), then accessing properties of a non-object ('$row->afh_id'), and we ended up showing filter 1. This was spotted because we actually got notices in production.
Bug: T231632
Change-Id: I6436c7d2df8c1f0fc971f4a4079dac9118aa8209
There's not need to use the variableholder in this hacky (albeit common)
whay, since the row already holds the action.
Note, this doesn't guarantee that the next two lines won't fail - I'd
need to see the actual var dump (T231542#5450720) to determine exactly
why this is failing.
Bug: T231542
Change-Id: I2112b046d00e06b575d15ab3d7da57484fd9cbbd
IMHO these can be considered unit tests; they were already fast, but now
they're executed in an instant.
This requires several changes: 1 - delay retrieving messages in
AFPUserVisibleException, to avoid having to deal with i18n whenever we
want to test exceptions; 2 - Use some DI for Parser and Tokenizer.
Equivset-dependend tests are also moved to a new class, thus helping to
fix the AF part of T189560.
Change-Id: If4585bf9bb696857005cf40a0d6985c36ac7e7a8
This dependency is wrong, and removing it will also allow creating an
edit API.
Bug: T213037
Depends-On: Id8412e2b8a4e873fd4821ecc1a3c95710be9a870
Change-Id: If8e745a3227cea5093ea3fd8f5b201adedaba3ae
Some of these are transformed into real unit tests, while the
AbuseFilterSaveTest class is refactored to avoid using the DB and to use
a lot more of mocks and DI.
Depends-On: I22743557e162fd23b3b4e52951a649d8c21109c8
Change-Id: Id8412e2b8a4e873fd4821ecc1a3c95710be9a870
There are lots of calls to $user->isAllowed which could be simplified
using available accessors like canEdit(). So simplify those calls and
avoid duplication.
Note that using canEdit also fixes a bug which affected blocked users:
we used to show e.g. the import link, and not to display as disabled
several text fields, while blocked users cannot actually edit filters.
Depends-On: I22743557e162fd23b3b4e52951a649d8c21109c8
Change-Id: I62779e940949ef49018a9c6d901bb6e10aa81da8
There are lots of cases where we can inject a User object without
additional efforts. Now $wgUser is only used inside AFComputedVariable,
which is a little bit harder to handle because some instances of that
class are serialized in the DB, and thus we cannot easily change the
constructor until T213006 is resolved.
This partly copies what Ia474f02dfeee8c7d067ee7e555c08cbfef08f6a6 tried
to do, but adopting a different approach for various can*() methods:
they're now static methods in the AbuseFilter class, so future callers
don't need to instantiate an AbuseFilterView class. This also allows to
re-use those methods in an API module for editing filters (T213037).
Bug: T213037
Bug: T159299
Change-Id: I22743557e162fd23b3b4e52951a649d8c21109c8
While this is not as important as throwing for too few parameters, IMHO
it's still important to fail in this case. Mostly because if a function
receives too many parameters, chances are that who wrote the filter
didn't do that intendedly, and thus there may be a hidden bug.
Bonus: fix a few docblocks.
Bug: T230803
Change-Id: Iac2931f17b50ace8c8f4c2faa44b3f54ca134c54
For now it will only report successful parse. Next step is formally
deprecating the old one (escalated to warning), then removing it in
favour of the new one (in another MW version).
Bug: T212730
Change-Id: I5dd11fd67d8e57d1d0c52ddfa026920ebfc5ee13
This allows a little bit more of abstraction: we can store other data in the
tree, without having to store it in a specific node (e.g. the variables map,
which is still unused). It also adds a few typehints, and specializes
the return value of eval'ing the AST: previously, it was the one of
evalNode, which wasn't guaranteed to be an AFPData. Now we have this
guarantee. Last but not least, we can now measure runtime metrics for
evalTree, which doesn't recurse.
Bonus: fix a check in the old parser, which used the wrong variable when
reporting outofbounds errors.
Change-Id: Iff806793b1d968e9bb6220f1459f3d0ac587c7da
And fix a couple of minor bugs.
Bug: T156096
Depends-On: I3b85087677607573f4fa68681735dc35348dcd87
Change-Id: Ia4c713a1d45827f6a8bc5566a8d8835c49f8108a
After having removed the export link in
I72f46247f4323fb5bfe7fa74f332076dbd346187, we don't have any tool to
show for new filters. So avoid outputting an empty section.
Change-Id: Ia07bccdbadb7b874397135bc3f7468d6e0b9eb13
Ensure that the variable isn't set before marking it as DUNDEFINED:
that's only for when we cannot use a default, but if the variable is set
we already have one. Most notably, this fixes conditionals handling: right
now, if you have a conditional with an assignment in both
branches, the variable will be undefined. That's obviously wrong, so
it's fixed in this patch.
Plus: catch only AFPExceptions in a test to avoid unintentionally
catching the assert exception; simplify some assignments using wfSetVar.
Depends-On: I446a307e5395ea8cc8ec5ca5d5390b074bea2f24
Change-Id: I8e7f7710b8cb37ada8531b631456a3ce7b27ee45
This patch includes various fixes to how func arguments are handled in
CachingParser:
- Add a comment about a future improvement of checkSyntax, which we
could limit to try building the AST.
- Having enough args for each function is now also checked when
building the AST. This allows implementing the previous point without
stopping to report notenoughargs at syntaxcheck-time (otherwise it'd be
a runtime error). And it also ensure that we check for the params count
inside skipped branches, e.g. inside if/else: these were already only
discovered at runtime in CachingParser. The old parser is not affected
by this change, because when checking syntax it will always execute
all branches, and at runtime it will skip braces altogether.
- Fix arg count for CachingParser, which previously added a bogus param
in case of a function called without parameters. This was fixed for
the other parser in I484fe2994292970276150d2e417801453339e540, and I
just ported the updated fix. Also note that the CachingParser was
already failing for e.g. `count()`, but instead of complaining about
missing arguments, it failed hard when trying to pass NULL to
evalNode.
- Fixed some tests not to use setExpectedException, which caused the
previous point to remain unnoticed: calling that method prevents the
loop from continuing, and thus only the AbuseFilterParser part was
being executed. The new implementation checks the exception ID and is
thus more future-proof if the i18n message changes.
- Fixed some function names in error reporting for the old parser.
- The arg count is now checked outside of the function handlers, thus
it's no more necessary to call checkEnoughArguments at the beginning
of each handler. This also produces clearer error messages in case of
aliases (e.g. set/set_var).
- Check the args count even if some of the args are DUNDEFINED. This is
much easier now that the check is outside of the handler. This will
make syntax check fail for e.g. `contains_any(added_lines)`.
Bug: T156095
Change-Id: I446a307e5395ea8cc8ec5ca5d5390b074bea2f24
This was broken in I34c040dbeb3ab01158fb3db22496def6ccaf72d9. I thought
the members of that object were always arrays, but I was wrong.
Plus typehint a few array parameters and make a couple of methods
private since they're only used in this class.
Bug: T230639
Change-Id: I0c51359769c4b3054f95755a96e7e0a2d8e5bf15
Now it's always wider, and so is the "notes" field. Moreover, the
fallback textarea has the exact same size. Plus removed a parameter
which only made it hard to write a CSS rule for the textarea. Since the
textarea is generated by the same code, and we're always using it for
the same thing (filter syntax, regardless of the final goal), make it
always use the same name.
Bug: T230591
Change-Id: Ibb308e80d954c0e81aa09249c38c39572f157948
Problems fixed:
- Don't hardcode duration in the message
- Move duration to a constant
- Fix wrong parameter order for AbuseFilter::blockAutopromote
- Log a warning if we cannot block autopromotion
- Remove the $performer parameter, as it should only and always be the
filter user.
Bug: T230296
Change-Id: Ice9e4b21033c430cf1fd34182c63ca64ad2f5d3e
If $parser->parse returns a falsey value (=null), that's because the
filter doesn't have any statement. But that's not a valid reason not to
cache the filter. Hence, return whatever parse() is returning inside the
callback, so that the result is always cached.
Change-Id: Ib6b0e72d882dc484456a3be6bbc74da36ef48bf7
The regression itself was fixed in
I980aec3481a52ecc35f1811a366014a5581a7cdb, so this patch only adds a
test for it.
Also remove a comment about CachingParser failures: we don't want to
encourage people to remove it from tests anymore.
Bug: T152281
Change-Id: I3ad49050ea49bf45d3226878e091da3c8dbefdb1
Just like we do for functions, it doesn't really make sense to have
keywords separately, in AFPData.
Change-Id: I208a9b1ce2bd12038e9fbcc515c48d604ec80eb8
This patch keeps the current behaviour for everything (since DUNDEFINED
was always casted to boolean false), but handles the cast at a higher
level instead of relying on what AFPData::castTypes will do. This way
it's easier to spot places where we may get DUNDEFINED, and decide how
to handle them one by one.
Change-Id: I1070e15ea03c7dd4a4231b87afbc42240a558581
Instead of returning a successful message, return null and log a
warning. Also, make autopromoteBlockKey public + internal and use it
from Hooks instead of duplicating the logic.
Follow-up: I03feb05218789a3b73a31c9a94216daafcb7c145
Change-Id: I8ce96d1bd0239003f8ee6a45f412b9502d542a18
Follow-up of Iaca492371f48fecf543268c179a651841ed12c3f. This patch adds
the new module, plus some technical changes to private details-related
methods and globals.
Bug: T210329
Depends-On: I613dbadb8f75c8c4116a362607563a436a73d321
Change-Id: I3c45b74c36c191083df184ed57416067a75f6591
To keep a clear distinction between "private details" (i.e. user's ip)
and "private filters" (i.e. not publicly viewable). This patch renames
rights, i18n keys and methods names.
The patch for renaming globals and rights in WMF config is
I7e6b3d4453403edb6aa602587374b4ff5b6d625f.
Bug: T211004
Change-Id: I613dbadb8f75c8c4116a362607563a436a73d321
This allows us to extract yet another static method from the AbuseFilter
class. This class should be expanded in the future, and an example use
case could be Ia5fd4f0b35fcabf045a7b49fa40fa85b72c92544.
Depends-On: I7c0170167b508132cd16e566c654a6c98dd683e9
Change-Id: I1bb45e47c3b42c01388b99778ce833e4e44419e1
This is more complicated than the := operator, because the var name
could be a complicated expression, and we have to handle a function
call. This patch only covers the case where the variable name is a
literal, which is enough for WMF production.
Bug: T214674
Change-Id: I6c0f8e95663919a0235b5ccf0c88ad0a539315a7
As for all mostly unused consequences, blockautopromote has a couple of
major problems: first, it blocked the status for a random time between 3
and 7 days, which to me makes no sense at all (is it some sort of
casino?), and this patch fixes it to 5 days. Second, nothing was logged,
not the blocking nor the unblocking. Here I'm adding a LogHandler for
two new sub-actions of 'rights' to keep track of both action.
Bug: T49412
Change-Id: If48a48f5b8baaf9e77c0826466f5d03bb7f691d0
Follow-up I7831f3ed9f7c0656e0e8f77ded049c20eca682ba, really avoid the leak. My addition was pointless because we need DUNDEFINED, not DEMPTY, and I spent way too much time trying to understand what was still wrong.
Still have to get used to these new names...
Change-Id: I332967f6fb00b67fd355547b19638c95ffa5bba7
Parsing wikitext and retrieving its links are operations which we share
with the edit, so that if a filter does that, it won't be done later
upon saving.
Thus, add a static variable to subtract such time and avoid to erroneously log as slow any filter using those variables.
Bug: T219092
Depends-On: I24fbd41ac188a9cf6a7d3ca33dce349aedc9faa6
Change-Id: I7c0170167b508132cd16e566c654a6c98dd683e9
As shown in the coverage reports [0], some empty operand logging lines are covered, but no test should have empty operands. I see one of the cause is skipOverBraces keeping $result as is, even if DEMPTY, so turn it into a DUNDEFINED.
[0] - https://doc.wikimedia.org/cover-extensions/AbuseFilter/includes/parser/AbuseFilterParser.php.html
Change-Id: I7831f3ed9f7c0656e0e8f77ded049c20eca682ba
Instead of scattering the process all over the code (and doing it
together with checking if the key already exists).
Wrap the logic in new methods for better readability.
Depends-On: Ib12e072a245fcad93c6c6bd452041f3441f68bb7
Change-Id: I24fbd41ac188a9cf6a7d3ca33dce349aedc9faa6
The last step of the profiling overhaul. See T53294 for the original description by Dragons flight.
Note: Here I'm adding a FixMe for a problem which already exists in the code
and the child patch will fix it.
Bug: T53294
Depends-On: I2d8c8f8278073a9420e3eb373fb89a655925618a
Change-Id: Ib12e072a245fcad93c6c6bd452041f3441f68bb7
This adds a new get(Type)Vars method for every action type. The goal is
to 1-have shorter methods, which is always good; 2-try to make this code
a bit more testable.
I left as a todo moving all these methods to a separate class, the idea
being to make them non-static and thus easier to be tested.
Depends-On: I2eab2e50356eeb5224446ee2d0df9c787ae95b80
Change-Id: I6de2dd27a8f972b3f74c730a1516639f8c622166
I5ec4ab44c4e88aaf18c0d7b73355d27050beeda7 almost fixed this bug, but we
also have to make it possible to access builtin variables as arrays.
This will only make sense for a few variables (e.g. added_lines and
removed_lines), but I don't think we should validate it when checking
syntax.
Bug: T198531
Change-Id: I417e1b8d4802bbfccd091ce5c7617659cfd1e4ea
