Bug 14566 – Changing document.domain does not raise javascript exception

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Sridhar Gurivireddy]

http://sgurivireddy.googlepages.com/document_domain.html

changing document.domain to invalid location does not raise javascript exception.

[Sam Weinig]

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)

[Alexey Proskuryakov]

FWIW, HTML5 currently specifies that a security exception must be raised when trying to set document.domain to a prohibited value.

[Yuan Song]

Created attachment 38612 [details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value.

[Yuan Song]

Comment on attachment 38612 [details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value.

It passes the test case at http://sgurivireddy.googlepages.com/document_domain.html, provided by the original bug reporter.

[Sam Weinig]

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-

[Yuan Song]

(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.

[Yuan Song]

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).

[Eric Seidel (no email)]

Comment on 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).

Tests like this area always better as js-only tests:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

[Yuan Song]

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).

[Darin Adler]

The patch looks good, but I don't see an answer to the "Have you tested IE?" question.

[Yuan Song]

(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.

[Eric Seidel (no email)]

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+

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

No clue how, but it would appear this change breaks basic-textareas.html.

[Eric Seidel (no email)]

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-

[Yuan Song]

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.

[Darin Adler]

(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.

[Yuan Song]

(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?

[Yuan Song]

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.

[Darin Adler]

(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.

[Darin Adler]

(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.

[Eric Seidel (no email)]

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.

[Darin Adler]

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?

[Yuan Song]

(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.

[Darin Adler]

(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.

[Eric Seidel (no email)]

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?

[Adam Barth]

(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.

[Eric Seidel (no email)]

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/

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

This change added a DOM-related layout test to fast/js.  That’s wrong.