Closed Bug 1087182 Opened 6 years ago Closed 6 years ago
Only send Prefer:Safe header if Web Filtering is on (not just Parental Controls in general) on Windows
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
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?
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+
> can we use http://mxr.mozilla.org/mozilla-central/source/mfbt/WindowsVersion.h? > > IsWin8OrLater() Nice--thanks.
Assignee: nobody → jduell.mcbugs
Attachment #8515242 - Flags: review+
Forgot to qref
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.
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.
OK, looks like this is ready to land. Doug, do we want this on Aurora/Beta?
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
Jason, could you provide a try link for this change? Thanks!
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-
Status: NEW → RESOLVED
Closed: 6 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+
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.
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.
You need to log in before you can comment on or make changes to this bug.