Opened 8 years ago

Last modified 3 years ago

#14078 assigned defect

[patch] [cla] dojo.hash doesn't work on IE for sites using document.domain

Reported by: Mike Wilson Owned by:
Priority: high Milestone: 1.15
Component: Core Version: 1.6.1
Keywords: Cc: Rob Retchless
Blocked By: Blocking:

Description

Some corporate sites use document.domain to become allow cross-site access between frames with different host names but same domain name. When used together with dojo.hash, this throws a Permission Denied error in IE 6-8 as these are using the IE workaround with a hidden iframe.

What's happening is that the page is then on another security domain than the hidden iframe, and the page is not allowed to access the window.location property on the iframe. Example:

www.dojotoolkit.org/mypage.html
    document.domain = "dojotoolkit.org";
    <iframe src="www.dojotoolkit.org/dojo/resources/blank.html">

The error can be verified with the following test case when deploying on a sub-domain such as www.dojotoolkit.org:

<!DOCTYPE html>
<!-- This test to be served on www.dojotoolkit.org and opened in IE 6/7/8 -->
<html>
	<head>
		<title></title>
		<script>
			document.domain = "dojotoolkit.org";
		</script>
		<script src="dojo/1.6.1/dojo/dojo.js"></script>
	</head>
	<body>
		<script>
			dojo.require("dojo.hash");

			dojo.ready(function() {
				dojo.hash("myanchor");
			});
		</script>
	</body>
</html>

The solution is to make sure the hidden iframe sets the same document.domain as its parent page.

I have provided a patch that supplies the desired domain as a parameter to the iframe, and then also makes sure to wait for onload in the iframe before doing anything with it, to make sure that the domain has been set.

We are currently running with this tweak in production on 1.6.1 so it would be great to see it included in 1.6.2.

Attachments (3)

dojo.hash-and-resources.blank.patch (4.1 KB) - added by Mike Wilson 8 years ago.
Updates to hash.js and blank.html to make dojo.hash work for document.domain
dojo.hash.14078.patch (4.8 KB) - added by Adam Peller 8 years ago.
patch from retchless (IBM, CCLA)
14078-2011-11-10.diff (4.8 KB) - added by Kenneth G. Franqueiro 8 years ago.
Updated patch that can apply against current trunk.

Download all attachments as: .zip

Change History (36)

Changed 8 years ago by Mike Wilson

Updates to hash.js and blank.html to make dojo.hash work for document.domain

comment:1 Changed 8 years ago by Adam Peller

Owner: set to Rob Retchless

comment:2 Changed 8 years ago by Mike Wilson

I just read that 1.7.0 RC1 is on its way. If 1.7.0 goes stable before 1.6.2 I can make a patch for trunk instead. Will this bugfix have a chance to be included in 1.7.0 if I do?

comment:3 Changed 8 years ago by Adam Peller

We're cutting rc1 today. Unless it's trivial I'd suggest we shoot for 1.7.1, which probably won't be far behind. You should try to get in touch with retchless either way.

comment:4 Changed 8 years ago by Rob Retchless

The patch looks good. My only concern is that this patch modifies blank.html, which is a common dojo resource. We might need to point at a separate html file for dojo.hash, or at least preserve the isLoaded = true, and do the extra domain check more defensively.

comment:5 Changed 8 years ago by Mike Wilson

Ah, right, I missed that blank.html is used in other places as well. It should be enough to make the document.domain assignment conditional, only triggered when a domain parameter is supplied.

I couldn't find any use of isLoaded, that's why I didn't preserve it, as I assumed it was left-over code from back in the day. But I could clearly be wrong here (too :-). In case it was used to detect load of the iframe, I instead use iframe.onload for that purpose in the patch.

Just let me know if you want me to take a further look at these things!

Changed 8 years ago by Adam Peller

Attachment: dojo.hash.14078.patch added

patch from retchless (IBM, CCLA)

comment:6 Changed 8 years ago by Adam Peller

Milestone: tbd1.7.1

comment:7 Changed 8 years ago by Rob Retchless

@mikewse, attached an updated patch that applies this fix to trunk, and creates a separate html file. The rest of the code is identical to your patch. I've sent an email to the dojo-contributors list asking whether this new html file is necessary, so stay tuned. Targeted for 1.7.1.

@peller, based on mikewse's request above, this should also be considered for 1.6.2.

comment:8 Changed 8 years ago by Mike Wilson

It'd be great to see this fix applied to 1.6.2 as well as it's a no-brainer for us to upgrade quickly to 1.6.2, while 1.7 may require a bit more testing! :-)

Feel free to improve my patch - I intentionally kept my modifications as simple as possible to illustrate the needed changes. The code line moved out from resetState() may call for some restructuring though, and I could imagine that a small refactoring could benefit the separation of the IeUriMonitor? from the rest of the code.

comment:9 Changed 8 years ago by Kenneth G. Franqueiro

Milestone: 1.7.11.8

Bumping to 1.8; can backport if there is a solid argument.

comment:10 Changed 8 years ago by Mike Wilson

Sorry if being blunt, but this ticket is about a bug that will throw errors at IE users on EVERY site that assigns document.domain and uses dojo.hash. The provided patch is trivial apart from the additions to blank.html. IMO this bug should be fixed in 1.6.x, 1.7.x and trunk.

Caring for Dojo, I think the part throwing the Permission Denied exception must be fixed in all the mentioned versions, as this error probably breaks other parts of the author's page (it did for us) and Dojo comes out more of a trouble-maker than a blessing.

An alternative to fixing the bug would be to just disable dojo.hash's iframe workaround in document.domain scenarios for now. This is a two-line fix that is still much much better than leaving it unfixed until 1.8. I can provide a patch if this is the preferred solution.

comment:11 Changed 8 years ago by Kenneth G. Franqueiro

That seems somewhat reasonable to me, but I would still suggest we fix on trunk first, kick the tires, then think about backporting.

I'm attaching a new patch against current trunk since the one from a few months back didn't merge quite cleanly. Doesn't seem to cause regressions in existing automated tests. (Not sure if it's possible for us to add an automated test for this xd scenario, but maybe we can add something like the test provided in the description?)

Changed 8 years ago by Kenneth G. Franqueiro

Attachment: 14078-2011-11-10.diff added

Updated patch that can apply against current trunk.

comment:12 Changed 8 years ago by Mike Wilson

Having a test case would be good, but it requires the test page to load from a domain (f ex www.somehost.com), not localhost or ip. If this is available, point me in the right direction and I can make up a test case.

comment:13 Changed 8 years ago by Mike Wilson

@kfg: trunk is ready for 1.8 so could you start the tire-kicking by applying the patch?

comment:14 Changed 8 years ago by Mike Wilson

Hey guys, could you apply this patch to trunk please?

comment:15 Changed 8 years ago by bill

Cc: Kenneth G. Franqueiro added

CC'ing kgf. Kgf, see above.

comment:16 Changed 7 years ago by Colin Snover

Milestone: 1.82.0

1.8 has been tagged; moving all outstanding tickets to next major release milestone.

comment:17 Changed 7 years ago by Mike Wilson

Is it really so controversial to add this bugfix, so it has to be delayed until 2.0?

This ticket contains a test case that allows you to manually verify that this is a bug in Dojo 1.6, 1.7 and 1.8. The same test case allows you to verify that the patch fixes the bug. The existing automated dojo.hash tests verify that nothing else is changed for non document-domain scenarios.

Kfg's updated patch has removed the only controversy found during discussions by letting dojo.hash use its own html file instead of blank.html. This patch should be straightforward to apply.

comment:18 Changed 7 years ago by bill

Milestone: 2.01.9

Kgf, can you check into 1.9 [initially]? I checked the history on hash.js and the owner is basically retchless (except he's not a committer), so the proxy committer might as well be you.

comment:19 Changed 7 years ago by dylan

Cc: Kenneth G. Franqueiro removed

This patch apparently does not work for IE7, which is why it was not committed.

If you can look into that, I'll push this forward for 1.9.

comment:20 Changed 7 years ago by bill

Owner: changed from Rob Retchless to Brian Arnold
Status: newassigned

comment:21 Changed 7 years ago by bill

Milestone: 1.91.10

Bumping this ticket since we are past the deadline for the 1.9RC. The fix can be put into 1.9.1 too, if desired.

comment:22 Changed 6 years ago by dylan

Owner: changed from Brian Arnold to bpayton

comment:23 Changed 5 years ago by bill

Cc: Rob Retchless added

I tried the test case in the description, putting it at http://bill.dojotoolkit.org/git/14078.html, and running on IE8. I don't see any console exceptions though, and it does append the #myanchor tag to the URL. What is the actual behavior vs. the expected behavior?

Last edited 5 years ago by bill (previous) (diff)

comment:24 Changed 5 years ago by Mike Wilson

This has been a long time. Give me a few days to dig up my notes and other test cases.

comment:25 Changed 5 years ago by Mike Wilson

So, I've had a look at this now.

The expected behavior when executing dojo.hash("myanchor") is to: 1) update the address bar 2) add an entry in the back/forward history Also, when navigation between anchors is done: 3) issue hashchange event

Most of the complex IE-specific code in dojo.hash relates to (2) as there by default is no history for anchor navigation in IE6/7. This is fixed in dojo.hash by using an iframe and it was the iframe that caused the document.domain problems. The page is loaded as 14078.html and after that sets 14078.html#myanchor. When (2) is working correctly, you should be able to back/forward between the anchors. You can try in Firefox to see how it works. Manually calling dojo.hash("abc") from the console command line should add yet another history entry and so forth.

The page at http://bill.dojotoolkit.org/git/14078.html is using Dojo 1.10 and it seems things are not working exactly the same as when I reported this bug on Dojo 1.6. These are my findings:

IE8: There is no iframe created so I assume the fix code is not activated on IE8 any longer. AFAIK the fix was never needed on IE8 but targeted it by accident back in Dojo 1.6. So (2) works but what we see is the browser's own support for it.

IE7 and IE6: The iframe is created so the fix code is initialized. Though, (2) isn't working so the fix isn't doing its job. I don't know what may have changed between Dojo 1.6 and 1.10 so I can't tell if the whole fix code somehow isn't properly triggered and therefore we don't get the document.domain error, or if this error just happens silently and this is the reason we don't get (2).

So, to restore full fuctionality you should look for ways to get (2) working in IE7. But on the other hand, getting something like this to work in IE7 is probably low priority, so might be time for my comment:10, remove the whole IE fix iframe code and let these users do without back/forward navigation between anchors.

comment:26 Changed 5 years ago by Colin Snover

IE6/7 are not on the list of supported environments any more. Given that, is there actually an issue here to be resolved?

comment:27 Changed 5 years ago by Colin Snover

Owner: changed from bpayton to Mike Wilson
Status: assignedpending

comment:28 Changed 5 years ago by dylan

It sounds like he's suggesting that we remove dead/useless code that doesn't work as expected, and was only there for IE 6/7...

comment:29 Changed 5 years ago by bill

Here's the relevant code from dojo/hash.js:

domReady(function(){
        if("onhashchange" in dojo.global && (!has("ie") || (has("ie") >= 8 && document.compatMode != "BackCompat"))){   //need this IE browser test because "onhashchange" exists in IE8 in IE7 mode
                _connect = aspect.after(dojo.global,"onhashchange",_dispatchEvent, true);
        }else{
                if(document.addEventListener){ // Non-IE
                        _recentHash = _getHash();
                        setInterval(_pollLocation, _pollFrequency); //Poll the window location for changes
                }else if(document.attachEvent){ // IE7-
                        //Use hidden iframe in versions of IE that don't have onhashchange event
                        _ieUriMonitor = new IEUriMonitor();
                }
                // else non-supported browser, do nothing.
        }
});

Then there's a bunch of other code that branches on whether or not _ieUriMonitor exists.

Chrome, firefox and IE8 all hit the first (outer) if(), so there's no setInterval() or _ieUriMonitor. I agree, we can get rid of all that code.

OTOH although 1.9 desupported IE6 & IE7, we didn't actively remove IE6 and IE7 code. So one could argue not to do it now too.

comment:30 Changed 5 years ago by Mike Wilson

Status: pendingnew

I would remove the IEUriMonitor/iframe stuff just based on the fact that it has caused user visible errors before. Apparently things have changed since Dojo 1.6 so this isn't happening now (or there is an environment dependency we are unaware of) but this may as well reverse again in a future version, and there are no test cases to discover it. So, I would weigh the benefit of keeping it (none, as it isn't working) against the risk (potentially reappearing user errors).

Btw Colin: I'm not a committer in Dojo so I probably shouldn't be owner of the bug (I'm the reporter).

comment:31 Changed 5 years ago by dylan

Milestone: 1.101.11
Owner: Mike Wilson deleted
Status: newassigned

Ok, based on this, I'm moving out to 1.11. Removing owner for now.

comment:32 Changed 4 years ago by dylan

Milestone: 1.111.12

Unfortunately moving to 1.12.

comment:33 Changed 3 years ago by dylan

Milestone: 1.131.15

Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.

Note: See TracTickets for help on using tickets.