Fetch Request should not require origin to be set

RESOLVED INVALID

Status

()

RESOLVED INVALID
4 years ago
4 years ago

People

(Reporter: bkelly, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
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?
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.
(Reporter)

Comment 2

4 years ago
Are these patches uploaded anywhere in particular?  I need to adapt the Cache patches to match.
Flags: needinfo?(nsm.nikhil)
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
Flags: needinfo?(nsm.nikhil)
(Reporter)

Comment 4

4 years ago
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.
(Reporter)

Comment 5

4 years ago
I ended up doing this in bug 940273 in a rebase.  So this bug is invalid.
No longer blocks: 1110144
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.