The default bug view has changed. See this FAQ.

Make nsContentUtils and nsObjectLoadingContent use TestPermissionFromPrincipal instead of TestPermission

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 637824 [details] [diff] [review]
Patch
Attachment #637824 - Flags: review?(jonas)
(Assignee)

Comment 1

5 years ago
Comment on attachment 637824 [details] [diff] [review]
Patch

I will use that bug for various content/ and dom/ changes.
Attachment #637824 - Flags: review?(jonas)
(Assignee)

Comment 2

5 years ago
Actually, just too...
Summary: Make nsContentUtils use TestPermissionFromPrincipal instead of TestPermission → Make nsContentUtils andnsObjectLoadingContent use TestPermissionFromPrincipal instead of TestPermission
(Assignee)

Comment 3

5 years ago
Created attachment 637827 [details] [diff] [review]
Patch
Attachment #637824 - Attachment is obsolete: true
Attachment #637827 - Flags: review?(jonas)
Blocks: 756644
Attachment #637827 - Flags: review?(jonas) → review+
(Assignee)

Updated

5 years ago
Depends on: 774819
(Assignee)

Comment 4

5 years ago
Created attachment 643168 [details] [diff] [review]
Patch v1.1
Attachment #637827 - Attachment is obsolete: true
Created attachment 643217 [details] [diff] [review]
Patch v2
Attachment #643168 - Attachment is obsolete: true
Attachment #643217 - Flags: review?(mounir)
(Assignee)

Comment 6

5 years ago
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.
Attachment #643217 - Flags: review?(mounir) → review-
(Assignee)

Comment 7

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

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Comment 8

5 years ago
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 ;)
Attachment #643217 - Flags: review- → review+
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.
Whiteboard: [leave open]
Attachment #643217 - Flags: checkin+
Comment on attachment 643217 [details] [diff] [review]
Patch v2

This got backed out again
Attachment #643217 - Flags: checkin+
Created attachment 643727 [details] [diff] [review]
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?
Attachment #643217 - Attachment is obsolete: true
Attachment #643727 - Flags: review?(dkeeler)
Whiteboard: [leave open]
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.
Attachment #643727 - Flags: review?(dkeeler)
Comment on attachment 643727 [details] [diff] [review]
Patch v3

Jared: Could you have a look at the test changes here.
Attachment #643727 - Flags: review?(jaws)
https://hg.mozilla.org/mozilla-central/rev/78e6e0765f35
Summary: Make nsContentUtils andnsObjectLoadingContent use TestPermissionFromPrincipal instead of TestPermission → Make nsContentUtils and nsObjectLoadingContent use TestPermissionFromPrincipal instead of TestPermission
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) {
  ...
}
Attachment #643727 - Flags: review?(joshmoz)
Attachment #643727 - Flags: review?(jaws)
Attachment #643727 - Flags: review+

Comment 16

5 years ago
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.
Attachment #643727 - Flags: review?(joshmoz) → review+
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 on attachment 643727 [details] [diff] [review]
Patch v3

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

re-asking for review. See comment above.
Attachment #643727 - Flags: review+ → review?(joshmoz)

Comment 19

5 years ago
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.
Attachment #643727 - Flags: review?(joshmoz) → review+
Jonas, can you add a comment to nsObjectLoadingContent.cpp to state why SystemPrincipals default to allow=false?
blocking-basecamp: --- → +
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5cd832d82ef
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/c5cd832d82ef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.