Closed
Bug 388597
Opened 17 years ago
Closed 17 years ago
[FIXr]Content policy checks for XBL loads show up in profiles
Categories
(Core :: XBL, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•17 years ago
|
||
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.
Assignee | ||
Comment 4•17 years ago
|
||
Hmm. That's a good point. Ok, lemme see what I can do here. ;)
Sounds good to me.
Assignee | ||
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 8•17 years ago
|
||
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.
Assignee | ||
Comment 10•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Summary: Content policy checks for XBL loads show up in profiles → [FIX]Content policy checks for XBL loads show up in profiles
Comment 11•17 years ago
|
||
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.
Assignee | ||
Comment 12•17 years ago
|
||
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?
Comment 13•17 years ago
|
||
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+
Assignee | ||
Comment 16•17 years ago
|
||
> 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).
Assignee | ||
Comment 17•17 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 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
Assignee | ||
Comment 18•17 years ago
|
||
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?
Assignee | ||
Comment 20•17 years ago
|
||
Oh, hmm. Yeah, that would work. File a bug, and I'll see what I can do?
Filed bug 391438
Updated•12 years ago
|
Depends on: CVE-2012-3975
You need to log in
before you can comment on or make changes to this bug.
Description
•