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

[feature request] auto-upgrade mixed content #4381

Closed
diracdeltas opened this issue May 9, 2019 · 8 comments · Fixed by brave/brave-core#4537
Closed

[feature request] auto-upgrade mixed content #4381

diracdeltas opened this issue May 9, 2019 · 8 comments · Fixed by brave/brave-core#4537

Comments

@diracdeltas
Copy link
Member

diracdeltas commented May 9, 2019

Test plan

See brave/brave-core#4537

Description

Chrome has an experiment to auto-upgrade various types of mixed content: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/ZJxkCJq5zo4/4sSMVZzBAwAJ, https://docs.google.com/document/d/1dp-kuN25wnEbMPNWBxM8LvOjyeydWpXPklNnGcsWK1o/edit#.

This is a good candidate to consider for enabling by default in Brave

Some considerations:

  • Sites which serve different content over HTTPS vs HTTP can cause a confusing situation in which a site experiencing auto-upgrades is not simply broken but just wrong. A contrived example: forbes used to serve their regular site on http://forbes.com and their SecureDrop landing page on https://forbes.com. If a site iframed forbes.com, it would be totally different after the auto-upgrade.
  • At what point in the request cycle should HTTPS Everywhere upgrades happen? Currently I think they might happen after mixed content blocking, which is non-ideal. Ideally I think the order should be HTTPS-E upgrades -> mixed content auto-upgrade -> any content blocking.
@diracdeltas diracdeltas added the priority/P5 Not scheduled. Don't anticipate work on this any time soon. label May 21, 2019
@jonathanKingston
Copy link

Chrome's current plan is to roll out videos and audio in Chrome 80 and measure breakage such that images would be considered in 81, this suggests Brave could match that timeline or even go first.

I'll take a look at the upgrading before HTTPS Everywhere and the current ordering.

@estark37
Copy link

estark37 commented Feb 6, 2020

Yep, it would be great to see Brave ship this as well. Note that we're also planning to ship a "Not Secure" omnibox warning for mixed images in M80, in hopes of driving sites to fix before autoupgrading+blocking in 81.

@diracdeltas diracdeltas added priority/P3 The next thing for us to work on. It'll ride the trains. and removed priority/P5 Not scheduled. Don't anticipate work on this any time soon. labels Feb 6, 2020
@jonathanKingston
Copy link

Yep, it would be great to see Brave ship this as well. Note that we're also planning to ship a "Not Secure" omnibox warning for mixed images in M80, in hopes of driving sites to fix before autoupgrading+blocking in 81.

Enabling this warning also applies to insecure forms and I can't see a flag to upgrade forms, is that something you have considered?

chromium --enable-features=AutoupgradeMixedContent --enable-features=PassiveMixedContentWarning https://very.badssl.com/

@jonathanKingston
Copy link

I just confirmed Chromium code MixedContentChecker::ShouldAutoupgrade() does get called before: OnBeforeURLRequest_HttpsePreFileWork().
This means mixed upgrades from the HTTPS Everywhere list would no longer have any impact.

@estark37
Copy link

estark37 commented Feb 6, 2020

Yep, it would be great to see Brave ship this as well. Note that we're also planning to ship a "Not Secure" omnibox warning for mixed images in M80, in hopes of driving sites to fix before autoupgrading+blocking in 81.

Enabling this warning also applies to insecure forms and I can't see a flag to upgrade forms, is that something you have considered?

I don't think this should be the case. I can't reproduce on https://mixed-form.badssl.com. very.badssl.com might contain a mixed image as well as a mixed form which is maybe why you're seeing the warning on that page?

chromium --enable-features=AutoupgradeMixedContent --enable-features=PassiveMixedContentWarning https://very.badssl.com/

@jonathanKingston
Copy link

jonathanKingston commented Feb 7, 2020

I don't think this should be the case. I can't reproduce on https://mixed-form.badssl.com. very.badssl.com might contain a mixed image as well as a mixed form which is maybe why you're seeing the warning on that page?

Yeah I can't seem to reproduce this effectively any more either, it might be something brave is doing differently which is where I was replicating it more often. Perhaps the parameter to the field trial for image upgrading got flipped on my profile?

I uploaded a patch which turns on the warning but also upgrades images by default.

I also chose to turn off the URL bar insecure script control which and instead permit controlling an override on a site specific basis:
image
This certainly could be left to another patch however reading through other bugs it sounded like this was an intended change.

@diracdeltas
Copy link
Member Author

diracdeltas commented Feb 7, 2020

Perhaps the parameter to the field trial for image upgrading got flipped on my profile?

We should be disabling field trials generally as of #4551

However, we purposefully enabled the setting to not show EV indicators (https://github.com/brave/brave-core/pull/2471/files#diff-9413752841345e7c426bf4dd82fe7e60R143) but I guess that's no longer necessary since Chrome does it too.

Thanks for the info!

fmarier pushed a commit to brave/brave-core that referenced this issue Feb 13, 2020
@fmarier fmarier added this to the 1.6.x - Nightly milestone Feb 14, 2020
@bbondy bbondy modified the milestones: 1.6.x - Beta, 1.7.x - Dev Mar 10, 2020
@btlechowski
Copy link

btlechowski commented Mar 25, 2020

Verification passed on

Brave 1.7.71 Chromium: 80.0.3987.149 (Official Build) dev (64-bit)
Revision 5f4eb224680e5d7dca88504586e9fd951840cac6-refs/branch-heads/3987_137@{#16}
OS Ubuntu 18.04 LTS

Verified test plan from brave/brave-core#4537
Verified the http was upgraded to https
Verified page is secured

image

Verification passed on

Brave 1.7.74 Chromium: 80.0.3987.149 (Official Build) dev (64-bit)
Revision 5f4eb224680e5d7dca88504586e9fd951840cac6-refs/branch-heads/3987_137@{#16}
OS Windows 10 OS Version 1803 (Build 17134.1006)

Verification PASSED on macOS 10.15.3 x64 Catalina using the following build:

Brave | 1.7.78 Chromium: 80.0.3987.149 (Official Build) dev (64-bit)
-- | --
Revision | 5f4eb224680e5d7dca88504586e9fd951840cac6-refs/branch-heads/3987_137@{#16}
OS | macOS Version 10.15.3 (Build 19D76)
  • Verified the STR/Test plan outlined under Upgrade mixed content connection brave-core#4537
  • ensured that http://mixed.badssl.com was upgraded to https://mixed.badssl.com
  • ensured that the page is "yellow" and upgraded to secure
  • ensured that Learn more opened a Brave support page
  • ensured Certificate, Cookies and Site Settings worked as expected when click the lock
  • ensured the above is working on both Tor/PB windows

Screen Shot 2020-03-30 at 12 28 42 PM

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.

8 participants