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

Permission host mismatch #13475

Merged
merged 5 commits into from
Mar 27, 2018
Merged

Permission host mismatch #13475

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4dbc4fd
Fixes #13471 - Permissions should show the correct host for which the…
jumde Mar 19, 2018
183ff2b
fix
jumde Mar 26, 2018
63d27c8
fix
jumde Mar 27, 2018
ab4fbc1
fix
jumde Mar 27, 2018
bc39599
update
jumde Mar 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions app/filtering.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ function registerPermissionHandler (session, partition) {
const isPrivate = module.exports.isPrivate(partition)
// Keep track of per-site permissions granted for this session.
let permissions = null
session.setPermissionRequestHandler((origin, mainFrameUrl, permissionTypes, muonCb) => {
session.setPermissionRequestHandler((mainFrameOrigin, requestingUrl, permissionTypes, muonCb) => {
if (!permissions) {
permissions = {
media: {
Expand Down Expand Up @@ -427,35 +427,35 @@ function registerPermissionHandler (session, partition) {
// TODO(bridiver) - the permission handling should be converted to an action because we should never call `appStore.getState()`
// Check whether there is a persistent site setting for this host
const appState = appStore.getState()
const isBraveOrigin = origin.startsWith(`chrome-extension://${config.braveExtensionId}/`)
const isPDFOrigin = origin.startsWith(`${pdfjsOrigin}/`)
const isBraveOrigin = mainFrameOrigin.startsWith(`chrome-extension://${config.braveExtensionId}/`)
const isPDFOrigin = mainFrameOrigin.startsWith(`${pdfjsOrigin}/`)
let settings
let tempSettings
if (mainFrameUrl === appUrlUtil.getBraveExtIndexHTML() || isPDFOrigin || isBraveOrigin) {
if (requestingUrl === appUrlUtil.getBraveExtIndexHTML() || isPDFOrigin || isBraveOrigin) {
// lookup, display and store site settings by the origin alias
origin = isPDFOrigin ? 'PDF Viewer' : 'Brave Browser'
requestingUrl = isPDFOrigin ? 'PDF Viewer' : 'Brave Browser'
// display on all tabs
mainFrameUrl = null
mainFrameOrigin = null
// Lookup by exact host pattern match since 'Brave Browser' is not
// a parseable URL
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'), requestingUrl)
tempSettings = siteSettings.getSiteSettingsForHostPattern(appState.get('temporarySiteSettings'), requestingUrl)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

} else if (requestingUrl.startsWith('magnet:')) {
// Show "Allow magnet URL to open an external application?", instead of
// "Allow null to open an external application?"
// This covers an edge case where you open a magnet link tab, then disable Torrent Viewer
// and restart Brave. I don't think it needs localization. See 'Brave Browser' above.
origin = 'Magnet URL'
requestingUrl = 'Magnet URL'
} else {
// Strip trailing slash
origin = getOrigin(origin)
settings = siteSettings.getSiteSettingsForURL(appState.get('siteSettings'), origin)
tempSettings = siteSettings.getSiteSettingsForURL(appState.get('temporarySiteSettings'), origin)
requestingUrl = getOrigin(requestingUrl)
Copy link
Member

Choose a reason for hiding this comment

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

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

settings = siteSettings.getSiteSettingsForURL(appState.get('siteSettings'), requestingUrl)
tempSettings = siteSettings.getSiteSettingsForURL(appState.get('temporarySiteSettings'), requestingUrl)
}

let response = []

if (origin == null) {
if (requestingUrl == null) {
response = new Array(permissionTypes.length)
Copy link
Member

@bsclifton bsclifton Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

response.fill(false, 0, permissionTypes.length)
muonCb(response)
Expand All @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

response.push(true)
} else if (permission === 'fullscreen' && alwaysAllowFullscreen) {
// Always allow fullscreen if setting is ON
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

@bsclifton bsclifton Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

UPDATE: found it-
PlatformNotificationServiceImpl::DisplayNotification

I wanted to find an example of the args passed in

Copy link
Member

@bsclifton bsclifton Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Super glad you fixed this mismatch issue 😄

Copy link
Member

Choose a reason for hiding this comment

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

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


// If this is a duplicate, clear the previous callback and use the new one
if (permissionCallbacks[message]) {
Expand All @@ -511,9 +511,9 @@ function registerPermissionHandler (session, partition) {
{text: locale.translation('deny')},
{text: locale.translation('allow')}
],
frameOrigin: getOrigin(mainFrameUrl),
frameOrigin: getOrigin(mainFrameOrigin),
options: {
persist: !!origin,
persist: !!requestingUrl,
index: i
},
message
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! fixed

Copy link
Member

Choose a reason for hiding this comment

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

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

}
if (response.length === permissionTypes.length) {
permissionCallbacks[message] = null
Expand Down
5 changes: 4 additions & 1 deletion js/lib/urlutil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,10 @@ const UrlUtil = {
if (parsed.protocol === 'about:') {
return [parsed.protocol, parsed.path].join('')
}
return parsed.origin.replace(/\/+$/, '')
if (parsed.origin.length !== 0) {
return parsed.origin.replace(/\/+$/, '')
}
return parsed.href.replace(/\/+$/, '')
Copy link
Member

Choose a reason for hiding this comment

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

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"

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

@jumde jumde Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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: '' }

Copy link
Member

@diracdeltas diracdeltas Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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

}
if (parsed.host && parsed.protocol) {
return parsed.slashes ? [parsed.protocol, parsed.host].join('//') : [parsed.protocol, parsed.host].join('')
Expand Down