Make WordPress Core

Opened 6 months ago

Closed 5 months ago

#65047 closed defect (bug) (fixed)

Missing escaping for XML error message

Reported by: maheshpatel Owned by: SergeyBiryukov
Priority: normal Milestone: 7.1
Component: Pings/Trackbacks Version:
Severity: normal Keywords: has-patch
Cc: Focuses:

Description (last modified by sabernhardt)

Current Code:

  if ( $error ) {
      echo '<?xml version="1.0" encoding="utf-8"?' . ">\n";
      echo "<response>\n";
      echo "<error>1</error>\n";
      echo "<message>$error_message</message>\n";
      echo '</response>';
  }

Fix:

  if ( $error ) {
      echo '<?xml version="1.0" encoding="utf-8"?' . ">\n";
      echo "<response>\n";
      echo "<error>1</error>\n";
      echo "<message>" . esc_html ( $error_message ) . "</message>\n";
      echo '</response>';
  }

Why It Matters:

  • XML special characters (&, <, >) can break XML parsing
  • Error messages come from user actions or system states

Change History (8)

This ticket was mentioned in ​PR #11527 on ​WordPress/wordpress-develop by ​maheshpatel27.


6 months ago
#1

  • Keywords has-patch added

Add escaping [src/wp-trackback.php#L37 src/wp-trackback.php]

if ( $error ) {

echo '<?xml version="1.0" encoding="utf-8"?' . ">\n";
echo "<response>\n";
echo "<error>1</error>\n";
echo "<message>" . esc_html ( $error_message ) . "</message>\n";
echo '</response>';

}

#2 @maheshpatel
6 months ago

This issue has been resolved and generated PR
PR - ​https://github.com/WordPress/wordpress-develop/pull/11527

#3 @sabernhardt
6 months ago

  • Description modified (diff)
  • Version trunk

The trackback_response() function has been available since changeset 8, and the escaping functions were added later.

Would esc_xml() be a better choice?

#4 @SergeyBiryukov
6 months ago

  • Milestone Awaiting Review → 7.1

​@westonruter commented on ​PR #11527:


6 months ago
#5

I would say that this warrants adding a unit test, but it doesn't seem there are any unit tests for the functionality in wp-trackback.php.

​@pbiron commented on ​PR #11527:


6 months ago
#6

I would say that this warrants adding a unit test, but it doesn't seem there are any unit tests for the functionality in wp-trackback.php.

Agreed...wish I had time to work on that ;-(

Here's some info for whoever does write unit tests:

  • the main diff between esc_html() and esc_xml() is that esc_xml() leaves ​CDATA Sections intact. That is, if the $error_message contained something like <![CDATA[<greeting>Hello, world!</greeting>]]>, esc_html() would replace the < and > chars with &lt; and &gt;, whereas esc_xml() won't.
  • so, it'd be good if the unit tests included $error_message's with and without CDATA Sections

​@westonruter commented on ​PR #11527:


6 months ago
#7

I don't consider tests to be a blocker here.

#8 @SergeyBiryukov
5 months ago

  • Owner set to SergeyBiryukov
  • Resolution → fixed
  • Status new → closed

In 62414:

Pings/Trackbacks: Escape the XML error message in trackback_response().

Props maheshpatel, pbiron, sabernhardt, westonruter, SergeyBiryukov.
Fixes #65047.

Note: See TracTickets for help on using tickets.