Closed
Bug 1351147
Opened 4 years ago
Closed 3 years ago
Support v4/ThreatHit request in Safe Browsing V4
Categories
(Toolkit :: Safe Browsing, enhancement, P2)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: tnguyen, Assigned: tnguyen)
References
(Depends on 2 open bugs, Blocks 2 open bugs)
Details
(Whiteboard: pwphish-prep)
Attachments
(4 files, 1 obsolete file)
http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/url-classifier/chromium/safebrowsing.proto#201-246 The protobuf ThreatHit is used to send a report of how client arrived at the unsafe url.
Updated•4 years ago
|
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee | ||
Comment 1•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Attachment #8873767 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•4 years ago
|
||
I tested manually and get 200 OK when trying to send v4/ThreatHits. But I did not add ClientInfo and UserInfo to the request body. The ClientInfo and UserInfo are included in https://intranet.mozilla.org/SafeBrowsing, but not included in the last Chromium code. Those fields have not added to Safe Browsing spec yet, not sure if google server has supported them yet
Assignee | ||
Comment 5•4 years ago
|
||
After our discussion, Google server is supposed to support ClientInfo and UserInfo that are not included in Chrome // Client-reported identification. ClientInfo client_info = 5; // Details about the user that encountered the threat. message UserInfo { // The UN M.49 region code associated with the user's location. string region_code = 1; // Unique ID stable over a week or two bytes user_id = 2; } // Details about the user that encountered the threat. UserInfo user_info = 6; I see ISO 3166 is what we are using in Firefox, we send a request to Mozilla Location Service server http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/modules/libpref/init/all.js#5446 And get the response in ISO 3166 standard https://mozilla.github.io/ichnaea/api/region.html#api-region-latest Francois, could we change to use ISO 3166 instead of UN M.49?
Flags: needinfo?(francois)
Comment 6•4 years ago
|
||
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #5) > I see ISO 3166 is what we are using in Firefox, we send a request to Mozilla > Location Service server > http://searchfox.org/mozilla-central/rev/ > 20963d7269b1b14d455f47bc0260d0653015bf84/modules/libpref/init/all.js#5446 > And get the response in ISO 3166 standard > https://mozilla.github.io/ichnaea/api/region.html#api-region-latest > Francois, could we change to use ISO 3166 instead of UN M.49? The problem is that using a country code is too specific. Some countries are too small and so these users will stand out. M.49 allows us to send the region instead of the country. So we would need a mapping from countries to regions.
Flags: needinfo?(francois)
Assignee | ||
Comment 7•4 years ago
|
||
(In reply to François Marier [:francois] from comment #6) > The problem is that using a country code is too specific. Some countries are > too small and so these users will stand out. M.49 allows us to send the > region instead of the country. > > So we would need a mapping from countries to regions. Ah, I see, thanks Francois
Comment 8•4 years ago
|
||
The updated ThreatSourceType enum (see https://mana.mozilla.org/wiki/display/FIREFOX/Safe+Browsing) has a TAB_RESOURCE for any redirects encountered while fetching a bad subresource.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•4 years ago
|
||
(In reply to François Marier [:francois] from comment #8) > The updated ThreatSourceType enum (see > https://mana.mozilla.org/wiki/display/FIREFOX/Safe+Browsing) has a > TAB_RESOURCE for any redirects encountered while fetching a bad subresource. ... // A resource loaded within the final TAB_URL. TAB_RESOURCE = 4; It looks like google wants us to send all sub-resource loadings of a warning page (eg all images, iframes, styles on the warning page). Seems quite a lot of things to do
Comment 13•4 years ago
|
||
mozreview-review |
Comment on attachment 8874777 [details] Bug 1351147 - Update protobuf to support threatHit api https://reviewboard.mozilla.org/r/146020/#review170430
Attachment #8874777 -
Flags: review?(francois) → review+
Comment 14•4 years ago
|
||
mozreview-review |
Comment on attachment 8874778 [details] Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete https://reviewboard.mozilla.org/r/146022/#review170434 Thank you for breaking this up into a separate commit! I'm thinking that we should probably use something like `aFullHash` instead of `aCompletion`. Completions are V2 jargon and it makes the code harder to understand for anybody outside of our team. ::: commit-message-756c9:1 (Diff revision 2) > +Bug 1351147 - Use completion instead of prefix in OnClassifyComplete Can you describe in a few words why this change is needed? Also, because a given prefix can map to multiple full hashes, I think it's worth pointing out that we can provide the completion here because the (list, provider, completion) tuple is only filled in when the URL matches. Is that right? ::: netwerk/base/nsIClassifiedChannel.idl:26 (Diff revision 2) > * @param aList > * Name of the Safe Browsing list that matched (e.g. goog-phish-shavar). > * @param aProvider > * Name of the Safe Browsing provider that matched (e.g. google) > - * @param aPrefix > - * Hash prefix of URL that matched Safe Browsing list. > + * @param aCompletion > + * Completion hash of URL that matched Safe Browsing list. "Full hash of" ::: netwerk/base/nsIParentChannel.idl:44 (Diff revision 2) > * @param aList > * Name of the list that matched > * @param aProvider > * Name of provider that matched > - * @param aPrefix > - * String represents hash prefix that matched > + * @param aCompletion > + * String represents completion hash that matched "Full hash that matched" ::: netwerk/base/nsIURIClassifier.idl:38 (Diff revision 2) > * @param aList > * Name of the list that matched > * @param aProvider > * Name of provider that matched > - * @param aPrefix > - * Hash prefix of URL that matched > + * @param aCompletion > + * Completion hash of URL that matched "Full hash of URL that matched" ::: toolkit/components/url-classifier/tests/mochitest/test_classifier_match.html:165 (Diff revision 2) > let prin = ssm.createCodebasePrincipal(uri, {}); > - SpecialPowers.doUrlClassify(prin, null, false, function(errorCode, table, provider, prefix) { > + SpecialPowers.doUrlClassify(prin, null, false, function(errorCode, table, provider, completion) { > is(errorCode, test.expect.error, `Test url ${test.url} correct error`); > is(table, test.expect.table, `Test url ${test.url} correct table`); > is(provider, test.expect.provider, `Test url ${test.url} correct provider`); > - is(prefix, btoa(test.expect.prefix), `Test url ${test.url} correct prefix`); > + is(completion, btoa(test.expect.completion), `Test url ${test.url} correct completion`); "correct fullhash"
Attachment #8874778 -
Flags: review?(francois) → review-
Comment 15•4 years ago
|
||
mozreview-review |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review170436 That's a great start Thomas! One thing that would be nice is to add a test for the `makeThreatHitReport()` function. ::: modules/libpref/init/all.js:5395 (Diff revision 1) > pref("browser.safebrowsing.provider.google4.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site="); > pref("browser.safebrowsing.provider.google4.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url="); > pref("browser.safebrowsing.provider.google4.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%&url="); > pref("browser.safebrowsing.provider.google4.advisoryURL", "https://developers.google.com/safe-browsing/v4/advisory"); > pref("browser.safebrowsing.provider.google4.advisoryName", "Google Safe Browsing."); > +pref("browser.safebrowsing.provider.google4.threatHitReportURL", "https://safebrowsing.googleapis.com/v4/threatHits?$ct=application/x-protobuf&key=%GOOGLE_API_KEY%&$httpMethod=POST"); How about `google4.dataSharingURL`? It seems like we are overusing the word "report" already. This feature really is all about sharing data with the Safe Browsing provider. ::: modules/libpref/init/all.js:5431 (Diff revision 1) > pref("plugins.flashBlock.enabled", false); > > // Allow users to ignore Safe Browsing warnings. > pref("browser.safebrowsing.allowOverride", true); > > +pref("browser.safebrowsing.threatHitReporting.automatic", false); I propose `browser.safebrowsing.provider.google4.dataSharing.enabled = false`. It should be provider-specific since we're only reporting the hits from that provider's list. ::: netwerk/base/nsChannelClassifier.cpp:1200 (Diff revision 1) > // protection or Safe Browsing. > // Do update the security state of the document and fire a security > // change event. > SetBlockedContent(mChannel, aErrorCode, aList, aProvider, aCompletion); > > + if (aErrorCode != NS_ERROR_TRACKING_URI && Given that we will make this pref provider-specific, you can just check that the given provider has dataSharing enabled. ::: netwerk/base/nsIURIClassifier.idl:111 (Diff revision 1) > + * Create and send a threat hit report > + * > + * @param aChannel: channel which has encountered the threat hit. > + * @param aList: string which represents listname of the hit > + * @param aProvider: string which represents provider of the hit > + * @param aCompletion: string which represents the completion hash of the hit "full hash" ::: netwerk/base/nsIURIClassifier.idl:115 (Diff revision 1) > + * @param aProvider: string which represents provider of the hit > + * @param aCompletion: string which represents the completion hash of the hit > + * > + */ > + > + void processThreatHitReport(in nsIChannel aChannel, Maybe we should call this "sendThreatHitReport"? ::: toolkit/components/telemetry/Histograms.json:5065 (Diff revision 1) > "bug_numbers": [1338082], > "description": "Negative cache duration (ms) received in fullhash response from any v4 provider" > }, > + "URLCLASSIFIER_REPORT_THREATHIT_NETWORK_ERROR": { > + "record_in_processes": ["main", "content"], > + "alert_emails": ["safebrowsing-telemetry@mozilla.org"], Add your email address to the list too. All new telemetry probes needs the email of a real person. ::: toolkit/components/telemetry/Histograms.json:5066 (Diff revision 1) > "description": "Negative cache duration (ms) received in fullhash response from any v4 provider" > }, > + "URLCLASSIFIER_REPORT_THREATHIT_NETWORK_ERROR": { > + "record_in_processes": ["main", "content"], > + "alert_emails": ["safebrowsing-telemetry@mozilla.org"], > + "expires_in_version": "63", You can make it expire "never" since it's [Category 1](https://wiki.mozilla.org/Firefox/Data_Collection#Data_Collection_Categories) data. ::: toolkit/components/telemetry/Histograms.json:5070 (Diff revision 1) > + "alert_emails": ["safebrowsing-telemetry@mozilla.org"], > + "expires_in_version": "63", > + "kind": "enumerated", > + "n_values": 30, > + "bug_numbers": [1351147], > + "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused,11=partial transfer,12=inadequate security,13=unknown host,14=dns lookup queue full,15=unknown proxy host" nit: space before 5, 11-15 Also the closing parenthesis is missing. ::: toolkit/components/telemetry/Histograms.json:5072 (Diff revision 1) > + "kind": "enumerated", > + "n_values": 30, > + "bug_numbers": [1351147], > + "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused,11=partial transfer,12=inadequate security,13=unknown host,14=dns lookup queue full,15=unknown proxy host" > + }, > + "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS2": { That should be "STATUS", not "STATUS2". We only add a number when we change the meaning of an existing probe. ::: toolkit/components/telemetry/Histograms.json:5074 (Diff revision 1) > + "bug_numbers": [1351147], > + "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused,11=partial transfer,12=inadequate security,13=unknown host,14=dns lookup queue full,15=unknown proxy host" > + }, > + "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS2": { > + "record_in_processes": ["main", "content"], > + "alert_emails": ["safebrowsing-telemetry@mozilla.org"], Add your email here too. ::: toolkit/components/telemetry/Histograms.json:5075 (Diff revision 1) > + "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused,11=partial transfer,12=inadequate security,13=unknown host,14=dns lookup queue full,15=unknown proxy host" > + }, > + "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS2": { > + "record_in_processes": ["main", "content"], > + "alert_emails": ["safebrowsing-telemetry@mozilla.org"], > + "expires_in_version": "63", never ::: toolkit/components/url-classifier/SBTelemetryUtils.h:1 (Diff revision 1) > +//* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ Maybe we should call this just "TelemetryUtils"? Is it going to pollute the global namespace? Otherwise, we should avoid using an acronym and just use "UrlClassifierTelemetryUtils". ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2113 (Diff revision 1) > + NS_ENSURE_SUCCESS(rv, rv); > + > + RefPtr<nsThreatHitReportListener> listener = new nsThreatHitReportListener(); > + rv = reportChannel->AsyncOpen2(listener); > + if (NS_FAILED(rv)) { > + LOG(("Sent threat hit report failed")); "Failure to send Safe Browsing threat hit report. ::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp (Diff revision 1) > NS_ENSURE_SUCCESS(rv, rv); > > return NS_OK; > } > > -// We might need to expand the bucket here if telemetry shows lots of errors Nice refactor! ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:606 (Diff revision 1) > + NS_ENSURE_TRUE(!aHashBase64.IsEmpty(), NS_ERROR_FAILURE); > + > + ThreatHit hit; > + nsresult rv; > + > + // 1) Set threat types. I think the 1, 2, 3, 4, 5, 6 comments are unnecessary. It's already quite clear since you broke it up in paragraphs. ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:637 (Diff revision 1) > + > + // XXX 6) Set UserInfo > + // Bug 1387364: we need to add unique ID stable over 1-2 week > + // Bug 1372456: Have to fetch region code before building the report message > + > + //------------------------------------------------------------------- I would also omit these comments.
Attachment #8893745 -
Flags: review?(francois) → review-
Comment 16•4 years ago
|
||
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #12) > (In reply to François Marier [:francois] from comment #8) > > The updated ThreatSourceType enum (see > > https://mana.mozilla.org/wiki/display/FIREFOX/Safe+Browsing) has a > > TAB_RESOURCE for any redirects encountered while fetching a bad subresource. > > ... > // A resource loaded within the final TAB_URL. > TAB_RESOURCE = 4; > It looks like google wants us to send all sub-resource loadings of a warning > page (eg all images, iframes, styles on the warning page). Seems quite a lot > of things to do No, that's only if the sub-resource was the one that got blocked. Here are some examples: Case 1: 1. page.com redirects to page.net 2. page.net loads badimage.com 3. badimage.com matches the malware list We would send the following: - TAB_URL = page.net - TAB_REDIRECT = page.com - MATCHING_URL = badimage.com Case 2: 1. page.com redirects to page.net 2. page.net loads image.net which redirects to badimage.com 3. badimage.com matches the malware list We would send the following: - TAB_URL = page.net - TAB_REDIRECT = page.com - MATCHING_URL = badimage.com - TAB_RESOURCE = image.net
Assignee | ||
Comment 17•4 years ago
|
||
Ah, I see. The description of the type is rather confusing. Thanks for clarifying that and taking a look.
Assignee | ||
Comment 18•4 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review170436 I have been trying to find a way to test that func, but still don't have good a way at the moment :(. The reason is we have to test in gtest (protobuf things) but have to touch to many things in channel ( and it's member like loadinfo and redirect chains). And server only returns OK, not OK for now, so not sure whether what we are writing to that is correct.
Assignee | ||
Comment 19•4 years ago
|
||
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #18) > Comment on attachment 8893745 [details] > Bug 1351147 - Support v4/ThreatHit request in SafeBrowsing V4 > > https://reviewboard.mozilla.org/r/164822/#review170436 > > I have been trying to find a way to test that func, but still don't have > good a way at the moment :(. > The reason is we have to test in gtest (protobuf things) but have to touch > to many things in channel ( and it's member like loadinfo and redirect > chains). > And server only returns OK, not OK for now, so not sure whether what we are > writing to that is correct. I am writing a mochitest to see if we are going to send a threathit request from a correct channel, full hash, but we may not know whether the request is correct. Presumably, we may have to ask google to see from server side
Assignee | ||
Comment 20•4 years ago
|
||
MozReview-Commit-ID: 8RNPt9ZK6Kr
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•3 years ago
|
||
Continue working on this bug and I fixed the comments of the last review. Francois, if you have time, could you please take a look at this?
Comment hidden (mozreview-request) |
Comment 26•3 years ago
|
||
mozreview-review |
Comment on attachment 8874778 [details] Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete https://reviewboard.mozilla.org/r/146022/#review187312 r+ with the comments below ::: commit-message-756c9:3 (Diff revisions 2 - 3) > -Bug 1351147 - Use completion instead of prefix in OnClassifyComplete > +Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete > + > +When an URL encourters a list, we have to collect the matching full hash to I think this is what you mean: "In order to optionally report the full hash back to Google, we need to keep it around in the callback." ::: netwerk/protocol/data/DataChannelParent.cpp:55 (Diff revision 3) > } > > NS_IMETHODIMP > DataChannelParent::SetClassifierMatchedInfo(const nsACString& aList, > const nsACString& aProvider, > - const nsACString& aPrefix) > + const nsACString& aCompletion) Looks like you missed one: `aCompletion` should be `aFullHash` here too.
Attachment #8874778 -
Flags: review?(francois) → review+
Comment 27•3 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874778 [details] Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete https://reviewboard.mozilla.org/r/146022/#review187312 > I think this is what you mean: > > "In order to optionally report the full hash back to Google, we need to keep it around in the callback." Actually, looking at what I wrote last time I reviewed this patch, let's expand this: "In order to optionally report the full hash back to Google, we need to keep it around in the callback. While a prefix is not the same as a full hash (multiple full hashes can map to the same prefix), in this case, the callback will only be called when the full hash matches." Does that sound right to you Thomas?
Comment 28•3 years ago
|
||
mozreview-review |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review187324 ::: commit-message-b7512:1 (Diff revision 3) > +Bug 1351147 - Support v4/ThreatHit request in SafeBrowsing V4 Support ThreatHit requests in Safe Browsing V4 ::: netwerk/base/nsChannelClassifier.cpp:1245 (Diff revision 3) > // protection or Safe Browsing. > // Do update the security state of the document and fire a security > // change event. > SetBlockedContent(mChannel, aErrorCode, aList, aProvider, aFullHash); > > + if (aErrorCode == NS_ERROR_MALWARE_URI || We also have `NS_ERROR_HARMFUL_URI` now. ::: netwerk/base/nsIURIClassifier.idl:106 (Diff revision 3) > * This is an internal interface used only for testing purposes. > */ > ACString classifyLocal(in nsIURI aURI, in ACString aTables); > + > + /** > + * Create and send a threat hit report I would suggest "Report to the provider that a Safe Browsing warning was shown." ::: netwerk/base/nsIURIClassifier.idl:108 (Diff revision 3) > ACString classifyLocal(in nsIURI aURI, in ACString aTables); > + > + /** > + * Create and send a threat hit report > + * > + * @param aChannel: channel which has encountered the threat hit. "channel for which the URL matched something on the threat list" ::: toolkit/components/telemetry/Histograms.json:5017 (Diff revision 3) > "high": 86400000, > "n_buckets": 50, > "bug_numbers": [1338082], > "description": "Negative cache duration (ms) received in fullhash response from any v4 provider" > }, > + "URLCLASSIFIER_REPORT_THREATHIT_NETWORK_ERROR": { Let's shorten that to `URLCLASSIFIER_THREATHIT_NETWORK_ERROR`. ::: toolkit/components/telemetry/Histograms.json:5020 (Diff revision 3) > "description": "Negative cache duration (ms) received in fullhash response from any v4 provider" > }, > + "URLCLASSIFIER_REPORT_THREATHIT_NETWORK_ERROR": { > + "record_in_processes": ["main", "content"], > + "alert_emails": ["tnguyen@mozilla.com, safebrowsing-telemetry@mozilla.org"], > + "expires_in_version": "never", Also add: "releaseChannelCollection": "opt-out", so that we also collect this Category 1 data on release. ::: toolkit/components/telemetry/Histograms.json:5024 (Diff revision 3) > + "alert_emails": ["tnguyen@mozilla.com, safebrowsing-telemetry@mozilla.org"], > + "expires_in_version": "never", > + "kind": "enumerated", > + "n_values": 30, > + "bug_numbers": [1351147], > + "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused, 11=partial transfer, 12=inadequate security, 13=unknown host, 14=dns lookup queue full, 15=unknown proxy host)" "Whether or not an error was encountered while sending a Safe Browsing ThreatHit report..." ::: toolkit/components/telemetry/Histograms.json:5026 (Diff revision 3) > + "kind": "enumerated", > + "n_values": 30, > + "bug_numbers": [1351147], > + "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused, 11=partial transfer, 12=inadequate security, 13=unknown host, 14=dns lookup queue full, 15=unknown proxy host)" > + }, > + "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS": { `URLCLASSIFIER_THREATHIT_REMOTE_STATUS` ::: toolkit/components/telemetry/Histograms.json:5029 (Diff revision 3) > + "description": "Network error from SafeBrowsing threat hit report. (0=sucess, 1=unknown error, 2=already connected, 3=not connected, 4=connection refused,5=net timeout, 6=offline, 7=port access not allowed, 8=net reset, 9=net interrupt, 10=proxy connection refused, 11=partial transfer, 12=inadequate security, 13=unknown host, 14=dns lookup queue full, 15=unknown proxy host)" > + }, > + "URLCLASSIFIER_REPORT_THREATHIT_REMOTE_STATUS": { > + "record_in_processes": ["main", "content"], > + "alert_emails": ["tnguyen@mozilla.com, safebrowsing-telemetry@mozilla.org"], > + "expires_in_version": "never", Add: `"releaseChannelCollection": "opt-out",` ::: toolkit/components/telemetry/Histograms.json:5033 (Diff revision 3) > + "alert_emails": ["tnguyen@mozilla.com, safebrowsing-telemetry@mozilla.org"], > + "expires_in_version": "never", > + "kind": "enumerated", > + "n_values": 16, > + "bug_numbers": [1351147], > + "description": "Server HTTP status code from SafeBrowsing threat hit report. (0=1xx, 1=200, 2=2xx, 3=204, 4=3xx, 5=400, 6=4xx, 7=403, 8=404, 9=408, 10=413, 11=5xx, 12=502|504|511, 13=503, 14=505, 15=Other)" nit: "Safe Browsing" and "ThreatHit" ::: toolkit/components/url-classifier/nsIUrlClassifierUtils.idl:139 (Diff revision 3) > [array, size_is(aPrefixCount)] in string aPrefixes, > in uint32_t aListCount, > in uint32_t aPrefixCount); > > /** > + * Make threat hit report request body. nit: "ThreatHit" ::: toolkit/components/url-classifier/nsIUrlClassifierUtils.idl:147 (Diff revision 3) > + * @param aListName listname represented in string. > + * @param aHashBase64 hash-based hit represented in base64. > + * > + * @returns A base64 encoded string. > + */ > + ACString makeThreatHitReportV4(in nsIChannel aChannel, There's only a single ThreatHit API, we may as well drop the `V4` in the name of the function. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1936 (Diff revision 3) > NS_ENSURE_SUCCESS(rv, rv); > } > return NS_OK; > } > > +class nsThreatHitReportListener final No `ns` prefix on new classes. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2061 (Diff revision 3) > + > + nsString urlStr; > + rv = formatter->FormatURLPref(NS_ConvertUTF8toUTF16(reportUrlPref), urlStr); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (urlStr.IsEmpty() || NS_LITERAL_STRING("about:blank").Equals(urlStr)) { Why are you comparing this with "about:blank"? In case a user decides to put "about:blank" in the pref? ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2062 (Diff revision 3) > + nsString urlStr; > + rv = formatter->FormatURLPref(NS_ConvertUTF8toUTF16(reportUrlPref), urlStr); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (urlStr.IsEmpty() || NS_LITERAL_STRING("about:blank").Equals(urlStr)) { > + return NS_OK; We should have a `LOG()` here: <provider> is missing a ThreatHit data reporting URL. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:2117 (Diff revision 3) > + NS_ENSURE_SUCCESS(rv, rv); > + > + RefPtr<nsThreatHitReportListener> listener = new nsThreatHitReportListener(); > + rv = reportChannel->AsyncOpen2(listener); > + if (NS_FAILED(rv)) { > + LOG(("Failure to send Safe Browsing threat hit report")); nit: "ThreatHit" ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:478 (Diff revision 3) > > +// Remove ref, query, userpass, anypart which may contain sensitive data > +static nsresult > +GetSpecWithoutSensitiveData(nsIURI* aUri, nsACString &aSpec) > +{ > + nsCOMPtr<nsIURI> clone; This should have a runtime check to make sure that `aUri` is not NULL. ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:500 (Diff revision 3) > + return NS_OK; > +} > + > +// Add a single threatsource from channel to threatHit message > +static nsresult > +SetThreatSourceFromChannel(ThreatHit& aHit, nsIChannel *aChannel, I would rename this function to `AddThreatSourceFromChannel` and then you can get rid of the comment above it. ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:544 (Diff revision 3) > + } > + return NS_OK; > +} > +// Add a single threatsource from redirect entry to threatHit message > +static nsresult > +SetThreatSourceFromRedirectEntry(ThreatHit& aHit, `AddThreatSourceFromRedirectEntry`, comment not needed ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:545 (Diff revision 3) > + return NS_OK; > +} > +// Add a single threatsource from redirect entry to threatHit message > +static nsresult > +SetThreatSourceFromRedirectEntry(ThreatHit& aHit, > + nsIRedirectHistoryEntry *aRediretEntry, typo: `aRedirectEntry` (missing the `c`) ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:548 (Diff revision 3) > +static nsresult > +SetThreatSourceFromRedirectEntry(ThreatHit& aHit, > + nsIRedirectHistoryEntry *aRediretEntry, > + ThreatHit_ThreatSourceType aType) > +{ > + NS_ENSURE_ARG_POINTER(aRediretEntry); That should probably be a runtime check. ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:585 (Diff revision 3) > + return NS_OK; > +} > + > +// Add top level tab url and redirect threatsources to threatHit message > +static nsresult > +SetTabThreatSources(ThreatHit& aHit, nsIChannel *aChannel) `AddTabThreatSources()` ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:598 (Diff revision 3) > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = thirdPartyUtil->GetTopWindowForChannel(aChannel, getter_AddRefs(win)); > + NS_ENSURE_SUCCESS(rv, rv); > + > + auto* pwin = nsPIDOMWindowOuter::From(win); I suspect this might leak if not manually `delete`d later. Probably better to use an explicit smart pointer here. ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:623 (Diff revision 3) > + bool isTopUri = false; > + rv = topUri->Equals(uri, &isTopUri); > + if (NS_SUCCEEDED(rv) && !isTopUri) { > + nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo(); > + if (loadInfo && loadInfo->RedirectChain().Length()) { > + SetThreatSourceFromRedirectEntry(aHit, loadInfo->RedirectChain()[0], Maybe we should guard against `loadInfo->RedirectChain()` not returning any elements and check the length before looking at the first element. ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:656 (Diff revision 3) > +nsUrlClassifierUtils::MakeThreatHitReportV4(nsIChannel *aChannel, > + const nsACString& aListName, > + const nsACString& aHashBase64, > + nsACString &aRequest) > +{ > + NS_ENSURE_ARG_POINTER(aChannel); That should probably be a runtime check, unless you're already it in the caller. ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:666 (Diff revision 3) > + nsresult rv; > + > + uint32_t threatType; > + rv = ConvertListNameToThreatType(aListName, &threatType); > + NS_ENSURE_SUCCESS(rv, rv); > + hit.set_threat_type((ThreatType)threatType); Can you use a C++ cast here instead? ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:672 (Diff revision 3) > + > + hit.set_platform_type(GetPlatformType()); > + > + nsCString hash; > + Unused << Base64Decode(aHashBase64, hash); > + auto threatEntry = hit.mutable_entry(); Let's add an assert like "hash.length <= size-of-a-full-hash". That will help if we ever get passed a bad base64 string. ::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:690 (Diff revision 3) > + std::string s; > + hit.SerializeToString(&s); > + > + nsCString out; > + rv = Base64URLEncode(s.size(), > + (const uint8_t*)s.c_str(), C++ cast (`reinterpret_cast` I imagine) ::: toolkit/components/url-classifier/tests/mochitest/test_threathit_report.html:60 (Diff revision 3) > + return hasher.finish(false); > +} > + > +var testDatas = [ > + { url: "itisaphishingsite.org/phishing.html", > + list: "test-phish-proto", Let's make this `mochitest-phish-proto` to prevent any future conflicts if we ever get rid of the `-simple` lists. ::: toolkit/components/url-classifier/tests/mochitest/test_threathit_report.html:61 (Diff revision 3) > +} > + > +var testDatas = [ > + { url: "itisaphishingsite.org/phishing.html", > + list: "test-phish-proto", > + provider: "test", Maybe `mochitest` here too? ::: toolkit/components/url-classifier/tests/mochitest/test_threathit_report.html:62 (Diff revision 3) > + > +var testDatas = [ > + { url: "itisaphishingsite.org/phishing.html", > + list: "test-phish-proto", > + provider: "test", > + // The base64 of protobuf binary represention of response: typo: "representation" nit: "binary protobuf representation" instead of "protobuf binary representation" ::: toolkit/components/url-classifier/tests/mochitest/test_threathit_report.html:210 (Diff revision 3) > + let progressListener = { > + onSecurityChange(aWebProgress, aRequest, aState) { > + let utils = Cc["@mozilla.org/url-classifier/utils;1"]. > + getService(Ci.nsIUrlClassifierUtils); > + expected = aTestData.reportUrl + "&$req=" + > + utils.makeThreatHitReportV4(aRequest, If I understand this correctly, it's checking that what is sent to the server matches the output of the function that is used to generate the reports. So it's a bit like comparing a function with itself. It's not likely to fail very often :) Maybe we should instead, in the `checkResults()` function, unpack the protobuf and confirm that the various URLs and attributes are the ones we expect to see? ::: toolkit/components/url-classifier/tests/mochitest/threathit.sjs:1 (Diff revision 3) > +const CC = Components.Constructor; This should also have a public domain header. It feels like we have a couple of very similar SJS files that basically just store and replay some strings. No need to do it in this patch, but perhaps we should merge them into a single generic one at some stage.
Attachment #8893745 -
Flags: review?(francois) → review-
Assignee | ||
Comment 29•3 years ago
|
||
mozreview-review |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review187768 ::: toolkit/components/url-classifier/tests/mochitest/threathit.sjs:1 (Diff revision 3) > +const CC = Components.Constructor; Ah, we have a sort of similar sjs files. They do not just store strings, but store and parse V2 content (encoded base64). In V4, I think should only store/save binary file and do not need ( or it's hard to do) to parse the binary protobuf request. Obviously we could merge V2 and V4 to an unified update.sjs, but it looks like we have to modify several old tests which use V2 -simple list. Seems it would be better to do in a follow up bug.
Assignee | ||
Comment 30•3 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review187324 > If I understand this correctly, it's checking that what is sent to the server matches the output of the function that is used to generate the reports. > > So it's a bit like comparing a function with itself. It's not likely to fail very often :) > > Maybe we should instead, in the `checkResults()` function, unpack the protobuf and confirm that the various URLs and attributes are the ones we expect to see? Yes, you are right. I thought about that before writing the test, but the protobuf and its class interfaces are only available in cpp. I did not try to add that, because I think we may not want to add a new testing xpcom component to access cpp only protobuf classes (the test xpcom component will be also created and added to the build package manifest but never be used). Indeed, we only test this sort of unpacking in Gtest. It's a closed mind, I've thought that I may add a new interface to nsIUrlClassifierUtils.idl to parse/unpack the request, but for testing only
Comment 31•3 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review187324 > Yes, you are right. I thought about that before writing the test, but the protobuf and its class interfaces are only available in cpp. I did not try to add that, because I think we may not want to add a new testing xpcom component to access cpp only protobuf classes (the test xpcom component will be also created and added to the build package manifest but never be used). Indeed, we only test this sort of unpacking in Gtest. > It's a closed mind, I've thought that I may add a new interface to nsIUrlClassifierUtils.idl to parse/unpack the request, but for testing only Ok, that's something we should solve because our protobuf tests aren't that great otherwise. Let's land this patch without it and fix the test in a follow-up bug. Can you file one please?
Comment 32•3 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review187768 > Ah, we have a sort of similar sjs files. They do not just store strings, but store and parse V2 content (encoded base64). In V4, I think should only store/save binary file and do not need ( or it's hard to do) to parse the binary protobuf request. > Obviously we could merge V2 and V4 to an unified update.sjs, but it looks like we have to modify several old tests which use V2 -simple list. Seems it would be better to do in a follow up bug. Fair enough, I was probably thinking of the V2 tests. No need to merge the V2/V4 test cases. They are pretty different as you point out.
Assignee | ||
Comment 33•3 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874778 [details] Bug 1351147 - Use fullhash instead of prefix in OnClassifyComplete https://reviewboard.mozilla.org/r/146022/#review187312 > Actually, looking at what I wrote last time I reviewed this patch, let's expand this: > > "In order to optionally report the full hash back to Google, we need to keep it around in the callback. While a prefix is not the same as a full hash (multiple full hashes can map to the same prefix), in this case, the callback will only be called when the full hash matches." > > Does that sound right to you Thomas? Thanks you for rephasing it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•3 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review187324 Thanks Francois for the detailed review > Why are you comparing this with "about:blank"? In case a user decides to put "about:blank" in the pref? It may return "about:blank" when the pref is missing. http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/toolkit/components/urlformatter/nsIURLFormatter.idl#44 > I suspect this might leak if not manually `delete`d later. Probably better to use an explicit smart pointer here. This is only a static_cast from nsCOMPtr<mozIDOMWindowProxy> win, it does not new a pointer > Maybe we should guard against `loadInfo->RedirectChain()` not returning any elements and check the length before looking at the first element. topLoadInfo->RedirectChain() returns an array in which IsEmpty() is exactly the same Length() == 0. I think it's not necessary, the for loop indexing will do that > Let's add an assert like "hash.length <= size-of-a-full-hash". > > That will help if we ever get passed a bad base64 string. I guess you say "==", we only send a 32 bytes fullhash > Let's make this `mochitest-phish-proto` to prevent any future conflicts if we ever get rid of the `-simple` lists. http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp#264 We defined a test-phish-proto table for v4 test and we have to use the correct table. That's a limitation that we have to add a new table there to support mochitest-phish-proto
Comment 38•3 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review187324 > This is only a static_cast from nsCOMPtr<mozIDOMWindowProxy> win, it does not new a pointer Sorry I misread that code. > http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp#264 > > We defined a test-phish-proto table for v4 test and we have to use the correct table. That's a limitation that we have to add a new table there to support mochitest-phish-proto I forgot about that. You're right, let's use the name we've already defined.
Comment 39•3 years ago
|
||
mozreview-review |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review188556 Looks great! r+ and datareview+ ::: netwerk/base/nsChannelClassifier.cpp:1174 (Diff revision 4) > +{ > + > + nsAutoCString provider(aProvider); > + nsPrintfCString reportEnablePref("browser.safebrowsing.provider.%s.dataSharing.enabled", > + provider.get()); > + if (!Preferences::GetBool(reportEnablePref.get())) { Let's be explicit about the default value of this pref if it's not set: GetBool(reportEnablePref.get(), false)
Attachment #8893745 -
Flags: review?(francois) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 45•3 years ago
|
||
Fixed lint errors after running ESLint and rebased lastest m-c code
Assignee | ||
Comment 46•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4442b36e97e1e5af3721a2fd4a4f37937de3a2d1&selectedJob=133245365
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•3 years ago
|
||
Try failed because the test url itisaphishingsite.org conflicted with previous test test_reporturl.html Change it to itisaphishingsite1.org and change update/gethash binary respectively (the attach file "Dump response" is what I used to generate binary)
Comment 49•3 years ago
|
||
mozreview-review |
Comment on attachment 8874777 [details] Bug 1351147 - Update protobuf to support threatHit api https://reviewboard.mozilla.org/r/146020/#review188660 Just a few minor comments in passing: Our static analysis bot found that you could be using `nullptr` instead of many `NULL`s in this code. (6 occurrences in your patch, and 76 others in the same file which could be fixed drive-by). ::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4030 (Diff revision 5) > +} > +const ThreatHit_UserInfo& ThreatHit_UserInfo::default_instance() { > +#ifdef GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER > + protobuf_AddDesc_safebrowsing_2eproto(); > +#else > + if (default_instance_ == NULL) protobuf_AddDesc_safebrowsing_2eproto(); Quick note: Our static analysis bot found a potential improvement on this line: Warning: Use nullptr [clang-tidy: modernize-use-nullptr] ``` if (default_instance_ == NULL) protobuf_AddDesc_safebrowsing_2eproto(); ^ nullptr ``` ::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4035 (Diff revision 5) > + if (default_instance_ == NULL) protobuf_AddDesc_safebrowsing_2eproto(); > +#endif > + return *default_instance_; > +} > + > +ThreatHit_UserInfo* ThreatHit_UserInfo::default_instance_ = NULL; Same quick note as above: ``` ThreatHit_UserInfo* ThreatHit_UserInfo::default_instance_ = NULL; ^ nullptr ``` ::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4261 (Diff revision 5) > _cached_size_ = 0; > threat_type_ = 0; > platform_type_ = 0; > entry_ = NULL; > + client_info_ = NULL; > + user_info_ = NULL; Same quick note as above: ``` client_info_ = NULL; ^ nullptr user_info_ = NULL; ^ nullptr ``` ::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4319 (Diff revision 5) > ZR_(threat_type_, platform_type_); > if (has_entry()) { > if (entry_ != NULL) entry_->::mozilla::safebrowsing::ThreatEntry::Clear(); > } > + if (has_client_info()) { > + if (client_info_ != NULL) client_info_->::mozilla::safebrowsing::ClientInfo::Clear(); Same quick note as above: ``` if (client_info_ != NULL) client_info_->::mozilla::safebrowsing::ClientInfo::Clear(); ^ nullptr ``` ::: toolkit/components/url-classifier/protobuf/safebrowsing.pb.cc:4322 (Diff revision 5) > } > + if (has_client_info()) { > + if (client_info_ != NULL) client_info_->::mozilla::safebrowsing::ClientInfo::Clear(); > + } > + if (has_user_info()) { > + if (user_info_ != NULL) user_info_->::mozilla::safebrowsing::ThreatHit_UserInfo::Clear(); Same quick note as above: ``` if (user_info_ != NULL) user_info_->::mozilla::safebrowsing::ThreatHit_UserInfo::Clear(); ^ nullptr ```
Comment 50•3 years ago
|
||
mozreview-review |
Comment on attachment 8893745 [details] Bug 1351147 - Support ThreatHit requests in SafeBrowsing V4 https://reviewboard.mozilla.org/r/164822/#review188662 Two more small suggestions from our static analysis bot. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1940 (Diff revision 7) > +public: > + NS_DECL_ISUPPORTS > + NS_DECL_NSIREQUESTOBSERVER > + NS_DECL_NSISTREAMLISTENER > + > + ThreatHitReportListener() {} Quick note: Our static analysis bot found a potential improvement for this line: Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ``` ThreatHitReportListener() {} ^ = default; ``` ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1943 (Diff revision 7) > + NS_DECL_NSISTREAMLISTENER > + > + ThreatHitReportListener() {} > + > +private: > + ~ThreatHitReportListener() {} Similar quick note: Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default] ``` ~ThreatHitReportListener() {} ^ = default; ```
Assignee | ||
Comment 51•3 years ago
|
||
safebrowsing.pb.cc is automatically generated file from running protobuf. We may not want to change this file. Is there any way to exclude the file from running static analysis? Static analysis running seems interesting, which tool are we using now?
Flags: needinfo?(janx)
Assignee | ||
Comment 52•3 years ago
|
||
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #51) > Static analysis running seems interesting, which tool are we using now? Warning: Use nullptr [clang-tidy: modernize-use-nullptr] Looks like that is clang-tidy
Comment hidden (mozreview-request) |
Assignee | ||
Comment 54•3 years ago
|
||
Updated the commit, only changed as suggestions in nsUrlClassifierDBService, but I still keep the safebrowsing.pb.cc Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e668f9c3c99020bac1d8b021bf112db4927bd705
Comment 55•3 years ago
|
||
Pushed by tnguyen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f2b1824179f8 Update protobuf to support threatHit api r=francois https://hg.mozilla.org/integration/autoland/rev/7c7be12b43be Use fullhash instead of prefix in OnClassifyComplete r=francois https://hg.mozilla.org/integration/autoland/rev/d10aeb884922 Support ThreatHit requests in SafeBrowsing V4 r=francois
Assignee | ||
Updated•3 years ago
|
Flags: needinfo?(janx)
Comment 56•3 years ago
|
||
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #51) > safebrowsing.pb.cc is automatically generated file from running protobuf. We > may not want to change this file. Is there any way to exclude the file from > running static analysis? Ah, indeed if the file is generated automatically, we wouldn't want our tools to change it. We can simply add it to /tools/rewriting/ThirdPartyPaths.txt. I can do that, but which of these paths should I make our tools ignore? - toolkit/components/url-classifier/protobuf/safebrowsing.pb.* - toolkit/components/url-classifier/protobuf/* - toolkit/components/url-classifier/* Additionally, is there any way to make protobuf generate C++11 files, e.g. that use `nullptr` instead of `NULL`? (Not a big issue, but nice to have if it's easy to do.) > Static analysis running seems interesting, which tool are we using now? For this analysis we're indeed using clang-tidy. And you can now easily run it too by using: ./mach static-analysis check path/to/file.cpp
Flags: needinfo?(tnguyen)
Comment 57•3 years ago
|
||
(In reply to Jan Keromnes [:janx] from comment #56) > We can simply add it to /tools/rewriting/ThirdPartyPaths.txt. I can do that, > but which of these paths should I make our tools ignore? > > - toolkit/components/url-classifier/protobuf/safebrowsing.pb.* > - toolkit/components/url-classifier/protobuf/* > - toolkit/components/url-classifier/* I've added toolkit/components/url-classifier/chromium/* and toolkit/components/url-classifier/protobuf/* to ThirdPartyPaths.txt in bug 1403527. Please confirm that this looks reasonable.
Comment 58•3 years ago
|
||
(In reply to Jan Keromnes [:janx] from comment #57) > I've added toolkit/components/url-classifier/chromium/* and > toolkit/components/url-classifier/protobuf/* to ThirdPartyPaths.txt in bug > 1403527. > > Please confirm that this looks reasonable. To ignore all of the protobuf-related code: devtools/shared/heapsnapshot/CoreDump.pb.* gfx/layers/protobuf/* toolkit/components/downloads/chromium/* toolkit/components/protobuf/src/* toolkit/components/url-classifier/chromium/* toolkit/components/url-classifier/protobuf/*
Flags: needinfo?(tnguyen)
Comment 59•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f2b1824179f8 https://hg.mozilla.org/mozilla-central/rev/7c7be12b43be https://hg.mozilla.org/mozilla-central/rev/d10aeb884922
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•3 years ago
|
Whiteboard: pwphish-prep
You need to log in
before you can comment on or make changes to this bug.
Description
•