Status
()
People
(Reporter: hchang, Assigned: hchang)
Tracking
Firefox Tracking Flags
(firefox50 fixed)
Details
(Whiteboard: #sbv4-m0)
Attachments
(1 attachment, 14 obsolete attachments)
Most likely the API will be nsIUrlClassifierUtils::MakeUpdateRequestV4
(Assignee) | ||
Comment 1•3 years ago
|
||
Created attachment 8756266 [details] [diff] [review] 0001-Bug-1275507-Add-new-XPCOM-API-to-create-update-reque.patch
(Assignee) | ||
Updated•3 years ago
|
Assignee: nobody → hchang
(Assignee) | ||
Comment 2•3 years ago
|
||
Created attachment 8756805 [details] [diff] [review] Part 1: Add nsIUrlClassifierUtils.makeUpdateRequestV4
Attachment #8756266 -
Attachment is obsolete: true
(Assignee) | ||
Comment 3•3 years ago
|
||
Created attachment 8756806 [details] [diff] [review] Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl
(Assignee) | ||
Comment 4•3 years ago
|
||
Created attachment 8756807 [details] [diff] [review] Part 3: Test case
(Assignee) | ||
Updated•3 years ago
|
Attachment #8756805 -
Flags: review?(francois)
(Assignee) | ||
Updated•3 years ago
|
Attachment #8756806 -
Flags: review?(francois)
(Assignee) | ||
Updated•3 years ago
|
Attachment #8756807 -
Flags: review?(francois)
(Assignee) | ||
Comment 5•3 years ago
|
||
Created attachment 8757912 [details] [diff] [review] Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl
(Assignee) | ||
Updated•3 years ago
|
Attachment #8756806 -
Attachment is obsolete: true
Attachment #8756806 -
Flags: review?(francois)
(Assignee) | ||
Updated•3 years ago
|
Attachment #8756805 -
Flags: review?(francois)
(Assignee) | ||
Updated•3 years ago
|
Attachment #8756807 -
Flags: review?(francois)
(Assignee) | ||
Comment 6•3 years ago
|
||
Created attachment 8758603 [details] [diff] [review] Part 2: nsIUrlClassifierUtils.makeUpdateRequestV4 impl
Attachment #8757912 -
Attachment is obsolete: true
(Assignee) | ||
Updated•3 years ago
|
Attachment #8756805 -
Flags: review?(francois)
(Assignee) | ||
Updated•3 years ago
|
Attachment #8756807 -
Flags: review?(francois)
(Assignee) | ||
Updated•3 years ago
|
Attachment #8758603 -
Flags: review?(francois)
Comment 7•3 years ago
|
||
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-
Updated•3 years ago
|
Attachment #8756805 -
Flags: review?(francois)
Comment 8•3 years ago
|
||
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•3 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•3 years ago
|
||
Created attachment 8769531 [details] [diff] [review] Part 2: Test case
(Assignee) | ||
Comment 11•3 years ago
|
||
Created attachment 8769539 [details] [diff] [review] Part 1: Add nsIUrlClassifierUtils.makeUpdateRequestV4
Attachment #8769530 -
Attachment is obsolete: true
(Assignee) | ||
Updated•3 years ago
|
Attachment #8769531 -
Flags: review?(francois)
(Assignee) | ||
Updated•3 years ago
|
Attachment #8769539 -
Flags: review?(francois)
(Assignee) | ||
Comment 12•3 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.
Updated•3 years ago
|
Whiteboard: [sbv4-m0]
(Assignee) | ||
Updated•3 years ago
|
Whiteboard: [sbv4-m0] → #sbv4-m0
Updated•3 years ago
|
Attachment #8769539 -
Flags: review?(francois) → review+
Comment 13•3 years ago
|
||
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) | ||
Comment 15•3 years 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•3 years ago
|
||
Created attachment 8771225 [details] [diff] [review] Part 2: Functions to make update request for v4 (carry r+)
(Assignee) | ||
Updated•3 years 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•3 years 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•3 years 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•3 years 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•3 years 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•3 years 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•3 years ago
|
Attachment #8771260 -
Flags: review?(francois)
(Assignee) | ||
Updated•3 years ago
|
Attachment #8771260 -
Attachment is obsolete: true
(Assignee) | ||
Updated•3 years ago
|
Attachment #8771261 -
Attachment is obsolete: true
(Assignee) | ||
Comment 22•3 years 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•3 years 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•3 years 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+
Comment 25•3 years ago
|
||
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 26•3 years ago
|
||
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•3 years 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•3 years ago
|
Attachment #8772671 -
Attachment is obsolete: true
Comment 28•3 years 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•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29ead859749a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•