1275507 - XPCOM API to create SafeBrowsing v4 update request

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Henry Chang [:hchang]]

Most likely the API will be nsIUrlClassifierUtils::MakeUpdateRequestV4

Comment 1

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1275507-Add-new-XPCOM-API-to-create-update-reque.patch

Comment 2

[Henry Chang [:hchang]]

Attachment: Part 1: Add nsIUrlClassifierUtils.makeUpdateRequestV4

Comment 3

[Henry Chang [:hchang]]

Attachment: Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl

Comment 4

[Henry Chang [:hchang]]

Attachment: Part 3: Test case

Comment 5

[Henry Chang [:hchang]]

Attachment: Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl

Comment 6

[Henry Chang [:hchang]]

Attachment: Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl

Comment 7

[François Marier [:francois]]

Comment on attachment 8758603 [details] [diff] [review]
Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl

Review of attachment 8758603 [details] [diff] [review]:
-----------------------------------------------------------------

Let merge patches 1 and 2 since Part 1 is so small :)

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ +81,5 @@
> +
> +static PlatformType
> +GetPlatformType()
> +{
> +  //////////////////////////////////////////////////////////

I'd rather not import all of this Chromium stuff when we can just use our own XP_WIN, XP_MACOSX and XP_LINUX.

The only place where we'll have to be careful is Android where apparently we need to look for "#ifdef ANDROID". Eventually Android will be using a completely different Safe Browsing API anyways.

@@ +158,5 @@
> +  aListUpdateRequest->set_threat_type(aThreatType);
> +  aListUpdateRequest->set_platform_type(GetPlatformType());
> +  aListUpdateRequest->set_threat_entry_type(URL);
> +
> +  // Only RAW data is supported. TODO: Rice-Golomb encoded data.

Maybe we should file a bug for Rice Golomb encoding now and put the bug number in here.

Comment 8

[François Marier [:francois]]

Comment on attachment 8756807 [details] [diff] [review]
Part 3: Test case

Review of attachment 8756807 [details] [diff] [review]:
-----------------------------------------------------------------

That's a neat way of testing this!

::: toolkit/components/url-classifier/tests/unit/test_safebrowsing_protobuf.js
@@ +6,5 @@
> +  let requestNoList = urlUtils.makeUpdateRequestV4([], [], 0);
> +
> +  // Only one valid list name.
> +  let requestOneValid =
> +    urlUtils.makeUpdateRequestV4(["goog-phish-shavar"], ["AAAAAA"], 1);

That should be "goog-phish-proto" as per my comment in bug 1273412.

@@ +14,5 @@
> +    urlUtils.makeUpdateRequestV4(["bad-list-name"], ["AAAAAA"], 1);
> +
> +  // One valid and one invalid list name.
> +  let requestOneInvalidOneValid =
> +    urlUtils.makeUpdateRequestV4(["goog-phish-shavar", "bad-list-name"],

Also "goog-phish-proto"

Comment 9

[Henry Chang [:hchang]]

Attachment: Part 1: Add nsIUrlClassifierUtils.makeUpdateRequestV4

Comment 10

[Henry Chang [:hchang]]

Attachment: Part 2: Test case

Comment 11

[Henry Chang [:hchang]]

Attachment: Part 1: Add nsIUrlClassifierUtils.makeUpdateRequestV4

Comment 12

[Henry Chang [:hchang]]

Francois,

Your review comments have been addressed in the new patches except that I use "goog4-*-proto" in the test case. Please refer to Bug 1273412, where I am asking if we should use "goog4-*-proto" instead of "goog-*-proto". Thanks.

Comment 13

[François Marier [:francois]]

Comment on attachment 8769531 [details] [diff] [review]
Part 2: Test case

Review of attachment 8769531 [details] [diff] [review]:
-----------------------------------------------------------------

Only one thing to change: "goog4" should be "goog"

Comment 15

[Henry Chang [:hchang]]

Attachment: Part 1: "Table name" (used by v2) to "threat type" (used by v4) conversion

Comment 16

[Henry Chang [:hchang]]

Attachment: Part 2: Functions to make update request for v4 (carry r+)

Comment 17

[Henry Chang [:hchang]]

Attachment: Part 1: "Table name" (used by v2) to "threat type" (used by v4) conversion

Comment 18

[Henry Chang [:hchang]]

Attachment: Part 2: Functions to make update request for v4 (carry r+)

Comment 19

[Henry Chang [:hchang]]

Comment on attachment 8771260 [details] [diff] [review]
Part 1: "Table name" (used by v2) to "threat type" (used by v4) conversion

Hi Francois,

Yes. Chromium is using SOCIAL_ENGINEERING_PUBLIC as well. Besides, in the very first "safebrowsing.proto" you shared with us, there's only "SOCIAL_ENGINEERING", which has the same enum value '2' (same as "SOCIAL_ENGINEERING_PUBLIC" in the up-to-date safebrowsing.proto I copied from Chromium).

[1] https://cs.chromium.org/search/?q=SOCIAL_ENGINEERING_PUBLIC&sq=package:chromium&type=cs

Comment 20

[Henry Chang [:hchang]]

Attachment: Bug 1275507 - XPCOM API to create SB v4 update request.

Review commit: https://reviewboard.mozilla.org/r/65412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65412/

Comment 21

[Henry Chang [:hchang]]

Attachment: Bug 1275507 - Part 2: New nsIUrlClassifierUtils function to create update request.

Review commit: https://reviewboard.mozilla.org/r/65414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65414/

Comment 22

[Henry Chang [:hchang]]

Comment on attachment 8772670 [details]
Bug 1275507 - XPCOM API to create SB v4 update request.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65412/diff/1-2/

Comment 23

[Henry Chang [:hchang]]

Comment on attachment 8772671 [details]
Bug 1275507 - Part 2: New nsIUrlClassifierUtils function to create update request.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65414/diff/1-2/

Comment 24

[Henry Chang [:hchang]]

Comment on attachment 8772671 [details]
Bug 1275507 - Part 2: New nsIUrlClassifierUtils function to create update request.

Carry r+ from the last review.

Comment 25

[François Marier [:francois]]

Comment on attachment 8772670 [details]
Bug 1275507 - XPCOM API to create SB v4 update request.

https://reviewboard.mozilla.org/r/65412/#review62728

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:130
(Diff revision 2)
>    _retval.Append(temp);
>  
>    return NS_OK;
>  }
>  
> +// We will use "goog4-*-proto" as the list name for v4, where "goog4"

nit: the comment should also be "goog", not "goog4"

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:138
(Diff revision 2)
> +static const struct {
> +  const char* mListName;
> +  uint32_t mThreatType;
> +} THREAT_TYPE_CONV_TABLE[] = {
> +  { "goog-phish-proto",    SOCIAL_ENGINEERING_PUBLIC},
> +  { "goog-malware-proto",  MALWARE_THREAT},

nit: if these were in alphabetical order ("goog-malware" before "goog-phish") then they would also match the numerical order in `safebrowsing.proto`

Comment 26

[François Marier [:francois]]

Comment on attachment 8772671 [details]
Bug 1275507 - Part 2: New nsIUrlClassifierUtils function to create update request.

https://reviewboard.mozilla.org/r/65414/#review62730

r+ once you remove the extra return statement I pointed out.

Also, you may want to combine these two patches. They're quite small and the second doesn't work without the first.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:97
(Diff revision 2)
> +  return WINDOWS_PLATFORM;
> +#else
> +  #error Unrecognized platform type.
> +#endif
> +
> +  return PLATFORM_TYPE_UNSPECIFIED;

I think this is dead code.

In the normal case, there will be two return statements in this function.

In the case of a non-standard platform then it won't compile so we won't use this return statement.

Comment 27

[Henry Chang [:hchang]]

Comment on attachment 8772670 [details]
Bug 1275507 - XPCOM API to create SB v4 update request.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65412/diff/2-3/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29ead859749a
XPCOM API to create SB v4 update request. r=francois

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29ead859749a