Add SafeBrowsingNavigationObserver to listen to navigation events

chrome/browser/safe_browsing/safe_browsing_navigation_observer.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#ifndef CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_NAVIGATION_OBSERVER_H_
+#define CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_NAVIGATION_OBSERVER_H_
+
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "url/gurl.h"
+
+struct RetargetingDetails;

[Charlie Reis, 2016/09/16 23:35:05]

nit: This appears unused in the .h file.

[Jialiu Lin, 2016/09/22 21:30:28]

Done.

+
+namespace content {
+class NavigationHandle;
+class NotificationRegistrar;
+class RenderFrameHost;
+struct ResourceRedirectDetails;
+}
+
+namespace safe_browsing {
+
+// Observes the navigation events that might lead to a download.
+class SafeBrowsingNavigationObserver : public content::NotificationObserver,

[Charlie Reis, 2016/09/16 23:35:05]

It looks like this is trying to have one instance that observes all WebContents,
which isn't supported. (See the WebContentsObserver::Observe documentation.)

Also, is this meant to be limited to just WebContents in normal tabs, or other
non-traditional WebContents like background pages, etc?  (Hopefully just normal
tabs.)

[Jialiu Lin, 2016/09/22 21:30:28]

Change to one observer per WebContents instead. Thanks for pointing this out. 

+                                       public content::WebContentsObserver {
+ public:
+  struct NavigationEvent {

[Charlie Reis, 2016/09/16 23:35:06]

Please have a comment with some description of how this will be used (as well as
details like whether it will track subframe navigations as well).

[Jialiu Lin, 2016/09/22 21:30:28]

Done.

+    NavigationEvent();
+    NavigationEvent(NavigationEvent&& nav_event);
+    NavigationEvent& operator=(NavigationEvent&& nav_event);
+    ~NavigationEvent();
+    GURL source_url;
+    int source_tab_id;

[Charlie Reis, 2016/09/16 23:35:05]

Please document these fields.  For example, WebContents doesn't have an ID, so
it's not immediately clear what ID this is tracking.

[Jialiu Lin, 2016/09/22 21:30:29]

Done.

+
+    GURL target_url;
+    int target_tab_id;
+
+    int frame_id;         // Frame tree node id.
+    GURL main_frame_url;  // Main frame url of the source.
+
+    double timestamp;  // Timestamp of last time this object is updated.
+
+    bool is_user_initiated;

[Charlie Reis, 2016/09/16 23:35:05]

Mention how you track this.  Does it boil down to is_browser_initiated ||
has_user_gesture?

[Jialiu Lin, 2016/09/22 21:30:28]

Done.

+    bool has_committed;
+    bool is_server_redirect;
+    bool is_finished;

[Charlie Reis, 2016/09/16 23:35:05]

What event does this refer to?  (There are many possible ways to declare
something finished.)

[Jialiu Lin, 2016/09/22 21:30:29]

This field is no longer needed. Removed. 

+    GURL server_redirect_url;

[Charlie Reis, 2016/09/16 23:35:05]

Normally we keep track of a "redirect chain" of URLs.  Which one is this?  (And
maybe it should be listed closer to is_server_redirect?)

[Jialiu Lin, 2016/09/22 21:30:28]

If there are more than one server redirect, this is the final redirect it
resolves to. 

+  };
+
+  struct GurlHash {
+    std::size_t operator()(const GURL& url) const {
+      return std::hash<std::string>()(url.spec());
+    }
+  };
+
+  typedef std::unordered_map<GURL, std::vector<NavigationEvent>, GurlHash>
+      NavigationMap;
+  typedef std::unordered_map<content::NavigationHandle*, NavigationEvent>
+      NavigationHandleMap;

[Charlie Reis, 2016/09/16 23:35:05]

nit: Switch order to be consistent with the declarations below?

Also, do these typedefs need to be public, or can they be declared private?

[Jialiu Lin, 2016/09/22 21:30:28]

NavigationMap is used in both unit tests and browser tests, so make it public is
easier.

+
+  SafeBrowsingNavigationObserver();
+
+  ~SafeBrowsingNavigationObserver() override;
+
+  void ClearNavigationMap();
+
+  // TODO(jialiul): more functions coming for managing navigation_map_.
+
+ protected:
+  void RecordNavigationPendingEntry(
+      const content::NotificationSource& source,
+      const content::NotificationDetails& details);
+
+  // Record navigation event when observes a chrome::NOTIFICATION_RETARGETING

[Charlie Reis, 2016/09/16 23:35:05]

nit: s/observes/observing/

[Jialiu Lin, 2016/09/22 21:30:29]

Done.

+  // notification. Retargeting means navigation triggered by one webcontents

[Charlie Reis, 2016/09/16 23:35:05]

nit: WebContents

[Jialiu Lin, 2016/09/22 21:30:29]

Done.

+  // finished in another. e.g. open link in a new tab/window either by user or
+  // by code.
+  void RecordRetargeting(const content::NotificationDetails& details);

[Charlie Reis, 2016/09/16 23:35:05]

Huh.  I wasn't familiar with NOTIFICATION_RETARGETING.  NotificationObserver is
deprecated, so I wonder if there's another way we should be listening for this. 
(Not sure, though, so maybe we can leave it for now.)

[Jialiu Lin, 2016/09/22 21:30:28]

I tried WebContentsObserver::DidOpenRequestedURL as suggested by Nick and Avi.
Unfortunately it is not working as expected. Let's keep it for now until I
figure out an alternative way.

+
+  // content::NotificationObserver:
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override;
+
+  // content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
+

[Charlie Reis, 2016/09/16 23:35:05]

nit: No blank lines between method override declarations.

[Jialiu Lin, 2016/09/22 21:30:29]

Done.

+  void DidRedirectNavigation(
+      content::NavigationHandle* navigation_handle) override;
+
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override;
+
+ private:
+  friend class SBNavigationObserverBrowserTest;
+  friend class SBNavigationObserverTest;
+
+  NavigationMap* navigation_map() { return &navigation_map_; }
+
+  // Map keyed on NavigationHandle* to keep track of all the ongoing navigation
+  // events. NavigationHandle pointers are owned by Navigator class. Since a

[Charlie Reis, 2016/09/16 23:35:06]

I think NavigationHandle is owned by RenderFrameHost, rather than Navigator.

[Jialiu Lin, 2016/09/22 21:30:28]

Done.

+  // NavigationHandle object will be destructed after navigation is done,
+  // corresponding entry in this map will be removed from navigation_handle_map_
+  // and added to navigation_map_ at the end of DidFinishNavigation(...).
+  NavigationHandleMap navigation_handle_map_;
+
+  // navigation_map_ keeps track of all the observed navigations. This map is
+  // keyed on target url. Since the same url can be requested multiple times
+  // across different tabs and frames, the value of this map is a vector of
+  // NavigationEvent ordered by navigation finish time. Entries in
+  // navigation_map_ will be removed if they are older than t minutes since

[Charlie Reis, 2016/09/16 23:35:05]

Please say what t is.

I still have to wonder if we're recording too much here.  Even with a small time
limit, a large number of navigations will require a fair amount more memory than
before (and any time limit could let an attacker just wait long enough for the
events to expire before proceeding with the download).

I keep wondering if there's a point at which we can forget certain navigation
events and still have what you need, but I suppose that's hard.  An attacker
could always close the original tab (or navigate it to a different page) after
opening a new tab, and we'd lose info about the original page.

[Jialiu Lin, 2016/09/22 21:30:29]

Ah, sorry, I should mention that t is a place holder here. 
I was planning to create a constant in safe_browsing_service.h and refer to that
constant in this comment.
Anyway, let me put "2 minutes" for now.

+  // their corresponding navigations finish.
+  NavigationMap navigation_map_;
+  content::NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(SafeBrowsingNavigationObserver);
+};
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_NAVIGATION_OBSERVER_H_