Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832092:
Added origin property to Uri class Added unit test coverage for origin property (Closed)
|
Created:
8 years, 4 months ago by aam-me Modified:
8 years, 4 months ago CC:
reviews_dartlang.org, cdn1 Visibility:
Public. |
DescriptionAdded origin property to Uri class Added unit test coverage for origin property
BUG=dart:4232
TEST=uri_test
Committed: https://code.google.com/p/dart/source/detail?r=11124
Patch Set 1 #Patch Set 2 : Adjusted line width to be under 80 characters #Patch Set 3 : Error out on non-http/https schemes #
Total comments: 5
Patch Set 4 : Validate scheme, domain. TODO in place for IllegalArgumentException->StateException. Use Expect.thr… #
Total comments: 5
Patch Set 5 : Fixed comments, removed redundant toString, fixed tests #Messages
Total messages: 21 (0 generated)
This Origin business looks a bit more tricky than I expected. I'm trying to find someone with more experience in this area than myself who can help us review this. Thank you very much for uploading this patch!
On 2012/08/02 17:07:54, ahe wrote: > This Origin business looks a bit more tricky than I expected. I'm trying to find > someone with more experience in this area than myself who can help us review > this. > > Thank you very much for uploading this patch! This looks fine to me. The key thing is that if any of scheme, host or port are different this is a distinct origin. Schemes have default ports, so: http://google.com:80 http://google.com are not distinct. The patch above makes no assumptions about the relationship between scheme and port, so it is conservative in this respect anyway. As such I don't see any problems with it.
What is the intended use case for this? I don't have enough context. Is this going to be used to make decisions about whether an action should be allowed or is it a user-facing convenience?
Hi Cris, Thank you for taking a look! On 2012/08/02 19:39:34, Cris Neckar wrote: > What is the intended use case for this? I don't have enough context. Is this > going to be used to make decisions about whether an action should be allowed or > is it a user-facing convenience? The method (aka getter in this case) is similar to WebKit's window.location.origin property. The Uri class is part of the platform libraries of Dart, so it is conceivable that a server would use this method to make decisions, but it will not be used by Chrome for this purpose. Cheers, Peter
> The method (aka getter in this case) is similar to WebKit's > window.location.origin property. > > The Uri class is part of the platform libraries of Dart, so it is conceivable > that a server would use this method to make decisions, but it will not be used > by Chrome for this purpose. My concern is that this doesn't actually implement the whole spec. For example you don't handle cases where the origin should be considered globally unique or strange one offs like javascript uris. That being said as long as this is not being used for security decisions inside Chrome I am probably OK with the simple approach. I would ask though that there be a note in the documentation that emphasizes that this method should not be used for making security decisions (especially since it could potentially be used in server side apps). I'll leave the the code review to someone who understands dart :)
Hi Cris, I feared you would say something like that. So I'm wondering if there is something we could do to simplify the implementation short term, for example, throw an exception if the scheme isn't http or https, or if the hostname isn't in ASCII? Cheers, Peter On 2012/08/02 20:14:32, Cris Neckar wrote: > > The method (aka getter in this case) is similar to WebKit's > > window.location.origin property. > > > > The Uri class is part of the platform libraries of Dart, so it is conceivable > > that a server would use this method to make decisions, but it will not be used > > by Chrome for this purpose. > > My concern is that this doesn't actually implement the whole spec. For example > you don't handle cases where the origin should be considered globally unique or > strange one offs like javascript uris. > > That being said as long as this is not being used for security decisions inside > Chrome I am probably OK with the simple approach. I would ask though that there > be a note in the documentation that emphasizes that this method should not be > used for making security decisions (especially since it could potentially be > used in server side apps). > > I'll leave the the code review to someone who understands dart :)
On 2012/08/02 20:24:51, ahe wrote: > Hi Cris, > > I feared you would say something like that. So I'm wondering if there is > something we could do to simplify the implementation short term, for example, > throw an exception if the scheme isn't http or https, or if the hostname isn't > in ASCII? Yeah I don't have an issue with this implementation provided that it is made clear it shouldn't be used for security critical things. Throwing an exception when an unhandled condition is hit seems reasonable short term though.
On 2012/08/02 20:30:27, Cris Neckar wrote: > Yeah I don't have an issue with this implementation provided that it is made > clear it shouldn't be used for security critical things. Throwing an exception > when an unhandled condition is hit seems reasonable short term though. If we throw exceptions, do we need to say it shouldn't be used for security critical things?
On 2012/08/02 20:37:03, ahe wrote: > On 2012/08/02 20:30:27, Cris Neckar wrote: > > Yeah I don't have an issue with this implementation provided that it is made > > clear it shouldn't be used for security critical things. Throwing an exception > > when an unhandled condition is hit seems reasonable short term though. > > If we throw exceptions, do we need to say it shouldn't be used for security > critical things? Yes, I would prefer to document that this is not a complete implementation.
Thank you, Cris! Alexander, are you up for changing this to be good enough for not needing a security warning? Cheers, Peter On 2012/08/02 20:50:22, Cris Neckar wrote: > On 2012/08/02 20:37:03, ahe wrote: > > On 2012/08/02 20:30:27, Cris Neckar wrote: > > > Yeah I don't have an issue with this implementation provided that it is made > > > clear it shouldn't be used for security critical things. Throwing an > exception > > > when an unhandled condition is hit seems reasonable short term though. > > > > If we throw exceptions, do we need to say it shouldn't be used for security > > critical things? > > Yes, I would prefer to document that this is not a complete implementation.
On 2012/08/02 20:56:06, ahe wrote: > Thank you, Cris! > > Alexander, are you up for changing this to be good enough for not needing a > security warning? > > Cheers, > Peter > > On 2012/08/02 20:50:22, Cris Neckar wrote: > > On 2012/08/02 20:37:03, ahe wrote: > > > On 2012/08/02 20:30:27, Cris Neckar wrote: > > > > Yeah I don't have an issue with this implementation provided that it is > made > > > > clear it shouldn't be used for security critical things. Throwing an > > exception > > > > when an unhandled condition is hit seems reasonable short term though. > > > > > > If we throw exceptions, do we need to say it shouldn't be used for security > > > critical things? > > > > Yes, I would prefer to document that this is not a complete implementation. Do we ultimately need something like WebCore SecurityOrigin? http://code.google.com/p/webkit-mirror/source/browse/Source/WebCore/page/Secu... Looks like it is used to implement Location::origin: http://code.google.com/p/webkit-mirror/source/browse/Source/WebCore/page/Loca...
> Do we ultimately need something like WebCore SecurityOrigin? Yes, best case we would just use the existing code.
> > Do we ultimately need something like WebCore SecurityOrigin? > Yes, best case we would just use the existing code. Wanted to clarify - is WebCore's Location::origin() method that uses SecurityOrigin::toString() good for secure-sensitive logic? Here are the links to code fragments: http://code.google.com/p/webkit-mirror/source/browse/Source/WebCore/page/Secu... http://code.google.com/p/webkit-mirror/source/browse/Source/WebCore/page/Loca... Basically, SecurityOrigin::toString() will return string literal "null" for all unique or 'file://' urls. User has to be aware of such "null" return values if it is to be used in security sensitive applications. It seems that SecurityOrigin class was designed to be used directly in such applications. We can add such class too. But then that could be done in parallel with URI.origin property returning 'scheme://server:port' string.
I believe that the literal "null" is a handled condition which loosely means that the origin can't be well represented. That being said, I am fine with just erroring out for things other than http/https. No need to mention it in documentation. There is such a narrow set of use cases where this will even be relevant that it probably isn't really a concern. Thanks On 2012/08/03 12:45:00, aam wrote: > > > Do we ultimately need something like WebCore SecurityOrigin? > > Yes, best case we would just use the existing code. > > Wanted to clarify - is WebCore's Location::origin() method that uses > SecurityOrigin::toString() good for secure-sensitive logic? > > Here are the links to code fragments: > http://code.google.com/p/webkit-mirror/source/browse/Source/WebCore/page/Secu... > http://code.google.com/p/webkit-mirror/source/browse/Source/WebCore/page/Loca... > > Basically, SecurityOrigin::toString() will return string literal "null" for all > unique or 'file://' urls. User has to be aware of such "null" return values if > it is to be used in security sensitive applications. > It seems that SecurityOrigin class was designed to be used directly in such > applications. We can add such class too. > > But then that could be done in parallel with URI.origin property returning > 'scheme://server:port' string.
On 2012/08/06 20:08:31, Cris Neckar wrote: > > That being said, I am fine with just erroring out for things other than > http/https. Thanks, Cris, for your input. How about throwing IllegalArgumentException for non-http/https? Please take a look at the updated patch if you have a chance.
LGTM from a security perspective. Someone else can give you a code review.
https://chromiumcodereview.appspot.com/10832092/diff/4003/lib/uri/uri.dart File lib/uri/uri.dart (right): https://chromiumcodereview.appspot.com/10832092/diff/4003/lib/uri/uri.dart#ne... lib/uri/uri.dart:175: String get origin() { Please add something like: if (scheme == "") { throw new StateException("Cannot use origin without a scheme"); } https://chromiumcodereview.appspot.com/10832092/diff/4003/lib/uri/uri.dart#ne... lib/uri/uri.dart:177: throw new IllegalArgumentException( This should be a StateException which is coming soon. Please add a TODO, like: // TODO(aprelev@gmail.com): Use StateException instead. https://chromiumcodereview.appspot.com/10832092/diff/4003/lib/uri/uri.dart#ne... lib/uri/uri.dart:181: _addIfNonEmpty(sb, scheme, scheme, ':'); scheme is never empty now. https://chromiumcodereview.appspot.com/10832092/diff/4003/lib/uri/uri.dart#ne... lib/uri/uri.dart:182: if (domain != null || port != 0) { Right now, the invariant is that domain is never null. I do have a CL to change that, but I think this needs to be something like: if (domain === null || domain == "") throw StateException... https://chromiumcodereview.appspot.com/10832092/diff/4003/lib/uri/uri.dart#ne... lib/uri/uri.dart:219: Extra line at end of file.
Thank you, Peter, for the review. I updated the patch accordingly. Please, let me know if there is anything else wrong or missing.
LGTM. Sorry for dropping this. It got buried in my inbox. I'm still working my way through backlog from a vacation that ended three weeks ago :-( Cris, thank you so much for helping us out with the security aspects of this. Cheers, Peter https://chromiumcodereview.appspot.com/10832092/diff/13001/lib/uri/uri.dart File lib/uri/uri.dart (right): https://chromiumcodereview.appspot.com/10832092/diff/13001/lib/uri/uri.dart#n... lib/uri/uri.dart:171: * Returns URI's origin: scheme://domain:port for http/https schemes Comments should be proper sentences. That is, they should end with a period or such. https://chromiumcodereview.appspot.com/10832092/diff/13001/lib/uri/uri.dart#n... lib/uri/uri.dart:172: * Throws IllegalArgumentException for all other schemes Not sure if you want a period or comma on the previous line. If a comma, then "Throws" should be "throws". https://chromiumcodereview.appspot.com/10832092/diff/13001/lib/uri/uri.dart#n... lib/uri/uri.dart:173: * See (http://www.w3.org/TR/2011/WD-html5-20110405/origin-0.html#origin) There is an example on lines 13-15 for how to include a link. https://chromiumcodereview.appspot.com/10832092/diff/13001/lib/uri/uri.dart#n... lib/uri/uri.dart:197: sb.add(port.toString()); I believe toString is no longer necessary. https://chromiumcodereview.appspot.com/10832092/diff/13001/tests/utils/uri_te... File tests/utils/uri_test.dart (right): https://chromiumcodereview.appspot.com/10832092/diff/13001/tests/utils/uri_te... tests/utils/uri_test.dart:126: () => const Uri("http", null, "", 80, "/a/b/c", You probably need to update this to use fromComponents.
Peter, thanks for the comments. Everything should be addressed now.
LGTM. I'll submit it on your behalf. |
