[webkit-dev] Window::isSafeScript: which URLs to use in the error message

[Maciej Stachowiak]

On Aug 31, 2007, at 2:24 PM, Anyang Ren wrote:

> In kjs_window.cpp, Window::isSafeScript(ExecState *exec), we have:
>
>  KURL actURL = activeFrame->loader()->url();
>  WebCore::String actDomain = actURL.host();
>  ...
>  KURL thisURL = frame->loader()->url();
>  ...
>  WebCore::String thisDomain = thisURL.host();
>
>  if (actDomain == thisDomain && actURL.protocol() ==
> thisURL.protocol() && actURL.port() == thisURL.port())
>    return true;
>
>  if (Interpreter::shouldPrintExceptions()) {
>      printf("Unsafe JavaScript attempt to access frame with URL %s
> from frame with URL %s. Domains, protocols and ports must match.\n",
>             thisDocument->URL().latin1(), actDocument- 
> >URL().latin1());
>  }
>  String message = String::format("Unsafe JavaScript attempt to access
> frame with URL %s from frame with URL %s. Domains, protocols and ports
> must match.\n",
>                  thisDocument->URL().latin1(), actDocument- 
> >URL().latin1());
>
> Since thisURL and actURL are the URLs used in the test, why are
> we using thisDocument->URL() and actDocument->URL() in the
> error messages?

I think perhaps the message should mention both. thisURL and actURL  
(not such great names!) may identify the frame that is considered to  
own this frame in cases like about:blank or javascript: frame source,  
but they are not the true URL of the frame, so may not be very helpful  
in the error message. I think the format should be something like:

"Unsafe JavaScript attempt to access frame with URL %s (owned by %s)  
from frame with URL %s (owned by %s). Domains, protocols and ports  
must match.\n"

But the "owned by" part should probably be dropped entirely in the  
common case where the URLs are the same.

We'd take a patch to improve this.

> In fact, actDocument could be NULL.  Earlier in this function, we  
> have:
>
>  WebCore::Document* actDocument = activeFrame->document();
>
>  if (actDocument) {
>    if (thisDocument->domainWasSetInDOM() && actDocument- 
> >domainWasSetInDOM()) {
>      if (thisDocument->domain() == actDocument->domain())
>        return true;
>    }
>  }
>
> The if (actDocument) test suggests that actDocument could
> be NULL.

I'm not sure it's possible for a frame to be executing script with a  
null document, so I don't think this is actually possible. In fact, in  
current trunk, I don't think it is ever possible for any frame to have  
a null document.

Regards,
Maciej