XPCOM API to create SafeBrowsing v4 update request

RESOLVED FIXED in Firefox 50

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: hchang, Assigned: hchang)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: #sbv4-m0)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 14 obsolete attachments)

58 bytes, text/x-review-board-request
francois
: review+
Details | Review
(Assignee)

Description

2 years ago
Most likely the API will be nsIUrlClassifierUtils::MakeUpdateRequestV4
(Assignee)

Updated

2 years ago
Blocks: 1274112
(Assignee)

Updated

2 years ago
Depends on: 1275198
(Assignee)

Comment 1

2 years ago
Created attachment 8756266 [details] [diff] [review]
0001-Bug-1275507-Add-new-XPCOM-API-to-create-update-reque.patch
(Assignee)

Updated

2 years ago
Assignee: nobody → hchang
(Assignee)

Comment 2

2 years ago
Created attachment 8756805 [details] [diff] [review]
Part 1: Add nsIUrlClassifierUtils.makeUpdateRequestV4
Attachment #8756266 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
Created attachment 8756806 [details] [diff] [review]
Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl
(Assignee)

Comment 4

2 years ago
Created attachment 8756807 [details] [diff] [review]
Part 3: Test case
(Assignee)

Updated

2 years ago
Attachment #8756805 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8756806 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8756807 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Depends on: 1273412
(Assignee)

Comment 5

2 years ago
Created attachment 8757912 [details] [diff] [review]
Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl
(Assignee)

Updated

2 years ago
Attachment #8756806 - Attachment is obsolete: true
Attachment #8756806 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8756805 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8756807 - Flags: review?(francois)
(Assignee)

Comment 6

2 years ago
Created attachment 8758603 [details] [diff] [review]
Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl
Attachment #8757912 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Blocks: 1167038
(Assignee)

Updated

2 years ago
Attachment #8756805 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8756807 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8758603 - Flags: review?(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.
Attachment #8758603 - Flags: review?(francois) → review-
Attachment #8756805 - Flags: review?(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"
Attachment #8756807 - Flags: review?(francois) → review-
(Assignee)

Comment 9

2 years ago
Created attachment 8769530 [details] [diff] [review]
Part 1: Add nsIUrlClassifierUtils.makeUpdateRequestV4
Attachment #8756805 - Attachment is obsolete: true
Attachment #8756807 - Attachment is obsolete: true
Attachment #8758603 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
Created attachment 8769531 [details] [diff] [review]
Part 2: Test case
(Assignee)

Comment 11

2 years ago
Created attachment 8769539 [details] [diff] [review]
Part 1: Add nsIUrlClassifierUtils.makeUpdateRequestV4
Attachment #8769530 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8769531 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8769539 - Flags: review?(francois)
(Assignee)

Comment 12

2 years ago
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.
Whiteboard: [sbv4-m0]
(Assignee)

Updated

a year ago
Whiteboard: [sbv4-m0] → #sbv4-m0
Attachment #8769539 - Flags: review?(francois) → review+
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"
Attachment #8769531 - Flags: review?(francois) → review+
(Assignee)

Updated

a year ago
Duplicate of this bug: 1273412
(Assignee)

Comment 15

a year ago
Created attachment 8771224 [details] [diff] [review]
Part 1: "Table name" (used by v2) to "threat type" (used by v4) conversion
Attachment #8769531 - Attachment is obsolete: true
Attachment #8769539 - Attachment is obsolete: true
(Assignee)

Comment 16

a year ago
Created attachment 8771225 [details] [diff] [review]
Part 2: Functions to make update request for v4 (carry r+)
(Assignee)

Updated

a year ago
Attachment #8771225 - Attachment description: Part 2: Functions to make update request for v4 → Part 2: Functions to make update request for v4 (carry r+)
Attachment #8771225 - Flags: review+
(Assignee)

Comment 17

a year ago
Created attachment 8771260 [details] [diff] [review]
Part 1: "Table name" (used by v2) to "threat type" (used by v4) conversion
Attachment #8771224 - Attachment is obsolete: true
(Assignee)

Comment 18

a year ago
Created attachment 8771261 [details] [diff] [review]
Part 2: Functions to make update request for v4 (carry r+)
Attachment #8771225 - Attachment is obsolete: true
Attachment #8771261 - Flags: review+
(Assignee)

Comment 19

a year ago
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
Attachment #8771260 - Flags: review?(francois)
(Assignee)

Comment 20

a year ago
Created attachment 8772670 [details]
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/
(Assignee)

Comment 21

a year ago
Created attachment 8772671 [details]
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/
(Assignee)

Updated

a year ago
Attachment #8771260 - Flags: review?(francois)
(Assignee)

Updated

a year ago
Attachment #8771260 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8771261 - Attachment is obsolete: true
(Assignee)

Comment 22

a year ago
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/
Attachment #8772670 - Flags: review?(francois)
Attachment #8772671 - Flags: review?(francois)
(Assignee)

Comment 23

a year ago
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/
(Assignee)

Comment 24

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

Carry r+ from the last review.
Attachment #8772671 - Flags: review?(francois) → review+
No longer blocks: 1167038
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`
Attachment #8772670 - Flags: review?(francois) → review+
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.
Attachment #8772671 - Flags: review+
(Assignee)

Comment 27

a year ago
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/
Attachment #8772670 - Attachment description: Bug 1275507 - Part 1: Create "threat type" to "list name" conversion function. → Bug 1275507 - XPCOM API to create SB v4 update request.
(Assignee)

Updated

a year ago
Attachment #8772671 - Attachment is obsolete: true

Comment 28

a year ago
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29ead859749a
XPCOM API to create SB v4 update request. r=francois

Comment 29

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29ead859749a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

a year ago
Depends on: 1288825
No longer depends on: 1275198
Depends on: 1288840
You need to log in before you can comment on or make changes to this bug.