When JavaScript functions are cloned as part of brutal sharing, the principals may be incorrect--they should be retrieved from the scope chain of the function object rather than from the principals copied during the function clone.

Accept bug; needed for skins (right?), set TFV.

Comments about the changes: All pretty straightforward except for some refactoring that was required in nsScriptSecurityManager to handle the alternate means of retrieving principals. Also, I noticed that fp->argv[-2] can be nonnull even when fp->fun is null and made a change to account for that in JS_GetFrameFunctionObject. Is this right?

Hyatt, how soon do you need this? /be

Comments, mostly nits: - use NS_STATIC_CAST to go from jsp to nsJSPrin in GetScriptPrincipal. - for loop in IsCapabilityEnabled should be do {...} while ((fp = ...) != NULL) since fp must be non-null the first time through. - for loop in GetPrincipalAndFrame could be while ((fp = ...) != NULL) {...}. - I forget why fp->argv would be non-null, but fp->fun would be null -- did you figure out what was being called or executed in that case? I.e., what was fp->argv[-2], if not the callee object? What were fp->script->filename and lineno? Thanks. /be

So in the skins meeting today, we decided that XBL installed through the skin installer should not be allowed to have scripts on it at all. I'm going to be adding an API to the chrome registry that will tell you, for a given chrome URL, whether or not it should be allowed to access scripts. I'll put checks at the XBL level that will disallow the kinds of nodes that can contain scripts, thus barring XBL from being unsafe inside skins. In all other cases, I think the principal should be cloned just as we're doing here.

Brendan, take your time. This blocks included scripts via a <script> tag from working and blocks me from doing XBL brutal sharing. I have other things I can work on in the meantime, and I could probably do this work as "performance work" following beta2.

I was really asking about timing just to see whether norris's patch needed to go in M15, but it sounds like it'll go in next week when the trunk opens for M16. /be

Mass-adding beta2 keyword to all skins bugs.

- use NS_STATIC_CAST to go from jsp to nsJSPrin in GetScriptPrincipal. * done. - for loop in IsCapabilityEnabled should be do {...} while ((fp = ...) != NULL) since fp must be non-null the first time through. * I changed it to a for loop so that I could avoid restating "fp = JS_FrameIterator(cx, &fp)" for the end of the loop statements and for the continue. I was willing to pay the redundant conditional test for clarity. - for loop in GetPrincipalAndFrame could be while ((fp = ...) != NULL) {...}. I also liked the clarity of the explicit "next case" in the for construct. - I forget why fp->argv would be non-null, but fp->fun would be null -- did you figure out what was being called or executed in that case? I.e., what was fp->argv[-2], if not the callee object? What were fp->script->filename and lineno? Thanks. * It's a native function, so fp->script is null. The native function is WrappedNative_Call, so fp->argv[-2] is a XPCWrappedNativeWithCall. * I also discovered a security hole when looking through the code again. In CanExecuteFunction, if there is no script currently executing we cannot just blindly grant access. In this case the point is not who's asking, but what code will be executed. I talked with jband about what to do for a JSContext in this case and he pointed me to the "safe" JSContext used by XPConnect.

[my paragraphs below start with tab. /be] - for loop in IsCapabilityEnabled should be do {...} while ((fp = ...) != NULL) since fp must be non-null the first time through. * I changed it to a for loop so that I could avoid restating "fp = JS_FrameIterator(cx, &fp)" for the end of the loop statements and for the continue. I was willing to pay the redundant conditional test for clarity. This is not a big deal, but I don't see why the call needs to be restated if it's nested in the do-while condition. What am I missing? - for loop in GetPrincipalAndFrame could be while ((fp = ...) != NULL) {...}. I also liked the clarity of the explicit "next case" in the for construct. Again not a big deal, but both readers and (non-optimizing) compilers will have to deal with the lengthier and more redundant form. The 'while ((c = getchar()) != EOF) ...' loop goes all the way back to the K&R C book, first edition. It wouldn't have done for K&R to write 'for (c = getchar(); c != EOF; c = getchar()) ...', IMHO ;-) * It's a native function, so fp->script is null. The native function is WrappedNative_Call, so fp->argv[-2] is a XPCWrappedNativeWithCall. Oh, of course -- thanks, duh. I was worried about this case requiring similar checks in other code I was hacking, but now I see that it's separate. See the uses of fp->argv[-2] in jsfun.c. * I also discovered a security hole when looking through the code again. In CanExecuteFunction, if there is no script currently executing we cannot just blindly grant access. In this case the point is not who's asking, but what code will be executed. Hope I'm not confused: what about an event handler being called from C++? Won't it be called with no script currently executing, yet still not want the safe context? *I talked with jband about what to do for a JSContext in this case and he pointed me to the "safe" JSContext used by XPConnect. /be

Sorry, I read "do {...} while ((fp = ...) != NULL)" as "do {...} while (fp != NULL)". Yours is a fine change, I'll do it in both places. Whether an event handler is called from C++ or JS makes no difference to this method: it's trying to determine whether to run the handler based on the origin of the code for the handler. So if the pref is set to disable JS in mail, an event handler shouldn't run if it was delivered as part of a mail message, even if the code invoking the event handler is C++.

Fix checked in.

Verified per norris' comments.

verified