Disable remote lookups.

Comment on attachment 8475287 [details] [diff] [review] Disable remote lookups ( Review of attachment 8475287 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/downloads/ApplicationReputation.cpp @@ +426,5 @@ > + nsCString serviceUrl; > + NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl), > + NS_ERROR_NOT_AVAILABLE); > + if (serviceUrl.EqualsLiteral("")) { > + return NS_ERROR_NOT_AVAILABLE; I was wondering why you'd not return OnComplete(false, NS_OK) as happens in the non-Windows codepath, but it looks like http://dxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/ApplicationReputation.cpp#626 means it ends up doing that anyway?

Comment on attachment 8475325 [details] [diff] [review] Disable remote lookups ( Review of attachment 8475325 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/downloads/ApplicationReputation.cpp @@ +426,5 @@ > + nsCString serviceUrl; > + NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl), > + NS_ERROR_NOT_AVAILABLE); > + if (serviceUrl.EqualsLiteral("")) { > + return OnComplete(false, NS_ERROR_NOT_AVAILABLE); This looks wrong. http://dxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIApplicationReputation.idl#93 * NS_OK if and only if the query succeeded. If it did, then * shouldBlock is meaningful (otherwise it defaults to false). This * may be NS_ERROR_FAILURE if the response cannot be parsed, or * NS_ERROR_NOT_AVAILABLE if the service has been disabled or is not * reachable. This means that you do end up ignoring the local lists again, because returning an error means that shouldBlock is not meaningful, per the above. (or the documentation is wrong) From reading that, you need OnComplete(false, NS_OK) when the remote server isn't set but you still want local blocks, which matches the !(#ifdef XP_WIN) path.

Comment on attachment 8475325 [details] [diff] [review] Disable remote lookups ( Review of attachment 8475325 [details] [diff] [review]: ----------------------------------------------------------------- After reading the code closer, this is OK, but I'm still curious about the test. ::: toolkit/components/downloads/ApplicationReputation.cpp @@ +426,5 @@ > + nsCString serviceUrl; > + NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl), > + NS_ERROR_NOT_AVAILABLE); > + if (serviceUrl.EqualsLiteral("")) { > + return OnComplete(false, NS_ERROR_NOT_AVAILABLE); Reading more, we'll bail out as soon as we get a local hit, so that's not an issue, and the calling code never actually checks the rv value (which wouldn't matter if it defaults to shouldBlock=false anyway) So it'll work. I guess this also leaves room for UX like "the download looks fine but the AppRep server couldn't be contacted". ::: toolkit/components/downloads/test/unit/test_app_rep.js @@ -170,5 @@ > - gAppRep.queryReputation({ > - sourceURI: createURI("http://example.com"), > - fileSize: 12, > - }, function onComplete(aShouldBlock, aStatus) { > - // We should be getting NS_ERROR_NOT_AVAILABLE if the service is disabled Why doesn't this test work any more?

Comment on attachment 8475325 [details] [diff] [review] Disable remote lookups ( Review of attachment 8475325 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/downloads/test/unit/test_app_rep.js @@ -170,5 @@ > - gAppRep.queryReputation({ > - sourceURI: createURI("http://example.com"), > - fileSize: 12, > - }, function onComplete(aShouldBlock, aStatus) { > - // We should be getting NS_ERROR_NOT_AVAILABLE if the service is disabled It works now, reverting. It didn't work in the first iteration of this patch.

Comment on attachment 8475393 [details] [diff] [review] Disable remote lookups ( Approval Request Comment [Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=662819 [User impact if declined]: Users will have improved malware detection through remote lookups, but continue have no way to recover the downloaded file. [Describe test coverage new/current, TBPL]: This requires manual testing on a release build to make sure the pref browser.safebrowsing.appRepURL is blank on beta and release. Unittests are test_app_rep.js and test_app_rep_windows.js in toolkit/components/url-classifier. [Risks and why]: Pretty low risk. This makes malware detection the same as in FF 31 instead of increasing coverage. [String/UUID change made/needed]: None.

Erm, I was wrong about the test change not being needed. The old way threw immediately, the new way does not.

Btw, the patch attached for beta approval is still correct and combines both comment 13 and comment 9.

https://hg.mozilla.org/mozilla-central/rev/cb5e6705205d https://hg.mozilla.org/mozilla-central/rev/082bcda8015a

This fixes & re-enables the disabled test.

Comment on attachment 8475981 [details] [diff] [review] Re-enable removed ApplicationReputation tests Review of attachment 8475981 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/downloads/test/unit/test_app_rep.js @@ +171,5 @@ > + gAppRep.queryReputation({ > + sourceURI: createURI("http://example.com"), > + fileSize: 12, > + }, function onComplete(aShouldBlock, aStatus) { > + var isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes); Yikes, is this the only way to do this? It might be cleaner just to do this test in test_app_rep_windows.js.

It's what the documentation recommends: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests Moving the test to test_app_rep_windows.js also works, but loses the test coverage on non-Windows. (It's currently verifying that the local lists are still checked if the appRepURL is empty, i.e. it's exactly testing what you just patched)

Hmm, not true given that the patch is local to the XP_WIN codepath. I guess we can move it then.

Patch is on m-c already. (comment 18)

Comment on attachment 8476091 [details] [diff] [review] v2. Re-enable removed ApplicationReputation tests Review of attachment 8476091 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Probably better not to do this kind of follow on work in the same bug that's being evaluated for uplift in order to avoid confusing approvers.

Comment on attachment 8475393 [details] [diff] [review] Disable remote lookups ( Approved for Aurora and Beta. This functionality will be reenabled in a later release.

https://hg.mozilla.org/releases/mozilla-aurora/rev/d5cbd256758e https://hg.mozilla.org/releases/mozilla-beta/rev/b554afc480aa

Matt, please verify that browser.safebrowsing.appRepURL is blank on beta, filled in on nightly and aurora, and that you can download files on Windows.

I've verified that pref does not appear in Fx32, but does in Fx33 and Fx34. I've downloaded files - including an EXE - on Windows 8 Fx32 and no issues.

i have problems downloading files after this landed in beta, see bug 1057764