[FIXr]Content policy checks for XBL loads show up in profiles

RESOLVED FIXED

Status

()

Core
XBL
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({regression})

unspecified
x86
Linux
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Opening the font preferences dialog, about 20% of the total time is spent doing content policy checks for the (chrome) XBL loads for whatever it's doing.  This is a change from bug 379959.

I'd like to get bug 204140 in before addressing this, though, because that will change the loading principal.
Flags: blocking1.9?
Maybe we want to generally pass the loading principal to NS_CheckContentLoadPolicy and skip content policy checks if it's system?  And maybe change skin stuff to get the system principal the same way content stuff does?

I doubt any content policy impls really want to catch cases of chrome loading things anyway...

Longer term, passing the loading principal to content policy would be a good idea, imo.  Better than passing the origin URI in some ways.
Sounds like a great plan to me.
We could even just pass the loading principal to NS_CheckContentLoadPolicy for now and let that dig out the URI when passing it to the cp impls. I always feel safer when things take principals than uris, less risk to be grabbing stuff like baseuris that way.
Hmm.  That's a good point.  Ok, lemme see what I can do here.  ;)
Sounds good to me.
sicking, in txSyncCompileObserver::loadURI how are aReferrerURI and mCallerPrincipal related?  Which one should be used for the content policy check?

It looks like we use aReferrerURI for the same-origin check....
Use mCallerPrincipal.

Say that page A links to stylesheet B which links to stylesheet C. When loading stylesheet C, mCallerPrincipal will be that of page A, whereas aReferrerURI will be that of stylesheet B.

This does feel like the desirable behavior, but I'm not really sure. What does the CSS code do?

For same-origin checks it doesn't matter much right now as there are same-origin policies everywhere. But this might change in the future what with <?access-control?> and all.
The CSS code uses the principal of the thing the URI came from.  So if page A loads sheet B which imports sheet C, the security and content policy checks for C are done using the principal of B.
I'd be fine with doing that for XSLT too.

Unfortunately we're dealing with strings in much of the code in order to support standalone transformiix too. These days standalone transformiix does use XPCOM, but since nsIPrincipal lives in caps it probably can't be used. In any case standalone transformiix is sort of broken anyway and it's future unknown.

Axel, Peter, you got any opinion on if it's ok to use nsIPrincipal in txStylesheetCompiler? We could just keep it as null for a (at this point theoretical) standalone build.
Created attachment 273418 [details] [diff] [review]
Proposed fix

I did the obvious thing for XSLT for now; we can do cleanup in a followup bug, I would hope.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #273418 - Flags: superreview?(jonas)
Attachment #273418 - Flags: review?(jonas)
Summary: Content policy checks for XBL loads show up in profiles → [FIX]Content policy checks for XBL loads show up in profiles
I am coming a little late, but - yes, I don't know of any content policies that want to prevent chrome from loading something. So making chrome an exception from content policies sounds like a good idea to me.
Wladimir, what do you think of API changes in, say, the mozilla 2 timeframe to pass the principal or equivalent in instead of the origin URI?
This would be very useful information. Currently I use hacks in Adblock Plus to determine the origin of a request (chrome / which website / special treatment for javascript and data URLs). A principal would make this much easier.
Flags: blocking1.9? → blocking1.9+
Is the docshell the only place where we don't have a principal? If so, could we call GetCodebasePrincipal/GetSystemPrincipal there and remove the aOriginURI argument? And file a bug on fixing the LoadInternal signature to take a principal.
Comment on attachment 273418 [details] [diff] [review]
Proposed fix

r/sr=me with or without that.
Attachment #273418 - Flags: superreview?(jonas)
Attachment #273418 - Flags: superreview+
Attachment #273418 - Flags: review?(jonas)
Attachment #273418 - Flags: review+
> Is the docshell the only place where we don't have a principal?

Yes.

> If so, could we call GetCodebasePrincipal/GetSystemPrincipal there and remove
> the aOriginURI argument?

Hmm... Yes.  But then all the loads that don't have an origin URI right now would not get content policy checks, which I think is wrong.

> And file a bug on fixing the LoadInternal signature to take a principal.

Probably a good idea for moz2.  Until then, we have (effectively) frozen interfaces that call into that function that should really be taking an nsIPrincipal but aren't (nsIWebNavigation, I'm looking at you).
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Summary: [FIX]Content policy checks for XBL loads show up in profiles → [FIXr]Content policy checks for XBL loads show up in profiles
Filed bug 391298 on the XSLT stuff.
> Hmm... Yes.  But then all the loads that don't have an origin URI right now
> would not get content policy checks, which I think is wrong.

Why not? Couldn't you just pass null for originPrincipal which would then get turned into a null for requestOrigin?
Oh, hmm.  Yeah, that would work.  File a bug, and I'll see what I can do?
Filed bug 391438
Depends on: 421228
Depends on: 770684
Depends on: 771942
You need to log in before you can comment on or make changes to this bug.