Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(driver): only fail security state if scheme is not cryptographic #8338

Merged
merged 2 commits into from
Apr 17, 2019

Conversation

patrickhulce
Copy link
Collaborator

Summary
Currently no http site seems to be auditable by the extension in Canary. AFAICT, Chrome has started to fire security state insecure events, but not over the websocket protocol for some reason, just chrome.debugger... 😕

It seems like we should be able to just do the security check if the scheme is cryptographic and ignore it if it wasn't cryptographic? There are some other properties in the object that look promising (displayedInsecureContentStyle === 'insecure' maybe?), but want @hoten 's take since he's our resident security state expert :)

Event we get for bad ssl site
image

Event we get for http site
image

@connorjclark
Copy link
Collaborator

connorjclark commented Apr 16, 2019

Would we prefer to be more explicit and just do the check if https?

https://cs.chromium.org/chromium/src/url/gurl.cc?type=cs&sq=package:chromium&g=0&l=385

that's all Chrome is doing.

re: wss, how do we handle insecure wss now? does it get caught? never considered insecure websockets

@patrickhulce
Copy link
Collaborator Author

Would we prefer to be more explicit and just do the check if https?

I could be on board for that! Just means we have to thread the URL down to here which is kinda pain

@connorjclark
Copy link
Collaborator

I could be on board for that!

cool

Just means we have to thread the URL down to here which is kinda pain

ok you've convinced me

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

passes the important http://i.paul.irish test (insecure scheme -> secure scheme but insecure state)

LGTM

explanations,
schemeIsCryptographic,
}) => {
if (securityState === 'insecure' && schemeIsCryptographic) {
Copy link
Member

Choose a reason for hiding this comment

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

yeah, the nice thing about schemeIsCryptographic is that it's always testing the scheme of the URL on which this event was fired†. It would be difficult to get the current URL in here and (in the case of redirects) to make sure the URL we're checking is the one associated with the event we're checking.

I guess the question is are there cases where schemeIsCryptographic is false but we should still be checking for the securityState, otherwise we're testing a Chrome-generated error page?

†uh, at least I assume it is :)

@paulirish
Copy link
Member

wfm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants