Skip to content

GitLab

T
tangled
Commit f77c3ffe authored by vabr's avatar vabr Committed by Commit bot
Browse files
Options

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

Review-Url: https://codereview.chromium.org/2439953004
Cr-Commit-Position: refs/heads/master@{#427051}
parent ccac5320
Hide whitespace changes
Inline Side-by-side
Showing with 45 additions and 13 deletions
chrome/browser/ui/passwords/password_manager_presenter.cc
View file @ f77c3ffe
......@@ -63,11 +63,11 @@ std::string GetEntryTypeCode(bool is_android_uri, bool is_clickable) {
return "2";
}
// Creates key for sorting password or password exception entries.
// The key is eTLD+1 followed by subdomains
// (e.g. secure.accounts.example.com => example.com.accounts.secure).
// If |entry_type == SAVED|, username, password and federation are appended to
// the key. The entry type code (non-Android, Android w/ or w/o affiliated web
// Creates key for sorting password or password exception entries. The key is
// eTLD+1 followed by the reversed list of domains (e.g.
// secure.accounts.example.com => example.com.com.example.accounts.secure). If
// |entry_type == SAVED|, username, password and federation are appended to the
// key. The entry type code (non-Android, Android w/ or w/o affiliated web
// realm) is also appended to the key.
std::string CreateSortKey(const autofill::PasswordForm& form,
PasswordEntryType entry_type) {
......@@ -77,18 +77,15 @@ std::string CreateSortKey(const autofill::PasswordForm& form,
std::string origin = password_manager::GetShownOriginAndLinkUrl(
form, &is_android_uri, &link_url, &is_clickable);
if (!is_clickable) { // e.g. android://com.example.r => r.example.com.
if (!is_clickable) // e.g. android://com.example.r => r.example.com.
origin = password_manager::StripAndroidAndReverse(origin);
}
std::string site_name =
net::registry_controlled_domains::GetDomainAndRegistry(
origin, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
if (site_name.empty()) // e.g. localhost.
site_name = origin;
std::string key =
site_name + password_manager::SplitByDotAndReverse(StringPiece(
&origin[0], origin.length() - site_name.length()));
std::string key = site_name + password_manager::SplitByDotAndReverse(origin);
if (entry_type == PasswordEntryType::SAVED) {
key = key + kSortKeyPartsSeparator +
......
chrome/browser/ui/passwords/password_manager_presenter_unittest.cc
View file @ f77c3ffe
......@@ -137,7 +137,7 @@ void PasswordManagerPresenterTest::SortAndCheckPositions(
const SortEntry& entry = test_entries[i];
std::unique_ptr<autofill::PasswordForm> form(new autofill::PasswordForm());
form->signon_realm = entry.origin;
form->origin = GURL(base::ASCIIToUTF16(entry.origin));
form->origin = GURL(entry.origin);
if (entry_type == PasswordEntryType::SAVED) {
form->username_value = base::ASCIIToUTF16(entry.username);
form->password_value = base::ASCIIToUTF16(entry.password);
......@@ -163,8 +163,7 @@ void PasswordManagerPresenterTest::SortAndCheckPositions(
if (entry.expected_position >= 0) {
SCOPED_TRACE(testing::Message("position in sorted list: ")
<< entry.expected_position);
EXPECT_EQ(GURL(base::ASCIIToUTF16(entry.origin)),
list[entry.expected_position]->origin);
EXPECT_EQ(GURL(entry.origin), list[entry.expected_position]->origin);
if (entry_type == PasswordEntryType::SAVED) {
EXPECT_EQ(base::ASCIIToUTF16(entry.username),
list[entry.expected_position]->username_value);
......@@ -268,6 +267,20 @@ TEST_F(PasswordManagerPresenterTest, Sorting_HideDuplicates) {
PasswordEntryType::SAVED);
}
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);
}
TEST_F(PasswordManagerPresenterTest, Sorting_PasswordExceptions) {
const SortEntry test_cases[] = {
{"http://example-b.com", nullptr, nullptr, nullptr, nullptr, 1},
......@@ -305,4 +318,26 @@ TEST_F(PasswordManagerPresenterTest, Sorting_Federations) {
PasswordEntryType::SAVED);
}
TEST_F(PasswordManagerPresenterTest, Sorting_SpecialCharacters) {
// URLs with encoded special characters should not cause crash during sorting.
LOG(INFO) << GURL("http://abč.com");
LOG(INFO) << GURL("http://abc.com");
LOG(INFO) << GURL("http://ábc.com");
LOG(INFO) << GURL("http://uoy.com");
LOG(INFO) << GURL("http://üöÿ.com");
LOG(INFO) << GURL("http://zrc.com");
LOG(INFO) << GURL("http://žřč.com");
const SortEntry test_cases[] = {
{"https://xn--bea5m6d.com/", "user_a", "pwd", nullptr, nullptr, 4},
{"https://uoy.com/", "user_a", "pwd", nullptr, nullptr, 1},
{"https://zrc.com/", "user_a", "pwd", nullptr, nullptr, 6},
{"https://abc.com/", "user_a", "pwd", nullptr, nullptr, 0},
{"https://xn--ab-fma.com/", "user_a", "pwd", nullptr, nullptr, 2},
{"https://xn--bc-lia.com/", "user_a", "pwd", nullptr, nullptr, 3},
{"https://xn--ndalk.com/", "user_a", "pwd", nullptr, nullptr, 5},
};
SortAndCheckPositions(test_cases, arraysize(test_cases),
PasswordEntryType::SAVED);
}
} // namespace
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment