WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 7 months ago

Last modified 4 months ago

#46026 closed defect (bug) (fixed)

WSODs protection returns incorrect content type for XML Requests

Reported by: spacedmonkey Owned by: spacedmonkey
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.1
Component: Site Health Keywords: has-patch needs-testing servehappy
Focuses: multisite Cc:
PR Number:

Description

Similar to #45933, calls to wp_die do not return the correct content type, when the requests resource in xml, such RSS feeds, trackbacks and other xml requests such as sitemaps (via a plugin like yoast). Returning the correct content type can have serious issues for third party tools that request say a feed and get a html page in return.

Attachments (3)

46026.diff (3.8 KB) - added by spacedmonkey 9 months ago.
46026.2.diff (3.2 KB) - added by spacedmonkey 9 months ago.
46026.3.diff (3.3 KB) - added by spacedmonkey 9 months ago.

Download all attachments as: .zip

Change History (30)

@spacedmonkey
9 months ago

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


9 months ago

#2 @spacedmonkey
9 months ago

@birgire Thanks for the feedback, I have fixed those issues in my latest patch here.

I have also added, the following changes.

  1. Now checks to see if is_feed and is_trackback return true.
  2. Added more content type to accepted list for xml + oembed.

#3 @afragen
9 months ago

Does the output need to be escaped in some fashion?

#4 @birgire
9 months ago

Thanks @spacedmonkey

@afragen What about using <![CDATA[...]]> for e.g. the external input of $message?

Some minor things I noticed in 46026.diff:

  • It would be nice to add more information to the parameter's description of _wp_wrap_xml(), like the type and description.
  • Missing dot at the end of the _wp_wrap_xml() description.
  • Extra new lines at the end of wp_is_xml_request() and the indentation might need some adjustments.
  • Should we format the wp_die in the description of _xml_wp_die_handler(), e.g. add a link for the doc parsing, like {@see wp_die} ?
  • The function wp_is_xml_request() is not complicated, but it will be a part of the public API, so should unit tests be added for it?

I would also have suggested using e.g. "halt" instead of "kill", but I see that the latter is already used by core :-)

Not related to this ticket, but I don't see any is_callable() on the filterable callbacks in wp_die().

#5 @pento
9 months ago

  • Milestone changed from Awaiting Review to 5.1

Setting Milestone to 5.1 for review.

This ticket was mentioned in Slack in #core-php by flixos90. View the logs.


9 months ago

#7 @flixos90
9 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


9 months ago

#9 @birgire
9 months ago

In 46026.2.diff there's a typo in _xml_wp_die_handler() regarding the content type. Should be e.g. text/xml rather than application/json.

The optional error title input is not used in the output of _xml_wp_die_handler(), so I wonder if the error title input should be handled in a similar way as the error message, i.e. with it's own element:

<error>
    <title>...</title>
    <message>...</message>
    ...
</error>

I also wonder if _json_wp_die_handler() should add the error title to the output.

I like the simplification in 46026.2.diff compared to 46026.diff regarding skipping the array2xml processing and just construct the xml directly.

#10 @spacedmonkey
9 months ago

Thanks for your feedback @birgire

In 46026.3.diff are the following changes

  • Now returns <title> node.
  • Now returns content type xml/text
  • Also checks is_comment_feed
  • Load.php now has a empty line at the end.


This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


9 months ago

#13 @flixos90
9 months ago

  • Milestone changed from 5.1 to 5.2

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


9 months ago

#15 @flixos90
7 months ago

  • Milestone changed from 5.2 to 5.3

#16 @flixos90
7 months ago

  • Milestone 5.3 deleted
  • Resolution set to invalid
  • Status changed from assigned to closed

This ticket is based on the old fatal error recovery mode implementation and will be covered as part of #46130.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


7 months ago

#18 @spacedmonkey
7 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Reopening as still valid and not part of #46130.

#19 @SergeyBiryukov
7 months ago

  • Milestone set to 5.3

Restoring the milestone.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


7 months ago

#22 @SergeyBiryukov
7 months ago

  • Milestone changed from 5.3 to 5.2

Moving to 5.2, per the latest #core-php chat.

#23 @SergeyBiryukov
7 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 45016:

Bootstrap/Load: Add support for XML requests to wp_die().

In addition to AJAX, XML-RPC, JSON, and JSONP requests, wp_die() now handles XML requests correctly, returning information in the expected content type.

Props spacedmonkey, birgire.
Fixes #46026. See #44458.

#24 @SergeyBiryukov
7 months ago

In 45017:

Docs: Improve documentation for wp_die() handlers after [45015] and [45016].

See #46543, #46025, #46026.

#25 @SergeyBiryukov
7 months ago

In 45018:

Docs: Fix typo in wp_is_xml_request() description.

See #46026.

#27 @spacedmonkey
4 months ago

  • Component changed from Bootstrap/Load to Site Health
Note: See TracTickets for help on using tickets.