Change behavior of subdocument Flash blocking to be third-party Flash blocking

RESOLVED FIXED in Firefox 53

Status

()

P1
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: bytesized, Assigned: bytesized)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

Attachments

(1 attachment)

List-based Flash Blocking (Bug 1307604) currently uses a list to block Flash in subdocuments, but it should instead block Flash in third-party subdocuments only.

Subdocuments that will now be exempted from the third-party Flash block list can be described as follows:

 - The subdocument must have the exact same domain, subdomain and scheme (http/https) as the top level document.
 - The subdocument must not have been loaded within a third-party subdocument.

Comment 1

2 years ago
Yep sorry again for missing that detail!
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8846066 - Flags: review?(benjamin)
(Assignee)

Updated

2 years ago
Attachment #8846066 - Flags: review?(kyle)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121150

::: toolkit/components/url-classifier/flash-block-lists.rst:45
(Diff revision 1)
> +Third-Party Documents
> +=====================
> +
> +For a document to be considered a Third-Party document, it must not be a top-level document, and must meet AT LEAST ONE of these requirements:
> +
> +* The document's parent is a Third-Party document.

These rules describe what "third-party" means in a roundabout way. Rather than hand-coding a definition of third-party, we should be using standard same-origin checks which already do all of this for free and probably catch the edge cases.

nsIPrincipal->Equals(parent)
Attachment #8846066 - Flags: review?(benjamin) → review-

Comment 4

2 years ago
mozreview-review
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121152

::: commit-message-800ba:1
(Diff revision 1)
> +Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

Also, this patch appears to not have any tests.
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121152

> Also, this patch appears to not have any tests.

There is an existing Flash Blocking test (toolkit/components/url-classifier/tests/browser/browser_flash_block_lists.js), which I have modified to verify that Flash blocking works as expected for third-party and non-third-party subdocuments. The test modifications are included in this patch. Did you want additional testing beyond this?
(Assignee)

Updated

2 years ago
Flags: needinfo?(benjamin)
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121150

> These rules describe what "third-party" means in a roundabout way. Rather than hand-coding a definition of third-party, we should be using standard same-origin checks which already do all of this for free and probably catch the edge cases.
> 
> nsIPrincipal->Equals(parent)

I will change the code to use `nsIPrincipal::Equals`, but it sounds like you are also unhappy with my documentation for the desired functionality. I thought that I summarized our conversation regarding this well. Could you tell me how you would like the documentation changed?

Comment 7

2 years ago
I think that rather than redefining third-party, this should just say "subdocument which is not same-origin" and assume the web-standards definition of a same-origin frame.
Flags: needinfo?(benjamin)
What about the additional testing (Comment 5)?
Flags: needinfo?(benjamin)

Comment 9

2 years ago
Is there any automated testing for this feature? If so those tests should be updated to reflect this change in the policy. Otherwise this really needs some mochitests.
Flags: needinfo?(benjamin)
As I said in Comment 5, there is an automated test for this feature located at toolkit/components/url-classifier/tests/browser/browser_flash_block_lists.js which this patch modifies to reflect this change in policy.

Is additional testing, other than what I have mentioned, needed here?
Flags: needinfo?(benjamin)
Oh, I'm sorry I somehow missed that. I was looking for a mochitest. Yes that looks like it covers what you need.
Flags: needinfo?(benjamin)
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121994

r=me but I'm not a DOM peer so qdot or bz also needs to review this
Attachment #8846066 - Flags: review?(benjamin) → review+
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review122194

LGTM
Attachment #8846066 - Flags: review?(kyle) → review+

Comment 15

2 years ago
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3defd1bf850f
Change behavior of subdocument Flash blocking to be Third-Party Flash blocking r=bsmedberg,qdot
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Study's handling of third-party domains will be incorrect
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not for the feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: This changes the behavior of code that is currently pref-ed off and will only be exposed to users who are part of the study.
[String changes made/needed]: none
Attachment #8846066 - Flags: approval-mozilla-beta?
Attachment #8846066 - Flags: approval-mozilla-aurora?

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3defd1bf850f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

For shield study purpose of making flash click-to-play by default. Beta53+ & Aurora54+.
Attachment #8846066 - Flags: approval-mozilla-beta?
Attachment #8846066 - Flags: approval-mozilla-beta+
Attachment #8846066 - Flags: approval-mozilla-aurora?
Attachment #8846066 - Flags: approval-mozilla-aurora+
status-firefox53: --- → affected
status-firefox54: --- → affected

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0383139bd7b
status-firefox54: affected → fixed
has problems on beta , so maybe need a rebase or so 

grafting 405310:d0383139bd7b "Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking r=bsmedberg,qdot a=gchang"
merging dom/base/nsDocument.cpp
merging dom/base/nsDocument.h
merging dom/base/nsIDocument.h
merging toolkit/components/url-classifier/tests/browser/browser_flash_block_lists.js
warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/base/nsDocument.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/base/nsIDocument.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(ksteuber)
Ah. It seems that I requested that Bug 1338287 be uplifted to 52 (which was denied), but not 53. I will request uplift on that bug first.
Flags: needinfo?(ksteuber)
(In reply to Kirk Steuber [:bytesized] from comment #21)
> Ah. It seems that I requested that Bug 1338287 be uplifted to 52 (which was
> denied), but not 53. I will request uplift on that bug first.

Hey Kirk, this actually did the trick :) worked after the other bug landed :)

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/993b574eb828
status-firefox53: affected → fixed
I'm porting the flash-classifier to a necko layer and I wonder why we have nsIDocument::IsThirdParty() instead of using mozIThirdPartyUtil::IsThirdPartyWindow/IsThirdPartyChannel().

I'm asking that because they produce 2 different results: mozIThirdPartyUtil considers "first-party" if URLs have the same base-domain, but nsIDocument::IsThirdParty() doesn't. This means that: http://subdomain.example.com is a first-party of http://example.com with the former, but it's not with the latter method.

Do we have any particular constrains related to the flash-classifier to have this nsIDocument::IsThirdParty() or we can unify our code base and use mozIThirdPartyUtil?
Flags: needinfo?(ksteuber)
Discussed on IRC
Flags: needinfo?(ksteuber)
You need to log in before you can comment on or make changes to this bug.