Closed Bug 1274112 Opened 8 years ago Closed 8 years ago

Implement Safe Browsing v4 update request

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox51 --- verified

People

(Reporter: hchang, Assigned: hchang)

References

Details

(Whiteboard: #sbv4-m0)

Attachments

(2 files, 1 obsolete file)

Safe Browsing v4 uses safebrowsing.proto to serialize the update request (and hash completion request for sure). This bug is to track if we could correctly build the update request based on safebrowsing.proto.
Assignee: nobody → hchang
Depends on: 1275195
Depends on: 1275198
Depends on: 1275507
Whiteboard: #sbv4-m0
Attached patch Bug1274112.patch (obsolete) — Splinter Review
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66336/diff/1-2/
Attachment #8773602 - Flags: review?(francois)
Attachment #8773598 - Attachment is obsolete: true
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66336/diff/2-3/
This bug is a little annoying since the actual way to send the request is not well-documented. What I can do for now is to read Chromium source code. Fortunately it's not complicated so I managed to correctly send the request and get the correct response. To have you understand the patch more quickly, I am listing the steps to send a correct request:

1) Generate a binary encoded request with protobuf API. This has been properly documented.
2) Encode the binary request to base64. Properly documented, too.
3) The update URL (hope it will not change again) is: (learned from Chromium source code...)

https://safebrowsing.googleapis.com/v4/threatListUpdates:fetch?$req=%REQUEST_BASE64%&$ct=application/x-protobuf&key=%GOOGLE_API_KEY% [1]

where %REQUEST_BASE64% should be replaced with what we encoded in step (2) and %GOOGLE_API_KEY% needs to be a real valid key.

4) Request method is "GET"
5) A extended HTTP header needs to be set: "X-HTTP-Method-Override: POST" [3]

Given that the current update request is sent by POST method, I had to do some changes to listmanager and StreamUpdater so that we can send with GET method whenever needed. Those changes are mainly

1) Add a flag to a bunch of functions and structs to indicate if the request is a POST.
2) If it is NOT a POST request, we should set "X-HTTP-Method-Override: POST"
3) We currently use "RequestBody" in many spots. Since the request might be embedded in the URL, I've changed all the "RequestBody" to "Request" except |AddRequestBody| (since it is definitely the body in that case)

There is alos a test case to this bug to verify if the correct request is assembled and sent to the server, including everything I mentioned above.

[1] https://chromium.googlesource.com/chromium/src.git/+/f7dbf39be31d8aa9214d5d84da613508d4e06491/components/safe_browsing_db/v4_update_protocol_manager.cc#355
[2] https://chromium.googlesource.com/chromium/src.git/+/f7dbf39be31d8aa9214d5d84da613508d4e06491/components/safe_browsing_db/v4_protocol_manager_util.cc#27
[3] https://chromium.googlesource.com/chromium/src.git/+/f7dbf39be31d8aa9214d5d84da613508d4e06491/components/safe_browsing_db/v4_protocol_manager_util.cc#138
Depends on: 1276595
No longer depends on: 1275198
No longer depends on: 1275195
Status: NEW → ASSIGNED
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66336/diff/3-4/
Adding Matt as a QA contact on this one. We need to verify that we haven't broken Safe Browsing V2 updates.

Whether or not V4 updates work at this point, is not as important since they're not enabled by default.
QA Contact: mwobensmith
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

https://reviewboard.mozilla.org/r/66336/#review64674

This looks really good! Kudos for reverse-engineering the _actual_ protocol :)

I've only got a few minor comments/suggestions.

::: toolkit/components/url-classifier/content/listmanager.js:358
(Diff revision 4)
>    //   request: list of tables and existing chunk ranges from tableData
>    // }
> -  var streamerMap = { tableList: null, tableNames: {}, request: "" };
> +  var streamerMap = { tableList: null,
> +                      tableNames: {},
> +                      request: "",
> +                      isPostRequest: "" };

Shouldn't that be `false` instead of `""`?

::: toolkit/components/url-classifier/content/listmanager.js:389
(Diff revision 4)
>      }
>    }
>  
>    if (useProtobuf) {
> -    // TODO: Bug 1275507 - XPCOM API to build v4 update request.
> -    streamerMap.request = "";
> +    let tableArray = streamerMap.tableList.split(',');
> +    let stateArray = [];

It would be nice if you could add a quick comment to the end of this line describing the format of the "state". Something like:

    // "" (never downloaded), "not fresh" or "fresh"

The above names are just guesses :)

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:106
(Diff revision 4)
>  ///////////////////////////////////////////////////////////////////////////////
>  // nsIUrlClassifierStreamUpdater implementation
>  
>  nsresult
>  nsUrlClassifierStreamUpdater::FetchUpdate(nsIURI *aUpdateUrl,
> -                                          const nsACString & aRequestBody,
> +                                          const nsACString & aRequest,

I see why you renamed this variable since it's no longer technically in the body of the HTTP request.

However, "request" is a little ambiguous and suggests that maybe it also contains headers.

How about "aRequestPayload"?

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:197
(Diff revision 4)
>    return NS_OK;
>  }
>  
>  nsresult
>  nsUrlClassifierStreamUpdater::FetchUpdate(const nsACString & aUpdateUrl,
> -                                          const nsACString & aRequestBody,
> +                                          const nsACString & aRequest,

Same comment here (aRequestPayload).

::: toolkit/components/url-classifier/tests/unit/test_listmanager.js:36
(Diff revision 4)
>    }
>  ];
>  
> -// This table has a different update URL.
> -const TEST_TABLE_DATA_ANOTHER = {
> -  tableName: "test-listmanageranother-digest256",
> +// This table has a different update URL (for v4).
> +const TEST_TABLE_DATA_V4 = {
> +  tableName: "goog-phish-proto",

Should we use a name like "test-phish-proto" instead to make sure we don't conflict with the real phishing list?

::: toolkit/components/url-classifier/tests/unit/test_listmanager.js:134
(Diff revision 4)
>      gUpdateResponse = "n:1000\n";
>  
> +    // We test the request against the query string since v4 request
> +    // would be appened to the query string. The request is generated
> +    // by protobuf API (binary) then encoded to base64 format.
> +    let requestV4 = gUrlUtils.makeUpdateRequestV4([TEST_TABLE_DATA_V4.tableName],

If I understand this test properly, the only thing it's testing is the conversion from a protobuf string to base64.

We're comparing the output of a function with the output of the same function. It would be surprising if it failed :)

How stable are protobuf strings? Can't we just hardcode the expected base64 string?

::: toolkit/components/url-classifier/tests/unit/test_listmanager.js:201
(Diff revision 4)
>  
> -  gHttpServAnother.registerPathHandler("/safebrowsing/update", function(request, response) {
> -    let body = NetUtil.readInputStreamToString(request.bodyInputStream,
> -                                               request.bodyInputStream.available());
> +  gHttpServV4.registerPathHandler("/safebrowsing/update", function(request, response) {
> +    // V4 update request body should be empty.
> +    equal(request.bodyInputStream.available(), 0);
>  
> -    // Verify if the request is as expected.
> +    // Not on the spec. Learn from Chromium source code...

grammar nit: "Learn from" -> "Found in"

::: toolkit/components/url-classifier/tests/unit/test_listmanager.js:210
(Diff revision 4)
> +    equal(request.method, "GET");
> +
> +    // V4 append the base64 encoded request to the query string.
> +    equal(request.queryString, gExpectedQueryV4);
> +
> +    // Respond a V2 compatible content for now.

Maybe we should include in this comment the bug number for consuming updates for real (part of milestone 1).
Attachment #8773602 - Flags: review?(francois) → review-
https://reviewboard.mozilla.org/r/66336/#review64674

> It would be nice if you could add a quick comment to the end of this line describing the format of the "state". Something like:
> 
>     // "" (never downloaded), "not fresh" or "fresh"
> 
> The above names are just guesses :)

Since the state of the table is told by the last update response and is opaque to us, I might just describe this fact in the comment :)

> I see why you renamed this variable since it's no longer technically in the body of the HTTP request.
> 
> However, "request" is a little ambiguous and suggests that maybe it also contains headers.
> 
> How about "aRequestPayload"?

Sure! I will rename it again!

> Should we use a name like "test-phish-proto" instead to make sure we don't conflict with the real phishing list?

IIRC, xpcshell-test would NOT bring up SafeBrowsing.jsm so we are safe to use this table name. However, I know it still looks scary so I will make this change. (the table name and threat type conversion needs to be updated to recognize this testing table name.)

> If I understand this test properly, the only thing it's testing is the conversion from a protobuf string to base64.
> 
> We're comparing the output of a function with the output of the same function. It would be surprising if it failed :)
> 
> How stable are protobuf strings? Can't we just hardcode the expected base64 string?

This test is mainly testing if the HTTP request is correctly assembled and sent to the server. The expected HTTP request is:

1) The request method is "GET"
2) The HTTP header "X-HTTP-Method-Override" is set to "POST"
3) The request payload (base64-encoded protobuf binary) is properly appended to the query string.

Besides, I hard-coded the base64 string in the first place actually and then changed to what you see. The reason is we are not testing the function  (makeUpdateRequestV4) but testing if the function is used correctly. The correctness of the function itself should be in other unit and we just believe the function in this test.
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66336/diff/4-5/
Attachment #8773602 - Flags: review- → review?(francois)
Attachment #8773602 - Flags: review?(francois) → review-
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

https://reviewboard.mozilla.org/r/66336/#review64918

I think you forgot to rename the test table.

::: toolkit/components/url-classifier/tests/unit/test_listmanager.js:212
(Diff revisions 4 - 5)
>      // V4 append the base64 encoded request to the query string.
>      equal(request.queryString, gExpectedQueryV4);
>  
> -    // Respond a V2 compatible content for now.
> +    // Respond a V2 compatible content for now. In the future we can
> +    // send a meaningful response to test Bug 1284178 to see if the
> +    // update is successfully stored to databse.

typo: databse
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66336/diff/5-6/
Attachment #8773602 - Flags: review- → review?(francois)
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

https://reviewboard.mozilla.org/r/66336/#review65140
Attachment #8773602 - Flags: review?(francois) → review+
Marking this bug as "Do not land" to ensure it doesn't accidentally get pulled into 50. We should land it in 51.
Summary: Implement Safe Browsing v4 update request → [DO NOT LAND] Implement Safe Browsing v4 update request
Summary: [DO NOT LAND] Implement Safe Browsing v4 update request → Implement Safe Browsing v4 update request
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66336/diff/6-7/
Review commit: https://reviewboard.mozilla.org/r/69246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69246/
Attachment #8773602 - Attachment description: Bug 1274112 - Make update request v4. → Bug 1274112 - Part 1: Make update request v4.
Attachment #8777768 - Flags: review?(francois)
Comment on attachment 8773602 [details]
Bug 1274112 - Part 1: Make update request v4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66336/diff/7-8/
Comment on attachment 8777768 [details]
Bug 1274112 - Part 2: Use protobuf API to parse v4 update response.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69246/diff/1-2/
Comment on attachment 8777768 [details]
Bug 1274112 - Part 2: Use protobuf API to parse v4 update response.

https://reviewboard.mozilla.org/r/69246/#review66526

I'm not 100% sure about these so feel free to tell me I'm wrong.

::: toolkit/components/url-classifier/ProtocolParser.cpp:707
(Diff revision 2)
>  }
>  
> +///////////////////////////////////////////////////////////////////////
> +// ProtocolParserProtobuf
> +
> +ProtocolParserProtobuf::ProtocolParserProtobuf()

Do we need to call the parent constructor to ensure that the members will be initialized properly?

https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/toolkit/components/url-classifier/ProtocolParser.cpp#64-68

::: toolkit/components/url-classifier/ProtocolParser.cpp:712
(Diff revision 2)
> +ProtocolParserProtobuf::ProtocolParserProtobuf()
> +{
> +}
> +
> +ProtocolParserProtobuf::~ProtocolParserProtobuf()
> +{

Similarly, don't we need to call the destructor from the parent's class to so that CleanupUpdates() will get called on destruction?

https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/toolkit/components/url-classifier/ProtocolParser.cpp#74
Attachment #8777768 - Flags: review?(francois)
Comment on attachment 8777768 [details]
Bug 1274112 - Part 2: Use protobuf API to parse v4 update response.

https://reviewboard.mozilla.org/r/69246/#review66534
https://reviewboard.mozilla.org/r/69246/#review66526

> Do we need to call the parent constructor to ensure that the members will be initialized properly?
> 
> https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/toolkit/components/url-classifier/ProtocolParser.cpp#64-68

If none of the parent ctors is not in the member initializer list of a sub-class, the default ctor (ctor with no arguement) will be called automatically. Since we have a default ctor (and it's the only one :)) of ProtocolParser so that's totally fine :)

> Similarly, don't we need to call the destructor from the parent's class to so that CleanupUpdates() will get called on destruction?
> 
> https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/toolkit/components/url-classifier/ProtocolParser.cpp#74

The dtor of the parent class will be automatically called before calling the subclass dtor so it's also totally fine here :)
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae2cbe1419d1
Part 1: Make update request v4. r=francois
https://hg.mozilla.org/integration/autoland/rev/09aabc8742d5
Part 2: Use protobuf API to parse v4 update response. r=francois
https://hg.mozilla.org/mozilla-central/rev/ae2cbe1419d1
https://hg.mozilla.org/mozilla-central/rev/09aabc8742d5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(In reply to François Marier [:francois] from comment #7)
> Adding Matt as a QA contact on this one. We need to verify that we haven't
> broken Safe Browsing V2 updates.
> 
> Whether or not V4 updates work at this point, is not as important since
> they're not enabled by default.

I verified that v2 Safe Browsing still works on today's Fx51.

* New profile
* Wait ~12 hours for list updates
* Use links on the test page http://testsafebrowsing.appspot.com and verify appropriate blocks
Status: RESOLVED → VERIFIED
Depends on: 1305567
You need to log in before you can comment on or make changes to this bug.