Bug 51714 – Move security logic out of the JavaScript binding for location into the DOM class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Darin Adler]

Move security logic out of the JavaScript binding for location into the DOM class

[Darin Adler]

Created attachment 77630 [details]
Patch

[WebKit Review Bot]

Attachment 77630 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7255240

[Darin Adler]

Comment on attachment 77630 [details]
Patch

I'm taking down the patch so I can add the include and so fix the Chromium build (at least) and also investigate some regression test failures that indicate I made a small mistake somewhere.

[Darin Adler]

Adam, this is the work you recently suggested.

[WebKit Review Bot]

Attachment 77630 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7270232

[Darin Adler]

Created attachment 77651 [details]
Patch

[Adam Barth]

Comment on attachment 77651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77651&action=review

Very nice.  I've suggested some FIXME comments below.  It's amazing to me how much of this machinery was redundant.  Thanks for working on this code.  I'll follow up with the V8 change once this patch lands.

> WebCore/bindings/js/JSDOMBinding.cpp:701
>  Frame* toDynamicFrame(ExecState* exec)
>  {
> -    return JSBindingState(exec).firstFrame();
> +    return firstDOMWindow(exec)->frame();
>  }

We should probably rename this function to something involving "first" to be consistent with our new names.

> WebCore/bindings/js/JSDOMBinding.cpp:706
>  bool processingUserGesture()
>  {
> -    return JSBindingState(JSMainThreadExecState::currentState()).processingUserGesture();
> -}
> -
> -KURL completeURL(ExecState* exec, const String& relativeURL)
> -{
> -    JSBindingState state(exec);
> -    return completeURL(&state, relativeURL);
> +    return ScriptController::processingUserGesture();
>  }

We should probably inline this function into its call sites.

> WebCore/page/DOMWindow.cpp:1612
> -void DOMWindow::setLocation(const String& urlString, DOMWindow* activeWindow, DOMWindow* firstWindow)
> +void DOMWindow::setLocation(const String& urlString, DOMWindow* activeWindow, DOMWindow* firstWindow,
> +    SetLocationLocking locking)

A line break!  In WebKit!  :)

> WebCore/page/Location.cpp:262
> +    // FIXME: It's not clear this cross-origin security check is valuable.
> +    // We allow one page to change the location of another. Why block attempts to reload?
> +    // Other location operations simply block use of JavaScript URLs cross origin.

Maybe it has to do with re-POSTing form data?  The other location operations can only generate GET requests.

[Darin Adler]

(In reply to comment #7)
> > WebCore/bindings/js/JSDOMBinding.cpp:701
> >  Frame* toDynamicFrame(ExecState* exec)
> >  {
> > -    return JSBindingState(exec).firstFrame();
> > +    return firstDOMWindow(exec)->frame();
> >  }
> 
> We should probably rename this function to something involving "first" to be consistent with our new names.

My intention is to delete this entirely ASAP.

> > WebCore/bindings/js/JSDOMBinding.cpp:706
> >  bool processingUserGesture()
> >  {
> > -    return JSBindingState(JSMainThreadExecState::currentState()).processingUserGesture();
> > -}
> > -
> > -KURL completeURL(ExecState* exec, const String& relativeURL)
> > -{
> > -    JSBindingState state(exec);
> > -    return completeURL(&state, relativeURL);
> > +    return ScriptController::processingUserGesture();
> >  }
> 
> We should probably inline this function into its call sites.

The reason I didn't in this patch was concern over the header dependency on ScriptController.

> > WebCore/page/DOMWindow.cpp:1612
> > -void DOMWindow::setLocation(const String& urlString, DOMWindow* activeWindow, DOMWindow* firstWindow)
> > +void DOMWindow::setLocation(const String& urlString, DOMWindow* activeWindow, DOMWindow* firstWindow,
> > +    SetLocationLocking locking)
> 
> A line break!  In WebKit!  :)

LOL.

> > WebCore/page/Location.cpp:262
> > +    // FIXME: It's not clear this cross-origin security check is valuable.
> > +    // We allow one page to change the location of another. Why block attempts to reload?
> > +    // Other location operations simply block use of JavaScript URLs cross origin.
> 
> Maybe it has to do with re-POSTing form data?  The other location operations can only generate GET requests.

Interesting point.

[Darin Adler]

Committed r74800: <http://trac.webkit.org/changeset/74800>