Bug 14566 - Changing document.domain does not raise javascript exception
Summary: Changing document.domain does not raise javascript exception
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://sgurivireddy.googlepages.com/d...
Keywords:
Depends on:
Blocks:
 
Reported: 2007-07-09 12:35 PDT by Sridhar Gurivireddy
Modified: 2009-10-08 19:35 PDT (History)
8 users (show)

See Also:


Attachments
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value. (2.98 KB, patch)
2009-08-26 06:56 PDT, Yuan Song
sam: review-
Details | Formatted Diff | Diff
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value (test case added). (5.65 KB, patch)
2009-09-03 12:39 PDT, Yuan Song
no flags Details | Formatted Diff | Diff
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). (5.95 KB, patch)
2009-09-08 05:34 PDT, Yuan Song
eric: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
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) (7.12 KB, patch)
2009-09-21 01:58 PDT, Yuan Song
no flags Details | Formatted Diff | Diff
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) (7.76 KB, patch)
2009-09-23 06:54 PDT, Yuan Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sridhar Gurivireddy 2007-07-09 12:35:10 PDT
http://sgurivireddy.googlepages.com/document_domain.html

changing document.domain to invalid location does not raise javascript exception.
Comment 1 Sam Weinig 2007-07-10 11:06:45 PDT
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 2 Alexey Proskuryakov 2007-07-10 11:25:18 PDT
FWIW, HTML5 currently specifies that a security exception must be raised when trying to set document.domain to a prohibited value.
Comment 3 Yuan Song 2009-08-26 06:56:22 PDT
Created attachment 38612 [details]
Patch to raise SECURITY_ERR exception if an attempt is made to change document.domain to an invalid value.
Comment 4 Yuan Song 2009-08-26 06:59:20 PDT
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.
Comment 5 Sam Weinig 2009-08-26 07:07:01 PDT
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-
Comment 6 Yuan Song 2009-08-27 09:44:40 PDT
(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.
Comment 7 Yuan Song 2009-09-03 12:39:32 PDT
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).
Comment 8 Eric Seidel (no email) 2009-09-04 00:40:26 PDT
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
Comment 9 Yuan Song 2009-09-08 05:34:29 PDT
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).
Comment 10 Darin Adler 2009-09-08 09:26:43 PDT
The patch looks good, but I don't see an answer to the "Have you tested IE?" question.
Comment 11 Yuan Song 2009-09-08 11:52:09 PDT
(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 12 Eric Seidel (no email) 2009-09-18 11:50:59 PDT
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 13 WebKit Commit Bot 2009-09-18 12:18:54 PDT
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 14 Eric Seidel (no email) 2009-09-18 12:23:47 PDT
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 15 WebKit Commit Bot 2009-09-18 13:06:25 PDT
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 16 Eric Seidel (no email) 2009-09-18 13:10:04 PDT
No clue how, but it would appear this change breaks basic-textareas.html.
Comment 17 Eric Seidel (no email) 2009-09-18 13:12:32 PDT
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-
Comment 18 Yuan Song 2009-09-21 01:58:40 PDT
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.
Comment 19 Darin Adler 2009-09-22 10:57:27 PDT
(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.
Comment 20 Yuan Song 2009-09-23 06:15:40 PDT
(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?
Comment 21 Yuan Song 2009-09-23 06:54:39 PDT
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.
Comment 22 Darin Adler 2009-09-23 09:34:22 PDT
(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.
Comment 23 Darin Adler 2009-09-23 09:34:49 PDT
(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.
Comment 24 Eric Seidel (no email) 2009-09-24 13:02:02 PDT
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 25 Darin Adler 2009-09-24 13:09:11 PDT
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?
Comment 26 Yuan Song 2009-09-25 04:51:34 PDT
(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.
Comment 27 Darin Adler 2009-09-25 10:09:42 PDT
(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.
Comment 28 Eric Seidel (no email) 2009-09-25 10:12:53 PDT
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?
Comment 29 Adam Barth 2009-09-25 10:36:45 PDT
(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 30 Eric Seidel (no email) 2009-09-25 10:38:36 PDT
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 31 WebKit Commit Bot 2009-09-25 10:53:52 PDT
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>
Comment 32 WebKit Commit Bot 2009-09-25 10:54:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Mark Rowe (bdash) 2009-10-08 19:35:51 PDT
This change added a DOM-related layout test to fast/js.  That’s wrong.