Closed Bug 375457 Opened 14 years ago Closed 13 years ago
Data URL has incorrect type-checking logic
User-Agent: Opera/9.10 (Windows NT 5.0; U; en) Build Identifier: nsHTMLCanvasElement.cpp line 330 says: if (!JSVAL_IS_STRING(argv) && !JSVAL_IS_STRING(argv)) 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
Assignee: nobody → joe
Status: NEW → ASSIGNED
Attachment #307376 - Flags: review?(vladimir)
Attachment #307376 - Flags: review?(vladimir) → review+
Need approval before this can land.
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?
Attachment #307376 - Flags: approval1.9? → approval1.9+
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
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+
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.