Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Don't double-escape the ellipses in Language::truncateForVisual()

Browse source 
It turns out this gets rid of a bunch of suppressed
"SecurityCheck-DoubleEscaped" that appear to have been accurate
warnings.

There seems to have been some confusion about how ::truncateForVisual()
is supposed to be used; in particular it is to be passed *unescaped*
output, because it is not (generally speaking) safe to truncate
HTML-escaped strings.  The goal of ::truncateForVisual() is to have
a specific number of codepoints in the output for display purposes,
the encoding of those codepoints is not an issue (htmlspecialchars
can be applied to the *return value*.)  If you need a specific number
of *bytes* you should be using ::truncateForDatabase().  If you want
a certain number of *HTML bytes* then the ::truncateHtml() method
is probably what you want.

Slightly refactor some code in RevDelLogItem to avoid a false positive.

Bug: T301205
Bug: T290624
Change-Id: I893362e049aedfa699043fcf27caf4815196f748
This commit is contained in:
C. Scott Ananian 2022-02-07 17:37:56 -05:00
parent 45d6341cab
commit 9e033de4f2
12 changed files with 13 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
includes/Storage/PageUpdater.php
View file

@ -1349,7 +1349,6 @@ class PageUpdater {
// Update recentchanges
if ( !( $this->flags & EDIT_SUPPRESS_RC ) ) {
// Add RC row to the DB
// @phan-suppress-next-line SecurityCheck-DoubleEscaped
RecentChange::notifyEdit(
$now,
$this->getPage(),
@ -1475,7 +1474,6 @@ class PageUpdater {
// Update recentchanges
if ( !( $this->flags & EDIT_SUPPRESS_RC ) ) {
// Add RC row to the DB
// @phan-suppress-next-line SecurityCheck-DoubleEscaped
RecentChange::notifyNew(
$now,
$this->getPage(),

1
includes/actions/MarkpatrolledAction.php
View file

@ -87,7 +87,6 @@ class MarkpatrolledAction extends FormAction {
'diff' => $revId,
'oldid' => $rc->getAttribute( 'rc_last_oldid' )
];
// @phan-suppress-next-line SecurityCheck-DoubleEscaped Triggered by RecentChange::getAttribute
$revlink = $this->linkRenderer->makeLink( $title, $revId, [], $query );
$pagelink = $this->linkRenderer->makeLink( $title, $title->getPrefixedText() );

1
includes/changes/ChangesList.php
View file

@ -685,7 +685,6 @@ class ChangesList extends ContextSource {
$s .= ' <span class="' . $deletedClass . '">' .
$this->msg( 'rev-deleted-user' )->escaped() . '</span>';
} else {
// @phan-suppress-next-line SecurityCheck-DoubleEscaped
$s .= $this->getLanguage()->getDirMark() . Linker::userLink( $rc->mAttribs['rc_user'],
$rc->mAttribs['rc_user_text'] );
$s .= Linker::userToolLinks(

1
includes/changes/RCCacheEntryFactory.php
View file

@ -285,7 +285,6 @@ class RCCacheEntryFactory {
$userLink = ' <span class="history-deleted">' .
$this->context->msg( 'rev-deleted-user' )->escaped() . '</span>';
} else {
// @phan-suppress-next-line SecurityCheck-DoubleEscaped Triggered by Linker?
$userLink = Linker::userLink(
$cacheEntry->mAttribs['rc_user'],
$cacheEntry->mAttribs['rc_user_text'],

1
includes/changes/RecentChange.php
View file

@ -1172,7 +1172,6 @@ class RecentChange implements Taggable {
*/
public function getAttribute( $name ) {
if ( $name === 'rc_comment' ) {
// @phan-suppress-next-line SecurityCheck-DoubleEscaped
return CommentStore::getStore()
->getComment( 'rc_comment', $this->mAttribs, true )->text;
}

2
includes/export/XmlDumpWriter.php
View file

@ -377,7 +377,6 @@ class XmlDumpWriter {
} else {
if ( $rev->getComment()->text != '' ) {
$out .= " "
// @phan-suppress-next-line SecurityCheck-DoubleEscaped getComment is polluted by truncate
. Xml::elementClean( 'comment', [], strval( $rev->getComment()->text ) )
. "\n";
}
@ -601,7 +600,6 @@ class XmlDumpWriter {
} else {
$comment = CommentStore::getStore()->getComment( 'log_comment', $row )->text;
if ( $comment != '' ) {
// @phan-suppress-next-line SecurityCheck-DoubleEscaped CommentStore is polluted by truncate
$out .= " " . Xml::elementClean( 'comment', null, strval( $comment ) ) . "\n";
}
}

1
includes/gallery/TraditionalImageGallery.php
View file

@ -264,7 +264,6 @@ class TraditionalImageGallery extends ImageGalleryBase {
// Preloaded into LinkCache in toHTML
return $linkRenderer->makeKnownLink(
$nt,
// @phan-suppress-next-line SecurityCheck-DoubleEscaped Triggered by Language::truncateForVisual
is_int( $this->getCaptionLength() ) ?
$lang->truncateForVisual( $nt->getText(), $this->getCaptionLength() ) :
$nt->getText(),

9
includes/language/Language.php
View file

@ -3528,6 +3528,13 @@ class Language {
* This provides multibyte version of truncateForDatabase() method of this class,
* suitable for truncation based on number of characters, instead of number of bytes.
*
* The input should be a raw UTF-8 string and *NOT* be HTML
* escaped. It is not safe to truncate HTML-escaped strings,
* because the entity can be truncated! Use ::truncateHtml() if you
* need a specific number of HTML-encoded bytes, or
* ::truncateForDatabase() if you need a specific number of PHP
* bytes.
*
* If $length is negative, the string will be truncated from the beginning.
*
* @since 1.31
@ -3575,7 +3582,7 @@ class Language {
# Use the localized ellipsis character
if ( $ellipsis == '...' ) {
$ellipsis = wfMessage( 'ellipsis' )->inLanguage( $this )->escaped();
$ellipsis = wfMessage( 'ellipsis' )->inLanguage( $this )->text();
}
if ( $length == 0 ) {
return $ellipsis; // convention

3
includes/logging/LogPage.php
View file

@ -125,7 +125,6 @@ class LogPage {
if ( $this->updateRecentChanges ) {
$titleObj = SpecialPage::getTitleFor( 'Log', $this->type );
// @phan-suppress-next-line SecurityCheck-DoubleEscaped
RecentChange::notifyLog(
$now, $titleObj, $this->performer, $this->getRcComment(), '',
$this->type, $this->action, $this->target, $this->comment,
@ -140,7 +139,6 @@ class LogPage {
// Notify external application via UDP.
// We send this to IRC but do not want to add it the RC table.
$titleObj = SpecialPage::getTitleFor( 'Log', $this->type );
// @phan-suppress-next-line SecurityCheck-DoubleEscaped
$rc = RecentChange::newLogEntry(
$now, $titleObj, $this->performer, $this->getRcComment(), '',
$this->type, $this->action, $this->target, $this->comment,
@ -362,7 +360,6 @@ class LogPage {
$this->performer = $performer;
// @phan-suppress-next-line SecurityCheck-DoubleEscaped
$logEntry = new ManualLogEntry( $this->type, $action );
$logEntry->setTarget( $target );
$logEntry->setPerformer( $performer );

1
includes/rcfeed/IRCColourfulRCFeedFormatter.php
View file

@ -100,7 +100,6 @@ class IRCColourfulRCFeedFormatter implements RCFeedFormatter {
$flag = $attribs['rc_log_action'];
} else {
$store = MediaWikiServices::getInstance()->getCommentStore();
// @phan-suppress-next-line SecurityCheck-DoubleEscaped
$comment = self::cleanupForIRC( $store->getComment( 'rc_comment', $attribs )->text );
$flag = '';
if ( !$attribs['rc_patrolled']

8
includes/revisiondelete/RevDelLogItem.php
View file

@ -19,6 +19,7 @@
* @ingroup RevisionDelete
*/
use MediaWiki\MediaWikiServices;
use MediaWiki\Revision\RevisionRecord;
/**
@ -128,9 +129,10 @@ class RevDelLogItem extends RevDelItem {
// User links and action text
$action = $formatter->getActionText();
$comment = $this->commentStore->getComment( 'log_comment', $this->row )->text;
$comment = $this->list->getLanguage()->getDirMark()
. Linker::commentBlock( $comment );
$commentRaw = $this->commentStore->getComment( 'log_comment', $this->row )->text;
$commentFormatter = MediaWikiServices::getInstance()->getCommentFormatter();
$dirMark = $this->list->getLanguage()->getDirMark();
$comment = $dirMark . $commentFormatter->formatBlock( $commentRaw );
if ( LogEventsList::isDeleted( $this->row, LogPage::DELETED_COMMENT ) ) {
$comment = '<span class="history-deleted">' . $comment . '</span>';

1
includes/search/SearchHighlighter.php
View file

@ -556,7 +556,6 @@ class SearchHighlighter {
$found = $m[2];
// @phan-suppress-next-line SecurityCheck-DoubleEscaped Triggered by Language::truncateForVisual
$line = htmlspecialchars( $pre . $found . $post );
$pat2 = '/(' . $terms . ")/i";
$line = preg_replace( $pat2, "<span class='searchmatch'>\\1</span>", $line );