Move Linker::makeExternalLink to the LinkRenderer service, as has been
done with the other static methods of Linker.
In order to allow phan's SecurityCheckPlugin to perform a more accurate
analysis of taintedness, tweak the API of Linker::makeExternalLink to
clearly indicate via the type system whether the link text has already
been escaped or not: a `string` argument will always be escaped, and
if the argument is already escaped it should be passed as an HtmlArmor
object. In refactoring, `Message` arguments were also common, and accept
them as-is to avoid the caller having to think about whether to call
Message::text() or Message::escaped().
This allows us to provide a more precise taint type to the $text argument,
avoids an opaque boolean argument, and avoids spurious errors from
SecurityCheck.
We also require the caller to explicitly pass a Title context, instead
of implicitly relying on the global $wgTitle. This works cleanly
everywhere except for CommentParser, which has a $selfLinkTarget which
generally works as the title context for the external link, but which
is nullable. The original Linker::makeExternalLink() used $wgTitle as
a fallback, but $wgTitle can also be null in some circumstances. The
title context only determines how $wgNoFollowNsExceptions is handled,
so existing code basically just ignored $wgNoFollowNsExceptions when
$wgTitle was null, which isn't terrible. A future refactor could/should
clean up CommentParser to ensure that there is always a non-null title
context that can be used.
Change-Id: I9bcf4780f388ba639a9cc882dd9dd42eda5736ae
CVE-2024-PENDING
A malicious editor could potentially create an edit summary or log
summary with links such that a link marker is in an attribute which
will then get replaced allowing the content to break out of the
attribute leading to an XSS.
Bug: T355538
Change-Id: If20a8a95e84bb2f6e132bdda4907e3db6f133a8e
Make phan stricter about conditional variable declaration
Remaining false positive issues are suppressed.
The suppression and the setting change can only be done together
Bug: T259172
Change-Id: I1f200ac37df7448453688bf464a8250c97313e5d
Make phan stricter about array keys
Remaining false positive issues are suppressed.
The suppression and the setting change can only be done together
Bug: T304887
Depends-On: I3105a5fd4826f8667b5232834defc5ec93be32a1
Depends-On: Ie9610a6e83731468311edb3ed17f80fc509de385
Change-Id: I701f12ab94478c3b8e7fd82110ade74a8e6b04ef
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
* Some functions accept only string, cast ints and floats to string
* After preg_matches or explode() casts numbers to int to do maths
* Cast unix timestamps to int to do maths
* Cast return values from timestamp format function to int
* Cast bitwise operator to bool when needed as bool
* php internal functions like floor/round/ceil documented to return
float, most cases the result is used as int, added casts
Found by phan strict checks
Change-Id: Icb2de32107f43817acc45fe296fb77acf65c1786
Check the more common case (local cached title) first, before
calling isAlwaysKnown which can be expensive due to hooks.
Follows-up Ica8733fb4a890fd2d2fc37eb85657c3715805133.
Bug: T293665
Change-Id: I8eb6144a3f1de5ebf9b4bd44e4021f9b6837f442
Existence of global userpages (or similar nonlocal pages) can only be
known if the relevant title hook is involved, but LinkBatch is caching
these pages as bad links immediately after querying the local database.
CommentParser is then relying on this information to treat them as
always bad; thus preempting any further checks that might be done by
LinkRenderer to properly account for their magical existence.
Now title always-known status will be checked, to preempt bad linking.
Bug: T293665
Change-Id: Ica8733fb4a890fd2d2fc37eb85657c3715805133
* In LinkBatch::addObj(), reject interwiki links with a warning.
Otherwise the link is added to the batch by ns/title and later
reconstructed as if it were a local link without an interwiki
prefix.
* In CommentParser, treat interwiki links as always good, don't defer
the existence check.
* In LinkBatch, inject a LoggerInstance instead of calling LoggerFactory
in four places.
* Add a regression test, and some general tests for known links.
Bug: T300311
Change-Id: I0e5825eb48a6ba2932aea69a4d0fff3439c50ff5
CommentParser:
* Move comment formatting backend from Linker to a CommentParser service.
Allow link existence and file existence to be batched.
* Rename $local to $samePage since I think that is clearer.
* Rename $title to $selfLinkTarget since it was unclear what the title
was used for.
* Rename the "autocomment" concept to "section link" in public
interfaces, although the old term remains in CSS classes.
* Keep unsafe HTML pass-through in separate "unsafe" methods, for easier
static analysis and code review.
CommentFormatter:
* Add CommentFormatter and RowCommentFormatter services as a usable
frontend for comment batches, and to replace the Linker static methods.
* Provide fluent and parametric interfaces.
Linker:
* Remove Linker::makeCommentLink() without deprecation -- nothing calls
it and it is obviously an internal helper.
* Soft-deprecate Linker methods formatComment(), formatLinksInComment(),
commentBlock() and revComment().
Caller migration:
* CommentFormatter single: Linker, RollbackAction, ApiComparePages,
ApiParse
* CommentFormatter parametric batch: ImageHistoryPseudoPager
* CommentFormatter fluent batch: ApiQueryFilearchive
* RowCommentFormatter sequential: History feed, BlocklistPager,
ProtectedPagesPager, ApiQueryProtectedTitles
* RowCommentFormatter with index: ChangesFeed, ChangesList,
ApiQueryDeletedrevs, ApiQueryLogEvents, ApiQueryRecentChanges
* RevisionCommentBatch: HistoryPager, ContribsPager
Bug: T285917
Change-Id: Ia3fd50a4a13138ba5003d884962da24746d562d0
