Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
311033003:
Implementing mixed content for forms posting to insecure location from secure ones (Closed)
|
Created:
6 years, 6 months ago by mhm Modified:
6 years, 6 months ago Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionImplementing mixed content for forms posting to insecure location from secure ones.
When a form "action" attribute is pointing to insecure location from a secure one, this is flagged as mixed content. This is even checked if the pages dynamically changes the "action" attribute after the initial loading. The mixed content is non-blocking by default, however, this can be overridden in the preferences, and this will cause a submit to silently fail, other than a console message.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175714
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175926
Patch Set 1 : Adding mixed content to forms submitting to non-secure location from a secure one #
Total comments: 23
Patch Set 2 : Addressed reviewers comments and fixed formatting. #
Total comments: 7
Patch Set 3 : Adding comments and formatting. #Patch Set 4 : Addressed reviewers comments, added counter for mixed content and made warning messages more develo… #
Total comments: 3
Patch Set 5 : Housekeeping a boolean that is not needed any more. #
Total comments: 8
Patch Set 6 : Addressed the comments by adding more counters, fixing the mixed content warning and cleaning code #
Total comments: 2
Patch Set 7 : Fixed the issues with applying the patch. #Patch Set 8 : Fixing the patch errors. #
Total comments: 5
Patch Set 9 : Fixed the isse of incosistency with inspector #
Total comments: 4
Patch Set 10 : Moved the check for mixed content to a better location. #
Total comments: 8
Patch Set 11 : Addressing the previous comment and changing m_action to KURL. #
Total comments: 9
Patch Set 12 : Moved the code to check mixed content to the parseAttribute. #Patch Set 13 : Fixed the error when action attribute is empty. #Messages
Total messages: 64 (0 generated)
jww@ can you please review my cl.
Looking pretty good. Take a look at my comments and we'll circle back after you address them. https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... File LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-allowed.html (right): https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-allowed.html:14: if (window.testRunner) Extra spaces in front of lines 14 and 15. https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-allowed.html:19: <p>This test opens a window that show a form with "action" pointing to insecure sp: Should be "...that shows..." https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... File LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-blocked.html (right): https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-blocked.html:13: if (window.testRunner) Extra spaces before lines 13 & 14. https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-blocked.html:18: <p>This test opens a window that show a form with "action" pointing to insecure sp: Should be "...that shows..." and "...to an insecure..." https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... File LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame.html (right): https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame.html:17: <p>This test opens a window that show a form with "action" pointing to insecure sp: Should be "...that shows..." and "...pointing to an insecure..." https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:351: KURL actionURL = document().completeURL(m_attributes.action().isEmpty() ? document().url().string() : m_attributes.action()); You should probably factor all of this out into a utility method called getActionURL() or something, since you're repeating this logic elsewhere. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:761: KURL actionURL = document().completeURL(m_attributes.action().isEmpty() ? document().url().string() : m_attributes.action()); See comment above about factoring this logic out. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:762: if (!m_insecureSubmissionReported && MixedContentChecker::isMixedContent(document().securityOrigin(), actionURL)) { Although it's technically not needed, for clarify, can you add a #include for MixedContentChecker.h at the top of this file? https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:810: void HTMLFormElement::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModificationReason) Does this get called when the object is initially created? That is, during parsing, when the action attribute is set, does that call attributeChanged? If so, then the logic in finishParsingChildren() is redundant and can be removed. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:817: KURL actionURL = document().completeURL(m_attributes.action().isEmpty() ? document().url().string() : m_attributes.action()); Assuming you don't remove the logic in finishParsingChildren, this logic looks identical to the logic there, so you probably want to factor this out into a utility method as well. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... File Source/core/html/HTMLFormElement.h (right): https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.h:187: bool m_insecureSubmissionReported; I don't think we want to keep track of this. The behavior I think we want is: * Report a mixed content warning for a form action that is mixed content * If the form action changes, even if it was initially mixed content, if it changes to a *different* mixed content action, we should get another warning. (I believe this is matches what happens if you try to change an img src programatically to a new mixed content src, for example, but please verify). * Additionally, we want a mixed content warning when a submission is attempted with a mixed content form action. This variable prevents multiple warnings from occurring, even if the action is changed to a new mixed content URL.
Uploading the CL soon addressing all the comments. https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... File LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-allowed.html (right): https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-allowed.html:14: if (window.testRunner) On 2014/06/05 03:49:46, jww wrote: > Extra spaces in front of lines 14 and 15. Done. https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-allowed.html:19: <p>This test opens a window that show a form with "action" pointing to insecure On 2014/06/05 03:49:46, jww wrote: > sp: Should be "...that shows..." Done. https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... File LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-blocked.html (right): https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-blocked.html:13: if (window.testRunner) On 2014/06/05 03:49:46, jww wrote: > Extra spaces before lines 13 & 14. Done. https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-blocked.html:18: <p>This test opens a window that show a form with "action" pointing to insecure On 2014/06/05 03:49:46, jww wrote: > sp: Should be "...that shows..." and "...to an insecure..." Done. https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... File LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame.html (right): https://codereview.chromium.org/311033003/diff/1/LayoutTests/http/tests/secur... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame.html:17: <p>This test opens a window that show a form with "action" pointing to insecure On 2014/06/05 03:49:46, jww wrote: > sp: Should be "...that shows..." and "...pointing to an insecure..." Done. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:351: KURL actionURL = document().completeURL(m_attributes.action().isEmpty() ? document().url().string() : m_attributes.action()); On 2014/06/05 03:49:46, jww wrote: > You should probably factor all of this out into a utility method called > getActionURL() or something, since you're repeating this logic elsewhere. Done. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:761: KURL actionURL = document().completeURL(m_attributes.action().isEmpty() ? document().url().string() : m_attributes.action()); On 2014/06/05 03:49:46, jww wrote: > See comment above about factoring this logic out. Done. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:762: if (!m_insecureSubmissionReported && MixedContentChecker::isMixedContent(document().securityOrigin(), actionURL)) { On 2014/06/05 03:49:46, jww wrote: > Although it's technically not needed, for clarify, can you add a #include for > MixedContentChecker.h at the top of this file? Done. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:810: void HTMLFormElement::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModificationReason) Yes it does get called :-) Removed redundant code. On 2014/06/05 03:49:46, jww wrote: > Does this get called when the object is initially created? That is, during > parsing, when the action attribute is set, does that call attributeChanged? If > so, then the logic in finishParsingChildren() is redundant and can be removed. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:810: void HTMLFormElement::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModificationReason) On 2014/06/05 03:49:46, jww wrote: > Does this get called when the object is initially created? That is, during > parsing, when the action attribute is set, does that call attributeChanged? If > so, then the logic in finishParsingChildren() is redundant and can be removed. Done. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.cpp:817: KURL actionURL = document().completeURL(m_attributes.action().isEmpty() ? document().url().string() : m_attributes.action()); On 2014/06/05 03:49:46, jww wrote: > Assuming you don't remove the logic in finishParsingChildren, this logic looks > identical to the logic there, so you probably want to factor this out into a > utility method as well. Done. https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... File Source/core/html/HTMLFormElement.h (right): https://codereview.chromium.org/311033003/diff/1/Source/core/html/HTMLFormEle... Source/core/html/HTMLFormElement.h:187: bool m_insecureSubmissionReported; Agree. Removed! On 2014/06/05 03:49:46, jww wrote: > I don't think we want to keep track of this. The behavior I think we want is: > * Report a mixed content warning for a form action that is mixed content > * If the form action changes, even if it was initially mixed content, if it > changes to a *different* mixed content action, we should get another warning. (I > believe this is matches what happens if you try to change an img src > programatically to a new mixed content src, for example, but please verify). > * Additionally, we want a mixed content warning when a submission is > attempted with a mixed content form action. > > This variable prevents multiple warnings from occurring, even if the action is > changed to a new mixed content URL.
Removed redundant checks of mixed content, report mixed content if the action changes from insecure to another insecure place and re-factored the code that computes the action URL. This is in addition to some other formatting fixes.
Awesome. LGTM, with two minor nits to your comments. https://codereview.chromium.org/311033003/diff/20001/Source/core/html/HTMLFor... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/20001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.cpp:350: // Mixed content with form submission to insecure "action" This comment is actually a bit unclear. Can you be more explicit with something like: "On submission of a form a secure origin with an action pointing to an insecure action, if passive mixed content blocking is enabled, block the submission." https://codereview.chromium.org/311033003/diff/20001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.cpp:806: // it is mixed content. You should probably clarify "passive mixed content."
mkwst@ Can you please OWNER review this CL for me. https://codereview.chromium.org/311033003/diff/20001/Source/core/html/HTMLFor... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/20001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.cpp:350: // Mixed content with form submission to insecure "action" On 2014/06/05 17:35:05, jww wrote: > This comment is actually a bit unclear. Can you be more explicit with something > like: > > "On submission of a form a secure origin with an action pointing to an insecure > action, if passive mixed content blocking is enabled, block the submission." Done. https://codereview.chromium.org/311033003/diff/20001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.cpp:806: // it is mixed content. On 2014/06/05 17:35:05, jww wrote: > You should probably clarify "passive mixed content." Done.
Apologies. The terminal was waiting for me to add a title :-) mkwst@ can you please OWNER review this one for me.
Thanks for taking a stab at this. I think it needs one more pass, however. Comments inline. https://codereview.chromium.org/311033003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-allowed-expected.txt (right): https://codereview.chromium.org/311033003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/security/mixedContent/insecure-formSubmission-in-main-frame-allowed-expected.txt:1: CONSOLE WARNING: line 2: The page at 'https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-formSubmission.html' was loaded over HTTPS, but displayed insecure content from 'http://127.0.0.1:8080/security/resources/boring.html': this content should also be loaded over HTTPS. This error message doesn't work for forms. The content wasn't displayed. I'd suggest adding another method to MixedContentChecker (similarly to what the WebSocket folks have done) so that you can pass enough data into the internal method to generate a more relevant error string. I think that needs to happen in this CL, as it's going to be quite confusing to developers otherwise. https://codereview.chromium.org/311033003/diff/20001/Source/core/html/HTMLFor... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/20001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.cpp:352: if (!frame->loader().mixedContentChecker()->canDisplayInsecureContent(document().securityOrigin(), actionURL)) Please add a UseCounter here so we know what percentage of form submissions are being treated as passive mixed content. See https://codereview.chromium.org/312253003/ for some counters I just added to ResourceLoader. I'd also suggest adding one UseCounter::count call to the top of this method to measure the total number of submissions as a baseline. https://codereview.chromium.org/311033003/diff/20001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.cpp:808: if (MixedContentChecker::isMixedContent(document().securityOrigin(), actionURL)) This is strange, but I understand why you did it. :) How about: if (!document().....->canDisplayInsecureContent()) // Set the HTMLFormElement action to the empty string. } if (...isMixedContent(...)) { UseCounter::count(...); }
mkwst@ can you please have a look for an OWNER review? Addressed all the previously pointed comments.
Some nits. https://codereview.chromium.org/311033003/diff/60001/Source/core/html/HTMLFor... File Source/core/html/HTMLFormElement.h (right): https://codereview.chromium.org/311033003/diff/60001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.h:187: bool m_insecureSubmissionReported; You seem to have added this back in. Can you remove please? https://codereview.chromium.org/311033003/diff/60001/Source/core/loader/Mixed... File Source/core/loader/MixedContentChecker.h (right): https://codereview.chromium.org/311033003/diff/60001/Source/core/loader/Mixed... Source/core/loader/MixedContentChecker.h:56: Seems like you added an extra space here.
Thanks jww@ and apologies. mkwst@ I guess it should be clean now. https://codereview.chromium.org/311033003/diff/60001/Source/core/html/HTMLFor... File Source/core/html/HTMLFormElement.h (right): https://codereview.chromium.org/311033003/diff/60001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.h:187: bool m_insecureSubmissionReported; On 2014/06/05 23:00:48, jww wrote: > You seem to have added this back in. Can you remove please? Done.
https://codereview.chromium.org/311033003/diff/80001/Source/core/frame/UseCou... File Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/311033003/diff/80001/Source/core/frame/UseCou... Source/core/frame/UseCounter.h:437: MixedContentForm = 436, I should have been more clear: I think it would be valuable to split this counter into three counters: 1. How many forms do we see, total? 2. How many mixed-content forms do we see? 3. How many mixed-content forms are submitted? I'm not at all convinced that we'll be able to do anything with form submissions without effecting some large subset of the web. I'm more optimistic about being able to treat forms with insecure actions as passive mixed content. In order to evaluate those, we'll need counters for both, and a baseline that tells us what subset of the web we're effecting. https://codereview.chromium.org/311033003/diff/80001/Source/core/html/HTMLFor... File Source/core/html/HTMLFormElement.h (right): https://codereview.chromium.org/311033003/diff/80001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.h:189: KURL getActionURL() const We're only using this in the method you introduced, correct? If so, let's not add to the external signature. I'd prefer to see you implement this as a static method in the .cc file. https://codereview.chromium.org/311033003/diff/80001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.h:191: return document().completeURL(m_attributes.action().isEmpty() ? document().url().string() : m_attributes.action()); Why convert the document's URL into a string, and then convert it into a URL? Just return the document's URL if `action` is empty, and call `completeURL` on the attribute if it's not empty. (Sorry. I should have mentioned both of these in the initial review. I didn't pay enough attention at the time.) https://codereview.chromium.org/311033003/diff/80001/Source/core/loader/Mixed... File Source/core/loader/MixedContentChecker.cpp (right): https://codereview.chromium.org/311033003/diff/80001/Source/core/loader/Mixed... Source/core/loader/MixedContentChecker.cpp:95: void MixedContentChecker::logWarning(bool allowed, const String& action1, const String& action2, const KURL& target) const I'd prefer that we drop both strings, and turn this into an enum with four states (perhaps `{ EXECUTION, DISPLAY, CONNECTION, DISPLAY }`). That would also allow us to fix the WebSockets message, which is bad. If we had such an enum, I'd also like it to be passed through can*InsecureContentInternal so that we can drop the boolean arguments. Boolean arguments are practically meaningless at the callsite; Blink style generally prefers enums for readability.
mkwst@ can you please give it a look now? I addressed all the issues raised previously. https://codereview.chromium.org/311033003/diff/80001/Source/core/frame/UseCou... File Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/311033003/diff/80001/Source/core/frame/UseCou... Source/core/frame/UseCounter.h:437: MixedContentForm = 436, > 1. How many forms do we see, total? There is a counter already doing that (UseCounter::FormElement) > 2. How many mixed-content forms do we see? Now (UseCounter::MixedContentForm) is counting that. > 3. How many mixed-content forms are submitted? Added (UseCounter::MixedContentSubmittedForm) for that. https://codereview.chromium.org/311033003/diff/80001/Source/core/html/HTMLFor... File Source/core/html/HTMLFormElement.h (right): https://codereview.chromium.org/311033003/diff/80001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.h:189: KURL getActionURL() const On 2014/06/06 12:18:18, Mike West wrote: > We're only using this in the method you introduced, correct? If so, let's not > add to the external signature. I'd prefer to see you implement this as a static > method in the .cc file. Done. https://codereview.chromium.org/311033003/diff/80001/Source/core/html/HTMLFor... Source/core/html/HTMLFormElement.h:191: return document().completeURL(m_attributes.action().isEmpty() ? document().url().string() : m_attributes.action()); On 2014/06/06 12:18:18, Mike West wrote: > Why convert the document's URL into a string, and then convert it into a URL? > Just return the document's URL if `action` is empty, and call `completeURL` on > the attribute if it's not empty. > > (Sorry. I should have mentioned both of these in the initial review. I didn't > pay enough attention at the time.) Done. https://codereview.chromium.org/311033003/diff/80001/Source/core/loader/Mixed... File Source/core/loader/MixedContentChecker.cpp (right): https://codereview.chromium.org/311033003/diff/80001/Source/core/loader/Mixed... Source/core/loader/MixedContentChecker.cpp:95: void MixedContentChecker::logWarning(bool allowed, const String& action1, const String& action2, const KURL& target) const On 2014/06/06 12:18:18, Mike West wrote: > I'd prefer that we drop both strings, and turn this into an enum with four > states (perhaps `{ EXECUTION, DISPLAY, CONNECTION, DISPLAY }`). That would also > allow us to fix the WebSockets message, which is bad. > > If we had such an enum, I'd also like it to be passed through > can*InsecureContentInternal so that we can drop the boolean arguments. Boolean > arguments are practically meaningless at the callsite; Blink style generally > prefers enums for readability. Done.
Style nit. https://codereview.chromium.org/311033003/diff/100001/Source/core/loader/Mixe... File Source/core/loader/MixedContentChecker.h (right): https://codereview.chromium.org/311033003/diff/100001/Source/core/loader/Mixe... Source/core/loader/MixedContentChecker.h:69: Display, Style: Individual enum cases should be all caps (see https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... for an example).
https://codereview.chromium.org/311033003/diff/100001/Source/core/loader/Mixe... File Source/core/loader/MixedContentChecker.h (right): https://codereview.chromium.org/311033003/diff/100001/Source/core/loader/Mixe... Source/core/loader/MixedContentChecker.h:69: Display, On 2014/06/06 19:19:52, jww wrote: > Style: Individual enum cases should be all caps (see > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > for an example). Scratch that; I'm getting my style guides confused :-)
LGTM, thanks for going through a few iterations. :)
The CQ bit was checked by mohammed@chromium.org
On 2014/06/06 20:22:04, mhm wrote: > The CQ bit was checked by mailto:mohammed@chromium.org Thanks for your help mkwst@ and jww@
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohammed@chromium.org/311033003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/frame/UseCounter.h
Hunk #1 FAILED at 437.
1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej
Patch: Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
2ed3422a7828a9db51a75dcf027912adafe221f4..835ef591828213521bc230136cfd92ca3ee62675
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -437,7 +437,8 @@ public:
MixedContentRaw = 437,
MixedContentImage = 438,
MixedContentMedia = 439,
-
+ MixedContentForm = 440,
+ MixedContentSubmittedForm = 441,
// Add new features immediately above this line. Don't change assigned
// numbers of any item, and don't reuse removed slots.
// Also, run update_use_counter_feature_enum.py in
chromium/src/tools/metrics/histograms/
The CQ bit was checked by mohammed@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohammed@chromium.org/311033003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/frame/UseCounter.h
Hunk #1 FAILED at 438.
1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej
Patch: Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
2ed3422a7828a9db51a75dcf027912adafe221f4..d681b8c1d0ac26bc2e034a9fba88448dcea9cfcf
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -438,6 +438,8 @@ public:
MixedContentImage = 438,
MixedContentMedia = 439,
+ MixedContentForm = 441,
+ MixedContentSubmittedForm = 442,
// Add new features immediately above this line. Don't change assigned
// numbers of any item, and don't reuse removed slots.
// Also, run update_use_counter_feature_enum.py in
chromium/src/tools/metrics/histograms/
The CQ bit was checked by mohammed@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohammed@chromium.org/311033003/140001
Message was sent while issue was closed.
Change committed as 175714
Message was sent while issue was closed.
https://codereview.chromium.org/311033003/diff/140001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/140001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:68: return (action.isEmpty() ? document.url() : document.completeURL(action)); No need for the outer ( ) https://codereview.chromium.org/311033003/diff/140001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:69: } No indent for namespaces, please. https://codereview.chromium.org/311033003/diff/140001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:818: Element::attributeChanged(name, AtomicString("")); There's no reason to wrap "" in AtomicString(...). Is it correct to call Element::attributeChanged twice? Will that generate two mutation records?
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/319183008/ by abarth@chromium.org. The reason for reverting is: This CL incorrectly integrates with the inspector. Notice that in Element::didModifyAttribute, InspectorInstrumentation::didModifyDOMAttr will be called with the original value, not the mutated value..
Message was sent while issue was closed.
On 2014/06/06 22:53:01, abarth wrote: > A revert of this CL has been created in > https://codereview.chromium.org/319183008/ by mailto:abarth@chromium.org. > > The reason for reverting is: This CL incorrectly integrates with the inspector. > > Notice that in Element::didModifyAttribute, > InspectorInstrumentation::didModifyDOMAttr will be called with the original > value, not the mutated value.. I see what you are saying (thanks jww@ for the offline discussion about this). So there are two paths to address the issue of preventing submission when the user block mixed content: 1) The current one implemented. However, this will require some change to "Element::didModifyAttribute" such that "InspectorInstrumentation::didModifyDOMAttr" will (get) the value of the attribute that just changed. 2) [which I prefer] is that we can keep the value of the element. However, when the user tries to submit the form a console message is shown and submit would just return silently. abarth@ which one would you prefer. CC: mkwst@ any thought?
Message was sent while issue was closed.
On 2014/06/07 at 00:05:30, mohammed wrote: > I see what you are saying (thanks jww@ for the offline discussion about this). > So there are two paths to address the issue of preventing submission when the user block mixed content: > 1) The current one implemented. However, this will require some change to "Element::didModifyAttribute" such that "InspectorInstrumentation::didModifyDOMAttr" will (get) the value of the attribute that just changed. > > 2) [which I prefer] is that we can keep the value of the element. However, when the user tries to submit the form a console message is shown and submit would just return silently. > > abarth@ which one would you prefer. I'd recommend approach (2). The idea is that the form@action attribute would still have whatever value the developer set, but we would treat it differently depending on the mixed content policy. That's consistent with how iframe@src works in the same situation.
Message was sent while issue was closed.
abarth@ can you please have a look at it now. https://codereview.chromium.org/311033003/diff/140001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/140001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:68: return (action.isEmpty() ? document.url() : document.completeURL(action)); On 2014/06/06 22:49:48, abarth wrote: > No need for the outer ( ) Done. https://codereview.chromium.org/311033003/diff/140001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:69: } On 2014/06/06 22:49:48, abarth wrote: > No indent for namespaces, please. Done.
Message was sent while issue was closed.
https://codereview.chromium.org/311033003/diff/160001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/160001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:410: } It seems like we should do the mixed content check here, right after we do the SandboxedForms check. If you disagree, can you explain why the SandboxedForms check should be here but the mixed content check should be on line 361? https://codereview.chromium.org/311033003/diff/160001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:824: } Is there a reason to trigger this use counter for mixed content action URLs if the form is never submitted? It seems like we care more about how often users actually submit mixed content forms.
Message was sent while issue was closed.
abarth@ please see my comments and have a look at this for me. https://codereview.chromium.org/311033003/diff/160001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/160001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:410: } On 2014/06/07 08:59:50, abarth wrote: > It seems like we should do the mixed content check here, right after we do the > SandboxedForms check. If you disagree, can you explain why the SandboxedForms > check should be here but the mixed content check should be on line 361? Done. https://codereview.chromium.org/311033003/diff/160001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:824: } mkwst@ suggested that we need to count three things; (i) how many forms we see. (ii) how many forms have mixed content. (iii) how many forms have mixed content and are submitted. This can be useful when we see so many mixed content forms but a very small fraction of them git submitted, we can decide to treat this as an "active" mixed content as it will affect a very small number of users. On 2014/06/07 08:59:50, abarth wrote: > Is there a reason to trigger this use counter for mixed content action URLs if > the form is never submitted? It seems like we care more about how often users > actually submit mixed content forms.
Message was sent while issue was closed.
On 2014/06/09 at 15:55:35, mohammed wrote: > mkwst@ suggested that we need to count three things; (i) how many forms we see. (ii) how many forms have mixed content. (iii) how many forms have mixed content and are submitted. > > This can be useful when we see so many mixed content forms but a very small fraction of them git submitted, we can decide to treat this as an "active" mixed content as it will affect a very small number of users. We already count (i): http://www.chromestatus.com/metrics/feature/timeline/popularity/84 I'm not sure what decision we'd make based on data from (ii). Getting data from (iii) seems like the most relevant for deciding how to treat that case because it's what users actually experience.
Message was sent while issue was closed.
On 2014/06/09 16:21:49, abarth wrote: > On 2014/06/09 at 15:55:35, mohammed wrote: > > mkwst@ suggested that we need to count three things; (i) how many forms we > see. (ii) how many forms have mixed content. (iii) how many forms have mixed > content and are submitted. > > > > This can be useful when we see so many mixed content forms but a very small > fraction of them git submitted, we can decide to treat this as an "active" mixed > content as it will affect a very small number of users. > > We already count (i): > > http://www.chromestatus.com/metrics/feature/timeline/popularity/84 > > I'm not sure what decision we'd make based on data from (ii). Getting data from > (iii) seems like the most relevant for deciding how to treat that case because > it's what users actually experience. If we only get data for (iii) we implicitly assume that any mixed content form is always submitted by the user. I am not sure that we can assume that. This is why we need to see how many forms are mixed content (ii) and how many of these get submitted (iii).
Message was sent while issue was closed.
https://codereview.chromium.org/311033003/diff/180001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/180001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:405: KURL actionURL = getActionURL(document(), m_attributes.action()); You no longer need to call getActionURL because empty Strings have already been weeded out on line 397. Also, all the rest of this code gets the URL from submission->action() rather than from m_attributes.action(). We should probably match them. https://codereview.chromium.org/311033003/diff/180001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:412: if (!document().contentSecurityPolicy()->allowFormAction(KURL(submission->action()))) It looks like the correct way to get a KURL from submission->action() is to pass the string directly to the KURL constructor rather than to call completeURL on it. Please refactor this code so that we only parse the URL once in this function. Please consider changing the type of m_action in FormSubmission to KURL. I doubt there's a reason to keep it as a String. https://codereview.chromium.org/311033003/diff/180001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:824: } Please remove this part of the change. I don't think we need to count this information and it makes the engine slower. https://codereview.chromium.org/311033003/diff/180001/Source/core/loader/Mixe... File Source/core/loader/MixedContentChecker.cpp (right): https://codereview.chromium.org/311033003/diff/180001/Source/core/loader/Mixe... Source/core/loader/MixedContentChecker.cpp:97: message.append("displayed insecure content from '" + target.elidedString() + "': this content should also be loaded over HTTPS.\n"); String::append is extremely slow. Please use StringBuilder instead.
Message was sent while issue was closed.
On 2014/06/09 at 16:25:53, mohammed wrote: > If we only get data for (iii) we implicitly assume that any mixed content form is always submitted by the user. I am not sure that we can assume that. If a mixed content form is never submitted, it doesn't harm the user and we can ignore it. > This is why we need to see how many forms are mixed content (ii) and how many of these get submitted (iii). Suppose 80% of web pages contained forms with mixed content action URLs but only 0.00001% of them were ever submitted. In that (extreme!) case, we could still block submission of mixed content forms, which would provide users 100% protection against this vulnerability.
Message was sent while issue was closed.
On 2014/06/09 16:33:53, abarth wrote: > On 2014/06/09 at 16:25:53, mohammed wrote: > > If we only get data for (iii) we implicitly assume that any mixed content form > is always submitted by the user. I am not sure that we can assume that. > > If a mixed content form is never submitted, it doesn't harm the user and we can > ignore it. > > > This is why we need to see how many forms are mixed content (ii) and how many > of these get submitted (iii). > > Suppose 80% of web pages contained forms with mixed content action URLs but only > 0.00001% of them were ever submitted. In that (extreme!) case, we could still > block submission of mixed content forms, which would provide users 100% > protection against this vulnerability. I see your point now. One final comment .. do you think we should have at least a counter that counts the total number of forms submitted such that we can know the ration of num_blocked/num_submitted ?
Message was sent while issue was closed.
On 2014/06/09 17:24:05, mhm wrote: > I see your point now. > > One final comment .. do you think we should have at least a counter that counts > the total number of forms submitted such that we can know the ration of > num_blocked/num_submitted ? Sure.
Message was sent while issue was closed.
abarth@ please let me know what do you thing now. https://codereview.chromium.org/311033003/diff/180001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/180001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:405: KURL actionURL = getActionURL(document(), m_attributes.action()); On 2014/06/09 16:31:37, abarth wrote: > You no longer need to call getActionURL because empty Strings have already been > weeded out on line 397. > > Also, all the rest of this code gets the URL from submission->action() rather > than from m_attributes.action(). We should probably match them. Done. https://codereview.chromium.org/311033003/diff/180001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:412: if (!document().contentSecurityPolicy()->allowFormAction(KURL(submission->action()))) On 2014/06/09 16:31:37, abarth wrote: > It looks like the correct way to get a KURL from submission->action() is to pass > the string directly to the KURL constructor rather than to call completeURL on > it. Please refactor this code so that we only parse the URL once in this > function. > > Please consider changing the type of m_action in FormSubmission to KURL. I > doubt there's a reason to keep it as a String. Done. https://codereview.chromium.org/311033003/diff/180001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:824: } On 2014/06/09 16:31:37, abarth wrote: > Please remove this part of the change. I don't think we need to count this > information and it makes the engine slower. Done. https://codereview.chromium.org/311033003/diff/180001/Source/core/loader/Mixe... File Source/core/loader/MixedContentChecker.cpp (right): https://codereview.chromium.org/311033003/diff/180001/Source/core/loader/Mixe... Source/core/loader/MixedContentChecker.cpp:97: message.append("displayed insecure content from '" + target.elidedString() + "': this content should also be loaded over HTTPS.\n"); On 2014/06/09 16:31:38, abarth wrote: > String::append is extremely slow. Please use StringBuilder instead. Done.
Message was sent while issue was closed.
https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:43: #include "core/frame/csp/ContentSecurityPolicy.h" This header doesn't appear to be necessary. Are all these new headers needed? https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:420: UseCounter::count(document(), UseCounter::FormsSubmitted); Can we put this into the |else| branch of the previous if-statement? Otherwise, the stats will be skewed by the canSubmitToInsecureForm setting. https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:816: } Please delete this function. We don't want to override this function. https://codereview.chromium.org/311033003/diff/200001/Source/core/loader/Form... File Source/core/loader/FormSubmission.cpp (right): https://codereview.chromium.org/311033003/diff/200001/Source/core/loader/Form... Source/core/loader/FormSubmission.cpp:198: KURL actionURL = (copiedAttributes.action().isEmpty() ? document.url() : copiedAttributes.action()); There's no need for ( ) around the ? :
Message was sent while issue was closed.
abarth@ O commented on your comments below. Please let me know what do you think. https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:43: #include "core/frame/csp/ContentSecurityPolicy.h" On 2014/06/09 22:06:23, abarth wrote: > This header doesn't appear to be necessary. Are all these new headers needed? I didn't include them myself. I just moved them up to comply with the alphabetical order of formatting pre-submit was complaining about. https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:420: UseCounter::count(document(), UseCounter::FormsSubmitted); On 2014/06/09 22:06:23, abarth wrote: > Can we put this into the |else| branch of the previous if-statement? Otherwise, > the stats will be skewed by the canSubmitToInsecureForm setting. Done. https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:816: } On 2014/06/09 22:06:23, abarth wrote: > Please delete this function. We don't want to override this function. The reason we have this here is that we want to give developer a console warning of "passive" mixed content when they have a form with insecure action. This is should during loading and when dynamically changing the attribute. The main goal is to alert developers and drive them away from such insecure practices. https://codereview.chromium.org/311033003/diff/200001/Source/core/loader/Form... File Source/core/loader/FormSubmission.cpp (right): https://codereview.chromium.org/311033003/diff/200001/Source/core/loader/Form... Source/core/loader/FormSubmission.cpp:198: KURL actionURL = (copiedAttributes.action().isEmpty() ? document.url() : copiedAttributes.action()); On 2014/06/09 22:06:23, abarth wrote: > There's no need for ( ) around the ? : Done.
Message was sent while issue was closed.
On 2014/06/09 at 23:27:59, mohammed wrote: > abarth@ O commented on your comments below. Please let me know what do you think. > > https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... > File Source/core/html/HTMLFormElement.cpp (right): > > https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... > Source/core/html/HTMLFormElement.cpp:43: #include "core/frame/csp/ContentSecurityPolicy.h" > On 2014/06/09 22:06:23, abarth wrote: > > This header doesn't appear to be necessary. Are all these new headers needed? > > I didn't include them myself. I just moved them up to comply with the alphabetical order of formatting pre-submit was complaining about. Ah, ok. > https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... > Source/core/html/HTMLFormElement.cpp:816: } > On 2014/06/09 22:06:23, abarth wrote: > > Please delete this function. We don't want to override this function. > > The reason we have this here is that we want to give developer a console warning of "passive" mixed content when they have a form with insecure action. This is should during loading and when dynamically changing the attribute. The main goal is to alert developers and drive them away from such insecure practices. We should do that work in HTMLFormElement::parseAttribute
Message was sent while issue was closed.
abarth@ I moved the check to parseAttribute. Please have a look at it now for me. Sorry about all these rounds. First bug in Blink :-) https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/311033003/diff/200001/Source/core/html/HTMLFo... Source/core/html/HTMLFormElement.cpp:816: } On 2014/06/09 22:06:23, abarth wrote: > Please delete this function. We don't want to override this function. Done.
lgtm
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10897) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11357)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
Fixed the case when the action attribute is empty Attributes::m_action need to be empty too. abarth@ I am not sure if I need another lgtm?
Fixed the case when the action attribute is empty Attributes::m_action need to be empty too. abarth@ I am not sure if I need another lgtm?
lgtm
The CQ bit was checked by mohammed@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohammed@chromium.org/311033003/240001
The CQ bit was unchecked by mohammed@chromium.org
The CQ bit was checked by mohammed@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohammed@chromium.org/311033003/240001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by mohammed@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohammed@chromium.org/311033003/240001
Message was sent while issue was closed.
Change committed as 175926 |
