375457 - toDataURL has incorrect type-checking logic

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P1)

Description

[Philip Taylor]

User-Agent:       Opera/9.10 (Windows NT 5.0; U; en)
Build Identifier: 

nsHTMLCanvasElement.cpp line 330 says:

  if (!JSVAL_IS_STRING(argv[0]) && !JSVAL_IS_STRING(argv[1]))
    return NS_ERROR_DOM_SYNTAX_ERR;

which looks wrong since it should be || instead of &&. Currently it will accept toDataURL(string, nonstring), when it shouldn't.

This is only in the 2-argument form of the function, which only works if the caller is trusted, so this shouldn't be a security problem for the normal web.

Reproducible: Didn't try

Comment 1

[Joe Drew (not getting mail)]

Simple fix, but doesn't "strictly" have a patch.

Comment 2

[Joe Drew (not getting mail)]

Attachment: simple patch

Comment 3

[Reed Loden [:reed]]

Need approval before this can land.

Comment 4

[Reed Loden [:reed]]

Comment on attachment 307376 [details] [diff] [review]
simple patch

Please re-add the checkin-needed keyword once you have approval.

Comment 5

[Reed Loden [:reed]]

Checking in content/html/content/src/nsHTMLCanvasElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLCanvasElement.cpp,v  <--  nsHTMLCanvasElement.cpp
new revision: 3.26; previous revision: 3.25
done

Comment 6

[Joe Drew (not getting mail)]

Attachment: a simple test that just ensures we throw an exception when calling toDataURL with null and objects

Comment 7

[Reed Loden [:reed]]

Checking in dom/tests/mochitest/bugs/Makefile.in;
/cvsroot/mozilla/dom/tests/mochitest/bugs/Makefile.in,v  <--  Makefile.in
new revision: 1.23; previous revision: 1.22
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug375457.html,v
done
Checking in dom/tests/mochitest/bugs/test_bug375457.html;
/cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug375457.html,v  <--  test_bug375457.html
initial revision: 1.1
done

Comment 8

[Johnny Stenback (:jst)]

Joe and Vlad, I'm working on some changes to how we handle optional arguments in IDL and one of the methods I was able to change from using JS specific argument handling code to simply using IDL method/parameter flags to deal with the optional arguments was canvas.toDataURL(). With that work in place I ran into the tests that were added as part of this bug. The signature of the method in question takes two string arguments, both of which are optional. Now, XPConnect will happily convert any argument you pass in as a string argument to a string, as it does, and should, for all other methods, and I'm now wondering if we don't actually want the same behavior here. It doesn't seem to make sense to treat these arguments differently here from everywhere else (well, almost at least) in the code.

Are there spec reasons behind what was done in the tests that were added here? If not, I would argue we simply remove this test, or make it test based on the string values rather than how the strings were generated and passed in, or something.

Comment 9

[Peter Van der Beken [:peterv]]

I'm going to remove the test as part of bug 459452 (which contains the patch that jst refers to in comment 8). The spec says "Other arguments must be ignored and must not cause the user agent to raise an exception".