fix various noscript issues

fix #8403 1. change noscript icon from button to span to fix CSP error 2. fix repeated toggling of noScriptInfo dialog 3. refactor to remove deprecated code and combine windowActions 4. increase noscript click area 5. fix security icon disappearing on twitter.com test plan: 1. disable scripts on twitter.com. the lock icon should not disappear 2. click noscript icon 3. open browser console. you should not see any CSP errors. 4. now click the noscript icon repeatedly. you should see the popup come up and disappear repeatedly.
brave · Apr 20, 2017 · 3f3e167 · 3f3e167
1 parent 8e0bb2e
commit 3f3e167
Show file tree

Hide file tree

Showing 9 changed files with 53 additions and 69 deletions.
diff --git a/app/renderer/components/navigation/urlBar.js b/app/renderer/components/navigation/urlBar.js
@@ -52,7 +52,6 @@ class UrlBar extends ImmutableComponent {
         this.urlInput.setSelectionRange(len, len + suffixLen)
       }
     }, 10)
-    this.onNoScript = this.onNoScript.bind(this)
   }
 
   get locationValueSuffix () {
@@ -498,7 +497,7 @@ class UrlBar extends ImmutableComponent {
   }
 
   onNoScript () {
-    windowActions.setNoScriptVisible(!this.props.noScriptIsVisible)
+    windowActions.setNoScriptVisible()
   }
 
   onContextMenu (e) {
@@ -568,12 +567,12 @@ class UrlBar extends ImmutableComponent {
       {
         !this.showNoScriptInfo
         ? null
-        : <span className={css(styles.noScriptContainer)}>
-          <button
+        : <span className={css(styles.noScriptContainer)}
+          onClick={this.onNoScript}>
+          <span
             data-l10n-id='noScriptButton'
             data-test-id='noScriptButton'
-            className={css(styles.noScriptButton)}
-            onClick={this.onNoScript} />
+            className={css(styles.noScriptButton)} />
         </span>
       }
       {
@@ -592,8 +591,8 @@ class UrlBar extends ImmutableComponent {
 const styles = StyleSheet.create({
   noScriptContainer: {
     display: 'flex',
-    paddingLeft: '5px',
-    marginRight: '-3px',
+    padding: '5px',
+    marginRight: '-8px',
     WebkitAppRegion: 'drag'
   },
   noScriptButton: {

diff --git a/docs/state.md b/docs/state.md
@@ -474,7 +474,6 @@ WindowStore
     security: {
       blockedRunInsecureContent: Array<string>, // sources of blocked active mixed content
       isSecure: (boolean|number), // true = fully secure, false = fully insecure, 1 = partially secure
-      isExtendedValidation: boolean, // is using https ev. not currently used.
       loginRequiredDetail: {
         isProxy: boolean,
         host: string,

diff --git a/docs/windowActions.md b/docs/windowActions.md
@@ -724,25 +724,13 @@ Similar to setBlockedBy but for httpse redirects
 
 
 
-### setNoScript(frameProps, source) 
-
-Sets which scripts were blocked on a page.
-
-**Parameters**
-
-**frameProps**: `Object`, The frame to set blocked info on
-
-**source**: `string`, Source of blocked js
-
-
-
 ### setNoScriptVisible(isVisible) 
 
-Sets whether the noscript icon is visible.
+Sets/toggles whether the noscriptinfo dialog is visible.
 
 **Parameters**
 
-**isVisible**: `boolean`, Sets whether the noscript icon is visible.
+**isVisible**: `boolean`, if undefined, toggle the current state
 
 
 

diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js
@@ -861,21 +861,8 @@ const windowActions = {
   },
 
   /**
-   * Sets which scripts were blocked on a page.
-   * @param {Object} frameProps - The frame to set blocked info on
-   * @param {string} source - Source of blocked js
-   */
-  setNoScript: function (frameProps, source) {
-    dispatch({
-      actionType: windowConstants.WINDOW_SET_NOSCRIPT,
-      frameProps,
-      source
-    })
-  },
-
-  /**
-   * Sets whether the noscript icon is visible.
-   * @param {boolean} isVisible
+   * Sets/toggles whether the noscriptinfo dialog is visible.
+   * @param {boolean=} isVisible - if undefined, toggle the current state
    */
   setNoScriptVisible: function (isVisible) {
     dispatch({

diff --git a/js/components/frame.js b/js/components/frame.js
@@ -820,13 +820,14 @@ class Frame extends ImmutableComponent {
         return
       }
       if (e.isMainFrame && !e.isErrorPage && !e.isFrameSrcDoc) {
+        if (e.url && e.url.startsWith(appConfig.noScript.twitterRedirectUrl) &&
+          this.getFrameBraverySettings(this.props).get('noScript') === true) {
+          // This result will be canceled immediately by sitehacks, so don't
+          // update the load state; otherwise it will not show the security
+          // icon.
+          return
+        }
         windowActions.onWebviewLoadStart(this.frame, e.url)
-        // Clear security state
-        windowActions.setBlockedRunInsecureContent(this.frame)
-        windowActions.setSecurityState(this.frame, {
-          secure: null,
-          runInsecureContent: false
-        })
       }
     }
 

diff --git a/js/constants/appConfig.js b/js/constants/appConfig.js
@@ -39,7 +39,8 @@ module.exports = {
     enabled: false
   },
   noScript: {
-    enabled: false
+    enabled: false,
+    twitterRedirectUrl: 'https://mobile.twitter.com/i/nojs_router'
   },
   flash: {
     enabled: false,

diff --git a/js/data/siteHacks.js b/js/data/siteHacks.js
@@ -4,6 +4,7 @@
 
 const urlParse = require('../../app/common/urlParse')
 const base64Encode = require('../lib/base64').encode
+const appConfig = require('../constants/appConfig')
 
 // Polyfill similar to this: https://github.com/gorhill/uBlock/blob/de1ed89f62bf041416d2a721ec00741667bf3fa8/assets/ublock/resources.txt#L385
 const googleTagManagerRedirect = 'data:application/javascript;base64,' + base64Encode(`(function() { var noopfn = function() { ; }; window.ga = window.ga || noopfn; })();`)
@@ -119,7 +120,7 @@ module.exports.siteHacks = {
     onBeforeSendHeaders: function(details) {
       if (details.requestHeaders.Referer &&
         details.requestHeaders.Referer.startsWith('https://twitter.com/') &&
-        details.url.startsWith('https://mobile.twitter.com/')) {
+        details.url.startsWith(appConfig.noScript.twitterRedirectUrl)) {
         return {
           cancel: true
         }

diff --git a/js/state/frameStateUtil.js b/js/state/frameStateUtil.js
@@ -142,7 +142,7 @@ function getFrameByKey (windowState, key) {
 
 function isFrameSecure (frame) {
   frame = makeImmutable(frame)
-  if (frame && frame.getIn(['security', 'isSecure']) != null) {
+  if (frame && typeof frame.getIn(['security', 'isSecure']) === 'boolean') {
     return frame.getIn(['security', 'isSecure'])
   } else {
     return false
@@ -465,8 +465,7 @@ function addFrame (windowState, tabs, frameOpts, newKey, partitionNumber, active
       caseSensitivity: false
     },
     security: {
-      isSecure: urlParse(url).protocol === 'https:',
-      certDetails: null
+      isSecure: null
     },
     unloaded: frameOpts.unloaded,
     parentFrameKey,

diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js
@@ -306,22 +306,33 @@ const doAction = (action) => {
       })
       break
     case windowConstants.WINDOW_WEBVIEW_LOAD_START:
-      windowState = windowState.mergeIn(['frames', frameStateUtil.getFramePropsIndex(frameStateUtil.getFrames(windowState), action.frameProps)], {
-        loading: true,
-        provisionalLocation: action.location,
-        startLoadTime: new Date().getTime(),
-        endLoadTime: null
-      })
-      windowState = windowState.mergeIn(['tabs', frameStateUtil.getFramePropsIndex(frameStateUtil.getFrames(windowState), action.frameProps)], {
-        loading: true,
-        provisionalLocation: action.location
-      })
-      // For about:newtab we want to have the urlbar focused, not the new frame.
-      // Otherwise we want to focus the new tab when it is a new frame in the foreground.
-      if (action.location !== getTargetAboutUrl('about:newtab')) {
-        focusWebview(activeFrameStatePath(windowState))
+      {
+        const framePath = ['frames', frameStateUtil.getFramePropsIndex(frameStateUtil.getFrames(windowState), action.frameProps)]
+        // Reset security state
+        windowState =
+          windowState.deleteIn(framePath.concat(['security', 'blockedRunInsecureContent']))
+        windowState = windowState.mergeIn(framePath.concat(['security']), {
+          isSecure: null,
+          runInsecureContent: false
+        })
+        // Update loading UI
+        windowState = windowState.mergeIn(framePath, {
+          loading: true,
+          provisionalLocation: action.location,
+          startLoadTime: new Date().getTime(),
+          endLoadTime: null
+        })
+        windowState = windowState.mergeIn(['tabs'].concat(framePath), {
+          loading: true,
+          provisionalLocation: action.location
+        })
+        // For about:newtab we want to have the urlbar focused, not the new frame.
+        // Otherwise we want to focus the new tab when it is a new frame in the foreground.
+        if (action.location !== getTargetAboutUrl('about:newtab')) {
+          focusWebview(activeFrameStatePath(windowState))
+        }
+        break
       }
-      break
     case windowConstants.WINDOW_WEBVIEW_LOAD_END:
       windowState = windowState.mergeIn(['frames', frameStateUtil.getFramePropsIndex(frameStateUtil.getFrames(windowState), action.frameProps)], {
         loading: false,
@@ -524,7 +535,9 @@ const doAction = (action) => {
       windowState = windowState.setIn(['ui', 'mouseInTitlebar'], action.mouseInTitlebar)
       break
     case windowConstants.WINDOW_SET_NOSCRIPT_VISIBLE:
-      windowState = windowState.setIn(['ui', 'noScriptInfo', 'isVisible'], action.isVisible)
+      const noScriptInfoPath = ['ui', 'noScriptInfo', 'isVisible']
+      windowState = windowState.setIn(noScriptInfoPath,
+        typeof action.isVisible === 'boolean' ? action.isVisible : !windowState.getIn(noScriptInfoPath))
       break
     case windowConstants.WINDOW_SET_SITE_INFO_VISIBLE:
       windowState = windowState.setIn(['ui', 'siteInfo', 'isVisible'], action.isVisible)
@@ -610,10 +623,6 @@ const doAction = (action) => {
         windowState = windowState.setIn(path.concat(['security', 'runInsecureContent']),
                                         action.securityState.runInsecureContent)
       }
-      if (action.securityState.certDetails) {
-        windowState = windowState.setIn(path.concat(['security', 'certDetails']),
-                                        action.securityState.certDetails)
-      }
       break
     case windowConstants.WINDOW_SET_BLOCKED_BY:
       const blockedByPath = ['frames', frameStateUtil.getFramePropsIndex(frameStateUtil.getFrames(windowState), action.frameProps), action.blockType, 'blocked']