Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#58978 closed defect (bug) (fixed)

Post embed iframe throws soft(?) DOMException for emoji-loader's references to sessionStorage

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.3
Component: Emoji Keywords: has-patch dev-feedback fixed-major
Focuses: javascript Cc:

Description (last modified by westonruter)

While working on #58931 I discovered that a post embed from a site running WordPress 6.3-RC3 was causing an error to appear in the console:

Uncaught (in promise) DOMException: Failed to read the 'sessionStorage' property from 'Window': The document is sandboxed and lacks the 'allow-same-origin' flag.
    at getSessionSupportTests (http://localhost:8889/2023/06/07/emoji/embed/:83:3)
    at http://localhost:8889/2023/06/07/emoji/embed/:354:22
    at new Promise (<anonymous>)
    at wpEmojiLoader (http://localhost:8889/2023/06/07/emoji/embed/:353:2)
    at http://localhost:8889/2023/06/07/emoji/embed/:438:4

Then when I wrap getSessionSupportTests in a try/catch block, I see another error appear:

Uncaught DOMException: Failed to read the 'sessionStorage' property from 'Window': The document is sandboxed and lacks the 'allow-same-origin' flag.
    at setSessionSupportTests (http://localhost:8889/2023/06/07/emoji/embed/:115:3)
    at worker.onmessage (http://localhost:8889/2023/06/07/emoji/embed/:379:6)

Wrapping that instance with a try/catch block then causes the errors to be suppressed. The issue is that Post Embed iframes lack the allow-same-origin value for the sandbox attribute, and so sessionStorage is not available.

What's surprising about this is even though an error appears in the console about an uncaught exception being thrown, I don't see the execution actually being interrupted anywhere. If I add code like this to wp-embed:

console.log( 'sessionStorage', sessionStorage );
console.log( 'sessionStorage.getItem("foo")', sessionStorage.getItem("foo") );
console.log( 'Howdy!' );

Then sure enough my console reads:

typeof sessionStorage === object
sessionStorage.getItem("foo") === null
Howdy!
Uncaught (in promise) DOMException: Failed to read the 'sessionStorage' property from 'Window': The document is sandboxed and lacks the 'allow-same-origin' flag.

And the callstack points to the exception being raised at typeof sessionStorage === object, even though the subsequent lines still execute.

I introduced the use of sessionStorage in #58472. I had added try/catch blocks around the calls to sessionStorage.getItem() and sessionStorage.setItem() didn't thinking that code like this could possibly cause an error:

if (
        typeof sessionStorage !== 'undefined' &&
        sessionStorageKey in sessionStorage
) {

But it does. It seems Chrome will at least emit a silent DOMException the first time that the sessionStorage object is touched. It doesn't seem to impact the execution, however.

So fixing this will clear up errors from the console, but it doesn't seem it will have any other impact.

Note this issue can be reproduced with a post embed from the same site running on the wordpress-develop dev environment.

Change History (18)

#1 @westonruter
10 months ago

  • Milestone set to 6.3

I'm assigning this to 6.3 with the expectation that it would be punted to 6.3.1.

#2 @westonruter
10 months ago

  • Description modified (diff)

This ticket was mentioned in PR #4965 on WordPress/wordpress-develop by @westonruter.


10 months ago
#3

  • Keywords has-patch added

This simply moves the try/catch blocks from inside the if statements that reference sessionStorage to instead wrap these if statements.

Trac ticket: https://core.trac.wordpress.org/ticket/58978

#5 @flixos90
10 months ago

While it doesn't make a functional difference, it may still make sense to commit for the WP 6.3 release still, to suppress that exception? It's extremely late in the cycle, but this is also a very simple change, so I'd be on board with that.

@westonruter commented on PR #4965:


10 months ago
#6

Humm. Somehow I pushed my branch this to origin by mistake when I meant to push it to my fork. (I'm surprised I was able to push to WordPress/wordpress-develop to begin with.) Going to create a separate PR because I suspect the branch will get blown away by the mirroring process.

This ticket was mentioned in PR #4966 on WordPress/wordpress-develop by @westonruter.


10 months ago
#7

This removes the if statements which were attempting to prevent errors related to interacting with sessionStorage. It turns out that merely referencing the sessionStorage object will cause an error, although the error does not cause an interruption in execution (in Chrome at least).

Trac ticket: https://core.trac.wordpress.org/ticket/58978

@westonruter commented on PR #4965:


10 months ago
#8

@felixarntz here's another updated version: https://github.com/WordPress/wordpress-develop/pull/4966

@westonruter commented on PR #4966:


10 months ago
#9

With whitespace changes suppressed:

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/134745/b53b398d-0f8e-482f-a72e-3fa890b37b80

#10 @westonruter
10 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 56358:

Emoji: Suppress console errors from sessionStorage usage in sandboxed post embed iframe.

Amends [56074].

Props westonruter, flixos90.
Fixes #58978.
See #58472.

#12 @westonruter
10 months ago

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

Reopening for 6.3 consideration.

#13 follow-up: @kirasong
10 months ago

@azaozz Do you have any opinions / thoughts on whether we should backport this for 6.3?

#14 @audrasjb
10 months ago

  • Keywords fixed-major added

Just noting that if we backport this to 6.3, we'll probably need to ship a RC4 today.

Adding fixed-major as this ticket is reopened for a potential backport. The workflow keyword can be removed if this ticket is moved to 6.3.1.

This ticket was mentioned in Slack in #core by priethor. View the logs.


10 months ago

#16 @SergeyBiryukov
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 56364:

Emoji: Suppress console errors from sessionStorage usage in sandboxed post embed iframe.

Amends [56074].

Props westonruter, flixos90.
Reviewed by westonruter, SergeyBiryukov.
Merges [56358] to the 6.3 branch.
Fixes #58978. See #58472.

#17 in reply to: ↑ 13 @azaozz
10 months ago

Replying to mikeschroder:

The patch LGTM for merging, just silences some possible errors. Thanks @SergeyBiryukov for merging it.

This ticket was mentioned in Slack in #core by priethor. View the logs.


10 months ago

Note: See TracTickets for help on using tickets.