[Webkit-unassigned] [Bug 28313] Combine ThreadableLoaderOptions::CrossOriginRequestPolicy and CrossOriginRedirectPolicy

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28313





--- Comment #7 from Aaron Boodman <aa at chromium.org>  2009-08-16 23:43:41 PDT ---
(In reply to comment #4)
> (From update of attachment 34930 [details])
> How does this fix relate to bug 27740?

Sorry, I wasn't aware of that bug. The checks here are a superset (more strict)
than the existing checks.  I think that this patch makes the check in
WebCoreSynchronousLoader unnecessary, but I'm not sure. I don't think this
patch is incompatible with it.

> +        Bug 28313: Combine ThreadableLoaderOptions::crossOriginRequestPolicy
> and ThreadableLoaderOptions::crossOriginRedirectPolicy
> +        since they are always set the same way.
> 
> Please also include bug URL for easy clicking. Personally, I think that "Bug
> 28313: " prefix becomes unnecessary with that, and do not use it, but in the
> past, some people have indicated that they liked it.

Done.

> +        for access control, so we should never redirect across origins. But in
> two edge cases, we were:
> +
> +        * Synchronous XHR: cross-origin request that redirects to same-origin
> resource.
> 
> Were we? I think this is caught by checks in ResourceHandle, e.g. in
> connection:willSendRequest:redirectResponse: in ResourceHandleMac.mm. The first
> time this function is called, original url is stored to m_url, and then, the
> redirected URL is compared to that, not to document origin.

[Side note: I messed up the change log in my first patch. It was not
synchronous loads this case didn't work with, but asynchronous load. I've fixed
that now.]

For the case of page on origin A requesting resource from origin B that
redirects back to a resource on origin A, we do trigger the check in
ResourceHandleMac.mm.  However, it does not seem to stop the redirect from
proceeding. Without this patch, the redirect occurs and we load the same-origin
resource.

> +        (WebCore::DocumentThreadableLoader::isLegalRedirect): Ditto.
> 
> Maybe "allowed" would be more neutral? "Legal" makes me think that this is a
> precisely defined term from some spec (like characters legal in some
> production), which it isn't.
> 
> Just asking for your opinion.

Good idea, I've changed this.

> +    // request and response URLs. This isn't a perfect though, since a URL
> could redirect to itself.
> 
> Do you mean that a series of redirects could return us to the same host?
> "Redirect to itself" seem confusing.
> 
> +#include "KURL.h"
>
> No need to include the header, KURL is only used as a reference here.

Right. I did need a forward declare of KURL though. I changed this.

> +        Also, tightened up behavior of XMLHttpRequest with cross-origin
> redirects and access control. We have not implemented redirects
> +        for access control, so we should never redirect across origins. But in
> two edge cases, we were:
> 
> LayoutTests ChangeLog lacks bug URL, and starts with "also".

Done.

> +Tests that a cross-origin request that redirects to a same-origin resource
> succeeds.
> 
> Looks like all subtests actually expect failure, not success.

Fixed.

> There are tabs in layout test.

Fixed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.