All users were logged out of Bugzilla on October 13th, 2018

Only send Prefer:Safe header if Web Filtering is on (not just Parental Controls in general) on Windows

VERIFIED FIXED in Firefox 35

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

unspecified
mozilla36
All
Windows 8
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox34 wontfix, firefox35 verified, firefox36 verified)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 8509273 [details] [diff] [review]
v1

We're currently sending Prefer:Safe if any Parental Controls are set.  We should limit this to only sending it when Web Filtering is set, as there are parental control configurations (like limiting time spent online, but w/o any filtering) that shouldn't be telling web sites to modify their content.

One wrinkle is that IIUC (I'm checking with some sources) the IWPCSettings::GetRestrictions() system call does not actually correctly provide the granularity we need (i.e. it doesn't correctly set WPCFLAG_WEB_FILTERED when filtering is on) unless we're on Windows >= 8.  If true we only get a monolithic Parental Controls are on/off to base our decision on. I'm assuming we want to default to assuming filtering is on (and sending Prefer:Safe) in that case.

Patch attached. I'm holding off on formal review until I've verified the API issues here.
Attachment #8509273 - Flags: feedback?(dougt)
Comment on attachment 8509273 [details] [diff] [review]
v1

I've gotten word from Microsoft that indeed the WPCFLAG_WEB_FILTERED flag is not reliable on Win7.  So this ready for review.  Doug, note the policy choice to interpret filtering as "on" if parental controls are on at all when we don't have the ability to find out for sure.  That's apparently what IE does--I have no opinion on whether that's best for our users too, but it's what the patch does right now.
Attachment #8509273 - Flags: feedback?(dougt) → review?(dougt)
I don't have a windows box, so this will need to be tested by someone.  In a nutshell we need to 

1) Windows 7 and earlier: test with Parental Controls On and verify that Prefer:Safe is sent by the browser when they are on and not when they're off.

  - Setup:  
  http://windows.microsoft.com/en-us/windows7/products/features/parental-controls

  - use Wireshark to see if Prefer:Safe is sent.  Or a site that we know behaves differently for prefer:safe (but seeing it on the wire seems easiest and most definitive to me).

2) Windows 8: The same tests as for #1, except that we want to test


IIUC the setup is

  1) set up Windows Parental Controls (looks like it's different
Keywords: qawanted
Sorry, somehow hit Submit before I was done.  Let me redo #2

2) Windows 8: The same tests as for #1, except that we want to test

- 1) Parental controls off: no Prefer:safe
- 2) Parental controls on, but Web Filtering is not: no Prefer:safe
- 3) Parental controls on, including web filtering: Prefer:safe gets sent.

Set up for windows 8 is here:

  http://windows.microsoft.com/en-us/windows-8/family-safety
Doug--are you waiting for QA here, or vice versa?
Flags: needinfo?(dougt)

Comment 5

4 years ago
Comment on attachment 8509273 [details] [diff] [review]
v1

Review of attachment 8509273 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/parentalcontrols/nsParentalControlsServiceWin.cpp
@@ +27,5 @@
>  
> +// Windows 7 can only detect whether Parental Controls are on/off: Windows >= 8
> +// has more granularity and can tell specifically whether Web Filtering is on
> +// (vs configs like restricted hours w/o filtering)
> +static bool CanDetectWebFiltering()

can we use http://mxr.mozilla.org/mozilla-central/source/mfbt/WindowsVersion.h?

IsWin8OrLater()
Attachment #8509273 - Flags: review?(dougt) → review+
Created attachment 8515242 [details] [diff] [review]
v2: uses IsWin8OrLater()

> can we use http://mxr.mozilla.org/mozilla-central/source/mfbt/WindowsVersion.h?
> 
> IsWin8OrLater()

Nice--thanks.
Assignee: nobody → jduell.mcbugs
Flags: needinfo?(dougt)
Attachment #8515242 - Flags: review+
Created attachment 8515323 [details] [diff] [review]
v3

Forgot to qref
Attachment #8515242 - Attachment is obsolete: true
Attachment #8515323 - Flags: review+
Jason,

I tried applying the patch from comment #7 into m-c on both Win 7 and Win 8 machines but the build keeps failing. I tried several times but keep hitting the same errors.

STR:

- downloaded the patch from comment #7 and renamed it from 1087182_windows_parental.v3 to 1087182_windows_parental.diff
- hg qimport 1087182_windows_parental.diff
- hg qpush
- hg qapplied (made sure the patch was applied)
- ./mach build > out.txt 2>&1 (build fails)
- ./mach clobber
- hg qpop
- hg qapplied (made sure that patch wasn't applied anymore)
- ./mach build > out.txt 2>&1 (build finishes and runs with ./mach run)

* http://pastebin.mozilla.org/7120892 [partial Windows 7 log]
* http://pastebin.mozilla.org/7120809 [partial Windows 8 log]

Let me know if there's something I'm missing or doing incorrectly.
Created attachment 8517650 [details] [diff] [review]
v4: was missing "use namespace mozilla"
Attachment #8515323 - Attachment is obsolete: true
Attachment #8517650 - Flags: review+
Thanks Jason, that definitely did the trick! As per the instructions in comment #2, I went through the following verification:

Once the settings were applied, used WireShark and filtered by HTTP packets. Visited several websites and ensured that "Prefer: safe\r\n" was appearing/not appearing under Hypertext Transfer Protocol for each HTTP request packet.

Windows 7 Pro x64:
==================

Tommy (Parental Controls Off):
- Parental Controls Off: "Prefer: safe\r\n" not being sent via the HTTP request header

Jimmy (Parental Controls On):
- Parental Controls On: "Prefer: safe\r\n" is appearing under the HTTP request header

Windows 8.1 x64:
================

John (Parental Controls On)
- Parental Controls On; Web Filtering Off: "Prefer: safe\r\n" not being sent via the HTTP request header
- Parental Controls On; Web Filtering On: "Prefer: safe\r\n" is appearing under the HTTP request header

Peter (Parental Controls Off)
- Parental Controls Off: "Prefer: safe\r\n" not being sent via the HTTP request header
Jason, looks like everything is working as expected. Let me know if there's something I missed.
Flags: needinfo?(jduell.mcbugs)
OK, looks like this is ready to land.  Doug, do we want this on Aurora/Beta?
Flags: needinfo?(jduell.mcbugs) → needinfo?(dougt)
Keywords: qawanted → checkin-needed
Flags: needinfo?(dougt)
Comment on attachment 8517650 [details] [diff] [review]
v4: was missing "use namespace mozilla"

Approval Request Comment
[Feature/regressing bug #]: Been broken for long time
[User impact if declined]: parental controls don't work for a subset of use cases (we always filter even if parent hasn't chosen to turn on web filtering)
[Describe test coverage new/current, TBPL]:  Manually tested.
[Risks and why]:  very low--codepath only hit when parental controls turned on, one line fix even then.
[String/UUID change made/needed]:  none
Attachment #8517650 - Flags: approval-mozilla-release?
Attachment #8517650 - Flags: approval-mozilla-beta?
Attachment #8517650 - Flags: approval-mozilla-aurora?
Jason, could you provide a try link for this change? Thanks!
Keywords: checkin-needed
Comment on attachment 8517650 [details] [diff] [review]
v4: was missing "use namespace mozilla"

Even if we do a 33.1.1, we won't take this one since we have this bug for a while
Attachment #8517650 - Flags: approval-mozilla-release? → approval-mozilla-release-
https://hg.mozilla.org/mozilla-central/rev/60b2d69d81ca
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8517650 [details] [diff] [review]
v4: was missing "use namespace mozilla"

Given that we've shipped with this for a while and we're at the final beta in the 34 cycle, I think we should opt to do the safe thing and ship this fix in 35. Although this is a small patch, it has only just landed on m-c and has had no testing with any of our pre-release audiences at this point.
Attachment #8517650 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8517650 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c775c8888ef
status-firefox33: --- → wontfix
status-firefox34: --- → wontfix
status-firefox35: --- → fixed
status-firefox36: --- → fixed
I went through m-c verification using changeset ae27ae77e32f with the test cases listed in comment # 10. I received "Prefer: safe\r\n" under the HTTP request header when expected.
status-firefox36: fixed → verified
I went through m-a verification using changeset 390a34a40ea4 with the test cases listed in comment # 10. I received "Prefer: safe\r\n" under the HTTP request header when expected.
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
You need to log in before you can comment on or make changes to this bug.