Webpages can trigger loads with invalid URLs
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 27 Apr 2014 16:06:27 +0000 (16:06 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 27 Apr 2014 16:06:27 +0000 (16:06 +0000)
https://bugs.webkit.org/show_bug.cgi?id=132224
rdar://problem/16697142

Reviewed by Alexey Proskuryakov.

Invalid URLs can be a way to trick the user about what website they
are looking at.  Still trying to figure out a good way to regression-test this.

* dom/Document.cpp:
(WebCore::Document::processHttpEquiv): Pass a URL rather than a String to
the navigation scheduler.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::receivedFirstData): Ditto.

* loader/NavigationScheduler.cpp:
(WebCore::ScheduledURLNavigation::ScheduledURLNavigation): Take a URL rather
than a string.
(WebCore::ScheduledURLNavigation::url): Ditto.
(WebCore::ScheduledRedirect::ScheduledRedirect): Ditto.
(WebCore::ScheduledLocationChange::ScheduledLocationChange): Ditto.
(WebCore::ScheduledRefresh::ScheduledRefresh): Ditto.
(WebCore::NavigationScheduler::shouldScheduleNavigation): Added a check that
prevents navigation to any URL that is invalid, except for JavaScript URLs,
which need not be valid.
(WebCore::NavigationScheduler::scheduleRedirect): Use URL instead of String.
(WebCore::NavigationScheduler::scheduleLocationChange): Use URL instead of
String. Also got rid of empty string check since empty URLs are also invalid,
and so shouldScheduleNavigation will take care of it.
(WebCore::NavigationScheduler::scheduleRefresh): Use URL instead of String.

* loader/NavigationScheduler.h: Take URL instead of String. Also removed some
unneeded incldues and uses of WTF_MAKE_NONCOPYABLE. NavigationScheduler is
already noncopyable because it has a reference for a data member, and the
disabler doesn't have any real reason to be noncopyable.

* loader/SubframeLoader.cpp:
(WebCore::SubframeLoader::loadOrRedirectSubframe): Pass a URL rather than a
String to the NavigationScheduler.
* page/DOMWindow.cpp:
(WebCore::DOMWindow::createWindow): Ditto.

* page/SecurityOrigin.cpp:
(WebCore::SecurityOrigin::urlWithUniqueSecurityOrigin): Return a URL instead
of a String.
* page/SecurityOrigin.h: Updated for above change.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@167856 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/NavigationScheduler.cpp
Source/WebCore/loader/NavigationScheduler.h
Source/WebCore/loader/SubframeLoader.cpp
Source/WebCore/page/DOMWindow.cpp
Source/WebCore/page/SecurityOrigin.cpp
Source/WebCore/page/SecurityOrigin.h

index 7b348f2..d344a18 100644 (file)
@@ -1,3 +1,52 @@
+2014-04-27  Darin Adler  <darin@apple.com>
+
+        Webpages can trigger loads with invalid URLs
+        https://bugs.webkit.org/show_bug.cgi?id=132224
+        rdar://problem/16697142
+
+        Reviewed by Alexey Proskuryakov.
+
+        Invalid URLs can be a way to trick the user about what website they
+        are looking at.  Still trying to figure out a good way to regression-test this.
+
+        * dom/Document.cpp:
+        (WebCore::Document::processHttpEquiv): Pass a URL rather than a String to
+        the navigation scheduler.
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::receivedFirstData): Ditto.
+
+        * loader/NavigationScheduler.cpp:
+        (WebCore::ScheduledURLNavigation::ScheduledURLNavigation): Take a URL rather
+        than a string.
+        (WebCore::ScheduledURLNavigation::url): Ditto.
+        (WebCore::ScheduledRedirect::ScheduledRedirect): Ditto.
+        (WebCore::ScheduledLocationChange::ScheduledLocationChange): Ditto.
+        (WebCore::ScheduledRefresh::ScheduledRefresh): Ditto.
+        (WebCore::NavigationScheduler::shouldScheduleNavigation): Added a check that
+        prevents navigation to any URL that is invalid, except for JavaScript URLs,
+        which need not be valid.
+        (WebCore::NavigationScheduler::scheduleRedirect): Use URL instead of String.
+        (WebCore::NavigationScheduler::scheduleLocationChange): Use URL instead of
+        String. Also got rid of empty string check since empty URLs are also invalid,
+        and so shouldScheduleNavigation will take care of it.
+        (WebCore::NavigationScheduler::scheduleRefresh): Use URL instead of String.
+
+        * loader/NavigationScheduler.h: Take URL instead of String. Also removed some
+        unneeded incldues and uses of WTF_MAKE_NONCOPYABLE. NavigationScheduler is
+        already noncopyable because it has a reference for a data member, and the
+        disabler doesn't have any real reason to be noncopyable.
+
+        * loader/SubframeLoader.cpp:
+        (WebCore::SubframeLoader::loadOrRedirectSubframe): Pass a URL rather than a
+        String to the NavigationScheduler.
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::createWindow): Ditto.
+
+        * page/SecurityOrigin.cpp:
+        (WebCore::SecurityOrigin::urlWithUniqueSecurityOrigin): Return a URL instead
+        of a String.
+        * page/SecurityOrigin.h: Updated for above change.
+
 2014-04-27  Zan Dobersek  <zdobersek@igalia.com>
 
         ScriptExecutionContext::Task should work well with C++11 lambdas
index b7c17ed..1520eb3 100644 (file)
@@ -2824,14 +2824,15 @@ void Document::processHttpEquiv(const String& equiv, const String& content)
         styleResolverChanged(DeferRecalcStyle);
     } else if (equalIgnoringCase(equiv, "refresh")) {
         double delay;
-        String url;
-        if (frame && parseHTTPRefresh(content, true, delay, url)) {
-            if (url.isEmpty())
-                url = m_url.string();
+        String urlString;
+        if (frame && parseHTTPRefresh(content, true, delay, urlString)) {
+            URL completedURL;
+            if (urlString.isEmpty())
+                completedURL = m_url;
             else
-                url = completeURL(url).string();
-            if (!protocolIsJavaScript(url))
-                frame->navigationScheduler().scheduleRedirect(delay, url);
+                completedURL = completeURL(urlString);
+            if (!protocolIsJavaScript(completedURL))
+                frame->navigationScheduler().scheduleRedirect(delay, completedURL);
             else {
                 String message = "Refused to refresh " + m_url.stringCenterEllipsizedToLength() + " to a javascript: URL";
                 addConsoleMessage(MessageSource::Security, MessageLevel::Error, message);
index e3a5dad..a148925 100644 (file)
@@ -666,16 +666,17 @@ void FrameLoader::receivedFirstData()
         return;
 
     double delay;
-    String url;
-    if (!parseHTTPRefresh(m_documentLoader->response().httpHeaderField("Refresh"), false, delay, url))
+    String urlString;
+    if (!parseHTTPRefresh(m_documentLoader->response().httpHeaderField("Refresh"), false, delay, urlString))
         return;
-    if (url.isEmpty())
-        url = m_frame.document()->url().string();
+    URL completedURL;
+    if (urlString.isEmpty())
+        completedURL = m_frame.document()->url();
     else
-        url = m_frame.document()->completeURL(url).string();
+        completedURL = m_frame.document()->completeURL(urlString);
 
-    if (!protocolIsJavaScript(url))
-        m_frame.navigationScheduler().scheduleRedirect(delay, url);
+    if (!protocolIsJavaScript(completedURL))
+        m_frame.navigationScheduler().scheduleRedirect(delay, completedURL);
     else {
         String message = "Refused to refresh " + m_frame.document()->url().stringCenterEllipsizedToLength() + " to a javascript: URL";
         m_frame.document()->addConsoleMessage(MessageSource::Security, MessageLevel::Error, message);
index ff59500..350567e 100644 (file)
@@ -97,7 +97,7 @@ private:
 
 class ScheduledURLNavigation : public ScheduledNavigation {
 protected:
-    ScheduledURLNavigation(double delay, SecurityOrigin* securityOrigin, const String& url, const String& referrer, LockHistory lockHistory, LockBackForwardList lockBackForwardList, bool duringLoad, bool isLocationChange)
+    ScheduledURLNavigation(double delay, SecurityOrigin* securityOrigin, const URL& url, const String& referrer, LockHistory lockHistory, LockBackForwardList lockBackForwardList, bool duringLoad, bool isLocationChange)
         : ScheduledNavigation(delay, lockHistory, lockBackForwardList, duringLoad, isLocationChange)
         , m_securityOrigin(securityOrigin)
         , m_url(url)
@@ -109,7 +109,7 @@ protected:
     virtual void fire(Frame& frame) override
     {
         UserGestureIndicator gestureIndicator(wasUserGesture() ? DefinitelyProcessingUserGesture : DefinitelyNotProcessingUserGesture);
-        frame.loader().changeLocation(m_securityOrigin.get(), URL(ParsedURLString, m_url), m_referrer, lockHistory(), lockBackForwardList(), false);
+        frame.loader().changeLocation(m_securityOrigin.get(), m_url, m_referrer, lockHistory(), lockBackForwardList(), false);
     }
 
     virtual void didStartTimer(Frame& frame, Timer<NavigationScheduler>& timer) override
@@ -119,7 +119,7 @@ protected:
         m_haveToldClient = true;
 
         UserGestureIndicator gestureIndicator(wasUserGesture() ? DefinitelyProcessingUserGesture : DefinitelyNotProcessingUserGesture);
-        frame.loader().clientRedirected(URL(ParsedURLString, m_url), delay(), currentTime() + timer.nextFireInterval(), lockBackForwardList());
+        frame.loader().clientRedirected(m_url, delay(), currentTime() + timer.nextFireInterval(), lockBackForwardList());
     }
 
     virtual void didStopTimer(Frame& frame, bool newLoadInProgress) override
@@ -137,19 +137,19 @@ protected:
     }
 
     SecurityOrigin* securityOrigin() const { return m_securityOrigin.get(); }
-    String url() const { return m_url; }
+    const URL& url() const { return m_url; }
     String referrer() const { return m_referrer; }
 
 private:
     RefPtr<SecurityOrigin> m_securityOrigin;
-    String m_url;
+    URL m_url;
     String m_referrer;
     bool m_haveToldClient;
 };
 
 class ScheduledRedirect : public ScheduledURLNavigation {
 public:
-    ScheduledRedirect(double delay, SecurityOrigin* securityOrigin, const String& url, LockHistory lockHistory, LockBackForwardList lockBackForwardList)
+    ScheduledRedirect(double delay, SecurityOrigin* securityOrigin, const URL& url, LockHistory lockHistory, LockBackForwardList lockBackForwardList)
         : ScheduledURLNavigation(delay, securityOrigin, url, String(), lockHistory, lockBackForwardList, false, false)
     {
         clearUserGesture();
@@ -163,20 +163,20 @@ public:
     virtual void fire(Frame& frame) override
     {
         UserGestureIndicator gestureIndicator(wasUserGesture() ? DefinitelyProcessingUserGesture : DefinitelyNotProcessingUserGesture);
-        bool refresh = equalIgnoringFragmentIdentifier(frame.document()->url(), URL(ParsedURLString, url()));
-        frame.loader().changeLocation(securityOrigin(), URL(ParsedURLString, url()), referrer(), lockHistory(), lockBackForwardList(), refresh);
+        bool refresh = equalIgnoringFragmentIdentifier(frame.document()->url(), url());
+        frame.loader().changeLocation(securityOrigin(), url(), referrer(), lockHistory(), lockBackForwardList(), refresh);
     }
 };
 
 class ScheduledLocationChange : public ScheduledURLNavigation {
 public:
-    ScheduledLocationChange(SecurityOrigin* securityOrigin, const String& url, const String& referrer, LockHistory lockHistory, LockBackForwardList lockBackForwardList, bool duringLoad)
+    ScheduledLocationChange(SecurityOrigin* securityOrigin, const URL& url, const String& referrer, LockHistory lockHistory, LockBackForwardList lockBackForwardList, bool duringLoad)
         : ScheduledURLNavigation(0.0, securityOrigin, url, referrer, lockHistory, lockBackForwardList, duringLoad, true) { }
 };
 
 class ScheduledRefresh : public ScheduledURLNavigation {
 public:
-    ScheduledRefresh(SecurityOrigin* securityOrigin, const String& url, const String& referrer)
+    ScheduledRefresh(SecurityOrigin* securityOrigin, const URL& url, const String& referrer)
         : ScheduledURLNavigation(0.0, securityOrigin, url, referrer, LockHistory::Yes, LockBackForwardList::Yes, false, true)
     {
     }
@@ -184,7 +184,7 @@ public:
     virtual void fire(Frame& frame) override
     {
         UserGestureIndicator gestureIndicator(wasUserGesture() ? DefinitelyProcessingUserGesture : DefinitelyNotProcessingUserGesture);
-        frame.loader().changeLocation(securityOrigin(), URL(ParsedURLString, url()), referrer(), lockHistory(), lockBackForwardList(), true);
+        frame.loader().changeLocation(securityOrigin(), url(), referrer(), lockHistory(), lockBackForwardList(), true);
     }
 };
 
@@ -304,12 +304,18 @@ inline bool NavigationScheduler::shouldScheduleNavigation() const
     return m_frame.page();
 }
 
-inline bool NavigationScheduler::shouldScheduleNavigation(const String& url) const
+inline bool NavigationScheduler::shouldScheduleNavigation(const URL& url) const
 {
-    return shouldScheduleNavigation() && (protocolIsJavaScript(url) || NavigationDisablerForBeforeUnload::isNavigationAllowed());
+    if (!shouldScheduleNavigation())
+        return false;
+    if (protocolIsJavaScript(url))
+        return true;
+    if (!url.isValid())
+        return false;
+    return NavigationDisablerForBeforeUnload::isNavigationAllowed();
 }
 
-void NavigationScheduler::scheduleRedirect(double delay, const String& url)
+void NavigationScheduler::scheduleRedirect(double delay, const URL& url)
 {
     if (!shouldScheduleNavigation(url))
         return;
@@ -343,12 +349,10 @@ LockBackForwardList NavigationScheduler::mustLockBackForwardList(Frame& targetFr
     return LockBackForwardList::No;
 }
 
-void NavigationScheduler::scheduleLocationChange(SecurityOrigin* securityOrigin, const String& url, const String& referrer, LockHistory lockHistory, LockBackForwardList lockBackForwardList)
+void NavigationScheduler::scheduleLocationChange(SecurityOrigin* securityOrigin, const URL& url, const String& referrer, LockHistory lockHistory, LockBackForwardList lockBackForwardList)
 {
     if (!shouldScheduleNavigation(url))
         return;
-    if (url.isEmpty())
-        return;
 
     if (lockBackForwardList == LockBackForwardList::No)
         lockBackForwardList = mustLockBackForwardList(m_frame);
@@ -401,7 +405,7 @@ void NavigationScheduler::scheduleRefresh()
     if (url.isEmpty())
         return;
 
-    schedule(std::make_unique<ScheduledRefresh>(m_frame.document()->securityOrigin(), url.string(), m_frame.loader().outgoingReferrer()));
+    schedule(std::make_unique<ScheduledRefresh>(m_frame.document()->securityOrigin(), url, m_frame.loader().outgoingReferrer()));
 }
 
 void NavigationScheduler::scheduleHistoryNavigation(int steps)
index 6ccdbca..7db21a7 100644 (file)
@@ -34,8 +34,6 @@
 #include "FrameLoaderTypes.h"
 #include "Timer.h"
 #include <wtf/Forward.h>
-#include <wtf/Noncopyable.h>
-#include <wtf/PassRefPtr.h>
 
 namespace WebCore {
 
@@ -43,10 +41,9 @@ class FormSubmission;
 class Frame;
 class ScheduledNavigation;
 class SecurityOrigin;
+class URL;
 
 class NavigationDisablerForBeforeUnload {
-    WTF_MAKE_NONCOPYABLE(NavigationDisablerForBeforeUnload);
-
 public:
     NavigationDisablerForBeforeUnload()
     {
@@ -64,8 +61,6 @@ private:
 };
 
 class NavigationScheduler {
-    WTF_MAKE_NONCOPYABLE(NavigationScheduler);
-
 public:
     explicit NavigationScheduler(Frame&);
     ~NavigationScheduler();
@@ -73,9 +68,8 @@ public:
     bool redirectScheduledDuringLoad();
     bool locationChangePending();
 
-    void scheduleRedirect(double delay, const String& url);
-    void scheduleLocationChange(SecurityOrigin*, const String& url, const String& referrer, LockHistory = LockHistory::Yes,
-        LockBackForwardList = LockBackForwardList::Yes);
+    void scheduleRedirect(double delay, const URL&);
+    void scheduleLocationChange(SecurityOrigin*, const URL&, const String& referrer, LockHistory = LockHistory::Yes, LockBackForwardList = LockBackForwardList::Yes);
     void scheduleFormSubmission(PassRefPtr<FormSubmission>);
     void scheduleRefresh();
     void scheduleHistoryNavigation(int steps);
@@ -87,7 +81,7 @@ public:
 
 private:
     bool shouldScheduleNavigation() const;
-    bool shouldScheduleNavigation(const String& url) const;
+    bool shouldScheduleNavigation(const URL&) const;
 
     void timerFired(Timer<NavigationScheduler>&);
     void schedule(std::unique_ptr<ScheduledNavigation>);
index 655dcf5..8b96def 100644 (file)
@@ -324,7 +324,7 @@ Frame* SubframeLoader::loadOrRedirectSubframe(HTMLFrameOwnerElement& ownerElemen
 {
     Frame* frame = ownerElement.contentFrame();
     if (frame)
-        frame->navigationScheduler().scheduleLocationChange(m_frame.document()->securityOrigin(), url.string(), m_frame.loader().outgoingReferrer(), lockHistory, lockBackForwardList);
+        frame->navigationScheduler().scheduleLocationChange(m_frame.document()->securityOrigin(), url, m_frame.loader().outgoingReferrer(), lockHistory, lockBackForwardList);
     else
         frame = loadSubframe(ownerElement, url, frameName, m_frame.loader().outgoingReferrer());
 
index 7f9aaf8..f206dd3 100644 (file)
@@ -2018,7 +2018,7 @@ PassRefPtr<Frame> DOMWindow::createWindow(const String& urlString, const AtomicS
         newFrame->loader().changeLocation(activeWindow.document()->securityOrigin(), completedURL, referrer, LockHistory::No, LockBackForwardList::No);
     else if (!urlString.isEmpty()) {
         LockHistory lockHistory = ScriptController::processingUserGesture() ? LockHistory::No : LockHistory::Yes;
-        newFrame->navigationScheduler().scheduleLocationChange(activeWindow.document()->securityOrigin(), completedURL.string(), referrer, lockHistory, LockBackForwardList::No);
+        newFrame->navigationScheduler().scheduleLocationChange(activeWindow.document()->securityOrigin(), completedURL, referrer, lockHistory, LockBackForwardList::No);
     }
 
     // Navigating the new frame could result in it being detached from its page by a navigation policy delegate.
index 28c711f..c47b62b 100644 (file)
@@ -36,6 +36,7 @@
 #include "SecurityPolicy.h"
 #include "ThreadableBlobRegistry.h"
 #include <wtf/MainThread.h>
+#include <wtf/NeverDestroyed.h>
 #include <wtf/StdLibExtras.h>
 #include <wtf/text/StringBuilder.h>
 
@@ -597,10 +598,10 @@ bool SecurityOrigin::isSameSchemeHostPort(const SecurityOrigin* other) const
     return true;
 }
 
-String SecurityOrigin::urlWithUniqueSecurityOrigin()
+URL SecurityOrigin::urlWithUniqueSecurityOrigin()
 {
     ASSERT(isMainThread());
-    DEPRECATED_DEFINE_STATIC_LOCAL(const String, uniqueSecurityOriginURL, (ASCIILiteral("data:,")));
+    static NeverDestroyed<URL> uniqueSecurityOriginURL(ParsedURLString, ASCIILiteral("data:,"));
     return uniqueSecurityOriginURL;
 }
 
index eb70550..755f0a0 100644 (file)
@@ -205,7 +205,7 @@ public:
     // (and whether it was set) but considering the host. It is used for postMessage.
     bool isSameSchemeHostPort(const SecurityOrigin*) const;
 
-    static String urlWithUniqueSecurityOrigin();
+    static URL urlWithUniqueSecurityOrigin();
 
 private:
     SecurityOrigin();