Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search Suggestions Enhancement #38137

Closed
anthonypkeane opened this issue May 7, 2024 · 8 comments · Fixed by brave/brave-core#24376
Closed

Search Suggestions Enhancement #38137

anthonypkeane opened this issue May 7, 2024 · 8 comments · Fixed by brave/brave-core#24376

Comments

@anthonypkeane
Copy link

Description:

For context please search for Spec: Brave Search Suggestions Default On (2024) on GDrive

Security Review:https://github.com/brave/reviews/issues/1567

When Brave Search is set as the default search engine (Standard Tab), search suggestions will be enabled by default. Given this, for New Users of Brave who download the browser in countries where Brave Search is the default, please:

  • Remove the iOS search suggestions opt-in as it will no longer be necessary.

Additionally, for all users of Brave:

For Reference:
The 12 countries where Brave Search is currently set as default include: US, CA, GB, DE, FR, MX, BR, IN, ES, AR, AT, IT.

Additionally:

  • If a user enables search suggestions in settings at any point, that preference should be respected, regardless of the search engine in use.
  • If a user disables search suggestions in settings, that preference should also be respected, regardless of the search engine.
@anthonypkeane anthonypkeane added the OS/iOS Fixes related to iOS browser functionality label May 7, 2024
@iccub iccub self-assigned this May 8, 2024
@soner-yuksel
Copy link

So multiple questions and concerns from mobile side here related with Additionally section.

  1. Prevent search suggestions from displaying when text is copy-pasted into the URL bar

So this is very vague, user can add a character at any moment of search display before search is executed.

To determine this, the only way I can think of is to compare the entry done in search bar and the entry after a new entry is performed is combination of existing text + what is in clipboard.

So a question arise here when user copied a text we hid the search suggestions section but lets say user added a new character to the copied text should we redisplay search suggestion again. First of all this is not easy to detect and the execution of this will make search suggestions look janky and buggy in some circumstances.

I am kinda questioning and struggling the to understand the reasoning behind this. And this should be mentioned in the security ticket open for this issue, it is not mentioned there.

  1. Remove personally identifiable information (PII) and sensitive data from URL bar input, similar to how it is currently handled in search

First concern is we should not manipulate the text user copied for any reason on client. It is the user who is entering this text malicious or not. Our responsibility is to securely execute or block etc the query entered.

This what we are doing already, for instance you cant execute a malicious script from url bar etc.

Also the example code linked from Search Repo is not removing any PII or something from the query, it is doing some process related with copy pasting and doing it in a way first determining the query is copied if it is longer than certain number of characters and using a function ?! called isSuspiciousQuery or isSafeQueryURLs and returning a boolean after some algorithm we have no details bout here. And we also have no idea where is this result used.

IMO both these options should not be implemented on client side. And if it will, they should be properly clarified and documented in addition this requirement should be mentioned in security ticket.

@iccub
Copy link

iccub commented May 17, 2024

For 1 just hide it when you paste, if you modify it then they can appear no matter if you edited back to the original contents or not.
But I do not see a reason, browsers do not do it. And it is useful if I paste a name of the hotel and it can be recommend more words to it

Overall I think this ticket can be split into few smaller ones.

@soner-yuksel
Copy link

soner-yuksel commented May 20, 2024

Totally agree we should be adding these additional parts into new tickets so we can discuss their relevance and also open new security tickets for them.

For 1 I think we are on the same page, I really don not understand the reasoning behind it and we should carry over to a new issue and discuss it there.

And for 2 it needs brand new ticket with proper requirements even to start and also security ticket for proposed changes which isnt so clear.

@iccub
Copy link

iccub commented May 20, 2024

I found more details in the spec, so it's not really up to us. I will work on it and I shared our concerns in that spec as well.
I will also ask if we can implement it step by step

@anthonypkeane
Copy link
Author

Please note that there are other settings like #38688 (comment) that are at play here. Disabling Search Suggestions entirely should disable probably disable (#38688 (comment))

@kjozwiak
Copy link
Member

@Uni-verse @hffvld I went through brave/brave-core#24376 (comment) as we had uplifts into 1.69.x & 1.70.x but decided to let the above ride the trains via 1.71.x. Can use my example as a template but I would check a few more regions just in case to ensure everything is working. For example:

  • check two regions that have Brave Search as the default to ensure the above work is working as expected
  • check two regions that doesn't use Brave Search as the default and ensure they're still getting enable search suggestions

@Uni-verse Uni-verse added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Oct 17, 2024
@Uni-verse
Copy link
Contributor

Uni-verse commented Oct 17, 2024

Verified on iPhone 12 running iOS 18 using version 1.71.112

Brave Default SE (US)

  • Ensured Show Search Suggestions is enabled in Search Engine settings.
  • Ensured search suggestions are shown when entering search term into omnibox and enable search suggestions modal is not shown in the omnibox area.
  • Ensured search suggestions are now shown when entering invalid/suspicious search terms.
  • Ensured disabling Show Search Suggestions will not show search suggestions.
example example example example example
Image Image Image Image Image

Brave Default SE (France)

  • Ensured Show Search Suggestions is enabled in Search Engine settings.
  • Ensured search suggestions are shown when entering search term into omnibox and enable search suggestions modal is not shown in the omnibox area.
  • Ensured search suggestions are now shown when entering invalid/suspicious search terms.
  • Ensured disabling Show Search Suggestions will not show search suggestions.
  • Ensured changing default SE will still show search suggestions.
example example example example example
Image Image Image Image Image

Brave Not Default SE (Singapore)

  • Ensured Show Search Suggestions is disabled in Search Engine settings for region where Brave is not default.
  • Ensured that enable search suggestions dialog is shown in omnibox.
  • Ensured that search suggestions are shown after opting in the omnibox and the setting is not enabled in Search Engine settings.
example example example example example
Image Image Image Image Image

Brave Not Default SE (South Africa)

  • Ensured Show Search Suggestions is disabled in Search Engine settings for region where Brave is not default.
  • Ensured that enable search suggestions dialog is shown in omnibox.
  • Ensured that search suggestions are shown after opting in the omnibox and the setting is not enabled in Search Engine settings.
example example example
Image Image Image

@Uni-verse
Copy link
Contributor

Uni-verse commented Oct 17, 2024

Verified on iPad running iOS 17.5.1 using version 1.71.115

Brave Default SE (US)

  • Ensured Show Search Suggestions is enabled in Search Engine settings.
  • Ensured search suggestions are shown when entering search term into omnibox and enable search suggestions modal is not shown in the omnibox area.
  • Ensured search suggestions are now shown when entering invalid/suspicious search terms.
  • Ensured disabling Show Search Suggestions will not show search suggestions.
example example example
Image Image Image

Brave Not Default SE (Sweden)

  • Ensured Show Search Suggestions is disabled in Search Engine settings for region where Brave is not default.
  • Ensured that enable search suggestions dialog is shown in omnibox.
  • Ensured that search suggestions are shown after opting in the omnibox and the setting is not enabled in Search Engine settings.
example example example example example
Image Image Image Image Image

Brave Not Default Upgrade Case (Russia)

  • Ensured Show Search Suggestions is disabled in Search Engine settings for region where Brave is not default.
  • Ensured that enable search suggestions dialog is shown in omnibox.
  • Ensured that search suggestions are shown after opting in the omnibox and the setting is not enabled in Search Engine settings.
example example example
Image Image Image

@Uni-verse Uni-verse added QA Pass - iPad and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants