The getLocalURL() method can return url with query string when
wgArticlePath is configured to do so, and query string of GET form
would be ignored by browsers.
The setAction() method is problematic (T285464 and above) and hard to
warn the wrong usage. I'm going to go through and fix every use case,
and finally deprecate it.
Change-Id: I66b634f0cc996be3d7048d410b46fe77c88f9879
This is a breaking change for the API: 'hidden' is now either true or
false, depending on afl_deleted. 'implicit' is no longer a possible
value, the caller should compute it instead if necessary.
Then simplify the remaining usage of isHidden, using a temporary private
method.
Bug: T291718
Change-Id: I97b5195d306c35ddca3f071d9ff4d896f9fd5c8d
Use setTitle() instead of setAction(), T285464.
HTMLForm would set edit token for post form, use setTokenSalt() to amend.
HTMLForm would fetch user input value from the request itself, since
the two form shared the same field name, the 'default' params assigned
are unfunctional.
HTMLForm would prefix descriptor keys with 'wp' as the default name
of generated input fields, make use of this feature.
Bug: T285464
Change-Id: I2cc3c1d042998b65df5ee51f0715fe25a5e18e72
Introduce shorter methods, one for each steps, so that it's easier to
understand what the code is doing and figure out if the order makes
sense. The ConsequencesExecutor test is now a proper unit test. Also
simplify AbuseFilterConsequencesTest, removing old/wrong logic and
fixing two expected values that were actually wrong (but worked because
of the aforementioned wrong logic).
The only functional changes should be:
- We pick the longest block *after* checking the ConsequenceDisabler
consequences, so e.g. if a filter has a long block + warn and another
filter has a shorter block, we still keep the second one if warn will
disable the block.
- Remove disallow in presence of dangerous actions after checking
ConsequenceDisabler's and deduplicating blocks. Otherwise we may
remove disallow for filters where block (etc.) doesn't end up being
disabled. We may also want to consider not removing disallow at all,
now that messages are customizable.
Bug: T303059
Change-Id: If00adbf2056758222eaaea70b16d3b4f89502c20
This was likely a rebase artefact or something: the 'implode' was meant
to be called with two parameters as usual. Currently, the parameters are
simply concatenated which makes the logs quite hard to read.
Change-Id: I84f9a7cb05e210f60a791d513dfb5b74fa7dfb8a
- Use a /64 range for IPv6 instead of /16.
- Fix a curious and serious bug for IPv6, where grouping by range
would only use the first (!) number of the IP address, due to the
'v6-' prefix returned by IP::toHex.
- Fail hard if the identifier is unknown -- it's not something that's
supposed to happen.
- Include the type name in each identifier, instead of prefixing all
type names to all identifiers. This makes it easier to understand the
parts of the key.
- Test the whole lot.
Bug: T211101
Change-Id: I54c4209f2f0d5a4c5e7b81bed240ca3e28a2ded7
List which actions were disabled, or explicitly say that no actions were
disabled if that's the case. Also avoid the word "throttle" in messages
as it may be hard to translate. Also don't suggest optimizations to the
filter conditions -- unoptimized rules have nothing to do with a filter
being throttled.
Bug: T200036
Change-Id: Id989fb185453d068b7685241ee49189a2df67b5f
This is a plain value object that represents the action being filtered,
replacing associative arrays that were being used up to this point.
We should now check whether it's possible to make it not require an
accountname (which complicates things), and then use it in related
classes as well, e.g. Parameters.
Change-Id: I9550c14819b600c97c46b632cc1c2d447972d69c
The existing filters on WMF wikis has been changes such that calls
to rmspecials() are now rmspecials(rmwhitespace()) to ensure no change
is made in behaviour. Filter admins can change this back if filter is
not meant to trigger when part of the input is contains spaces.
Bug: T263024
Change-Id: Idde09b50fb8eda357afbedc1199a5483fa8217c1
Replaced with the more readable ::disableClientCache() method, added
in 1.38. Minimum MW version bumped to match.
Depends-On: I7c89e20528a0d91173f0edcb997dcae631935ee5
Change-Id: I91d0b8e8f69a2d309b6fc61e13bfb5d86dc0218d
old_links and all_links are an array. Casting
them to string and then splitting by newlines
is a no-op.
Change-Id: I05c69f14e981ac2842032e7db888f4841d6b48b7
We should avoid using ObjectCache directly and use services instead. On
top of that, db-replicated writes to objectcache table that should be
avoided and it's now blocking schema changes.
This should go to MainStash.
Bug: T272512
Change-Id: I9c76399ec4c0ea6644f9ca1b4536428052e5ac38
These are not necessarily old links, the new links
can also be retrieved using this code path.
Also print debug messages before the code execution.
Change-Id: I1a85bb7b5a2af4fe514625d2236cf92f15daf304
The special page itself is a MessageLocalizer
SpecialPage::getContext on constructor results in global state
Change-Id: I6a3dd263dbe02111aea65a087ce69a5a191f63fc
The report is now generated in ParserOutput, not Parser, meaning we can
simply avoid passing the `enableLimitReport` option (off by default) if
we don't want the report to be there.
Depends-On: I154c0a77a5b0287b5572614d56339fb57ac56c33
Change-Id: I8cdab35c475f10433234ddb55b5e6a0cc8109498
Note that this doesn't have to be applied in WMF prod, since the indexes
are already correct there.
Bug: T291725
Change-Id: I7fcee0581f469e0d7ada43d1f30f31061c5bf5c0
This does the same as before, replacing \r\n as well as \r with \n.
Additionally the new method applies an rtrim() on both strings. I
believe this is even a good thing. It possibly removes irrelevant
noise from the diff.
Change-Id: I584740a24e6b25bbcbc928c2369f09b785a485c8
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
