Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add option to allow mixed content #3447

Closed
wants to merge 1 commit into from
Closed

Add option to allow mixed content #3447

wants to merge 1 commit into from

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Aug 26, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Ran git rebase -i to squash commits if needed.
  1. Add option to allow mixed content
  2. Change icon of http and mixed content to fa-unlock

fix #3443

Detect blocked mixed content
screen shot 2016-08-26 at 18 05 33

After allowing mixed content
screen shot 2016-08-26 at 18 13 59

Pure http site
screen shot 2016-08-26 at 18 13 45

@@ -557,6 +558,8 @@ class Frame extends ImmutableComponent {
this.webview.addEventListener('content-blocked', (e) => {
if (e.details[0] === 'javascript') {
windowActions.setBlockedBy(this.frame, 'noScript', e.details[1])
} else if (!this.allowRunningInsecureContent()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can now manage running and displaying insecure content through the displayInsecureContent and runInsecureContent content settings which will allow us to treat them like any other site settings. I know you already did a lot of work using the old method and I'm sorry I didn't catch this sooner, but setting these through the webview is problematic because it restarts the renderer process and will potentially break some sites. We had a small number of exceptions before, but if we're opening this up as a general setting we should transition to the contentSetting

Copy link
Member Author

@darkdh darkdh Aug 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification. Changes has been made in 66dc10c.

@diracdeltas diracdeltas self-assigned this Aug 26, 2016
@darkdh darkdh force-pushed the 3443 branch 6 times, most recently from 8c6af21 to 66dc10c Compare August 28, 2016 02:36
@@ -123,6 +127,10 @@ const getContentSettingsFromSiteSettings = (appState) => {
addContentSettings(contentSettings.javascript, hostPattern, '*',
hostSetting.noScript ? 'block' : 'allow')
}
if (typeof hostSetting.allowActiveMixedContent === 'boolean') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to name this setting runInsecureContext. It's easier if we don't have to translate names across various parts of the app

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right. it will make the code more readable. addressed it in cecb967

@bridiver
Copy link
Collaborator

can you add some tests for this? There is some inconsistent property naming that looks like it should be causing issues

@darkdh darkdh force-pushed the 3443 branch 2 times, most recently from ea401ab to 4888633 Compare September 1, 2016 04:53
2. Change icon of http and mixed content to fa-unlock
3. Remove twitch out of site hack
4. Add test

fix brave#3443
reuire brave/muon#47

Auditors: @bridiver

Test Plan:
Visit https://mixed-script.badssl.com/ and click the urlbar lock to
temporarily allow run insecure content and the background color will be red.
@bridiver
Copy link
Collaborator

bridiver commented Sep 1, 2016

just want to get @bradleyrichter @diracdeltas feedback on the popup. I think we should have something like

"Insecure content has been blocked from running on this page. If you allow this content to run it will not be encrypted and it may transmit unencrypted data to other sites."

button - "Run Insecure Content"

@bridiver
Copy link
Collaborator

bridiver commented Sep 1, 2016

warning

@bradleyrichter
Copy link
Contributor

Is it modal? If not, no need for the darkening.

@bridiver
Copy link
Collaborator

bridiver commented Sep 1, 2016

closed with 7078680

@bridiver bridiver closed this Sep 1, 2016
@darkdh darkdh deleted the 3443 branch September 9, 2016 07:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option to temporarily disable mixed content protection
4 participants