402610 - support redirected chunk data in safebrowsing updates

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Dave Camp (:dcamp)]

Google is considering updating the safebrowsing protocol to allow inline chunk data to be replaced with a URL for the chunk data.

We'll need to update the url-classifier service to deal with that correctly.

Comment 1

[Mike Connor [:mconnor]]

Will block on this if it goes forward, wanted to keep it on radar.

Comment 2

[Dave Camp (:dcamp)]

Attachment: worker proxy helper

This is a small helper patch that I'll be using in the actual patch.

This patch allocates a single proxy for accessing the worker thread and reuses it, rather than allocating a new proxy for each worker thread request.

Comment 3

[Dave Camp (:dcamp)]

Attachment: v1

This patch adds support for u: lines as specified in v2.1 of the safebrowsing spec.  I haven't been able to test it against google's live server (it isn't running quite yet) so I'm not asking for review yet.

Comment 4

[Dave Camp (:dcamp)]

re-asking for blocking-firefox3.  This change will be happening on google's servers shortly.

Comment 5

[Dave Camp (:dcamp)]

Comment on attachment 293216 [details] [diff] [review]
v1

The servers are now up and running and this patch has been tested against it, so I'm asking for review.

Comment 6

[Tony Chang (Google)]

Comment on attachment 293216 [details] [diff] [review]
v1

>diff -r 0d7cd50fd002 toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl
> /**
>+ * The nsIUrlClassifierUpdateObserver interface is implemented by
>+ * clients streaming updates to the url-classifier (usually
>+ * nsUrlClassifierStreamUpdater.
>+ */
>+[scriptable, uuid(113671b8-c5cc-47d9-bc57-269568c7ce29)]
>+interface nsIUrlClassifierUpdateObserver : nsISupports {

Nice refactoring into an interface.

>   /**
>+   * Begin an update process.
>+   */
>+  void beginUpdate(in nsIUrlClassifierUpdateObserver updater);

It might be nice to add some comments explaining updates and streams.

>   /**
>+   * Finish an individual stream update.  Must be called for every
>+   * beginStream() call, before the next bingStream() or finishUpdate().
>+   *
>+   * The update observer's streamFinished will be called once the
>+   * stream has been processed.

Typo: bingStream.

>diff -r 0d7cd50fd002 toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp
>+NS_IMETHODIMP
>+nsUrlClassifierStreamUpdater::StreamFinished()
>+{
>+  nsresult rv;
>+
>+  // Pop off a pending URL and update it.
>+  if (mPendingUpdateUrls.Length() > 0) {
>+    rv = FetchUpdate(mPendingUpdateUrls[0], NS_LITERAL_CSTRING(""));
>+    if (NS_FAILED(rv)) {
>+      LOG(("Error fetching update url: %s\n", mPendingUpdateUrls[0].get()));
>+      mDBService->CancelUpdate();
>+      return rv;
>+    }

It looks like a malformed URL will cause the whole update to be canceled.  Is that intended (i.e., maybe we should just ignore and try the next pending update?).

Also, shouldn't this be a while loop so we try all the URLs?  It would be nice to add a unittest for this case.

>+NS_IMETHODIMP
>+nsUrlClassifierStreamUpdater::UpdateSuccess(PRUint32 requestedTimeout)
>+{
>+  LOG(("nsUrlClassifierStreamUpdater::UpdateSuccess [this=%p]", this));
>+  NS_ASSERTION(mPendingUpdateUrls.Length() == 0,
>+               "Didn't fetch all update URLs.");
>+
>+  nsCOMPtr<nsIUrlClassifierCallback> successCallback = mSuccessCallback;
>+  DownloadDone();

There should be a comment explaining you need to cache mSuccessCallback because DownloadDone clears it.

>+NS_IMETHODIMP
>+nsUrlClassifierStreamUpdater::UpdateError(PRUint32 result)
>+{
>+  LOG(("nsUrlClassifierStreamUpdater::UpdateSuccess [this=%p]", this));
>+
>+  nsCOMPtr<nsIUrlClassifierCallback> errorCallback = mUpdateErrorCallback;
>+  DownloadDone();

Same as above.

Comment 7

[Dave Camp (:dcamp)]

Attachment: v2

Comment 8

[Dave Camp (:dcamp)]

(In reply to comment #6)

> It might be nice to add some comments explaining updates and streams.

Added.

> Typo: bingStream.

Fixed.

> It looks like a malformed URL will cause the whole update to be canceled.  Is
> that intended (i.e., maybe we should just ignore and try the next pending
> update?).

As discussed on IRC, this was intended.

> Also, shouldn't this be a while loop so we try all the URLs?  It would be nice
> to add a unittest for this case.

We don't need a while loop here.  We only start the download for one stream at a time.

> There should be a comment explaining you need to cache mSuccessCallback because
> DownloadDone clears it.

Fixed, along with the UpdateError() one.

Comment 9

[Dave Camp (:dcamp)]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.252; previous revision: 1.251
done
Checking in browser/components/safebrowsing/content/malware-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/malware-warden.js,v  <--  malware-warden.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl,v  <--  nsIUrlClassifierDBService.idl
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v  <--  nsUrlClassifierDBService.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.h,v  <--  nsUrlClassifierDBService.h
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp,v  <--  nsUrlClassifierStreamUpdater.cpp
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.h,v  <--  nsUrlClassifierStreamUpdater.h
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/components/url-classifier/tests/unit/head_urlclassifier.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js,v  <--  head_urlclassifier.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/url-classifier/tests/unit/test_addsub.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_addsub.js,v  <--  test_addsub.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/url-classifier/tests/unit/test_dbservice.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_dbservice.js,v  <--  test_dbservice.js
new revision: 1.9; previous revision: 1.8
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_streamupdater.js,v
done
Checking in toolkit/components/url-classifier/tests/unit/test_streamupdater.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_streamupdater.js,v  <--  test_streamupdater.js
initial revision: 1.1
done