Closed Bug 1547704 Opened 7 months ago Closed 5 months ago

Investigate if CaptiveDetect.jsm requires LOAD_BYPASS_URL_ClASSIFIER flag

Categories

(Toolkit :: Safe Browsing, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: dimi, Assigned: valentin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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

Flags: needinfo?(valentin.gosu)

(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

Flags: needinfo?(valentin.gosu)

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

Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → WONTFIX

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

Assignee: nobody → valentin.gosu
Status: RESOLVED → REOPENED
Type: enhancement → task
Priority: -- → P2
Resolution: WONTFIX → ---

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

Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f8b0d976dd31
CaptiveDetect.jsm requires LOAD_BYPASS_URL_ClASSIFIER flag r=mayhemer
Status: REOPENED → RESOLVED
Closed: 7 months ago5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.