Skip to content
This repository has been archived by the owner. It is now read-only.

/ browser-laptop Archived

View certificate on mixed content site #8531

Merged
merged 1 commit into from Apr 28, 2017
Merged

View certificate on mixed content site #8531

merged 1 commit into from Apr 28, 2017

Conversation

@darkdh
Copy link
Member

darkdh commented Apr 27, 2017 •
edited

fix #8530

Auditors: @diracdeltas

Test Plan:

  1. Go to https://mixed-script.badssl.com/
  2. We should be able to view certificate
  3. Click load unsafe scripts
  4. We should still have option to view certificate
  • 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).

screen shot 2017-04-27 at 2 16 54 pm

screen shot 2017-04-27 at 2 16 46 pm
@darkdh darkdh added the security label Apr 27, 2017
@darkdh darkdh added this to the 0.15.1 milestone Apr 27, 2017
@darkdh darkdh self-assigned this Apr 27, 2017
@darkdh darkdh requested a review from diracdeltas Apr 27, 2017
@diracdeltas
Copy link
Member

diracdeltas commented Apr 27, 2017

lgtm!
@diracdeltas
Copy link
Member

diracdeltas commented Apr 27, 2017

seems like there is a Travis test failure, not sure if intermittent: https://travis-ci.org/brave/browser-laptop/jobs/226508576#L5866
@diracdeltas
Copy link
Member

diracdeltas commented Apr 27, 2017

I can't check this due to a muon error, but we should also show the Show certificate button on an HTTPS site with an invalid certificate. Ex: go to https://wrong.host.badssl.com/ and click through the warning, then click on the unlock icon.
@darkdh
Copy link
Member Author

darkdh commented Apr 27, 2017

will add that.
@darkdh darkdh force-pushed the 8530 branch from 026cd01 to e842b4b Apr 27, 2017
@darkdh
Copy link
Member Author

darkdh commented Apr 27, 2017

fixed in e842b4b
and also fix an error of getWebContents when you navigate to https://wrong.host.badssl.com/

An uncaught exception occurred in the main process Uncaught Exception:
TypeError: tabs.getWebContents is not a function
    at tabsReducer (/Users/darkdh/Projects/browser-laptop-bootstrap/src/browser-laptop/app/browser/reducers/tabsReducer.js:136:26)
    at reducers.reduce (/Users/darkdh/Projects/browser-laptop-bootstrap/src/browser-laptop/js/stores/appStore.js:365:24)
    at Array.reduce (native)
    at applyReducers (/Users/darkdh/Projects/browser-laptop-bootstrap/src/browser-laptop/js/stores/appStore.js:363:51)
    at handleAppAction (/Users/darkdh/Projects/browser-laptop-bootstrap/src/browser-laptop/js/stores/appStore.js:400:14)
    at callbacks.forEach (/Users/darkdh/Projects/browser-laptop-bootstrap/src/browser-laptop/js/dispatcher/appDispatcher.js:84:7)
    at Array.forEach (native)
    at AppDispatcher.dispatchToOwnRegisteredCallbacks (/Users/darkdh/Projects/browser-laptop-bootstrap/src/browser-laptop/js/dispatcher/appDispatcher.js:83:20)
    at AppDispatcher.dispatchInternal (/Users/darkdh/Projects/browser-laptop-bootstrap/src/browser-laptop/js/dispatcher/appDispatcher.js:109:10)
    at AppDispatcher.dispatch (/Users/darkdh/Projects/browser-laptop-bootstrap/src/browser-laptop/js/dispatcher/appDispatcher.js:68:12)
@diracdeltas
diracdeltas suggested changes Apr 27, 2017
View changes
js/components/siteInfo.js Outdated
@@ -204,6 +206,7 @@ class SiteInfo extends ImmutableComponent {
connectionInfo =
<div className={css(styles.connectionInfo__wrapper)}>
<div data-l10n-id='insecureConnectionInfo' />
{viewCertificateButton}

This comment has been minimized.

Sign in to view
Copy link
@diracdeltas

diracdeltas Apr 27, 2017

Member

i think this should only appear if the site is actually HTTPS, otherwise clicking the button doesn't do anything.

screen shot 2017-04-27 at 22 50 19
@darkdh darkdh force-pushed the 8530 branch from e842b4b to dbde7d0 Apr 27, 2017
@darkdh
Copy link
Member Author

darkdh commented Apr 27, 2017

Use different description to separate certificate error and plain http
screen shot 2017-04-27 at 7 31 05 pm
@diracdeltas
Copy link
Member

diracdeltas commented Apr 27, 2017 •
edited

this doesn't seem to clear the bad certificate state when you navigate from wrong.host.badssl.com to example.com:
screen shot 2017-04-27 at 23 34 04

for now, it might be simpler to just show the 'Show certificate' button whenever the URL's protocol is HTTPS. the special text for bad certificates is nice but not necessary. i think all the cases where we need the 'view certificate' button are covered by urlParse(this.location).protocol === 'https:'
@darkdh
View certificate on mixed content site
Verified
This commit was signed with a verified signature.
darkdh Anthony Tseng
GPG key ID: E14094A20D3AAEEC Learn about signing commits
Loading status checks…
93d3b88 
fix #8530

Auditors: @diracdeltas

Test Plan:
1. Go to https://mixed-script.badssl.com/
2. We should be able to view certificate
3. Click load unsafe scripts
4. We should still have option to view certificate

1. Go to https://wrong.host.badssl.com/
2. Click proceed to site
3. We should still have option to view certificate
4. Go to http://example.com/
5. We shouldn't be able to view certificate
@darkdh darkdh force-pushed the 8530 branch from dbde7d0 to 93d3b88 Apr 27, 2017
@darkdh
Copy link
Member Author

darkdh commented Apr 27, 2017

@diracdeltas I think I sort the state out in 93d3b88
@diracdeltas
diracdeltas approved these changes Apr 28, 2017
View changes
@diracdeltas diracdeltas merged commit 754951e into master Apr 28, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@diracdeltas diracdeltas deleted the 8530 branch Apr 28, 2017
@darkdh darkdh mentioned this pull request May 3, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@diracdeltas diracdeltas

Assignees

@darkdh darkdh

Labels
security
Projects
None yet
Milestone
  0.15.300 (Frozen,...
Linked issues

Successfully merging this pull request may close these issues.

View Certificate on mixed content site
2 participants
@darkdh @diracdeltas
You can’t perform that action at this time.