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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Robin Steuber (they/them) [:bytesized]]

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

[Benjamin Smedberg]

Yep sorry again for missing that detail!

Comment 2

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

The previous implementation regarding to the Flash Blocking Subdocument list blocked all subdocuments that matched the list. This patch changes that so that subdocuments are only blocked if they are on the Subdocument Block List and also are loaded in a Third-Party context.

Subdocuments are classified as third-party if they meet ANY of the following criteria:
 - The subdocument's parent document is classified as third-party.
 - The subdocument's scheme (http/https) does not match its parent's scheme.
 - The subdocument's domain and subdomains do not match those its parent.

The changes to cert8.db and key3.db add the https certificate for subdocument.example.com so that testing can verify that a scheme mismatch between the document and its parent results in a third-party classification.

Review commit: https://reviewboard.mozilla.org/r/119150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119150/

Comment 3

[Benjamin Smedberg]

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)

Comment 4

[Benjamin Smedberg]

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.

Comment 5

[Robin Steuber (they/them) [:bytesized]]

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?

Comment 6

[Robin Steuber (they/them) [:bytesized]]

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

[Benjamin Smedberg]

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.

Comment 8

[Robin Steuber (they/them) [:bytesized]]

What about the additional testing (Comment 5)?

Comment 9

[Benjamin Smedberg]

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.

Comment 10

[Robin Steuber (they/them) [:bytesized]]

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?

Comment 11

[Benjamin Smedberg]

Oh, I'm sorry I somehow missed that. I was looking for a mochitest. Yes that looks like it covers what you need.

Comment 12

[Robin Steuber (they/them) [:bytesized]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119150/diff/1-2/

Comment 13

[Benjamin Smedberg]

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

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3defd1bf850f
Change behavior of subdocument Flash blocking to be Third-Party Flash blocking r=bsmedberg,qdot

Comment 16

[Robin Steuber (they/them) [:bytesized]]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3defd1bf850f

Comment 18

[Gerry Chang [:gchang]]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d0383139bd7b

Comment 20

[Carsten Book [:Tomcat]]

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')

Comment 21

[Robin Steuber (they/them) [:bytesized]]

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.

Comment 22

[Carsten Book [:Tomcat]]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/993b574eb828

Comment 24

[Andrea Marchesini [:baku]]

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?

Comment 25

[Robin Steuber (they/them) [:bytesized]]

Discussed on IRC