Frontend EV Certificate Changes #11776

sashaperigo · 2017-11-03T00:09:33Z

Require: brave/muon#377
Fixes #791

Work in progress PR for the frontend EV certificate changes. Still needs lots of work on the CSS end!

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:
Updated relevant unit tests.

Go to github.com
There should be GitHub, Inc. [US] displayed in URL bar
(organization name + country code)

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

luixxiul · 2017-11-20T15:21:51Z

@sashaperigo do you need any assistance? 😄 Maybe I could help working on CSS/LESS.

darkdh · 2017-11-21T22:56:13Z

@luixxiul , @cezaraugusto
feel free to push CSS changes to this PR. This is the current look

darkdh · 2017-11-21T22:56:46Z

Changes made according to muon change brave/muon@6c7c9ae
ready for review

diracdeltas · 2017-11-22T20:35:37Z

app/renderer/components/frame/frame.js

+      if (e.securityInfo.securityLevel === 'ev-secure') {
+        if (e.securityInfo.certificate &&
+            e.securityInfo.certificate.organizationNames.length &&
+            e.securityInfo.certificate.countryName) {


I think we shouldn't require countryName, since it's not a required field in EV certs according to https://cabforum.org/ev-certificate-contents/.

addressed in 130d7ef

diracdeltas · 2017-11-22T20:50:49Z

app/renderer/components/frame/frame.js

@@ -682,15 +683,27 @@ class Frame extends React.Component {
        isSecure = false
        const parsedUrl = urlParse(this.props.location)
        ipc.send(messages.CHECK_CERT_ERROR_ACCEPTED, parsedUrl.host, this.props.tabId)
-      } else if (['warning', 'passive-mixed-content'].includes(e.securityState)) {
+      } else if (e.securityState === 'warning' ||


i think we can get rid of the check for 'warning' since it was removed in https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebSecurityStyle.h?q=kWebSecurityStyleNeutral&dr=CSs&l=11&rcl=ec1bba9ca0f2e6d210af6f93948708a95c406ce7&dlp=chromium&dlf=src/third_party/WebKit/public/platform/WebSecurityStyle.h&dlc=1c4d759e44259650dfb2c426a7f997d2d0bc73dc&dlr=3&dlgp=third_party/WebKit/public/platform/WebSecurityStyle.h&dlgr=chromium/chromium/src&drp=chromium&drf=src/third_party/WebKit/public/platform/WebSecurityStyle.h&drc=ec1bba9ca0f2e6d210af6f93948708a95c406ce7&drr=4&drgp=third_party/WebKit/public/platform/WebSecurityStyle.h&drgr=chromium/chromium/src

addressed in 130d7ef

diracdeltas · 2017-12-05T23:00:13Z

js/stores/windowStore.js

@@ -624,6 +625,10 @@ const doAction = (action) => {
          windowState = windowState.setIn(path.concat(['security', 'runInsecureContent']),
            action.securityState.runInsecureContent)
        }
+        if (action.securityState.evCert !== undefined) {


i think this defaults to null, not undefined: https://github.com/brave/browser-laptop/pull/11776/files#diff-c3c556f1ee31674844d6d0be95748d8eR677

nvm, i guess this has to check for undefined so null can be used to clear previous values

diracdeltas · 2017-12-05T23:03:16Z

my muon build process isn't working but the changes lgtm

cezaraugusto

I'm ok with front-end changes in here and if there's a need for improvement that can be done in a follow-up. lgtm

darkdh · 2017-12-19T23:05:12Z

muon PR has been merged and will be available in 4.6.1

jumde · 2018-01-19T23:22:53Z

less/navigationBar.less

@@ -863,9 +864,9 @@
      align-items: center;
      justify-content: center;
      height: @urlbarFormHeight;
-      width: @urlbarFormHeight;
+      // width: @urlbarFormHeight;


if the width is not required, can we remove it instead of commenting it out?

removed in 37b0969#diff-02c4b23ad267fe636760179e32fa29ceL867

diracdeltas · 2018-01-20T02:20:54Z

app/renderer/components/frame/frame.js

        // Passive mixed content should not upgrade an insecure connection to a
        // partially-secure connection. It can only downgrade a secure
        // connection.
-        isSecure = this.props.isSecure !== false ? 1 : false
+        isSecure = 1


this code will actually never be reached because securityState is insecure when e.securityInfo.mixedContentStatus === 'content-status-displayed'. this is also an issue in 0.20.x - see #12742. i am fixing it

cezaraugusto · 2018-01-22T18:53:51Z

pushed 37b0969 which removed commented property (#11776 (review)) and removed ev cert from titlemode (#11776 (review))

diracdeltas · 2018-01-22T18:54:40Z

app/renderer/components/navigation/urlBar.js

@@ -481,7 +482,23 @@ class UrlBar extends React.Component {
    return props
  }

+  get showEvCert () {
+    return !this.props.titleMode && <span className='evCert'> {this.props.evCert} </span>


this will return false if we are in titleMode. according to react docs (https://reactjs.org/docs/conditional-rendering.html#preventing-component-from-rendering), it should return null when an element isn't rendered.

thanks changed in 97f689b via push -f

cezaraugusto · 2018-01-22T18:54:56Z

each new commit auto dismisses previous reviews please confirm if we're good to go now, thanks

diracdeltas

i'd prefer #12743 be merged first since it's for an earlier milestone and these two PRs will conflict. otherwise lgtm.

darkdh

++, I will get #12743 merged first and then rebased this PR on it

@bridiver

Auditors: @bridiver, @diracdeltas, @cezaraugusto

darkdh · 2018-01-22T20:40:18Z

rebased

cezaraugusto

since #12743 is merged and this is rebased I'm merging, thanks all

Frontend EV Certificate Changes

cezaraugusto · 2018-01-22T20:50:22Z

master a3e3911
0.21.x 1e91183

follow-up of #11776 fix #12780

diracdeltas self-requested a review November 3, 2017 00:09

diracdeltas assigned sashaperigo Nov 3, 2017

diracdeltas added security PR/work-in-progress ⚒ labels Nov 3, 2017

luixxiul added the QA/test-plan-required label Nov 13, 2017

luixxiul added the PR/blocked label Nov 20, 2017

darkdh self-assigned this Nov 21, 2017

darkdh force-pushed the frontend_ev branch from 2479f2a to 4051d0a Compare November 21, 2017 22:53

darkdh added QA/test-plan-specified and removed PR/work-in-progress ⚒ QA/test-plan-required labels Nov 21, 2017

darkdh requested review from bridiver and cezaraugusto November 21, 2017 22:54

darkdh removed the request for review from cezaraugusto November 21, 2017 23:04

darkdh assigned cezaraugusto Nov 21, 2017

darkdh requested review from bsclifton and bbondy November 21, 2017 23:04

diracdeltas reviewed Nov 22, 2017

View reviewed changes

darkdh force-pushed the frontend_ev branch from 4051d0a to 130d7ef Compare November 29, 2017 20:09

diracdeltas reviewed Dec 5, 2017

View reviewed changes

diracdeltas previously approved these changes Dec 5, 2017

View reviewed changes

diracdeltas requested a review from jumde December 13, 2017 19:35

cezaraugusto previously approved these changes Dec 19, 2017

View reviewed changes

luixxiul removed the PR/blocked label Dec 22, 2017

jumde reviewed Jan 19, 2018

View reviewed changes

diracdeltas reviewed Jan 20, 2018

View reviewed changes

jumde previously approved these changes Jan 22, 2018

View reviewed changes

mikemaccana mentioned this pull request Jan 22, 2018

Prominently show validated legal identity & jurisdiction to users #791

Closed

cezaraugusto dismissed jumde’s stale review via 86c8c24 January 22, 2018 18:50

cezaraugusto force-pushed the frontend_ev branch from 86c8c24 to 37b0969 Compare January 22, 2018 18:51

diracdeltas reviewed Jan 22, 2018

View reviewed changes

cezaraugusto force-pushed the frontend_ev branch from 37b0969 to 97f689b Compare January 22, 2018 18:57

diracdeltas previously approved these changes Jan 22, 2018

View reviewed changes

darkdh previously approved these changes Jan 22, 2018

View reviewed changes

sashaperigo and others added 4 commits January 22, 2018 12:37

Fixes issue brave#791, frontend EV certificate changes

036abb1

Address change of 6c7c9aeece14c57d69c348748433886fc65198e8

582ea22

Auditors: @bridiver, @diracdeltas, @cezaraugusto

tweak certificate spacing in urlbar

85785f8

do not show up ev cert if urlbar is in title mode

0cf5bfe

darkdh dismissed stale reviews from diracdeltas and themself via 0cf5bfe January 22, 2018 20:39

darkdh force-pushed the frontend_ev branch from 97f689b to 0cf5bfe Compare January 22, 2018 20:39

cezaraugusto approved these changes Jan 22, 2018

View reviewed changes

cezaraugusto merged commit a3e3911 into brave:master Jan 22, 2018

cezaraugusto added a commit that referenced this pull request Jan 22, 2018

Merge pull request #11776 from sashaperigo/frontend_ev

1e91183

Frontend EV Certificate Changes

diracdeltas mentioned this pull request Jan 22, 2018

EV certificate string overflows #12780

Closed

cezaraugusto added a commit that referenced this pull request Jan 22, 2018

enforce max limit to ev cert display

acc9aec

follow-up of #11776 fix #12780

cezaraugusto added a commit that referenced this pull request Jan 22, 2018

enforce max limit to ev cert display

2cac2b4

follow-up of #11776 fix #12780

cezaraugusto added a commit that referenced this pull request Jan 22, 2018

enforce max limit to ev cert display

b98c2fa

follow-up of #11776 fix #12780

cezaraugusto mentioned this pull request Jan 22, 2018

enforce max limit to ev cert display #12781

Merged

10 tasks

cndouglas mentioned this pull request Feb 5, 2018

Extended validation certificate info text should be clickable #13011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frontend EV Certificate Changes #11776

Frontend EV Certificate Changes #11776

sashaperigo commented Nov 3, 2017 •

edited by luixxiul

Loading

luixxiul commented Nov 20, 2017 •

edited

Loading

darkdh commented Nov 21, 2017

darkdh commented Nov 21, 2017

diracdeltas Nov 22, 2017

darkdh Nov 29, 2017

diracdeltas Nov 22, 2017

darkdh Nov 29, 2017

diracdeltas Dec 5, 2017

diracdeltas Dec 5, 2017

diracdeltas commented Dec 5, 2017

cezaraugusto left a comment

darkdh commented Dec 19, 2017

jumde Jan 19, 2018

cezaraugusto Jan 22, 2018

diracdeltas Jan 20, 2018

diracdeltas Jan 20, 2018

cezaraugusto commented Jan 22, 2018

diracdeltas Jan 22, 2018

cezaraugusto Jan 22, 2018

cezaraugusto commented Jan 22, 2018

diracdeltas left a comment

darkdh left a comment

darkdh commented Jan 22, 2018

cezaraugusto left a comment

cezaraugusto commented Jan 22, 2018

Frontend EV Certificate Changes #11776

Frontend EV Certificate Changes #11776

Conversation

sashaperigo commented Nov 3, 2017 • edited by luixxiul Loading

luixxiul commented Nov 20, 2017 • edited Loading

darkdh commented Nov 21, 2017

darkdh commented Nov 21, 2017

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 Dec 5, 2017

cezaraugusto left a comment

Choose a reason for hiding this comment

darkdh commented Dec 19, 2017

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

cezaraugusto commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezaraugusto commented Jan 22, 2018

diracdeltas left a comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

darkdh commented Jan 22, 2018

cezaraugusto left a comment

Choose a reason for hiding this comment

cezaraugusto commented Jan 22, 2018

sashaperigo commented Nov 3, 2017 •

edited by luixxiul

Loading

luixxiul commented Nov 20, 2017 •

edited

Loading