Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#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's profile dsifford Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.9.9 Priority: normal
Severity: minor Version: 4.4
Component: Embeds Keywords: good-first-bug has-patch needs-testing
Focuses: javascript Cc:

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 6 years ago.
Patch (hope I did this correctly)
wp-embed-patch.44832.2.diff (412 bytes) - added by kadamwhite 6 years ago.
Adjust patch for trunk JS reorganization

Download all attachments as: .zip

Change History (17)

#1 follow-up: @swissspidy
6 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release
  • Severity changed from normal to minor
  • Version changed from trunk to 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
6 years ago

D'oh! You're right!

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

#3 @SergeyBiryukov
6 years ago

  • Milestone changed from Future Release to 4.9.9

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

@dsifford
6 years ago

Patch (hope I did this correctly)

#4 @swissspidy
6 years ago

  • Keywords has-patch added; needs-patch removed

@kadamwhite
6 years ago

Adjust patch for trunk JS reorganization

#5 @kadamwhite
6 years ago

  • Keywords needs-testing added

#6 @SergeyBiryukov
6 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 43593:

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

Props dsifford, kadamwhite.
Fixes #44832.

#7 @SergeyBiryukov
6 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.9.9 consideration.

#8 @SergeyBiryukov
6 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.


6 years ago

#10 @SergeyBiryukov
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to 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
6 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
6 years ago

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

#13 @pento
6 years ago

  • Keywords fixed-major removed

[43600] can be ported to the 5.0 branch.

#14 @pento
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#15 @SergeyBiryukov
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to 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.