Closed Bug 375457 Opened 14 years ago Closed 13 years ago

toDataURL has incorrect type-checking logic

Categories

(Core :: Canvas: 2D, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: philip, Assigned: joe)

Details

(Keywords: testcase)

Attachments

(2 files)

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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Simple fix, but doesn't "strictly" have a patch.
Priority: -- → P2
Attached patch simple patchSplinter Review
Assignee: nobody → joe
Status: NEW → ASSIGNED
Attachment #307376 - Flags: review?(vladimir)
Priority: P2 → P1
Need approval before this can land.
Keywords: checkin-needed
Comment on attachment 307376 [details] [diff] [review]
simple patch

Please re-add the checkin-needed keyword once you have approval.
Attachment #307376 - Flags: approval1.9?
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Flags: in-testsuite?
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
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
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.
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".
You need to log in before you can comment on or make changes to this bug.