Make WordPress Core

Opened 6 weeks ago

Closed 2 weeks ago

Last modified 2 weeks ago

#65785 closed enhancement (fixed)

No safe way to add XML namespaces to feeds

Reported by: pfefferle Owned by: westonruter
Priority: normal Milestone: 7.2
Component: Feeds Version: 2.8
Severity: normal Keywords: has-patch has-unit-tests commit
Cc: Focuses:

Description (last modified by westonruter)

rss2_ns and atom_ns are echo-only actions. Callbacks print their attributes directly into the <rss> / <feed> element, so a callback can not know what was already printed. When two callbacks add the same namespace, the element ends up with a duplicate attribute:

<rss version="2.0" xmlns:source="http://source.scripting.com/" xmlns:source="http://source.scripting.com/" >

A duplicate attribute is a well-formedness error in XML, and conforming parsers treat it as fatal. Some feed readers recover from it, but every strict consumer refuses the whole feed: browsers that render the feed as XML, and everything based on XSLT, which needs a proper XML parse before it can transform.

XML Parsing Error: duplicate attribute

This is easy to hit: two plugins (or a plugin and a theme) that support the same standard will add the same namespace, and both do nothing wrong. There is no API to check for it, so the only defensive option is to buffer the complete action output. This works, but it is more a hack than a solution, and it only helps if everyone does it.

Maybe we could add a filter with the prefix as key, next to the existing actions:

<?php
$namespaces = apply_filters( "{$type}_ns_list", array() );
foreach ( $namespaces as $prefix => $uri ) {
    printf( ' xmlns:%s="%s"', sanitize_key( $prefix ), esc_url( $uri ) );
}
do_action( "{$type}_ns" );

With the prefix as key, duplicates would not be possible anymore, and the actions could stay for backwards compatibility. The same would work for rdf_ns, rss2_comments_ns and atom_comments_ns.

Change History (13)

#1 @abcd95
6 weeks ago

Hey there! ๐Ÿ‘‹

Introducing a new associative array-based filter (e.g., {$type}_ns_list) is the best path forward:

  • Using an array [ 'prefix' => 'uri' ] inherently deduplicates prefixes.
  • Core can centrally handle sanitization (sanitize_key() and esc_url()) before printing.
  • We preserve backward compatibility by applying the filter *before* do_action( "{$type}_ns" ).

This aligns perfectly with how we already securely construct dynamic attributes in other areas (like filtering body_class before output). I'll attach a patch for this.

This ticket was mentioned in โ€‹PR #12812 on โ€‹WordPress/wordpress-develop by โ€‹@pfefferle.


6 weeks ago
#2

  • Keywords has-patch has-unit-tests added

The rss2_ns and atom_ns actions are echo-only, so two plugins that add the same namespace produce a duplicate attribute, and the whole feed becomes invalid XML.

This adds get_feed_namespaces() and feed_namespaces() (named after feed_content_type()) and a wp_feed_namespaces filter. Namespaces are keyed by their prefix, so the same namespace can not be printed twice. The default namespaces of the templates moved into the filtered list, otherwise a plugin could still duplicate them. The *_ns actions stay untouched for backwards compatibility.

What it does not solve: a plugin that still echoes a namespace via the old actions can produce a duplicate, like before. That would need output buffering around the actions, and I did not want to go there.

#3 @pfefferle
6 weeks ago

  • Keywords has-patch has-unit-tests removed

Hey @abcd95 I also have prepared a PR. Will also push it right now.

#4 @pfefferle
6 weeks ago

  • Keywords has-patch has-unit-tests added

#5 @westonruter
5 weeks ago

  • Component General โ†’ Feeds
  • Description modified (diff)
  • Milestone Awaiting Review โ†’ Future Release

โ€‹@westonruter commented on โ€‹PR #12812:


5 weeks ago
#6

This is looking good to me. My only question is whether we should prefix the functions with wp_. There is wp_title_rss() already. The new wp_feed_namespaces filter has it too. So why not wp_feed_namespaces() and wp_get_feed_namespaces() too?

โ€‹@pfefferle commented on โ€‹PR #12812:


3 weeks ago
#7

@westonruter I think both are fine. I was using the pattern of feed_content_type and get_feed_build_date. But I also was not consistent and might have to rename the filter if we keep it without wp_.

โ€‹@westonruter commented on โ€‹PR #12812:


2 weeks ago
#9

@westonruter do you think we can ignore URNs for now? โ€‹#12812 (comment)

@pfefferle Sorry for the delay. โ€‹Yes!

โ€‹@pfefferle commented on โ€‹PR #12812:


2 weeks ago
#10

Sorry for the delay.

No problem :)

#11 @westonruter
2 weeks ago

  • Keywords commit added
  • Milestone Future Release โ†’ 7.2
  • Owner set to westonruter
  • Status new โ†’ reviewing

#12 @westonruter
2 weeks ago

  • Resolution โ†’ fixed
  • Status reviewing โ†’ closed

In 63394:

Feeds: Add a filterable list of XML namespaces.

The rss2_ns, rss2_comments_ns, rdf_ns, atom_ns, and atom_comments_ns actions are echo-only: a callback prints its xmlns attribute straight into the feed's root element with no way to know what another callback already printed. Two plugins declaring the same namespace therefore emit a duplicate attribute, which is a well-formedness error, so every strict consumer rejects the whole feed rather than just ignoring the repetition.

The new wp_get_feed_namespaces() and wp_feed_namespaces() functions build and print that list from a wp_feed_namespaces filter, keyed by prefix so the same prefix cannot be declared twice. Prefixes are validated as XML names, with the reserved xml and xmlns rejected, and namespace URIs are escaped on output. The namespaces previously hard-coded in the bundled feed templates now come from this list and are merged back in after the filter runs, so a callback can neither remove them nor duplicate them.

The *_ns actions are left in place and still fire, so a plugin echoing a namespace through one of them behaves exactly as before. Their documentation now points to the filter as the safer alternative.

Developed in โ€‹https://github.com/WordPress/wordpress-develop/pull/12812.

Props pfefferle, westonruter, abcd95.
Fixes #65785.

#13 @westonruter
2 weeks ago

  • Version โ†’ 2.8
Note: See TracTickets for help on using tickets.