Bug 38285 – REGRESSION: Sandboxing code breaks some javascript: URLs in empty windows

• Component: Page Loading
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Steps to reproduce: 
1. Open a new empty window or tab.
2. Enter in address field: javascript:window.open('http://digg.com/')

Results: nothing happens.
Expected results: digg.com opens.

The failing check is in FrameLoader::createWindow():
    if (isDocumentSandboxed(SandboxNavigation))

<rdar://problem/7903453>

[Alexey Proskuryakov]

Created attachment 54835 [details]
naive fix

[Darin Adler]

Comment on attachment 54835 [details]
naive fix

The reason sandbox flags start with SandboxAll is the principle that it's safest to start with restrictions and relax them rather than the other way around. Changing the default setting affects an unknown set of code paths. That having been said, this change is OK with me. I'd love to hear Adam's take on it too.

[Alexey Proskuryakov]

One thing I'm unsure about is relationship between flags in FrameLoader and in Document's security origin. FrameLoader::updateSandboxFlags() is called while opening the new tab/window, and it changes m_sandboxFlags to None. But it's too late for document's SecurityOrigin, because it's created earlier than that, so it still has the original flags from FrameLoader.

[Adam Barth]

I'll look at this in more detail later.  The sandbox flags on FrameLoader are the flags a new document loaded into that frame would get.  The ones on the document's security origin are the ones the document was given when it was loaded.  These can be different if someone changes the sandbox attribute after a document is loaded.

[Alexey Proskuryakov]

Created attachment 54952 [details]
better fix

Adam suggested a better fix - we can propagate sandbox flags in init() earlier. This doesn't seem to affect any known behavior, but is generally nicer than starting with SandboxNone.

[Adam Barth]

Comment on attachment 54952 [details]
better fix

Thanks!  You might as well land the test you wrote too.  It doesn't hurt to add extra tests.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/58695>.