Reporter, have you tested this in IE? I believe what you are seeing here is the difference in behavior between Firefox and IE. IE should let you set document.domain to any value, but Firefox only lets you set it to the superdomain (ie. bugs.webkit.org to webkit.org)
Comment on attachment 38612[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value.
This needs a test case. Also, have you tested IE, not that it would be a blocker, it is just good to have the facts.
r-
(In reply to comment #5)
Would I better modify an existing test case (e.g. http/tests/security/xss-DENIED-invalid-domain-change.html, there is a catch exception block) or add a new one? By the way, I've tested IE8 and it also throws an exception.
Created attachment 39002[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (test case added).
Created attachment 39180[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (test case changed to js-only test).
(In reply to comment #10)
> The patch looks good, but I don't see an answer to the "Have you tested IE?"
> question.
I answered it briefly in Comment #6. However, I've tested IE7 and IE8. Both of them only allow to set document.domain to either the same domain as that of the page (i.e. bugs.webkit.org to bugs.webkit.org) or the super domain (i.e. bugs.webkit.org to webkit.org). Otherwise, an "Invalid argument" exception is thrown. Furthermore, in case of IE8, the exception "This method can't be used in this context." is thrown if I try to set document.domain to any value in a local html file.
Comment on attachment 39180[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (test case changed to js-only test).
Yuan is not a committer, setting cq+
Comment on attachment 39180[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (test case changed to js-only test).
Rejecting patch 39180 from commit-queue.
Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11277 test cases.
fast/forms/basic-textareas.html -> failed
Exiting early after 1 failures. 6211 tests run.
117.50s total testing time
6210 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
2 test cases (<1%) had stderr output
Comment on attachment 39180[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (test case changed to js-only test).
We seem to have a lot of flakey tests.
fast/forms/basic-textareas.html -> failed
seems unrelated. Lets try again.
Comment on attachment 39180[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (test case changed to js-only test).
Rejecting patch 39180 from commit-queue.
Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11277 test cases.
fast/forms/basic-textareas.html -> failed
Exiting early after 1 failures. 6211 tests run.
117.54s total testing time
6210 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
2 test cases (<1%) had stderr output
Comment on attachment 39180[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (test case changed to js-only test).
+CONSOLE MESSAGE: line 86: Error: SECURITY_ERR: DOM Exception 18
It would appear that basic-textareas is doing something wrong.
This patch needs revision. r-
Created attachment 39843[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value ("document.domain" is not set to "mydummydomain" any more in fast/forms/basic-textareas.html)
In this new patch, "document.domain" is not set to "mydummydomain" any more in fast/forms/basic-textareas.html. It looks to me that it's faulty and not necessary. Because "document.domain" is set to "layouttests" if a local html file is launched by DumpRenderTree, so the access to the iframe is allowed anyway. Running this modified basic-textarea.html in DumpRenderTree gives exactly the same output as the original one. The test is conducted in WebKit gtk port in Ubuntu. I'm now trying to borrow a Mac to verify it.
(In reply to comment #18)
> In this new patch, "document.domain" is not set to "mydummydomain" any more in
> fast/forms/basic-textareas.html. It looks to me that it's faulty and not
> necessary. Because "document.domain" is set to "layouttests" if a local html
> file is launched by DumpRenderTree, so the access to the iframe is allowed
> anyway.
We’d like these tests to work inside the web browser as well as in DumpRenderTree when possible.
(In reply to comment #19)
> We’d like these tests to work inside the web browser as well as in
> DumpRenderTree when possible.
I agree. However, in this test case "fast/forms/basic-textarea.html", the "CSS1Compat" part never works in the browser before. Because the "standardsIframe" has a data URL, which allows no cross-frame access. So it makes the "CSS1Compat" part of the test fail in the browser.
While the DumpRenderTree allows the frames that have file URL to gain universal access to any other frames. Then it makes this part of the text runnable.
So setting "document.domain" here is not only unnecessary, but also faulty. Removing the code of setting "document.domain" won't change the original behavior of the test case in any aspect.
Making this test case runnable in a browser is a separate issue. My proposal is to make "standardsIframe" load an empty HTML page with DOCTYPE defined instead of using data URL. I already tried and it worked. But shall I include this in my patch or a new patch instead?
Created attachment 39994[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (make fast/forms/basic-textareas.html runnable in a browser)
Please refer to Comment #20 for the reason behind this updated patch. I figured that I might as well submit this updated patch to solve the problem that "fast/forms/basic-textareas.html" is not runnable in a browser, because submitting a separate patch to simply fix test case does not make sense.
(In reply to comment #20)
> So setting "document.domain" here is not only unnecessary, but also faulty.
> Removing the code of setting "document.domain" won't change the original
> behavior of the test case in any aspect.
OK.
> My proposal is
> to make "standardsIframe" load an empty HTML page with DOCTYPE defined instead
> of using data URL. I already tried and it worked. But shall I include this in
> my patch or a new patch instead?
Seems fine to do that separately.
(In reply to comment #21)
> Please refer to Comment #20 for the reason behind this updated patch. I figured
> that I might as well submit this updated patch to solve the problem that
> "fast/forms/basic-textareas.html" is not runnable in a browser, because
> submitting a separate patch to simply fix test case does not make sense.
OK.
From Darin's comments it looks like he's OK with your updated patch. But I'd prefer he set the r+ himself. If you want this committed by the commit-bot you'll need to set commit-queue=? as well.
Comment on attachment 39994[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (make fast/forms/basic-textareas.html runnable in a browser)
This patch raises SECURITY_ERR if you set domain to the same value it already has. Is that the behavior we want?
(In reply to comment #25)
I can't see that it does. The first if statement of Document::setDomain returns silently (no exception thrown) if the new and old domain are the same, as expected. This check was already there.
I verified manually that re-setting the same domain doesn't throw any exception in my build of WebKit. I also verified that IE, Firefox, and Opera behave the same way.
(In reply to comment #26)
> I can't see that it does.
OK. My mistake; the code to handle that case was outside the patch context, I guess.
If there was a test case covering this, then I would feel even more confident. I’ll review now.
I agree with Darin that tests are the best way to get rid of any ambiguity. Do we have a test which sets the domain to itself and verifies that it doesn't do anything crazy?
(In reply to comment #28)
> I agree with Darin that tests are the best way to get rid of any ambiguity. Do
> we have a test which sets the domain to itself and verifies that it doesn't do
> anything crazy?
We have tons of tests like that. That's how we test most of the document.domain security quirks.
Comment on attachment 39994[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (make fast/forms/basic-textareas.html runnable in a browser)
Sounds good to me then. :) Another patch is landing now, so assuming the bots stay green, you patch should be landed in 20 minutes or so: http://webkit-commit-queue.appspot.com/
Comment on attachment 39994[details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (make fast/forms/basic-textareas.html runnable in a browser)
Clearing flags on attachment: 39994
Committed r48761: <http://trac.webkit.org/changeset/48761>
2009-08-26 06:56 PDT, Yuan Song
2009-09-03 12:39 PDT, Yuan Song
2009-09-08 05:34 PDT, Yuan Song
commit-queue: commit-queue-
2009-09-21 01:58 PDT, Yuan Song
2009-09-23 06:54 PDT, Yuan Song