Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13251012:
Fix b180514: Some exception entries cannot be deleted from a setting panel. (Closed)
|
Created:
7 years, 8 months ago by yhirano Modified:
7 years, 8 months ago CC:
chromium-reviews, tyoshino (SeeGerritForStatus) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix b180514: Some exception entries cannot be deleted from a setting panel.
This bug is caused by single trailing dot trimming from the input.
Fixed it by forbidding a pattern if canonicalization is not idempotent, i.e. the pattern canonicalized twice differs from the pattern canonicalized once.
BUG=180514
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192984
Patch Set 1 #
Total comments: 2
Patch Set 2 : Forbid multiple ending dots. #
Total comments: 4
Patch Set 3 : Forbid consecutive dots in the host part. #Patch Set 4 : Forbid the empty host. #Patch Set 5 : Refactoring #
Total comments: 3
Patch Set 6 : Do not check the host part of a pattern strictly #
Total comments: 4
Patch Set 7 : Fix code style #
Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/13251012/diff/1/chrome/common/content_setting... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/13251012/diff/1/chrome/common/content_setting... chrome/common/content_settings_pattern.cc:188: canonicalized_host = TrimEndingDots(canonicalized_host); this change seems wrong. I'll comment on the bug report
I rewrote the patch and it forbids multiple ending dots. https://codereview.chromium.org/13251012/diff/1/chrome/common/content_setting... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/13251012/diff/1/chrome/common/content_setting... chrome/common/content_settings_pattern.cc:188: canonicalized_host = TrimEndingDots(canonicalized_host); On 2013/04/02 08:09:36, jochen wrote: > this change seems wrong. I'll comment on the bug report Done.
https://codereview.chromium.org/13251012/diff/4002/chrome/common/content_sett... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/13251012/diff/4002/chrome/common/content_sett... chrome/common/content_settings_pattern.cc:178: host[host.size() - 1] == '.' && What about a..a? Please move this logic to the chrome/common/content_settings_pattern_parser.cc which is responsible for parsing strings to content settings patters. (see CL 13470012 as an example). https://codereview.chromium.org/13251012/diff/4002/chrome/common/content_sett... File chrome/common/content_settings_pattern_unittest.cc (right): https://codereview.chromium.org/13251012/diff/4002/chrome/common/content_sett... chrome/common/content_settings_pattern_unittest.cc:210: EXPECT_TRUE(Pattern(".").IsValid()); An empty domain (host) does not make much sense for defining site specific content settings exceptions since it does not match any particular domain. Valid content settings patterns require the domain (host) to be non empty. We should not allow this pattern.
https://codereview.chromium.org/13251012/diff/4002/chrome/common/content_sett... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/13251012/diff/4002/chrome/common/content_sett... chrome/common/content_settings_pattern.cc:178: host[host.size() - 1] == '.' && On 2013/04/02 19:16:46, markusheintz_ wrote: > What about a..a? > > Please move this logic to the chrome/common/content_settings_pattern_parser.cc > which is responsible for parsing strings to content settings patters. > > (see CL 13470012 as an example). Done. https://codereview.chromium.org/13251012/diff/4002/chrome/common/content_sett... File chrome/common/content_settings_pattern_unittest.cc (right): https://codereview.chromium.org/13251012/diff/4002/chrome/common/content_sett... chrome/common/content_settings_pattern_unittest.cc:210: EXPECT_TRUE(Pattern(".").IsValid()); On 2013/04/02 19:16:46, markusheintz_ wrote: > An empty domain (host) does not make much sense for defining site specific > content settings exceptions since it does not match any particular domain. > > Valid content settings patterns require the domain (host) to be non empty. > > We should not allow this pattern. Done.
LGTM from my side. Please also wait for Jochen's LGTM
https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || host.find("..") != std::string::npos) { 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.
https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || host.find("..") != std::string::npos) { 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: > 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.
On 2013/04/03 09:48:40, markusheintz_ wrote: > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > File chrome/common/content_settings_pattern_parser.cc (right): > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || > host.find("..") != std::string::npos) { > 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 Actually I made a mistake: GURL("http://a..a") and GURL("http://a..") are valid GURLs. So Jochen's suggestions is good. > 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: > > 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.
https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || host.find("..") != std::string::npos) { On 2013/04/03 09:12:36, jochen wrote: > 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. 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.
On 2013/04/03 09:59:10, yhirano wrote: > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > File chrome/common/content_settings_pattern_parser.cc (right): > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || > host.find("..") != std::string::npos) { > On 2013/04/03 09:12:36, jochen wrote: > > 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. > > 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. I think we can forbid consecutive dots in a domain name, according to the section 3.5 of RFC 1034 (http://tools.ietf.org/html/rfc1034).
On 2013/04/03 09:59:10, yhirano wrote: > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > File chrome/common/content_settings_pattern_parser.cc (right): > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || > host.find("..") != std::string::npos) { > On 2013/04/03 09:12:36, jochen wrote: > > 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. > > 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? Why do we want that? A single dot is a valid dns name. And GURL accepts consecutive dots as valid (according to Markus' last comment), so when we build a pattern from a GURL why might end up with something like this anyway. > > 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.
On 2013/04/03 10:51:56, jochen wrote: > On 2013/04/03 09:59:10, yhirano wrote: > > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > > File chrome/common/content_settings_pattern_parser.cc (right): > > > > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > > chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || > > host.find("..") != std::string::npos) { > > On 2013/04/03 09:12:36, jochen wrote: > > > 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. > > > > 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? > > Why do we want that? > > A single dot is a valid dns name. And GURL accepts consecutive dots as valid > (according to Markus' last comment), so when we build a pattern from a GURL why > might end up with something like this anyway. > > > > > 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. Maybe I understand. Your proposal is, "If the host name of a pattern is unchanged against the second trim, the pattern is valid.", isn't it? Here are test cases. (input) (first trim) (second trim) (result) ".." "." "." Valid "a.." "a." "a" Invalid "..a" "..a" "..a" Valid "." "." "." Valid
On 2013/04/03 11:12:29, yhirano wrote: > On 2013/04/03 10:51:56, jochen wrote: > > On 2013/04/03 09:59:10, yhirano wrote: > > > > > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > > > File chrome/common/content_settings_pattern_parser.cc (right): > > > > > > > > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > > > chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || > > > host.find("..") != std::string::npos) { > > > On 2013/04/03 09:12:36, jochen wrote: > > > > 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. > > > > > > 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? > > > > Why do we want that? > > > > A single dot is a valid dns name. And GURL accepts consecutive dots as valid > > (according to Markus' last comment), so when we build a pattern from a GURL > why > > might end up with something like this anyway. > > > > > > > > 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. > Maybe I understand. > Your proposal is, "If the host name of a pattern is unchanged against the second > trim, the pattern is valid.", isn't it? > > Here are test cases. > (input) (first trim) (second trim) (result) > ".." "." "." Valid > "a.." "a." "a" Invalid > "..a" "..a" "..a" Valid > "." "." "." Valid which of these cases do you think is wrong?
1-3 is wrong if they are meant to be domains On 2013/04/03 11:13:40, jochen wrote: > On 2013/04/03 11:12:29, yhirano wrote: > > On 2013/04/03 10:51:56, jochen wrote: > > > On 2013/04/03 09:59:10, yhirano wrote: > > > > > > > > > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > > > > File chrome/common/content_settings_pattern_parser.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > > > > chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || > > > > host.find("..") != std::string::npos) { > > > > On 2013/04/03 09:12:36, jochen wrote: > > > > > 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. > > > > > > > > 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? > > > > > > Why do we want that? > > > > > > A single dot is a valid dns name. And GURL accepts consecutive dots as valid > > > (according to Markus' last comment), so when we build a pattern from a GURL > > why > > > might end up with something like this anyway. > > > > > > > > > > > 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. > > Maybe I understand. > > Your proposal is, "If the host name of a pattern is unchanged against the > second > > trim, the pattern is valid.", isn't it? > > > > Here are test cases. > > (input) (first trim) (second trim) (result) > > ".." "." "." Valid > > "a.." "a." "a" Invalid > > "..a" "..a" "..a" Valid > > "." "." "." Valid > > which of these cases do you think is wrong?
Just because GURL supports this does not mean it is valid. :( Also we are talking about three different cases: A) http://www.example.com. (Valid) B) http://www.example.com.. (invalid) C) http://www..exmaple.com or http://www.exam..le.com (invalid) On 2013/04/03 11:14:43, markusheintz_ wrote: > 1-3 is wrong if they are meant to be domains > > On 2013/04/03 11:13:40, jochen wrote: > > On 2013/04/03 11:12:29, yhirano wrote: > > > On 2013/04/03 10:51:56, jochen wrote: > > > > On 2013/04/03 09:59:10, yhirano wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > > > > > File chrome/common/content_settings_pattern_parser.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_set... > > > > > chrome/common/content_settings_pattern_parser.cc:142: if (host == "." || > > > > > host.find("..") != std::string::npos) { > > > > > On 2013/04/03 09:12:36, jochen wrote: > > > > > > 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. > > > > > > > > > > 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? > > > > > > > > Why do we want that? > > > > > > > > A single dot is a valid dns name. And GURL accepts consecutive dots as > valid > > > > (according to Markus' last comment), so when we build a pattern from a > GURL > > > why > > > > might end up with something like this anyway. > > > > > > > > > > > > > > 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. > > > Maybe I understand. > > > Your proposal is, "If the host name of a pattern is unchanged against the > > second > > > trim, the pattern is valid.", isn't it? > > > > > > Here are test cases. > > > (input) (first trim) (second trim) (result) > > > ".." "." "." Valid > > > "a.." "a." "a" Invalid > > > "..a" "..a" "..a" Valid > > > "." "." "." Valid > > > > which of these cases do you think is wrong?
Right, but the actual issue is that repeatedly converting a pattern to a string and back breaks the assumptions. My point is: instead of trying to reimplement a domain name verification but enumerating all invalid cases we can think of, why not just checking this one assumption we rely on. (Also, "." is a valid domain name which the proposed CL would filter out) On Wed, Apr 3, 2013 at 1:15 PM, <markusheintz@chromium.org> wrote: > Just because GURL supports this does not mean it is valid. :( > > Also we are talking about three different cases: > A) http://www.example.com. (Valid) > B) http://www.example.com.. (invalid) > C) http://www..exmaple.com or http://www.exam..le.com (invalid) > > > > > On 2013/04/03 11:14:43, markusheintz_ wrote: > >> 1-3 is wrong if they are meant to be domains >> > > On 2013/04/03 11:13:40, jochen wrote: >> > On 2013/04/03 11:12:29, yhirano wrote: >> > > On 2013/04/03 10:51:56, jochen wrote: >> > > > On 2013/04/03 09:59:10, yhirano wrote: >> > > > > >> > > > >> > > >> > >> > > https://codereview.chromium.**org/13251012/diff/11001/** > chrome/common/content_**settings_pattern_parser.cc<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc> > >> > > > > File chrome/common/content_**settings_pattern_parser.cc (right): >> > > > > >> > > > > >> > > > >> > > >> > >> > > https://codereview.chromium.**org/13251012/diff/11001/** > chrome/common/content_**settings_pattern_parser.cc#**newcode142<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc#newcode142> > >> > > > > chrome/common/content_**settings_pattern_parser.cc:**142: if >> (host == "." >> > || > >> > > > > host.find("..") != std::string::npos) { >> > > > > On 2013/04/03 09:12:36, jochen wrote: >> > > > > > 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. >> > > > > >> > > > > 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? >> > > > >> > > > Why do we want that? >> > > > >> > > > A single dot is a valid dns name. And GURL accepts consecutive dots >> as >> valid >> > > > (according to Markus' last comment), so when we build a pattern >> from a >> GURL >> > > why >> > > > might end up with something like this anyway. >> > > > >> > > > > >> > > > > 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. >> > > Maybe I understand. >> > > Your proposal is, "If the host name of a pattern is unchanged against >> the >> > second >> > > trim, the pattern is valid.", isn't it? >> > > >> > > Here are test cases. >> > > (input) (first trim) (second trim) (result) >> > > ".." "." "." Valid >> > > "a.." "a." "a" Invalid >> > > "..a" "..a" "..a" Valid >> > > "." "." "." Valid >> > >> > which of these cases do you think is wrong? >> > > > https://codereview.chromium.**org/13251012/<https://codereview.chromium.org/1... >
It is OK for me if a solution 1) fixes the issue, and 2) is consistent. jochen@'s and markusheintz_@'s solutions both meet the points and so I am no string opinion which is better. On 2013/04/03 11:19:00, jochen wrote: > Right, but the actual issue is that repeatedly converting a pattern to a > string and back breaks the assumptions. > > My point is: instead of trying to reimplement a domain name verification > but enumerating all invalid cases we can think of, why not just checking > this one assumption we rely on. > > (Also, "." is a valid domain name which the proposed CL would filter out) > > > On Wed, Apr 3, 2013 at 1:15 PM, <mailto:markusheintz@chromium.org> wrote: > > > Just because GURL supports this does not mean it is valid. :( > > > > Also we are talking about three different cases: > > A) http://www.example.com. (Valid) > > B) http://www.example.com.. (invalid) > > C) http://www..exmaple.com or http://www.exam..le.com (invalid) > > > > > > > > > > On 2013/04/03 11:14:43, markusheintz_ wrote: > > > >> 1-3 is wrong if they are meant to be domains > >> > > > > On 2013/04/03 11:13:40, jochen wrote: > >> > On 2013/04/03 11:12:29, yhirano wrote: > >> > > On 2013/04/03 10:51:56, jochen wrote: > >> > > > On 2013/04/03 09:59:10, yhirano wrote: > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://codereview.chromium.**org/13251012/diff/11001/** > > > chrome/common/content_**settings_pattern_parser.cc<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc> > > > >> > > > > File chrome/common/content_**settings_pattern_parser.cc (right): > >> > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > https://codereview.chromium.**org/13251012/diff/11001/** > > > chrome/common/content_**settings_pattern_parser.cc#**newcode142<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc#newcode142> > > > >> > > > > chrome/common/content_**settings_pattern_parser.cc:**142: if > >> (host == "." > >> > > || > > > >> > > > > host.find("..") != std::string::npos) { > >> > > > > On 2013/04/03 09:12:36, jochen wrote: > >> > > > > > 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. > >> > > > > > >> > > > > 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? > >> > > > > >> > > > Why do we want that? > >> > > > > >> > > > A single dot is a valid dns name. And GURL accepts consecutive dots > >> as > >> valid > >> > > > (according to Markus' last comment), so when we build a pattern > >> from a > >> GURL > >> > > why > >> > > > might end up with something like this anyway. > >> > > > > >> > > > > > >> > > > > 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. > >> > > Maybe I understand. > >> > > Your proposal is, "If the host name of a pattern is unchanged against > >> the > >> > second > >> > > trim, the pattern is valid.", isn't it? > >> > > > >> > > Here are test cases. > >> > > (input) (first trim) (second trim) (result) > >> > > ".." "." "." Valid > >> > > "a.." "a." "a" Invalid > >> > > "..a" "..a" "..a" Valid > >> > > "." "." "." Valid > >> > > >> > which of these cases do you think is wrong? > >> > > > > > > > https://codereview.chromium.**org/13251012/%3Chttps://codereview.chromium.org...> > >
> I am no strong opinion I have no strong opinion On 2013/04/03 11:33:24, yhirano wrote: > It is OK for me if a solution > 1) fixes the issue, and > 2) is consistent. > > jochen@'s and markusheintz_@'s solutions both meet the points and so I am no > string opinion which is better. > > On 2013/04/03 11:19:00, jochen wrote: > > Right, but the actual issue is that repeatedly converting a pattern to a > > string and back breaks the assumptions. > > > > My point is: instead of trying to reimplement a domain name verification > > but enumerating all invalid cases we can think of, why not just checking > > this one assumption we rely on. > > > > (Also, "." is a valid domain name which the proposed CL would filter out) > > > > > > On Wed, Apr 3, 2013 at 1:15 PM, <mailto:markusheintz@chromium.org> wrote: > > > > > Just because GURL supports this does not mean it is valid. :( > > > > > > Also we are talking about three different cases: > > > A) http://www.example.com. (Valid) > > > B) http://www.example.com.. (invalid) > > > C) http://www..exmaple.com or http://www.exam..le.com (invalid) > > > > > > > > > > > > > > > On 2013/04/03 11:14:43, markusheintz_ wrote: > > > > > >> 1-3 is wrong if they are meant to be domains > > >> > > > > > > On 2013/04/03 11:13:40, jochen wrote: > > >> > On 2013/04/03 11:12:29, yhirano wrote: > > >> > > On 2013/04/03 10:51:56, jochen wrote: > > >> > > > On 2013/04/03 09:59:10, yhirano wrote: > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > https://codereview.chromium.**org/13251012/diff/11001/** > > > > > > chrome/common/content_**settings_pattern_parser.cc<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc> > > > > > >> > > > > File chrome/common/content_**settings_pattern_parser.cc (right): > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > https://codereview.chromium.**org/13251012/diff/11001/** > > > > > > chrome/common/content_**settings_pattern_parser.cc#**newcode142<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc#newcode142> > > > > > >> > > > > chrome/common/content_**settings_pattern_parser.cc:**142: if > > >> (host == "." > > >> > > > || > > > > > >> > > > > host.find("..") != std::string::npos) { > > >> > > > > On 2013/04/03 09:12:36, jochen wrote: > > >> > > > > > 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. > > >> > > > > > > >> > > > > 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? > > >> > > > > > >> > > > Why do we want that? > > >> > > > > > >> > > > A single dot is a valid dns name. And GURL accepts consecutive dots > > >> as > > >> valid > > >> > > > (according to Markus' last comment), so when we build a pattern > > >> from a > > >> GURL > > >> > > why > > >> > > > might end up with something like this anyway. > > >> > > > > > >> > > > > > > >> > > > > 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. > > >> > > Maybe I understand. > > >> > > Your proposal is, "If the host name of a pattern is unchanged against > > >> the > > >> > second > > >> > > trim, the pattern is valid.", isn't it? > > >> > > > > >> > > Here are test cases. > > >> > > (input) (first trim) (second trim) (result) > > >> > > ".." "." "." Valid > > >> > > "a.." "a." "a" Invalid > > >> > > "..a" "..a" "..a" Valid > > >> > > "." "." "." Valid > > >> > > > >> > which of these cases do you think is wrong? > > >> > > > > > > > > > > > > https://codereview.chromium.**org/13251012/%253Chttps://codereview.chromium.o...> > > >
Ok after discussing this with Jochen offline, let's go with Jochen's proposal and check if canonicalizing the pattern again changes it. It the pattern changes after canonicalizing it the second time then it should be invalid. Please note that there are two validation methods: |Validate| and |LegacyValidate|. Therefor the |Build| method should be a good place for this check. Please add a comment that with a description why this check is there. On Wed, Apr 3, 2013 at 1:37 PM, <yhirano@chromium.org> wrote: > I am no strong opinion >> > I have no strong opinion > > > On 2013/04/03 11:33:24, yhirano wrote: > >> It is OK for me if a solution >> 1) fixes the issue, and >> 2) is consistent. >> > > jochen@'s and markusheintz_@'s solutions both meet the points and so I >> am no >> string opinion which is better. >> > > On 2013/04/03 11:19:00, jochen wrote: >> > Right, but the actual issue is that repeatedly converting a pattern to a >> > string and back breaks the assumptions. >> > >> > My point is: instead of trying to reimplement a domain name verification >> > but enumerating all invalid cases we can think of, why not just checking >> > this one assumption we rely on. >> > >> > (Also, "." is a valid domain name which the proposed CL would filter >> out) >> > >> > >> > On Wed, Apr 3, 2013 at 1:15 PM, <mailto:markusheintz@chromium.**org<markusheintz@chromium.org>> >> wrote: >> > >> > > Just because GURL supports this does not mean it is valid. :( >> > > >> > > Also we are talking about three different cases: >> > > A) http://www.example.com. (Valid) >> > > B) http://www.example.com.. (invalid) >> > > C) http://www..exmaple.com or http://www.exam..le.com (invalid) >> > > >> > > >> > > >> > > >> > > On 2013/04/03 11:14:43, markusheintz_ wrote: >> > > >> > >> 1-3 is wrong if they are meant to be domains >> > >> >> > > >> > > On 2013/04/03 11:13:40, jochen wrote: >> > >> > On 2013/04/03 11:12:29, yhirano wrote: >> > >> > > On 2013/04/03 10:51:56, jochen wrote: >> > >> > > > On 2013/04/03 09:59:10, yhirano wrote: >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > >> >> > > >> > > https://codereview.chromium.****org/13251012/diff/11001/** >> > > >> > >> > > chrome/common/content_****settings_pattern_parser.cc<htt** > ps://codereview.chromium.org/**13251012/diff/11001/chrome/** > common/content_settings_**pattern_parser.cc<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc> > > > >> > > >> > >> > > > > File chrome/common/content_****settings_pattern_parser.cc >> (right): >> > >> > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > >> >> > > >> > > https://codereview.chromium.****org/13251012/diff/11001/** >> > > >> > >> > > chrome/common/content_****settings_pattern_parser.cc#****newcode142< > https://codereview.**chromium.org/13251012/diff/** > 11001/chrome/common/content_**settings_pattern_parser.cc#**newcode142<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc#newcode142> > > > >> > > >> > >> > > > > chrome/common/content_****settings_pattern_parser.cc:****142: >> if >> > >> (host == "." >> > >> >> > > || >> > > >> > >> > > > > host.find("..") != std::string::npos) { >> > >> > > > > On 2013/04/03 09:12:36, jochen wrote: >> > >> > > > > > 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. >> > >> > > > > >> > >> > > > > 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? >> > >> > > > >> > >> > > > Why do we want that? >> > >> > > > >> > >> > > > A single dot is a valid dns name. And GURL accepts consecutive >> dots >> > >> as >> > >> valid >> > >> > > > (according to Markus' last comment), so when we build a pattern >> > >> from a >> > >> GURL >> > >> > > why >> > >> > > > might end up with something like this anyway. >> > >> > > > >> > >> > > > > >> > >> > > > > 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. >> > >> > > Maybe I understand. >> > >> > > Your proposal is, "If the host name of a pattern is unchanged >> against >> > >> the >> > >> > second >> > >> > > trim, the pattern is valid.", isn't it? >> > >> > > >> > >> > > Here are test cases. >> > >> > > (input) (first trim) (second trim) (result) >> > >> > > ".." "." "." Valid >> > >> > > "a.." "a." "a" Invalid >> > >> > > "..a" "..a" "..a" Valid >> > >> > > "." "." "." Valid >> > >> > >> > >> > which of these cases do you think is wrong? >> > >> >> > > >> > > >> > > >> > >> > > https://codereview.chromium.****org/13251012/%253Chttps://code** > review.chromium.org/13251012/ <http://codereview.chromium.org/13251012/>> > >> > > >> > > > > https://codereview.chromium.**org/13251012/<https://codereview.chromium.org/1... >
Thank you very much! I will do that. On 2013/04/03 12:13:18, markusheintz_ wrote: > Ok after discussing this with Jochen offline, let's go with Jochen's > proposal and check if canonicalizing the pattern again changes it. It the > pattern changes after canonicalizing it the second time then it should be > invalid. > > Please note that there are two validation methods: |Validate| and > |LegacyValidate|. Therefor the |Build| method should be a good place for > this check. > > Please add a comment that with a description why this check is there. > > > On Wed, Apr 3, 2013 at 1:37 PM, <mailto:yhirano@chromium.org> wrote: > > > I am no strong opinion > >> > > I have no strong opinion > > > > > > On 2013/04/03 11:33:24, yhirano wrote: > > > >> It is OK for me if a solution > >> 1) fixes the issue, and > >> 2) is consistent. > >> > > > > jochen@'s and markusheintz_@'s solutions both meet the points and so I > >> am no > >> string opinion which is better. > >> > > > > On 2013/04/03 11:19:00, jochen wrote: > >> > Right, but the actual issue is that repeatedly converting a pattern to a > >> > string and back breaks the assumptions. > >> > > >> > My point is: instead of trying to reimplement a domain name verification > >> > but enumerating all invalid cases we can think of, why not just checking > >> > this one assumption we rely on. > >> > > >> > (Also, "." is a valid domain name which the proposed CL would filter > >> out) > >> > > >> > > >> > On Wed, Apr 3, 2013 at 1:15 PM, > <mailto:markusheintz@chromium.**org<markusheintz@chromium.org>> > >> wrote: > >> > > >> > > Just because GURL supports this does not mean it is valid. :( > >> > > > >> > > Also we are talking about three different cases: > >> > > A) http://www.example.com. (Valid) > >> > > B) http://www.example.com.. (invalid) > >> > > C) http://www..exmaple.com or http://www.exam..le.com (invalid) > >> > > > >> > > > >> > > > >> > > > >> > > On 2013/04/03 11:14:43, markusheintz_ wrote: > >> > > > >> > >> 1-3 is wrong if they are meant to be domains > >> > >> > >> > > > >> > > On 2013/04/03 11:13:40, jochen wrote: > >> > >> > On 2013/04/03 11:12:29, yhirano wrote: > >> > >> > > On 2013/04/03 10:51:56, jochen wrote: > >> > >> > > > On 2013/04/03 09:59:10, yhirano wrote: > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > > > >> > > https://codereview.chromium.****org/13251012/diff/11001/** > >> > > > >> > > >> > > > > chrome/common/content_****settings_pattern_parser.cc<htt** > > ps://codereview.chromium.org/**13251012/diff/11001/chrome/** > > > common/content_settings_**pattern_parser.cc<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc> > > > > > > >> > > > >> > >> > > > > File chrome/common/content_****settings_pattern_parser.cc > >> (right): > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > > > >> > > https://codereview.chromium.****org/13251012/diff/11001/** > >> > > > >> > > >> > > > > chrome/common/content_****settings_pattern_parser.cc#****newcode142< > > https://codereview.**chromium.org/13251012/diff/** > > > 11001/chrome/common/content_**settings_pattern_parser.cc#**newcode142<https://codereview.chromium.org/13251012/diff/11001/chrome/common/content_settings_pattern_parser.cc#newcode142> > > > > > > >> > > > >> > >> > > > > chrome/common/content_****settings_pattern_parser.cc:****142: > >> if > >> > >> (host == "." > >> > >> > >> > > || > >> > > > >> > >> > > > > host.find("..") != std::string::npos) { > >> > >> > > > > On 2013/04/03 09:12:36, jochen wrote: > >> > >> > > > > > 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. > >> > >> > > > > > >> > >> > > > > 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? > >> > >> > > > > >> > >> > > > Why do we want that? > >> > >> > > > > >> > >> > > > A single dot is a valid dns name. And GURL accepts consecutive > >> dots > >> > >> as > >> > >> valid > >> > >> > > > (according to Markus' last comment), so when we build a pattern > >> > >> from a > >> > >> GURL > >> > >> > > why > >> > >> > > > might end up with something like this anyway. > >> > >> > > > > >> > >> > > > > > >> > >> > > > > 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. > >> > >> > > Maybe I understand. > >> > >> > > Your proposal is, "If the host name of a pattern is unchanged > >> against > >> > >> the > >> > >> > second > >> > >> > > trim, the pattern is valid.", isn't it? > >> > >> > > > >> > >> > > Here are test cases. > >> > >> > > (input) (first trim) (second trim) (result) > >> > >> > > ".." "." "." Valid > >> > >> > > "a.." "a." "a" Invalid > >> > >> > > "..a" "..a" "..a" Valid > >> > >> > > "." "." "." Valid > >> > >> > > >> > >> > which of these cases do you think is wrong? > >> > >> > >> > > > >> > > > >> > > > >> > > >> > > > > https://codereview.chromium.****org/13251012/%25253Chttps://code** > > review.chromium.org/13251012/ <http://codereview.chromium.org/13251012/>> > > > >> > > > >> > > > > > > > > > https://codereview.chromium.**org/13251012/%3Chttps://codereview.chromium.org...> > >
jochen@, markusheints_@, can you review the patch again?
lgtm https://codereview.chromium.org/13251012/diff/11002/chrome/common/content_set... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/13251012/diff/11002/chrome/common/content_set... chrome/common/content_settings_pattern.cc:160: return ContentSettingsPattern(parts_, is_valid_); nit. just return ContentSettingsPattern() https://codereview.chromium.org/13251012/diff/11002/chrome/common/content_set... chrome/common/content_settings_pattern.cc:170: return ContentSettingsPattern(); nit { } around body since the if condition is multiline
LGTM Thanks a lot for fixing this issue and all your patient during the code review :) .
Thank you very much for reviewing. https://codereview.chromium.org/13251012/diff/11002/chrome/common/content_set... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/13251012/diff/11002/chrome/common/content_set... chrome/common/content_settings_pattern.cc:160: return ContentSettingsPattern(parts_, is_valid_); On 2013/04/05 11:23:47, jochen wrote: > nit. just return ContentSettingsPattern() Done. https://codereview.chromium.org/13251012/diff/11002/chrome/common/content_set... chrome/common/content_settings_pattern.cc:170: return ContentSettingsPattern(); On 2013/04/05 11:23:47, jochen wrote: > nit { } around body since the if condition is multiline Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/13251012/31001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/13251012/31001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/13251012/31001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/13251012/31001
Message was sent while issue was closed.
Change committed as 192984 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
