On second thought, no need to keep the migration script now, since it's
unusable anyway.
Also remove an usage in SpecialAbuseLog, likely a rebase artefact.
Change-Id: I938924b3617ef30046d8317e68a101ed2c1883d3
This replaces the previous pattern of callers having to use
RevisionLookup if the result was 'implicit'. Also, in some cases where
we were just hiding things if the visibility was !== true, properly
handle the implicit case by using the new method. Make the new method
return string constants rather than bool|string.
The new method also fixes some potential info leaks which happened when
the row was hidden, the user could view suppressed AbuseLog entries, but
the associated revision was also deleted and the user couldn't see it
(this shouldn't be relevant for WMF wikis since AF deletion is
oversight-level).
Also add a bunch of tests for the various cases to ensure we don't
regress again.
Bug: T261532
Change-Id: I929f865acf5d207b739cb3af043f70cb59243ee0
ParserStatus is now more lightweight, and doesn't know about "result"
and "from cache". Instead, it has an isValid() method which is merely a
shorthand for checking whether getException() is null.
Introduce a child class, RuleCheckerStatus, which knows about result and
cache and can be (un)serialized.
This removes the ambiguity of the $result field, and helps the
transition to a new RuleChecker class.
Change-Id: I0dac7ab4febbfdabe72596631db630411d967ab5
Something somewhere is leaving error_reporting in a dirty state
causing AbuseFilter's ConsequencesExecutorTest case to fail for
the core change Ic9fee6cdd88001025.
Per T253461, we're meant to eventually remove this anyway, so might
as well remove it in areas that are known to get it wrong somehow.
Change-Id: I2a665f09a357f2f2cc258d8c4011d49a7ab9c13b
The old parser now has the correct name "Evaluator", so the
ParserFactory name was outdated. Additionally, the plan is to create a
new RuleChecker class, acting as a facade for the different
parsing-related stages (lexer, parser, evaluator, etc.), which is what
most if not all callers should use. The RuleCheckerFactory still returns
a FilterEvaluator for now.
Also, "Parser" is a specific term defining *how* things happen
internally, whereas "RuleChecker" describes *what* callers should expect
from the new class.
Change-Id: I25b47a162d933c1e385175aae715ca38872b1442
Remove unnecessary setters, injecting everything in the constructor.
These were leftovers from before the introduction of ParserFactory.
Remove public access to the conds used, include the information inside
the returned ParserStatus instead, and consequently simplify callers.
Change-Id: I0a30e044877c6c858af3ff73f819d5ec7c4cc769
So that the method can be typehinted in core.
Also add phan-var to fix broken master build due to typehint additions
in core.
Change-Id: I4a072e00ffeeb437753fc3d3c1f15de9929df510
This commit adds a class AFPSyntaxChecker which can statically analyze
a filter code to detect the following errors:
- unbound variables (which comes in two modes: conservative and liberal,
default to conservative)
- unused variables (disabled by default for compatibilty)
- assignment on built-in identifiers
- function application's arity mismatch
- function application's invalid function name
- non-string literal in the first argument of set / set_var
The existing parser and evaluator are modified as follows:
- The new (caching) evaluator no longer needs to perform variable
hoisting at runtime.
- Note that for array assignment, this changes the semantics.
- The new parser is more lenient, reducing parsing errors.
The static analyzer will catch these errors instead, allowing us
to give a much better error message and reduces the complexity of
the parser.
* The parser now allows function name to be any identifier.
* The parser now allows arity mismatch to occur.
* The parser now allows the first argument of set to be any expression.
Concretely, obvious changes that users will see are:
1. a := [1]; false & (a[] := 2); a[0] === 1
would evaluate to true, while it used to evaluate to the undefined value
due to hoisting
2. f(1)
will now error with 'f is not a valid function' as opposed to
'Unexpected "T_BRACE"'
3. length
will now error with 'Illegal use of built-in identifier "length"'
as opposed to 'Expected a ('
Appendix: conservative and liberal mode
The conservative mode is completely compatible with the current evaluator.
That is,
false & (a := 1); a
will not deem `a` as unbound, though this is actually undesirable because
`a` would then be bound to the troublesome undefined value.
The liberal mode rejects the above pattern by deeming `a` as unbound.
However, it also rejects
true & (a := 1); a
even though (a := 1) is always executed. Since there are several filters
in Wikimedia projects that rely on this behavior, we default the mode
to conservative for now.
Note that even the liberal mode doesn't really respect lexical scope
appeared in some other programming languages (see also T234690).
For instance:
(if true then (a := 1) else (a := 2) end); a
would be accepted by the liberal checker, even though under lexical scope,
`a` would be unbound. However, it is unlikely that lexical scope
will be suitable for the filter language, as most filters in
Wikimedia projects that have user-defined variable do violate lexical scope.
Bug: T260903
Bug: T238709
Bug: T237610
Bug: T234690
Bug: T231536
Change-Id: Ic6d030503e554933f8d220c6f87b680505918ae2
Create a dedicated "Exception" sub-namespace and remove the "AFP"
prefix, a leftover from the pre-namespace era.
Change-Id: I7e5fded9316d8b7d1628bc1a6ba8b1879ac901e1
Previously, for non-newly-created pages, AbuseFilter would get the text
for filtering twice: once in AbuseFilterHooks::filterEdit(), and then
again in RunVariableGenerator::getEditTextForFiltering(). (Plus another
call for the text of the previous revision.) The first copy of the text
is only passed into RunVariableGenerator::getEditVars(), and there only
used if the title doesn’t exist, otherwise it’s overwritten with the
second copy. Instead, let’s make AbuseFilterHooks not get the text at
all, and only get the text from the content when we actually need it
(the content is new).
Change-Id: Id12430fa6ba4643113b945e0d0c01b9c0ee1742f
This reverts commit 15fc159cb1.
Reason for revert: this is breaking the addition of rev ids to filter
hits after edits are saved. I suspect this is because the context wikipage
is for a different title than the one being edited, though I'm not sure
way - regardless, testing on patchdemo shows that with this revert
is applied, rev ids are once again added to filter hits.
Bug: T286140
Change-Id: I3ab6324a73050154cef1c20a2bf8307eb11eea2d
If the content language is English and the message is invalid as
a username, or the content language is not English and both the
content language version and the English version are invalid, the
user in FilterUser would not be created - now, avoid the onwiki
version of the English message in the fallback, so it could only
be invalid if the default in the i18n files was invalid.
Bug: T284364
Change-Id: I9e9f44b7663e810de70fb9ac7f6760f83dd4895b
The master version of the extension is only meant to support the most
recent version of MediaWiki.
Change-Id: I33612e69fc37bf5eb70133c8f0e95199dd7fcb65
UserEditTracker::getUserEditCount now allows anonymous users,
but it returns null and phan is aware of this. Suppress this
warning until at least 1.37 is required.
Change-Id: I9962abe08fa31d55421d8bdda23ea0a1c0471a86
Sharing a handler class with UserRenameHandler means that attempting to
merge users fails due to a missing interface if AbuseFilter and MergeUser
are installed but Renameuser is not installed.
Change-Id: I1244ab1c446840ff2648248f943d7fc784b889a7
These are part of legacy styles and aren't provided by all skins.
Using Html::successbox abstracts the classes away.
Internally that uses div class="successbox" instead.
Bug: T280766
Change-Id: I0cca59e2f391510095c2c6fb187ace5e91fdde8b
Follow-up I574bda15f0f5c92a7d97a6e3150981b8f97ee7fc
Apologies for not noticing before:
If somebody hadn't already added the afl_filter_id column, the
rename-indexes patch would try to rename a non-existing index
(filter_timestamp_full and fail). So put rename-indexes after the other
patch.
Then, for the afl_filter_id patch, check the column and not the index.
We were checking the index because it's the last thing that the DB patch
does (so if the index is found, we can be certain that the patch was
fully applied). However, now that renaming the index happens afterwards,
if somebody had already added afl_filter_id (with the old index name),
running the updater would try adding it again, because the new index
name isn't found (as it's renamed later).
Change-Id: I0250a7c187202facd932c160ace57930db510f64
Extensions are supposed to return false to break hook chains when failed, which can avoid unnecessary call of later handlers in other extensions and work around with problems caused by difference betwen multiple triggers.
On mediawiki version 1.36 and before, just returning false in this hook can't display error message by default.
Set $status->value manually still to provide backward compatibility.
Bug: T280312
Change-Id: I78888247063c726ebcd18ba54a21d6c7891481fc
Migration was completed in MW 1.34, so it's no longer necessary to
call ActorMigration.
Bug: T278917
Change-Id: I26ad45b6d26756c3074c44f0192ceb04fb2698ae
- Inject dependencies.
- Make class variables private or protected and rename them.
Untangling the circular dependency is left for a future patch.
Change-Id: I5d625e30171bfbf60d9f5a94fa50475fdfe853dd
The block log entry will be automatically suppressed, until we can
implement a better solution.
Bug: T152394
Change-Id: I8bae477ad7e4d0190335363ac2decf28e4313da1
The only user we were worried about has now migrated to this; it auto-
ran in other installs just fine, so let's proceed.
Bug: T269712
Depends-On: I2b905f1e13ec13ec94d33527803c91c04b491eb2
Change-Id: Ie7d6bc95ebc871b0effee069e2146f2750d5f408
All methods were moved to the new parser. Tests and other pieces were
adjusted to expect just a single parser. There are still some TODOs
(remove AFPTransitionBase, remove $this->mCur), but these are left for
another commit.
Note that the new parser was not renamed: this is because the names are
wrong anyway (CachingParser is more of an Evaluator than a Parser, and
AFPTreeParser is the real parser, and should be renamed as well).
NOTE to reviewers: this patch looks quite big, but if you diff the old
parser with the new version of the CachingParser, you'll notice that the
diff is actually small, since everything was basically copied verbatim.
Bug: T239990
Change-Id: Ie914ef64c70503a201b4d2dec698ca2fa8e69b10
- mark properties as private (unused outside)
and rename them to avoid legacy naming
- do result filtering server-side
- order query by timestamp
Change-Id: If2d714753a2b040c5cefa8f8126f82a3c08dab44
Some of these api modules still retrieve other services
statically, this patch is focused just on injecting the
permission manager and setting up DI
Bug: T259960
Change-Id: Ic5196f230d68604fdf321f705377a1e6e1e2efca
Article::prepareContentForEdit is deprecated and being removed,
refer to WikiPage::prepareContentForEdit instead
Plus remove an extra line
Change-Id: Ie4438c710639a16557816b53510ce230d15d641c
UserEditTracker checks that the user is not anonymous, whereas
User::getEditCount() would just return null. This was not spotted by
tests because UserEditTracker is mocked.
Bug: T277859
Follow-up: I8a55bd5cb17bbc259ec36c40261058e0b46ee4a6
Change-Id: I05fb6cc780c80b72b3278e6dc670ed2025628ffb
This is kind of a nuclear option, if anything in a row is hidden, we
hide the whole row. This is just to keep this patch slim. A public
follow-up will adjust the visibility
Bug: T274152
Change-Id: I6063c02fa261c4cc0e6dbbb2db4e111eb85912c2
Otherwise we'd be telling whether the filter matches or not the edit. If
we're showing all edits regardless of whether they match the filter, we
can keep showing the row: it will be redacted (and the filter result
hidden) by AbuseFilterChangesList.
Bug: T223654
Change-Id: I3f7dbd8b873d411e37c8c3aac2339bf5ec36907d
There are various info leaks for both deleted rc rows, and suppressed
AbuseLog entries.
Bug: T223654
Change-Id: I4900b1be73323599d74e3164447f81eded094d75
The hook names contain a dash, which is mapped to an underscore by the
hook runner (see Ie8c8fb603b33ff95c8f8d52f392227f147c528d8), and the
previous method names weren't matching this.
Follow-up: Ic5c82a367e34135bbc0f00ece5aeef4f2d92881b
Change-Id: Ie80b62c49b2f4aaea49d5a1883f513348689d16a
Dashes are mapped to underscores, but following the "modern" convention,
the hook name should be pure PascalCase.
Bug: T275798
Change-Id: I77909b3ee772b983c7933f3b82230476772bd3b5
Requires injecting a temporary block factory, and excluding
ManualLogEntry::insert from the test, but it's now much cleaner and
quicker.
It still cannot be a unit test due to the usage of User.
Change-Id: Iba9732d6d79733b31b45eb4d0187b1c8a82499dc
It is currently possible to save a filter with an invalid group, if you
manually change the form data. So prevent this by validating the group
before saving.
Change-Id: I03f80b8c6ab583a357273f7b2679a424ac784db7
Remove using of User::getCanonicalName since this method will be hard-deprecated. Now it is soft-deprecated
Bug: T275030
Change-Id: I3ce1199f18276096279ce3c80f63e53d023a0f5a
Everyone can examine generated variables but not everyone
can test filters. Concerns Special:AbuseFilter/examine.
Change-Id: I9c205a0f1d9a7fdf15c4998d43983b9fa37f4694
This commit doesn't change any permissions for anybody.
It's the first step to achieve what the task asks for.
Bug: T242821
Change-Id: I8060ca926e6769b11d470fe4037854cda496000d
1 - Change the structure of if/elseif for readability
2 - In the old parser, if there's an empty argument, never add it (the
new parser was already doing that).
Bug: T156095
Bug: T156096
Change-Id: I4237b1a0ba01e7ce04dcc945f7daf34612fcf07d
Introduce a clear distinction between internal exceptions and
user-visible exceptions, leaving AFPException as base abstract class.
Later, it should be possible to narrow some types around, e.g. in
ParserStatus (that might work with user-visible exceptions only).
Also a future TODO is putting all the exceptions in their own namespace
(probably ...\Parser\Exception).
Change-Id: I4e33a45117f0a3e73af03cc1e3f2734beaf2b5e1
Thanks to this, we will be able to provide more information
to consequences and watchers, which will open door for new
features and possibly cleaner code.
Change-Id: I7135509823ea84b2a2923d2c1831ce293b98a9f9
Small refactoring. Create checkAllFiltersInternal and process
its return value in checkAllFilters to ensure compatibility.
Also fix some annotations.
Change-Id: If9d296de48f08d719f1700f88870002b814c5b31
This is a small refactoring. The method is protected,
so we only take care of compatibility of ::checkAllFilters.
This might be also be useful if we decide to work on T174554.
Change-Id: I83cd58ec325972264e86d7a73366c0affed0a37e
