Introduce url::Origin to represent Web Origin.

Introduce url::Origin to represent Web Origin.

Introduce url::Origin to represent a serialized Web Origin defined in RFC6455.
This class wraps a string representation of blink-side SecurityOrigin object.

BUG=339373
R=tyoshino@chromium.org, ricea@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256789

[yhirano, 2014-02-19 08:15:48]

PTAL?

Using GURL to represent a Web Origin seems not correct.
If this CL is landed we don't need https://codereview.chromium.org/158453004/
any more.

[Adam Rice, 2014-02-19 08:21:50]

lgtm

I agree.

[tyoshino (SeeGerritForStatus), 2014-02-20 04:38:03]

lgtm

[yhirano, 2014-02-21 01:40:12]

+tsepez for websocket_messges.h

[Tom Sepez, 2014-02-21 20:40:19]

On 2014/02/21 01:40:12, yhirano wrote:
> +tsepez for websocket_messges.h

Sorry, but this seems to go against one of our principles.  In order to improve
the ability to audit IPC messages, we never want URLs or origin information
passed as strings.  Rather, we want them passed as a type which clearly
indicates their sensitive nature.

[Tom Sepez, 2014-02-21 20:47:40]

On 2014/02/21 20:40:19, Tom Sepez wrote:
> On 2014/02/21 01:40:12, yhirano wrote:
> > +tsepez for websocket_messges.h
> 
> Sorry, but this seems to go against one of our principles.  In order to
improve
> the ability to audit IPC messages, we never want URLs or origin information
> passed as strings.  Rather, we want them passed as a type which clearly
> indicates their sensitive nature.

+abarth

Now, if you wanted to have a new structure that represented an Origin on the
chromium side, that might be acceptable.  Adam, is there something in blink we
should be exposing (Some kind of a struct { scheme; host; port })?

[abarth-chromium, 2014-02-21 21:50:56]

It's too hard to represent an origin as a struct on the browser side.  It's
sensible to talk about the ASCII representation of an origin, but that loses
information.  There's no way to recover the information once you've converted a
SecurityOrigin object into a string.

[Tom Sepez, 2014-02-22 00:49:17]

Ok. So I guess it stays a GURL.

The reason the security team cares so much about this is that in a
site-isolation world, there is no reason to trust the origin as specified by the
renderer.  The closest check we have is
ChildProcessSecurityPolicy::CanReadCookiesForOrigin() which is becoming
increasingly inaccurately named as it represents "does process X have permission
for Origin Y?"  At some point, you'll want to be calling this. And that
interface takes GURL(), so down the road, you'd instantiate a GURL anyways to
make a security check.  Might as well indicate in the IPC interface that it's
such and should be treated with care.

[abarth-chromium, 2014-02-22 00:58:10]

I mean, you could have a type that wrapped a std::string if you wanted.  Not all
ASCII serializations of origins can be represented using GURL.

[Adam Rice, 2014-02-24 02:25:17]

On 2014/02/22 00:58:10, abarth wrote:
> I mean, you could have a type that wrapped a std::string if you wanted.  Not
all
> ASCII serializations of origins can be represented using GURL.

We were creating the GURL from a string on the renderer side anyway, so all the
GURL was gaining us was some reassurance that the string looked roughly like a
URL.

The original motivation was this change was that GURL::GetOrigin doesn't return
a RFC6454-compliant string, so we had to post-process it, introducing
unnecessary complexity. Is there some existing method to get an
RFC6454-compliant string from a GURL that we overlooked?

[abarth-chromium, 2014-02-24 03:09:04]

On 2014/02/24 02:25:17, Adam Rice wrote:
> On 2014/02/22 00:58:10, abarth wrote:
> > I mean, you could have a type that wrapped a std::string if you wanted.  Not
> all
> > ASCII serializations of origins can be represented using GURL.
> 
> We were creating the GURL from a string on the renderer side anyway, so all
the
> GURL was gaining us was some reassurance that the string looked roughly like a
> URL.
> 
> The original motivation was this change was that GURL::GetOrigin doesn't
return
> a RFC6454-compliant string, so we had to post-process it, introducing
> unnecessary complexity. Is there some existing method to get an
> RFC6454-compliant string from a GURL that we overlooked?

Nope.  GURL predates RFC6454.

[abarth-chromium, 2014-02-24 03:11:04]

I'm confused why you would need that though.  It's not correct to take a
WebDocument's URL and call GetOrigin on it.  The only place you can sensibly get
an origin string for use in WebSockets is from the WebSecurityOrigin associated
with the WebDocument.  In that case, you'll get a WebString.  It's incorrect to
attempt to parse that string as a URL.  It's not a URL.

[Adam Rice, 2014-02-24 03:31:23]

On 2014/02/24 03:11:04, abarth wrote:
> I'm confused why you would need that though.  It's not correct to take a
> WebDocument's URL and call GetOrigin on it.  The only place you can sensibly
get
> an origin string for use in WebSockets is from the WebSecurityOrigin
associated
> with the WebDocument.  In that case, you'll get a WebString.  It's incorrect
to
> attempt to parse that string as a URL.  It's not a URL.

I have verified that that is what we are doing:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

So, to summarise the situation as I understand it, we have agreement that

1. We should definitely not use std::string for the origin in the IPC message
2. We should definitely not use GURL for the origin in the IPC message, and
3. We should definitely not use any other type for the origin in the IPC
message.

Did I miss anything?

[abarth-chromium, 2014-02-24 03:41:09]

On 2014/02/24 03:31:23, Adam Rice wrote:
> So, to summarise the situation as I understand it, we have agreement that
> 
> 1. We should definitely not use std::string for the origin in the IPC message
> 2. We should definitely not use GURL for the origin in the IPC message, and
> 3. We should definitely not use any other type for the origin in the IPC
> message.
> 
> Did I miss anything?

Lol.

We probably need to introduce a new type (e.g., blink::WebOriginString or
content::SerializedOrigin).  That way the IPC message can use a type and you'll
be able to represent all the ASCII serializations of origins.

[yhirano, 2014-02-24 06:09:42]

On 2014/02/24 03:41:09, abarth wrote:
> 
> We probably need to introduce a new type (e.g., blink::WebOriginString or
> content::SerializedOrigin).  That way the IPC message can use a type and
you'll
> be able to represent all the ASCII serializations of origins.

Done in PS3.

[abarth-chromium, 2014-02-24 06:17:06]

This approach seems fine to me.  @tsepez: Thoughts?

https://codereview.chromium.org/170843007/diff/310001/content/common/serializ...
File content/common/serialized_origin.h (right):

https://codereview.chromium.org/170843007/diff/310001/content/common/serializ...
content/common/serialized_origin.h:19: explicit SerializedOrigin(const
blink::WebString& origin);
I probably would have taken a WebSecurityOrigin directly here and called
toString on it.  Maybe that needs to be done in a separate file to keep the
Window split DLL build happy?

https://codereview.chromium.org/170843007/diff/310001/net/websockets/websocke...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/170843007/diff/310001/net/websockets/websocke...
net/websockets/websocket_channel.h:45: const std::string&,
It's too bad we've lost the type information at this point.

[Adam Rice, 2014-02-24 06:48:25]

https://codereview.chromium.org/170843007/diff/310001/content/common/websocke...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/170843007/diff/310001/content/common/websocke...
content/common/websocket_messages.h:48: IPC_STRUCT_TRAITS_MEMBER(string)
Indentation.

[Tom Sepez, 2014-02-24 18:30:32]

LGTM.  To echo Adam's comment, its too bad this isn't defined at the same level
as GURL so the type info can flow all the way down.

Now, there's a follow-up task to find other places that should be using this. 
I'll file a bug after this lands and put myself onto it.

Thanks for taking the time to make this clean.

[Tom Sepez, 2014-02-24 18:30:52]

https://codereview.chromium.org/170843007/diff/310001/content/common/serializ...
File content/common/serialized_origin.h (right):

https://codereview.chromium.org/170843007/diff/310001/content/common/serializ...
content/common/serialized_origin.h:15: 
nit: possibly a comment that explains that this represents an origin as in RFC
6454.

[yhirano, 2014-02-25 01:34:56]

On 2014/02/24 18:30:32, Tom Sepez wrote:
> LGTM.  To echo Adam's comment, its too bad this isn't defined at the same
level
> as GURL so the type info can flow all the way down.
> 

Hmm. Is is OK to
 - Put this class into url/
 - Add conversion functions between WebSecurityOrigin and this class to
WebSecurityOrigin.h (like in WebURL) in a future CL
? > abarth, tsepez

[abarth-chromium, 2014-02-25 07:19:51]

/me defers to Tom.

[yhirano, 2014-03-03 09:04:45]

tsepez, are you OK with #20?

[yhirano, 2014-03-03 15:41:53]

I uploaded PS5 in accordance with #20.

https://codereview.chromium.org/170843007/diff/310001/content/common/serializ...
File content/common/serialized_origin.h (right):

https://codereview.chromium.org/170843007/diff/310001/content/common/serializ...
content/common/serialized_origin.h:15: 
On 2014/02/24 18:30:52, Tom Sepez wrote:
> nit: possibly a comment that explains that this represents an origin as in RFC
> 6454.

Done.

https://codereview.chromium.org/170843007/diff/310001/content/common/serializ...
content/common/serialized_origin.h:19: explicit SerializedOrigin(const
blink::WebString& origin);
On 2014/02/24 06:17:06, abarth wrote:
> I probably would have taken a WebSecurityOrigin directly here and called
> toString on it.  Maybe that needs to be done in a separate file to keep the
> Window split DLL build happy?

I will add a type conversion operator to blink::WebSecurityOrigin in a futuer
CL.

https://codereview.chromium.org/170843007/diff/310001/content/common/websocke...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/170843007/diff/310001/content/common/websocke...
content/common/websocket_messages.h:48: IPC_STRUCT_TRAITS_MEMBER(string)
On 2014/02/24 06:48:26, Adam Rice wrote:
> Indentation.

deleted

https://codereview.chromium.org/170843007/diff/310001/net/websockets/websocke...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/170843007/diff/310001/net/websockets/websocke...
net/websockets/websocket_channel.h:45: const std::string&,
On 2014/02/24 06:17:06, abarth wrote:
> It's too bad we've lost the type information at this point.

Done.

[Tom Sepez, 2014-03-03 19:09:29]

LGTM. +brettw to keep him in the loop.

https://codereview.chromium.org/170843007/diff/440001/content/public/common/c...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/170843007/diff/440001/content/public/common/c...
content/public/common/common_param_traits.cc:8: #include
"content/public/common/content_constants.h"
nit: blank line between system includes and other includes.

[yhirano, 2014-03-03 20:32:50]

https://codereview.chromium.org/170843007/diff/440001/content/public/common/c...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/170843007/diff/440001/content/public/common/c...
content/public/common/common_param_traits.cc:8: #include
"content/public/common/content_constants.h"
On 2014/03/03 19:09:30, Tom Sepez wrote:
> nit: blank line between system includes and other includes.

Done.

[yhirano, 2014-03-06 22:38:03]

brettw: PTAL?

[brettw, 2014-03-07 17:43:51]

I don't see an object named WebOrigin. What is that? Is that a Blink
SecurityOrigin object? It would be nice if the commit message was more explicit
about this.

Is this only needed for WebSockets or is the intention to use this for more
stuff in the future? If this seems likely to only be used by WebSockets, I think
putting it in net/websockets would be a better place. It doesn't seem like this
has to do with parsing and canonicalizing URLs, but rather just a typesafe
wrapper for sending origins across IPC, so would belong at a higher level if
possible.

[yhirano, 2014-03-10 00:54:56]

On 2014/03/07 17:43:51, brettw wrote:
> I don't see an object named WebOrigin. What is that? Is that a Blink
> SecurityOrigin object? It would be nice if the commit message was more
explicit
> about this.
Done.

> Is this only needed for WebSockets or is the intention to use this for more
> stuff in the future? If this seems likely to only be used by WebSockets, I
think
> putting it in net/websockets would be a better place. It doesn't seem like
this
> has to do with parsing and canonicalizing URLs, but rather just a typesafe
> wrapper for sending origins across IPC, so would belong at a higher level if
> possible.
No, as in #18, Tom will create a follow up CL.

[brettw, 2014-03-10 20:59:03]

I feel like "SerializedOrigin" is the wrong name. Looking at it, it's not really
a serialized version of anything. This only make sense if you're thinking about
the WebOrigin object in Blink and making this one out of that, but this is
supposed to be at a lower level.

What about "URLOrigin"? Or "url::Origin".

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.cc
File url/serialized_origin.cc (right):

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.c...
url/serialized_origin.cc:8: #include <string>
You don't need this since the header file uses it.

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.c...
url/serialized_origin.cc:13: : string_(origin) {}
It seems like there is an expected format of the origin information. Is this
something we could assert here? Like could this make a GURL and make sure it's
the same except for a trailing slash?

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.h
File url/serialized_origin.h (right):

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.h...
url/serialized_origin.h:22: bool operator==(const SerializedOrigin& that) const
{
How about writing a real function named like "bool SameOriginAs(...)" which
would be in better keeping with the style guide.

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.h...
url/serialized_origin.h:30: URL_EXPORT std::ostream& operator<<(std::ostream&
out,
I don't think we should have this. I did the one for GURL (as well as the
various operator overloads I mentioned above) despite the style rule against
doing so because it is such a pervasive object. I don't think this one is up to
that level.

[yhirano, 2014-03-12 02:27:05]

Renamed: SerializedOrigin -> url::Origin

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.cc
File url/serialized_origin.cc (right):

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.c...
url/serialized_origin.cc:8: #include <string>
On 2014/03/10 20:59:04, brettw wrote:
> You don't need this since the header file uses it.

Done.

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.c...
url/serialized_origin.cc:13: : string_(origin) {}
On 2014/03/10 20:59:04, brettw wrote:
> It seems like there is an expected format of the origin information. Is this
> something we could assert here? Like could this make a GURL and make sure it's
> the same except for a trailing slash?

Done.

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.h
File url/serialized_origin.h (right):

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.h...
url/serialized_origin.h:22: bool operator==(const SerializedOrigin& that) const
{
On 2014/03/10 20:59:04, brettw wrote:
> How about writing a real function named like "bool SameOriginAs(...)" which
> would be in better keeping with the style guide.

Done.

https://codereview.chromium.org/170843007/diff/600001/url/serialized_origin.h...
url/serialized_origin.h:30: URL_EXPORT std::ostream& operator<<(std::ostream&
out,
On 2014/03/10 20:59:04, brettw wrote:
> I don't think we should have this. I did the one for GURL (as well as the
> various operator overloads I mentioned above) despite the style rule against
> doing so because it is such a pervasive object. I don't think this one is up
to
> that level.

Done.

[brettw, 2014-03-12 05:34:54]

LGTM with suggestion

https://codereview.chromium.org/170843007/diff/660001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/170843007/diff/660001/url/origin.cc#newcode14
url/origin.cc:14: DCHECK(origin == "null" || MatchPattern(origin, "?*://?*"));
Seems to me that this should also accept an empty string, or the default
constructor should initialize to null to make clear that an empty origin string
is always illegal.

https://codereview.chromium.org/170843007/diff/660001/url/origin.cc#newcode15
url/origin.cc:15: DCHECK_NE(origin[origin.size() - 1], '/');
...and this shouldn't crash on empty string.

[yhirano, 2014-03-12 05:48:20]

https://codereview.chromium.org/170843007/diff/660001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/170843007/diff/660001/url/origin.cc#newcode14
url/origin.cc:14: DCHECK(origin == "null" || MatchPattern(origin, "?*://?*"));
On 2014/03/12 05:34:54, brettw wrote:
> Seems to me that this should also accept an empty string, or the default
> constructor should initialize to null to make clear that an empty origin
string
> is always illegal.

Thanks, done.

https://codereview.chromium.org/170843007/diff/660001/url/origin.cc#newcode15
url/origin.cc:15: DCHECK_NE(origin[origin.size() - 1], '/');
On 2014/03/12 05:34:54, brettw wrote:
> ...and this shouldn't crash on empty string.

Done.

[abarth-chromium, 2014-03-12 18:54:59]

url/ LGTM

[yhirano, 2014-03-13 01:21:06]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-13 01:22:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/170843007/710002

[commit-bot: I haz the power, 2014-03-13 03:58:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 03:58:16]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[yhirano, 2014-03-13 04:01:02]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-13 04:01:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/170843007/710002

[yhirano, 2014-03-13 06:57:31]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2014-03-13 06:57:32]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-13 06:57:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/170843007/710002

[commit-bot: I haz the power, 2014-03-13 10:23:00]

Change committed as 256789