disableWebSecurity -> allowDisplayingInsecureContent #359

jnugh · 2016-11-04T19:39:01Z

Allow mixed content does not use disableWebSecurity anymore.

We don't use a sledgehammer to crack a nut when this gets merged. Disabling security features is usually a bad idea - in this case we do not need to disable all security features to reach the goal.
As of electron 1.4.5 we can disable only certain security features as we need to.

This also fixes the youtube preview issue which was related to the disabled CORS security feature.

Complete Mattermost Contributor Agreement
Execute npm run prettify to format codes
Update CHANGELOG.md and/or doc/*.md if it's necessary.
Write about environment which you tested
- Ubuntu 16.10 Unity

yuya-oc · 2016-11-05T10:39:55Z

Tested on macOS 10.12. The feature works well!

On the https server (pre-release.mattermost.com), they appear:

"http"(insecure) images
youtube preview

yuya-oc

In test/specs/browser/settings_test.js, the test script confirms whether the attribute is actually applied. So please update two things.

Then, please update the description in src/browser/settings.jsx.

yuya-oc · 2016-11-05T10:41:00Z

src/browser/index.jsx

@@ -343,7 +343,7 @@ var MattermostView = React.createClass({
    // This option disables the same-origin policy and allows js/css/plugins not only content like images.


Please update.

yuya-oc · 2016-11-05T10:41:17Z

src/browser/index.jsx

@@ -343,7 +343,7 @@ var MattermostView = React.createClass({
    // This option disables the same-origin policy and allows js/css/plugins not only content like images.
    if (config.disablewebsecurity === true) {
      // webview.setAttribute('disablewebsecurity', false) disables websecurity. (electron's bug?)


Should be removed.

yuya-oc · 2016-11-05T10:47:00Z

CHANGELOG.md

@@ -23,6 +23,8 @@ Release date: TBD
 ### Bug Fixes
 - Fixed wrong cursor for "Edit" and "Remove" in Setting page
 - Fixed an issue where "Zoom in/out" does not properly work
+ - "Allow mixed content" does not disable web-security generally


Is there any other exact description? For example, ""Allow mixed content" did not disable insecure css/javascript". Alternatively, ""Allow mixed content" now allows only insecure images" in Improvements section. Any thoughts?

It still allows insecure (as in http content on an https served mattermost instance) JS and CSS inside the webview.
However when someone manages to inject CSS / JS into the webview by hacking the server itself he cannot do kinda all things from within the webview.

yuya-oc · 2016-11-05T10:58:02Z

You can find the artifacts to test here: https://circleci.com/gh/mattermost/desktop/768#artifacts

jasonblais · 2016-11-05T18:49:18Z

Tested on 64-bit Windows installer and works great. Big thanks @jnugh!

Test cases:

Mixed content enabled: http, https images and YouTube videos render
Mixed content disabled: http images don't render, YouTube videos and https images do

I assume the build failure is due to the test script Yuya mentions above.

jasonblais · 2016-11-05T18:49:57Z

One note: Anything else we should test with the Electron dependency bump from 1.4.2 --> 1.4.5?

I noticed that the zoom in/out issue @yuya-oc described here no longer reproduces. If I click on the tab bar or the chat area, zooming in and out works properly.

The zoom rendering issues only reproduce when restarting the app as described in #334.

yuya-oc · 2016-11-09T15:11:45Z

@jasonblais As far as reading Electron changelog, I can't find significant change which is applicable for our app.

And I tested the app on Windows 10 again. I think that the issue still reproduces when clicking the tab bar. Would you check again?

jasonblais · 2016-11-09T22:57:16Z

I tried again on Windows 10, and I was able to reproduce it after clicking on the tab bar twice.

After clicking on tab bar once

After clicking on tab bar twice

jnugh · 2016-11-15T17:16:26Z

Just rebased and pushed multiple changes:

Updated the tests - no failures anymore on my machine
Changed some comments inside the code, as requested in the review, I didn't change the code itself
Updated Changelog to be more specific about the security features disabled previously

jnugh · 2016-11-15T17:29:29Z

https://circleci.com/gh/mattermost/desktop/780#artifacts 😄

jasonblais

Testing on the latest artifact, /780, http images no longer render whether Allow mixed content is on or not.

It was working with /768, not sure what changed?

Sample image I used for testing: http://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png

jasonblais · 2016-11-15T19:55:35Z

CHANGELOG.md

@@ -24,6 +24,8 @@ Release date: TBD
 ### Bug Fixes
 - Fixed wrong cursor for "Edit" and "Remove" in Setting page
 - Fixed an issue where "Zoom in/out" does not properly work
+ - The "Allow mixed content" setting used to disable multiple security features to allow http content


What does multiple security features to allow http content mean..?

@jnugh Are there any updates for here?

It means: allowRunningInsecureContent is disabled and CORS is enabled now.

Cool, wondering how we might be able to translate it to a non-technical end user or admin. Would this be accurate?

The "Allow mixed content" setting that enables http content no longer disables existing features such as YouTube previews

Which is what the next line says ;) I will simply remove this note and leave

YouTube preview works, even if mixed content is allowed

yuya-oc · 2016-11-15T23:22:56Z

In package.json, Electron is not 1.4.5 probably due to rebasing. Please check.

jnugh · 2016-11-16T08:25:43Z

You are right, seems like I confused the prebuilt package version I removed during merge with the resulting one 😞
Sorry for that - https://circleci.com/gh/mattermost/desktop/781#artifacts should have a working version in a few seconds.

jasonblais · 2016-11-16T13:07:28Z

Sorry @jnugh, it seems like the build is failing, can you help take a look?

jnugh · 2016-11-16T16:26:13Z

Seems like CircleCI (or the the Docker Repo) is having issues right now.
Err https://apt.dockerproject.org ubuntu-trusty/main amd64 Packages
gnutls_handshake() failed: Handshake failed

I restarted twice by pushing a new ref - will try again later.

yuya-oc · 2016-11-17T15:53:38Z

I restarted CircleCI, and the test is passing.

https://circleci.com/gh/mattermost/desktop/784#artifacts

jasonblais · 2016-11-18T00:53:11Z

Test cases:

Mixed content enabled: http, https images and YouTube videos render
Mixed content disabled: http images don't render, YouTube videos and https images do

All passed on 64-bit Windows installer 👍

Thanks @jnugh :)

yuya-oc · 2016-11-19T03:37:34Z

Tested on macOS, the test cases passed!

jasonblais · 2016-11-22T20:22:24Z

Thanks @jnugh! :)

yuya-oc · 2016-11-23T11:18:49Z

@jnugh Thanks many times!

yuya-oc suggested changes Nov 5, 2016

View reviewed changes

yuya-oc added All Platforms null Windows (looking for testers) labels Nov 5, 2016

yuya-oc added this to the v3.5.0 milestone Nov 5, 2016

jasonblais removed the Windows (looking for testers) label Nov 5, 2016

jnugh force-pushed the yt branch from 13278c7 to c8e573b Compare November 15, 2016 17:13

jasonblais added MacOS (looking for testers) Windows (looking for testers) labels Nov 15, 2016

jasonblais requested changes Nov 15, 2016

View reviewed changes

Fixes mattermost#161

3d8d24c

jnugh force-pushed the yt branch from c8e573b to a44298e Compare November 16, 2016 08:24

jnugh force-pushed the yt branch 2 times, most recently from c1f61a8 to 6b13674 Compare November 16, 2016 15:39

jasonblais approved these changes Nov 18, 2016

View reviewed changes

jasonblais removed the Windows (looking for testers) label Nov 18, 2016

yuya-oc removed the MacOS (looking for testers) label Nov 19, 2016

jnugh added 2 commits November 22, 2016 21:19

📕 Changelog entry for mixed content update

d2b8892

Update settings specs to check for the webpreferences attribute

b715637

jnugh force-pushed the yt branch from 6b13674 to b715637 Compare November 22, 2016 20:20

yuya-oc approved these changes Nov 23, 2016

View reviewed changes

yuya-oc merged commit faa9f84 into mattermost:master Nov 23, 2016

jasonblais mentioned this pull request Aug 30, 2017

clicking on a file:// protocol path does not open windows explorer since version 3.5.0 #579

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disableWebSecurity -> allowDisplayingInsecureContent #359

disableWebSecurity -> allowDisplayingInsecureContent #359

jnugh commented Nov 4, 2016

yuya-oc commented Nov 5, 2016

yuya-oc left a comment

yuya-oc Nov 5, 2016

yuya-oc Nov 5, 2016

yuya-oc Nov 5, 2016

jnugh Nov 15, 2016

yuya-oc commented Nov 5, 2016

jasonblais commented Nov 5, 2016

jasonblais commented Nov 5, 2016

yuya-oc commented Nov 9, 2016 •

edited

jasonblais commented Nov 9, 2016

jnugh commented Nov 15, 2016

jnugh commented Nov 15, 2016 •

edited

jasonblais left a comment

jasonblais Nov 15, 2016

yuya-oc Nov 19, 2016

jnugh Nov 19, 2016

jasonblais Nov 19, 2016

jnugh Nov 22, 2016

yuya-oc commented Nov 15, 2016

jnugh commented Nov 16, 2016

jasonblais commented Nov 16, 2016

jnugh commented Nov 16, 2016

yuya-oc commented Nov 17, 2016

jasonblais commented Nov 18, 2016

yuya-oc commented Nov 19, 2016

jasonblais commented Nov 22, 2016

yuya-oc commented Nov 23, 2016

		@@ -343,7 +343,7 @@ var MattermostView = React.createClass({
		// This option disables the same-origin policy and allows js/css/plugins not only content like images.

disableWebSecurity -> allowDisplayingInsecureContent #359

disableWebSecurity -> allowDisplayingInsecureContent #359

Conversation

jnugh commented Nov 4, 2016

yuya-oc commented Nov 5, 2016

yuya-oc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuya-oc commented Nov 5, 2016

jasonblais commented Nov 5, 2016

jasonblais commented Nov 5, 2016

yuya-oc commented Nov 9, 2016 • edited

jasonblais commented Nov 9, 2016

jnugh commented Nov 15, 2016

jnugh commented Nov 15, 2016 • edited

jasonblais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuya-oc commented Nov 15, 2016

jnugh commented Nov 16, 2016

jasonblais commented Nov 16, 2016

jnugh commented Nov 16, 2016

yuya-oc commented Nov 17, 2016

jasonblais commented Nov 18, 2016

yuya-oc commented Nov 19, 2016

jasonblais commented Nov 22, 2016

yuya-oc commented Nov 23, 2016

yuya-oc commented Nov 9, 2016 •

edited

jnugh commented Nov 15, 2016 •

edited