1547704 - Investigate if CaptiveDetect.jsm requires LOAD_BYPASS_URL_ClASSIFIER flag

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, task, P2)

Description

[Dimi Lee [:dimi]]

Check if CaptiveDetect.jsm[1] requires LOAD_BYPASS_URL_ClASSIFIER[2].

[1] https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/security/manager/ssl/nsNSSCallbacks.cpp#264

[2] https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/netwerk/base/nsIChannel.idl#255

Comment 1

[Dimi Lee [:dimi]]

Hi Valentin,
We have recently changed the behavior of how URL Classifier determines whether a channel should be classified[1] by using the information in the channel.
To be extra careful for not blocking critical channel because of bugs, we added this LOAD_BYPASS_URL_ClASSIFIER flag as a safeguard, whenever we see this flag, we do not classify it no matter the information in the channel.

I am not familiar with CaptiveDetect.jsm, do you think the channel here[2] is a critical channel and needs the
flag to make sure it is never classified?

Here are some sample ideas about what is a critical channel:

• channels related to an update
• channels may cause security issues if blocked
• channels may prevent the browser to launch if blocked
• channels break basic functionality if blocked

thank you!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1522412
[2] https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/toolkit/components/captivedetect/CaptiveDetect.jsm#22

Comment 2

[Valentin Gosu [:valentin] (he/him)]

(In reply to Dimi Lee [:dimi][:dlee] from comment #1)

• channels break basic functionality if blocked

Depends if you count captive portal detection as basic functionality.
A classification failure likely wouldn't be the end of the world, but performing the classification for this endpoint is probably pointless anyway. I say we should add the flag. And also to the NetworkConnectivityService

Comment 3

[Dimi Lee [:dimi]]

(In reply to Valentin Gosu [:valentin] from comment #2)

(In reply to Dimi Lee [:dimi][:dlee] from comment #1)

• channels break basic functionality if blocked

Depends if you count captive portal detection as basic functionality.
A classification failure likely wouldn't be the end of the world, but performing the classification for this endpoint is probably pointless anyway. I say we should add the flag. And also to the NetworkConnectivityService

Thanks, Valentin!
So I guess we don't need to add the flag.
No matter if the flag is added or not, both of the channels created in CaptiveDetect.jsm and in NetworkConnectivityService won't be classified.
The flag acts as an extra safeguard to ensure URL classifier doesn't count on the information in the channel to decide if it should be classified, so even if the data is wrong, we will still skip classification.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

We decided this counts as basic functionality, as it's sometimes used for TRR and proxy operations.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1547704 - CaptiveDetect.jsm requires LOAD_BYPASS_URL_ClASSIFIER flag r=mayhemer

Comment 6

[Dimi Lee [:dimi]]

Hi Valentin,
Do you think it is ok to use ServiceRequest[1] to replace XHR here?
We also force bypassing classifier check when beConservative is set [2]

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1325501
[2] https://searchfox.org/mozilla-central/rev/c0ca77697c6868482f30af873ec8069f2c080a34/netwerk/base/nsNetUtil.cpp#3096

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8b0d976dd31
CaptiveDetect.jsm requires LOAD_BYPASS_URL_ClASSIFIER flag r=mayhemer

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f8b0d976dd31