663211 - safebrowsing tests shouldn't load internet pages

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[[e-mail] (:bhearsum)]

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

[(not currently active) Ted Mielczarek]

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 3

[Phil Ringnalda (:philor)]

Attachment: 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

[(not currently active) Ted Mielczarek]

Comment on attachment 538706 [details] [diff] [review]
Fix v.1

Do these need to be non-404?

Comment 5

[Phil Ringnalda (:philor)]

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

[(not currently active) Ted Mielczarek]

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)]

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)]

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)]

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 10

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/99fc38a20ef3

Comment 11

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/99fc38a20ef3

Comment 12

[[e-mail] (:bhearsum)]

Thanks all!

Comment 13

[[e-mail] (:bhearsum)]

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

[(not currently active) Ted Mielczarek]

Drivers: note that this is a test-harness-only change with zero risk to the actual shipping product.

Comment 15

[[e-mail] (:bhearsum)]

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

[[e-mail] (:bhearsum)]

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]]

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)]

(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 19

[[e-mail] (:bhearsum)]

Comment on attachment 538706 [details] [diff] [review]
Fix v.1

http://hg.mozilla.org/releases/mozilla-2.0/rev/e1727a01dc94
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a6fe9498f635

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.2 (by looking at tests). Whee.

Comment 21

[Virgil Dicu [:virgil] [QA]]

Hello.

Are there any guidelines anyone could provide in order to verify this issue?

Comment 22

[(not currently active) Ted Mielczarek]

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.