Permission host mismatch #13475

jumde · 2018-03-16T08:00:11Z

Test plan

Open https://google.com
Change the address in the URL bar to sms://ggle.com. Verify the external permission message is for sms://ggle.com and not https://google.com
Launch a webserver with test.html:
<a href="sms://ggle.com">Test</a>
Open http(s):///test.html
Click on the hyperlink, verify the permission is requested for sms://ggle.com and not <webserver>

codecov-io · 2018-03-16T08:26:39Z

Codecov Report

Merging #13475 into master will decrease coverage by 0.08%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #13475      +/-   ##
==========================================
- Coverage   56.74%   56.66%   -0.09%     
==========================================
  Files         285      285              
  Lines       28797    28894      +97     
  Branches     4755     4775      +20     
==========================================
+ Hits        16341    16372      +31     
- Misses      12456    12522      +66

Flag	Coverage Δ
#unittest	`56.66% <0%> (-0.09%)`	⬇️

Impacted Files	Coverage Δ
app/filtering.js	`17.7% <0%> (-0.48%)`	⬇️
app/renderer/components/preferences/pluginsTab.js	`40.32% <0%> (-8.7%)`	⬇️
app/common/lib/ledgerUtil.js	`93.38% <0%> (-0.82%)`	⬇️
app/browser/tabs.js	`23.34% <0%> (-0.45%)`	⬇️
js/about/aboutActions.js	`8.87% <0%> (-0.22%)`	⬇️
js/actions/appActions.js	`18.59% <0%> (ø)`	⬆️
app/browser/api/ledger.js	`61.6% <0%> (+0.03%)`	⬆️
app/sessionStore.js	`88.4% <0%> (+0.18%)`	⬆️
app/common/state/ledgerState.js	`84.95% <0%> (+0.22%)`	⬆️
... and 1 more

diracdeltas · 2018-03-16T15:42:46Z

app/filtering.js

@@ -495,9 +496,10 @@ function registerPermissionHandler (session, partition) {
        }
      }

+      let parsedMainFrameUrl = new URL(mainFrameUrl)


can this use urlParse?

With urlParse:

tps://oogle.com/search?q=s%3A%2F%2Fwww.lcebook.com%2F { hash: '', hostname: '', host: '', href: 'tps://oogle.com/search?q=s%3A%2F%2Fwww.lcebook.com%2F', path: '//oogle.com/search?q=s%3A%2F%2Fwww.lcebook.com%2F', pathname: '//oogle.com/search', port: '', protocol: 'tps:', query: 'q=s%3A%2F%2Fwww.lcebook.com%2F', search: '?q=s%3A%2F%2Fwww.lcebook.com%2F', origin: '' }

with new URL

URL { href: tps://oogle.com/search?q=s%3A%2F%2Fwww.lcebook.com%2F protocol: tps: hostname: oogle.com pathname: /search search: ?q=s%3A%2F%2Fwww.lcebook.com%2F }

Fails here: <a href="tp://ttt.com">Test Url</a>

diracdeltas · 2018-03-16T15:43:06Z

could you please add a test plan with the expected behavior?

diracdeltas · 2018-03-16T22:25:04Z

app/common/urlParse.js

-  urlParse = require('url').parse
-}
-
+let urlParse = require('url').parse


there are some security issues with Node's default URL parser - can this behavior be fixed in Muon's url parser?

yes, working on that.

diracdeltas · 2018-03-19T23:11:07Z

app/filtering.js

@@ -511,7 +511,7 @@ function registerPermissionHandler (session, partition) {
            {text: locale.translation('deny')},
            {text: locale.translation('allow')}
          ],
-          frameOrigin: getOrigin(mainFrameUrl),
+          frameOrigin: getOrigin(origin),


i think you want mainFrameUrl here

The variable names need to be updated here.

In brave/muon#540. I updated the origin to be the security origin of the current frame, and the mainFrameUrl to the URL requesting the permission.

diracdeltas · 2018-03-20T22:03:14Z

@jumde previously, the openExternal permission (as I understood it), was granted to the main frame origin, not the origin that is being opened. Ex: if you go to brave.com and click the mailto: link at the bottom of the page, the expected behavior is that it prompts you whether to allow brave.com to open external URLs.

If we are going to change it to the behavior specified in your test plan, we need to also:

update the permission message to "Allow URL_TO_BE_OPENED to be opened in an external application?" instead of "Allow MAIN_FRAME_URL to open external applications"?
delete the existing openExternal permission settings, since those are no longer valid with this change

i agree that the behavior in this PR makes more sense than the existing behavior, but i also want to note that the scope of these changes is larger than #13471 (which can be fixed by just changing the displayed URL to be the external URL in the case where a main frame URL doesn't exist or if the request was initiated by directly entering a link in the URL bar).

could you open a new issue for the change in the openExternal permissions model?

diracdeltas · 2018-03-22T23:44:17Z

js/lib/urlutil.js

+      if (parsed.origin.length !== 0) {
+        return parsed.origin.replace(/\/+$/, '')
+      }
+      return parsed.href.replace(/\/+$/, '')


is this change still needed? i don't think this works on arbitrary URLs, ex:

muon.url.parse('sms://bing/kfdk').href.replace(/\/+$/, '') // returns "sms://bing/kfdk"

let's remove this diff if it's no longer needed

This is still needed, muon.url.parse for muon.url.parse('sms://bing/kfdk') returns NULL origin. The warning message if this change is not present, the warning message will read Allow Brave Browser to open an external application?

@jumde what if instead of this change, you just change https://github.com/brave/browser-laptop/pull/13475/files#diff-7b5b041c814805dd8496488bc6a53d3cR469 to parsed.origin != undefined so that the null origin will trigger https://github.com/brave/browser-laptop/pull/13475/files#diff-7b5b041c814805dd8496488bc6a53d3cR479 ?

also please add a test case in the getOrigin unit tests

Host is also null for arbitrary URLs:

Url: hps://nodejs.org/en/download/ { hash: '', hostname: '', host: '', href: 'hps://nodejs.org/en/download/', path: '//nodejs.org/en/download/', pathname: '//nodejs.org/en/download/', port: '', protocol: 'hps:', query: '', search: '', origin: '' }

@jumde i see. what if instead of changing the behavior of getOrigin, we just change https://github.com/brave/browser-laptop/pull/13475/files#diff-8a9bac13ef6264feb2b6da7c18d86b3cR500 to requestingOrigin || requestingUrl || 'Brave Browser' so it displays correctly?

i am reluctant to change getOrigin since it's used in various places for security checks

bsclifton · 2018-03-26T21:12:31Z

app/filtering.js

    }

    let response = []

-    if (origin == null) {
+    if (requestingUrl == null) {
      response = new Array(permissionTypes.length)


This looks to be the first real change (origin => requestingUrl)- everything else above this line is just renaming, is that correct?

bsclifton · 2018-03-26T21:31:50Z

app/filtering.js

@@ -497,7 +497,7 @@ function registerPermissionHandler (session, partition) {

      // Display 'Brave Browser' if the origin is null; ex: when a mailto: link
      // is opened in a new tab via right-click
-      const message = locale.translation('permissionMessage').replace(/{{\s*host\s*}}/, origin || 'Brave Browser').replace(/{{\s*permission\s*}}/, permissions[permission].action)
+      const message = locale.translation('permissionMessage').replace(/{{\s*host\s*}}/, getOrigin(requestingUrl) || 'Brave Browser').replace(/{{\s*permission\s*}}/, permissions[permission].action)


Do you have an example of where the setPermissionRequestHandler handler gets called from? I see the definition in Muon, but am not sure when the handler is called?

UPDATE: found it-
PlatformNotificationServiceImpl::DisplayNotification

I wanted to find an example of the args passed in

Looking at the code in Muon...
mainFrameOrigin maps to current_origin
requestingUrl maps to requesting_origin

Super glad you fixed this mismatch issue 😄

bsclifton

Changes look great! 😄 The problem pops out at you once the variables are renamed. Great job 😄 👍

diracdeltas · 2018-03-26T22:24:49Z

app/filtering.js

@@ -530,7 +530,7 @@ function registerPermissionHandler (session, partition) {
          response[index] = result
          if (persist) {
            // remember site setting for this host
-            appActions.changeSiteSetting(origin, permission + 'Permission', result, isPrivate)
+            appActions.changeSiteSetting(requestingUrl, permission + 'Permission', result, isPrivate)


I'm confused why we are getting the permissions for the main frame origin in https://github.com/brave/browser-laptop/pull/13475/files#diff-8a9bac13ef6264feb2b6da7c18d86b3cR452, but here we apply permissions by requestingUrl instead of mainFrameOrigin. seems like these two lines should either both use requestingUrl or both use mainFrameOrigin

Good catch! fixed

see https://github.com/brave/browser-laptop/pull/13475/files#r177257591 - this should apply the change by requesting origin, not full URL

diracdeltas · 2018-03-26T22:27:03Z

app/filtering.js

-      settings = siteSettings.getSiteSettingsForHostPattern(appState.get('siteSettings'), origin)
-      tempSettings = siteSettings.getSiteSettingsForHostPattern(appState.get('temporarySiteSettings'), origin)
-    } else if (mainFrameUrl.startsWith('magnet:')) {
+      settings = siteSettings.getSiteSettingsForHostPattern(appState.get('siteSettings'), mainFrameOrigin)


are you sure this and the other getSiteSettingsForHostPattern lines aren't supposed to use getOrigin(requestingUrl) instead of mainFrameOrigin since that's what is actually shown in the message?

https://github.com/brave/browser-laptop/pull/13475/files#diff-8a9bac13ef6264feb2b6da7c18d86b3cR500

@bsclifton

… permissions are requested Auditors - @bsclifton, @diracdeltas

diracdeltas · 2018-03-26T22:42:54Z

app/filtering.js

-      tempSettings = siteSettings.getSiteSettingsForHostPattern(appState.get('temporarySiteSettings'), origin)
-    } else if (mainFrameUrl.startsWith('magnet:')) {
+      settings = siteSettings.getSiteSettingsForHostPattern(appState.get('siteSettings'), requestingUrl)
+      tempSettings = siteSettings.getSiteSettingsForHostPattern(appState.get('temporarySiteSettings'), requestingUrl)


you have to do requestingUrl = getOrigin(requestingUrl) before calling these settings since these were previously saved by origin not full URL

or better yet, define const requestingOrigin = getOrigin(requestingUrl) in the function scope so that it's harder to confuse URL with origin

diracdeltas · 2018-03-26T22:44:23Z

app/filtering.js

-      origin = getOrigin(origin)
-      settings = siteSettings.getSiteSettingsForURL(appState.get('siteSettings'), origin)
-      tempSettings = siteSettings.getSiteSettingsForURL(appState.get('temporarySiteSettings'), origin)
+      requestingUrl = getOrigin(requestingUrl)


see https://github.com/brave/browser-laptop/pull/13475/files#r177257591

diracdeltas · 2018-03-26T22:44:48Z

app/filtering.js

@@ -497,7 +497,7 @@ function registerPermissionHandler (session, partition) {

      // Display 'Brave Browser' if the origin is null; ex: when a mailto: link
      // is opened in a new tab via right-click
-      const message = locale.translation('permissionMessage').replace(/{{\s*host\s*}}/, origin || 'Brave Browser').replace(/{{\s*permission\s*}}/, permissions[permission].action)
+      const message = locale.translation('permissionMessage').replace(/{{\s*host\s*}}/, getOrigin(requestingUrl) || 'Brave Browser').replace(/{{\s*permission\s*}}/, permissions[permission].action)


can also be simplified using https://github.com/brave/browser-laptop/pull/13475/files#r177257591

diracdeltas · 2018-03-27T00:12:20Z

app/filtering.js

-    if (mainFrameUrl === appUrlUtil.getBraveExtIndexHTML() || isPDFOrigin || isBraveOrigin) {
+    let requestingOrigin = getOrigin(requestingUrl)
+
+    if (requestingOrigin === appUrlUtil.getBraveExtIndexHTML() || isPDFOrigin || isBraveOrigin) {


you want requestingUrl in this line i think

diracdeltas · 2018-03-27T01:56:45Z

@jumde thanks for making the changes. PR looks good to me now :). i will test it once the muon build is out.

diracdeltas · 2018-03-27T19:47:29Z

app/filtering.js

@@ -471,7 +471,7 @@ function registerPermissionHandler (session, partition) {
        response.push(false)
      } else if (permission === 'fullscreen' &&
        // The Torrent Viewer extension is always allowed to show fullscreen media
-        origin.startsWith('chrome-extension://' + config.torrentExtensionId)) {
+        mainFrameOrigin.startsWith('chrome-extension://' + config.torrentExtensionId)) {


this throws an error when you go to http://www.orimi.com/pdf-test.pdf and click the fullscreen button

should be requestingOrigin instead of mainFrameOrigin i think

actually it shouldn't - https://github.com/brave/browser-laptop/pull/13475/files#diff-8a9bac13ef6264feb2b6da7c18d86b3cR440 set the mainFrameOrigin to null deliberately

so i think this just has to check if mainFrameOrigin is null

diracdeltas · 2018-03-27T19:55:03Z

app/filtering.js

-    } else if (mainFrameUrl.startsWith('magnet:')) {
+      settings = siteSettings.getSiteSettingsForHostPattern(appState.get('siteSettings'), requestingOrigin)
+      tempSettings = siteSettings.getSiteSettingsForHostPattern(appState.get('temporarySiteSettings'), requestingOrigin)
+    } else if (requestingOrigin.startsWith('magnet:')) {


this if block` is actually not necessary anymore since it will show the magnet URL itself

bsclifton

Test plan works! 👍

diracdeltas

lgtm

bsclifton · 2018-03-27T21:28:50Z

I am an idiot and just merged again without squashing 😛

Permission host mismatch

bsclifton · 2018-03-27T21:37:25Z

master 5694502
0.23.x ac2b5fd
0.22.x cd7a878

jumde added the PR/work-in-progress ⚒ label Mar 16, 2018

diracdeltas reviewed Mar 16, 2018

View reviewed changes

jumde self-assigned this Mar 16, 2018

jumde requested a review from bsclifton March 16, 2018 18:00

diracdeltas reviewed Mar 16, 2018

View reviewed changes

jumde force-pushed the permissions_host branch from 1979990 to aa4c5fb Compare March 19, 2018 22:48

jumde mentioned this pull request Mar 19, 2018

Incorrect hosts were granted OpenExternal permissions brave/muon#540

Merged

jumde removed the PR/work-in-progress ⚒ label Mar 19, 2018

diracdeltas reviewed Mar 19, 2018

View reviewed changes

jumde mentioned this pull request Mar 22, 2018

Display correct host when requesting external app permisssion #13554

Closed

diracdeltas added this to the 0.22.x (Beta Channel) milestone Mar 22, 2018

diracdeltas reviewed Mar 22, 2018

View reviewed changes

bsclifton reviewed Mar 26, 2018

View reviewed changes

bsclifton previously approved these changes Mar 26, 2018

View reviewed changes

diracdeltas reviewed Mar 26, 2018

View reviewed changes

jumde dismissed bsclifton’s stale review via 87d6877 March 26, 2018 22:35

Fixes #13471 - Permissions should show the correct host for which the…

4dbc4fd

… permissions are requested Auditors - @bsclifton, @diracdeltas

jumde force-pushed the permissions_host branch from 87d6877 to 4dbc4fd Compare March 26, 2018 22:41

diracdeltas reviewed Mar 26, 2018

View reviewed changes

fix

183ff2b

diracdeltas reviewed Mar 27, 2018

View reviewed changes

jumde added 2 commits March 26, 2018 17:18

fix

63d27c8

fix

ab4fbc1

kjozwiak mentioned this pull request Mar 27, 2018

brave doesn't check URL protocol and requests external application permissions #13471

Closed

diracdeltas reviewed Mar 27, 2018

View reviewed changes

update

bc39599

bsclifton approved these changes Mar 27, 2018

View reviewed changes

diracdeltas approved these changes Mar 27, 2018

View reviewed changes

bsclifton merged commit 5694502 into master Mar 27, 2018

bsclifton deleted the permissions_host branch March 27, 2018 21:28

bsclifton added a commit that referenced this pull request Mar 27, 2018

Merge pull request #13475 from brave/permissions_host

ac2b5fd

Permission host mismatch

bsclifton added a commit that referenced this pull request Mar 27, 2018

Merge pull request #13475 from brave/permissions_host

cd7a878

Permission host mismatch

Permission host mismatch #13475

Permission host mismatch #13475

Conversation

jumde commented Mar 16, 2018 • edited Loading

Test plan

codecov-io commented Mar 16, 2018 • edited Loading

Codecov Report

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

diracdeltas commented Mar 16, 2018

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

diracdeltas commented Mar 20, 2018 • edited Loading

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

jumde Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

diracdeltas Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

bsclifton Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

bsclifton Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

bsclifton 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

diracdeltas Mar 26, 2018 • edited Loading

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

Choose a reason for hiding this comment

diracdeltas commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

diracdeltas left a comment

Choose a reason for hiding this comment

bsclifton commented Mar 27, 2018

bsclifton commented Mar 27, 2018

jumde commented Mar 16, 2018 •

edited

Loading

codecov-io commented Mar 16, 2018 •

edited

Loading

diracdeltas commented Mar 20, 2018 •

edited

Loading

jumde Mar 27, 2018 •

edited

Loading

diracdeltas Mar 27, 2018 •

edited

Loading

bsclifton Mar 26, 2018 •

edited

Loading

bsclifton Mar 26, 2018 •

edited

Loading

bsclifton Mar 26, 2018 •

edited

Loading

diracdeltas Mar 26, 2018 •

edited

Loading