Last Comment Bug 663211 - safebrowsing tests shouldn't load internet pages
: safebrowsing tests shouldn't load internet pages
Status: RESOLVED FIXED
: verified1.9.2
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Phil Ringnalda (:philor)
:
Mentors:
: 627292 (view as bug list)
Depends on:
Blocks: 626999
  Show dependency treegraph
 
Reported: 2011-06-09 13:32 PDT by Ben Hearsum (:bhearsum)
Modified: 2014-05-27 12:25 PDT (History)
8 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
.x-fixed
.20-fixed


Attachments
Fix v.1 (982 bytes, patch)
2011-06-11 11:28 PDT, Phil Ringnalda (:philor)
ted: review+
mars.martian+bugmail: feedback+
asa: approval‑mozilla‑aurora+
dveditz: approval2.0+
dveditz: approval1.9.2.20+
Details | Diff | Review

Description Ben Hearsum (:bhearsum) 2011-06-09 13:32:53 PDT
Currently, there's two safebrowsing tests that load a www.mozilla.org webpage:
http://mxr.mozilla.org/mozilla-central/source/browser/components/safebrowsing/content/test/browser_bug415846.js#40
http://mxr.mozilla.org/mozilla-central/source/browser/components/safebrowsing/content/test/browser_bug400731.js#26


We've been trying to get rid of tests like this so we can shut off network access to build & test machines (bug 617414). Can these be replaced with something loading from the local webserver?
Comment 1 Ted Mielczarek [:ted.mielczarek] 2011-06-10 08:02:55 PDT
I think it should be fine to redirect www.mozilla.org to the local webserver like we do with fxfeeds:
http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt#126

We might need to add some content so it doesn't 404, though.

Alternately, maybe we can add some test URLs in the default phishing/malware db on example.com? Not sure if that's worth the effort.
Comment 2 Phil Ringnalda (:philor) 2011-06-10 23:41:47 PDT
*** Bug 627292 has been marked as a duplicate of this bug. ***
Comment 3 Phil Ringnalda (:philor) 2011-06-11 11:28:54 PDT
Created attachment 538706 [details] [diff] [review]
Fix v.1

Yeah, pretty sure this is the patch I was talking about writing in bug 627292, before I confused myself into thinking that it wasn't needed. Shoving more stuff into the db and testing with a db other than what we ship just doesn't sound like that much fun.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-06-11 11:34:02 PDT
Comment on attachment 538706 [details] [diff] [review]
Fix v.1

Do these need to be non-404?
Comment 5 Phil Ringnalda (:philor) 2011-06-11 11:40:13 PDT
Comment on attachment 538706 [details] [diff] [review]
Fix v.1

http://tbpl.mozilla.org/?tree=Try&rev=9b9fa1e53ce8 says they don't need to be anything but a 404, and my vague understanding of how safebrowsing works says it makes sense that it won't care whether a bad URI is a 404 or a picture of a kitten, but let's ask.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2011-06-11 12:02:57 PDT
Comment on attachment 538706 [details] [diff] [review]
Fix v.1

Okay. r=me on the addition, would be good to get someone who understands safebrowsing to check that testing with 404s is still valid.
Comment 7 Mehdi Mulani [:mmm] (I don't check this) 2011-06-19 08:08:22 PDT
I tried accessing its-a-trap.html with my internet disconnected and was still able to retrieve the warning.
Do the tests explicitly fail when no [www.]mozilla.com server is provided?
Comment 8 Phil Ringnalda (:philor) 2011-06-19 08:29:09 PDT
Oh, two steps up the dependency chain, not just one like I thought: no, they don't fail, according to bug 617414 comment 31.
Comment 9 Mehdi Mulani [:mmm] (I don't check this) 2011-06-19 09:19:59 PDT
Comment on attachment 538706 [details] [diff] [review]
Fix v.1

This will stifle any outgoing requests as expected and a 404 is OK for safebrowsing.

Ted: Bugs 662046 and 662213 discuss removing its-a-trap and its-an-attack and providing new pages.
Comment 11 Mounir Lamouri (:mounir) 2011-06-20 08:45:27 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/99fc38a20ef3
Comment 12 Ben Hearsum (:bhearsum) 2011-06-20 08:53:37 PDT
Thanks all!
Comment 13 Ben Hearsum (:bhearsum) 2011-06-22 12:04:22 PDT
Comment on attachment 538706 [details] [diff] [review]
Fix v.1

Drivers, this patch applies cleanly to 1.9.2 and mozilla-aurora. I'd like to land it in both of those places to stop the safebrowsing tests from attempting to access the internet, which will start to fail if they continue to once bug 646046 is done.
Comment 14 Ted Mielczarek [:ted.mielczarek] 2011-06-22 12:19:51 PDT
Drivers: note that this is a test-harness-only change with zero risk to the actual shipping product.
Comment 15 Ben Hearsum (:bhearsum) 2011-06-27 08:01:20 PDT
Comment on attachment 538706 [details] [diff] [review]
Fix v.1

http://hg.mozilla.org/releases/mozilla-aurora/rev/6c38dd703981

I think setting checkin+ is the right thing to do here?
Comment 16 Ben Hearsum (:bhearsum) 2011-06-27 08:03:47 PDT
Comment on attachment 538706 [details] [diff] [review]
Fix v.1

Looks we're not turning off mozilla-2.0 quite yet either, so we need this there, too.
> Drivers, this patch applies cleanly to 1.9.2 and mozilla-aurora. I'd like to
> land it in both of those places to stop the safebrowsing tests from
> attempting to access the internet, which will start to fail if they continue
> to once bug 646046 is done.
> note that this is a test-harness-only change with zero risk to the
> actual shipping product.
Comment 17 Daniel Veditz [:dveditz] 2011-06-27 10:48:24 PDT
Comment on attachment 538706 [details] [diff] [review]
Fix v.1

Approved for 1.9.2.19, a=dveditz for release-drivers
Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Comment 18 Phil Ringnalda (:philor) 2011-06-27 12:23:56 PDT
(In reply to comment #15)
> I think setting checkin+ is the right thing to do here?

Nope, that would say "this is checked in on the trunk, but I'm leaving the bug open for something else." What you wanted to avoid the LegNeatoBot nagging me about pushing to Aurora was "status-firefox6: fixed".
Comment 20 [On PTO until 6/29] 2011-08-08 17:47:02 PDT
Verified for 1.9.2 (by looking at tests). Whee.
Comment 21 Virgil Dicu [:virgil] [QA] 2011-08-31 06:53:01 PDT
Hello.

Are there any guidelines anyone could provide in order to verify this issue?
Comment 22 Ted Mielczarek [:ted.mielczarek] 2011-08-31 08:14:02 PDT
This was purely a test harness fix. You could verify it by running a packet sniffer (like Wireshark), and running the safebrowsing mochitests and verifying that they do not contact mozilla.org, if you wanted.

Note You need to log in before you can comment on or make changes to this bug.