simonhong
commented
1 month ago Below code is upstream's query checker before sending query to suggest url.
If default provider url is used, stoped sending if IsQueryPotentiallyPrivate() is true.
If keyword provider url is used, don't care the result of IsQueryPotentiallyPrivate().
Default provider is used when user types inputs at omnibox.
Keyword provider is used when user types search keyword(ex, :g or :br) and then type inputs.
ex) :br below
@brave/sec-team Would you check below two Qs?
- Is ok to skip
IsQueryPotentiallyPrivate()to keyword search? -> It's acceptable - Is
IsQueryPotentiallyPrivate()sufficient for our criteria for making search suggestion default on?
bool SearchProvider::IsQuerySuitableForSuggest(bool* query_is_private) const {
*query_is_private = IsQueryPotentiallyPrivate();
// Don't run Suggest in incognito mode, if the engine doesn't support it, or
// if the user has disabled it. Also don't send potentially private data
// to the default search provider. (It's always okay to send explicit
// keyword input to a keyword suggest server, if any.)
const TemplateURL* default_url = providers_.GetDefaultProviderURL();
const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
return !client()->IsOffTheRecord() && client()->SearchSuggestEnabled() &&
((default_url && !default_url->suggestions_url().empty() &&
!*query_is_private) ||
(keyword_url && !keyword_url->suggestions_url().empty()));
}
bool SearchProvider::IsQueryPotentiallyPrivate() const {
if (input_.text().empty())
return false;
// Check the scheme. If this is UNKNOWN/URL with a scheme that isn't
// http/https/ftp, we shouldn't send it. Sending things like file: and data:
// is both a waste of time and a disclosure of potentially private, local
// data. Other "schemes" may actually be usernames, and we don't want to send
// passwords. If the scheme is OK, we still need to check other cases below.
// If this is QUERY, then the presence of these schemes means the user
// explicitly typed one, and thus this is probably a URL that's being entered
// and happens to currently be invalid -- in which case we again want to run
// our checks below. Other QUERY cases are less likely to be URLs and thus we
// assume we're OK.
if (!base::EqualsCaseInsensitiveASCII(input_.scheme(), url::kHttpScheme) &&
!base::EqualsCaseInsensitiveASCII(input_.scheme(), url::kHttpsScheme) &&
!base::EqualsCaseInsensitiveASCII(input_.scheme(), url::kFtpScheme))
return (input_.type() != metrics::OmniboxInputType::QUERY);
// Don't send URLs with usernames, queries or refs. Some of these are
// private, and the Suggest server is unlikely to have any useful results
// for any of them. Also don't send URLs with ports, as we may initially
// think that a username + password is a host + port (and we don't want to
// send usernames/passwords), and even if the port really is a port, the
// server is once again unlikely to have and useful results.
// Note that we only block based on refs if the input is URL-typed, as search
// queries can legitimately have #s in them which the URL parser
// overaggressively categorizes as a url with a ref.
const url::Parsed& parts = input_.parts();
if (parts.username.is_nonempty() || parts.port.is_nonempty() ||
parts.query.is_nonempty() ||
(parts.ref.is_nonempty() &&
(input_.type() == metrics::OmniboxInputType::URL)))
return true;
// Don't send anything for https except the hostname. Hostnames are OK
// because they are visible when the TCP connection is established, but the
// specific path may reveal private information.
if (base::EqualsCaseInsensitiveASCII(input_.scheme(), url::kHttpsScheme) &&
parts.path.is_nonempty())
return true;
return false;
}
fmarier
kjozwiak
GeetaSarvadnya
Description
We should not pass sensitive data to fetch search suggestions This is prerequisite before enabling search suggestions on by default.
Steps to reproduce
12345678in omnibox and check no suggestions in omnibox popup (we prevent suggestions for digits longer than 7)1234567in omnibox and check some suggestions added in omnibox popupActual result
n/a
Expected result
n/a
Reproduces how often
Easily reproduced
Brave version (brave://version info)
All versions
Channel information
Reproducibility
Miscellaneous information
cc @rebron