Hardening the 'url::Origin' implementation.

Hardening the 'url::Origin' implementation.

The current 'url::Origin' implementation is little more than a wrapper
for simple string comparison. This patch improves the implementation to
bring it into line with the requirements of RFC6454 and the WHATWG's
URL specification.

BUG=490074

[Mike West, 2015-05-22 16:29:13]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, rsleevi@chromium.org

[Mike West, 2015-05-22 16:29:14]

Splitting this out from https://codereview.chromium.org/1151843002, as I agree
with most of Ryan's comments re://net on that patch. I'll have to be a bit more
careful about the approach there.

I think this is an improvement over the current implementation of `url::Origin`.
Before I poke the //url OWNERS, WDYT, Jochen and Ryan?

[Ryan Sleevi, 2015-05-22 18:37:57]

I'd like to soft-push-back on this until it's a little clearer the *why*

That is, I'd like to pretend url::Origin doesn't exist (indeed, if I beat you to
crbug.com/490207, it won't). In that model, let's look at what it is we need,
and why we need it, then we figure out how to do it. You suggested you were
going to be looking at that?

From the other CL, I talked a lot about the surprising edge cases, that's why I
wanted to make sure we had documented (perhaps on the bug) why/what it is we're
trying to do. Is there a common idiom that's dangerous? Wrong? What are we
trying to solve, etc.

I guess I'm treating this as someone saying "Harden base::Frobber"
implementation, except base::Frobber is scheduled for deletion. We haven't
really "needed" base::Frobber, but if there really IS a common problem that a
base::Frobber would/should solve, then we should document that.

[Ryan Sleevi, 2015-05-22 20:43:36]

Thanks for keeping me involved in this CL, even though it mostly touches //url

I'm still pushing back a little to make sure this is the right thing to do. I
agree with this CL being smaller, it's more clearly directly related to the
"Here's the existing use of url::Origin, done right[ish]", but I guess taking a
step back, there's still the open question(ish) of whether this code should have
ever been using url::Origin to begin with (or should we just nuke that code)

I think if we assume url::Origin is the right thing to do, we're getting better
- although there's a lot of edge cases and weird things that all need to be
thoroughly spelled out in the header (much like url::GURL does, quite
verbosely). My ideal outcome is that "virtually" every method has a full
explanation of nuances, preconditions, weird things, etc.

I also don't think we'll be able to get away with just touching url::Origin
here, and there may actually be pre-work to happen before changing url::Origin.
I sort of touch on this below re: some of the scheme handling. Indeed, even my
suggestion of
IsStandardScheme() || IsFileScheme()

is probably the wrong thing to be implementing, and instead it should be
something like SchemeHasHost (maybe, maybe not; file is such a special snowflake
here that it's quite hard to fully reason about expectations)

Anyways, here's the next round of feedback

https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
content/public/common/common_param_traits.cc:57: WriteParam(m, p.port());
DANGER: If p.port()'s signature were to hypothetically change, we'd end up in a
mismatch between ReadParam() and WriteParam that wouldn't be obvious (because
line 65 is explicit)

I'm not sure what the normal IPC idiom is - maybe it doesn't care (e.g. line
81), but that strikes me as a danger

https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
content/public/common/common_param_traits.cc:65: unsigned short port;
DANGER: This isn't explicitly sized, but it would seem like for IPC, it should
be. (e.g. line 88)

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc#newcode24
url/origin.cc:24: // Special-case 'file://':
I don't think this should be necessary - that is, it should be entirely invalid
to supply file:// URLs to a scheme/host/port construct because File URLs are
'empty host', full path.

That is, in this constructor, I suspect the right answer would be something like

DCHECK(IsStandard(...) && !CompareScheme(..., kFileScheme))

Assuming we create & expose a CompareScheme (or perhaps just 'internally' expose
the CompareScheme from url_util, rather than exporting from url::)

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc#newcode25
url/origin.cc:25: if (scheme == kFileScheme) {
BUG: The scheme comparisons should operate case-insentively, hence why I
suggested a CompareScheme function above. An equality check like this won't
help.

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc#newcode79
url/origin.cc:79: return scheme_ == scheme;
BUG: Case-sensitivity

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc#newcode83
url/origin.cc:83: return SchemeIs(kHttpsScheme) || SchemeIs(kWssScheme);
We should probably be breaking this out from both GURL and Origin and into an
(internal) free function helper

https://codereview.chromium.org/1153763002/diff/1/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode15
url/origin.h:15: // Origin represents a scheme/host/port tuple, as described in
RFC6454.
At the risk of being a pain, I think it would be great to expand this
documentation here, especially if we expect to build more code on top of it.

// Represents a tuple of scheme/host/port, as described in RFC 6454
---^ nit, I like spaces in my RFC #s ;)
//
// Conceptually, an origin is just a tuple of scheme/host/port, but
// as described in RFC 6454, there are a number of subtle behaviours
// to be aware of.
//
// The first is that not everything that can be expressed as a URL
// has an origin. [something about scheme's without hier-parts, like data URIs,
and the nuances thereof]
//
// The second is the concept of the "null" origin [something something]
//
// etc
//
// Usage:
// - If you're trying to do X
//   [sample]
// - If you're trying to do Y
//   [sample]

(Please don't take the above, as I think I did a poor job at phrasing, but I
wanted to try to illustrate the type of concepts). 

I think there's still a lot of sharp edges in this API, which is why I'm still
sort of uneasy, but the best way I suppose to smooth those edges is to both
document them and show, idiomatically in the documentation, how to avoid those
edges. Things like should you be checking if the GURL is valid beforehand, etc.

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode44
url/origin.h:44: bool unique() const { return unique_; }
I'm not sure what the utility of this is, nor if it's the right thing to expose.
Maybe the documentation comments above will sort of address this.

For example, is this trying to express whether it's the null origin? Is that the
concept we should be exposing? Or perhaps we should be exposing whether or not
the GURL was valid to begin with (assuming invalid GURL == null origin ==
unique).

I don't recall from the other CL you checking this. If there isn't concrete code
you can point to needing this, I'd actually prefer we not include it at all, at
least not until we can actually demonstrate and explain the need.

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode48
url/origin.h:48: // to a user in browser UI.
s/browser//

//net, and below (including URL) have zero knowledge or requirement that they be
used as or in a browser.

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode55
url/origin.h:55: // 2. Origins with a
Incomplete?

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode56
url/origin.h:56: // 3. IPv6 addresses serialize with bracketed hostnames:
'http://[::1]'.
Why is this a divergence? This is exactly what's required of IPv6 addresses in
an origin. The host-part of a URL is always bracketed (7.1 references RFC 3986,
which incorporated the changes from RFC2732)

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode59
url/origin.h:59: // TODO(mkwst): Remove tese once blink::WebSerializedOrigin is
gone.
s/these/

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode60
url/origin.h:60: explicit Origin(const std::string& origin);
STYLE: This belongs at line 31, even if it's deprecated.

Rather than grouping these two, you should document this more fully, exactly how
it behaves w/r/t invalid inputs, etc.

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode61
url/origin.h:61: std::string string() const { return serialize(); }
Naming: ToString() / AsString()

[Mike West, 2015-05-28 07:24:17]

Patchset #2 (id:20001) has been deleted

[Mike West, 2015-05-28 07:24:29]

https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
content/public/common/common_param_traits.cc:57: WriteParam(m, p.port());
On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> DANGER: If p.port()'s signature were to hypothetically change, we'd end up in
a mismatch between ReadParam() and WriteParam that wouldn't be obvious (because
line 65 is explicit)
> 
> I'm not sure what the normal IPC idiom is - maybe it doesn't care (e.g. line
81), but that strikes me as a danger

My impression is that this is safe; if we update the signature of `url::Origin`,
we need to update the IPC param traits as well. +tsepez@, who will have a more
canonical opinion.

https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
content/public/common/common_param_traits.cc:65: unsigned short port;
On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> DANGER: This isn't explicitly sized, but it would seem like for IPC, it should
be. (e.g. line 88)

Following HostPortPair's lead, and converting this to a `uint16`.

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc#newcode24
url/origin.cc:24: // Special-case 'file://':
On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> I don't think this should be necessary - that is, it should be entirely
invalid to supply file:// URLs to a scheme/host/port construct because File URLs
are 'empty host', full path.

Dropped the special-case.

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc#newcode25
url/origin.cc:25: if (scheme == kFileScheme) {
On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> BUG: The scheme comparisons should operate case-insentively, hence why I
suggested a CompareScheme function above. An equality check like this won't
help.

Switched to using `url::LowerCaseEqualsASCII` to match `GURL::SchemeIs`.

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc#newcode83
url/origin.cc:83: return SchemeIs(kHttpsScheme) || SchemeIs(kWssScheme);
On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> We should probably be breaking this out from both GURL and Origin and into an
(internal) free function helper

Internal to what?

Note that content/public/common/origin_util.h exists; I think at least some of
this kind of functionality could either be folded into that set of methods, or
brought back into //url.

https://codereview.chromium.org/1153763002/diff/1/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode15
url/origin.h:15: // Origin represents a scheme/host/port tuple, as described in
RFC6454.
On 2015/05/22 at 20:43:36, Ryan Sleevi wrote:
> At the risk of being a pain, I think it would be great to expand this
documentation here, especially if we expect to build more code on top of it.

Makes sense.

> // The second is the concept of the "null" origin [something something]

In Blink::SecurityOrigin, we call this concept "unique", which seems to map well
to RFC 6454's "globally unique origin" terminology. "null" is just the
serialization of a globally unique origin. I'd prefer to be consistent with
Blink here, and call it "unique".

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode44
url/origin.h:44: bool unique() const { return unique_; }
On 2015/05/22 at 20:43:36, Ryan Sleevi wrote:
> I don't recall from the other CL you checking this. If there isn't concrete
code you can point to needing this, I'd actually prefer we not include it at
all, at least not until we can actually demonstrate and explain the need.

Happy to drop it. If it turns out that we need it, adding it back is trivial. I
know we use the SecurityOrigin analog in a number of places in Blink, but
perhaps we don't need it in //content, //net, or //chrome.

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode48
url/origin.h:48: // to a user in browser UI.
On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> s/browser//
> 
> //net, and below (including URL) have zero knowledge or requirement that they
be used as or in a browser.

Got it. Expanded this comment to point to the FAQ about rendering origins to
users.

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode55
url/origin.h:55: // 2. Origins with a
On 2015/05/22 at 20:43:36, Ryan Sleevi wrote:
> Incomplete?

Killed this; we're not divergent.

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode57
url/origin.h:57: std::string serialize() const;
Are you still unhappy with serialization being defined here? Should this be
"ToString"?

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode60
url/origin.h:60: explicit Origin(const std::string& origin);
On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> STYLE: This belongs at line 31, even if it's deprecated.
> 
> Rather than grouping these two, you should document this more fully, exactly
how it behaves w/r/t invalid inputs, etc.

Moved, and incorporated the documentation into the big block at the top.

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode61
url/origin.h:61: std::string string() const { return serialize(); }
On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> Naming: ToString() / AsString()

Can't rename or remove until Blink doesn't use 'string()' anymore.

[Ryan Sleevi, 2015-05-28 07:32:53]

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/1153763002/diff/1/url/origin.cc#newcode83
url/origin.cc:83: return SchemeIs(kHttpsScheme) || SchemeIs(kWssScheme);
On 2015/05/28 07:24:29, Mike West wrote:
> On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> > We should probably be breaking this out from both GURL and Origin and into
an
> (internal) free function helper
> 
> Internal to what?

Oh, to GURL. Same with the scheme comparisons - a non-exported from GURL set of
functions for working across the two (Origins and GURLs)

https://codereview.chromium.org/1153763002/diff/1/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode51
url/origin.h:51: // https://tools.ietf.org/html/rfc6454#section-6, with the
following
Follow-up expansion: Is this serializing to Unicode or ASCII? It's not obvious
from the signature() which


[Secret Answer: If you only allow the inputs to be GURLs, then it's ASCII. If
you let the host be supplied by the caller, and you don't force
canonicalization, you can't really reason about it other than documenting "Do
this" and "Don't do this". As someone who has fought a long hell during reviews
of trying to clarify when brackets are needed and when they are for hostnames
(not if giving to DNS, are if using in URL), I suspect the U-Label/A-Label hell
of hostnames will bite you here if you don't explicitly canonicalize
scheme/host/port bits yourself and document explicitly as such]

https://codereview.chromium.org/1153763002/diff/1/url/origin.h#newcode57
url/origin.h:57: std::string serialize() const;
On 2015/05/28 07:24:29, Mike West wrote:
> Are you still unhappy with serialization being defined here? Should this be
> "ToString"?

Well, naming, s/serialize/Serialize()/, but if the goal is RFC 6454, then I
guess I don't get to grumble.

The concern I have is that people will assume it's for IPC serialization, and
that's maybe-true/maybe-not/hard-to-tell (e.g. it'd be a lossy operation, as
data URIs would serialize to 'null', for example)

[Ryan Sleevi, 2015-05-28 07:33:21]

https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
content/public/common/common_param_traits.cc:57: WriteParam(m, p.port());
On 2015/05/28 07:24:29, Mike West wrote:
> On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> > DANGER: If p.port()'s signature were to hypothetically change, we'd end up
in
> a mismatch between ReadParam() and WriteParam that wouldn't be obvious
(because
> line 65 is explicit)
> > 
> > I'm not sure what the normal IPC idiom is - maybe it doesn't care (e.g. line
> 81), but that strikes me as a danger
> 
> My impression is that this is safe; if we update the signature of
`url::Origin`,
> we need to update the IPC param traits as well. +tsepez@, who will have a more
> canonical opinion.

You didn't add tsepez, fyi

[Ryan Sleevi, 2015-05-28 07:46:30]

https://codereview.chromium.org/1153763002/diff/40001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/1153763002/diff/40001/url/origin.cc#newcode27
url/origin.cc:27: // valid URL, which is unexpected). Otherwise, pass the data
through GURL
Blah. You mind filing a bug on Chrome for this?

Bonus points if you also throw Annevk a bug through the WHATWG to add it to the
interop test cases for the URL living standard

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode10
url/origin.h:10: #include "url/gurl.h"
not needed - forward declare?

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode30
url/origin.h:30: // As you'd expect, there are a few subtleties to note:
STYLE: Not strictly forbidden by
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_...
, but I've seen push-back (and seem to recall list discussion, but I'm being
lazy) about the use of passive voice in comments (right up there with pronouns
in comments considered harmful)

Solution:

"There are a few subtleties to note:"

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode39
url/origin.h:39: //   as well. See
https://www.chromestatus.com/features/5755326842273792.
pronouns blah blah

TODO(mkwst): If this changes in Blink, this should be kept in sync here as well.
See blah

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode47
url/origin.h:47: //   URL". That is
'filesystem:https%3F//example.com/temporary/file.png' is
Is the 3F strictly necessary?

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode50
url/origin.h:50: // * The host component of an IPv6 address includes brackets.
s/brackets./brackets, the same as in the URL representation./

(Basically, explaining *why* this is)

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode68
url/origin.h:68: //   some cases: 'net::AuthHandler', for instance, builds an
authorization cache
LAYERING: Talking about how it's used is a layering issue. Rather than describe
net::AuthHandler's properties, describe why it's a useful property (e.g.
omitting the specific reference)

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode79
url/origin.h:79: Origin(const std::string& scheme, const std::string& host,
uint16 port);
something something what's the form of host

(U-Label or A-Label)

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode91
url/origin.h:91: // about 'blob' URLs at the moment.
s/moment./moment, other than that they exist./

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode95
url/origin.h:95: // 'Origin(GURL([string]))'.
Is this true? I seem to recall abarth@'s argument was that it wouldn't?

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode107
url/origin.h:107: bool SchemeIsCryptographic() const;
If these aren't demonstrably needed yet, it'd be good to omit.

There's also the question whether the pattern should be
GURL::SchemeIs
Origin::SchemeIs

or url::SchemeIs(gurl.scheme() || origin.scheme(), ...)

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode119
url/origin.h:119: //
https://www.chromium.org/Home/chromium-security/enamel#TOC-Presenting-Origins....
My gut is that this is a layering issue. I see a few more have been introduced
into //url recently about referencing higher layers or Chromium. Historically,
this was seen as a bad thing.

That is, do the Chrome Security teams' thoughts on origin display apply to all
embedders? to Chromium Embedded? To Steam? Maybe they're good ideas, but you can
see the tension.

It's more of a nit here, because I really don't want to fight with the people I
suspect would fight over this, and since I agree with the page, I wouldn't argue
that it's *bad*

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode122
url/origin.h:122: // TODO(mkwst): Remove once blink::WebSerializedOrigin is
gone.
something something bug #?

https://codereview.chromium.org/1153763002/diff/40001/url/url_constants.h
File url/url_constants.h (right):

https://codereview.chromium.org/1153763002/diff/40001/url/url_constants.h#new...
url/url_constants.h:33: // The serialization of a globally unique origin.
spec ref to 6454 here

[Mike West, 2015-05-28 12:05:39]

Patchset #3 (id:60001) has been deleted

[Mike West, 2015-05-28 13:49:08]

mkwst@chromium.org changed reviewers:
+ tsepez@chromium.org

[Mike West, 2015-05-28 13:49:09]

On 2015/05/28 at 07:33:21, rsleevi wrote:
>
https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
> File content/public/common/common_param_traits.cc (right):
> 
>
https://codereview.chromium.org/1153763002/diff/1/content/public/common/commo...
> content/public/common/common_param_traits.cc:57: WriteParam(m, p.port());
> On 2015/05/28 07:24:29, Mike West wrote:
> > On 2015/05/22 at 20:43:35, Ryan Sleevi wrote:
> > > DANGER: If p.port()'s signature were to hypothetically change, we'd end up
in
> > a mismatch between ReadParam() and WriteParam that wouldn't be obvious
(because
> > line 65 is explicit)
> > > 
> > > I'm not sure what the normal IPC idiom is - maybe it doesn't care (e.g.
line
> > 81), but that strikes me as a danger
> > 
> > My impression is that this is safe; if we update the signature of
`url::Origin`,
> > we need to update the IPC param traits as well. +tsepez@, who will have a
more
> > canonical opinion.
> 
> You didn't add tsepez, fyi

_actually_ adding tsepez@. :)

https://codereview.chromium.org/1153763002/diff/40001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/1153763002/diff/40001/url/origin.cc#newcode27
url/origin.cc:27: // valid URL, which is unexpected). Otherwise, pass the data
through GURL
On 2015/05/28 at 07:46:29, Ryan Sleevi wrote:
> Blah. You mind filing a bug on Chrome for this?

Sure. https://crbug.com/493123

> Bonus points if you also throw Annevk a bug through the WHATWG to add it to
the interop test cases for the URL living standard

It doesn't seem to be a bug in the public `URL` constructor interface we expose
to the web. Perhaps KURL is acting differently than GURL here?

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode10
url/origin.h:10: #include "url/gurl.h"
On 2015/05/28 07:46:29, Ryan Sleevi wrote:
> not needed - forward declare?

Done.

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode30
url/origin.h:30: // As you'd expect, there are a few subtleties to note:
On 2015/05/28 07:46:30, Ryan Sleevi wrote:
> STYLE: Not strictly forbidden by
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_...
> , but I've seen push-back (and seem to recall list discussion, but I'm being
> lazy) about the use of passive voice in comments (right up there with pronouns
> in comments considered harmful)
> 
> Solution:
> 
> "There are a few subtleties to note:" 

Ok. So comments should be as boring as possible. Check. ;)

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode39
url/origin.h:39: //   as well. See
https://www.chromestatus.com/features/5755326842273792.
On 2015/05/28 07:46:30, Ryan Sleevi wrote:
> pronouns blah blah
> 
> TODO(mkwst): If this changes in Blink, this should be kept in sync here as
well.
> See blah

Actually, this is more complicated than I thought. Blink treats `file:` URLs as
unique, unless the `allow file access from file urls` flag is flipped. //url, of
course, shouldn't know anything about that.

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode47
url/origin.h:47: //   URL". That is
'filesystem:https%3F//example.com/temporary/file.png' is
On 2015/05/28 07:46:30, Ryan Sleevi wrote:
> Is the 3F strictly necessary?

Probably not.

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode50
url/origin.h:50: // * The host component of an IPv6 address includes brackets.
On 2015/05/28 07:46:29, Ryan Sleevi wrote:
> s/brackets./brackets, the same as in the URL representation./
> 
> (Basically, explaining *why* this is)

Done.

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode68
url/origin.h:68: //   some cases: 'net::AuthHandler', for instance, builds an
authorization cache
On 2015/05/28 07:46:30, Ryan Sleevi wrote:
> LAYERING: Talking about how it's used is a layering issue. Rather than
describe
> net::AuthHandler's properties, describe why it's a useful property (e.g.
> omitting the specific reference)

Done (though I think I want to contest the notion that comments can be layering
violations).

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode79
url/origin.h:79: Origin(const std::string& scheme, const std::string& host,
uint16 port);
On 2015/05/28 07:46:30, Ryan Sleevi wrote:
> something something what's the form of host
> 
> (U-Label or A-Label)

A-label.

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode91
url/origin.h:91: // about 'blob' URLs at the moment.
On 2015/05/28 07:46:30, Ryan Sleevi wrote:
> s/moment./moment, other than that they exist./

Done.

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode95
url/origin.h:95: // 'Origin(GURL([string]))'.
On 2015/05/28 07:46:30, Ryan Sleevi wrote:
> Is this true?

Since it's implemented here as 'Init(GURL([string]))', yes, it's true.

> I seem to recall abarth@'s argument was that it wouldn't?

I think abarth@'s argument was that blink::WebSecurityOrigin has data that
`url::Origin` doesn't, so that passing them back and forth would be a lossy
operation.

One thing that occurred to me on this point is that we could add
`content::Origin` as a subclass of `url::Origin` that retained some of those
interesting bits (like universal access, etc). Depending on how the repo merge
goes, we could even make blink::WebSecurityOrigin that subclass directly.

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode107
url/origin.h:107: bool SchemeIsCryptographic() const;
On 2015/05/28 07:46:30, Ryan Sleevi wrote:
> If these aren't demonstrably needed yet, it'd be good to omit.

I can add them when I start replacing `GetOrigin()` calls, I suppose. It seems
pretty clear that one of the operations we do on origins is check the scheme.

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode119
url/origin.h:119: //
https://www.chromium.org/Home/chromium-security/enamel#TOC-Presenting-Origins....
On 2015/05/28 07:46:29, Ryan Sleevi wrote:
> My gut is that this is a layering issue. I see a few more have been introduced
> into //url recently about referencing higher layers or Chromium. Historically,
> this was seen as a bad thing.
> 
> That is, do the Chrome Security teams' thoughts on origin display apply to all
> embedders? to Chromium Embedded? To Steam? Maybe they're good ideas, but you
can
> see the tension.
> 
> It's more of a nit here, because I really don't want to fight with the people
I
> suspect would fight over this, and since I agree with the page, I wouldn't
argue
> that it's *bad*

Then let's wait to see if people want to fight about it, because it seems like
generally good practice that doesn't appear to me to be tightly bound with
Chrome's UI. *shrug*

If more folks push back, fine. If not, I'd like not to preemptively remove good
recommendations.

https://codereview.chromium.org/1153763002/diff/40001/url/origin.h#newcode122
url/origin.h:122: // TODO(mkwst): Remove once blink::WebSerializedOrigin is
gone.
On 2015/05/28 07:46:30, Ryan Sleevi wrote:
> something something bug #?

Done.

[Tom Sepez, 2015-05-28 16:07:53]

These kinds of patches are  usually trouble, because we need to ensure
compatibility between getting an origin from a string, and getting an origin
piecemeal.  It would be bad it we could craft a message so that the receiver
would get a representation that would be impossible to get via a string parse,
for example.

[Tom Sepez, 2015-05-28 16:15:33]

See https://code.google.com/p/chromium/issues/detail?id=166486 for the mess I
nearly stepped into when contemplating the same thing about URLs.

[Mike West, 2015-05-28 17:51:48]

On 2015/05/28 at 16:15:33, tsepez wrote:
> See https://code.google.com/p/chromium/issues/detail?id=166486 for the mess I
nearly stepped into when contemplating the same thing about URLs.

I think the question is a little simpler here, as invalid/non-standard GURLs map
to unique Origins. Here, I think there would only be a lossy conversion in the
case of an invalid GURL (which would spawn an invalid Origin on one side of the
IPC, but a valid, unique Origin on the other side); we could deal with that by
passing along the validity bit as well (e.g. `url::Origin(scheme, host, port,
isvalid)`.

Are there cases I'm missing where parsing an invalid URL on one side of the IPC
would turn into a valid URL on the other? I suspect there are, since you're
warning me about it (but I hope they'd be dealt with via the extra bit). :)

[Tom Sepez, 2015-05-29 15:37:18]

> Are there cases I'm missing where parsing an invalid URL on one side of the
IPC
> would turn into a valid URL on the other? I suspect there are, since you're
> warning me about it (but I hope they'd be dealt with via the extra bit). :)

Usually we get burned by things like embedded NULs, which later muck with the
comparisons.
Do we ever send these from a renderer to the browser?  As I think about it more,
in a site isolation
world, we can't really trust anything the renderer says about origins, so if the
flow is always the
other way, I think we're covered.

[Ryan Sleevi, 2015-05-29 16:29:38]

On 2015/05/29 15:37:18, Tom Sepez wrote:
> > Are there cases I'm missing where parsing an invalid URL on one side of the
> IPC
> > would turn into a valid URL on the other? I suspect there are, since you're
> > warning me about it (but I hope they'd be dealt with via the extra bit). :)
> 
> Usually we get burned by things like embedded NULs, which later muck with the
> comparisons.
> Do we ever send these from a renderer to the browser? 

Yes, we send them from the renderer in the case of WebSockets, and they're fully
trusted. You LG'd that usage, which surprised me ;)

> As I think about it more,
> in a site isolation
> world, we can't really trust anything the renderer says about origins, so if
the
> flow is always the
> other way, I think we're covered.

Agreed. That's why I raised concerns with Mike that we may need to distinguish
"invalid parsed" from "unique" origins, so that the latter can be detected and
rejected programmatically. Moreso its a sign of programmer error, since an
attacker could serialize "unique" origins, but since unique strictly represents
a privilege drop, it's not too bad.

But we are in a state where we are actively using and trusting the renderers for
this.