116888 - Java applets inserted as <OBJECT> run in spite of Java is turned off in Preferences.

Status: VERIFIED FIXED

(Core Graveyard :: Java: OJI, defect, P1)

Description

[Manko]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.7) Gecko/20011221
BuildID:    2001122106

When Java is disabled in Preferences, applets described as <OBJECT> tag are
starting.

Reproducible: Always
Steps to Reproduce:
1. Turn off Java in preferences.
2. Go to URL
http://www.student.oulu.fi/%7esairwas/object-test/java/test1.html

Actual Results:  Java applet is running.

Expected Results:  Java applet must not run and browser must display alternate
content instead.

Comment 1

[R.B.]

Also seen on Linux 2001122508.

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Hmm, that's not good. Not sure who owns this though. Let's try OJI.

Comment 3

[Manko]

Please, change status of this bug to "Accepted"

Comment 4

[Manko]

This bug also presents in build 2002011003

Comment 5

[Igor Nekrestyanov]

Attachment: Proposed patch

It seems we nether checked for java enabled/disabled for non-applet case.
Please, review proposed patch.

Comment 6

[Peter Lubczynski]

-->Igor

Comment 7

[Peter Lubczynski]

Comment on attachment 64507 [details] [diff] [review]
Proposed patch

r=peterl

Comment 8

[Patrick C. Beard]

What if some internal part of the browser wants to use Java, not on behalf 
of content? I think this is too low level a place to put this logic. We should 
instead check for <OBJECT> tag invocation of Java, and prevent it there.

Comment 9

[Igor Nekrestyanov]

Attachment: Updated patch

Ok, i made new version of patch that does check for Object/Applet/Embed tag.
It works for me but i do not know how to test it with any 
"internal component written in java".

This patch obsoletes 64507 (i have no permission to mark it obsolete myself :(
)

Anyway, please review.

Comment 10

[Peter Lubczynski]

What if....

+     rv = pti2->GetTagType(&tagType);
+ 
+     if(rv != NS_OK) {
+       return rv;
+     } 

fails for cases when we don't have tags like full-page plugins or "internal
components"?

Does/should Liveconnect work?

Comment 11

[Manko]

It's necessary don't forget that if Java is turned off, browser must display
inner content of <OBJECT> tag.

For example, if in HTML code described:

<object codetype="application/java" classid="java:mygame.class" width="250"
height="250">
You have turned off Java and can't enjoy my wonderful Java game! <img
src="/img/emotions/sad.png" />
</object>

browser must display text:
"You have turned off Java and can't enjoy my wonderful Java game!" and inline
image (sad.png)

Does your patch support this feature?

Comment 12

[Igor Nekrestyanov]

Attachment: Updated patch according to peter comments

Comment 13

[Igor Nekrestyanov]

peter: Good point. I just updated the patch to fix this.

manko: Yes, it shows alternative text if object is disabled.
       It is the same as in case no java plugin is installed.

Comment 14

[Peter Lubczynski]

Nominating mozilla0.9.8 as this probably is a slight security problem.

Comment 15

[Peter Lubczynski]

Comment on attachment 65600 [details] [diff] [review]
Updated patch according to peter comments

r=peterl

Comment 16

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.8

Comment 17

[Patrick C. Beard]

Wouldn't it be cleaner to put this logic up in
nsPluginHost::InstantiateEmbededPlugin()? There's already code in there that
checks to see if Java is enabled. It would be a shame to add yet another test of
the same thing...

Comment 18

[Igor Nekrestyanov]

Attachment: patch v4

Well. originally i was thinking about disabling java not only
for embeded objects but for full-page too. 
(what if cgi-script will output something with "java-vm" content-type?)

Anyway, with patrick's comment on potential internal java components 
i'd rather to consider this bug as "case of embeded java object" 
and in this conditions we can move check one level upper
(and this will in fact will not add new checks indeed it will remove some :)
Patch is attached.

However, i feel that "Disable java" label is quite misleading.
(Does this mean we want ot disable access from javascript embeded into 
page to jvm? What about full-page plugins?) and may be something like 
"disable applets" will be more correct. Shall we file new bug for this?

Comment 19

[Igor Nekrestyanov]

To simplify review i'd like to point that we can get to this place 
only if we haveapplet, embed or object tag type.

See
http://lxr.mozilla.org/mozilla/source/modules/plugin/base/src/nsPluginHostImpl.cpp#3205
for details

Comment 20

[Manko]

Igor Nekrestyanov wrote:
> may be something like "disable applets" will be more correct

Yes, i think this is very nice idea. And more, probably, behavior for each type
of plugins (Java, Flash, sound and video objects etc.) should be pointed in
Preferences separately.

Comment 21

[Peter Lubczynski]

Igor, won't you patch also disable other plugins when Java is disabled? 

How about something like this:
  if (tagType == nsPluginTagType_Applet ||
      (tagType == nsPluginTagType_Object && 
        PL_strcasecmp(aMimeType, "application/x-java")))

or just simply checking for "application/x-java". 

Comment 22

[Peter Lubczynski]

Oh, could you attach patches in -u format with enough context to see what's
going on. Thanks!

Comment 23

[Igor Nekrestyanov]

Attachment: Patch that checks for mimetype (diff -u)

peter: thanks for good point. I have added check for java mime-type.
       Would you please review this one and mark others as obsolete?
       (Despite the fact i have attached other patches i have no permission to 

	change them myself :( )

Comment 24

[Peter Lubczynski]

Igor,

It looks like this would be the third time this comparison is done against the
same string. Could this maybe just be done once at the top and then use a
boolean throughout the function?

Comment 25

[Igor Nekrestyanov]

Attachment: Patch that eliminate duplicate mimetype checks

ok. new version of patch is attached.

Comment 26

[Peter Lubczynski]

Comment on attachment 67089 [details] [diff] [review]
Patch that eliminate duplicate mimetype checks

r=peterl

Comment 27

[Patrick C. Beard]

Comment on attachment 67089 [details] [diff] [review]
Patch that eliminate duplicate mimetype checks

In the future please use unified diffs (diff -u). You can still use the test
for nsPluginTagType_Applet as a short circuit to identify Java. Use the string
compares as *ADDITIONAL* tests if the tag type is not nsPluginTagType_Applet.

Comment 28

[Igor Nekrestyanov]

Attachment: Patch adapted to latest comment from beard

Comment 29

[Peter Lubczynski]

Comment on attachment 67099 [details] [diff] [review]
Patch adapted to latest comment from beard

r=peterl, that looks better, Patrick?

Comment 30

[Igor Nekrestyanov]

Attachment: new version that checks for prefixes of mimetypes

Taking into account bug 120390 (see comment #15 there) i guess it will 
be smarter to require identity of prefixes instead of full strings 
when comparing mime-types. 

peter: thank you for pointing me to 120390

Comment 31

[Jerome Lacoste]

Igor I suggest that you put a comment on why you do a strncmp instead of a of
strcmp. 
E.g.

// use strncmp instead of strcmp to allow for application/x-java-applet;version=...
// cf bug 120390 comment #15

Nice team work!

Comment 32

[Patrick C. Beard]

Comment on attachment 67277 [details] [diff] [review]
new version that checks for prefixes of mimetypes

sr=beard
I assume this is to handle the fact that Sun further qualifies these MIME types
with version strings. Looks good.

Comment 33

[Peter Lubczynski]

Comment on attachment 67277 [details] [diff] [review]
new version that checks for prefixes of mimetypes

r=peterl

Igor, do you have CVS check-in access?

Comment 34

[Igor Nekrestyanov]

No, i still can not check this in myself.
Please do this for me.
Thank you in advance.

Comment 35

[Peter Lubczynski]

Checked into trunk and 0.9.8 branch, marking FIXED.

Thanks all!

Comment 36

[Patty Mac]

verified on all platforms (commercial trunk: 2002-02-06-06-trunk)