Closed Bug 1384753 Opened 2 years ago Closed 2 years ago

Move csd.proto into a common Safe Browsing directory and update it

Categories

(Toolkit :: Safe Browsing, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: francois, Assigned: dimi)

References

(Blocks 1 open bug)

Details

(Whiteboard: pwphish-prep)

Attachments

(4 files)

Currently csd.proto is only used by download protection and lives in https://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/toolkit/components/downloads/generate_csd.sh

Since it will be also required by the password protection feature, we should probably create a place for common Safe Browsing files and utilities.

It will also have to be updated to the latest upstream version:

https://cs.chromium.org/chromium/src/components/safe_browsing/csd.proto
The latest version of csd.proto requires version 3 of the protobuf compiler: https://github.com/google/protobuf/issues/1747

We'll need to update the version we bundle in toolkit/components/protobuf
See Also: → 1198917
Depends on: 1385461
It's possible we can just hack the upstream csd.proto file and comment-out the reserved field.
Is there any diff in output between using protobuf 3.0.0 and 2.6.1? Basically safebrowsing.proto use protobuf 2 (syntax = "proto2"). I am concerning if SB still works when we compile and generate output using v3.0 (although, it said that v3.0 supports both proto2 and proto3).
Do you know whether Google wants to upgrade protobuf completely to 3.0? It would be good because JS will be supported from 3.0 and we can use JS generated files in our test (parsing for example)
Flags: needinfo?(francois)
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #4)
> Is there any diff in output between using protobuf 3.0.0 and 2.6.1?
> Basically safebrowsing.proto use protobuf 2 (syntax = "proto2"). I am
> concerning if SB still works when we compile and generate output using v3.0
> (although, it said that v3.0 supports both proto2 and proto3).

It seems like every version of protobuf changes the output slightly. Not necessarily in a meaningful way though.

And you're right, version 3 of the library supports both the proto2 and proto3 specifications, so it's backwards-compatible.

> Do you know whether Google wants to upgrade protobuf completely to 3.0?

I don't know, but Chrome has upgraded the library to 3.3.0 at least.
Flags: needinfo?(francois)
Assignee: francois → dlee
Here's my WIP patch for moving the application reputation code into a new reputation service where we can put both application reputation and login reputation.
(In reply to François Marier [:francois] from comment #6)
> Created attachment 8913847 [details] [diff] [review]
> move-apprep-files-1384753.patch
> 
> Here's my WIP patch for moving the application reputation code into a new
> reputation service where we can put both application reputation and login
> reputation.

Great, thanks. BTW, I also forgot to ask you if you want to keep working on this one.
If yes, please feel free to take it back.
(In reply to Dimi Lee[:dimi][:dlee] from comment #7)
> Great, thanks. BTW, I also forgot to ask you if you want to keep working on
> this one.
> If yes, please feel free to take it back.

Feel free to finish this patch. I'm working on the protobuf library upgrade right now.
Whiteboard: pwphish-prep
Blocks: 1407879
Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

https://reviewboard.mozilla.org/r/192720/#review197890


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/reputationservice/LoginReputation.cpp:31
(Diff revision 1)
> +  }
> +
> +  return gLoginReputationService;
> +}
> +
> +LoginReputationService::LoginReputationService()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: toolkit/components/reputationservice/LoginReputation.cpp:35
(Diff revision 1)
> +
> +LoginReputationService::LoginReputationService()
> +{
> +}
> +
> +LoginReputationService::~LoginReputationService()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
Comment on attachment 8921700 [details]
Bug 1384753 - Move Application Reputation files into a new component.

https://reviewboard.mozilla.org/r/192718/#review198254

::: commit-message-a124f:1
(Diff revision 1)
> +Bug 1384753 - P1. Move csd.proto into a common Safe Browsing directory and update it. r?francois

You didn't actually update the CSD file, which is fine, we should do that in a different commit.

This commit message should be simply:

    Move Application Reputation files into a new component.

Also, we don't really need the "P1" since the order will be preserved automatically by MozReview.

::: toolkit/components/reputationservice/test/unit/test_app_rep_maclinux.js:7
(Diff revision 1)
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * This file tests signature extraction using Windows Authenticode APIs of

This comment is a lie and comes from a copy/paste error I made a long time ago.

We may as well remove it.
Attachment #8921700 - Flags: review?(francois) → review+
Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

https://reviewboard.mozilla.org/r/192720/#review198260

I would suggest adding one more thing:

- initialize the login reputation service at startup idle time if `browser.safebrowsing.passwords.enabled = true`.

::: commit-message-088a8:1
(Diff revision 1)
> +Bug 1384753 - P2. Add dummy login reputation service in reputationservice directory. r?francois

I would simply say "Add dummy login reputation service in reputationservice component."

::: toolkit/components/reputationservice/LoginReputation.cpp:40
(Diff revision 1)
> +LoginReputationService::~LoginReputationService()
> +{
> +}
> +
> +NS_IMETHODIMP
> +LoginReputationService::QueryReputation()

Maybe we shouldn't define this yet since we're not sure exactly what parameters it's going to take?
Attachment #8921701 - Flags: review?(francois) → review-
Blocks: 1411861
(In reply to François Marier [:francois] from comment #12)
> 
> You didn't actually update the CSD file, which is fine, we should do that in
> a different commit.

Yes, I want to update it in a separate one. I just created bug 1411861 for that.
(In reply to François Marier [:francois] from comment #13)
> Maybe we shouldn't define this yet since we're not sure exactly what
> parameters it's going to take?

The parameters is filled in Bug 1407878.
I'll remove |QueryReputation| in this patch.
Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

https://reviewboard.mozilla.org/r/192720/#review197890

> Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

Fix this by adding LOG to constructor & destructor which I was going to add them in Bug 1407878
Comment on attachment 8921701 [details]
Bug 1384753 - Add dummy login reputation service in reputationservice component.

https://reviewboard.mozilla.org/r/192720/#review198658

::: commit-message-088a8:3
(Diff revisions 1 - 3)
> -Bug 1384753 - P2. Add dummy login reputation service in reputationservice directory. r?francois
> +Bug 1384753 - Add dummy login reputation service in reputationservice component. r?francois
>  
> -MozReview-Commit-ID: Ie7PIiHYZ1g
> +Login reputation service is initialized when idle startup.

"during idle statup."

::: toolkit/components/reputationservice/LoginReputation.cpp:10
(Diff revisions 1 - 3)
>  
>  #include "LoginReputation.h"
>  
>  using namespace mozilla;
>  
> +// MOZ_LOG=LoginReputation:5

Adding logging early, great idea!

::: toolkit/components/reputationservice/LoginReputation.cpp:38
(Diff revisions 1 - 3)
>    return gLoginReputationService;
>  }
>  
>  LoginReputationService::LoginReputationService()
>  {
> +  LR_LOG(("Login reputation service started up"));

Maybe we should use "starting up" in order to look the same as the destructor.

::: browser/components/nsBrowserGlue.js:1113
(Diff revision 3)
>      // It's important that SafeBrowsing is initialized reasonably
>      // early, so we use a maximum timeout for it.
>      Services.tm.idleDispatchToMainThread(() => {
>        SafeBrowsing.init();
> +
> +      // Init login reputation service after initializing SafeBrowsing.jsm because

I would say simply:

    // Login reputation depends on the Safe Browsing API.
Attachment #8921701 - Flags: review?(francois) → review+
(In reply to Dimi Lee[:dimi][:dlee] from comment #23)
> Comment on attachment 8921701 [details]
> Bug 1384753 - Add dummy login reputation service in reputationservice
> component.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/192720/diff/4-5/

Singleton constructors require already-addrefed now (Bug 1409249), fix compile error.
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9801aade9511
Move Application Reputation files into a new component. r=francois
https://hg.mozilla.org/integration/autoland/rev/2f0da700e651
Add dummy login reputation service in reputationservice component. r=francois
Backed out for frequently failing chrome's toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html on Android:

https://hg.mozilla.org/integration/autoland/rev/806540586443b1304287811c592cfcc7d90e0cb6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2f0da700e6518876aeefff4e48eb5a0a2e055074&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=140220271&repo=autoland

[task 2017-10-27T15:48:47.309Z] 15:48:47     INFO -  90 INFO None91 INFO TEST-START | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
[task 2017-10-27T15:48:57.607Z] 15:48:57     INFO -  Buffered messages logged at 15:48:45
[task 2017-10-27T15:48:57.607Z] 15:48:57     INFO -  92 INFO SpawnTask.js | Entering test setup
[task 2017-10-27T15:48:57.608Z] 15:48:57     INFO -  93 INFO Using file picker download directory /data/data/org.mozilla.fennec_aurora/app_tmpdir/downloads
[task 2017-10-27T15:48:57.609Z] 15:48:57     INFO -  94 INFO Using default download directory /data/data/org.mozilla.fennec_aurora/app_tmpdir/downloads-1
[task 2017-10-27T15:48:57.610Z] 15:48:57     INFO -  95 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html | Should be able to distinguish between files saved with or without the file picker
[task 2017-10-27T15:48:57.611Z] 15:48:57     INFO -  Buffered messages logged at 15:48:46
[task 2017-10-27T15:48:57.613Z] 15:48:57     INFO -  96 INFO SpawnTask.js | Leaving test setup
[task 2017-10-27T15:48:57.613Z] 15:48:57     INFO -  97 INFO SpawnTask.js | Entering test test_downloads_saveAs
[task 2017-10-27T15:48:57.614Z] 15:48:57     INFO -  98 INFO Extension loaded
[task 2017-10-27T15:48:57.615Z] 15:48:57     INFO -  Buffered messages logged at 15:48:52
[task 2017-10-27T15:48:57.616Z] 15:48:57     INFO -  99 INFO Testing that saveAs=true uses the file picker as expected
[task 2017-10-27T15:48:57.617Z] 15:48:57     INFO -  100 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html | the file should have been cleaned up properly previously
[task 2017-10-27T15:48:57.618Z] 15:48:57     INFO -  Buffered messages finished
[task 2017-10-27T15:48:57.619Z] 15:48:57     INFO -  101 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html | downloads.download() works with saveAs=true
Flags: needinfo?(dlee)
Fix try error on android, looks good now
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5658c679833ef7859fda8fc6405353c1693b778d
Flags: needinfo?(dlee)
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/45181edd35f6
Move Application Reputation files into a new component. r=francois
https://hg.mozilla.org/integration/autoland/rev/a6217947aff0
Add dummy login reputation service in reputationservice component. r=francois
https://hg.mozilla.org/mozilla-central/rev/45181edd35f6
https://hg.mozilla.org/mozilla-central/rev/a6217947aff0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/97bf29612932
Port bug 1384753 to TB/IB/SM: Move Application Reputation files into a new component. rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.