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

Commit

Permalink
Various improvements to urlbar icon UX
Browse files Browse the repository at this point in the history
Fix #3651 (display insecure icon in titlemode)
Fix #5695 (expand siteInfo explanations, change insecure to red unlocked icon)
Also addresses edge case where a site is HTTPS-but-insecure, for instance due to having a SHA1 certificate, and tries to load mixed content. Ex: very.badssl.com. In this case, the siteInfo should not say that the site is partially secure.

Auditors: @bradleyrichter @darkdh

Test Plan:
Covered by automated tests
  • Loading branch information
diracdeltas committed Nov 17, 2016
1 parent 18ab968 commit c16baaa
Show file tree
Hide file tree
Showing 8 changed files with 78 additions and 20 deletions.
8 changes: 5 additions & 3 deletions app/extensions/brave/locales/en-US/app.properties
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,10 @@ apply=Apply
applyAndRestart=Apply and restart
fullscreenNotice={{origin}} is now fullscreen. Press ESC to exit.
secureConnection=Secure connection
secureConnectionInfo=This page was loaded securely over HTTPS.
mixedConnection=Partially insecure connection
insecureConnection=Using an insecure connection
insecureConnectionInfo=Your connection to this site is not private. An eavesdropper may be able to tamper with this page and read your data.
blockedTrackingElements={{blockedTrackingElementsSize}} Blocked tracking elements
replacedAds={{replacedAdsSize}} Ads replaced
blockedAds={{blockedAdsSize}} Ads blocked
Expand Down Expand Up @@ -207,12 +209,12 @@ phone=Phone
email=Email
editAddress=Edit Address
editCreditCard=Edit Credit Card
dismissAllowRunInsecureContent=Stay Secure
dismissAllowRunInsecureContent=Block Unsafe Scripts
allowRunInsecureContent=Load Unsafe Scripts
dismissDenyRunInsecureContent=Stay Insecure
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.
runInsecureContentWarning=This page is trying to load scripts from insecure sources. If you allow this content to run, it may transmit unencrypted data to other sites.
denyRunInsecureContentWarning=Your connection is not private. This page is currently loading scripts from insecure sources.
windowCaptionButtonMinimize=Minimize
windowCaptionButtonMaximize=Maximize
windowCaptionButtonRestore=Restore Down
Expand Down
7 changes: 3 additions & 4 deletions app/renderer/components/urlBarIcon.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ class UrlBarIcon extends ImmutableComponent {
get isInsecure () {
return this.props.isHTTPPage &&
this.props.isSecure === false &&
!this.props.active &&
!this.props.titleMode
!this.props.active
}
/**
* search icon:
Expand Down Expand Up @@ -66,9 +65,9 @@ class UrlBarIcon extends ImmutableComponent {
'fa': true,
// NOTE: EV style not approved yet; see discussion at https://github.com/brave/browser-laptop/issues/791
'fa-lock': this.isSecure,
'fa-exclamation-triangle': this.isInsecure,
'fa-unlock': this.isInsecure,
'fa-search': this.isSearch,
'fa-list': this.isAboutPage
'fa-list': this.isAboutPage && !this.props.titleMode
})
}
get iconStyles () {
Expand Down
20 changes: 13 additions & 7 deletions js/components/siteInfo.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,10 @@ class SiteInfo extends ImmutableComponent {
'fa-lock': true,
extendedValidation: this.isExtendedValidation
})} /><span data-l10n-id='secureConnection' /></li>
} else if (this.runInsecureContent) {
secureIcon = <li><span className='fa fa-exclamation-triangle' /><span data-l10n-id='mixedConnection' /></li>
} else if (this.isSecure && this.runInsecureContent) {
secureIcon = <li><span className='fa fa-unlock' /><span data-l10n-id='mixedConnection' /></li>
} else {
secureIcon = <li><span className='fa fa-exclamation-triangle' /><span data-l10n-id='insecureConnection' data-l10n-args={JSON.stringify(l10nArgs)} /></li>
secureIcon = <li><span className='fa fa-unlock' /><span data-l10n-id='insecureConnection' data-l10n-args={JSON.stringify(l10nArgs)} /></li>

This comment has been minimized.

Copy link
@bbondy

bbondy Jan 8, 2017

Member

unsecure sites fail here because l10nArgs isn't defined yet, I'll fix just an FYI.

This comment has been minimized.

Copy link
@bbondy

bbondy Jan 8, 2017

Member

It doesn't seem like it's caused here actually, so I guess it happened before but not sure why we didn't see it until recently.

This comment has been minimized.

Copy link
@bbondy

bbondy Jan 9, 2017

Member

Wow this is actually because we were transpiling let as var before, and then with this commit we stopped doing that:
19b5f7c

So the transpiling was hiding the bug

}

// Figure out the partition info display
Expand All @@ -77,9 +77,9 @@ class SiteInfo extends ImmutableComponent {
<span data-l10n-args={JSON.stringify(l10nArgs)} data-l10n-id='sessionInfo' /></li>
}

let runInsecureContentInfo = null
let connectionInfo = null
if (this.isBlockedRunInsecureContent) {
runInsecureContentInfo =
connectionInfo =
<li>
<ul>
<li><span className='runInsecureContentWarning' data-l10n-id='runInsecureContentWarning' /></li>
Expand All @@ -90,7 +90,7 @@ class SiteInfo extends ImmutableComponent {
</ul>
</li>
} else if (this.runInsecureContent) {
runInsecureContentInfo =
connectionInfo =
<li>
<ul>
<li><span className='denyRunInsecureContentWarning' data-l10n-id='denyRunInsecureContentWarning' /></li>
Expand All @@ -100,6 +100,12 @@ class SiteInfo extends ImmutableComponent {
</li>
</ul>
</li>
} else if (this.isSecure) {
connectionInfo =
<div className='connectionInfo' data-l10n-id='secureConnectionInfo' />
} else {
connectionInfo =
<div className='connectionInfo' data-l10n-id='insecureConnectionInfo' />
}

return <Dialog onHide={this.props.onHide} className='siteInfo' isClickDismiss>
Expand All @@ -111,7 +117,7 @@ class SiteInfo extends ImmutableComponent {
partitionInfo
}
{
runInsecureContentInfo
connectionInfo
}
</ul>
</Dialog>
Expand Down
4 changes: 4 additions & 0 deletions less/dialogs.less
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,7 @@
}
}
}
.connectionInfo {
max-width: 400px;
padding: 10px;
}
3 changes: 1 addition & 2 deletions less/forms.less
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,11 @@
}

ul {
font-size: smaller;
max-height: 300px;
overflow-y: auto;
margin-top: -20px;
padding: 0 10px;
max-width: 350px;
max-width: 400px;
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions less/navigationBar.less
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,10 @@
color: @chromeText;
min-width: 14px;
}
&.fa-unlock {
color: @siteInsecureColor;
opacity: 1.0;
}
}

.bookmarkButton {
Expand Down Expand Up @@ -898,14 +902,14 @@
min-width: 16px;

&.fa-lock,
&.fa-exclamation-triangle {
&.fa-unlock {
margin-top: 1px;
font-size: 16px;
min-height: 10px;
min-width: 16px;
}

&.fa-exclamation-triangle {
&.fa-unlock {
color: @siteInsecureColor;
}

Expand Down
2 changes: 1 addition & 1 deletion less/variables.less
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
@audioColor: @highlightBlue;
@focusUrlbarOutline: @highlightBlue;
@siteSecureColor: @highlightBlue;
@siteInsecureColor: #FCBA00;
@siteInsecureColor: #C63626;
@siteEVColor: green;
@loadTimeColor: @highlightBlue;
@activeTabDefaultColor: @chromePrimary;
Expand Down
46 changes: 45 additions & 1 deletion test/components/navigationBarTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -414,13 +414,29 @@ describe('navigationBar tests', function () {
.moveToObject(navigator)
.waitForExist(urlbarIcon)
.getAttribute(urlbarIcon, 'class').then((classes) =>
classes.includes('fa-exclamation-triangle')
classes.includes('fa-unlock')
))
.windowByUrl(Brave.browserWindowUrl)
.click(urlbarIcon)
.waitForVisible('[data-l10n-id="insecureConnection"]')
.keys(Brave.keys.ESCAPE)
})
it('Shows insecure URL icon in title mode', function * () {
const page1Url = Brave.server.url('page1.html')
yield this.app.client.tabByUrl(Brave.newTabUrl).url(page1Url).waitForUrl(page1Url).windowParentByUrl(page1Url)
yield this.app.client
.moveToObject(navigator)
.waitForExist(urlInput)
.waitForValue(urlInput)
yield this.app.client
.moveToObject(activeWebview)
.click(activeWebview)
.waitForExist(titleBar)
.isExisting(urlbarIcon).then((isExisting) => assert(isExisting))
.getAttribute(urlbarIcon, 'class').then((classes) =>
classes.includes('fa-unlock')
)
})
it('Shows secure URL icon', function * () {
const page1Url = 'https://badssl.com/'
yield this.app.client.tabByUrl(Brave.newTabUrl).url(page1Url).waitForUrl(page1Url).windowParentByUrl(page1Url)
Expand All @@ -436,6 +452,22 @@ describe('navigationBar tests', function () {
.waitForVisible('[data-l10n-id="secureConnection"]')
.keys(Brave.keys.ESCAPE)
})
it('Shows secure URL icon in title mode', function * () {
const page1Url = Brave.server.url('page1.html')
yield this.app.client.tabByUrl(Brave.newTabUrl).url(page1Url).waitForUrl(page1Url).windowParentByUrl(page1Url)
yield this.app.client
.moveToObject(navigator)
.waitForExist(urlInput)
.waitForValue(urlInput)
yield this.app.client
.moveToObject(activeWebview)
.click(activeWebview)
.waitForExist(titleBar)
.isExisting(urlbarIcon).then((isExisting) => assert(isExisting))
.getAttribute(urlbarIcon, 'class').then((classes) =>
classes.includes('fa-lock')
)
})
it('does not show secure icon if page load fails', function * () {
const page1Url = Brave.server.url('ssl_spoof.html')
yield this.app.client.tabByUrl(Brave.newTabUrl).url(page1Url).waitForUrl(page1Url).windowParentByUrl(page1Url)
Expand All @@ -458,6 +490,18 @@ describe('navigationBar tests', function () {
)
)
})
it('shows insecure icon on a site with a sha-1 cert', function * () {
const page1Url = 'https://very.badssl.com/'
yield this.app.client.tabByUrl(Brave.newTabUrl).url(page1Url).waitForUrl(page1Url).windowParentByUrl(page1Url)
yield this.app.client
.moveToObject(navigator)
.waitForExist(urlbarIcon)
.waitUntil(() =>
this.app.client.getAttribute(urlbarIcon, 'class').then((classes) =>
classes.includes('fa-unlock')
)
)
})
it('Blocks running insecure content', function * () {
const page1Url = 'https://mixed-script.badssl.com/'
yield this.app.client.tabByUrl(Brave.newTabUrl)
Expand Down

1 comment on commit c16baaa

@darkdh
Copy link
Member

@darkdh darkdh commented on c16baaa Nov 17, 2016

Choose a reason for hiding this comment

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

++

Please sign in to comment.