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
The "filter" fields can also accept a list of filters, and also global filters, so make it clear in the UI and in messages.
Change-Id: Ib258716d8e6792fd496938ebb4e8a2565d6370b7
In I1dc3be6da1cc9e03bc47e8f8c867089ad0100f6f we added fields to the array.
Update the version to avoid PHP errors while upgrading the wikis, for edits
stashed before the upgrade, and saved afterwards.
Change-Id: I5489b556b1b0e9cb2af862dbfa0621909a5e355d
Start deprecating "empty" logic operators, and now that we have DEMPTY, simplify handling of empty function arguments introduced in Ica3e49f5b00595a95513d9683732e490aa7aae17.
Bug: T156096
Change-Id: Ied6b385e8690b6cc6e69afcf614389f737ab95bd
As data could be "old" and it may have no meaning.
Also remove a superfluous isset(), as $row->af_hidden is always set.
Depends-On: I2d8c8f8278073a9420e3eb373fb89a655925618a
Change-Id: I072363706c61f272c4c3691de4078e2a19148424
Instead of seconds, and round the average condition at 1dp instead of 0.
Split from child patch by Dragons flight.
Depends-On: I2d8c8f8278073a9420e3eb373fb89a655925618a
Change-Id: I339aed5f8c1d49714e7927ce49286f9ce6c839f5
They're currently stored separately, so move matches count together with
other per-filter data to keep it consistent. This also removes a
parameter from filterMatchesKey, as it's not needed anymore.
Split from child patch by Dragons flight.
Bug: T53294
Depends-On: I8f47beb73cfc1b63c4b3c809fc6d65a1e66ee334
Change-Id: I2d8c8f8278073a9420e3eb373fb89a655925618a
Instead of having three keys, one for total actions, one for time and
one for conditions. This has several benefits: first, it avoids race
conditions which could happen having different keys. Second, it's much
more performant. Third, the code is also clearer to understand,
and more uniform with the one for global stats.
Split from child patch by Dragons flight.
Bug: T53294
Depends-On: I1dc3be6da1cc9e03bc47e8f8c867089ad0100f6f
Change-Id: I8f47beb73cfc1b63c4b3c809fc6d65a1e66ee334
This is the proper solution to replace
Ia8e38ba25d1989fe71714d2b76891c4587921466, using a class member and an
additional method. Plus, change checkFilter not to accept a prefix, but a boolean indicating if the filter is global (as that's how it's used currently).
This change also fixes an issue which caused profiling data for local
filters to be mixed with profiling data for global filters with the same
ID.
Depends-On: Iafc142d2e5ba7aa0fb0d3265fa05cace27679738
Change-Id: I1dc3be6da1cc9e03bc47e8f8c867089ad0100f6f
This patch includes:
* Making it possible to access offsets of a DNONE (returning a DNONE)
* Initializing user-defined variables as DNONE inside short-circuited branches
* Make DNONE propagate with other operators
* Make DNONE count as false for logic operators
* Remove a now-outaded bit in doLevelAtom. In case of shortcircuit,
$result is now DNONE instead of DNULL, and thus it's possible to
access offsets of it. Performance++!
* Don't allow modifying or adding an element of a DNONE as if it were an
array (to avoid inconsistencies)
This re-applies Id85c673337fa90a3782fd22eb9690cd996967111 with several fixes.
NOTE: Haven't tested locally, although I'm pretty confident thanks to
the amount of tests added.
Bug: T214674
Bug: T228677
Change-Id: I5ec4ab44c4e88aaf18c0d7b73355d27050beeda7
Instead of duplicating code in several files.
Depends-On: I2eab2e50356eeb5224446ee2d0df9c787ae95b80
Change-Id: Iafc142d2e5ba7aa0fb0d3265fa05cace27679738
Currently we strongly abuse (pardon the pun) the AbuseFilter class: its
purpose should be to hold static functions intended as generic utility
functions (e.g. to format messages, determine whether a filter is global
etc.), but we actually use it for all methods related to running filters.
This patch creates a new class, AbuseFilterRunner, containing all such
methods, which have been made non-static. This leads to several
improvements (also for related methods and the parser), and opens the
way to further improve the code.
Aside from making the code prettier, less global and easier to test,
this patch could also produce a performance improvement, although I
don't have tools to measure that.
Also note that many public methods have been removed, and almost any of
them has been made protected; a couple of them (the ones used from outside)
are left for back-compat, and will be removed in the future.
Change-Id: I2eab2e50356eeb5224446ee2d0df9c787ae95b80
This should allow more flexibility when checking syntax, and a saner
behaviour overall.
Aside from not throwing exception in certain cases, the results should
be almost equal to the ones you would get without this patch. However,
there are still a few things to improve (which for convenience I wrote
inside the parser test) and many to test.
Bug: T204654
Depends-On: I69bfec45c76509fb1112641393f78e8d8834adcd
Change-Id: I5a14d4b2bc3ffd9caaaa095f16f36b9b6009db05
Aside from the 14 thingy reported in the task, this syntax is awful! The
fix to the regex should only be intended as a temporary stopgap. A
proper fix would be to introduce a new syntax, like for instance the one
used in PHP.
Bug: T212726
Change-Id: Idc37a17ce539e6c63d67fc07d47d812569debe0e
Follow-up of 0b925da36e, somehow I forgot
to add the removal code for MySQL and SQLite to the Hooks.
Bug: T214592
Change-Id: If0d1d5430573273784ff6f6e338b0c2199f6d7bb
$data and $type are meant to be read-only and should have getter
functions, but as usual they're just public. Add getter methods, a
comment with a @private annotation and remove usages in our codebase.
Change-Id: I5e51efc9f982a4e340b48d20cb1b38a75bb10021
This property is meant to be private, since it has all kinds of
getters/setters, aside from one which is introduced in this patch.
Change-Id: I217b1e22cabd3c0468c84b1d6a69a6ed3c6fa8e6
This changes the buildFilterEditor function to be protected and to
behave consistently: so, instead of adding stuff to OutputPage inside it
and also returning other stuff to be added by the caller, the function
now adds everything itself.
Also, the message "you're editing an old version of the filter" is now
shown only if the user can see the filter.
Change-Id: I1f40af41c5de0f63aa6210a261928892da0b3f69
Now we have the key old-text for the old_wikitext variable, and the key
old-text-stripped for old_text. This can be confusing (see I61b2d252333ca634eae560d824f740f0f947b3d3), so use i18n keys more similar to the variable name.
NOTE: the keys will have to be changed on translatewiki if we want to avoid
confusing people.
Change-Id: Ie612350642ac4afc76f18639d988e72b4016b1e2
Split from I5a14d4b2bc3ffd9caaaa095f16f36b9b6009db05.
This adds a new data type to use for empty AFPDatas. Using NULL for that
makes it impossible to distinguish cases where we really got a null
value, and cases where there was nothing to parse.
For now, DNONE is the same as DNULL, but I've explicited DNULL where
necessary. A subsequent patch will make proper use of DNONE.
Bug: T156096
Change-Id: I69bfec45c76509fb1112641393f78e8d8834adcd
The current form is awkward. They're all like
[ actionname => [ 'action' => actionname, 'parameters' => params ] ]
This is greatly confusing since adds a nesting level, and just
duplicates the actionname information (also, we actually never retrieve
it from the internal array). Instead, change all of them to be
[ actionname => params ]
which is a lot shorter and clearer (and easier to handle).
A similar case is handled in I8134ecc41fbecdbed99faf406e9e3ca91b6123b9
(see PS 8..10).
Change-Id: I34c040dbeb3ab01158fb3db22496def6ccaf72d9
Use a more explicit TTL_WEEK, and add the version to avoid breaking the
world if we change the format.
Bug: T227299
Change-Id: I22705496ed8541c3dd9b643d78dff8886f4ff070
The 'AbuseFilter-filterAction' hook is deprecated in favour of a new
'AbuseFilterAlterVariables' hook, which provides a User object and has a better name, since it reflects what it should be used for, and doesn't include the name of a function which will be removed. The hook will be hard deprecated in a subsequent patch, to avoid test failures.
Depends-On: Ic0ecc8746e2883c746bef815a0fee4131f1a0646
Change-Id: I212b1e09e9c05d487d96b2f4c28f2a613e6ff3cf
This is somehow a follow-up of
Ieb04f019453033c275e211cfc9fd68d5d7c392ef. A new method is introduced to
cache a filter, which checks that all fields are there.
Depends-On: I7c1ea17adf7f42cf9260d416906bfbf3b8a20688
Change-Id: Ic0ecc8746e2883c746bef815a0fee4131f1a0646
As explained on phabricator, they don't work with shortcircuit, so they
already fail for all filters using them. Plus IMHO it's an unnecessary
deviation from PHP's behaviour, given that this syntax doesn't do what
users may expect.
Bug: T218906
Change-Id: If9e7545e14044c8dc3b4163bb6fca8ab0683b9fa
To make the switch to afl_filter_id and afl_global easier.
Bug: T227095
Depends-On: Ie550889495232b534c0f9aec31039cf21b2135b1
Change-Id: If557bad8f5c1a6d15e3556e4bfbd0330d7d49c59
Instead of relying on static methods and members in the AbuseFilter
class, move everything related to conditions inside the Parser, as the
amount of used conditions is something pertaining a single
AbuseFilter(Caching)Parser instance.
This change requires changing some signatures and adding parameters,
but will make introducing the new AbuseFilterRunner class easier (and
that will clean signatures, too).
Depends-On: I5b29ff556eca45fe59d15e2e3df4d06f1f6b3934
Change-Id: I7c1ea17adf7f42cf9260d416906bfbf3b8a20688
This is the second part of removing meta-variables. To achieve this, a
static property is added and another one removed.
Depends-On: I7f60df24dc8e706af289ebbbde7536c0baf8d5c3
Change-Id: I5b29ff556eca45fe59d15e2e3df4d06f1f6b3934
Fix the typehint, and use selectField instead of selectRow.
Follow-up of Ie550889495232b534c0f9aec31039cf21b2135b1 suggested by
Krinkle.
Change-Id: I7e74b7b484dfa487db96598ef7aef4895d7bf275
Now it returns an array with a bit more info, and has a different name
to reflect the fact that its input is now split in two parts. Plus, make
it throw whenever it gets an unexpected input, and add a bunch of test
cases for it.
Depends-On: Ib5fdeb75c1324f672b4ded39681f006fde34b4d1
Change-Id: Ie550889495232b534c0f9aec31039cf21b2135b1
Partial revert of I4dd81a723e2bdb828b90594ad66a3918d8ec5b6c.
Thinking again of it, I think it's not worth it to have this data over
the network. Plus, given that it's not-that-slow to be computed, I think
there can only be a performance gain in using APC (as opposed to e.g.
memcached/redis) for 99.9% of the filters.
Change-Id: I8c6a4a95ec12c18ede8e6419540f7a2ac943457c
Avoid all those data types (i.e. use null instead of false), use camelcase, make them private. Also, remove some logic to handle $lastEditPage being Article, as it can only be WikiPage.
Depends-On: I5a9db6e7c4356c9662a0b0a51e66252555b3d998
Depends-On: I359a618ffc4e45ce1fb70f2d1aa99a6668609e36
Change-Id: I7f60df24dc8e706af289ebbbde7536c0baf8d5c3
As all title variables would be null, and the result pretty meaningless.
NOTE: Please vote V+2 and submit manually. I359a618ffc4e45ce1fb70f2d
should then be +2ed right after that. This way, there is no need to create
two more patches just for a handful of tests being broken for a minute.
Bug: T144265
Bug: T219030
Depends-On: If6b91711534c0d60e1aa27bd5748c3023e29f376
Change-Id: I5a9db6e7c4356c9662a0b0a51e66252555b3d998
This is done for 3 reasons: 1-the code should hopefully be clearer;
2-FWIW, switch's are a little bit faster than elseifs (roughly 15%); 3-to
fix a bug with coverage driver which says those lines are not covered.
3 is a follow-up of I997576141943959d4602a9f839311108928ec766.
Change-Id: I2d69e421e384cb74a799c5c5f77d041a7e02d4c8
Added cachingParser back to *all* the parser tests, fixed a couple of
differences with the normal parser, and added a couple of tests so that
any cachingParser-related file has 100% coverage. Also move the remaining
get_matches tests inside parserTests, and specify the parser used in case of failure.
This also adds a new base class for parser-related tests with a couple
of util methods.
Bug: T201193
Change-Id: I980aec3481a52ecc35f1811a366014a5581a7cdb
Another crucial part to have covered. Also clarify that
AbuseFilterCentralDB can be of the form "dbname-prefix".
Remove a filter used for profiling and replace it with a global one:
we're still fine, and the list is kept shorter.
Bug: T201193
Depends-On: I5ee7ba44a6cd82a5ddb24fb4127af04d96e647f4
Change-Id: If6b91711534c0d60e1aa27bd5748c3023e29f376
Yet another important part to have covered. While for normal edits it
already works, for stashed ones it doesn't. That's why we need the patch
for checkAllFilters. Since for stashed edits profiling stats are all
zeros, this may explain T201334.
Changed the timestamp variable to use wfTimestamp instead of time() so
that we can fake it inside unit tests.
In a subsequent patch we should add average runtime conditions to tests
(really tricky).
Bug: T201193
Depends-On: Ib17821240b25c972a187e6b5eae42c5ada6c65e7
Change-Id: I5ee7ba44a6cd82a5ddb24fb4127af04d96e647f4
First step before removing this weird syntax. I'd love to add a unit
test for params count, but I couldn't find a way, since doLevelFunction
is protected, relies on class members, and the args count is local.
Bug: T153251
Change-Id: Ica3e49f5b00595a95513d9683732e490aa7aae17
Check that the provided param is not empty, as otherwise
Database::makeList will throw and the exception will bubble up to the
user.
Bug: T222531
Change-Id: Icf5db25037a0d0a7b4076f21e7f1c9a6ee1d5a87
[Also make use of the list() feature in one case I forgot before in
If2b6c95.]
-> Changed to use direct array access by Daimona per inline comment.
Change-Id: I708dff30b6e00ccab3257b2e6fa5995eb9e30e0f
This is the same as line 224, and I forgot to include this code path in
the same patch.
Bug: T187153
Change-Id: I28074680760a7070eb423b5eada1e35f829ed10a
This is fixing potential bugs where invalid strings with more than one
comma have silently been accepted.
Change-Id: Ib1e7d0c99973f243ef6faad6389bab688187c1cf
I find it obvious that a file called "AbuseFilterTokenizerTest" is a
"test for the AbuseFilterTokenizer class". A comment that is just
repeating this information is typicalls not helpful, but distracting
and a potential source of mistakes, e.g. when stuff is copy-pasted,
but the comment not adjusted.
Change-Id: I1d4cc06e9e5631955ff73bf675090cf9c33c9390
Split a method, use WAN cache so that we're enabled to use
getWithSetCallback, pass the "version" option there and adapt the test
to it.
Follow-up of I9b3bc36b552901bc6ca7609ee51e80be2979a9c4
Change-Id: I4dd81a723e2bdb828b90594ad66a3918d8ec5b6c
Caching the result of the tokenization is pretty important
performance-wise, so this test ensures that caching works as expected.
I have also extracted the method used to generate the cache key for
easier testing, and moved the cache instance to a class member because
otherwise that piece of code can't be tested...
Bug: T201193
Change-Id: I9b3bc36b552901bc6ca7609ee51e80be2979a9c4
These are the ones which other tests don't cover, mostly because no
filter syntax can trigger those cases. This patch should bring coverage
for AFPData to 100%.
Bug: T201193
Change-Id: I997576141943959d4602a9f839311108928ec766
If the number is int there's not need to intval it, and if it's float
there's no need to floatval... Just use it to determine the internal
data type, like it happens for sum and sub.
Change-Id: Ie00c4bb4e67ad1eface0cea3eb42d04aa5fb39cc
Follow-up of Ic30883f7d261d974a2be46308d023e2714119e95, with two files
that I forgot to git-add and a repositioning of comments to avoid the
last bracket to be reported as uncovered.
Bug: T201193
Change-Id: I6bf7e5892a0f49f6a138792f0aedf230a70c18a8
This patch mostly adds coverageIgnore comments for intendedly
unreachable code etc. Some of them could be made testable by adding a new
filter function (e.g. array cast), but this patch is meant to be
comment-only (aside from the parser test).
Ignoring coverage for these lines makes some methods reach 100%
coverage, which in turn makes it easier to look at the coverage chart
and identify at a glance which parts of the code *really* need to be
covered.
Bug: T201193
Change-Id: Ic30883f7d261d974a2be46308d023e2714119e95
These are specific tests for storeVarDump and loadVarDump, both alone
and in the context of running filters.
Also, include disabled variables in the VariableHolder object if they're
saved in the DB.
Bug: T201193
Depends-On: Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb
Change-Id: I5e35d773904a62105767ce6d7d962ab5525c2d12
The root cause is database rows holding a serialized revision object
(awful, right?), and to properly fix it we need a maintenance script,
still WIP (T213006).
This temp fix is to avoid flooding the exception channel.
Bug: T187153
Change-Id: I062934091fbd6213cf9bc10e8ad6864ce6a58254
And right when the throttle script seemed complete... Here is another
function! So, this change splits the logic in new functions called
sequentially, and the only actual change is adding the beautifyHistory
function. Its purpose is to search ANY row in abuse_filter_history with
empty/missing parameters and normalize it. More specifically, missing
period and count are inserted as 0, and for missing groups we add
"none", used by a newly introduced message. This way, messages shown on
Special:AbuseFilter/history will be clearer and won't have gaps.
Bug:T209565
Bug:T215787
Change-Id: I38395f4df9d83badfd26cdf584ffba743b6417a9
Try to get the groups from the var first, and compute them if they don't
exist. Use getEffectiveGroups instead of getGroups as it's done when
setting the lazy var loader. Avoid a pointless array_intersect within an
array_diff. Remove Phan @suppress and add docblock to make it pass.
Change-Id: I49ec6a1264b767cefea55df66ef3b02d4f443b57
Having a single return statement inside a function isn't always the
best, but having 5 is probably worse. This patch changes three long
if-return/if-return/... to a single if/elseif + return.
Change-Id: I5f4603627c61cf1b93859fe6bcd952eac8e82359
We JOIN integer and text, so Postgres would always fail on these. As
mentioned in the task description, this is only a temporary solution
(although a clean and durable one), while the long-term one is
I7460a2d63f60c2933b36f8383a8abdbba8649e12.
Bug: T42757
Change-Id: Ifddd0bca1e8eaa7c70511fb0d0588457b4fd0669
The reasoning is similar to the one of the parent patch (Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb). Plus, it records runtime metrics on action different than edits, as there's no reason not to do it.
No performance issues in production.
Bug: T191039
Depends-On: Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb
Change-Id: Ib1112e2fefd0631550d386ba87e5f87db84c3036
This variable was introduced to selectively enable profiling because
stats recording was bad for performance. Nowadays, stats are recorded in
a deferredupdate and don't harm performance anymore. Thus, this variable
can be removed and profiling be enabled by default.
Bug: T191039
Depends-On: Ib5fdeb75c1324f672b4ded39681f006fde34b4d1
Change-Id: Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb
The method is used to make afl_deleted = null treated as afl_deleted =
0. Digging into code history, I found that it's in place because:
*In rEABF14b850f891de27ea09a1439e3835f66c49ad773f the afl_deleted field
was introduced as NULL, and wasn't used.
*In rEABFfe39e38282fc4c7903eb3f8080dbf0bab0f697f4 it was ALTERed to be
"NOT NULL DEFAULT 0"
*And in rEABFa2ead8bfb5166e0b354f3bb3e09f39795cb5b1c0 this function was
introduced to "negate the need for a schema change".
However, when ALTERing afl_deleted to be NOT NULL DEFAULT 0, all NULL
values have been automatically converted to 0 thanks to the DEFAULT
clause, and being the column NOT NULL, of course no NULL are still
there... The ALTER was applied to all wikis (in 2010), so afl_deleted is
NOT NULL everywhere and we can safely treat it as such.
Change-Id: Iebd843629d26e392d2e24efc2795c767e854897a
Follow-up of I1721a3ba532d481e3ecf35f51099c1438b6b73b2. This is the only
wrong replacement: strict checking will let 5 / 0.0 pass, with
unexpected results. Adding a regression test for it, too.
Change-Id: I25dbe9fafa92fd9a11bd8bc6ab8e66f305b8d48e
Since double-equals are evil. I left some of them in place where I
wasn't sure, but I may be changed some which were intended to be
doubles. It could be a good idea to delay merging this patch until we'll
have more code coverage.
Change-Id: I1721a3ba532d481e3ecf35f51099c1438b6b73b2
These hooks can be called either when filtering the current action, or
to check an action from recentchanges. While AbuseFilter already handles
well these two cases, other extensions don't and need some context.
Depends on the patch below because we're changing generateTitleVars,
which already has a temporary extra parameter that I don't want to mess
up with.
Also follow-up I72933fcc9952fc1aabf6464b2fc0b04ec39c024b for a few
remaining uppercase vars.
Bug: T203166
Depends-On: If5f238cddb41ef92b141e36b4f2f15fd4cc86476
Change-Id: I1983b93bbadabd24d8bf94fa7bb14594d10e731e
Using MWProps will not raise an 'undefined index' notice and, if the
bitsdepth cannot be established, will return 0. This is way better than
returning the empty string or null, which cannot be treated as integers.
Bug: T218874
Change-Id: I2b585f3a2f257783c15f0fcdd2851f1e1ce4256b
Follow-up of I982d67aa62a899916a26452aceb9646df8c31232. The help text
was meant to be localized, and I probably forgot to do so in the
mentioned patch.
Change-Id: If394b02819911f9c97519b5c972977c38e6d83fa
If "tag" option is selected and the form is submitted without adding any
tag, just show it blank instead of adding an empty tag to the topbar.
Separately validate the empty tag case (and added a test for it).
Bug: T203353
Depends-On: I3b2e763bd8835207dc5df1db43d3e1881e6961c3
Change-Id: I8884b739fd17fa2eace5aac8775d3524aa606f1f
The uppercase is just a leftover from a long time ago. Currently,
variables are case-insensitive, and we already perform a strtolower when
saving them. Since most parts of the code already use lowercase, the
uppercase leftovers only make it harder to grep the code to find
variables. As a bonus, make Ace recognize variables in a
case-insensitive fashion.
Change-Id: I72933fcc9952fc1aabf6464b2fc0b04ec39c024b
Adding PHPdocs to every class members, in every file. This patch only
touches comments, and moved properties on their own lines. Note that
some of these properties would need to be moved, somehow changed, or
just removed (either because they're old, unused leftovers, or just
because we can move them to local scope), but I wanted to keep this
patch doc-only.
Change-Id: I9fe701445bea8f09d82783789ff1ec537ac6704b
This patch adds missing methods for testing a filter against uploads in
RC. Please note that (as discussed below) using wfFindFile could be
relatively expensive (as it will be executed for 0-100 RC rows).
If this is true, then we should either use another method (but I
couldn't find a suitable one), or simply reduce the amount of testable
uploads to a reasonable limit.
Bug: T170249
Change-Id: Id406d4e1571873f49bb11e69029311b24ececf49
As escaping is handled by makeExternalLink itself. This currently makes
seccheck fail for any patch and is a merge blocker.
Change-Id: I2d21632bbc59abd4ea48aebdb6572d53f8fc89cd
So that they're easier to read, and because readonly is semantically
more appropriate.
Bug: T217143
Change-Id: I76be8e7fb1cf46efd0c03cde74344be6cb2a0902
Where prevents is used as a setter, use the new setter methods;
where it is used to determine whether a block blocks the target
from editing their talk page, use appliesToUsertalk.
Block::prevents was deprecated and replaced by several other
methods in I0e131696419211.
Bug: T211578
Change-Id: I166cc6f64c0f895ff8c631d2655c1c3208131371
Following up I636b4e56f39282593c737ace1d6ff2d90900d997, enforce a basic
clientside validation and don't fill the field with the URL parameter if
it's not valid.
Change-Id: If4fd015dff64237375a0c4d3b9fbcefbd54dba3e
Some ObjectCache:: methods are soft deprecated since 1.28. Remove them
now, since the replacement is easy.
Change-Id: I713781d5e98238a1c194e97b5faae488a8ac190d
Using break could halt parsing between operations, instead use continue
to parse all operations.
Bug: T214642
Change-Id: If67ddaffef280c2448c55ae536013758617bba68
This adds the capability to filter AbuseLog using filter groups, if
there's at list an extra group (like flow). Since abuse_filter_log
doesn't store info about filter groups, this needs query on
abuse_filter, and its result must then be intersected with explicitly
searched filters, if any.
The way I wrote it takes several lines and IFs, but is meant to be less
subject to regression in case something gets moved.
Change-Id: I747ba491d2b390562ce5f71396eed095116d8eaf
For wgLang, there's a Language object available in the proximity, so just pass it.
For wgContLang, use MediaWikiServices.
Change-Id: Ic492007f2d5eeb8048d0919a4b9b7dd98c15c350
The following sniffs are failing and were disabled:
* MediaWiki.Usage.DeprecatedGlobalVariables.Deprecated$wgContLang
Change-Id: Ic167fc5e836c5437edc6b330e5d73f9913bc2859
In both SpecialAbuseLog and ApiQueryAbuseLog, we use
Title::getUserPermissionsErrors to check if the user is allowed to
perform 'abusefilter-log' on the API page... However, this is a
completely redundant check (which is also pretty expensive and queries
the master): for the SpecialPage, we can specify the required right in
the constructor and use checkPermissions, and for the API we can simply use checkUserRightsAny.
If I'm not mistaken, there's no benefit in using
getUserPermissionsErrors.
Change-Id: I4c4dbace67b24cc1f45e50ab1c0d251522935513
We do not validate the param, and instead only check if it was
specified. In the specific case of ViewHistory, specifying as "filter"
something invalid for a title (e.g. with a + inside) will throw an
exception, seen in production.
Change-Id: I636b4e56f39282593c737ace1d6ff2d90900d997
The case default was recently added, but didn't take into account that
"false" is valid too. Noticed by chance just before the train rolled
out.
Change-Id: I67ca475fa16ea449820f8c735531c2cc1b0ec975
Several people have reported throttle groups being hard to use, mostly
because the field doesn't have options with the usable groups. This is
because users can combine valid groups in many ways, and thus we don't
provide options. However, let's add an help link pointing to mw.org.
Change-Id: I982d67aa62a899916a26452aceb9646df8c31232
Now that Parser errors are on logstash, I noticed a huge spike of errors
on Wikimedia Commons, about 35000 per hour. They seem to be due to 2
broken filters, but id doesn't say which ones.
Change-Id: I8510319c075520f9a893cd7d56f2e30679e249ba
Tagging doesn't work for account creations, and probably never did. This
is because we used a wrong identifier for such actions. This patch fixes
the problem, although in the long term we should find a smarter way to
apply tags.
Also, clean AbuseFilter::$tagsToSet if the action will be prevented.
Depends-On: Ia8e38ba25d1989fe71714d2b76891c4587921466
Change-Id: I8edcca17ecdcf71397cc9b0d101e8b13ac112047
This may solve several issues, see T176291#4105438 for further details.
Bug: T191430
Bug: T176291
Depends-On: Iebbdeac7898b35beea79aa3d0cdf9d0fb265d726
Change-Id: Ia8e38ba25d1989fe71714d2b76891c4587921466
When updating the abuse_filter_history table, the sequence to use is the
one on afh_id... And we were using the af_id one since 2009.
Change-Id: I3e291c780119d74be5f47e745a8de13bda85486b
Instead of adding a message, do like core does by striking and greying
out the row. Plus, don't show the AbuseLog page description when hiding
entries, as it doesn't fit.
Change-Id: I645a89dd8df79d45ca440e0ba62adcdee921b8e9
Starting from PHP 7.3, passing the name of an undefined variable to compact() raises a notice. Always define $querypattern and $searchmode, so that this won't happen, and makes showList behave more uniformly.
Bug: T214269
Change-Id: Ib179a7e0e4fdd7b9d81b6930000203478e7a1e38
This is an old leftover, used to add global JS variables in a convoluted
way: using a hook and a total of 3 static properties. We can safely
remove all of this and just call OutputPage::addJsConfigVars, which BTW
is already called where we need it.
Change-Id: Ifad0618fa93b0c7a7e8b23f596234e622aa8846a
Right now, we allow empty messages, and when the "warn" action is
executed we use "abusefilter-warning" if no message is specified.
However, this also produces a PHP notice while editing a filter with
empty message (see Phab). With this patch, empty messages will be
rejected, and a follow-up will be discussed on Phab.
Update: added disallow message as follow-up of
Ic1de03a6944c43a346fa317ee0a217551f0d284a.
Bug: T203353
Depends-On: I8df247f61d9f3769e9580544f324dd174811e939
Change-Id: I71b1f81d10c02de4de141b1ab9b630d05cf4619c
*Don't reuse a message (which is bad), instead add a note for
translators. We can also move it on translatewiki.
*Don't show the AbuseLog link if the user cannot see the AbuseLog.
Change-Id: I4ce73b2160275fdc4b0b7bec722471696d8c6a4d
As follow-up of I10b1fd2d9bdfe518089c053d77fef568170ecb65, use
'AbuseFilter' instead of 'AbuseFilterDeprecatedVars' as channel name.
Raise level for null-title filtering. Since with a null title
several things are likely to break, a warning is more appropriate here.
Tweaked the message as well, to include the bug number and to avoid
pointlessly including the title (which is null).
Lower the level for stashedit hit/miss (as it's really spammy and not
that useful right now).
Use 'abusefilter' instead of 'AbuseFilter' for statsd so that everything
has the same prefix.
Also raise the level for parser exceptions and unrecognized
consequences.
Change-Id: I1f9988155e924232b201281795cd322636da8082
Follow up to Idbb3a70d08a195dfa21422e07f593d1eeba4521d
This also fixes the fetching of text for the stash edit code path
which was missed by the previous patch.
This now also uses the full old text in the variable holder.
Bug: T213453
Change-Id: Ib80bc6385ebb5dd82bb1a384dd0e162608bfcbfa
First step for removing meta-variables, the second one being removing
global_log_ids and local_log_ids.
Change-Id: I01cd79771c0ee0865abaef6757a930aacd8138d2
The RecentChange class has several getters and setters for the $mAttribs
property. Although the property is public, it's saner to use such
methods.
Change-Id: Ie8e37e80fdcf2917ee0e87b2a409f0afb91a4f92
When all the other patches will be merged, this workaround won't be
necessary, and by removing it we're finishing the first phase of
variables migration. Which could also be the only one if we decide not
to go on and remove the old ones.
Bug: T173889
Depends-On: I5c370b54e6516889624088e27928ad3a1f48a821
Depends-On: I6576497feaf6d2c475ee33a91feb6a640e2c20fe
Depends-On: I87a48fdc8b392b25eb02807e8d0f712d0a399ece
Depends-On: Ib29eb15c1a51c037d036be8dc1541d96ea4b174b
Depends-On: I909a99e80a895a9b009c33ea72e8e0a4ea0a1375
Change-Id: If5f238cddb41ef92b141e36b4f2f15fd4cc86476
This is for adding variables which can be computed even without an
ongoing action. Currently, we don't have any, except for timestamp (but
that's a bit special). Other extensions could. For instance, we'll be
able to expose the content of the spam blacklist.
Bug: T211680
Change-Id: Iba59fe8d190dd338ecc8cfd682205bce33c9738b
It's totally pointless to recreate all variable names, since we already
have them in builderValues. The only exception is for _restrictions
variables, although they should be handled in builderValues too.
Change-Id: I156ebb1e6e590d09ded093a23d19c0d635a503bf
Otherwise calling bufferTagsToSetByAction multiple times makes the list
grow, and IIRC the core doesn't call array_unique on the tags to apply.
Also clean the list after applying tags.
Change-Id: Iebbdeac7898b35beea79aa3d0cdf9d0fb265d726
The variable was disabled with I7f13773766e12f3d4b86451fdf3ae23e067ac373
in 2016, but not in the same way as old_text and old_html were disabled
in 2009. This patch uses the methods introduced with
Ife168522e6b1d8eb94ebbb8a16ae8831ec1dc497 to disable minor_edit in a
standard way, so that it won't be showed in new AbuseLog entries, and
won't be usable when writing filter syntax.
A warning will be emitted if a pre-existing filter is using it, so that
we'll be able to completely disable it in the future.
Change-Id: I5ad5219ee19a5e6ba2bfdffb4e0aad63c8951491
In T209565#4826952 I discovered that if the "abusefilter-blocker"
message is an invalid username, we silently end up without a system
user, thus risking to break something. Instead of silently failing, emit
a warning and use the default name. As I wrote in the code comment, we'd
better avoid throwing, because the message can be modified by anyone,
who could then break the site.
Change-Id: Ifa866bd9676945bf94e7e481adf6ad0d6cf4370c
This partly reverts If72b18bedac5e580487406e696aea1fd172ae45b. While
it's true that we don't need every filter, that method is public and
other code may need fields that we don't need. This way we can encourage the
use of this function (which caches the result) instead of direct DB
access.
Also, the method can currently accept global filters passed as
"global-<integer>", but saves them to cache with the same key as local
filters (i.e. local filter 15 and external global filter "global-15" are
both saved in AbuseFilter::$filterCache[15], which could lead to subtle
bug).
Change-Id: Ieb04f019453033c275e211cfc9fd68d5d7c392ef
My final testing unveiled 4 problems, see T209565#4780868. Testing again
after this patch yields the expected outcome.
Update: A fifth problem is that we cannot disable throttling if throttle
groups are empty or fully invalid: that case is similar to the one with
invalid rate, the throttle limit is never reached and thus throttle just
doesn't work. Instead, ask users to fix it by hand.
Bug: T203336
Bug: T209565
Change-Id: Id03c9880f60764efc596ac40b8662087fdb30550
We have two situations where we try to execute filters without a title.
However, the code doesn't handle it correctly: some points expect $title
to actually be a Title object, and we also pass it around using a hook
which explicitly says it always pass a Title. This patch adds two debug
points to help understand why we end up with null titles, so that we can
fix it upstream.
Bug: T144265
Change-Id: I35bfc483a0c69a5cbd38eae8ba299189955fa1ec
Currently it barely has any reason to exist, as it's a single-line
method. This patch moves there the global state, and also changes the
signature to allow shorter calls.
Change-Id: I7851fa41cbd96912b3859319ba97a501b1cbaa57
