[rfc][icedtea-web] Fix for PR1198, JSObject passed incorrectly to Javascript

[Jiri Vanek]

On 01/04/2013 06:19 PM, Adam Domurad wrote:
> On 01/04/2013 04:43 AM, Jiri Vanek wrote:
>> On 12/03/2012 08:03 PM, Adam Domurad wrote:
>>> On 11/30/2012 04:08 PM, Adam Domurad wrote:
>>>> Hi all. Attached is a fix for PR1198.
>>>>
>>>> I still plan to write unit tests for at least the new method
>>>> PluginAppletViewer.toObjectIDString, but I wanted to get this out before leaving for the weekend.
>>>>
>>>> One tricky thing with this patch was that it had to consolidate _a lot_ of duplicated
>>>> functionality (actually I found some subtle differences in handling, this should be more
>>>> consistent). Once that was done the actual patch was fairly straight forward.
>>>>
>>>> The basic issue was that JSObject was being passed as if it were a normal Java object, when the
>>>> liveconnect spec specifies that it should be returned as the underlying javascript object.
>>>>
>>>> A method was added to JSObject to get the underlying reference. This was kept package-private to
>>>> not pollute its public interface. As well, a new permission was added for accessing this method.
>>>> To access this outside of the package, a utility was created in JSUtil.
>>>>
>>>> This patch adds a special case to Java->JS communication when sending objects, to indicate that
>>>> a Javascript object is being passed.
>>>>
>>>> With patch applied, JSObjectFromEval should pass in all browsers.
>>>>
>>>> 2012-11-30  Adam Domurad <adomurad at redhat.com>
>>>>
>>>>     Fix PR1198: JSObject passed incorrectly to Javascript
>>>>     * plugin/icedteanp/IcedTeaJavaRequestProcessor.cc: Pass extra data for
>>>>     'jsobject' object result messages.
>>>>     * plugin/icedteanp/IcedTeaPluginRequestProcessor.cc: Same.
>>>>     * plugin/icedteanp/IcedTeaPluginUtils.cc: Add special casing of
>>>>     javascript references passed from java.
>>>>     * plugin/icedteanp/java/netscape/javascript/JSObjectUnboxPermission.java:
>>>>     New permission for unboxing a JSObject's internal reference.
>>>>     * plugin/icedteanp/java/netscape/javascript/JSObject.java
>>>>     (getInternalReference): New, package-private, retrieves internal
>>>>     reference (Must have proper permission).
>>>>     * plugin/icedteanp/java/netscape/javascript/JSUtil.java
>>>>     (getJSObjectInternalReference) New, utility for accessing
>>>>     JSObject#getInternalReference from outside the package.
>>>>     * plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
>>>>     (toObjectIDString): New, creates a string that precisely identifies a
>>>>     Java object.
>>>>     (handleMessage): Replace a lot of duplicated functionality with
>>>>     'toObjectIDString'.
>>>>     * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java: Replace
>>>>     duplicated functionality with 'toObjectIDString'.
>>>>     * tests/reproducers/simple/JSObjectFromEval/srcs/JSObjectFromEval.java:
>>>>     Don't print out type passed (differs from browser to browser).
>>>>     * tests/reproducers/simple/JSObjectFromEval/testcases/JSObjectFromEvalTest.java:
>>>>     Don't check type passed (differs from browser to browser). Remove
>>>>     known-to-fail. Reformat.
>>>
>>> As promised attached is the unit-test for the newly introduced Java->JS function (which
>>> encapsulates logic that was duplicated in many areas)
>>>
>>> Note that this patch contains a Makefile patch for unit testing sun.applet package, however this
>>> will be committed with another now-approved patch so I have left it out of the ChangeLog.
>>>
>>> ChangeLog:
>>> 2012-12-XX  Adam Domurad <adomurad at redhat.com>
>>>
>>>      Unit test for PluginAppletSecurityContext#toObjectIDString. Make
>>>      PluginAppletSecurityContext more unit-testable.
>>>      * plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
>>>      Don't initialize security manager in constructor. Fix a few Java->JS
>>>      corner cases.
>>>      * plugin/icedteanp/java/sun/applet/PluginMain.java: Initialize
>>>      SecurityManager before creating PluginAppletSecurityContext.
>>>      * tests/netx/unit/sun/applet/PluginAppletSecurityContextTest.java:
>>>      Unit test for all the corner cases of converting a Java object to a
>>>      string that can be precisely identified.
>>>
>>> Happy hacking,
>>> -Adam
>>>
>>>
>>
>> Tests first. A lot of questions....
>>
>>> jsfix-unittest.patch
>>>
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> --- a/Makefile.am
...
>>> -
>>> -        JNLPRuntime.disableExit();
>>> -
>>
>> Can't the moving of this initiliazaton cause some hole?
>
> This code is invoked exactly once, and I figured this type of thing shouldn't be in a constructor.
> It was causing problems for my unit tests because of the security manager it installed. A different
> solution is welcome.
> The problem is I need access to PluginAppletSecurityContext #toObjectIDString. It requires a
> security context instance. IMO this is an indicator that this class 'does too much' (a class called
> SecurityContext handling all the liveconnect logic??).
> If you feel I should do it, I can move liveconnect logic into a different class and then testing
> will be fine.

I understood why you did it, I wanted to ensure you know what you are doing :)
I completely agree that with this class doing to much. Some refactoring is possible as another 
changeset. But I'm for it (although not understanding all possible security impacts).

>> What corner cases it fixed? Can't they be fixed by better way?
>> Anyway - it should be in different changeset.
>>
>>>           URL u = null;
>>>           try {
>>>               u = new URL("file://");
>>> @@ -348,16 +337,16 @@ public class PluginAppletSecurityContext
>>>        */
>>>       public String toObjectIDString(Object obj, Class<?> type, boolean unboxPrimitives) {
>>>
>>> +        /* Void (can occur from declared return type), pass special "void" string: */
>>> +        if (type == Void.TYPE) {
>>> +            return "literalreturn void";
>>> +        }
>>> +
>>>           /* Null, pass special "null" string: */
>>>           if (obj == null) {
>>>               return "literalreturn null";
>>>           }
>>>
>>> -        /* Void (can occur from declared return type), pass special "void" string: */
>>> -        if (type == Void.TYPE) {
>>> -            return "literalreturn void";
>>> -        }
>>> -
>>>           /* Primitive, accurately represented by its toString() form: */
>>>           boolean returnAsString = ( type == Boolean.TYPE
>>>                                   || type == Byte.TYPE
>>> @@ -378,7 +367,7 @@ public class PluginAppletSecurityContext
>>>
>>>           /* Floating point number, we ensure we give enough precision: */
>>>           if ( type == Float.TYPE || type == Double.TYPE ||
>>> -                ( unboxPrimitives&&  (type == Double.class || type == Double.class) )) {
>>> +                ( unboxPrimitives&&  (type == Float.class || type == Double.class) )) {
>>
>> Why this? Also Shouldn't it be  in different changeset?
>
> I will move this into the fix itself, they were bugs discovered during unit testing :-)
>
> [.. rest snipped..]
Isn't missing updated  junit patch here?

Thanx for looking into it.
J.