219848 - Cannot programmatically (JavaScript) set value of HTMLInputElement

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Michael O.]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030918 Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030918 Firebird/0.6.1+

Scripts with UniversalBrowserRead/UniversalBrowserWrite/UniversalBrowserAccess
privileges do not have access to all appropriate DOM elements and methods. I
noticed this bug while working on a sidebar page which accesses a form in the
content page. The script was able to change the form method, radio button
values, checkbox values, and submit button text -- but textbox access was
glitchy and remotely calling form.submit() did not send the appropriate text
input value.

This issue does not only affect form elements.

Reproducible: Always

Steps to Reproduce:
1. Open the attached testcase and follow the test sequence.
2. When the script asks for cross-site scripting privileges, grant them.

Actual Results:  
1) Setting the textbox value does not replace the existing value, it inserts the
new value at the caret position.
2) Setting the textbox value to an empty string does not clear the textbox.
3) Retrieving the textbox value always returns an empty string.
4) Submitting the form via the sidebar does not send the current textbox value.

Expected Results:  
1) Setting the textbox value should replace the existing value.
2) Setting the textbox value to an empty string should clear the textbox.
3) Retrieving the textbox value should return the current value.
4) Submitting the form via the sidebar should send the current textbox value.

Comment 1

[Michael O.]

Attachment: sidebar testcase

Comment 2

[Michael O.]

It appears that the testcase must be conducted with a local file, otherwise it
throws an error when requesting privileges. New steps to reproduce:

1. Download the attached testcase and open it in the browser.
2. Follow the test sequence.
2. When the script asks for "UniversalBrowserRead UniversalBrowserWrite
UniversalBrowserAccess" privileges, grant them.

Comment 3

[Boris Zbarsky [:bzbarsky]]

caillon? jkeiser? jst?  Any ideas what's up here?

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Um, what is the error?  Also, the open in sidebar thing does not work for me, it
opens the page up in a new window, and the rest of the testcase seems to rely on
the sidebar.

Comment 5

[Michael O.]

The testcase is for Mozilla Firebird nightlies with sidebar support -- it uses
the "_search" target to open the page in the sidebar.

I should mention that these DOM access errors only occur with cross-site
scripting. When both pages (sidebar and content) are in the same domain, the
errors do not occur. That's my reason for the summary on this bug.

Comment 6

[Fabian Guisset]

*** Bug 219634 has been marked as a duplicate of this bug. ***

Comment 7

[Alan Donovan]

This bug report should be retitled:

  "Cannot programmatically (JavaScript) set value of HTMLInputElement"

since this is a more accurate description (the bug does not appear to be a
security issue).  However, I am not sufficiently privileged to make this change
myself.  

Bug report 219634 has been fused with this one.  Please see 219634 for further
details.

Comment 8

[Michael O.]

Changing summary and OS.

I'm continuing to suggest this is a security privileges bug because the errors
only exist with cross-site scripting.

Comment 9

[Michael O.]

Update regarding the testcase:

Using Mozilla Firebird nightly 2003-10-27, the testcase does not throw an error
when requesting cross-site scripting permissions. The testcase can be used
straight from the Bugzilla server and continues to exhibit the behavior
described in the original bug description.

Comment 10

[Boris Zbarsky [:bzbarsky]]

When I load the testcase attachment and click on the links all I get is a
separate window with the same attachment and then google loading when I click on
that link (current Mozilla build).  It seems that the testcase is using a
Firebird-specific window target or something along those lines...  

Comment 11

[Michael O.]

Frames-based testcase for Mozilla (Seamonkey):

http://www.cosmicat.com/x/test/20031027mozcss/

You'll need to add the following user preferences to enable the cross-site
scripting privileges:

user_pref("capability.policy.default.HTMLDocument.forms", "allAccess");
user_pref("capability.policy.default.HTMLFormElement.submit", "allAccess");
user_pref("capability.policy.default.HTMLInputElement.click", "allAccess");
user_pref("capability.policy.default.HTMLInputElement.getAttribute", "allAccess");
user_pref("capability.policy.default.HTMLInputElement.setAttribute", "allAccess");
user_pref("capability.policy.default.HTMLInputElement.value", "allAccess");

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, when I load that testcase in a debug build and click on those links, I get
an error return in the guts of classinfo... In particular, the call to
::JS_DefineObject on line 3753 (in nsWindowSH::GlobalResolve) returns null, so
we bail out of there and consequently bail out of nsWindowSH::NewResolve at line
4084.

Now what's odd is that just accessing the document.forms in the other frame
causes this... (that is, javascript:alert(window.google.document.forms) in the
URL bar on the test page leads to this NS_ENSURE_SUCCESS failing).

This returns an error out to XPC_WN_Helper_NewResolve (which calls
XPCThrower::Throw, by the way -- not sure why that exception does not make it
out to script), which returns JS_TRUE up the stack... so at this point the
failure seems to be "lost" so to say.

Not sure whether this has anything to do with this bug (which I can reproduce,
needless to say), but it's something to go on...

In any case, confirming bug.  Something is wrong here.  jst?  caillon?  brendan?
 peterv?  Any of you have any ideas?

Comment 13

[Peter Van der Beken [:peterv]]

This is probably mine.

Comment 14

[Brendan Eich [:brendan]]

Don't return true to the JS engine if you've JS_SetPendingException....

/be

Comment 15

[Peter Van der Beken [:peterv]]

That's easy to fix, probably by just making the |Throw(...)| into a |return
Throw(...)| (at
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappednativejsops.cpp#966).
Now to figure out why the JS_DefineObject fails for HTMLCollection. :-/

Comment 16

[Brendan Eich [:brendan]]

peterv: or better, |retval = Throw(...)| (don't add a return in the else clause
when the next line after the if-else is the final return retval; statement in
the method).

/be

Comment 17

[Peter Van der Beken [:peterv]]

Yeah, that's what I did first. On the other hand, *every* other call to Throw in
that file is |return Throw()| so I might as well turn the if around and return
early.

Comment 18

[Peter Van der Beken [:peterv]]

I'll attach a patch to fix the problems in the DOM, however it doesn't fix the
issues with the input. We do end up in SetValue, the value is stored in the
frame so we get here:
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLInputElement.cpp#773
and we end up in
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsTextControlFrame.cpp#3009
. I wonder if the SelectAll in
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsTextControlFrame.cpp#2992
is somehow failing.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Does the SelectAll return an error nsresult?

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: Fix for nsDOMClassInfo v1

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: Fix for nsDOMClassInfo v1 (diff -w for review only)

Comment 22

[Peter Van der Beken [:peterv]]

Comment on attachment 134580 [details] [diff] [review]
Fix for nsDOMClassInfo v1 (diff -w for review only)

We don't want to do security checks on defining DOM classes. Access to the DOM
classes (eg. aWindow.DOMClass) is still blocked by nsWindowSH::GetProperty.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 134580 [details] [diff] [review]
Fix for nsDOMClassInfo v1 (diff -w for review only)

sr=jst

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, I think I see the problem... I bet the bug is that the various editor
manipulations use selections and ranges... And ranges have these security checks..

In particular, I'm looking at the output of nsTextControlFrame::GetValue and
it's always empty.  The reason that happens is that down in
nsDocumentEncoder::EncodeToString when we call SerializeRangeToString the range
we are calling it on has:

  mStartOffset = 0, mEndOffset = 0, 
  mStartParent = {mRawPtr = 0x0 }, 
  mEndParent = { mRawPtr = 0x0 }

Naturally editor does not check the retval from calling SetStart() and
SetEnd()... and the form control code is not checking the retval from
OutputToString().  Nor is anyone checking a whole slew of other retvals.  Maybe
they should should be...

Anyway, I bet SelectAll has the same issues as EncodeToString

Comment 25

[Peter Van der Beken [:peterv]]

Checked in attachment 134579 [details] [diff] [review].

Comment 26

[Boris Zbarsky [:bzbarsky]]

*** Bug 235868 has been marked as a duplicate of this bug. ***

Comment 27

[Paulo Matos]

Since this has already a proposed fix, any ideia when this might be corrected?
Just wondering...

Comment 28

[timeless]

i don't see a proposed solution, may i assign the bug to you for you to fix?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Paulo, the patch in this bug has already been checked in.  It did not completely
fix the bug.  The right fix would make the checks mentioned in comment 24 take
the caller's priveleges into account properly.  Right now they do not.

Comment 30

[Boris Zbarsky [:bzbarsky]]

So to fix this, I think that we should change nsContentUtils::CanCallerAccess to
call CheckSameOriginPrincipal() followed by a more in-depth check that will take
into account Universal* stuff and all that if the principals have different
origins....

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: CanCallerAccess fix, v1

peterv already added the CheckSameOriginPrincipal in another patch. 

The lack of the UniversalBrowserRead check means that the XUL <editor> tag does
not work in non-chrome XUL.

So this just adds a check for UniversalBrowserRead, as suggested by jst.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 149766 [details] [diff] [review]
CanCallerAccess fix, v1

Actually, it's probably only some cases of non-chrome XUL that have this
problem (eg when you're trying to make the content of a remote page editable).

Comment 33

[Peter Van der Beken [:peterv]]

Please also add this to URIUtils::CanCallerAccess (in
mozilla/extensions/transformiix/source/base/txURIUtils.cpp).

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 149766 [details] [diff] [review]
CanCallerAccess fix, v1

+  rv = sSecurityManager->IsCapabilityEnabled("UniversalBrowserRead",
+					      &enabled);
+  NS_ENSURE_SUCCESS(rv, PR_FALSE);
+  if (enabled) {
+    return PR_TRUE;
+  }
+
+  return PR_FALSE;
 }

No need to check enabled here, just return enabled after the
NS_ENSURE_SUCCESS() check.

r+sr=jst

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

peterv: that method already has this check, near the top of the method.

jst: oh, right.  thanks!

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: CanCallerAccess fix, v2

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 149769 [details] [diff] [review]
CanCallerAccess fix, v2

Carrying forward review flags.

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 149769 [details] [diff] [review]
CanCallerAccess fix, v2

Folks basing products on the 1.7 branch, including the people I'm working with,
may want this.

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

CanCallerAccess fix checked into the trunk.

Comment 40

[Asa Dotzler [:asa]]

Comment on attachment 149769 [details] [diff] [review]
CanCallerAccess fix, v2

a=asa (on behalf of drivers) for checkin to Mozilla 1.7. We're wrapping things
up and so this will need to land today if it's going to make the release.

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: test case for CanCallerAccess fix

Oddly enough, the 1.7 branch works on this testcase without my patch (the trunk
required the patch).

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

After discussion with jst, checked into the 1.7 branch anyway, since it's the
right fix and is likely to fix other edge cases as well.  peterv: mark this bug
resolved if you wish... (though it's not checked in on the aviary branch.

Comment 43

[Benjamin Smedberg]

Committed on the aviary branch. Did this ever land on the trunk?

Comment 44

[Dan Mosedale (:dmosedale, :dmose)]

Yes, it landed on the trunk.

Comment 45

[Boris Zbarsky [:bzbarsky]]

I'm going to go out on a limb and mark this fixed. 

Comment 46

[Michael O.]

Confirming fixed. I've been using a cross-site scripted sidebar in Firefox 0.9.2
at work for the past three weeks with no problems. Form input values are
modified correctly and those modified values are sent correctly upon form
submission.

Big thanks to everyone involved in getting this fixed. Cheers!

Comment 47

[Boris Zbarsky [:bzbarsky]]

*** Bug 275194 has been marked as a duplicate of this bug. ***

Comment 48

[Boris Zbarsky [:bzbarsky]]

*** Bug 275194 has been marked as a duplicate of this bug. ***