Fix b180514: Some exception entries cannot be deleted from a setting panel.

chrome/common/content_settings_pattern_parser.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/common/content_settings_pattern_parser.h"
 
 #include "base/string_util.h"
 #include "chrome/common/url_constants.h"
 #include "extensions/common/constants.h"
 #include "googleurl/src/gurl.h"
@@ skipping 121 matching lines @@
     } else {
       builder->WithScheme(scheme);
     }
   } else {
     builder->WithSchemeWildcard();
   }
 
   if (host_component.IsNonEmpty()) {
     std::string host = pattern_spec.substr(host_component.start,
                                            host_component.len);
+    if (host == "." || host.find("..") != std::string::npos) {

[jochen (gone - plz use gerrit), 2013/04/03 09:12:36]

sorry for being late to the party

I would expect a check like this in ContentSettingsPattern::Builder::Validate()

At that point, we could do a more general check (because the pattern is already
canonicalized), and check that canonicalizing the pattern again doesn't change
it.

[markusheintz_, 2013/04/03 09:48:40]

Do you have any particular checks do you have in mind?

The two consecutive dots are an illegal substring. So while parsing the pattern
string seems the right place to do this check. I don't want to split the parsing
in two files. The Parser is the right place to do all the string parsing.

GURL("a..a") and GURL("A..") are illegal as well. Maybe we should add checks for
that to 
content_settings_pattern_unittest.cc. There is a test call GURL in this file.

On 2013/04/03 09:12:36, jochen wrote:

[yhirano, 2013/04/03 09:59:10]

Let me clarify.
As you say, Builder::Validate() is processed after the canonicalization. So, the
pattern will be changed as:
 "www.example.com.." => "www.example.com."
 "www.example.com." => "www.example.com"
 "www..example.com" => "www..example.com"
 "." => "."
 ".." => "."
.

Hence, if we want to validate as same as this patch, we should check
(1) The (modified) host doesn't have consecutive dots, and
(2) The host doesn't end with a dot

Do you think it is better than this patch?

  check that canonicalizing the pattern again doesn't change it.
Note that there are some points which should be considered:
(a) According to markusheintz_@'s suggestion, the current patch does not permit
patterns with a host having consecutive dots, such as "www..example.com". 
(b) net::TrimEndingDot does nothing for a host consisting of a single dot.
Hence, ".." will be canonicalized to ".", which will be canonicalized to itself.

For these two cases, idempotence check does not work, for at least, current
canonicalization.

+      builder->Invalid();
+      return;
+    }
+
     if (host == kHostWildcard) {
       builder->WithDomainWildcard();
     } else if (StartsWithASCII(host, kDomainWildcard, true)) {
       host = host.substr(kDomainWildcardLength);
       builder->WithDomainWildcard();
       builder->WithHost(host);
     } else {
       // If the host contains a wildcard symbol then it is invalid.
       if (host.find(kHostWildcard) != std::string::npos) {
         builder->Invalid();
@@ skipping 71 matching lines @@
   }
 
   if (!parts.is_port_wildcard) {
     str += std::string(kUrlPortSeparator) + parts.port;
   }
 
   return str;
 }
 
 }  // namespace content_settings