Context Navigation

← Previous Ticket
Next Ticket →

#44832 closed defect (bug) (fixed)

Lack of null check in wp-embed.js causes uncaught error to be thrown under certain circumstances.

Reported by:	dsifford	Owned by:	SergeyBiryukov
Priority:	normal	Milestone:	4.9.9
Component:	Embeds	Version:	4.4
Severity:	minor	Keywords:	good-first-bug has-patch needs-testing
Cc:		Focuses:	javascript

Description

The file and lines in question can be seen here: https://github.com/WordPress/WordPress/blob/5bbb3b38d96eb0aac29d3b368d541039a3b1fbb8/wp-includes/js/wp-embed.js#L31-L33

The issue specifically is that currently the script assumes there will always be a data parameter on message. This is not true in the case of Google ReCAPTCHA (and I assume many other 3rd party scripts).

In these cases, data is null and when attempting to access properties on null, an error is thrown.

This all can be fixed by changing the if statement from what it is currently to:

if ( data && ! ( data.secret || data.message || data.value ) ) {
	return;
}

Attachments (2)

wp-embed-patch.44832.diff (404 bytes ) - added by dsifford 8 years ago.: Patch (hope I did this correctly)
wp-embed-patch.44832.2.diff (412 bytes ) - added by kadamwhite 8 years ago.: Adjust patch for trunk JS reorganization

Download all attachments as: .zip

Change History (17)

#1 follow-up: ↓ 12 @swissspidy
8 years ago

Keywords needs-patch good-first-bug added
Milestone Awaiting Review → Future Release
Severity normal → minor
Version trunk → 4.4

Good catch! Although your proposed change wouldn't actually return early when data is null.

It would be more something like

if ( ! data || ! ( data.secret || data.message || data.value ) ) {
	return;
}

#2 @dsifford
8 years ago

D'oh! You're right!

I agree with your suggested change to my suggested change. :)

#3 @SergeyBiryukov
8 years ago

Milestone Future Release → 4.9.9

Just stumbled upon this while setting up reCAPTCHA. Moving to 4.9.9 for visibility.

@dsifford
8 years ago

Attachment wp-embed-patch.44832.diff added

Patch (hope I did this correctly)

#4 @swissspidy
8 years ago

Keywords has-patch added; needs-patch removed

@kadamwhite
8 years ago

Attachment wp-embed-patch.44832.2.diff added

Adjust patch for trunk JS reorganization

#5 @kadamwhite
8 years ago

Keywords needs-testing added

#6 @SergeyBiryukov
8 years ago

Owner set to SergeyBiryukov
Resolution → fixed
Status new → closed

In 43593:

Embeds: Avoid a JS error in wp.receiveEmbedMessage if data parameter is not set.

Props dsifford, kadamwhite.
Fixes #44832.

#7 @SergeyBiryukov
8 years ago

Keywords fixed-major added
Resolution fixed
Status closed → reopened

Reopening for 4.9.9 consideration.

#8 @SergeyBiryukov
8 years ago

In 43597:

Embeds: After [43593], move the new check to a separate line to avoid test_js_no_ampersands_in_compiled failure caused by UglifyJS task.

See #44832.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.

8 years ago

#10 @SergeyBiryukov
8 years ago

Resolution → fixed
Status reopened → closed

In 43600:

Embeds: Avoid a JS error in wp.receiveEmbedMessage if data parameter is not set.

Props dsifford, kadamwhite.
Merges [43593] and [43597] to the 4.9 branch.
Fixes #44832.

#11 @robhenley
8 years ago

I agree the check should be there but isn't the issue the generic "message" event that both wp-embed.min.js and Recaptcha's recaptcha__en.js are using? So receiveEmbedMessage is being called by Recaptcha without the proper arguments.

#12 in reply to: ↑ 1 @ekernercom
8 years ago

Last edited 8 years ago by ekernercom (previous) (diff)

#13 @pento
8 years ago

Keywords fixed-major removed

[43600] can be ported to the 5.0 branch.

#14 @pento
8 years ago

Resolution fixed
Status closed → reopened

#15 @SergeyBiryukov
8 years ago

Resolution → fixed
Status reopened → closed

In 43704:

Embeds: Avoid a JS error in wp.receiveEmbedMessage if data parameter is not set.

Props dsifford, kadamwhite.
Merges [43593] and [43597] to the 5.0 branch.
Fixes #44832.

Note: See TracTickets for help on using tickets.

Download in other formats:

Make WordPress Core

Context Navigation

#44832 closed defect (bug) (fixed)

Lack of null check in wp-embed.js causes uncaught error to be thrown under certain circumstances.

Description

Attachments (2)

Change History (17)

#1 follow-up: ↓ 12 @swissspidy 8 years ago

#2 @dsifford 8 years ago

#3 @SergeyBiryukov 8 years ago

@dsifford 8 years ago

#4 @swissspidy 8 years ago

@kadamwhite 8 years ago

#5 @kadamwhite 8 years ago

#6 @SergeyBiryukov 8 years ago

#7 @SergeyBiryukov 8 years ago

#8 @SergeyBiryukov 8 years ago

This ticket was mentioned in ​Slack in #core-restapi by kadamwhite. ​View the logs. 8 years ago

#10 @SergeyBiryukov 8 years ago

#11 @robhenley 8 years ago

#12 in reply to: ↑ 1 @ekernercom 8 years ago

#13 @pento 8 years ago

#14 @pento 8 years ago

#15 @SergeyBiryukov 8 years ago

Download in other formats:

#1 follow-up: ↓ 12 @swissspidy
8 years ago

#2 @dsifford
8 years ago

#3 @SergeyBiryukov
8 years ago

@dsifford
8 years ago

#4 @swissspidy
8 years ago

@kadamwhite
8 years ago

#5 @kadamwhite
8 years ago

#6 @SergeyBiryukov
8 years ago

#7 @SergeyBiryukov
8 years ago

#8 @SergeyBiryukov
8 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.

8 years ago

#10 @SergeyBiryukov
8 years ago

#11 @robhenley
8 years ago

#12 in reply to: ↑ 1 @ekernercom
8 years ago

#13 @pento
8 years ago

#14 @pento
8 years ago

#15 @SergeyBiryukov
8 years ago