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

extension icons should appear #11143

Merged
merged 1 commit into from
Dec 22, 2017
Merged

extension icons should appear #11143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions app/extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const contextMenus = require('./browser/extensions/contextMenus')
const extensionActions = require('./common/actions/extensionActions')
const config = require('../js/constants/config')
const appConfig = require('../js/constants/appConfig')
const {fileUrl} = require('../js/lib/appUrlUtil')
const {chromeUrl} = require('../js/lib/appUrlUtil')
const {getExtensionsPath, getBraveExtUrl, getBraveExtIndexHTML} = require('../js/lib/appUrlUtil')
const {getSetting} = require('../js/settings')
const settings = require('../js/constants/settings')
Expand Down Expand Up @@ -407,7 +407,9 @@ module.exports.init = () => {
extensionInfo.setState(installInfo.id, extensionStates.ENABLED)
extensionInfo.setInstallInfo(installInfo.id, installInfo)
installInfo.filePath = installInfo.base_path
installInfo.base_path = fileUrl(installInfo.base_path)

installInfo.base_path = chromeUrl(installInfo.base_path)

extensionActions.extensionInstalled(installInfo.id, installInfo)
extensionActions.extensionEnabled(installInfo.id)
})
Expand Down
7 changes: 7 additions & 0 deletions js/lib/appUrlUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ module.exports.fileUrl = (filePath) => {
return encodeURI('file://' + fileUrlPath)
}

module.exports.chromeUrl = (filePath = '') => {
Copy link
Member

Choose a reason for hiding this comment

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

would be good to have some unit tests for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 1bc13ba

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use a relative path instead and make it filesystem agnostic?

filePath = module.exports.fileUrl(filePath)
filePath = filePath.replace('file://', 'chrome://brave')
Copy link
Member

Choose a reason for hiding this comment

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

where is the chrome://brave to file:// mapping implemented in muon? it seems like a potential security hole since it can be used to bypass CSP restrictions; we should make sure chrome://brave loads are blocked in untrusted contexts.

Copy link
Member

Choose a reason for hiding this comment

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

also i'm curious what CSP violations are being triggered? app/extensions/brave/index.html explicitly allows img-src file: but not chrome: so i'm confused why the original issue happens

Copy link
Contributor Author

@kevinlawler kevinlawler Sep 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I played with the CSP file but couldn't get the violation to reveal itself either. It might be some other security restriction. The error from the console is

[75566:775:0922/145254.257520:ERROR:CONSOLE(0)] "Not allowed to load local resource:
    file:///Users/user/Desktop/brave/browser-laptop-bootstrap/src/browser-laptop/app/extensions/metalmash/images/icon-19.png", 
    source: chrome://brave/Users/user/Desktop/brave/browser-laptop-bootstrap/src/browser-laptop/app/extensions/brave/index-dev.html (0)

and it disappears when switching to chrome://brave. Subsequently the image loads.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@diracdeltas chrome://brave can't be loaded from web pages Not allowed to load local resource: chrome://brave/usr/local/brave/browser-laptop-bootstrap/README.md. When I originally tested them it also wasn't possible to load them by entering the url manually, but I just tested again and it is now possible because we added support for chrome://* urls in general so we should probably block them from the urlbar

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, the csp can't allow something that is prohibited by default (access to file resources) which is why the file:// urls don't work

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine if chrome://brave URLs can be loaded manually from the urlbar since that is also possible for file URLs. just want to make sure there is nothing they can do that files URLs can't do, other than be loaded in the chrome:// context.


return filePath
}

/**
* Gets the URL of a page hosted by the braveExtension or torrentExtension
* Returns 'chrome-extension://<...>'
Expand Down
8 changes: 8 additions & 0 deletions test/unit/lib/appUrlUtilTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,14 @@ describe('appUrlUtil test', function () {
assert.equal(fileUrl, expected)
})
})
describe('chromeUrl', function () {
it('can convert file paths', function () {
const filePath = '/users/bbondy/space here/tesT.html'
const chromeUrl = appUrlUtil.chromeUrl(filePath)
const expected = 'chrome://brave/users/bbondy/space%20here/tesT.html'
assert.equal(chromeUrl, expected)
})
})
describe('newFrameUrl', function () {
describe('when NEWTAB_MODE = HOMEPAGE', function () {
it('returns the configured home page', function () {
Expand Down