Page MenuHomePhabricator

phan-taint-check regression in LiquidThreads
Closed, ResolvedPublic

Description

Using 1.4.0:

<checkstyle version="6.5">
  <file name="./api/ApiQueryLQTThreads.php">
    <error line="248" severity="warning" message="Calling method \OutputPage::addHTML() in \ApiQueryLQTThreads::renderThread that outputs using tainted argument $oldOutputText. (Caused by: Builtin-\OutputPage::addHTML) (Caused by: ./api/ApiQueryLQTThreads.php +211)" source="SecurityCheck-XSS"/>
  </file>
  <file name="./api/ApiThreadAction.php">
    <error line="606" severity="warning" message="Calling method \OutputPage::addHTML() in \ApiThreadAction::renderThreadPostAction that outputs using tainted argument $oldOutputText. (Caused by: Builtin-\OutputPage::addHTML) (Caused by: ./api/ApiThreadAction.php +592)" source="SecurityCheck-XSS"/>
  </file>
</checkstyle>

Both cases are basically using OutputPage as an output buffer:

		// Set up OutputPage
		$out = $this->getOutput();
		$oldOutputText = $out->getHTML();
		$out->clearHTML();

		...

		$result = $out->getHTML();
		$out->clearHTML();
		$out->addHTML( $oldOutputText );

Event Timeline

Bawolff claimed this task.

This was fixed by suppressing the errors. See T203690

The errors do make sense though kind of, If anyone outputted anything XSS-y (in another extension, or in MW core, due to say a false positive), then getHtml() should be considered tainted, and putting it back into addHtml() should be considered an xss

sbassett triaged this task as Medium priority.Oct 15 2019, 7:10 PM