Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#31687 closed defect (bug) (fixed)

Customizer: fix errors when embedded in a different origin

Reported by: mattwiebe Owned by: ocean90
Priority: normal Milestone: 4.2
Component: Customize Version: 4.1
Severity: normal Keywords: has-patch
Cc: Focuses: javascript

Description

Changes introduced during the 4.1 cycle create errors when the Customizer is embedded (via iframe) in an origin other than wp-admin. On WordPress.com, we use https://wordpress.com/customize/yourblogname.wordpress.com as the route into loading the Customizer in an iframe. This worked fine with the stock Customizer before 4.1 but two changes were needed as of 4.1.

In customize-base.js, using _.isEqual() in api.Value::set produces a SecurityError, traced back to setting self.targetWindow in customize-controls.js, since _.isEqual(), when checking for equality, attempts to access an object's prototype chain. The blank window object in the iframe (foo.wordpress.com/wp-admin/customize.php) takes on the origin of the parent context (wordpress.com) and attempting to access any object properties produced a fatal SecurityError, crippling the app and preventing it from loading. The try/catch pattern in the attached patch isn't pretty, but it works.

In customize-controls.js, the attempt to set the parent's document.title likewise produced a SecurityError, but this one at least doesn't cripple the Customizer, it just litters the console with SecurityErrors. Again, try/catch can take care of this.

The patch we're using to alleviate these issues on wp.com is attached.

Attachments (5)

31687.diff​ (1.4 KB ) - added by mattwiebe 12 years ago.
31687.1.diff​ (827 bytes ) - added by mattwiebe 12 years ago.
31687.2.diff​ (1.2 KB ) - added by mattwiebe 12 years ago.
31687.3.diff​ (1.8 KB ) - added by mattwiebe 12 years ago.
31687.4.diff​ (2.1 KB ) - added by mattwiebe 12 years ago.

Download all attachments as: .zip

Change History (17)

@mattwiebe
12 years ago

#1 @mattwiebe
12 years ago

  • Keywords has-patch added

An alternative approach would just be not using wp.customize.Value for the targetWindow property. I don't think any of the sugar there is needed that badly. This would remove the try/catch overhead for every single api.Value instance in the Customizer.

For the document.title part, we could do a try/catch initially and remember whether it's an accessible origin or not, sidestepping the try/catch every time.

#2 @westonruter
12 years ago

  • Milestone Awaiting Review → Future Release

For document.title we should have just been using postMessage to begin with, eh?

An alternative for patching wp.customize.Value.set would be to just override the method for just targetWindow, and to eliminate the use of _.isEqual() in that Value's specific set method.

@mattwiebe
12 years ago

#3 @mattwiebe
12 years ago

An alternative for patching wp.customize.Value.set would be to just override the method for just targetWindow

Yeah, that's a good idea. 31687.1.diff does just that. Cleaner. :)

@mattwiebe
12 years ago

#4 @mattwiebe
12 years ago

For document.title we should have just been using postMessage to begin with, eh?

Yep, the framework's in place, let's use it. 31687.2.diff does that. Goodbye nasty try/catch :)

This ticket was mentioned in ​Slack in #core-customize by mattwiebe. ​View the logs.


12 years ago

#6 follow-up: @obenland
12 years ago

@ocean90, @westonruter, do you think this and #31742 would be something we could do in 4.2?

#7 in reply to: ↑ 6 ; follow-up: @westonruter
12 years ago

  • Keywords commit added
  • Milestone Future Release → 4.2
  • Owner set to ocean90
  • Status new → reviewing

Replying to obenland:

@ocean90, @westonruter, do you think this and #31742 would be something we could do in 4.2?

I haven't reviewed #31742 yet, but the changes here are great. To be clear, the changes to be committed in this ticket are contained in two separate patches: 31687.1.diff​ & 31687.2.diff​

@mattwiebe
12 years ago

#8 in reply to: ↑ 7 @mattwiebe
12 years ago

Replying to westonruter:

the changes to be committed in this ticket are contained in two separate patches: 31687.1.diff​ & 31687.2.diff​

I combined those two patches into 31687.3.diff​ for convenience.

#9 @ocean90
12 years ago

  • Resolution → fixed
  • Status reviewing → closed

In 31885:

Customizer: Avoid SecurityErrors when the Customizer is embedded in an origin other than wp-admin.

props mattwiebe.
fixes #31687.

@mattwiebe
12 years ago

#10 @mattwiebe
12 years ago

  • Resolution fixed
  • Status closed → reopened

Apologies for apparently not knowing how to test my own code, but this still can create SecurityErrors.

31687.4.diff​ fixes this by ensuring that targetWindow always has its set method overridden before a window object could wind up in there.

#11 @DrewAPicture
12 years ago

  • Keywords commit removed

#12 @ocean90
12 years ago

  • Resolution → fixed
  • Status reopened → closed

In 31893:

Customizer: [31885] actually hasn't fixed the SecurityErrors. This one does.

props mattwiebe.
fixes #31687.

Note: See TracTickets for help on using tickets.