Closed
Bug 1275507
Opened 7 years ago
Closed 7 years ago
XPCOM API to create SafeBrowsing v4 update request
Categories
(Toolkit :: Safe Browsing, defect)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: hchang, Assigned: hchang)
References
Details
(Whiteboard: #sbv4-m0)
Attachments
(1 file, 14 obsolete files)
Most likely the API will be nsIUrlClassifierUtils::MakeUpdateRequestV4
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → hchang
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8756266 -
Attachment is obsolete: true
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8756805 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Attachment #8756806 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Attachment #8756807 -
Flags: review?(francois)
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8756806 -
Attachment is obsolete: true
Attachment #8756806 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Attachment #8756805 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Attachment #8756807 -
Flags: review?(francois)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8757912 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Blocks: safebrowsingv4
Assignee | ||
Updated•7 years ago
|
Attachment #8756805 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Attachment #8756807 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Attachment #8758603 -
Flags: review?(francois)
Comment 7•7 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•7 years ago
|
Attachment #8756805 -
Flags: review?(francois)
Comment 8•7 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•7 years ago
|
||
Attachment #8756805 -
Attachment is obsolete: true
Attachment #8756807 -
Attachment is obsolete: true
Attachment #8758603 -
Attachment is obsolete: true
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8769530 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8769531 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Attachment #8769539 -
Flags: review?(francois)
Assignee | ||
Comment 12•7 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•7 years ago
|
Whiteboard: [sbv4-m0]
Assignee | ||
Updated•7 years ago
|
Whiteboard: [sbv4-m0] → #sbv4-m0
Updated•7 years ago
|
Attachment #8769539 -
Flags: review?(francois) → review+
Comment 13•7 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•7 years ago
|
||
Attachment #8769531 -
Attachment is obsolete: true
Attachment #8769539 -
Attachment is obsolete: true
Assignee | ||
Comment 16•7 years ago
|
||
Assignee | ||
Updated•7 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•7 years ago
|
||
Attachment #8771224 -
Attachment is obsolete: true
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8771225 -
Attachment is obsolete: true
Attachment #8771261 -
Flags: review+
Assignee | ||
Comment 19•7 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•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/65412/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65412/
Assignee | ||
Comment 21•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/65414/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65414/
Assignee | ||
Updated•7 years ago
|
Attachment #8771260 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Attachment #8771260 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8771261 -
Attachment is obsolete: true
Assignee | ||
Comment 22•7 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•7 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•7 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+
Updated•7 years ago
|
No longer blocks: safebrowsingv4
Comment 25•7 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•7 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•7 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•7 years ago
|
Attachment #8772671 -
Attachment is obsolete: true
Comment 28•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29ead859749a
Status: NEW → RESOLVED
Closed: 7 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
•