Hardening the 'url::Origin' implementation.

url/origin.h

Index: url/origin.h
diff --git a/url/origin.h b/url/origin.h
index 777e4e1ef481bc062557b2fc8d7fe2437f784c0d..e0e18480fc5d536a8708cdfdbe99236abdad0000 100644
--- a/url/origin.h
+++ b/url/origin.h
@@ -7,27 +7,137 @@
 
 #include <string>
 
+#include "url/gurl.h"

[Ryan Sleevi, 2015/05/28 07:46:29]

not needed - forward declare?

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

Done.

 #include "url/url_export.h"
 
 namespace url {
 
-// Origin represents a Web Origin serialized to a string.
-// See RFC6454 for details.
+// An Origin is a tuple of (scheme, host, port), as described in RFC 6454.
+//
+// Origins are the fundamental component of the web's security model, and
+// represent the boundries within which user agents generally compartmentalize
+// information, and between which user agents enforce access controls. That is,
+// "same-origin" resources are generally more able to directly share information
+// than "cross-origin" resources.
+//
+// This class represents a standard interface which ought to be used when code
+// needs to determine if two resources are "same-origin", and when a canonical
+// serialization of an origin is required.
+//
+// Some origins are "unique", meaning that they are not same-origin with any
+// other origin (including themselves). These are represented as ('', '', 0).
+//
+// As you'd expect, there are a few subtleties to note:

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

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:" 

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

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

+//
+// * Invalid and non-standard GURLs are parsed as unique origins. This includes
+//   non-hierarchical URLs like 'data:text/html,...' and 'javascript:alert(1)'.
+//
+// * GURLs with a scheme of 'file' are parsed as ('file', '', 0), and are
+//   same-origin with any other 'file'-based URL.
+//
+//   TODO(mkwst): If we end up changing this in Blink, we should change it here
+//   as well. See https://www.chromestatus.com/features/5755326842273792.

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

pronouns blah blah

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

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

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.

+//
+// * Unique origins all serialize to the string "null"; this means that the
+//   serialization of two unique origins will be '==' to each other, though the
+//   origins themselves are not "same-origin". This means that origins'
+//   serializations should not be relied upon for security checks.
+//
+// * GURLs with a scheme of 'filesystem' parse the origin out of the "internal
+//   URL". That is 'filesystem:https%3F//example.com/temporary/file.png' is

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

Is the 3F strictly necessary?

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

Probably not.

+//   parsed as ('https', 'example.com', 443).
+//
+// * The host component of an IPv6 address includes brackets.

[Ryan Sleevi, 2015/05/28 07:46:29]

s/brackets./brackets, the same as in the URL representation./

(Basically, explaining *why* this is)

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

Done.

+//
+// Usage:
+//
+// * To answer the question "Are |this| and |that| "same-origin" with each
+//   other?", use |Origin::IsSameOriginWith|:
+//
+//       if (this.IsSameOriginWith(that)) {
+//         // Do something amazing here.
+//       }
+//
+// * To get the serialization of |origin|, use the |serialize| method:
+//
+//       std::string serialization = origin.serialize();
+//
+//   Note that unique origins all serialize to "null"; this means that the
+//   serialization of two origins may '==' each other, even through the origins
+//   themselves are not "same-origin". This might be a useful property to use in
+//   some cases: 'net::AuthHandler', for instance, builds an authorization cache

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

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)

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

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

+//   HashMap based on origin serializations, and folds unique origins into the
+//   "everything else" bucket. Be aware of the difference, however, and choose
+//   wisely.
 class URL_EXPORT Origin {
  public:
+  // Creates a unique Origin.
   Origin();
+
+  // Creates an origin from a scheme/host/port tuple. If the scheme, host, or
+  // port are invalid, a unique origin will be created.
+  Origin(const std::string& scheme, const std::string& host, uint16 port);

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

something something what's the form of host

(U-Label or A-Label)

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

A-label.

+
+  // The mechanism for extracting an origin from a URL is defined in the URL
+  // specification (https://url.spec.whatwg.org/#origin). In particular, note
+  // that:
+  //
+  // * invalid and non-standard URLs are parsed as unique origins
+  // * URLs whose scheme is 'file' are parsed as ('file', '', 0)
+  // * 'filesystem' URLs parse the origin out of the "internal URL"
+  //
+  // TODO(mkwst): 'blob' URLs are incorrectly handled. Perhaps this is an
+  // argument for moving this concept elsewhere, as //url doesn't know anything
+  // about 'blob' URLs at the moment.

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

s/moment./moment, other than that they exist./

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

Done.

+  explicit Origin(const GURL& url);
+
+  // This is a shortcut constructor that has the same effect as
+  // 'Origin(GURL([string]))'.

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

Is this true? I seem to recall abarth@'s argument was that it wouldn't?

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

Since it's implemented here as 'Init(GURL([string]))', yes, it's true.
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.

+  //
+  // TODO(mkwst): Remove this once 'blink::WebSerializedOrigin' is gone.
   explicit Origin(const std::string& origin);
 
-  const std::string& string() const { return string_; }
+  // Returns true if the origin is "the same" as |other|, as defined in Section
+  // 5 of RFC6454: https://tools.ietf.org/html/rfc6454#section-5. Note that
+  // unique origins are not "the same" as any other origin, including
+  // themselves.
+  bool IsSameOriginWith(const Origin& other) const;
+
+  bool SchemeIs(const char* scheme) const;
+  bool SchemeIsCryptographic() const;

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

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(), ...)

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

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.

+
+  std::string scheme() const { return scheme_; }
+  std::string host() const { return host_; }
+  uint16 port() const { return port_; }
+  bool is_valid() const { return valid_; }
 
-  bool IsSameAs(const Origin& that) const {
-    return string_ == that.string_;
-  }
+  // Returns a serialization of the origin as defined in Section 6 of RFC 6454
+  // (https://tools.ietf.org/html/rfc6454#section-6).
+  //
+  // This is _not_ the serialization of the origin which ought to be displayed
+  // to a user: the guidelines for such display are covered in detail at
+  // https://www.chromium.org/Home/chromium-security/enamel#TOC-Presenting-Origins-To-Users.

[Ryan Sleevi, 2015/05/28 07:46:29]

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*

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

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.

+  std::string serialize() const;
+
+  // TODO(mkwst): Remove once blink::WebSerializedOrigin is gone.

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

something something bug #?

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

Done.

+  std::string string() const { return serialize(); }
 
  private:
+  void Init(const GURL& url);
+
+  std::string scheme_;
+  std::string host_;
+  uint16 port_;
+  bool unique_;
+  bool serialization_requires_port_;
+  bool valid_;
+
   std::string string_;
 };
 
+// Stream operator so Origin can be used in assertion statements.
+URL_EXPORT std::ostream& operator<<(std::ostream& out, const url::Origin& url);
+
 }  // namespace url
 
 #endif  // URL_ORIGIN_H_