Implementing mixed content for forms posting to insecure location from secure ones

Source/core/html/HTMLFormElement.cpp

Index: Source/core/html/HTMLFormElement.cpp
diff --git a/Source/core/html/HTMLFormElement.cpp b/Source/core/html/HTMLFormElement.cpp
index 566e8c19e40f2aa73d99201f720d2b2f8ff7090b..e4ec40c3625ae71779feacd52e4462f0356c34ae 100644
--- a/Source/core/html/HTMLFormElement.cpp
+++ b/Source/core/html/HTMLFormElement.cpp
@@ -37,6 +37,10 @@
 #include "core/events/Event.h"
 #include "core/events/GenericEventQueue.h"
 #include "core/events/ScopedEventQueue.h"
+#include "core/frame/DOMWindow.h"
+#include "core/frame/LocalFrame.h"
+#include "core/frame/UseCounter.h"
+#include "core/frame/csp/ContentSecurityPolicy.h"

[abarth-chromium, 2014/06/09 22:06:23]

This header doesn't appear to be necessary.  Are all these new headers needed?

[mhm, 2014/06/09 23:27:59]

I didn't include them myself. I just moved them up to comply with the
alphabetical order of formatting pre-submit was complaining about.

 #include "core/html/HTMLCollection.h"
 #include "core/html/HTMLDialogElement.h"
 #include "core/html/HTMLImageElement.h"
@@ -46,12 +50,10 @@
 #include "core/html/forms/FormController.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
-#include "core/frame/DOMWindow.h"
-#include "core/frame/LocalFrame.h"
-#include "core/frame/UseCounter.h"
-#include "core/frame/csp/ContentSecurityPolicy.h"
+#include "core/loader/MixedContentChecker.h"
 #include "core/rendering/RenderTextControl.h"
 #include "platform/UserGestureIndicator.h"
+#include "wtf/text/AtomicString.h"
 
 using namespace std;
 
@@ -392,7 +394,7 @@ void HTMLFormElement::scheduleFormSubmission(PassRefPtr<FormSubmission> submissi
     }
 
     if (protocolIsJavaScript(submission->action())) {
-        if (!document().contentSecurityPolicy()->allowFormAction(KURL(submission->action())))
+        if (!document().contentSecurityPolicy()->allowFormAction(submission->action()))
             return;
         document().frame()->script().executeScriptIfJavaScriptURL(submission->action());
         return;
@@ -409,6 +411,14 @@ void HTMLFormElement::scheduleFormSubmission(PassRefPtr<FormSubmission> submissi
     if (!targetFrame->page())
         return;
 
+    if (MixedContentChecker::isMixedContent(document().securityOrigin(), submission->action())) {
+        UseCounter::count(document(), UseCounter::MixedContentFormsSubmitted);
+        if (!document().frame()->loader().mixedContentChecker()->canSubmitToInsecureForm(document().securityOrigin(), submission->action()))
+            return;
+    }
+
+    UseCounter::count(document(), UseCounter::FormsSubmitted);

[abarth-chromium, 2014/06/09 22:06:23]

Can we put this into the |else| branch of the previous if-statement?  Otherwise,
the stats will be skewed by the canSubmitToInsecureForm setting.

[mhm, 2014/06/09 23:27:59]

Done.

+
     submission->setReferrer(Referrer(document().outgoingReferrer(), document().referrerPolicy()));
     submission->setOrigin(document().outgoingOrigin());
 
@@ -477,7 +487,7 @@ void HTMLFormElement::finishRequestAutocomplete(AutocompleteResult result)
 void HTMLFormElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
 {
     if (name == actionAttr)
-        m_attributes.parseAction(value);
+        m_attributes.parseAction(document(), value);
     else if (name == targetAttr)
         m_attributes.setTarget(value);
     else if (name == methodAttr)
@@ -790,4 +800,19 @@ void HTMLFormElement::setDemoted(bool demoted)
     m_wasDemoted = demoted;
 }
 
+void HTMLFormElement::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModificationReason)
+{
+    Element::attributeChanged(name, newValue);
+    if (name == actionAttr) {
+        // If the new action attribute is pointing to insecure "action" location from a secure page
+        // it is marked as "passive" mixed content. In other words, it will just
+        // show a console warning unless the user override the preferences to
+        // block all mixed content.
+        KURL actionURL = (m_attributes.action().isEmpty() ? document().url() : m_attributes.action());
+        if (MixedContentChecker::isMixedContent(document().securityOrigin(), actionURL)) {
+            document().frame()->loader().mixedContentChecker()->canSubmitToInsecureForm(document().securityOrigin(), actionURL);
+        }
+    }
+}

[abarth-chromium, 2014/06/09 22:06:23]

Please delete this function.  We don't want to override this function.

[mhm, 2014/06/09 23:27:59]

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.

[mhm, 2014/06/10 00:40:57]

Done.

+
 } // namespace