Page MenuHomePhabricator

CI Phan check blocking CentralNotice patches
Closed, ResolvedPublic

Description

I don't know what changed to cause this... See https://integration.wikimedia.org/ci/job/mwext-php72-phan-docker/59603/console for details. Many thanks in advance for any input!!!

Event Timeline

<error line="188" severity="warning" message="Calling method \htmlspecialchars() in \CentralNoticePageLogPager::formatRow that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/CommentStoreComment.php +85; ../../includes/CommentStore.php +430)" source="SecurityCheck-DoubleEscaped"/>

<error line="191" severity="warning" message="Calling method \htmlspecialchars() in \CentralNoticePageLogPager::formatRow that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/CommentStoreComment.php +85; ../../includes/CommentStore.php +430)" source="SecurityCheck-DoubleEscaped"/>

Could you link to the patch?

Umherirrender added subscribers: Daimona, Umherirrender.

https://gerrit.wikimedia.org/r/#/c/606371/

[You can find the patch set of the output by looking at the status page in the left navigation]

Judging from the caused-by lines, this seems a false positive that can be suppressed. Taint-check is probably confused by CommentStore::createComment, which puts an escaped value in CommentStoreComment::text before inserting the value in the DB. However, in the read context of CentralNoticePageLogPager the value should be considered tainted, and thus escaped.

The underlying issue should probably be resolved by putting in CommentStoreComment::text values with consistent levels of escaping. IMHO the property should hold unsafe strings, so it's the core call to truncateForVisual that should be changed. Even more: Language::truncateForVisual probably shouldn't return an escaped string. Currently, it does so because it's using $ellipsis = wfMessage( 'ellipsis' )->inLanguage( $this )->escaped(); (in truncateInternal), but other than that, it's not escaping anything else in its return value. Instead, truncateInternal should return an unsafe string, and escaping should be up to the caller.

Change 607149 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@master] Suppress DoubleEscaped phan warning in CentralNoticePageLogPager

https://gerrit.wikimedia.org/r/607149

Change 607149 merged by Ejegg:
[mediawiki/extensions/CentralNotice@master] Suppress DoubleEscaped phan warning in CentralNoticePageLogPager

https://gerrit.wikimedia.org/r/607149