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

Check "Bring-in-Brave-feature-states-for-privacy-security.patch" from calyx #469

Open
uazo opened this issue Oct 26, 2023 · 4 comments
Open
Labels
task These are things I tell myself I should do

Comments

@uazo
Copy link
Owner

uazo commented Oct 26, 2023

source https://github.com/CalyxOS/platform_external_calyx_chromium/blob/android13/build/patches/Bring-in-Brave-feature-states-for-privacy-security.patch

@uazo uazo added the task These are things I tell myself I should do label Oct 26, 2023
@uazo
Copy link
Owner Author

uazo commented Oct 31, 2023

nothing to copy, most of them identical or blocked by the firewall.
for the different ones I do not understand the reason: @chirayudesai could I ask you why you have disabled anonymous frames, partitioned cookies, and DIPS?

FOR ME: Only to check EnableLensPing:

"The Lens ping should return response code 204, so opening it in the current tab will be invisible to the user"

in

void CoreTabHelper::TriggerLensPingIfEnabled() {
  if (lens::features::GetEnableLensPing()) { //
    lens_ping_start_time_ = base::TimeTicks::Now();
    awaiting_lens_ping_response_ = lens::features::GetLensPingIsSequential();

    // The Lens ping should return response code 204, so opening it in the
    // current tab will be invisible to the user.
    GURL lens_ping_url = GURL(lens::features::GetLensPingURL());
    content::OpenURLParams lens_ping_url_params(
        lens_ping_url, content::Referrer(), WindowOpenDisposition::CURRENT_TAB,
        ui::PAGE_TRANSITION_AUTO_TOPLEVEL, false);
    web_contents()->OpenURL(lens_ping_url_params);
  }
}

I don't like that statement for web_contents()->OpenURL()

@t-m-w
Copy link

t-m-w commented Nov 1, 2023

could I ask you why you have disabled anonymous frames, partitioned cookies, and DIPS?

tl;dr: I don't have an answer for those specific changes, since they're straight from Brave's privacy/security team(s), but if you think they should be enabled for us, we would definitely value that input!

Every so often, we take a look at Brave's changes, and if their privacy/security team flags something, typically it's considered a good idea to go along with it unless it seems obviously unwanted for us, since they are able to put more time and effort into their research than we would have the resources to do.

brave_main_delegate_browsertest.cc has been a convenient reference (obviously not that exact tag every time), especially with blame to make it easier to see the corresponding commits. Need to be a bit careful, though, since sometimes a test for a feature flag remains there even after Google removes the feature flag. Following along in the commit to the actual feature override file (not just the test) and seeing if the flag is truly still overridden in current revisions of the file is helpful for that.

Here are some of their commits about these changes. Occasionally there are issues or PRs with more detail to go along with them, but I couldn't easily find any for these:

Apart from that, my knowledge of these is limited, and it's possible some of these may be disabled for Brave-specific reasons that don't apply to us. They may have alternative implementations of their own -- there were one or two changes where that was obvious, so we skipped those -- and if you think any of these should remain enabled, great! We could certainly consider turning them back on in our variant, too.

@uazo
Copy link
Owner Author

uazo commented Nov 1, 2023

thank you for your reply!

Every so often, we take a look at Brave's changes, and if their privacy/security team flags something, typically it's considered a good idea to go along with it unless it seems obviously unwanted for us, since they are able to put more time and effort into their research than we would have the resources to do.

of course, we are in the same boat.
but if you want please also ask me, maybe we can help each other out, I normally check all the code for each flag, look for tools for verification and read the specs repos. but I am not infallible!

brave_main_delegate_browsertest.cc has been a convenient reference (obviously not that exact tag every time), especially with blame to make it easier to see the corresponding commits.

Thank you, I will take this into account in #405 and #474

Following along in the commit to the actual feature override file (not just the test) and seeing if the flag is truly still overridden in current revisions of the file is helpful for that.

I guess it's complex, I think I should do the brave build myself to keep it under control. For now I use https://github.com/search?q=repo%3Abrave%2Fbrave-core+Disable+features+flagged&type=commits or https://github.com/brave/brave-browser/projects/2
in cromite I have the same problem but with chromium, I will try to improve the situation with #403

I couldn't easily find any for these:

and it's possible some of these may be disabled for Brave-specific reasons that don't apply to us.

I think of DIPS mainly. Brave has a different management which I imagine overlaps with that done by chromium and so they disabled it.

and if you think any of these should remain enabled, great! We could certainly consider turning them back on in our variant, too.

No, I didn't mean that, make your own evaluations on that. I was simply looking for an explanation, here I am making my own decisions and I could be wrong, I was looking for a comparison.

@t-m-w
Copy link

t-m-w commented Nov 1, 2023

thank you for your reply!

Of course! And thanks for your detailed reply as well! In this case but also in general, you've dug much more into the code than we have, and CalyxOS Chromium is based on Cromite, so we value your evaluations a lot.

I'll give another look at your suggestions here soon, when I'm looking at Chromium again (one of the next builds).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task These are things I tell myself I should do
Projects
None yet
Development

No branches or pull requests

2 participants