Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
574283006:
Fix dictionary domain check problem. (Closed)
|
Created:
6 years, 3 months ago by Randy Smith (Not in Mondays) Modified:
6 years, 3 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix dictionary domain check problem.
Previous to this change, suffixing a "." to the URL from which a
dictionary was being suggested would allow servers to bypass the
check against a subdomain (e.g. evil.protected.good.com) setting a
dictionary to be used by a superdomain (.good.com).
BUG=389451
R=rsleevi@chromium.org
Committed: https://crrev.com/14fd659a2284bc9f301828ada49b8d3fe1f80550
Cr-Commit-Position: refs/heads/master@{#296246}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Incorporated comments and forbid dictionary domains with "."s. #
Total comments: 26
Patch Set 3 : First pass at simpler, targeted fix. #Patch Set 4 : Full implementation of simpler, targeted fix. #Patch Set 5 : Fixed glitch from self-review. #Messages
Total messages: 22 (1 generated)
Ryan, could you take a look at this? The key issue here, to my mind, is making the definition of domain-match in RFC 2965 consistent with the concept of FQDNs. For that, I'm looking for an RFC spec pedant to do the review. If that's not you, feel free to bounce this review (and hopefully suggest someone else :-}). My proposed resolution is to say that FQDN is a DNS concept, and HDN the relevant HTTP concept, and before we do HTTP processing of a host+domain we should make sure it's *not* an FQDN. Feel free to tell me that there's a better way to square this circle.
https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... net/base/sdch_manager.cc:21: bool FQDNToHDN(std::string* host) { 1) Naming: "HDN" is not common 2) RFC 2965 is obsoleted by abarth's RFC 6265 3) HDN refers to a cookie-level concept that I don't think applies here (that is, foo.com == foo.com., whereas for HDN, foo.com != foo.com. ) (c.f. 4.1.2.3 of 6265) https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... net/base/sdch_manager.cc:22: if (host->at(host->size() - 1) != '.') Why are you using at (which throws) vs [] (which doesn't?) https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... net/base/sdch_manager.cc:447: if (!matching_dictionary->CanUse(referring_url_hdn)) 1) Why do these functions use GURLs, rather than just hosts? 2) Rather than stripping '.', why can you not canonicalize by appending '.' if necessary? The latter is what we do in X509Certificate (for RFC6125 hostname matching).
On 2014/09/18 01:22:31, Ryan Sleevi wrote: > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc > File net/base/sdch_manager.cc (right): > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > net/base/sdch_manager.cc:21: bool FQDNToHDN(std::string* host) { > 1) Naming: "HDN" is not common > 2) RFC 2965 is obsoleted by abarth's RFC 6265 > 3) HDN refers to a cookie-level concept that I don't think applies here (that > is, http://foo.com == foo.com., whereas for HDN, http://foo.com != http://foo.com. ) (c.f. 4.1.2.3 of > 6265) > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > net/base/sdch_manager.cc:22: if (host->at(host->size() - 1) != '.') > Why are you using at (which throws) vs [] (which doesn't?) > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > net/base/sdch_manager.cc:447: if > (!matching_dictionary->CanUse(referring_url_hdn)) > 1) Why do these functions use GURLs, rather than just hosts? > 2) Rather than stripping '.', why can you not canonicalize by appending '.' if > necessary? > > The latter is what we do in X509Certificate (for RFC6125 hostname matching). Ryan: Rightly or wrongly, I want to get the philosophy here right before I dive into the coding. The question I find myself asking based on your response is "Is a domain string of the form 'x.y.z.' (i.e. with a trailing period) appropriate in an SDCH dictionary domain name? Is it appropriate in a URL?" Reading 6265 (slight embarrassed look--first RFC I implemented from here), makes me think that a domain with a trailing "." is *not* valid for cookie domains, and hence I'm inclined to think it shouldn't be valid for SDCH domains (the SDCH spec specifically references RFC 2965, RFC 2965 seems to not have a precise reference for HDN, and 6265 seems clear to me that the period shouldn't be there). With regard to the host component of an URL, tracing RFCs gets me to RFC 2396, sec 3.2.2, which makes me think that a trailing period *is* valid for the host component of a URL (to distinguish between relative and absolute domains). So I think this means that there is an incompleteness in the SDCH spec (paralleling the same one in 2965), and that the behavior we want is to disallow "." in the domain names of SDCH dictionaries, and if we need to compare between a domain and a host URL component, to canonicalize them both (either strip periods from the URL, or add one to the domain). Does that match your understanding/belief/opinion? If so, I'll dive into the implementation.
On 2014/09/18 18:30:05, rdsmith wrote: > On 2014/09/18 01:22:31, Ryan Sleevi wrote: > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc > > File net/base/sdch_manager.cc (right): > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > net/base/sdch_manager.cc:21: bool FQDNToHDN(std::string* host) { > > 1) Naming: "HDN" is not common > > 2) RFC 2965 is obsoleted by abarth's RFC 6265 > > 3) HDN refers to a cookie-level concept that I don't think applies here (that > > is, http://foo.com == foo.com., whereas for HDN, http://foo.com != > http://foo.com. ) (c.f. 4.1.2.3 of > > 6265) > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > net/base/sdch_manager.cc:22: if (host->at(host->size() - 1) != '.') > > Why are you using at (which throws) vs [] (which doesn't?) > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > net/base/sdch_manager.cc:447: if > > (!matching_dictionary->CanUse(referring_url_hdn)) > > 1) Why do these functions use GURLs, rather than just hosts? > > 2) Rather than stripping '.', why can you not canonicalize by appending '.' if > > necessary? > > > > The latter is what we do in X509Certificate (for RFC6125 hostname matching). > > Ryan: Rightly or wrongly, I want to get the philosophy here right before I dive > into the coding. The question I find myself asking based on your response is > "Is a domain string of the form 'x.y.z.' (i.e. with a trailing period) > appropriate in an SDCH dictionary domain name? Is it appropriate in a URL?" > > Reading 6265 (slight embarrassed look--first RFC I implemented from here), makes > me think that a domain with a trailing "." is *not* valid for cookie domains, > and hence I'm inclined to think it shouldn't be valid for SDCH domains (the SDCH > spec specifically references RFC 2965, RFC 2965 seems to not have a precise > reference for HDN, and 6265 seems clear to me that the period shouldn't be > there). With regard to the host component of an URL, tracing RFCs gets me to > RFC 2396, sec 3.2.2, which makes me think that a trailing period *is* valid for > the host component of a URL (to distinguish between relative and absolute > domains). So I think this means that there is an incompleteness in the SDCH > spec (paralleling the same one in 2965), and that the behavior we want is to > disallow "." in the domain names of SDCH dictionaries, and if we need to compare > between a domain and a host URL component, to canonicalize them both (either > strip periods from the URL, or add one to the domain). > > Does that match your understanding/belief/opinion? If so, I'll dive into the > implementation. Ryan: Ping?
On 2014/09/22 16:30:02, rdsmith wrote: > On 2014/09/18 18:30:05, rdsmith wrote: > > On 2014/09/18 01:22:31, Ryan Sleevi wrote: > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc > > > File net/base/sdch_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > net/base/sdch_manager.cc:21: bool FQDNToHDN(std::string* host) { > > > 1) Naming: "HDN" is not common > > > 2) RFC 2965 is obsoleted by abarth's RFC 6265 > > > 3) HDN refers to a cookie-level concept that I don't think applies here > (that > > > is, http://foo.com == foo.com., whereas for HDN, http://foo.com != > > http://foo.com. ) (c.f. 4.1.2.3 of > > > 6265) > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > net/base/sdch_manager.cc:22: if (host->at(host->size() - 1) != '.') > > > Why are you using at (which throws) vs [] (which doesn't?) > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > net/base/sdch_manager.cc:447: if > > > (!matching_dictionary->CanUse(referring_url_hdn)) > > > 1) Why do these functions use GURLs, rather than just hosts? > > > 2) Rather than stripping '.', why can you not canonicalize by appending '.' > if > > > necessary? > > > > > > The latter is what we do in X509Certificate (for RFC6125 hostname matching). > > > > Ryan: Rightly or wrongly, I want to get the philosophy here right before I > dive > > into the coding. The question I find myself asking based on your response is > > "Is a domain string of the form 'x.y.z.' (i.e. with a trailing period) > > appropriate in an SDCH dictionary domain name? Is it appropriate in a URL?" > > > > Reading 6265 (slight embarrassed look--first RFC I implemented from here), > makes > > me think that a domain with a trailing "." is *not* valid for cookie domains, > > and hence I'm inclined to think it shouldn't be valid for SDCH domains (the > SDCH > > spec specifically references RFC 2965, RFC 2965 seems to not have a precise > > reference for HDN, and 6265 seems clear to me that the period shouldn't be > > there). With regard to the host component of an URL, tracing RFCs gets me to > > RFC 2396, sec 3.2.2, which makes me think that a trailing period *is* valid > for > > the host component of a URL (to distinguish between relative and absolute > > domains). So I think this means that there is an incompleteness in the SDCH > > spec (paralleling the same one in 2965), and that the behavior we want is to > > disallow "." in the domain names of SDCH dictionaries, and if we need to > compare > > between a domain and a host URL component, to canonicalize them both (either > > strip periods from the URL, or add one to the domain). > > > > Does that match your understanding/belief/opinion? If so, I'll dive into the > > implementation. > > Ryan: Ping? Sorry, I missed this comment when going through my code reviews. It's important to note that RFC2965 *intentionally* (as does 6265) diverge from what is valid in a URL and the semantics of a trailing dot. This is one of those "cookies are special". If SDCH is trying to run parallel to cookies, _it is doing it wrong_, and that's a bad spec, and we shouldn't encourage it. I haven't read the SDCH spec (is there even one that is current? I know there's the paper, but at this point, we seem to have fairly diverged from that), but here's the thoughts: - similar to RFC 6125, you have two types of URLs - the 'presented' URL (those from the SDCH dictionary) and the 'reference' URL (that from the user) - As users are humans (hopefully, unless the machine uprising happened and I missed the menu), they're fallible, and thus may type in URLs sloppily. In this case, a reference URL MAY or MAY NOT be entered with trailing periods. - For dictionaries, the SDCH spec should be opinionated and indicate what the canonical form for encoding an SDCH dictionary is. Naively, I would expect it NOT have a trailing period, lest there be any confusion with the cookie-domain semantics - When matching, you normalize both (add a . or remove the ., it's the same), and compare The notion of "absolute" vs "relative" domains is murky, and I'd avoid that language all together, lest it muddy the waters. To directly answer your question: Is http://x.y.z./ (trailing dot) valid as a URL? Unquestionably, yes, and your code must be prepared to handle it. Is x.y.z. (trailing dot) valid in an SDCH dictionary? It depends on you/the spec. You can say yes (in which case, if the SDCH spec is saying anything about 'cookie domains', you have to be clear that the trailing dot does NOT imply cookie-special behaviour). You can say no (in which case, you could/should reject the dictionary). That's up to you / the SDCH spec authors.
On 2014/09/22 16:50:38, Ryan Sleevi wrote: > On 2014/09/22 16:30:02, rdsmith wrote: > > On 2014/09/18 18:30:05, rdsmith wrote: > > > On 2014/09/18 01:22:31, Ryan Sleevi wrote: > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc > > > > File net/base/sdch_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > > net/base/sdch_manager.cc:21: bool FQDNToHDN(std::string* host) { > > > > 1) Naming: "HDN" is not common > > > > 2) RFC 2965 is obsoleted by abarth's RFC 6265 > > > > 3) HDN refers to a cookie-level concept that I don't think applies here > > (that > > > > is, http://foo.com == foo.com., whereas for HDN, http://foo.com != > > > http://foo.com. ) (c.f. 4.1.2.3 of > > > > 6265) > > > > > > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > > net/base/sdch_manager.cc:22: if (host->at(host->size() - 1) != '.') > > > > Why are you using at (which throws) vs [] (which doesn't?) > > > > > > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > > net/base/sdch_manager.cc:447: if > > > > (!matching_dictionary->CanUse(referring_url_hdn)) > > > > 1) Why do these functions use GURLs, rather than just hosts? > > > > 2) Rather than stripping '.', why can you not canonicalize by appending > '.' > > if > > > > necessary? > > > > > > > > The latter is what we do in X509Certificate (for RFC6125 hostname > matching). > > > > > > Ryan: Rightly or wrongly, I want to get the philosophy here right before I > > dive > > > into the coding. The question I find myself asking based on your response > is > > > "Is a domain string of the form 'x.y.z.' (i.e. with a trailing period) > > > appropriate in an SDCH dictionary domain name? Is it appropriate in a URL?" > > > > > > > Reading 6265 (slight embarrassed look--first RFC I implemented from here), > > makes > > > me think that a domain with a trailing "." is *not* valid for cookie > domains, > > > and hence I'm inclined to think it shouldn't be valid for SDCH domains (the > > SDCH > > > spec specifically references RFC 2965, RFC 2965 seems to not have a precise > > > reference for HDN, and 6265 seems clear to me that the period shouldn't be > > > there). With regard to the host component of an URL, tracing RFCs gets me > to > > > RFC 2396, sec 3.2.2, which makes me think that a trailing period *is* valid > > for > > > the host component of a URL (to distinguish between relative and absolute > > > domains). So I think this means that there is an incompleteness in the SDCH > > > spec (paralleling the same one in 2965), and that the behavior we want is to > > > disallow "." in the domain names of SDCH dictionaries, and if we need to > > compare > > > between a domain and a host URL component, to canonicalize them both (either > > > strip periods from the URL, or add one to the domain). > > > > > > Does that match your understanding/belief/opinion? If so, I'll dive into > the > > > implementation. > > > > Ryan: Ping? > > Sorry, I missed this comment when going through my code reviews. No worries, 'tis what pings are for. Thanks for the detailed response; it's exactly what I was looking for. > It's important to note that RFC2965 *intentionally* (as does 6265) diverge from > what is valid in a URL and the semantics of a trailing dot. This is one of those > "cookies are special". If SDCH is trying to run parallel to cookies, _it is > doing it wrong_, and that's a bad spec, and we shouldn't encourage it. Well, this is why I asked you to review this CL :-}. So as I read the SDCH spec, it is indeed trying to run parallel to cookies. If you'd like to give me a sense as to in what ways cookies diverge from the ideal (other than not including the trailing dot), I can propose changes to the SDCH spec to move it back towards the ideal. > I haven't read the SDCH spec (is there even one that is current? I know there's > the paper, but at this point, we seem to have fairly diverged from that), but > here's the thoughts: Current working SDCH spec is available at https://docs.google.com/a/chromium.org/document/d/1REMkwjXY5yFOkJwtJPjCMwZ4Sh... . > - similar to RFC 6125, you have two types of URLs - the 'presented' URL (those > from the SDCH dictionary) and the 'reference' URL (that from the user) > - As users are humans (hopefully, unless the machine uprising happened and I > missed the menu), they're fallible, and thus may type in URLs sloppily. In this > case, a reference URL MAY or MAY NOT be entered with trailing periods. > - For dictionaries, the SDCH spec should be opinionated and indicate what the > canonical form for encoding an SDCH dictionary is. Naively, I would expect it > NOT have a trailing period, lest there be any confusion with the cookie-domain > semantics > - When matching, you normalize both (add a . or remove the ., it's the same), > and compare > > The notion of "absolute" vs "relative" domains is murky, and I'd avoid that > language all together, lest it muddy the waters. > > To directly answer your question: > Is http://x.y.z./ (trailing dot) valid as a URL? Unquestionably, yes, and your > code must be prepared to handle it. > Is x.y.z. (trailing dot) valid in an SDCH dictionary? It depends on you/the > spec. You can say yes (in which case, if the SDCH spec is saying anything about > 'cookie domains', you have to be clear that the trailing dot does NOT imply > cookie-special behaviour). You can say no (in which case, you could/should > reject the dictionary). That's up to you / the SDCH spec authors. Ok. Based on that, I'm going to make my current plan the following; please let me know if you have any objections: * I'm going to shift the code to match what I currently take as spec intent, and a) make sure we don't allow "."s in dictionary domains, b) strip off any "."s in URLs before comparing with dictionary domains. I want to get this done this weak, as the bug in question has security implications (not major ones, admittedly, given the current usage of SDCH, but still there). * WRT spec changes, I'll accept "in the spirt of the spec" as ok for the chrome implementation in the current context, but propose a spec change to match what I'm doing in the code. I don't do that as quickly (i.e. not aim to be done this week) and I'd like to have your input around the ways in which the SDCH spec is "doing it wrong" before I propose the change; I'll aim for that to be next week. No response needed from you on this unless you disagree with my plan; I'll throw another patch set at you later today.
On 2014/09/22 17:40:08, rdsmith wrote: > On 2014/09/22 16:50:38, Ryan Sleevi wrote: > > On 2014/09/22 16:30:02, rdsmith wrote: > > > On 2014/09/18 18:30:05, rdsmith wrote: > > > > On 2014/09/18 01:22:31, Ryan Sleevi wrote: > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc > > > > > File net/base/sdch_manager.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > > > net/base/sdch_manager.cc:21: bool FQDNToHDN(std::string* host) { > > > > > 1) Naming: "HDN" is not common > > > > > 2) RFC 2965 is obsoleted by abarth's RFC 6265 > > > > > 3) HDN refers to a cookie-level concept that I don't think applies here > > > (that > > > > > is, http://foo.com == foo.com., whereas for HDN, http://foo.com != > > > > http://foo.com. ) (c.f. 4.1.2.3 of > > > > > 6265) > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > > > net/base/sdch_manager.cc:22: if (host->at(host->size() - 1) != '.') > > > > > Why are you using at (which throws) vs [] (which doesn't?) > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > > > net/base/sdch_manager.cc:447: if > > > > > (!matching_dictionary->CanUse(referring_url_hdn)) > > > > > 1) Why do these functions use GURLs, rather than just hosts? > > > > > 2) Rather than stripping '.', why can you not canonicalize by appending > > '.' > > > if > > > > > necessary? > > > > > > > > > > The latter is what we do in X509Certificate (for RFC6125 hostname > > matching). > > > > > > > > Ryan: Rightly or wrongly, I want to get the philosophy here right before I > > > dive > > > > into the coding. The question I find myself asking based on your response > > is > > > > "Is a domain string of the form 'x.y.z.' (i.e. with a trailing period) > > > > appropriate in an SDCH dictionary domain name? Is it appropriate in a > URL?" > > > > > > > > > > Reading 6265 (slight embarrassed look--first RFC I implemented from here), > > > makes > > > > me think that a domain with a trailing "." is *not* valid for cookie > > domains, > > > > and hence I'm inclined to think it shouldn't be valid for SDCH domains > (the > > > SDCH > > > > spec specifically references RFC 2965, RFC 2965 seems to not have a > precise > > > > reference for HDN, and 6265 seems clear to me that the period shouldn't be > > > > there). With regard to the host component of an URL, tracing RFCs gets me > > to > > > > RFC 2396, sec 3.2.2, which makes me think that a trailing period *is* > valid > > > for > > > > the host component of a URL (to distinguish between relative and absolute > > > > domains). So I think this means that there is an incompleteness in the > SDCH > > > > spec (paralleling the same one in 2965), and that the behavior we want is > to > > > > disallow "." in the domain names of SDCH dictionaries, and if we need to > > > compare > > > > between a domain and a host URL component, to canonicalize them both > (either > > > > strip periods from the URL, or add one to the domain). > > > > > > > > Does that match your understanding/belief/opinion? If so, I'll dive into > > the > > > > implementation. > > > > > > Ryan: Ping? > > > > Sorry, I missed this comment when going through my code reviews. > > No worries, 'tis what pings are for. Thanks for the detailed response; it's > exactly what I was looking for. > > > It's important to note that RFC2965 *intentionally* (as does 6265) diverge > from > > what is valid in a URL and the semantics of a trailing dot. This is one of > those > > "cookies are special". If SDCH is trying to run parallel to cookies, _it is > > doing it wrong_, and that's a bad spec, and we shouldn't encourage it. > > Well, this is why I asked you to review this CL :-}. So as I read the SDCH > spec, it is indeed trying to run parallel to cookies. If you'd like to give me > a sense as to in what ways cookies diverge from the ideal (other than not > including the trailing dot), I can propose changes to the SDCH spec to move it > back towards the ideal. > > > I haven't read the SDCH spec (is there even one that is current? I know > there's > > the paper, but at this point, we seem to have fairly diverged from that), but > > here's the thoughts: > > Current working SDCH spec is available at > https://docs.google.com/a/chromium.org/document/d/1REMkwjXY5yFOkJwtJPjCMwZ4Sh... > . > > > - similar to RFC 6125, you have two types of URLs - the 'presented' URL (those > > from the SDCH dictionary) and the 'reference' URL (that from the user) > > - As users are humans (hopefully, unless the machine uprising happened and I > > missed the menu), they're fallible, and thus may type in URLs sloppily. In > this > > case, a reference URL MAY or MAY NOT be entered with trailing periods. > > - For dictionaries, the SDCH spec should be opinionated and indicate what the > > canonical form for encoding an SDCH dictionary is. Naively, I would expect it > > NOT have a trailing period, lest there be any confusion with the cookie-domain > > semantics > > - When matching, you normalize both (add a . or remove the ., it's the same), > > and compare > > > > The notion of "absolute" vs "relative" domains is murky, and I'd avoid that > > language all together, lest it muddy the waters. > > > > To directly answer your question: > > Is http://x.y.z./ (trailing dot) valid as a URL? Unquestionably, yes, and your > > code must be prepared to handle it. > > Is x.y.z. (trailing dot) valid in an SDCH dictionary? It depends on you/the > > spec. You can say yes (in which case, if the SDCH spec is saying anything > about > > 'cookie domains', you have to be clear that the trailing dot does NOT imply > > cookie-special behaviour). You can say no (in which case, you could/should > > reject the dictionary). That's up to you / the SDCH spec authors. > > Ok. Based on that, I'm going to make my current plan the following; please let > me know if you have any objections: > * I'm going to shift the code to match what I currently take as spec intent, and > a) make sure we don't allow "."s in dictionary domains, b) strip off any "."s in > URLs before comparing with dictionary domains. I want to get this done this > weak, as the bug in question has security implications (not major ones, > admittedly, given the current usage of SDCH, but still there). > * WRT spec changes, I'll accept "in the spirt of the spec" as ok for the chrome > implementation in the current context, but propose a spec change to match what > I'm doing in the code. I don't do that as quickly (i.e. not aim to be done this > week) and I'd like to have your input around the ways in which the SDCH spec is > "doing it wrong" before I propose the change; I'll aim for that to be next week. > > > No response needed from you on this unless you disagree with my plan; I'll throw > another patch set at you later today. Plan sounds good, and I'll add the spec to the list of specs to comment on today.
On 2014/09/22 17:40:08, rdsmith wrote: > On 2014/09/22 16:50:38, Ryan Sleevi wrote: > > On 2014/09/22 16:30:02, rdsmith wrote: > > > On 2014/09/18 18:30:05, rdsmith wrote: > > > > On 2014/09/18 01:22:31, Ryan Sleevi wrote: > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc > > > > > File net/base/sdch_manager.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > > > net/base/sdch_manager.cc:21: bool FQDNToHDN(std::string* host) { > > > > > 1) Naming: "HDN" is not common > > > > > 2) RFC 2965 is obsoleted by abarth's RFC 6265 > > > > > 3) HDN refers to a cookie-level concept that I don't think applies here > > > (that > > > > > is, http://foo.com == foo.com., whereas for HDN, http://foo.com != > > > > http://foo.com. ) (c.f. 4.1.2.3 of > > > > > 6265) > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > > > net/base/sdch_manager.cc:22: if (host->at(host->size() - 1) != '.') > > > > > Why are you using at (which throws) vs [] (which doesn't?) > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... > > > > > net/base/sdch_manager.cc:447: if > > > > > (!matching_dictionary->CanUse(referring_url_hdn)) > > > > > 1) Why do these functions use GURLs, rather than just hosts? > > > > > 2) Rather than stripping '.', why can you not canonicalize by appending > > '.' > > > if > > > > > necessary? > > > > > > > > > > The latter is what we do in X509Certificate (for RFC6125 hostname > > matching). > > > > > > > > Ryan: Rightly or wrongly, I want to get the philosophy here right before I > > > dive > > > > into the coding. The question I find myself asking based on your response > > is > > > > "Is a domain string of the form 'x.y.z.' (i.e. with a trailing period) > > > > appropriate in an SDCH dictionary domain name? Is it appropriate in a > URL?" > > > > > > > > > > Reading 6265 (slight embarrassed look--first RFC I implemented from here), > > > makes > > > > me think that a domain with a trailing "." is *not* valid for cookie > > domains, > > > > and hence I'm inclined to think it shouldn't be valid for SDCH domains > (the > > > SDCH > > > > spec specifically references RFC 2965, RFC 2965 seems to not have a > precise > > > > reference for HDN, and 6265 seems clear to me that the period shouldn't be > > > > there). With regard to the host component of an URL, tracing RFCs gets me > > to > > > > RFC 2396, sec 3.2.2, which makes me think that a trailing period *is* > valid > > > for > > > > the host component of a URL (to distinguish between relative and absolute > > > > domains). So I think this means that there is an incompleteness in the > SDCH > > > > spec (paralleling the same one in 2965), and that the behavior we want is > to > > > > disallow "." in the domain names of SDCH dictionaries, and if we need to > > > compare > > > > between a domain and a host URL component, to canonicalize them both > (either > > > > strip periods from the URL, or add one to the domain). > > > > > > > > Does that match your understanding/belief/opinion? If so, I'll dive into > > the > > > > implementation. > > > > > > Ryan: Ping? > > > > Sorry, I missed this comment when going through my code reviews. > > No worries, 'tis what pings are for. Thanks for the detailed response; it's > exactly what I was looking for. > > > It's important to note that RFC2965 *intentionally* (as does 6265) diverge > from > > what is valid in a URL and the semantics of a trailing dot. This is one of > those > > "cookies are special". If SDCH is trying to run parallel to cookies, _it is > > doing it wrong_, and that's a bad spec, and we shouldn't encourage it. > > Well, this is why I asked you to review this CL :-}. So as I read the SDCH > spec, it is indeed trying to run parallel to cookies. If you'd like to give me > a sense as to in what ways cookies diverge from the ideal (other than not > including the trailing dot), I can propose changes to the SDCH spec to move it > back towards the ideal. > > > I haven't read the SDCH spec (is there even one that is current? I know > there's > > the paper, but at this point, we seem to have fairly diverged from that), but > > here's the thoughts: > > Current working SDCH spec is available at > https://docs.google.com/a/chromium.org/document/d/1REMkwjXY5yFOkJwtJPjCMwZ4Sh... > . > > > - similar to RFC 6125, you have two types of URLs - the 'presented' URL (those > > from the SDCH dictionary) and the 'reference' URL (that from the user) > > - As users are humans (hopefully, unless the machine uprising happened and I > > missed the menu), they're fallible, and thus may type in URLs sloppily. In > this > > case, a reference URL MAY or MAY NOT be entered with trailing periods. > > - For dictionaries, the SDCH spec should be opinionated and indicate what the > > canonical form for encoding an SDCH dictionary is. Naively, I would expect it > > NOT have a trailing period, lest there be any confusion with the cookie-domain > > semantics > > - When matching, you normalize both (add a . or remove the ., it's the same), > > and compare > > > > The notion of "absolute" vs "relative" domains is murky, and I'd avoid that > > language all together, lest it muddy the waters. > > > > To directly answer your question: > > Is http://x.y.z./ (trailing dot) valid as a URL? Unquestionably, yes, and your > > code must be prepared to handle it. > > Is x.y.z. (trailing dot) valid in an SDCH dictionary? It depends on you/the > > spec. You can say yes (in which case, if the SDCH spec is saying anything > about > > 'cookie domains', you have to be clear that the trailing dot does NOT imply > > cookie-special behaviour). You can say no (in which case, you could/should > > reject the dictionary). That's up to you / the SDCH spec authors. > > Ok. Based on that, I'm going to make my current plan the following; please let > me know if you have any objections: > * I'm going to shift the code to match what I currently take as spec intent, and > a) make sure we don't allow "."s in dictionary domains, b) strip off any "."s in > URLs before comparing with dictionary domains. I want to get this done this > weak, as the bug in question has security implications (not major ones, > admittedly, given the current usage of SDCH, but still there). > * WRT spec changes, I'll accept "in the spirt of the spec" as ok for the chrome > implementation in the current context, but propose a spec change to match what > I'm doing in the code. I don't do that as quickly (i.e. not aim to be done this > week) and I'd like to have your input around the ways in which the SDCH spec is > "doing it wrong" before I propose the change; I'll aim for that to be next week. > > > No response needed from you on this unless you disagree with my plan; I'll throw > another patch set at you later today. Plan sounds good, and I'll add the spec to the list of specs to comment on today.
Ryan: New upload, PTAL. https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... net/base/sdch_manager.cc:21: bool FQDNToHDN(std::string* host) { On 2014/09/18 01:22:31, Ryan Sleevi wrote: > 1) Naming: "HDN" is not common > 2) RFC 2965 is obsoleted by abarth's RFC 6265 > 3) HDN refers to a cookie-level concept that I don't think applies here (that > is, http://foo.com == foo.com., whereas for HDN, http://foo.com != http://foo.com. ) (c.f. 4.1.2.3 of > 6265) Changed to "StripTrailingDot" and edited the explanatory text. I'll note for possible future discussion: I wouldn't mind having functions that refer directly to the controlling terminology in the RFCs (which is where FQDNToHDN came from). However, based on our discussion, I don't think the relationship is precisely specified in the RFCs, so I'll go with the concrete description of what I'm doing. https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... net/base/sdch_manager.cc:22: if (host->at(host->size() - 1) != '.') On 2014/09/18 01:22:31, Ryan Sleevi wrote: > Why are you using at (which throws) vs [] (which doesn't?) Because I found (*host)[] awkward and wasn't aware of the exceptional behavior :-}. Done. https://codereview.chromium.org/574283006/diff/1/net/base/sdch_manager.cc#new... net/base/sdch_manager.cc:447: if (!matching_dictionary->CanUse(referring_url_hdn)) On 2014/09/18 01:22:30, Ryan Sleevi wrote: > 1) Why do these functions use GURLs, rather than just hosts? I'm presuming you mean IsInSupportedDomain() and CanUse(). CanUse() checks path & port as well as host. IsInSupportedDomain() checks the scheme as well as the host/domain. IsInSupportedDomain() is problematic for other reasons, and I'd be open to a refactor (though I'd want to do it in such a way as to not cause even more problems for CL 423813002) but I'm basically happy with the abstraction boundary for CanUse(). > 2) Rather than stripping '.', why can you not canonicalize by appending '.' if > necessary? > > The latter is what we do in X509Certificate (for RFC6125 hostname matching). So I'm willing to do that for consistency sake, and will if you ask me again, but I'd rather keep the code and spec in alignment. I.e. if the spec prohibits a trailing "." for dictionary domains, I'd rather keep the dictionary domain free of the trailing ".", and strip it from the URL if it's there.
Now that I've had more time to read, I'm more confused why this is necessary. Apologies for the round-trip time, juggling a few other high priority items at this point. https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:26: if (host->size() == 0) host->empty() (a nit I hold on to from the days of 'dumb' stl implementations, but also the generic form of not requiring counting like other containers MAY) https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:29: if ((*host)[host->size() - 1] != '.') if (*host->rbegin() != '.') return false; Laziness is a virtue :) https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:36: void StripTrailingDotURL(GURL* gurl) { naming wise, this felt a little weird, although I understand how you got here. Perhaps just something as "NormalizeToDictionaryURL"? https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:40: return; Just inline this? You never use "StripTrailingDot" in this file https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:454: if (!matching_dictionary->CanUse(referring_url_hdn)) OK, I'm fairly confused now why it's necessary to do this. SdchManager::Dictionary::CanUse uses DomainMatch, which just calls gurl.DomainIs(...) and DomainIs handles the trailing . just fine (c.f https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&rcl=14... )
Responding quickly to your question based on my preference for nailing down the philosophy before doing code hacking :-}. https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:454: if (!matching_dictionary->CanUse(referring_url_hdn)) On 2014/09/22 20:30:37, Ryan Sleevi wrote: > OK, I'm fairly confused now why it's necessary to do this. > > SdchManager::Dictionary::CanUse uses DomainMatch, which just calls > > gurl.DomainIs(...) > > and DomainIs handles the trailing . just fine (c.f > https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&rcl=14... > ) This isn't necessary; it's the checks in AddSdchDictionary that are necessary. The failure that produced this CL was the referring URL for an added dictionary having a "." at the end and also being a subdomain of the domain it was trying to set an encoding dictionary for, which should have been bounced because you aren't allowed to set dictionaries for parent domains (i.e. you can't set a dictionary for google.com from www.hosted.google.com). It's a pure suffix check (as specified in the spec :-J), not done through DomainMatch, which failed when the trailing "." was present. I put it here (and forgot to change the naming, sorry) because I was doing a general change to strip trailing dots from referring URLs; I didn't want to have code in one place that did the strip and in another that didn't. If you'd like, I can convert this to a comment to the effect that CanUse uses DomainMatch(), which handles the trailing ".". But I suspect what you want to focus on is what the code in Dictionary::CanSet() and callers looks like, and then figure out from that whether or not we want a change here.
https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:523: void SdchManager::AddSdchDictionary(const std::string& dictionary_text, style nits: Perhaps for a future CL, but this should be on a line by itself, per the style guide, or dictionary_url should be aligned to the open ( https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:524: const GURL& dictionary_url_const) { pedantry naming: dictionary_url here, and normalized_dictionary_url below (which, yes, requires more code line changes) I just find the _const an unnerving "Reverse Hungarian Notation" ;) https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:552: DCHECK_LE(line_end, header_end); This is really unnerving. Shouldn't these be non-CHECK, handled errors? https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:560: if (colon_index > line_end) if line_end == npos, this will always be false. https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:570: name = base::StringToLowerASCII(name); Such wasteful copying :'( name == could be LowerCaseEqualsASCII(), these could be base::StringPieces, cats and dogs could live together in harmony... https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:580: base::StringToInt64(value, &seconds); Unchecked return value? Killing me smalls, KILLING ME :) https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:584: base::StringToInt(value, &port); This too! https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:592: line_start = line_end + 1; if line_end == npos, this will wrap around to 0 (since npos is typically -1, it's an unsigned int, so will overflow) However, because of 590, we'll never hit that (since npos will always be > header_end), but it does mean we'll have parsed a line that lacked a terminator, and violated your assumed DCHECKs. I'd feel rather awesome if this used base/strings/string_tokenizer in the future, but I'll leave that to a future CL. But yeah, complex parsing in //net, scurry :) https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:601: return; From your reply, it _seems_ that this is the ONLY necessary change, is that correct? All other bits are handled by DomainIs()?
Still ignoring code changes until I'm comfortable we're in sync on the change philosophy. (Sorry for the tears; I'll try to do what I can when I attack the code.) https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:601: return; On 2014/09/22 20:55:27, Ryan Sleevi wrote: > From your reply, it _seems_ that this is the ONLY necessary change, is that > correct? All other bits are handled by DomainIs()? I don't believe so; I think the stripping of the dot from the dictionary_url{_const} below is the most important point. Again, the failing example in the bug (now immortalized in one of the tests) was the dictionary_url having a trailing ".", and the domain for the SDCH dictionary *not* having that ".".
Sorry to continue to go back and forth with you. I have no doubt that this fixes the issue, but I do still have a lot of doubts that this is the 'right' way to fix the issue, based on our discussions. I apologize that I'm not that familiar with the SDCH code, and many of my questions are dumb and involve a latency hit. When reading the comments below, read "I tried to work through this" first, then come back to the BUG remark. https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:166: size_t postfix_domain_index = referrer_url_host.rfind(domain); BUG: Check your npos results! Bad things happen here if .rfind(domain) == npos (don't assume line 160 is gonna save your bacon) https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:168: if (referrer_url_host.size() == postfix_domain_index + domain.size()) { I find this logic really, really weird. Why not just use EndsWith? std::string dictionary_url_host = dictionary_url.host(); if (EndsWith(dictionary_url_host, domain, false) && domain.size() != referrer_url_host.size() && dictionary_url_host.find(".", 0, dictionary_url_host.size() - domain.size() - 1) != std::string::npos) { } The first ensures that www.google.com ends with google.com. This is really weird to check, since you just did this check on line 160 The next would be to check to make sure they both aren't google.com (since that prefix will never have a leading .) Then you check to see if the space [www.] has a dot (which it does), indicating it's a violation, based on what you said on line 139-141 https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:601: return; On 2014/09/22 21:09:56, rdsmith wrote: > On 2014/09/22 20:55:27, Ryan Sleevi wrote: > > From your reply, it _seems_ that this is the ONLY necessary change, is that > > correct? All other bits are handled by DomainIs()? > > I don't believe so; I think the stripping of the dot from the > dictionary_url{_const} below is the most important point. Again, the failing > example in the bug (now immortalized in one of the tests) was the dictionary_url > having a trailing ".", and the domain for the SDCH dictionary *not* having that > ".". I tried to work through why this is. That is, I'm trying to tease from you what the _necessary_ changes were, versus which you were trying to do for code health/consistency. Altogether, this feels like a much larger change at present, and I have trouble reasoning about the correctness of it. That is, broadly speaking, I'm trying to understand where you're making domain comparisons and why these are problematic. Rather than normalizing all the entry points, we should be trying to reduce the comparisons altogether, and, whenever possible, defer to GURL (which will do the right thing) IsInSupportedDomain - ok, sure, the blacklist considers google.com != google.com. . I can't tell if that's a feature (subdomain.foo.com != foo.com) or a bug. CanSet - - GetDomainAndRegistry (handles trailing .; I think) - DomainMatch(dictionary_url, domain) - does the right thing (re: GURL) So the only place that it matters is when you start matching referrer_url_host and postfix_domain_index. I've commented more in that function.
On 2014/09/22 22:49:43, Ryan Sleevi (expect_delays) wrote: > Sorry to continue to go back and forth with you. I have no doubt that this fixes > the issue, but I do still have a lot of doubts that this is the 'right' way to > fix the issue, based on our discussions. No problem--the reason I grabbed you as a reviewer was that I expected you to want to get things right. Having said that, I was also pinging you for your spec pedantry, and you've dived (doven?) way down into the code. That's fine, but I'd really rather follow you *after* I'm sure we're in sync on the spec, and the philosophy of the change, which probably requires being in sync on the problem. And, as you may think from the previous sentence, I feel like we're talking past each other at the moment. So let me touch on each of the high level issues and get your feedback on them: * Spec: I think we're in sync on this as per c#6. * Problem: I'm not sure whether we're in sync on this; I think my note above in https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... covers it, but I'm not certain that I've communicated that. To be as precise as possible: I have added an inline comment indicating the only change that I think is required to address the bug that prompted this CL. The changes in this CL other than that line are: a) forbidding trailing dots in SDCH domains as per our conversation, b) adding tests, and c) trying to treat trailing dots uniformly through the code. * Philosophy: I'm not sure whether you'd prefer a CL that just did a targeted fix, or one that tried to bring the code entire into line with the (currently imagined) spec. I'm willing to do either. I like to keep code consistent, so I dove in and did everything I thought was needed for the perspective shift, but if you'd prefer a surgical cut until I actually sit down and rewrite the spec, I'm willing. (Please also indicate as to whether you'd like me to try and fix all the problems you've identified (thank you!) or just keep the CL focussed and postpone those fixes for another CL.) If you think we're in sync on the above, I'll dive into the code and throw up a new PS that deals with as many of the issues you've raised as is appropriate given your guidance. Thanks, and my apologies for this being such a slog as well--I'm sure I could have managed this better, not least by raising the spec/philosophy in email. Sometimes code is a useful context for abstract discussions, but I don't think it's been so this time. > > I apologize that I'm not that familiar with the SDCH code, and many of my > questions are dumb and involve a latency hit. > > When reading the comments below, read "I tried to work through this" first, then > come back to the BUG remark. > https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:606: StripTrailingDotURL(&dictionary_url); >> I believe this is the only change required to solve the bug that prompted this CL <<
Anyone following along at home :-}: Ryan and I talked offline, and agreed to do a very targeted fix for this issue, and then worry about the more comprehensive spec-inclusive fix later. Ryan: I think I've whittled this down to as simple as you get; PTAL? https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:26: if (host->size() == 0) On 2014/09/22 20:30:37, Ryan Sleevi (expect_delays) wrote: > host->empty() > > (a nit I hold on to from the days of 'dumb' stl implementations, but also the > generic form of not requiring counting like other containers MAY) Done. https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:29: if ((*host)[host->size() - 1] != '.') On 2014/09/22 20:30:36, Ryan Sleevi (expect_delays) wrote: > if (*host->rbegin() != '.') > return false; > > Laziness is a virtue :) Agreed :-}. Done. https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:36: void StripTrailingDotURL(GURL* gurl) { On 2014/09/22 20:30:36, Ryan Sleevi (expect_delays) wrote: > naming wise, this felt a little weird, although I understand how you got here. > > Perhaps just something as "NormalizeToDictionaryURL"? I don't want "NormalizeToDictionary" because to me that pre-supposes the results of our discussion about the spec framework this code is to be written in. Having said that, I agree with your next comment, which frees up "StripTrailingDot", so I'll use that. https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:40: return; On 2014/09/22 20:30:36, Ryan Sleevi (expect_delays) wrote: > Just inline this? You never use "StripTrailingDot" in this file Done. https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:166: size_t postfix_domain_index = referrer_url_host.rfind(domain); On 2014/09/22 22:49:42, Ryan Sleevi (expect_delays) wrote: > BUG: Check your npos results! Bad things happen here if .rfind(domain) == npos > (don't assume line 160 is gonna save your bacon) Ignoring all comments on untouched code for this CL (applies to several following comments as well). https://codereview.chromium.org/574283006/diff/20001/net/base/sdch_manager.cc... net/base/sdch_manager.cc:524: const GURL& dictionary_url_const) { On 2014/09/22 20:55:26, Ryan Sleevi (expect_delays) wrote: > pedantry naming: dictionary_url here, and normalized_dictionary_url below > (which, yes, requires more code line changes) > > I just find the _const an unnerving "Reverse Hungarian Notation" ;) Done.
lgtm
On 2014/09/23 19:57:54, Ryan Sleevi (expect_delays) wrote: > lgtm I'm having this feeling of having raced at a shut and barricaded door to break it down and have it opened just before I got there :-} :-}. Thanks!
The CQ bit was checked by rdsmith@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/574283006/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 74bfdcf49ffc08522a8838937357dc10d95e1a3d
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/14fd659a2284bc9f301828ada49b8d3fe1f80550 Cr-Commit-Position: refs/heads/master@{#296246} |
