Closed Bug 101928 Opened 23 years ago Closed 21 years ago

nsIScriptSecurityManager::CheckLoadURI in wrong place

Categories

(Core :: Security: CAPS, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 120373
Future

People

(Reporter: jonsmirl, Assigned: security-bugs)

References

Details

(Keywords: topembed-, Whiteboard: T2)

nsScriptSecurityManager::CheckLoadURI is probably located on the wrong object. 
Looking at the code for nsScriptSecurityManager::CheckLoadURI it appears to be 
pretty much a standalone function without dependencies on 
nsScriptSecurityManager. I couldn't see any obvious dependencies on scripting.

nsScriptSecurityManager is derived from nsXPCScriptSecurityManager which causes 
a dependency on XPConnect, XPConnect in turn depends on Javascript. 
nsScriptSecurityManager::CheckLoadURI should be moved to another object (URI 
object?) without XPConnect and javascript dependencies.

One example of this is that the XSLT support ends up needing Javascript 
installed which it never uses. Breaking connections like this is important for 
embedded systems which run with a minimal set of components.
Blocks: 100107
for those of us reading these bugs, the file in question is at:
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#821
actually, that function is a function that's exposed on
nsIScriptSecurityManager, which seems like the right place to me. The fact that
the implementation function doesn't depend on anything seems irrelevant. 

I'm kind of thinking it might make sense to break nsIScriptSecurityManager.idl
into nsIScriptSecurityJS and nsIScriptSecuritySimple, and maybe even
nsIScriptSecurityCaps or something - it seems like nsIScriptSecurityManager is a
huge interface that does many things..

mitch, how does this splitting sound to you?
Summary: nsScriptSecurityManager::CheckLoadURI in wrong place → nsIScriptSecurityManager::CheckLoadURI in wrong place
Why is it on Script security manager? It doesn't have anything to do with 
scripting.
true enough. I don't see a nsISecurityManager, I guess it could be split out
that way:
nsIScriptSecurityManager
nsISecurityManager
nsICapabilityManager

or something.
Why don't we move this (and the CheckLoadURIStr helper func) to nsIIOService?
Anything using these will be using necko anyway, I would guess, and caps (which
uses this internally) needs necko for the URI argument in
CheckLoadURIFromScript, so that dependancy won't be a problem.

The internal helper functions ReportErrorToConsole and GetBaseURIScheme would
also move, but those are internal only. Why are are the part of the class,
instead of static helper functions now, btw? Noone else uses them, AFAICS, and
they're not exposed by the interface.
1) it seems really wrong to put uri security policy into necko. For instance,
necko should not have specialized knowledge of per-protocol security policy.
This function has specific policy regarding mail and news messages.
2) The fact that this is in fact in the same interface as CheckLoadURIFromScript
and CheckLoadURIStr leads me to believe that the author intended these functions
to be related. I can forsee this function some day depending on other members of
this class...

I really don't think the answer is to arbitrarily shuffle around code just to
reduce dependencies. we need to look at this from an architectural perspective
to determine what function the code serves, what related code it should live
with, and what interface should expose it. IMO, all of this points to "caps" not
"necko"

And to answer your question about the helper functions, this is common practice
for a class to have private methods, even if they are not exposed through some
interface. One of them is in fact declared static, and the only difference
between declaring something as a static helper function and a static class
method (when the code is actually called) is that one has a symbol exposed to
the linker, and one does not. Ultimately when you compile it into a dll, there
is no difference. The other one? Well, it could be declared static too.
True. However, mitch, darin, and I briefly discussed moving this to
nsIProtocolHandler, so that protocol handlers could be pluggable - see how aim
has to be hacked into the mozilla builds. I don't think that anything came of
this discussion, though. (This was the day before darin went on vacation, and we
didn't pick it up after he got back.) If this was done, then the protocol would
have knowledge of its own security requirements.

I think that this code does belong to necko - its not just moving code arround
for fun. CheckLoadUriFromScript does some js-checking, and calls checkloaduri.
They're related, but I don't think that they're inter-related. Every other
function in caps is related to calling out of/in to JS, and related privilages.
Thats not an argument for moving it out of caps, of course.

I know the difference between a private class-static method and a file-static
one. I was just curious if there were plans for something else in caps to use
those routines at some point, esp ReportErrorToConsole.

It was just a suggestion though, and theres nothing wrong with caps as a place
for it.
I agree that we shouldn't start arbitrarily shuffling code around to reduce
dependencies. We can reopen the discussion about allowing protocols to define
their own security policies; I think this is a good idea, should we open another
bug for it?

As for splitting up nsScriptSecurityManager, this is something I've been
thionking of doing for a while now, basically to split the JS-specific parts off
from the language-neutral policy storage parts. I don't have strong opinions one
way or the other about CheckLaodURI; there are some advantages to moving it into
Necko somehow. Let's continue to talk about it.
Status: NEW → ASSIGNED
performance, footprint, feature work, and re-architecture bugs will be addressed
in 0.9.8
Target Milestone: --- → mozilla0.9.8
i just hit this because wgate has a protocol which i will probably end up
hacking into this file. I filed bug 120373 for my concerns/ideas.

Oh, btw, how accurate is the tm? we just branched for 0.9.8...

Future
Target Milestone: mozilla0.9.8 → Future
Keywords: topembed
Keywords: topembedtopembed-
Keeping topembed-
Whiteboard: T2

*** This bug has been marked as a duplicate of 120373 ***
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.