Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 43 additions & 15 deletions src/wp-includes/media.php
Original file line number Diff line number Diff line change
Expand Up @@ -6736,24 +6736,38 @@ function wp_add_crossorigin_attributes( string $html ): string {
'SOURCE' => array( 'src' ),
);

while ( $processor->next_tag() ) {
/*
* Name of the bookmark on the AUDIO or VIDEO element the cursor is inside of,
* for as long as that element still needs the attribute. Closing tags are
* visited so the bookmark cannot outlive its element and be mistaken for the
* parent of a later SOURCE.
*/
$media_bookmark = null;

while ( $processor->next_tag( array( 'tag_closers' => 'visit' ) ) ) {
$tag = $processor->get_tag();

if ( ! isset( $cross_origin_tag_attributes[ $tag ] ) ) {
continue;
}
$crossorigin = $processor->get_attribute( 'crossorigin' );
if ( null !== $crossorigin ) {
continue;
}

if ( 'AUDIO' === $tag || 'VIDEO' === $tag ) {
$processor->set_bookmark( 'audio-video-parent' );
$media_bookmark = null;

if ( ! $processor->is_tag_closer() && null === $processor->get_attribute( 'crossorigin' ) ) {
$media_bookmark = 'audio-video-parent';
$processor->set_bookmark( $media_bookmark );
}
}

$processor->set_bookmark( 'resume' );
if ( $processor->is_tag_closer() ) {
continue;
}

$sought = false;
$crossorigin = $processor->get_attribute( 'crossorigin' );
if ( null !== $crossorigin ) {
continue;
}

$is_cross_origin = false;

Expand All @@ -6770,18 +6784,32 @@ function wp_add_crossorigin_attributes( string $html ): string {

if ( $is_cross_origin ) {
if ( 'SOURCE' === $tag ) {
$sought = $processor->seek( 'audio-video-parent' );
if ( null !== $media_bookmark ) {
$processor->set_bookmark( 'resume' );

/*
* A seek past MAX_SEEK_OPS leaves the cursor where it is, so mark the
* parent only once the cursor has reached it. Marking a SOURCE does
* nothing for CORS and would hide the media element that was missed.
*/
if ( $processor->seek( $media_bookmark ) ) {
$processor->set_attribute( 'crossorigin', 'anonymous' );
$processor->seek( 'resume' );
}

if ( $sought ) {
$processor->set_attribute( 'crossorigin', 'anonymous' );
/*
* The element is either marked or past the seek budget, which does not
* come back, so its remaining SOURCE children need nothing.
*/
$media_bookmark = null;
Comment on lines +6788 to +6804

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude went through the seek path here, findings below:

WP_HTML_Tag_Processor::seek() returns false without moving the cursor once MAX_SEEK_OPS (1000) is passed. With the return value unchecked, the cursor is still on the SOURCE when set_attribute() runs, so crossorigin lands on the SOURCE - where it does nothing for CORS - and the parent media element is left unmarked. Trunk guarded this with $sought.

Reproduced locally with intdiv( WP_HTML_Tag_Processor::MAX_SEEK_OPS, 2 ) + 1 copies of <video><source src="https://external.example.com/video.mp4" /></video>. Marking a parent costs two seeks, so the last pair goes over budget: the attribute winds up on the <source>, and seek() raises _doing_it_wrong() from inside the output buffer - the same failure this PR is fixing.

Suggested change
$processor->set_bookmark( 'resume' );
$processor->seek( $media_bookmark );
$processor->set_attribute( 'crossorigin', 'anonymous' );
$processor->seek( 'resume' );
// The element is marked, so further SOURCE children need nothing.
$media_bookmark = null;
$processor->set_bookmark( 'resume' );
/*
* A seek past MAX_SEEK_OPS leaves the cursor where it is, so mark
* the parent only once the cursor has actually reached it. Marking
* a SOURCE does nothing for CORS and would hide the missed parent.
*/
if ( $processor->seek( $media_bookmark ) ) {
$processor->set_attribute( 'crossorigin', 'anonymous' );
$processor->seek( 'resume' );
// The element is marked, so further SOURCE children need nothing.
$media_bookmark = null;
}

A test that fails without the guard and passes with it:

	/**
	 * A failed seek() must not leave the crossorigin attribute on the SOURCE element.
	 *
	 * WP_HTML_Tag_Processor::seek() returns false without moving the cursor once
	 * MAX_SEEK_OPS is exceeded. The cursor is then still on the SOURCE, so marking
	 * the element without checking the return value adds the attribute where it does
	 * nothing for CORS and leaves the parent media element unmarked.
	 *
	 * @ticket 65930
	 *
	 * @covers ::wp_add_crossorigin_attributes
	 */
	public function test_source_is_not_marked_when_seeking_the_parent_fails() {
		$this->setExpectedIncorrectUsage( 'WP_HTML_Tag_Processor::seek' );

		/*
		 * Marking a media element costs two seeks, one back to the parent and one
		 * forward to resume, so one more than half the budget outlasts it.
		 */
		$media_elements = intdiv( WP_HTML_Tag_Processor::MAX_SEEK_OPS, 2 ) + 1;

		$output = wp_add_crossorigin_attributes(
			str_repeat(
				'<video><source src="https://external.example.com/video.mp4" /></video>',
				$media_elements
			)
		);

		$this->assertSame(
			0,
			preg_match_all( '/<source\b[^>]*\bcrossorigin\b/i', $output ),
			'A SOURCE element must never receive the crossorigin attribute.'
		);

		$this->assertSame(
			$media_elements - 1,
			substr_count( $output, 'crossorigin="anonymous"' ),
			'Every media element reachable within the seek budget should have been marked.'
		);
	}

With both applied the class is green here, 30 tests and 38 assertions.

One thing the guard does not cover: a seek over budget still raises _doing_it_wrong() inside the display handler, so a page with 500+ marked media elements can still take the white screen. That is trunk's behavior too, so probably its own follow up rather than something for this PR.

Does that hold up on your end?

}
} else {
$processor->set_attribute( 'crossorigin', 'anonymous' );
}

if ( $sought ) {
$processor->seek( 'resume' );
$processor->release_bookmark( 'audio-video-parent' );
// A marked AUDIO or VIDEO needs nothing from its SOURCE children.
if ( 'AUDIO' === $tag || 'VIDEO' === $tag ) {
$media_bookmark = null;
}
}
}
}
Expand Down
52 changes: 52 additions & 0 deletions tests/phpunit/tests/media/wpCrossOriginIsolation.php
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ public function test_set_flag_preserves_upload_media_module_dependencies() {
* Verifies that cross-origin elements get crossorigin="anonymous" added.
*
* @ticket 64766
* @ticket 65930
*
* @runInSeparateProcess
* @preserveGlobalState disabled
Expand Down Expand Up @@ -409,6 +410,9 @@ public function data_elements_that_should_get_crossorigin() {
'cross-origin source inside video' => array(
'<video><source src="https://external.example.com/video.mp4" type="video/mp4" /></video>',
),
'multiple cross-origin sources' => array(
'<video><source src="https://external.example.com/video.mp4" type="video/mp4" /><source src="https://external.example.com/video.webm" type="video/webm" /></video>',
),
);
}

Expand All @@ -420,6 +424,7 @@ public function data_elements_that_should_get_crossorigin() {
* in credentialless mode without needing explicit CORS headers.
*
* @ticket 64766
* @ticket 65930
*
* @runInSeparateProcess
* @preserveGlobalState disabled
Expand All @@ -442,6 +447,47 @@ public function test_output_buffer_does_not_add_crossorigin( $html ) {
$this->assertStringNotContainsString( 'crossorigin="anonymous"', $output );
}

/**
* A failed seek() must not leave the crossorigin attribute on the SOURCE element.
*
* WP_HTML_Tag_Processor::seek() returns false without moving the cursor once
* MAX_SEEK_OPS is exceeded. The cursor is then still on the SOURCE, so marking
* the element without checking the return value adds the attribute where it does
* nothing for CORS and leaves the parent media element unmarked.
*
* @ticket 65930
*
* @covers ::wp_add_crossorigin_attributes
*/
public function test_source_is_not_marked_when_seeking_the_parent_fails() {
$this->setExpectedIncorrectUsage( 'WP_HTML_Tag_Processor::seek' );

/*
* Marking a media element costs two seeks, one back to the parent and one
* forward to resume, so one more than half the budget outlasts it.
*/
$media_elements = intdiv( WP_HTML_Tag_Processor::MAX_SEEK_OPS, 2 ) + 1;

$output = wp_add_crossorigin_attributes(
str_repeat(
'<video><source src="https://external.example.com/video.mp4" /></video>',
$media_elements
)
);

$this->assertSame(
0,
preg_match_all( '/<source\b[^>]*\bcrossorigin\b/i', $output ),
'A SOURCE element must never receive the crossorigin attribute.'
);

$this->assertSame(
$media_elements - 1,
substr_count( $output, 'crossorigin="anonymous"' ),
'Every media element reachable within the seek budget should have been marked.'
);
}

/**
* Data provider for elements that should not receive crossorigin="anonymous".
*
Expand All @@ -461,6 +507,12 @@ public function data_elements_that_should_not_get_crossorigin() {
'relative URL script' => array(
'<script src="/wp-includes/js/wp-embed.min.js"></script>',
),
'source outside a media element' => array(
'<picture><source src="https://external.example.com/image.avif" /><img src="/local-image.jpg" /></picture>',
),
'source in a video with crossorigin' => array(
'<audio src="/local-audio.mp3"></audio><video crossorigin="use-credentials"><source src="https://external.example.com/video.mp4" /></video>',
),
);
}

Expand Down
Loading