Closed Bug 1247807 Opened 4 years ago Closed 4 years ago
Mixed Content UI broken when using CSP directive 'upgrade-insecure-requests'
No description provided.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
We introduced the problem with Bug 663570 (Implement Meta CSP) because for Meta CSP we had to distinguish between preloads and actual loads, because the meta CSP might have been applied only speculatively. Before Meta CSP, we didn't have to distinguish between preloads and actual loads for upgrade-insecure-requests within mixedContentBlocker. We are performing the right upgrade from http to https within the backend [1,2,3], because we are relying on logic within the loadInfo. We even have good testcoverage for that (csp/test_upgrade_insecure.html) where we make sure that no *http* requests hits the server. Within nsMixedContentBlocker  however, we are relying on logic that lives on the document. We are performing the right check, but unfortunately mUpgradeInsecurePreloads holds the wrong value on the document. Problem is, that if a CSP is delivered through the header we *always* have to upgrade from http to https. Not depending on any speculatively applied CSP.  http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#2269  http://mxr.mozilla.org/mozilla-aurora/source/netwerk/base/nsNetUtil.cpp#2269  http://mxr.mozilla.org/mozilla-beta/source/netwerk/protocol/http/nsHttpChannel.cpp#339  http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#670
Comment on attachment 8718648 [details] [diff] [review] bug_1247807_mixed_content_ui_bug.patch >+ mUpgradeInsecurePreloads = mUpgradeInsecureRequests; Shouldn't this only happen if !mUpgradeInsecurePreloads? That is, if it's already true I don't think we should be changing it here... If it _can't_ be true, there should be a comment explaining why. r=me with that.
Attachment #8718648 - Flags: review?(bzbarsky) → review+
In fact it should not be possible, but nevertheless I agree with you. Let's play it safe and only set it if |!mUpgradeInsecurePreloads?|. Thanks Boris!
Comment on attachment 8719047 [details] [diff] [review] bug_1247807_mixed_content_ui_bug.patch Approval Request Comment Since the introduction of meta CSP (Bug 663570) we show a broken mixed content UI if the directive 'upgrade-insecure-requests' is used. The backend (security wise) is working as exptected - we have a wide test coverage for that (see e.g. csp/test_upgrade_insecure.html). [Feature/regressing bug #]: Bug 663570 [User impact if declined]: As described above, users might be confused because the see a broken UI even though the upgrade from http to https is performed correctly. [Describe test coverage new/current, TreeHerder]: Manual testing for the UI, backend is covered by test_upgrade_insecure.html [Risks and why]: Low, only affects pages that use the CSP directive 'upgrade-insecure-requests'. [String/UUID change made/needed]: No.
Comment on attachment 8719047 [details] [diff] [review] bug_1247807_mixed_content_ui_bug.patch Small change, improve the feature, taking it. Should be in 45 beta 7
You need to log in before you can comment on or make changes to this bug.