Closed Bug 1275507 Opened 5 years ago Closed 5 years ago

XPCOM API to create SafeBrowsing v4 update request

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

Details

(Whiteboard: #sbv4-m0)

Attachments

(1 file, 14 obsolete files)

58 bytes, text/x-review-board-request
francois
: review+
Details
Most likely the API will be nsIUrlClassifierUtils::MakeUpdateRequestV4
Blocks: 1274112
Depends on: 1275198
Assignee: nobody → hchang
Attachment #8756266 - Attachment is obsolete: true
Attached patch Part 3: Test case (obsolete) — Splinter Review
Attachment #8756805 - Flags: review?(francois)
Attachment #8756806 - Flags: review?(francois)
Attachment #8756807 - Flags: review?(francois)
Depends on: 1273412
Attachment #8756806 - Attachment is obsolete: true
Attachment #8756806 - Flags: review?(francois)
Attachment #8756805 - Flags: review?(francois)
Attachment #8756807 - Flags: review?(francois)
Attachment #8757912 - Attachment is obsolete: true
Attachment #8756805 - Flags: review?(francois)
Attachment #8756807 - Flags: review?(francois)
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-
Attachment #8756805 - Attachment is obsolete: true
Attachment #8756807 - Attachment is obsolete: true
Attachment #8758603 - Attachment is obsolete: true
Attached patch Part 2: Test case (obsolete) — Splinter Review
Attachment #8769530 - Attachment is obsolete: true
Attachment #8769531 - Flags: review?(francois)
Attachment #8769539 - Flags: review?(francois)
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]
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+
Duplicate of this bug: 1273412
Attachment #8769531 - Attachment is obsolete: true
Attachment #8769539 - Attachment is obsolete: true
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+
Attachment #8771225 - Attachment is obsolete: true
Attachment #8771261 - Flags: review+
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)
Attachment #8771260 - Flags: review?(francois)
Attachment #8771260 - Attachment is obsolete: true
Attachment #8771261 - Attachment is obsolete: true
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)
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 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: safebrowsingv4
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+
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.
Attachment #8772671 - Attachment is obsolete: true
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29ead859749a
XPCOM API to create SB v4 update request. r=francois
https://hg.mozilla.org/mozilla-central/rev/29ead859749a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.