Closed Bug 1233914 Opened 10 years ago Closed 9 years ago

ping doesn't honor the TP list

Categories

(Toolkit :: Safe Browsing, defect)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: francois, Assigned: dimi)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Test page
163 bytes, text/html
Details
MozReview Request: Bug 1233914 - P1. ping doesn't honor the TP list. r=francois
58 bytes, text/x-review-board-request
francois
: review+
Details
MozReview Request: Bug 1233914 - P2. Testcase. r=francois
58 bytes, text/x-review-board-request
francois
: review+
Details
Attached file Test page —
It is possible to get the browser to POST to a URL on the TP list via the ping attribute.
Component: DOM: Security → Safe Browsing
Product: Core → Toolkit
Blocks: 1207775
Assignee: nobody → dlee
(In reply to Dimi Lee[:dimi][:dlee] from comment #2) > Created attachment 8735780 [details] > MozReview Request: Bug 1233914 - P2. Testcase. r=francois > > Review commit: https://reviewboard.mozilla.org/r/42945/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/42945/ The setTimeout for 200 millisecond is copied from wpt[1] [1]https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/infrastructure/urls/resolving-urls/query-encoding/resources/resolve-url.js#46
Comment on attachment 8735780 [details] MozReview Request: Bug 1233914 - P2. Testcase. r=francois https://reviewboard.mozilla.org/r/42945/#review39607 I really like the way you are testing this. To make sure that the test case never breaks, I would suggest checking (using the same ping.sjs code) these cases: 1. When we ping a non-blaclisted URL, the ping does work successfully. 2. When we ping a blacklisted URL with Safe Browsing turned off, the ping does work successfully. 3. When we ping a blacklisted URL with Safe Browsing turned on, the ping is blocked. (i.e. what the test does already) ::: toolkit/components/url-classifier/tests/mochitest/mochitest.ini:25 (Diff revision 1) > + ping.sjs > > [test_classifier.html] > skip-if = (os == 'linux' && debug) #Bug 1199778 > [test_classifier_worker.html] > +[test_classify_flag.html] I would prefer a more descriptive name here, like "test_classify_ping" because "classify_flag" applies to all of the other bugs we need to fix :) ::: toolkit/components/url-classifier/tests/mochitest/test_classify_flag.html:4 (Diff revision 1) > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Bug 1207775 - Ensure that the channel classifier is used for all HTTP connections.</title> nit: I would use "Bug 1233914 - ping doesn't honor the TP list" here. ::: toolkit/components/url-classifier/tests/mochitest/test_classify_flag.html:32 (Diff revision 1) > + // Trigger ping. > + elm.click(); > + > + return new Promise(function(resolve, reject) { > + function checkPing() { > + // Temporarily disable safebrowsing because we need to know if the page is pinged. nit: indentation ::: toolkit/components/url-classifier/tests/mochitest/test_classify_flag.html:39 (Diff revision 1) > + var xhr = new XMLHttpRequest(); > + xhr.open('GET', url + "?id=1234"); > + xhr.onload = function() { > + SpecialPowers.setBoolPref("browser.safebrowsing.malware.enabled", true); > + > + ok(!xhr.response, "Page is pinged"); I don't understand this part of the test. Why is a lack of response body indicative of a successful ping? From what I can see in ping.sjs, a successful ping should instead return the text "ping" in the body of the response. Did I read that wrong? ::: toolkit/components/url-classifier/tests/mochitest/test_classify_flag.html:84 (Diff revision 1) > + > + classifierCommonScript.sendAsyncMessage("doUpdate", { testUpdate }); > + } > + > + SimpleTest.waitForExplicitFinish(); > + SpecialPowers.pushPrefEnv({"set": [ Are these prefs cleared later? https://dxr.mozilla.org/mozilla-central/rev/a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm#52-55
Attachment #8735780 - Flags: review?(francois)
Attachment #8735779 - Flags: review?(francois)
Comment on attachment 8735779 [details] MozReview Request: Bug 1233914 - P1. ping doesn't honor the TP list. r=francois https://reviewboard.mozilla.org/r/42943/#review39621 ::: docshell/base/nsDocShell.cpp:527 (Diff revision 1) > > if (!chan) { > return; > } > > // Don't bother caching the result of this URI load. I would add "but do not exempt it from Safe Browsing." to the end of that comment.
(In reply to François Marier [:francois] from comment #4) > To make sure that the test case never breaks, I would suggest checking > (using the same ping.sjs code) these cases: > > 1. When we ping a non-blaclisted URL, the ping does work successfully. > 2. When we ping a blacklisted URL with Safe Browsing turned off, the ping > does work successfully. > 3. When we ping a blacklisted URL with Safe Browsing turned on, the ping is > blocked. (i.e. what the test does already) OK!, I will add scenario 1 and 2 to the testcase, :) > > ::: toolkit/components/url-classifier/tests/mochitest/mochitest.ini:25 > (Diff revision 1) > > + ping.sjs > > > > [test_classifier.html] > > skip-if = (os == 'linux' && debug) #Bug 1199778 > > [test_classifier_worker.html] > > +[test_classify_flag.html] > > I would prefer a more descriptive name here, like "test_classify_ping" > because "classify_flag" applies to all of the other bugs we need to fix :) > Originally i plan to make this bug to test all classify flag bugs, but i think this testcase is getting complicated, I will rename the file and update description as you suggest. > ::: > toolkit/components/url-classifier/tests/mochitest/test_classify_flag.html:39 > (Diff revision 1) > > + var xhr = new XMLHttpRequest(); > > + xhr.open('GET', url + "?id=1234"); > > + xhr.onload = function() { > > + SpecialPowers.setBoolPref("browser.safebrowsing.malware.enabled", true); > > + > > + ok(!xhr.response, "Page is pinged"); > > I don't understand this part of the test. Why is a lack of response body > indicative of a successful ping? > > From what I can see in ping.sjs, a successful ping should instead return the > text "ping" in the body of the response. > > Did I read that wrong? > Currently i only test fail case(ping doesn't reach ping.sjs), so lack of response body means safebrowsing works successfully here. > ::: > toolkit/components/url-classifier/tests/mochitest/test_classify_flag.html:84 > (Diff revision 1) > > + > > + classifierCommonScript.sendAsyncMessage("doUpdate", { testUpdate }); > > + } > > + > > + SimpleTest.waitForExplicitFinish(); > > + SpecialPowers.pushPrefEnv({"set": [ > > Are these prefs cleared later? > > https://dxr.mozilla.org/mozilla-central/rev/ > a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/toolkit/components/url-classifier/ > tests/UrlClassifierTestUtils.jsm#52-55 I will add this, thanks !
After running testcase for multiple times, i found sometimes the url-classifier did cancel the channel but "ping" still sent. Need investigate more to find out the root cause.
(In reply to Dimi Lee[:dimi][:dlee] from comment #7) > After running testcase for multiple times, i found sometimes the > url-classifier did cancel the channel but "ping" still sent. > Need investigate more to find out the root cause. The root cause is the testcase in patch test malware protection instead of track protection. Classify a malware uri[1] happens after http connect[2], so suspend the channel when classify uri doesn't really block connect[3], only suspend |nsInputStream|. So POST request of ping may still sent if |channel.cancel| called after sending the ping request. Classify tracking uri doesn't have this issue because it is called before connect. [1]https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5460 [2]https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5448 [3]https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5538
Comment on attachment 8735779 [details] MozReview Request: Bug 1233914 - P1. ping doesn't honor the TP list. r=francois Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42943/diff/1-2/
Attachment #8735779 - Flags: review?(francois)
Attachment #8735780 - Flags: review?(francois)
Comment on attachment 8735780 [details] MozReview Request: Bug 1233914 - P2. Testcase. r=francois Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42945/diff/1-2/
Attachment #8735779 - Flags: review?(francois) → review+
Comment on attachment 8735779 [details] MozReview Request: Bug 1233914 - P1. ping doesn't honor the TP list. r=francois https://reviewboard.mozilla.org/r/42943/#review40181
Attachment #8735780 - Flags: review?(francois) → review+
Comment on attachment 8735780 [details] MozReview Request: Bug 1233914 - P2. Testcase. r=francois https://reviewboard.mozilla.org/r/42945/#review40183 Looks great, thanks for testing all of the possible cases here! ::: toolkit/components/url-classifier/tests/mochitest/test_classify_ping.html:4 (Diff revision 2) > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Bug 1233914 - ping doesn't honor the TP list" here.</title> Copy-paste typo: `" here.`
Comment on attachment 8735780 [details] MozReview Request: Bug 1233914 - P2. Testcase. r=francois Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42945/diff/2-3/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b1de959c9963 https://hg.mozilla.org/mozilla-central/rev/6fc436f9db85
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: