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

[Desktop] Review non-registered components #11544

Closed
5 tasks done
fmarier opened this issue Sep 1, 2020 · 14 comments
Closed
5 tasks done

[Desktop] Review non-registered components #11544

fmarier opened this issue Sep 1, 2020 · 14 comments
Assignees
Labels

Comments

@fmarier
Copy link
Member

fmarier commented Sep 1, 2020

@bsclifton found that the root cause for #10607 (comment) is that we don't register a necessary component.

It would be good to review the list of components we don't install to make sure there's nothing we should enable:

Going forward, is there a way we can detect new ones and file issues for deciding what to do with them?

@fmarier
Copy link
Member Author

fmarier commented Sep 1, 2020

  • File Types Policies

This is used by Safe Browsing and is a dependency of #6267.

@jumde
Copy link
Contributor

jumde commented Nov 25, 2020

@jumde
Copy link
Contributor

jumde commented Nov 25, 2020

Reached out to Andrew Whalley to check if Safety Tips and Certificate Error Assistant are still being maintained.

@mherrmann
Copy link

@jumde In the wiki, are you sure Legacy TLS Deprecation Configuration should be unsupported? See #10607 (comment).

@mherrmann
Copy link

Just to mention here that this is related to my PR brave/brave-core#7164. In short: I think Brave currently relies on a bug to not install unwanted components. My PR would fix this bug. So any solution to this issue should probably not rely on the bug being there. @jumde pointed out that the correct way to prevent a component from being installed is to not register it in the first place. Eg. by overriding functions like RegisterSafetyTipsComponent.

@mherrmann
Copy link

Also linking #8709 here because I think it is very closely related.

@fmarier
Copy link
Member Author

fmarier commented Nov 27, 2020

@jumde In the wiki, are you sure Legacy TLS Deprecation Configuration should be unsupported? See #10607 (comment).

Our fix isn't particularly obvious, but basically we replaced Chrome's list of exceptions with an empty list. So we disable TLS 1.0 and 1.1 for all sites, no exceptions. Therefore there is no need to load the component which updates Chrome's list.

@mherrmann
Copy link

Therefore there is no need to load the component which updates Chrome's list.

Do you mean the component should not be updated, or it should not be present in Brave at all? Or does it not matter?

@fmarier
Copy link
Member Author

fmarier commented Nov 27, 2020

Do you mean the component should not be updated, or it should not be present in Brave at all? Or does it not matter?

The component is not needed at all in Brave. It doesn't need to be updated or even be present.

@mherrmann
Copy link

Got it, thank you. Who can tell us which components exactly should and shouldn't be in Brave?

@jumde
Copy link
Contributor

jumde commented Nov 30, 2020

@mherrmann - I think just installing the components with a non-zero (0.0.0.0) version number on start (in nightly) makes sense. You can enable Crowd Deny as well to resolve #10280.

@mherrmann
Copy link

Thank you @jumde. Do you mean that the answer to my question "which components should not be installed" is "those components that show up with version 0.0.0.0 when Brave is installed from scratch"?

@jumde
Copy link
Contributor

jumde commented Dec 3, 2020

Issues to track enabling:

  1. Safety Tips: [Security] Enable Safety Tips #12999
  2. Certificate Error Assistant: Enable certificate error assistant #13010

@fmarier
Copy link
Member Author

fmarier commented Dec 9, 2020

All components have been reviewed and issues have been filed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants