I get worried whenever I see us doing security checks on URIs. Unless there is a Really Important Reason for it (and I can't think of one), this code should have a principal, not a URI. This is particularly weird because this code _also_ stores a domain....

Oops. I hadn't realized we'd filed a bug already. :(

What happens if nsDOMStorage is used in a document loaded from a javascript: URI? Trying to decided whether we should block on this or not...

P2 blocker for now. Dave, any thoughts here?

Dave, ping? Any thoughts on comment #3?

When used for globalStorage["foo.com"], mURI comes from the calling principal at creation time. When used for sessionStorage, it usually comes from the document's principal. But if a new window of the same domain is created after sessionStorage is used, the new storage's mURI will come from the document URI (session storages are cloned in nsDocShell::InternalLoad and nsGlobalWindow::OpenInternal, before we have the actual principal of the document that will end up having this session storage attached). So right now javascript: uris accessing storage work fine, except across new windows/sessionStorage clones. CanUseStorage uses a combination of the calling principal and mURI to decide whether access to storage is allowed based on cookie prefs. The calling principal is used at the top (if chrome, grant full permission) and mURI is used to evaluate cookie preferences. For how we're currently using mURI (deciding how to deal with cookie preferences), it should be ok to always use the calling principal to make decisions. This should fix javascript: uris in cloned windows. I'll put together a patch and some tests for that.

> (session > storages are cloned in nsDocShell::InternalLoad and > nsGlobalWindow::OpenInternal, before we have the actual principal of > the document that will end up having this session storage attached). There's another problem with javascript uris in new windows - the sessionStorage is never actually cloned, because we only compare if the URIs have the same domain. This should be pretty easy to work around short-term, by checking if the new URI's scheme inherits the principal. But longer-term we really ought to rework the session storage duplication to take place after we have a final principal for the load. Another option would be to just always clone the session storage object for the current domain. If the new load is from a different domain it will just get a new session storage anyway. If the new window ever navigates back to the original domain, it will have its session storage. I bring this option up because it's how the spec reads now, but I'm not sure it's necessarily the desired behavior.

(We could split off the cloning behavior into a separate bug, but "javascript uris in a new window" were the only observable difference I could think of changing the storage behavior, so unless we do something about it this bug will be hard to test)

Are we cloning before we even open the new channel? Or at some other point?

In the nsGlobalWindow case, we're cloning after opening the new channel. nsDocShell is cloning after creating the new channel, but that would be easy enough to fix.

Basically, if we clone when we get OnStartRequest, we'll have the new principal by then.... if we do it before, we don't.

Right. We clone near AsyncOpen(), not after OnStartRequest.

It probably wouldn't be too hard to store a clone of the current domain's session storage on the new docshell, and then in OnStartRequest decide whether to keep it or throw it away. I'll look in to that.

Could also store it on the channel and then decide when setting up the new document, right? I'd prefer that to storing on the docshell.

So to get window.open('javascript:alert(sessionStorage["foo"])') right, we actually need to have cloned the session storage before OnStartRequest(). So we could immediately copy over the session storage if we know the new load inherits our principal (we know it'll be able to access the same session storage). If not, we start the load and attach the potential storage to it, and decide when setting up the new document whether to keep it.

This is a first stab at removing mURI from the storage object and trying to fix up copying the storage object to the new docshell. The patch still needs some work, but I'd like a second opinion on whether this is a reasonable way to go about it.

Do we have to hand out the request? Can we just make the window watcher get it from the target docshell's loadgroup? It should be the defaultLoadRequest there. If that works (and we should probably double-check the URI at that point), then it seems like the right way to go... Alternately, we could use InternalLoad() here, though I guess that has some annoyances involved, right? In CopySessionStorage, what exactly is going on with javascript: URIs? Why do we need to smack the docshell in that case? Why not just put it on the javascript: channel and have it work normally? Then we wouldn't need to pass aTargetDocShell either. In CreateContentViewer, assert that the principal we get from mScriptGlobal is the channel principal of aRequest? That said, I don't see how mScriptGlobal/sop can be null at that point, so maybe assert that too? In this same method, replace the nested ifs with &&, please. I haven't read DOMStorage code carefully yet pending sorting out what javascript: is doing, but instead of GetSystemPrincipal+Equals could use IsSystemPrincipal. Why does window watcher need those headers you added?

(In reply to comment #17) > Do we have to hand out the request? Can we just make the window watcher get it > from the target docshell's loadgroup? It should be the defaultLoadRequest > there. Yeah, that should work. > In CopySessionStorage, what exactly is going on with javascript: URIs? Why do > we need to smack the docshell in that case? Why not just put it on the > javascript: channel and have it work normally? Then we wouldn't need to pass > aTargetDocShell either. As mentioned in comment #15, the javascript channel might need access to sessionStorage before OnStartRequest(). > Why does window watcher need those headers you added? Just cruft from working out some stuff, will remove those.

> As mentioned in comment #15, the javascript channel might need access to > sessionStorage before OnStartRequest(). This is only relevant when running the javascript: in a new window, right? If it's running against some other existing window we don't want to set up the sessionStorage, I would think...

(In reply to comment #19) > > As mentioned in comment #15, the javascript channel might need access to > > sessionStorage before OnStartRequest(). > > This is only relevant when running the javascript: in a new window, right? If > it's running against some other existing window we don't want to set up the > sessionStorage, I would think... Yeah, should probably move CopySessionStorage() into the isNewWindow clause above it.

Hmm. What about this use case: var win = window.open(); win.location = "javascript:whatever"; Should that work?

That is, maybe a better approach is to have the javascript: channel itself stick the session storage on the window it will execute against, and only if that window has never had anything loaded in it? The current code treats data: like JS, but for data: there is no special behavior needed here.

(In reply to comment #17) > Do we have to hand out the request? Can we just make the window watcher get it > from the target docshell's loadgroup? It should be the defaultLoadRequest > there. It turns out that a javascript: load never becomes the defaultLoadRequest, because it suppresses LOAD_DOCUMENT_URI and it adds LOAD_BACKGROUND...

Oh, right. Gah. What about using our built-in extensibility and putting the relevant storage object on the nsILoadInfo and then letting the docshell copy it over as needed? Just trying to not add more stuff to the docshell API here. :(

OK, this version just copies the storage to a new window during window.open() calls and targeted loads. The complexity needed to get the storage to tag along with the request was getting really unwieldy. A few things worth noting: To get "don't replace an existing storage" right (previous code would clobber existing storages during a targeted load to an existing window), we need to fix a problem where nsGlobalWindow would create an empty storage for the current principal whenever a storage changed. In the "dom-storage-changed" handler it was calling (indirectly) nsIDocShell::GetStorageForURI() to check if the notification applied to its own storage, which creates a new storage area if one doesn't exist. nsIDocShell::GetStorageForURI() also takes a URI where it should ideally take a principal. So this patch adds a getStorageForPrincipal() that can optionally create the storage if it doesn't exist, though it might be cleaner to separate that into createStorageForPrincipal() and getStorageForPrincipal(). If we're really looking to avoid adding API to docshell, we could solve this differently: all the code that checks for "do I have a session storage object" could check "do I have a session storage object, and if so does it have any items", and we could continue to use URIs instead of principals for getStorageForURI(). This would still leave us creating unused empty storages, and I'd still want to file a followup to replace getStorageForURI() and eventually AddSessionStorage() with nsIPrincipal equivalents once it's convenient to change API.

Comment on attachment 353947 [details] [diff] [review] WIP 2 >+++ b/docshell/base/nsDocShell.cpp Fri Dec 19 21:52:58 2008 -0800 >+nsDocShell::GetSessionStorageForPrincipal(nsIPrincipal* aPrincipal, >+ nsresult rv = aPrincipal->GetURI(getter_AddRefs(codebaseURI)); If it's correct that this doesn't use the domain, that should be clearly documented (ideally with a spec section reference). >+ PRBool canAccess = piStorage->CanAccess(aPrincipal); >+ NS_ASSERTION(canAccess, >+ "GetSessionStorageForPrincipal got a storage " >+ "that could not be accessed!"); >+ if (!canAccess) { Isn't the new setup exactly that we allow getting such storages and depend on the access check to not hand them out? If not, shouldn't this throw an exception instead of silently returning null? If so, the assert should go away. >+ NS_RELEASE(*aStorage); >+ *aStorage = nsnull; NS_RELEASE nulls out the pointer, so no need for the nsnull assignment. > nsDocShell::GetSessionStorageForURI(nsIURI* aURI, So with this change this is just left over and we can remove it in 3.2? Or is it still needed for some reason? >+nsDocShell::GetSessionStorageForURI(nsIURI* aURI, >+ PRBool aCreate, >+ nsCOMPtr<nsIURI> innerURI = NS_GetInnermostURI(aURI); >+ if (!innerURI) >+ return NS_OK; Can't happen in a sane way, can it? Either aURI is null (and then NS_GetInnermostURI will assert and crash), or NS_GetInnermostURI will return non-null, unless aURI is kinda broken. I think that should get an error return code. > nsCOMPtr<nsIDocShell> topDocShell = do_QueryInterface(topItem); > if (topDocShell != this) > return topDocShell->GetSessionStorageForURI(aURI, aStorage); What happened to aCreate? > nsCOMPtr<nsIDOMStorage> newstorage = > do_CreateInstance("@mozilla.org/dom/storage;1"); ... > *aStorage = newstorage; > NS_ADDREF(*aStorage); I know this was already here, but could be |newstorage.swap(*aStorage);|. Either way, though. >+++ b/dom/src/base/nsGlobalWindow.cpp Fri Dec 19 21:52:58 2008 -0800 >@@ -7223,48 +7225,35 @@ nsGlobalWindow::OpenInternal(const nsASt >+ printf("about to copy\n"); That should go, right? >+ if (aCalleePrincipal && mDocShell && newDocShell) { I was wondering... Would it make sense to just do the copying in window watcher instead of having it in both the OpenInternal callsite and the docshell callsite? Or would we still need the docshell code for targeted links into a window we opened some time before? >+++ b/dom/src/storage/nsDOMStorage.cpp Fri Dec 19 21:52:58 2008 -0800 >+GetPrincipalURIAndHost(nsIPrincipal* aPrincipal, nsIURI** aURI, nsString& aHost) >+ nsCAutoString spec; >+ innerURI->GetAsciiSpec(spec); Seems to be unused, no? >+nsDOMStorage::CanUseStorage(PRPackedBool* aSessionOnly) > // chrome can always use storage regardless of permission preferences > if (nsContentUtils::IsCallerChrome()) So this check is the only reason the GetPrincipalURIAndHost call below is safe without null-checking subjectPrincipal, right? That's worth documenting. It would also be nice if we tested this codepath (C++ call to CanUseStorage with no JS on the stack), somehow. >+nsDOMStorage::CacheStoragePermissions() So... Is it really ok that we smack mSessionOnly with whatever the subject principal happens to be right now? I guess it was that way already, but there's some serious documenting needed to explain why that's ok. As far as adding API to docshell, I'd just like us to not do it long-term. So if we're effectively deleting getStorageForURI and replacing it with getStorageForPrincipal, that's fine by me.

(In reply to comment #26) > >+ PRBool canAccess = piStorage->CanAccess(aPrincipal); > >+ NS_ASSERTION(canAccess, > >+ "GetSessionStorageForPrincipal got a storage " > >+ "that could not be accessed!"); > >+ if (!canAccess) { > > Isn't the new setup exactly that we allow getting such storages and depend on > the access check to not hand them out? If not, shouldn't this throw an > exception instead of silently returning null? If so, the assert should go > away. Changing this is probably fine, but I want to make sure I'm being clear on the intent of the new setup: The existing model is that every docshell has a set of storages, keyed by domain. GetSessionStorageForURI() will return the correct one for this domain, or create a new one if necessary. After browsing for awhile, multiple session storages might end up in the hash. So in the existing code, there is an implicit security check: when you look up your domain in the mStorages hash, it will only return a storage that your domain has access to, because the domain was the hash key. The old code added a second check when copying to a new window, to make sure the page that was loading would want the copied data. That check was causing problems, because it was difficult to predict whether the page would actually care about the data. So this patch just always copies, and relies on that implicit security check (the hash key for mStorages) to make sure the new page won't get access to the old storage. So if example.com loads a page on bar.com, example.com's storage will be copied, and will stay attached to that new docshell, but bar.com will never see it, because it will have a different mStorages key. If the user eventually goes back to example.com, they WILL see the cloned session storage (this is as specified, fwiw). I added the CanAccess() assertion because I wanted it to be a bit more clear that there are implicit security implications of that mStorages hash key. There should be no case where CanAccess() returns false, because the hash key should be unique to the set of people that can access that storage. All that said, it's probably worth returning an error code rather than silently failing, and documenting the assumption from the previous paragraph in the code. But I do think it's worthwhile to keep the assertion there, because there shouldn't be a case where the mStorages lookup returns an item we can't access.

(In reply to comment #26) > > nsDocShell::GetSessionStorageForURI(nsIURI* aURI, > > So with this change this is just left over and we can remove it in 3.2? Or is > it still needed for some reason? Yeah, this is left over, we can remove it whenever it's ok. This version puts getSessionStorageForPrincipal into a new interface for now. I'm not sure what the current plan is for when we're comfortable changing nsIDocShell, but I'll post a followup patch to move this method in there, and we can decide when it's best to land that. Let me know if you want to do this some other way. > >+ if (aCalleePrincipal && mDocShell && newDocShell) { > > I was wondering... Would it make sense to just do the copying in window > watcher instead of having it in both the OpenInternal callsite and the docshell > callsite? Or would we still need the docshell code for targeted links into a > window we opened some time before? As I understand the code, this should work fine - the only time it doesn't go through windowwatcher is when targeting a load in the same frameset, but we don't want to be copying in that case anyway. New patch moves that code into windowwatcher, and added a test case for this. > >+nsDOMStorage::CanUseStorage(PRPackedBool* aSessionOnly) > > // chrome can always use storage regardless of permission preferences > > if (nsContentUtils::IsCallerChrome()) > > So this check is the only reason the GetPrincipalURIAndHost call below is safe > without null-checking subjectPrincipal, right? That's worth documenting. It > would also be nice if we tested this codepath (C++ call to CanUseStorage with > no JS on the stack), somehow. This patch doesn't have a test yet, I need to look in to how to write a native test case. > >+nsDOMStorage::CacheStoragePermissions() > > So... Is it really ok that we smack mSessionOnly with whatever the subject > principal happens to be right now? I guess it was that way already, but > there's some serious documenting needed to explain why that's ok. This was added in 341524 to support cookie prefs. Entry points are supposed to be calling CacheStoragePermissions() to make sure this is up to date. Not pretty, but I added some documentation to the header. I can file a followup to find a more clear fix to that.

> But I do think it's worthwhile to keep the assertion there, Oh, absolutely!

Comment on attachment 355701 [details] [diff] [review] v3 Looks good to me. I suspect we can change the APIs for 1.9.2, but check in .planning?

Filed bug 473374 with the patch to clean up the API. Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/9dad087b466e Landed on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/635f4fcf9bd1