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

Re-block running insecure content #3808

Merged
merged 4 commits into from
Sep 9, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 4 additions & 1 deletion app/extensions/brave/locales/en-US/app.properties
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,9 @@ phone=Phone
email=Email
editAddress=Edit Address
editCreditCard=Edit Credit Card
denyRunInsecureContent=Stay Secure
dismissAllowRunInsecureContent=Stay Secure
allowRunInsecureContent=Load Unsafe Scripts
dismissDenyRunInsecureContent=Stay Unsecure
Copy link
Member

Choose a reason for hiding this comment

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

s/Unsecure/Insecure

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 138692c. Thanks

denyRunInsecureContent=Stop Loading Unsafe Scripts
runInsecureContentWarning=This page is trying to load scripts from insecure sources. If you allow this content to run it will not be encrypted and it may transmit unencrypted data to other sites.
denyRunInsecureContentWarning=This page is currently loading scripts from insecure sources. If you disallow this content to run it, transmitting data will be more safe with encrpyted connections.
Copy link
Member

Choose a reason for hiding this comment

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

second sentence is unnecessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 138692c. Thanks

5 changes: 3 additions & 2 deletions docs/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ AppStore
httpsEverywhere: boolean,
fingerprintingProtection: boolean,
flash: (number|boolean), // approval expiration time if allowed, false if never allow
ledgerPayments: boolean // False if site should not be paid by the ledger. Defaults to true.
ledgerPayments: boolean, // False if site should not be paid by the ledger. Defaults to true.
runInsecureContent: boolean // Allow active mixed content
}
},
temporarySiteSettings: {
Expand Down Expand Up @@ -272,7 +273,7 @@ WindowStore
realm: string
},
isExtendedValidation: boolean, // is using https ev
runInsecureContent: boolean, // has active mixed content
runInsecureContent: string, // first domain of running active mixed content
blockedRunInsecureContent: string, // first domain of blocked active mixed content
},
parentFrameKey: number, // the key of the frame this frame was opened from
Expand Down
4 changes: 3 additions & 1 deletion js/components/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ class Frame extends ImmutableComponent {

componentWillUnmount () {
this.expireFlash(this.origin)
// Delete runInsecureContent when the webview is closed
appActions.removeSiteSetting(this.origin, 'runInsecureContent')
}

updateWebview (cb, newSrc) {
Expand Down Expand Up @@ -764,7 +766,7 @@ class Frame extends ImmutableComponent {
const runInsecureContent = parsedUrl.protocol === 'https:' && this.runInsecureContent()
windowActions.setSecurityState(this.frame, {
secure: isSecure,
runInsecureContent: runInsecureContent
runInsecureContent: runInsecureContent ? this.props.location : null
Copy link
Member

Choose a reason for hiding this comment

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

why does location need to be in the securityState? SiteInfo can just do frameProps.get('location')

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds better. I will changed to use frameProps.get('location')

})
if (isSecure) {
// Check that there isn't a cert error.
Expand Down
25 changes: 21 additions & 4 deletions js/components/siteInfo.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,18 @@ class SiteInfo extends ImmutableComponent {
constructor () {
super()
this.onAllowRunInsecureContent = this.onAllowRunInsecureContent.bind(this)
this.onDenyRunInsecureContent = this.onDenyRunInsecureContent.bind(this)
}
onAllowRunInsecureContent () {
appActions.changeSiteSetting(siteUtil.getOrigin(this.isBlockedRunInsecureContent), 'runInsecureContent', true)
ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, {}, this.isBlockedRunInsecureContent)
Copy link
Member

Choose a reason for hiding this comment

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

the code above can also use this.location instead of this.isBlockedRunInsecureContent

Copy link
Member

Choose a reason for hiding this comment

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

but that is more of a refactor issue so this looks good to merge now. :)

this.props.onHide()
}
onDenyRunInsecureContent () {
appActions.removeSiteSetting(siteUtil.getOrigin(this.runInsecureContent), 'runInsecureContent')
Copy link
Member

Choose a reason for hiding this comment

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

should this have this.isPrivate as the last argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, you are right. I forgot to add it.

ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, {}, this.runInsecureContent)
this.props.onHide()
}
get isExtendedValidation () {
return this.props.frameProps.getIn(['security', 'isExtendedValidation'])
}
Expand Down Expand Up @@ -63,15 +69,26 @@ class SiteInfo extends ImmutableComponent {
<span data-l10n-args={JSON.stringify(l10nArgs)} data-l10n-id='sessionInfo' /></li>
}

let runInsecureContentWarning = null
let runInsecureContentInfo = null
if (this.isBlockedRunInsecureContent) {
runInsecureContentWarning =
runInsecureContentInfo =
<li>
<ul>
<li><span className='runInsecureContentWarning' data-l10n-id='runInsecureContentWarning' /></li>
<li>
<Button l10nId='allowRunInsecureContent' className='secondaryAltButton allowRunInsecureContentButton' onClick={this.onAllowRunInsecureContent} />
<Button l10nId='denyRunInsecureContent' className='primaryButton denyRunInsecureContentButton' onClick={this.props.onHide} />
<Button l10nId='dismissAllowRunInsecureContent' className='primaryButton dismissAllowRunInsecureContentButton' onClick={this.props.onHide} />
</li>
</ul>
</li>
} else if (this.runInsecureContent) {
runInsecureContentInfo =
<li>
<ul>
<li><span className='denyRunInsecureContentWarning' data-l10n-id='denyRunInsecureContentWarning' /></li>
<li>
<Button l10nId='denyRunInsecureContent' className='primaryButton denyRunInsecureContentButton' onClick={this.onDenyRunInsecureContent} />
<Button l10nId='dismissDenyRunInsecureContent' className='secondaryAltButton dismissDenyRunInsecureContentButton' onClick={this.props.onHide} />
</li>
</ul>
</li>
Expand All @@ -86,7 +103,7 @@ class SiteInfo extends ImmutableComponent {
partitionInfo
}
{
runInsecureContentWarning
runInsecureContentInfo
}
</ul>
</Dialog>
Expand Down
76 changes: 69 additions & 7 deletions test/components/navigationBarTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ const Brave = require('../lib/brave')
const config = require('../../js/constants/config')
const {urlBarSuggestions, urlInput, activeWebview, activeTabFavicon, activeTab, navigatorLoadTime,
navigator, titleBar, urlbarIcon, bookmarksToolbar, navigatorNotBookmarked, navigatorBookmarked,
saveButton, allowRunInsecureContentButton, denyRunInsecureContentButton} = require('../lib/selectors')
saveButton, allowRunInsecureContentButton, dismissAllowRunInsecureContentButton,
denyRunInsecureContentButton, dismissDenyRunInsecureContentButton} = require('../lib/selectors')
const urlParse = require('url').parse
const assert = require('assert')
const settings = require('../../js/constants/settings')
const searchProviders = require('../../js/data/searchProviders')
const messages = require('../../js/constants/messages')

describe('navigationBar', function () {
function * setup (client) {
Expand Down Expand Up @@ -283,10 +285,10 @@ describe('navigationBar', function () {
.waitForExist(urlbarIcon + '.fa-lock')
.click(urlbarIcon)
.waitForVisible('.runInsecureContentWarning')
.waitForVisible(denyRunInsecureContentButton)
.waitForVisible(dismissAllowRunInsecureContentButton)
.waitForVisible(allowRunInsecureContentButton)
.waitForVisible('[data-l10n-id="secureConnection"]')
.click(denyRunInsecureContentButton)
.click(dismissAllowRunInsecureContentButton)
// TODO(bridiver) there is a race condition here because we are waiting for a non-change
// and we need some way to verify that the page does not reload and allow insecure content
.tabByUrl(page1Url).waitUntil(() => {
Expand All @@ -298,7 +300,7 @@ describe('navigationBar', function () {
.click(urlbarIcon)
.waitForExist(urlbarIcon + '.fa-lock')
})
it('Temporarily allow running insecure content', function * () {
it('Temporarily allow/deny running insecure content', function * () {
const page1Url = 'https://mixed-script.badssl.com/'
yield this.app.client.tabByUrl(Brave.newTabUrl)
.loadUrl(page1Url)
Expand All @@ -311,8 +313,9 @@ describe('navigationBar', function () {
.windowByUrl(Brave.browserWindowUrl)
.waitForExist(urlbarIcon + '.fa-lock')
.click(urlbarIcon)
.waitForVisible('[data-l10n-id="secureConnection"]')
.waitForVisible('.runInsecureContentWarning')
.waitForVisible(denyRunInsecureContentButton)
.waitForVisible(dismissAllowRunInsecureContentButton)
.waitForVisible(allowRunInsecureContentButton)
.click(allowRunInsecureContentButton)
.tabByUrl(this.page1Url)
Expand All @@ -322,10 +325,69 @@ describe('navigationBar', function () {
)
})
.windowByUrl(Brave.browserWindowUrl)
.waitForExist(urlbarIcon)
.click(urlbarIcon)
.waitForExist(urlbarIcon + '.fa-unlock')
.waitForVisible('[data-l10n-id="mixedConnection"]')
.keys('\uE00C')
.waitForVisible('.denyRunInsecureContentWarning')
.waitForVisible(dismissDenyRunInsecureContentButton)
.waitForVisible(denyRunInsecureContentButton)
.click(denyRunInsecureContentButton)
.tabByUrl(this.page1Url)
.waitUntil(() => {
return this.app.client.getCssProperty('body', 'background-color').then((color) =>
color.value === 'rgba(128,128,128,1)'
)
})
.windowByUrl(Brave.browserWindowUrl)
.click(urlbarIcon)
.waitForExist(urlbarIcon + '.fa-lock')
})
it('Clear running insecure content on webview close', function * () {
const page1Url = 'https://mixed-script.badssl.com/'
const page2Url = Brave.server.url('page2.html')
yield this.app.client.tabByUrl(Brave.newTabUrl)
.loadUrl(page1Url)
// background color changes when insecure content runs
.waitUntil(() => {
return this.app.client.getCssProperty('body', 'background-color').then((color) =>
color.value === 'rgba(128,128,128,1)'
)
})
.windowByUrl(Brave.browserWindowUrl)
.waitForExist(urlbarIcon + '.fa-lock')
.click(urlbarIcon)
.waitForVisible('[data-l10n-id="secureConnection"]')
.waitForVisible('.runInsecureContentWarning')
.waitForVisible(dismissAllowRunInsecureContentButton)
.waitForVisible(allowRunInsecureContentButton)
.click(allowRunInsecureContentButton)
.tabByUrl(this.page1Url)
.waitUntil(() => {
return this.app.client.getCssProperty('body', 'background-color').then((color) =>
color.value === 'rgba(255,0,0,1)'
)
})
.windowByUrl(Brave.browserWindowUrl)
.ipcSend(messages.SHORTCUT_NEW_FRAME, page2Url, { openInForeground: false })
.waitUntil(function () {
return this.getWindowState().then((val) => {
return val.value.frames.length === 2
})
})
.ipcSend(messages.SHORTCUT_CLOSE_FRAME)
.waitUntil(function () {
return this.getWindowState().then((val) => {
return val.value.frames.length === 1
})
})
.windowByUrl(Brave.browserWindowUrl)
.tabByIndex(0)
.loadUrl(page1Url)
.waitUntil(() => {
return this.app.client.getCssProperty('body', 'background-color').then((color) =>
color.value === 'rgba(128,128,128,1)'
)
})
})
})

Expand Down
4 changes: 3 additions & 1 deletion test/lib/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,7 @@ module.exports = {
autofillAddressPanel: '.autofillAddressPanel',
autofillCreditCardPanel: '.autofillCreditCardPanel',
allowRunInsecureContentButton: '.allowRunInsecureContentButton',
denyRunInsecureContentButton: '.denyRunInsecureContentButton'
dismissAllowRunInsecureContentButton: '.dismissAllowRunInsecureContentButton',
denyRunInsecureContentButton: '.denyRunInsecureContentButton',
dismissDenyRunInsecureContentButton: '.dismissDenyRunInsecureContentButton'
}