769594 - Make nsContentUtils and nsObjectLoadingContent use TestPermissionFromPrincipal instead of TestPermission

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 1

[Mounir Lamouri (:mounir)]

Comment on attachment 637824 [details] [diff] [review]
Patch

I will use that bug for various content/ and dom/ changes.

Comment 2

[Mounir Lamouri (:mounir)]

Actually, just too...

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 643217 [details] [diff] [review]
Patch v2

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

That means we *never* gives "plugins" permissions to system principal? It might fix the failing test but that seems quite wrong.

Comment 7

[Mounir Lamouri (:mounir)]

I pushed the nsContentUtils changes given that _they_ pass try obviously:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78e6e0765f35

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 643217 [details] [diff] [review]
Patch v2

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

It seems that the plugin guys actually want that behavior, so r=me.
But be careful, only land the changes in nsObjectLoadingContent.cpp ;)

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I talked with keeler and he gave an ok over irc to go with the current patch since it maintains current behavior. I'll file a followup to figure out a policy for plugins in system-principal documents.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 643217 [details] [diff] [review]
Patch v2

This got backed out again

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v3

This seems to pass tests.

Keeler: I tried using the http URL for all tests, but some test started throwing due to a missing function, so it seems like some of the tests depend on being loaded as chrome. So I only changed the 12* tests.

Does this look ok?

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 643727 [details] [diff] [review]
Patch v3

The changes to the test look fine to me. The thing is, I'm not actually a reviewer (as far as I know - I'm assuming there's some sort of process or confirmation?), so you'll probably want to ask someone like Jared Wein if you need a formal review. So, I'm just going to clear the review flag.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 643727 [details] [diff] [review]
Patch v3

Jared: Could you have a look at the test changes here.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/78e6e0765f35

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 643727 [details] [diff] [review]
Patch v3

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

Josh, can you review the nsObjectLoadingContent.cpp changes here?

r=me for the test changes, but Josh needs to sign off on this too.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +499,5 @@
>  
>      nsCOMPtr<nsIPermissionManager> permissionManager = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    bool allowPerm = false;
> +    if (!nsContentUtils::IsSystemPrincipal(topDoc->NodePrincipal())) {

allowPerm will be false for SystemPrincipals, which I think is wrong here.

Can you change this to:
bool allowPerm = nsContentUtils::IsSystemPrincipal(topDoc->NodePrincipal());
if (!allowPerm) {
  ...
}

Comment 16

[Josh Aas]

Comment on attachment 643727 [details] [diff] [review]
Patch v3

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

I'm fine with this, but what :jaws said.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I talked this over with keeler yesterday.

The patch currently maintains existing behavior. Right now the code enforces click-to-play for chrome URIs unless someone has somehow managed to get chrome URIs into nsIPermissionManager, which generally shouldn't happen.

In fact, we even have tests which depends on nsIPermissionManager not returning ALLOW for these chrome URIs.

The way that the principal-aware functions on nsIPermissionManager is that they always return ALLOW for the system principal. However that caused click-to-play tests to fail. I'm happy to change these tests, but I need help if so.

I can't find the test in question any more, the tryserver pushes are too old. But I can re-push to get results again.

However, is it really worth making plugins not use click-to-play in chrome documents given that it seems like there's agreement that plugins shouldn't work in chrome documents at all?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 643727 [details] [diff] [review]
Patch v3

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

re-asking for review. See comment above.

Comment 19

[Josh Aas]

Comment on attachment 643727 [details] [diff] [review]
Patch v3

I'm fine with not dragging you into changing the existing behavior, but we should get to that sooner rather than later.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Jonas, can you add a comment to nsObjectLoadingContent.cpp to state why SystemPrincipals default to allow=false?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5cd832d82ef

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5cd832d82ef