It appears that autoloading classes via MediaWiki's PSR-4 autoloader has
a not insignificant performance penalty, especially when hundreds of
PSR-4 classes like HookRunner's hook interfaces are autoloaded. Using a
classmap autoloader, like we already do for PSR-4 classes from Composer
dependencies, is a potential way to reduce the performance impact
here.[1]
For core classes, this can be done by simply not excluding PSR-4 classes
in AutoloadGenerator, causing it to include appropriate mappings in the
generated autoload.php classmap. I had to exclude one class_alias()
declared in Result.php from the classmap with a NO_AUTOLOAD stanza,
because including it broke AutoLoaderStructureTest's assertion that all
aliases should be defined in the same file as the aliased class.
Assuming this is still an issue, this would already have been a problem
because the test was previously skipping every PSR-4 class. Excluding
this file via NO_AUTOLOAD just restores that status quo.
----
[1] https://phabricator.wikimedia.org/T274041#8358399
Bug: T274041
Change-Id: I0aa62c944d874bf7a9f3a240e72e58fe6a887b28
This is really hard to read. What is code, what is string? These
places are so simple, they really don't need the "{$var}" syntax.
Change-Id: I589dedb8c0193eec4eef500bbb896b5b790b727b
This covers all occurrences of /onfig->.*get( '/ in includes/.
Undoubtedly there are still plenty more to go.
Change-Id: I33196c4153437778496f40436bcde399638ac361
Make phan stricter about null types by setting null_casts_as_any_type to
false (the default in mediawiki-phan-config)
Remaining false positive issues are suppressed.
The suppression and the setting change can only be done together
Bug: T242536
Bug: T301991
Change-Id: I0f295382b96fb3be8037a01c10487d9d591e7e01
$wgShellLocale was a flawed solution to the problem of locale
dependence. MediaWiki has its own concept of locale (the Language
hierarchy) and any kind of dependence on the server's libc locale is
incorrect and harmful, leading to bugs. Developers have an expectation
that functions like strtolower() will work in a certain way, and
respecting the locale set in the environment at install time violates
this expectation.
The problems with using C as a locale, which led to $wgShellLocale, are:
* escapeshellarg() will strip non-ASCII characters. This can be worked
around by not using it. The security vulnerability it was trying to
fix can be prevented in another way.
* Shell commands like rsvg will fail to correctly interpret UTF-8
arguments. This is the reason for the putenv(). On Linux, this can
be fixed by using C.UTF-8, which we didn't know at the time. On
Windows, the problem is not relevant (there are unrelated issues
with UTF-8 arguments).
Bug: T291234
Change-Id: Ib5ac0e7bc720dcc094303a358ee1c7bbdcfc6447
To be able to use non-default Shellbox URLs, we need to be able to
pass in a service name when creating the ShellboxClient.
Have CommandFactory::createBoxed() take a $service parameter that can
be used to change which Shellbox will be hit, as intended.
Bug: T290193
Change-Id: Ic1671a69070f962dbb0083028faf34d6d437022a
In order to avoid major disruptions, I introduced default value to
fallback to but in longer term we probably should deprecated it.
Bug: T285105
Change-Id: I81d9ece769c4942ef2ca390a40ff9d2c24c9ece4
Make Command extend Shellbox's UnboxedCommand. Only a few MediaWiki-
specific features remain in the subclass.
Also add BoxedCommand abstraction and Shellbox client.
The Result alias didn't actually work, I just had to change the return
type hint.
Bug: T260330
Change-Id: Iff7428e4c5fe3959a5cda8e113f223caa0976fc1
firejail has an RCE in its handling of --output when dealing with untrusted
arguments (CVE-2020-17367 and CVE-2020-17368). We can avoid this issue by
preventing shelling out to firejail if any parameter starts with '--output'.
Bug: T258763
Change-Id: Ic6a5644566a51a948de7b42daf57b29ced3daff4
Firejail uses /run/firejail to store the source of bind mounts that it
sets up within the container. Hiding /run/firejail by blacklisting it
breaks Firejail and causes the command to not run.
For unknown reasons, in WMF production, firejail works with /run
blacklisted. But strace confirms it is doing a very dodgy thing that
probably works by accident.
So instead, blacklist /run/*, which restricts each thing under /run
separately. Use "noblacklist /run/firejail" to skip that subdirectory.
Bug: T262364
Change-Id: Ic66e91d9d0831faadc116afdff69fa05d2d3bb03
Fixes executable paths with spaces in them, argument escapes,
and other strange behavior in Windows.
Also, fixes some shell tests on Windows. This is done by using
PHP scripts instead of native POSIX executables like "cat".
Behavior should be exactly the same on non-Windows servers.
Bug: T183759
Change-Id: I2367a6c47e3774bf4fabfa8c66e4bc4c5c8a714a
By default on Linux, any maintenance script that runs a command that reads
from stdin hangs with SIGTTIN until it is killed by the timeout. So:
* Instead of passing the stdin FD to subprocesses by default, make the
default input be an empty string. This only really affects maintenance
scripts, since in web requests, stdin is /dev/null, effectively an
empty string already.
* Add Command::passStdin(), which reverts to the previous behaviour,
except that the wall clock timeout is disabled to avoid a hang due to
SIGTTOU/SIGTTIN.
* Use passStdin() when running "stty size".
* Fix the hilariously broken Maintenance::readlineEmulation(), which
accidentally tried to use Shell::escape() to run a command, as if it
worked like wfShellExec().
* Add Command::forwardStderr(), to support readlineEmulation().
Bug: T206957
Change-Id: I42a0ae8be885ab371ae9bd58c68a0f75b4b3bc79
This copies all of the non-Wikimedia specific entries from Wikimedia's
firejail profile, incluing disallowing access to /sbin and its variants,
important system files and various system utilities. Notably it blocks
access to /run which typically has UNIX sockets that allow for sandbox escape.
The one entry not copied over is disallowing /home because firejail does
that already, and it can cause problems if your development setup is
inside /home, but FirejailCommand already handles all of that appropriately.
Change-Id: I4fd1d3005f18c249b45c9b9a72dff2bef6542b61
Have ->restrict() overwrite any previous restrictions instead of adding
to the existing list. Multiple examples are provided on how this
function should be called going forward.
According to codesearch, all non-test uses of ->restrict() were already
expecting this behavior, passing values like:
Shell::RESTRICT_DEFAULT | Shell::NO_NETWORK
when trying to disable network access.
This is a breaking change, but IMO one that is going to fix more things
than it breaks.
Bug: T257278
Change-Id: I1895d1fc73cc793af2f82001e9d5874b7520f802
For compliance with the new version of the table interface policy
(T255803).
This patch was created by an automated search & replace operation
on the includes/ directory.
Bug: T257789
Change-Id: I17e5e92e24c708ffc846945a136347670a3a20c7
Fixes a regression from 3f94708eff. ExecutableFinder returns false if
the executable isn't found, but CommandFactory was expecting a null response,
This caused autodetection to always think firejail was present.
Adjust CommandFactoryTest to ensure we're always passing a string to
FirejailCommand. We need to switch findFirejail to protected so it can
be mocked.
Bug: T257282
Change-Id: Ie73418ebef6dce2bd5ec18fa38e29219d5bb2fd6
A terminating line break has not been required in wfDebug() since 2014,
however no migration was done. Some of these line breaks found their way
into LoggerInterface::debug() calls, where they mess up the formatting
of the debug log.
So, remove terminating line breaks from wfDebug() and
LoggerInterface::debug() calls.
Also:
* Fix the stripping of leading line breaks from the log header emitted
by Setup.php. This feature, accidentally broken in 2014, allows
requests to be distinguished in the log file.
* Avoid using the global variable $self.
* Move the logging of the client IP back to Setup.php. It was moved to
WebRequest in the hopes that it would not always be needed, however
$wgRequest->getIP() is now called unconditionally a few lines up in
Setup.php. This means that it is put in its proper place after the
"start request" message.
* Wrap the log header code in a closure so that variables like $name do
not leak into global scope.
* In Linker.php, remove a few instances of an unnecessary second
parameter to wfDebug().
Change-Id: I96651d3044a95b9d210b51cb8368edc76bebbb9e
Migrate all callers of Hooks::run() to use the new
HookContainer/HookRunner system.
General principles:
* Use DI if it is already used. We're not changing the way state is
managed in this patch.
* HookContainer is always injected, not HookRunner. HookContainer
is a service, it's a more generic interface, it is the only
thing that provides isRegistered() which is needed in some cases,
and a HookRunner can be efficiently constructed from it
(confirmed by benchmark). Because HookContainer is needed
for object construction, it is also needed by all factories.
* "Ask your friendly local base class". Big hierarchies like
SpecialPage and ApiBase have getHookContainer() and getHookRunner()
methods in the base class, and classes that extend that base class
are not expected to know or care where the base class gets its
HookContainer from.
* ProtectedHookAccessorTrait provides protected getHookContainer() and
getHookRunner() methods, getting them from the global service
container. The point of this is to ease migration to DI by ensuring
that call sites ask their local friendly base class rather than
getting a HookRunner from the service container directly.
* Private $this->hookRunner. In some smaller classes where accessor
methods did not seem warranted, there is a private HookRunner property
which is accessed directly. Very rarely (two cases), there is a
protected property, for consistency with code that conventionally
assumes protected=private, but in cases where the class might actually
be overridden, a protected accessor is preferred over a protected
property.
* The last resort: Hooks::runner(). Mostly for static, file-scope and
global code. In a few cases it was used for objects with broken
construction schemes, out of horror or laziness.
Constructors with new required arguments:
* AuthManager
* BadFileLookup
* BlockManager
* ClassicInterwikiLookup
* ContentHandlerFactory
* ContentSecurityPolicy
* DefaultOptionsManager
* DerivedPageDataUpdater
* FullSearchResultWidget
* HtmlCacheUpdater
* LanguageFactory
* LanguageNameUtils
* LinkRenderer
* LinkRendererFactory
* LocalisationCache
* MagicWordFactory
* MessageCache
* NamespaceInfo
* PageEditStash
* PageHandlerFactory
* PageUpdater
* ParserFactory
* PermissionManager
* RevisionStore
* RevisionStoreFactory
* SearchEngineConfig
* SearchEngineFactory
* SearchFormWidget
* SearchNearMatcher
* SessionBackend
* SpecialPageFactory
* UserNameUtils
* UserOptionsManager
* WatchedItemQueryService
* WatchedItemStore
Constructors with new optional arguments:
* DefaultPreferencesFactory
* Language
* LinkHolderArray
* MovePage
* Parser
* ParserCache
* PasswordReset
* Router
setHookContainer() now required after construction:
* AuthenticationProvider
* ResourceLoaderModule
* SearchEngine
Change-Id: Id442b0dbe43aba84bd5cf801d86dedc768b082c7
Add hook interfaces which were generated by a script which parses
hooks.txt and identifies caller namespaces and directories.
Hook interfaces are mostly placed in a Hook/ subdirectory
relative to the caller location. When there are callers in multiple
directories, a "primary" caller was manually selected. The exceptions to
this are:
* The source root, maintenance and tests, which use includes/Hook. Test
hooks need to be autoloadable in a non-test request so that
implementing test interfaces in a generic handler will not fail.
* resources uses includes/resourceloader/Hook
* The following third-level subdirectories had their hooks placed in
the parent ../Hook:
* includes/filerepo/file
* includes/search/searchwidgets
* includes/specials/forms
* includes/specials/helpers
* includes/specials/pagers
Parameters marked as legacy references in hooks.txt are passed
by value in the interfaces.
Bug: T240307
Change-Id: I6efe2e7dd1f0c6a3d0f4d100a4c34e41f8428720
This is a collection of random bits from my local stashes. This patch
intentionally only touches comments, no code.
Notably:
* Use more specific string[] instead of array, if possible.
* Some comments mention "or null", but miss to list the type.
Change-Id: I712b28964f125c8e3dcb4e3fb993757a09f96644
This is a very limited value class created in just one place, so it
looks like a good candidate for experimenting with strict types.
Change-Id: I777c713f8b3be6688c327f7e6fcf97cc9b7ab66e
This allows us to remove many suppressions for phan false positives.
Bug: T231636
Depends-On: I82a279e1f7b0fdefd3bb712e46c7d0665429d065
Change-Id: I5c251e9584a1ae9fb1577afcafb5001e0dcd41c7
HHVM does not support variadic arguments with type hints. This is
mostly not a big problem, because we can just drop the type hint, but
for some reason PHPUnit adds a type hint of "array" when it creates
mocks, so a class with a variadic method can't be mocked (at least in
some cases). As such, I left alone all the classes that seem like
someone might like to mock them, like Title and User. If anyone wants
to mock them in the future, they'll have to switch back to
func_get_args(). Some of the changes are definitely safe, like
functions and test classes.
In most cases, func_get_args() (and/or func_get_arg(), func_num_args() )
were only present because the code was written before we required PHP
5.6, and writing them as variadic functions is strictly superior. In
some cases I left them alone, aside from HHVM compatibility:
* Forwarding all arguments to another function. It's useful to keep
func_get_args() here where we want to keep the list of expected
arguments and their meanings in the function signature line for
documentation purposes, but don't want to copy-paste a long line of
argument names.
* Handling deprecated calling conventions.
* One or two miscellaneous cases where we're basically using the
arguments individually but want to use them as an array as well for
some reason.
Change-Id: I066ec95a7beb7c0665146195a08e7cce1222c788
