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

Source/core/html/HTMLFormElement.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 19 matching lines @@
 #include "bindings/v8/ScriptController.h"
 #include "bindings/v8/ScriptEventListener.h"
 #include "core/dom/Attribute.h"
 #include "core/dom/Document.h"
 #include "core/dom/ElementTraversal.h"
 #include "core/dom/IdTargetObserverRegistry.h"
 #include "core/events/AutocompleteErrorEvent.h"
 #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"
 #include "core/html/HTMLCollection.h"
 #include "core/html/HTMLDialogElement.h"
 #include "core/html/HTMLImageElement.h"
 #include "core/html/HTMLInputElement.h"
 #include "core/html/HTMLObjectElement.h"
 #include "core/html/RadioNodeList.h"
 #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;
 
 namespace WebCore {
 
 using namespace HTMLNames;
 
+namespace {
+
+KURL getActionURL(const Document& document, const String& action)
+{
+    return action.isEmpty() ? document.url() : document.completeURL(action);
+}
+
+} // namespace
+
 HTMLFormElement::HTMLFormElement(Document& document)
     : HTMLElement(formTag, document)
 #if !ENABLE(OILPAN)
     , m_weakPtrFactory(this)
 #endif
     , m_associatedElementsAreDirty(false)
     , m_imageElementsAreDirty(false)
     , m_hasElementsAssociatedByParser(false)
     , m_didFinishParsingChildren(false)
     , m_wasUserSubmitted(false)
@@ skipping 312 matching lines @@
     ASSERT(submission->data());
     ASSERT(submission->state());
     if (submission->action().isEmpty())
         return;
     if (document().isSandboxed(SandboxForms)) {
         // FIXME: This message should be moved off the console once a solution to https://bugs.webkit.org/show_bug.cgi?id=103274 exists.
         document().addConsoleMessage(SecurityMessageSource, ErrorMessageLevel, "Blocked form submission to '" + submission->action().elidedString() + "' because the form's frame is sandboxed and the 'allow-forms' permission is not set.");
         return;
     }
 
+    KURL actionURL = getActionURL(document(), m_attributes.action());

[abarth-chromium, 2014/06/09 16:31:37]

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.

[mhm, 2014/06/09 20:20:35]

Done.

+    if (MixedContentChecker::isMixedContent(document().securityOrigin(), actionURL))
+        UseCounter::count(document(), UseCounter::MixedContentSubmittedForm);
+    if (!document().frame()->loader().mixedContentChecker()->canSubmitToInsecureForm(document().securityOrigin(), actionURL))
+        return;
+
     if (protocolIsJavaScript(submission->action())) {
         if (!document().contentSecurityPolicy()->allowFormAction(KURL(submission->action())))

[abarth-chromium, 2014/06/09 16:31:37]

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.

[mhm, 2014/06/09 20:20:35]

Done.

             return;
         document().frame()->script().executeScriptIfJavaScriptURL(submission->action());
         return;
     }
 
     LocalFrame* targetFrame = document().frame()->loader().findFrameForNavigation(submission->target(), submission->state()->sourceDocument());
     if (!targetFrame) {
         if (!DOMWindow::allowPopUp(*document().frame()) && !UserGestureIndicator::processingUserGesture())
             return;
         targetFrame = document().frame();
@@ skipping 377 matching lines @@
     returnValue0 = radioNodeList(name, onlyMatchImg);
 }
 
 void HTMLFormElement::setDemoted(bool demoted)
 {
     if (demoted)
         UseCounter::count(document(), UseCounter::DemotedFormElement);
     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 = getActionURL(document(), m_attributes.action());
+        if (MixedContentChecker::isMixedContent(document().securityOrigin(), actionURL)) {
+            document().frame()->loader().mixedContentChecker()->canSubmitToInsecureForm(document().securityOrigin(), actionURL);
+            UseCounter::count(document(), UseCounter::MixedContentForm);
+        }
+    }
+}

[abarth-chromium, 2014/06/09 16:31:37]

Please remove this part of the change.  I don't think we need to count this
information and it makes the engine slower.

[mhm, 2014/06/09 20:20:35]

Done.

+
 } // namespace