1233906 - application reputation needs use the right user context origin attributes

Status: RESOLVED WONTFIX

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

Description

[Dave Huseby [:huseby]]

In the file toolkit/components/downloads/ApplicationReputation.cpp there is a call to createCodebasePrincipal that needs to have the default context origin attributes because we don't want user context isolation.

> toolkit/components/downloads/ApplicationReputation.cpp
>   line 299
>     nsCOMPtr<nsIPrincipal> principal =
>       BasePrincipal::CreateCodebasePrincipal(uri, attrs);

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Dave Huseby [:huseby]]

Comment on attachment 8703750 [details] [diff] [review]
patch

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

GlobalContextOriginAttributes -> DefaultContextOriginAttributes

Comment 3

[Andrea Marchesini [:baku]]

Attachment: bug_1233906.patch

Comment 4

[Andrea Marchesini [:baku]]

Attachment: bug_1233906.patch

Comment 5

[Dave Huseby [:huseby]]

Francois,

Are you familiar with the Application Reputation Service?  In the PendingDBLookup class, it creates a principal that it passes to the nsIUrlClassifierDBService when checking URI's for downloaded resources.  I'm not sure what the nsIUrlClassifierDBService does exactly so I'm not sure if we want to use a principal with default origin attributes or if we want to inherit the origin attributes from the document that loaded the resource.

If we want to inherit the origin attributes, I think we'll have to add a the origin attributes to the nsIApplicationReputationQuery class so that we have the origin attributes when creating the principal in PendingDBLookup.

Can you help me understand this better?

Comment 6

[Tanvi Vyas[:tanvi]]

Application reputation should probably be usercontext free.  It doesn't matter the context in which we detect a malicious url.  A malicious url is malicious and should be blocked regardless of context.

Not sure why francois removed his needinfo.  Perhaps him and Dave talked offline.

Comment 7

[François Marier [:francois]]

(In reply to Tanvi Vyas [:tanvi] from comment #6)
> Not sure why francois removed his needinfo.  Perhaps him and Dave talked
> offline.

Yes, Dave and I had a chat about it.

The conclusion was that we're going to leave the code as-is until the permission manager becomes origin-attribute aware. At that point, it might make sense to treat the "ignore this warning" permission (i.e. temporary user whitelist) as specific to the usercontext in which it occurred.

Comment 8

[Dave Huseby [:huseby]]

I'm resolving this as won't fixed because permissions aren't isolated at the moment.  When the time comes, we'll fix this.