1110475 - Fetch Request should not require origin to be set

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug it track removing the origin field from Fetch Request.  This is something Nikhil and I discussed over email, but I never got around to implementing on maple.  Now that we're moving to mozilla-central I thought I better write it up as a bug.

From the email:

Basically, I'm having trouble setting the InternalRequest origin value for all principals.  Talking to Jonas it appears that we probably don't need Request to have an origin at all.

The two places its currently used are in FetchDriver.cpp for:

1) To do the same origin check like this:

  nsAutoCString originURL;
  mRequest->GetOrigin(originURL);
  nsCOMPtr<nsIURI> originURI;
  rv = NS_NewURI(getter_AddRefs(originURI), originURL, nullptr, nullptr);
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return FailWithNetworkError();
  }

  nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
  rv = ssm->CheckSameOriginURI(requestURI, originURI, false);

2) Set the origin header:

    if (mRequest->ForceOriginHeader()) {
      nsCString origin;
      mRequest->GetOrigin(origin);
      httpChan->SetRequestHeader(NS_LITERAL_CSTRING("origin"),
                                 origin,
                                 false /* merge */);
    }

Jonas suggested that in both cases we probably should be use the page's principal instead.  So instead of calling mRequest->GetOrigin() you would query mPrincipal instead.

In the case of the same origin check, there is also a better way to do the same origin check:

  mPrincipal->CheckMayLoad(uri);

Also note that the origin may not be valid for all principals.  For example, a system principal returns "[System Principal]" instead of a URI.

Does dropping the InternalRequest origin member sound reasonable?

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I have integrated these changes into the fetch patches. Got rid of the origin and expect that whoever has an InternalRequest and wants to fetch a URL also has a valid principal.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Are these patches uploaded anywhere in particular?  I need to adapt the Cache patches to match.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Not yet, but it just removes the origin field. I already had the requirement that FetchDriver be passed a principal. That bit is in patch 5 of bug dom-fetch-api

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Ok, just be aware that P1 patch in bug 940273 currently adds a SetOrigin() method.  I'll have to update this when you refresh dom-fetch-api patches.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I ended up doing this in bug 940273 in a rebase.  So this bug is invalid.