origin.IsSameOriginWith(url::Origin::Create(gurl))

[Łukasz Anforowicz]

Hello,

I wanted to ask about your opinion on the following code pattern: `origin.IsSameOriginWith(url::Origin::Create(gurl))` (see code search;  note that this search doesn't cover cases where the pattern is split over more than one line [example]).  The context for the question is the desire to deprecate `url::Origin::Create` (see https://crbug.com/1270878 and an earlier discussion here).

Let me try to describe the code pattern in a bit more detail:

• This pattern is used by CORS, CORB to check if a `request_initiator_origin` is cross-origin from a `request_target_url`.
• The `request_initiator_origin` is usually represented as `url::Origin`.  But sometimes it is represented as `absl::optional<url::Origin>` where `absl::nullopt` indicates a browser-initiated request.
• The `request_target_url` is usually available as `GURL`.  It probably doesn't make sense to compare origins if the url is an about:blank or about:srcdoc.  I don't have a strong opinion on urls like data: or blob: URLs

Given above, I think I would like to propose adding the following method to url::Origin:

• bool Origin::IsSameOriginWith(const GURL& target_url)
• This is easy to discover (by being next to `bool Origin::IsSameOriginWith(const Origin& other_origin)`)
• We can DCHECK that `target_url` is not an about:blank (and maybe that it is not a blob: or filesystem: URL since these are tricky for dealing with inner URLs/origins)

Alternatives that I thought about and liked less than the proposal above:

• Separate, free function: bool IsSameOriginNetworkRequest(const absl::optional<url::Origin>& request_initiator, const GURL& target_url)
• Cons: This is less discoverable.
• Pros: The `request_initiator` parameter is explicitly named
• Neutral: Using `absl::optional` is orthogonal to the free-function-VS-method angle.  I think that implicit handling of `absl::optional` here would make things less, rather than more readable.
• Avoiding GURL: bool Origin::IsSameOriginWith(const SchemeHostPost& target_scheme_host_post)
• Pros: More explicit that this is about scheme+host+port tuple (and that we are ignoring GURL's path, query, etc)
• Cons
• inconvenient to use (callers have to do the conversion themselves)
• explicit conversion doesn't really solve the trickiness of GURL -> SchemeHostPort conversions (e.g. for about:blank, or blob: URLs)

WDYT?  Does `bool Origin::IsSameOriginWith(const GURL& target_url)` sound okay?

Thanks,

Lukasz

[Joe Mason]

+1 to adding a simple IsSameOriginWith(gurl) or equivalent method. Last time I implemented code that had to do these checks, it took me a long time to figure out the correct pattern to use.

Some thoughts inline:

On Tue, Nov 30, 2021 at 12:58 PM Łukasz Anforowicz <luk...@chromium.org> wrote:

• The `request_target_url` is usually available as `GURL`.  It probably doesn't make sense to compare origins if the url is an about:blank or about:srcdoc.  I don't have a strong opinion on urls like data: or blob: URLs

Does it make sense to return a tri-state kSameOrigin / kDifferentOrigin / kGURLHasNoOrigin? Then the caller doesn't need to remember all the side cases that need handling, but does need to decide what to do if the origin check simply doesn't make sense.

 

Given above, I think I would like to propose adding the following method to url::Origin:

• bool Origin::IsSameOriginWith(const GURL& target_url)
• This is easy to discover (by being next to `bool Origin::IsSameOriginWith(const Origin& other_origin)`)
• We can DCHECK that `target_url` is not an about:blank (and maybe that it is not a blob: or filesystem: URL since these are tricky for dealing with inner URLs/origins)

Hm, my suggestion above would accept about:blank, etc and return kGURLHasNoOrigin. The disadvantage is that we can't DCHECK that the caller is bothering to handle this result... If we do add DCHECK's that `target_url` is not about:blank, could we also add a free function `IsValidForSameOriginCheck(const GURL&)`, so that callers can check before calling IsSameOriginWith without having to know the complete list of corner cases to handle? Then we still get the DCHECK to ensure that they've made that call.

 

Alternatives that I thought about and liked less than the proposal above:

• Separate, free function: bool IsSameOriginNetworkRequest(const absl::optional<url::Origin>& request_initiator, const GURL& target_url)
• Cons: This is less discoverable.
• Pros: The `request_initiator` parameter is explicitly named
• Neutral: Using `absl::optional` is orthogonal to the free-function-VS-method angle.  I think that implicit handling of `absl::optional` here would make things less, rather than more readable.

I agree that using absl::optional when there's only a specific case that's valid to ignore the origin isn't clear enough. How about naming all overloads of the method `Origin::RequestInitiatorHasSameOriginWith(...)`? That's not 100% clear, but "With" at the end implies that the parameter is NOT the "request initiator", which implies the `this` Origin object must be.

Or, an IsSameOriginNetworkRequest free function would be discoverable if ALL the IsSameOrigin overloads were converted to free functions in the same header. I feel strongly they should all be in the same place, but as long as code search for "SameOrigin" finds all the functions / methods next to each other, I think they're discoverable enough.

(I don't have strong feelings about explicitly naming RequestInitiator in the API, though.)

• Avoiding GURL: bool Origin::IsSameOriginWith(const SchemeHostPost& target_scheme_host_post)
• Pros: More explicit that this is about scheme+host+port tuple (and that we are ignoring GURL's path, query, etc)
• Cons
• inconvenient to use (callers have to do the conversion themselves)
• explicit conversion doesn't really solve the trickiness of GURL -> SchemeHostPort conversions (e.g. for about:blank, or blob: URLs)

I like the idea of exposing the basic scheme+host+port comparison, but I also think that the most common operation of "I have a gurl, do an origin check on it" should be a one-liner.

So I'd suggest exposing 3 methods (or free functions), taking Origin, GURL, and SchemeHostPort, and making it clear in the function comments that the GURL and Origin versions are convenience wrappers around the SchemeHostPort version. That construction also makes it more obvious to the reader why GURL's that don't have a scheme+host+port are special cases.

Putting it all together, I suggest:

    // If neither this nor `other_origin` are opaque, extracts scheme+host+port from `other_origin` and calls IsSameOriginWith(scheme_host_port). Otherwise returns true iff they are both opaque and their internal nonces match.

    bool Origin::IsSameOriginWith(const Origin& other_origin) const;

    // Extracts scheme+host+port from `url` and calls IsSameOriginWith(scheme_host_port). `url` must have a valid scheme+host+port for a same-origin check. Use IsValidForSameOriginCheck to test whether a url is valid.

    // (After writing this I realized it's not quite correct, some url's get converted to "opaque" origins. So the text needs some more detail...)

    bool Origin::IsSameOriginWith(const GURL& url) const;

    // Returns true if `scheme_host_port` matches the scheme, host and port of this origin, false otherwise. If this origin is opaque, returns false.

    bool Origin::IsSameOriginWith(const SchemeHostPort& scheme_host_port) const;

    // Returns true if `url` is valid to pass to Origin::IsSameOriginWith, false otherwise.

    // ... comments here can list the corner cases and why they are not valid, including the "subtleties to note" at https://source.chromium.org/chromium/chromium/src/+/main:url/origin.h;l=104;drc=0931678cb31b9f68e28e179d550a96fabe26c0ec.

    bool IsValidForSameOriginCheck(const GURL& url);

I would find that API easy to understand the corner cases of and also easy to use. I especially think the discussion of the subtleties around converting url's to origins should be in the comments on a function that does that conversion, rather than in the header of the Origin class as they are now, since many people will not bother reading the rest of the file for context once they've found the obvious function to use.

WDYT?  Does `bool Origin::IsSameOriginWith(const GURL& target_url)` sound okay?

Thanks,

Lukasz

--
You received this message because you are subscribed to the Google Groups "net-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to net-dev+u...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CAA_NCUFEhLjJmYpwg5EcWOSyyv_iWQ-9ej34qUKm3%3DEcDBPo_g%40mail.gmail.com.

[Łukasz Anforowicz]

This is now implemented in a WIP CL at https://chromium-review.googlesource.com/c/chromium/src/+/3309669

One issue is that (as Daniel points out in a code review comment) having a url::Origin::IsSameOriginWith(const GURL&) overload means that it is now possible to write: rfh->GetLastCommittedOrigin().IsSameOriginWith(main_rfh->GetLastCommittedURL())

This does seem to be a problem / I don't know how to address this concern.  Some initial/random notes:

• We could in theory provide instead a url::Origin::IsSameOriginWith(const url::SchemeHostPort&) overload (as suggested by Joe above).  This would make the GURL-wielding callsites slightly more verbose (and therefore might steer the callers toward similarily verbose: rfh->GetLastCommittedOrigin().IsSameOriginWith(url::Origin::Create(main_rfh->GetLastCommittedURL()))
• In general, moving the url::Origin::Create into the new overload just centralizes the problem at hand (the problem of comparing the origin of about:blank + the problem of ignoring iframe.sandbox).  We can hope that in the future we'll be able to DCHECK against this in the new overload, but today many callers assume that they can pass any GURL in (including about:blank, blob:..., etc.)
• Maybe it is possible to have a clang-plugin-based compile check: if a value came from GetLastCommittedURL, then it is an error to pass this value to 1) methods that also have overloads accepting an url::Origin and/or 2) url::Origin::Create?
• Or maybe GetLastCommittedURL should return a GURLThatWontConvertToOrigin (not sure how exactly this would work in practice though - in the end this would probably need to provide a conversion to GURL, and after such conversion all protections would be gone...)
• Maybe we would still want to proceed with this change (not sure how to weigh the various considerations here: the problem is not new, but maybe the new pattern is more problematic?  the CL still provides other benefits: shorter callsites, getting rid of the error-prone url::Origin::Create [or rather, centralizing the problem])

-Lukasz

To unsubscribe from this group and stop receiving emails from it, send an email to net-dev+unsubscribe@chromium.org.

[Dominic Farolino]

Does it make sense to return a tri-state kSameOrigin / kDifferentOrigin / kGURLHasNoOrigin? Then the caller doesn't need to remember all the side cases that need handling, but does need to decide what to do if the origin check simply doesn't make sense.

Hmm, when does a GURL have no origin? Invalid GURLs? Otherwise, https://url.spec.whatwg.org/#concept-url-origin seems to be able to handle everything in some appropriate way. Of course Documents with URL=about:blank may have a non-opaque origin, but that's a case where it is known that Origin(Document.url) != Document.origin.

On Tue, Dec 7, 2021 at 3:48 PM Łukasz Anforowicz <luk...@chromium.org> wrote:

This is now implemented in a WIP CL at https://chromium-review.googlesource.com/c/chromium/src/+/3309669

One issue is that (as Daniel points out in a code review comment) having a url::Origin::IsSameOriginWith(const GURL&) overload means that it is now possible to write: rfh->GetLastCommittedOrigin().IsSameOriginWith(main_rfh->GetLastCommittedURL())

This does seem to be a problem / I don't know how to address this concern.  Some initial/random notes:

• We could in theory provide instead a url::Origin::IsSameOriginWith(const url::SchemeHostPort&) overload (as suggested by Joe above).  This would make the GURL-wielding callsites slightly more verbose (and therefore might steer the callers toward similarily verbose: rfh->GetLastCommittedOrigin().IsSameOriginWith(url::Origin::Create(main_rfh->GetLastCommittedURL()))
• In general, moving the url::Origin::Create into the new overload just centralizes the problem at hand (the problem of comparing the origin of about:blank + the problem of ignoring iframe.sandbox).  We can hope that in the future we'll be able to DCHECK against this in the new overload, but today many callers assume that they can pass any GURL in (including about:blank, blob:..., etc.)
• Maybe it is possible to have a clang-plugin-based compile check: if a value came from GetLastCommittedURL, then it is an error to pass this value to 1) methods that also have overloads accepting an url::Origin and/or 2) url::Origin::Create?
• Or maybe GetLastCommittedURL should return a GURLThatWontConvertToOrigin (not sure how exactly this would work in practice though - in the end this would probably need to provide a conversion to GURL, and after such conversion all protections would be gone...)

This is an interesting approach. If all is equal between GURL and GURLThatWontConvertToOrigin, it seems somewhat promising... If it would be easy to determine how many instances of GetLastCommittedURL() ultimately wind up reducing to a url::Origin, that information might give us an idea how feasible this would be.

• Maybe we would still want to proceed with this change (not sure how to weigh the various considerations here: the problem is not new, but maybe the new pattern is more problematic?  the CL still provides other benefits: shorter callsites, getting rid of the error-prone url::Origin::Create [or rather, centralizing the problem])

My 2c: I think the benefits that you mentioned are enough to justify this change, especially since the problem is not new. For my own curiosity, how common is it to get burned by using url::Origin::Create(GetLastCommittedURL())? Does this cause real security bugs in the wild, or other known bugs? I'm sure it does if we're talking about it, I'd just like to get a feel for the severity or pervasiveness of the problem.

-Lukasz

To unsubscribe from this group and stop receiving emails from it, send an email to net-dev+u...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CAA_NCUFEhLjJmYpwg5EcWOSyyv_iWQ-9ej34qUKm3%3DEcDBPo_g%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/e9f814b8-64e6-49a2-b512-474ecf0707abn%40chromium.org.