Consider merging all the various *OriginAttributes to just one

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: smaug, Assigned: baku)

Tracking

50 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

For the background this is done in Bug 1209162, at that time it's possible that the child iframe won't inherit the origin attributes from parent document in some cases in the FirefoxOS new security model project, although now the container project and the firstPartyIsolation won't have this requirement.
Assignee: nobody → amarchesini
Posted patch oa.patchSplinter Review
I think this is a nice improvement of our code base. I understand why we had that setup (DocShell/Principal/Necko/GenericOriginAttributes) but I think we don't need this level of complexity today.

I also done some little changes:

1. OriginAttributes::Inherit() instead multiple inherit methods.
2. OriginAttributes::StripAddonId()
3. Only 3 CTORs: no params, appid + isolatedmozBrowser, originAttributesDictionary. all of them public.
Attachment #8824082 - Flags: review?(huseby)
Note that while the different types contain the same members, they are actually used very differently.

In particular, DocshellOriginAttributes doesn't contain the correct mFirstPartyDomain information since the toplevel docshell can't correctly track the first-party-domain in its originAttributes (since there can be multiple differen first-party-domains active in a toplevel docshell at once).

Hence it's very useful to use the type system to ensure that we statically track when we need to grab the first-party information from elsewhere, and when not.
Comment on attachment 8824082 [details] [diff] [review]
oa.patch

Review of attachment 8824082 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/BasePrincipal.cpp
@@ +34,5 @@
>  
>  using dom::URLParams;
>  
>  void
> +OriginAttributes::StripUserContextIdAndFirstPartyDomain()

Two thoughts:
1) maybe make a single "strip" function that takes flags for which ones to reset?  StripAttributes(USER_CONTEXT_ID | FIRST_PARTY_DOMAIN)
2) if we keep these specialized functions, we should probably at least move them to the header and make them inline.

@@ +47,5 @@
> +  mAddonId.Truncate();
> +}
> +
> +void
> +OriginAttributes::Inherit(const OriginAttributes& aAttrs)

Inherit could take bitflags, similar to the strip function I mention above, and the flags specify which attributes to inherit and which ones to reset.

::: caps/BasePrincipal.h
@@ +43,5 @@
> +  void Inherit(const OriginAttributes& aAttrs);
> +
> +  void SetFirstPartyDomain(const bool aIsTopLevelDocument, nsIURI* aURI);
> +
> +  void StripUserContextIdAndFirstPartyDomain();

if you keep these functions, i think you should make them inline them.

@@ +45,5 @@
> +  void SetFirstPartyDomain(const bool aIsTopLevelDocument, nsIURI* aURI);
> +
> +  void StripUserContextIdAndFirstPartyDomain();
> +
> +  void StripAddonId();

inline

::: caps/nsScriptSecurityManager.cpp
@@ +299,5 @@
>                  nsNullPrincipal::CreateWithInheritedAttributes(loadInfo->LoadingPrincipal());
>              } else {
> +              OriginAttributes attrs;
> +              loadInfo->GetOriginAttributes(&attrs);
> +              attrs.StripAddonId();

attrs.Strip(OriginAttributes::ADDON_ID)

?

@@ +403,4 @@
>  
>      // For addons loadInfo might be null.
>      if (loadInfo) {
> +      attrs.Inherit(loadInfo->GetOriginAttributes());

// inherit all origin attributes except ADDON_ID
attrs.Inherit(loadInfo->GetOriginAttributes(), ~OriginAttributes::ADDON_ID);

?

::: docshell/base/nsDocShell.cpp
@@ +12289,5 @@
>            } else {
>              // get the OriginAttributes
> +            OriginAttributes attrs;
> +            loadInfo->GetOriginAttributes(&attrs);
> +            attrs.StripAddonId();

Strip(flags)?

@@ +14606,5 @@
>    }
>  
>    if (aIsNonSubresourceRequest) {
> +    OriginAttributes attrs;
> +    attrs.Inherit(mOriginAttributes);

Inherit(OA, flags)?

@@ +14658,5 @@
>  
>    bool isReload = mLoadType & LOAD_CMD_RELOAD;
>  
> +  OriginAttributes attrs;
> +  attrs.Inherit(mOriginAttributes);

Inherit(OA, flags)?

::: netwerk/base/nsSocketTransport2.h
@@ +312,1 @@
>      

please remove whitespace

::: netwerk/cookie/nsCookieService.cpp
@@ +2359,5 @@
>    return NS_NewArrayEnumerator(aEnumerator, cookieList);
>  }
>  
>  static nsresult
> +InitializeOriginAttributes(OriginAttributes* aAttrs,

can this be "OriginAttributes const *" ?

@@ +2434,5 @@
>                             bool              aIsSecure,
>                             bool              aIsHttpOnly,
>                             bool              aIsSession,
>                             int64_t           aExpiry,
> +                           OriginAttributes* aOriginAttributes)

can this be "OriginAttributes const *" ?

@@ +2558,5 @@
>  nsCookieService::RemoveNative(const nsACString &aHost,
>                                const nsACString &aName,
>                                const nsACString &aPath,
>                                bool aBlocked,
> +                              OriginAttributes* aOriginAttributes)

can this be "OriginAttributes const *" ?

::: netwerk/ipc/NeckoParent.cpp
@@ +831,5 @@
>  
>    // We only actually care about the loadContext.mPrivateBrowsing, so we'll just
>    // pass dummy params for nestFrameId, and originAttributes.
>    uint64_t nestedFrameId = 0;
> +  OriginAttributes attrs(NECKO_UNKNOWN_APP_ID, false);

is this constant necessary?  i noticed we have a NO_APP_ID in the nsScriptSecurity manager too.  maybe combine them into an OA constant like OriginAttributes::NO_APP_ID ?

@@ +864,5 @@
>  
>    // We only actually care about the loadContext.mPrivateBrowsing, so we'll just
>    // pass dummy params for nestFrameId, and originAttributes;
>    uint64_t nestedFrameId = 0;
> +  OriginAttributes attrs(NECKO_UNKNOWN_APP_ID, false);

same as above

::: netwerk/protocol/http/TunnelUtils.cpp
@@ +1595,5 @@
>  FWD_TS_PTR(GetRecvBufferSize, uint32_t);
>  FWD_TS(SetRecvBufferSize, uint32_t);
>  
>  nsresult
> +SocketTransportShim::GetOriginAttributes(mozilla::OriginAttributes* aOriginAttributes)

can this be "mozilla::OriginAttributes const *" ?

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3085,5 @@
>      }
>  
>      socketTransport->SetConnectionFlags(tmpFlags);
>  
> +    OriginAttributes originAttributes = mEnt->mConnInfo->GetOriginAttributes();

since you're making a copy we should do:

const OriginAttributes originAttributes =

or maybe even a const OriginAttributes &

::: security/manager/ssl/TransportSecurityInfo.h
@@ +70,3 @@
>  
>    PRErrorCode GetErrorCode() const;
>    

please include a separate patch that cleans up the extra whitespace in this file.
Attachment #8824082 - Flags: review?(huseby) → review+
This is an amazing cleanup.  Good job.  There's a couple files you touched that have extra whitespace in them.  Please submit a separate patch that cleans up the whitespace.  This is great.  Much cleaner.
> Two thoughts:

I like the StripAttributes();

> // inherit all origin attributes except ADDON_ID
> attrs.Inherit(loadInfo->GetOriginAttributes(), ~OriginAttributes::ADDON_ID);

All our Inherit calls strip addonId. Having an additional param can generate error eventually.
Plus addonID is going away.

> >  static nsresult
> > +InitializeOriginAttributes(OriginAttributes* aAttrs,
> 
> can this be "OriginAttributes const *" ?

No, it calls Init() and it is not const.

> > +  OriginAttributes attrs(NECKO_UNKNOWN_APP_ID, false);
> 
> is this constant necessary?  i noticed we have a NO_APP_ID in the
> nsScriptSecurity manager too.  maybe combine them into an OA constant like
> OriginAttributes::NO_APP_ID ?

Follow up?
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe52da5cb90
Merging all the various *OriginAttributes to just one, r=huseby
https://hg.mozilla.org/mozilla-central/rev/8fe52da5cb90
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Greetings from your friends at Thunderbird and SeaMonkey:
https://hg.mozilla.org/comm-central/rev/112f41f81a4115e6f0f130c78a77c0782f33b01d
Hi, Baku. It is confusing that when and where should we use inherit() since there is only one type of originAttributes after this bug. Do you have any guideline about this? Do we have to use inherit every time when we pass originAttributes across different modules, like from docShell to Necko?
Flags: needinfo?(amarchesini)
> originAttributes across different modules, like from docShell to Necko?

Exactly. We should not use OriginAttribute copy CTOR. I'll file a bug for this.
Unfortunately, we need to keep Inherit() until bug 1314361 is resolved.
Depends on: 1314361
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.