Providing the function name is often optional from the php code,
but it is needed for better logging, so make it mandatory and let phan
report issues about this.
Bug: T374546
Depends-On: Iaed5489a85a5a6e685829e151436afc94310fbd0
Depends-On: Ie2a1e5052e5b61bbb5b89905de942f47d3f1413d
Change-Id: I5227f2fa65850ac8c6f620900f22d1f4e7bfd470
Avoid the call to internal constructor of AndExpressionGroup and
OrExpressionGroup by creating a factory function similiar as the
IReadableDatabase::expr function for Expression objects.
This is also a replacement for calls to ISQLPlatform::makeList with
LIST_AND or LIST_OR argument to reduce passing sql as string to the
query builders.
Created two functions to allow the return type to be set for both
expression group to allow further calls of ->and() or ->or() on the
returned object.
Depending on the length of the array argument to makeList() it is
sometimes hard to see if the list gets converted to AND or OR, having
the operator in the function name makes it easier to read, so two
functions are helpful in this case as well.
Bug: T358961
Change-Id: Ica29689cbd0b111b099bb09b20845f85ae4c3376
It is really annoying that we have to escape '<' and '>'
as '\x3E' and '\x3C', otherwise Phan will not accept them
(emitting PhanUnextractableAnnotation).
Phan will also escape them in the output, generating errors like:
Argument 2 ($op) is '=<' of type '=\x3c'
but \Wikimedia\Rdbms\IDatabase::expr() takes
'!='|'='|'LIKE'|'NOT LIKE'|'\x3c'|'\x3c='|'\x3e'|'\x3e='
I've wanted to do this for a while, but I wasn't sure if it was
worth it, because the escaping makes the type hints and the errors
hard to read. Today I noticed a bug in UserSelectQueryBuilder
that would have been caught by this, and convinced myself.
Change-Id: I7a72368446f463a99b3d0cc7a90e8f7cdca454fe
In some cases ISQLPlatform::makeList() will accept an associative
array (with string keys), but ignore the keys completely. Provide a
warning when this happens, and improve type hints so that Phan warns
about it too.
(We only warn about string keys, and not about gaps in an array
with int keys, because they arise often due to using array_diff()
or array_unique(), and that would trigger too many warnings.)
In Expression we already type-hinted it as a list (int keys with no
gaps), and we're intentionally less flexible about some other cases,
so check for gaps too and throw an exception instead of a warning.
Change-Id: I63717d16eae7cccd929b5d232944b97989113b1e
In all cases, these mistakes would have already caused an error
("Wikimedia\Rdbms\Expression could not be converted to string")
or a notice ("Array to string conversion"). But now we also get
Phan warnings to avoid them, and a better message if they happen.
Add tests describing the invalid cases.
Change-Id: Idd4ba30a8145451e85018c35d5441910af76f009
This patch introduces a new namespace declaration,
MediaWiki\Xml and adds Xml and XmlSelect to it
and establishes class aliases marked as deprecated
since version 1.43.
Bug: T353458
Change-Id: I45cccd540b6e15f267d3ab588a064fbeb719d921
I've been having fun documenting all the terrible array shapes
accepted by the various RDBMS methods with Phan type hints,
but I'm increasingly worried that either Phan will change how
it interprets them or that someone will break them by accident.
Add a test file, similar to the one we have for testing taint
annotations for Phan SecurityCheckPlugin, to guard against that.
Start filling it with some tests for expr() / Expression.
Add two missing test cases I noticed while writing these.
Change-Id: Icb54d5a7529f7f82ff5d130dcea0a22450155c10
phan-taint-check-plugin hardcodes the sql options
GROUP BY, ORDER BY, HAVING, USE INDEX, IGNORE INDEX
to have taints. The query builder should have same taints
(MWVisitor::checkSQLOptions)
Change-Id: Ie56e1d30a760a0203de6989972cfa1215344fd31
This is useful for cases that we can't use the common expression
builders but we don't have a choice but to pass raw SQL. Common usecases
are comparing fields ("a.user_id > b.user_id") and function calls
("user_editcount > COUNT(rev_id)")
Bug: T210206
Change-Id: Ieb73d449262e22557f6f470105ca65ab0afc50e3
…to avoid mistakes where the return value of ->and() etc. isn't used.
https://github.com/phan/phan/wiki/Annotating-Your-Source-Code#phan-side-effect-free
> `@phan-side-effect-free` can be used to indicate that a function,
> method, or closure does not have any side effects, and should have
> its return value be used.
Change-Id: I610435f4cbf7335605a6713758d15f8722478b72
ParserOutput::getText() is not a simple getter, but does
transformations on the "text" of the ParserOutput; the simple getter
is named ::getRawText().
To maintain consistency, rename ParserOutput::setText() to
::setRawText() and the property name ParserOutput::$mText to
::$mRawText so future readers are not confused.
The JSON property name as it appears in the serialized ParserCache
is left as 'Text' so that we don't have any forward- or backward-
rollback issues.
Change-Id: I3ef34814ab9473cc70d0a6806e8c5a4a02b73491
* Switch out raw Exceptions, mostly for InvalidArgumentExceptions.
* Fake exceptions triggered to give Monolog a backtrace are for
some reason "traditionally" RuntimeExceptions, instead, so we
continue to use that pattern in remaining locations.
* Just entirely give up on PostgresResultWrapper's resource vs. object mess.
* Drop now-unneeded false positive hits.
Change-Id: Id183ab60994cd9c6dc80401d4ce4de0ddf2b3da0
They were supposed to be dropped before 1.41 release.
Depends-On: Icd972535725e65e6eaed25607f1fd1c69d6b3a5c
Change-Id: Ieeaf39ec07407a55daa51c350cd57b66283a27bb
addHeadItem was marking the wrong parameter as exec tainted.
Add tests for methods that should have their taint infered.
Add some additional taint annotations that seem missing.
Change-Id: I5d11d1c6ce44d18cca6844f2bad2c10b1ae43311
Note that row() and rows() cannot be described with annotations, and
they need to be hardcoded in taint-check instead
(I7623ba5112bfffbcf972cc366e0db99ee75b448e).
Also annotate SelectQueryBuilder::caller(), as a follow-up to
Ic9fb15e083cca75c2b5c6bddd1df87b148acca6e.
Bug: T253380
Change-Id: Id4a884f5b91683d3946c712ac4149ffe855d7683
This covers the tables, fields, and conds methods. Options and join
conds are more complex and will have to be special-cased in the plugin
itself.
Bug: T253380
Change-Id: Ic9fb15e083cca75c2b5c6bddd1df87b148acca6e
Copied from MediaWikiSecurityCheckPlugin.php.
Duplicate annotations from Xml::encode* to the corresponding
Html::encode* methods, given that these were moved recently but not
hardcoded in taint-check.
As the only difference, remove the HTML taintedness type from the return
value of Message::rawParams. If the argument is unsafe, it's reported
immediately thanks to exec_html. Else, it does not contribute to the
taintedness of the return value.
Bug: T321806
Change-Id: I5ed340e1d127fb3eab6d6f9b905693d05a393360
These were copied verbatim from MediaWikiSecurityCheckPlugin.php.
The exception is selectRowCount(), whose return value was hardcoded as
tainted in taint-check, but that's clearly not the case because it
returns just an integer.
Suppress a new issue in DbTestPreviewer that wasn't previously spotted
because the code ends up using IReadableDatabase::selectRow, but the
hardcoded data in taint-check only considers IDatabase and concrete
classes, not IReadableDatabase.
Bug: T321806
Change-Id: I0c59d286c50de4ea79571242649bdda8aec67b57
These are the same as taint-check's MediaWikiSecurityCheckPlugin.php.
The notable exception is methods in WebRequest that were previously
hardcoded as returning a safe value. This was a consequence of said
methods return safe types (e.g., int, bool). Instead of adding
taint-check annotations, add return typehints instead, which let
taint-check remove any taintedness.
Fix some taint-check issues that were previously not spotted or whose
suppressions were removed in other patches.
Also fix the following bugs spotted by phan thanks to the type hints:
- SpecialExport did not have explicit handling of null $depth, and just
returned 0 because null fails both the < and the > comparisons.
- Improve documentation of params and props in ProtectedPagesPager.
SpecialProtectedPages can pass null $namespace and $size.
- Remove unused parameter from SpecialProtectedPages::showOptions, of
which $ns and $size were not documented as nullable.
- Add FIXMEs in SpecialVersion about very inconsistent escaping.
Bug: T321806
Change-Id: I726f528856614c92329683a0ad8936a42e262748
Make sure that taint-check knows about these methods, especially as we
move the taintedness info from the plugin to annotations on the methods.
Some tests are currently disabled because they would fail. These will
be enabled in subsequent commits while adding annotations to the
relevant methods.
Note that the disabling needs to be done by making the suppression
annotation invalid (done by prepending 'xxx' in this patch), because
commenting out the code or adding a method-wide @suppress won't work.
Bug: T321806
Change-Id: I8b89a22a5c23a2ab25329bcb06c673168d24683d
As per comments on phabricator, the Suppressor was used when phan still
didn't have per-line suppression, which is no longer the case. The phan
script was also outdated and broken, so just remove everything.
Bug: T267859
Change-Id: Ic4a68dd30a4481656a5c44b2446cf851ea638c1c
As per https://www.php.net/manual/en/function.libxml-disable-entity-loader.php
this is technically unnecessary.
>However, as of libxml 2.9.0 entity substitution is disabled by default,
>so there is no need to disable the loading of external entities.
See also https://github.com/php/php-src/pull/5867
>Since the release of libxml 2.9.0 in 2012 external entity loading is
>disabled in libxml by default. This means that using
>libxml_disable_entity_loader() is no longer needed.
Hopefully helps prevent false positive reports from security scanning tools.
Change-Id: I7cabc5b8d44813d709a11db2f219ae16260542c7
MediaWiki core now runs phan 1.2.6, bringing in nearly 2+ years of upstream
fixes.
Configuration was moved from `tests/phan` to `.phan/`. The legacy bash wrapper
script is still kept in the repository in its own location for any extensions
that are still using it. It should be removed before 1.33 is released.
Since there's a lot of new issues being flagged, all currently failing issues
are suppressed, and will be fixed in follow-up patches.
We're dropping the jetbrains/phpstorm-stubs repository in favor of just
the minimal stubs we need. Stubs for PHP extensions are kept in
the new `.phan/internal_stubs` directory, since they're in a slightly
different format than normal stubs.
Normal stubs are kept in `.phan/stubs`. wikidiff2 and excimer are kept with
these since we're also the upstream for them.
Change-Id: I3fe437befa17f4fbaf97aa6271f659b56021f396
Use the library instead of duplicating most of the config/defaults that it
provides. MediaWiki core is different of course, so we have to override a
bunch of file/directory lists, but there was a lot being duplicated.
This is the first step in migrating to a newer phan version.
Change-Id: Ib5987ebdf208138d97e1aba8ef54438064063fe9
Add a Profiler subclass for the new excimer extension. Since it does not
provide function counts, it's a little bit awkward to return it in the
format required by getFunctionStats(), but getOutput() works quite well.
Fix totally broken ProfilerOutputDb, the first parameter to the
onTransactionCommitOrIdle callback is not a Database. It is in the
second parameter, but most callers do not use it.
Change-Id: Icb20f3a5b0b09ff2905f1711f3681c398aa026e2
Depends-On: I6a9ccf5a12ef998e029033adf08af95c42fb7f8e
There was a single instance of this issue which was reasonably simple
to solve. The code that triggered the issue was not wrong, but calling
the grandparent constructor directly is not the cleanest code so it
seems right to fix it. Switch revision initialization into an
overridable function to remove the need to even have a constructor in
the class.
Change-Id: Ic2af0d93e8d55e93061e3f4b5c7a4122509543f0
