Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439953004:
Remove invalid "suffix" assumption from CreateSortKey (Closed)
|
Created:
4 years, 2 months ago by vabr (Chromium) Modified:
4 years, 1 month ago Reviewers:
kolos1 CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove invalid "suffix" assumption from CreateSortKey
Currently, CreateSortKey in password_manager_presenter.cc assumes that domain
and registry name obtained from a hostname string is always a suffix of the
hostname string. This is true for ASCII-based examples like this:
hostname: subdomain.example.com
domain and registry: example.com
But it is not true for non-ASCII-based examples like this:
hostname: žřč.com
domain and registry: xn--bea5m6d.com
The reason is that to obtain the domain and registry, the hostname is being
canonicalized.
The domain and registry is used to create a sort key for a credential with the
given hostname. The idea is to start with the domain and registry part, and
then append the remaining subdomains (reversed). The remaining subdomains were
computed by taking the prefix of hostname of the length of the difference of
the length of the hostname minus the length of the domain and registry part.
This number may overflow in case the domain and registry is not a suffix of the
hostname.
This CL fixes the issue by appending the whole hostname to the domain and
registry part. The results look like this:
hostname: subdomain.example.com
old key: example.com.subdomain
new key: example.com.com.example.subdomain
hostname: žřč.com
old key: undefined
new key: xn--bea5m6d.com.com.žřč
This is safe (no assumptions about suffixes) and preserves the sorting order of
any two credentials before and after this change. It is potentially duplicating
the domain and registry part. An alternative solution could just canonicalize
the hostname before composing the key, but the code would be more complex for
an unclear benefit.
The CL also uses a more efficient call to GetDomainAndRegistry (avoiding
redoing the canonicalization of a known URL). It also introduces a test and
removes an unnecessary call to ASCIIToUTF16.
BUG=655949
Committed: https://crrev.com/f77c3ffef00e68fe59150c78619dbdf289677400
Cr-Commit-Position: refs/heads/master@{#427051}
Patch Set 1 #Patch Set 2 : Fix typo #
Total comments: 10
Patch Set 3 : Review comments #
Total comments: 2
Messages
Total messages: 22 (12 generated)
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vabr@chromium.org changed reviewers: + kolos@chromium.org
Hi Maxim, Please have a look. Cheers, Vaclav
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Vaclav for checking and fixing this bug. LGTM % some comments. Please fix CL's description: hostname: subdomain.example.com old key: example.com.subdomain new key: example.com.com.example.subdomain hostname: žřč.com old key: undefined new key: xn--bea5m6d.com.com.žřč https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:68: // (e.g. secure.accounts.example.com => example.com.accounts.secure). Fix the method description. The key is eTLD+1 followed by the reversed list of domains (e.g. secure.accounts.example.com => example.com.com.example.accounts.secure) https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:80: if (!is_clickable) { // e.g. android://com.example.r => r.example.com. Unnecessary curly brackets https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:269: To be sure we sort subdomains in the right way, please add this test here: TEST_F(PasswordManagerPresenterTest, Sorting_Subdomains) { const SortEntry test_cases[] = { {"http://example.com", "u", "p", nullptr, nullptr, 0}, {"http://b.example.com", "u", "p", nullptr, nullptr, 6}, {"http://a.example.com", "u", "p", nullptr, nullptr, 1}, {"http://1.a.example.com", "u", "p", nullptr, nullptr, 2}, {"http://2.a.example.com", "u", "p", nullptr, nullptr, 3}, {"http://x.2.a.example.com", "u", "p", nullptr, nullptr, 4}, {"http://y.2.a.example.com", "u", "p", nullptr, nullptr, 5}, }; SortAndCheckPositions(test_cases, arraysize(test_cases), PasswordEntryType::SAVED); } https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:310: {"https://xn--bea5m6d.com/", "user_a", "pwd", nullptr, nullptr, 0}, Does this test cause a crash? Shall we use "žřč.com" instead of "https://xn--bea5m6d.com/"? https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:312: SortAndCheckPositions(test_cases, arraysize(test_cases), Please add more items to |test_cases| to test if the sorting works well on domains with special characters.
Description was changed from ========== Remove invalid "suffix" assumption from CreateSortKey Currently, CreateSortKey in password_manager_presenter.cc assumes that domain and registry name obtained from a hostname string is always a suffix of the hostname string. This is true for ASCII-based examples like this: hostname: subdomain.example.com domain and registry: example.com But it is not true for non-ASCII-based examples like this: hostname: žřč.com domain and registry: xn--bea5m6d.com The reason is that to obtain the domain and registry, the hostname is being canonicalized. The domain and registry is used to create a sort key for a credential with the given hostname. The idea is to start with the domain and registry part, and then append the remaining subdomains (reversed). The remaining subdomains were computed by taking the prefix of hostname of the length of the difference of the length of the hostname minus the length of the domain and registry part. This number may overflow in case the domain and registry is not a suffix of the hostname. This CL fixes the issue by appending the whole hostname to the domain and registry part. The results look like this: hostname: subdomain.example.com old key: example.com.subdomain new key: example.com.subdomain.example.com hostname: žřč.com old key: undefined new key: xn--bea5m6d.com.žřč.com This is safe (no assumptions about suffixes) and preserves the sorting order of any two credentials before and after this change. It is potentially duplicating the domain and registry part. An alternative solution could just canonicalize the hostname before composing the key, but the code would be more complex for an unclear benefit. The CL also uses a more efficient call to GetDomainAndRegistry (avoiding redoing the canonicalization of a known URL). It also introduces a test and removes an unnecessary call to ASCIIToUTF16. BUG=655949 ========== to ========== Remove invalid "suffix" assumption from CreateSortKey Currently, CreateSortKey in password_manager_presenter.cc assumes that domain and registry name obtained from a hostname string is always a suffix of the hostname string. This is true for ASCII-based examples like this: hostname: subdomain.example.com domain and registry: example.com But it is not true for non-ASCII-based examples like this: hostname: žřč.com domain and registry: xn--bea5m6d.com The reason is that to obtain the domain and registry, the hostname is being canonicalized. The domain and registry is used to create a sort key for a credential with the given hostname. The idea is to start with the domain and registry part, and then append the remaining subdomains (reversed). The remaining subdomains were computed by taking the prefix of hostname of the length of the difference of the length of the hostname minus the length of the domain and registry part. This number may overflow in case the domain and registry is not a suffix of the hostname. This CL fixes the issue by appending the whole hostname to the domain and registry part. The results look like this: hostname: subdomain.example.com old key: example.com.subdomain new key: example.com.com.example.subdomain hostname: žřč.com old key: undefined new key: xn--bea5m6d.com.com.žřč This is safe (no assumptions about suffixes) and preserves the sorting order of any two credentials before and after this change. It is potentially duplicating the domain and registry part. An alternative solution could just canonicalize the hostname before composing the key, but the code would be more complex for an unclear benefit. The CL also uses a more efficient call to GetDomainAndRegistry (avoiding redoing the canonicalization of a known URL). It also introduces a test and removes an unnecessary call to ASCIIToUTF16. BUG=655949 ==========
Thanks, Maxim! I addressed your comments and will be sending to the CQ. Cheers, Vaclav https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:68: // (e.g. secure.accounts.example.com => example.com.accounts.secure). On 2016/10/24 03:35:35, kolos1 wrote: > Fix the method description. > The key is eTLD+1 followed by the reversed list of domains > (e.g. http://secure.accounts.example.com => example.com.com.example.accounts.secure) Done. https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter.cc:80: if (!is_clickable) { // e.g. android://com.example.r => r.example.com. On 2016/10/24 03:35:35, kolos1 wrote: > Unnecessary curly brackets Done. https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:269: On 2016/10/24 03:35:35, kolos1 wrote: > To be sure we sort subdomains in the right way, please add this test here: > > TEST_F(PasswordManagerPresenterTest, Sorting_Subdomains) { > const SortEntry test_cases[] = { > {"http://example.com", "u", "p", nullptr, nullptr, 0}, > {"http://b.example.com", "u", "p", nullptr, nullptr, 6}, > {"http://a.example.com", "u", "p", nullptr, nullptr, 1}, > {"http://1.a.example.com", "u", "p", nullptr, nullptr, 2}, > {"http://2.a.example.com", "u", "p", nullptr, nullptr, 3}, > {"http://x.2.a.example.com", "u", "p", nullptr, nullptr, 4}, > {"http://y.2.a.example.com", "u", "p", nullptr, nullptr, 5}, > }; > SortAndCheckPositions(test_cases, arraysize(test_cases), > PasswordEntryType::SAVED); > } Done. https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:310: {"https://xn--bea5m6d.com/", "user_a", "pwd", nullptr, nullptr, 0}, On 2016/10/24 03:35:35, kolos1 wrote: > Does this test cause a crash? Shall we use "žřč.com" instead of > "https://xn--bea5m6d.com/"? It does cause crash (without the fix, and on ASAN). xn--bea5m6d gets converted into žřč during GetShownOriginAndLinkUrl. https://codereview.chromium.org/2439953004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:312: SortAndCheckPositions(test_cases, arraysize(test_cases), On 2016/10/24 03:35:35, kolos1 wrote: > Please add more items to |test_cases| to test if the sorting works well on > domains with special characters. Done.
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kolos@chromium.org Link to the patchset: https://codereview.chromium.org/2439953004/#ps40001 (title: "Review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove invalid "suffix" assumption from CreateSortKey Currently, CreateSortKey in password_manager_presenter.cc assumes that domain and registry name obtained from a hostname string is always a suffix of the hostname string. This is true for ASCII-based examples like this: hostname: subdomain.example.com domain and registry: example.com But it is not true for non-ASCII-based examples like this: hostname: žřč.com domain and registry: xn--bea5m6d.com The reason is that to obtain the domain and registry, the hostname is being canonicalized. The domain and registry is used to create a sort key for a credential with the given hostname. The idea is to start with the domain and registry part, and then append the remaining subdomains (reversed). The remaining subdomains were computed by taking the prefix of hostname of the length of the difference of the length of the hostname minus the length of the domain and registry part. This number may overflow in case the domain and registry is not a suffix of the hostname. This CL fixes the issue by appending the whole hostname to the domain and registry part. The results look like this: hostname: subdomain.example.com old key: example.com.subdomain new key: example.com.com.example.subdomain hostname: žřč.com old key: undefined new key: xn--bea5m6d.com.com.žřč This is safe (no assumptions about suffixes) and preserves the sorting order of any two credentials before and after this change. It is potentially duplicating the domain and registry part. An alternative solution could just canonicalize the hostname before composing the key, but the code would be more complex for an unclear benefit. The CL also uses a more efficient call to GetDomainAndRegistry (avoiding redoing the canonicalization of a known URL). It also introduces a test and removes an unnecessary call to ASCIIToUTF16. BUG=655949 ========== to ========== Remove invalid "suffix" assumption from CreateSortKey Currently, CreateSortKey in password_manager_presenter.cc assumes that domain and registry name obtained from a hostname string is always a suffix of the hostname string. This is true for ASCII-based examples like this: hostname: subdomain.example.com domain and registry: example.com But it is not true for non-ASCII-based examples like this: hostname: žřč.com domain and registry: xn--bea5m6d.com The reason is that to obtain the domain and registry, the hostname is being canonicalized. The domain and registry is used to create a sort key for a credential with the given hostname. The idea is to start with the domain and registry part, and then append the remaining subdomains (reversed). The remaining subdomains were computed by taking the prefix of hostname of the length of the difference of the length of the hostname minus the length of the domain and registry part. This number may overflow in case the domain and registry is not a suffix of the hostname. This CL fixes the issue by appending the whole hostname to the domain and registry part. The results look like this: hostname: subdomain.example.com old key: example.com.subdomain new key: example.com.com.example.subdomain hostname: žřč.com old key: undefined new key: xn--bea5m6d.com.com.žřč This is safe (no assumptions about suffixes) and preserves the sorting order of any two credentials before and after this change. It is potentially duplicating the domain and registry part. An alternative solution could just canonicalize the hostname before composing the key, but the code would be more complex for an unclear benefit. The CL also uses a more efficient call to GetDomainAndRegistry (avoiding redoing the canonicalization of a known URL). It also introduces a test and removes an unnecessary call to ASCIIToUTF16. BUG=655949 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove invalid "suffix" assumption from CreateSortKey Currently, CreateSortKey in password_manager_presenter.cc assumes that domain and registry name obtained from a hostname string is always a suffix of the hostname string. This is true for ASCII-based examples like this: hostname: subdomain.example.com domain and registry: example.com But it is not true for non-ASCII-based examples like this: hostname: žřč.com domain and registry: xn--bea5m6d.com The reason is that to obtain the domain and registry, the hostname is being canonicalized. The domain and registry is used to create a sort key for a credential with the given hostname. The idea is to start with the domain and registry part, and then append the remaining subdomains (reversed). The remaining subdomains were computed by taking the prefix of hostname of the length of the difference of the length of the hostname minus the length of the domain and registry part. This number may overflow in case the domain and registry is not a suffix of the hostname. This CL fixes the issue by appending the whole hostname to the domain and registry part. The results look like this: hostname: subdomain.example.com old key: example.com.subdomain new key: example.com.com.example.subdomain hostname: žřč.com old key: undefined new key: xn--bea5m6d.com.com.žřč This is safe (no assumptions about suffixes) and preserves the sorting order of any two credentials before and after this change. It is potentially duplicating the domain and registry part. An alternative solution could just canonicalize the hostname before composing the key, but the code would be more complex for an unclear benefit. The CL also uses a more efficient call to GetDomainAndRegistry (avoiding redoing the canonicalization of a known URL). It also introduces a test and removes an unnecessary call to ASCIIToUTF16. BUG=655949 ========== to ========== Remove invalid "suffix" assumption from CreateSortKey Currently, CreateSortKey in password_manager_presenter.cc assumes that domain and registry name obtained from a hostname string is always a suffix of the hostname string. This is true for ASCII-based examples like this: hostname: subdomain.example.com domain and registry: example.com But it is not true for non-ASCII-based examples like this: hostname: žřč.com domain and registry: xn--bea5m6d.com The reason is that to obtain the domain and registry, the hostname is being canonicalized. The domain and registry is used to create a sort key for a credential with the given hostname. The idea is to start with the domain and registry part, and then append the remaining subdomains (reversed). The remaining subdomains were computed by taking the prefix of hostname of the length of the difference of the length of the hostname minus the length of the domain and registry part. This number may overflow in case the domain and registry is not a suffix of the hostname. This CL fixes the issue by appending the whole hostname to the domain and registry part. The results look like this: hostname: subdomain.example.com old key: example.com.subdomain new key: example.com.com.example.subdomain hostname: žřč.com old key: undefined new key: xn--bea5m6d.com.com.žřč This is safe (no assumptions about suffixes) and preserves the sorting order of any two credentials before and after this change. It is potentially duplicating the domain and registry part. An alternative solution could just canonicalize the hostname before composing the key, but the code would be more complex for an unclear benefit. The CL also uses a more efficient call to GetDomainAndRegistry (avoiding redoing the canonicalization of a known URL). It also introduces a test and removes an unnecessary call to ASCIIToUTF16. BUG=655949 Committed: https://crrev.com/f77c3ffef00e68fe59150c78619dbdf289677400 Cr-Commit-Position: refs/heads/master@{#427051} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f77c3ffef00e68fe59150c78619dbdf289677400 Cr-Commit-Position: refs/heads/master@{#427051}
Message was sent while issue was closed.
https://codereview.chromium.org/2439953004/diff/40001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/2439953004/diff/40001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:323: LOG(INFO) << GURL("http://abč.com"); Hi Vaclav, Is it your intention to output these URLs? Or did you forget to remove them? Regards, Maxim
Message was sent while issue was closed.
https://codereview.chromium.org/2439953004/diff/40001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/password_manager_presenter_unittest.cc (right): https://codereview.chromium.org/2439953004/diff/40001/chrome/browser/ui/passw... chrome/browser/ui/passwords/password_manager_presenter_unittest.cc:323: LOG(INFO) << GURL("http://abč.com"); On 2016/11/15 12:50:53, kolos1 wrote: > Hi Vaclav, > > Is it your intention to output these URLs? Or did you forget to remove them? > > Regards, > Maxim Oops, definitely unintentional. Thanks for spotting it. I filed https://codereview.chromium.org/2507493002 for removing them. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
