Bug 140624

Created attachment 244906 [details] Patch

Comment on attachment 244906 [details] Patch Attachment 244906 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5779264867663872 New failing tests: http/tests/security/mixedContent/websocket/insecure-websocket.html

Created attachment 244909 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 244906 [details] Patch Attachment 244906 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5237636742512640 New failing tests: http/tests/security/mixedContent/websocket/insecure-websocket.html

Created attachment 244911 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Created attachment 244918 [details] Patch

Created attachment 245831 [details] Block mixed content web sockets

Hey Alexy, I'd like to pick up work on mixed content blocking again. Could you take a look at bug #142469 please? I'm hesitant to proceed further without approval for how to handle that issue. In the meantime, I will update the patch in this bug to apply cleanly on master (and not depend on bug #140940). That will require removing the test insecure-websocket-in-main-frame-allowed, since we can't change settings until we solve bug #142469. Unrelated WARNING to make sure I don't forget: the patch I posted above is missing a protector RefPtr for the lambda in WebSocket::connect.

Doing this has always obviously been a good idea, and because none of the other major browsers allow it I'm not too concerned about the compatibility risk. (In reply to comment #9) > In the meantime, I will update the patch in this bug to apply cleanly on > master (and not depend on bug #140940). That will require removing the test > insecure-websocket-in-main-frame-allowed, since we can't change settings > until we solve bug #142469. I don't think we need a setting to allow insecure web sockets from a secure page, so I don't think that test is important. Please update the patch to trunk and I'll review when it's ready.

Thanks. (In reply to comment #10) > I don't think we need a setting to allow insecure web sockets from a secure > page, so I don't think that test is important. I agree in the case of web sockets, where there is no compatibility risk to blocking unconditionally, but it's a problem for other types of content. For GTK, we will need to revert the patch in bug #142378 for our next release in order to keep our existing API working, unless we come up with a solution for bug #142469 in the meantime, because bug #142378 actually _removed_ the ability for browsers to choose to block content that we don't want to block by default.

(In reply to comment #11) > Thanks. > > (In reply to comment #10) > > I don't think we need a setting to allow insecure web sockets from a secure > > page, so I don't think that test is important. > > I agree in the case of web sockets, where there is no compatibility risk to > blocking unconditionally, but it's a problem for other types of content. For > GTK, we will need to revert the patch in bug #142378 for our next release in > order to keep our existing API working, unless we come up with a solution > for bug #142469 in the meantime, because bug #142378 actually _removed_ the > ability for browsers to choose to block content that we don't want to block > by default. Understood. My comment was truly only about this specific feature and this specific test.

Turns out I didn't need a protector RefPtr due to the use of ActiveDOMObject::setPendingActivity(), which I had forgotten modifies the reference count.

Created attachment 255117 [details] Patch

Is this going to break 1Password extension in Safari?

I guess someone with a Mac could investigate that, but why do you think it would break? Surely they don't send passwords over an insecure WebSocket?

(In reply to comment #16) > I guess someone with a Mac could investigate that, but why do you think it > would break? Surely they don't send passwords over an insecure WebSocket? I believe their extension communicates with their native app on localhost with a WebSocket. I would be surprised if it were plaintext, but I suppose it's possible.

I don't know, but I would be surprised if it weren't plaintext - there is no reason to use wss when talking to localhost.

Looks like 1Password also supports Firefox, Chrome, and Opera, all of which block ws:// web sockets on https:// pages.

Other browsers also have different mechanisms for extensions, we don't know if 1Password even uses WebSockets there. We should check, I doubt that pure logic will be good enough.

Comment on attachment 255117 [details] Patch I'll mark this cq? instead of cq+ due to the concern about 1Password; please give it cq+ when you're comfortable committing it.

If nobody with a Mac dev environment has taken a look by Monday, I should be able to give 1 password a shot sometime Monday afternoon.

1Password uses WebSockets from their global page which is not http: but rather safari-extension: (or something close to that). Its worth testing, but I don't think it will be an issue.

Comment on attachment 255117 [details] Patch Clearing flags on attachment: 255117 Committed r185848: <http://trac.webkit.org/changeset/185848>

All reviewed patches have been landed. Closing bug.