[webkit-reviews] review granted: [Bug 51714] Move security logic out of the JavaScript binding for location into the DOM class : [Attachment 77651] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 29 21:18:39 PST 2010


Adam Barth <abarth at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 51714: Move security logic out of the JavaScript binding for location into
the DOM class
https://bugs.webkit.org/show_bug.cgi?id=51714

Attachment 77651: Patch
https://bugs.webkit.org/attachment.cgi?id=77651&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.


More information about the webkit-reviews mailing list