Closed Bug 769594 Opened 8 years ago Closed 8 years ago

Make nsContentUtils and nsObjectLoadingContent use TestPermissionFromPrincipal instead of TestPermission

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17
blocking-basecamp +

People

(Reporter: mounir, Assigned: mounir)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 4 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
No description provided.
Attachment #637824 - Flags: review?(jonas)
Comment on attachment 637824 [details] [diff] [review]
Patch

I will use that bug for various content/ and dom/ changes.
Attachment #637824 - Flags: review?(jonas)
Actually, just too...
Summary: Make nsContentUtils use TestPermissionFromPrincipal instead of TestPermission → Make nsContentUtils andnsObjectLoadingContent use TestPermissionFromPrincipal instead of TestPermission
Attached patch Patch (obsolete) — Splinter Review
Attachment #637824 - Attachment is obsolete: true
Attachment #637827 - Flags: review?(jonas)
Depends on: 774819
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #637827 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #643168 - Attachment is obsolete: true
Attachment #643217 - Flags: review?(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.
Attachment #643217 - Flags: review?(mounir) → review-
I pushed the nsContentUtils changes given that _they_ pass try obviously:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78e6e0765f35
Whiteboard: [leave open]
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.
Attached patch Patch v3Splinter Review
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)
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)
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 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 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?
https://hg.mozilla.org/mozilla-central/rev/c5cd832d82ef
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.