I just noticed the following code in the javascript: protocol handler: // Get principal nsCOMPtr<nsIPrincipal> principal; nsCOMPtr<nsIURI> referringUri; if (originalURI) { referringUri = originalURI; } else { nsCOMPtr<nsIDocShell> docShell; docShell = do_QueryInterface(globalOwner); if (!docShell) return NS_ERROR_FAILURE; if (NS_FAILED(docShell->GetCurrentURI(getter_AddRefs(referringUri)))) return NS_ERROR_FAILURE; } The "original URI" is absolutely not the referring URI. An original URI is when you start with resource:foo and resolve it to file:.../foo (the resource: is the original, the file: is the current URI). The referrer is a distinctly http-specific thing, so you need to QI to nsIHTTPChannel and use its interface to get a referrer.

Well, in this case the originalURI is the referrer. This code from nsWebShell.cpp sets it up this way: rv = pNetService->NewChannelFromURI(aCommand, aUri, loadGroup, requestor, aType, referrer /* referring uri */, 0, 0, getter_AddRefs(pChannel)); At any rate, we need some way to pass along the URI that is responsible for loading the channel (for http: and other protocols). This is the way nisheeth set it up. I suppose we could use the nsIChannel's owner attribute for this instead. Thoughts?

I just remembered that I also filed bug 29831 about that issue (that the webshell does it wrong too).

Warren, Can I add a parameter for the owner to NewChannelFromURI? Setting the owner after the channel is created is too late for javascript: and data: URIs, which need the owner in order to be able to run scripts under the appropriate principals.

Subject: Re: original/referring URI issue Date: Fri, 17 Mar 2000 12:13:06 -0800 From: Warren Harris <warren@netscape.com> To: Norris Boyd <norris@netscape.com> References: 1 Norris, I'm in the process of doing major surgury on nsIChannel -- making many of the initialization parameters become attributes. In the process of doing this, I'm fixing the js protocol to not evaluate until AsyncRead/OpenInputStream is called (this is the way it used to be some time ago, and it was changed to fix a bug, but we have a better fix now). I'll let you know when I get this in -- hopefully tonight. Warren Norris Boyd wrote: This is from bugs 31818 and 28387. Can I add a parameter for the owner to NewChannelFromURI? Setting the owner after the channel is created is too late for javascript: and data: URIs, which need the owner in order to be able to run scripts under the appropriate principals. Since the owner is already an attribute of the channel, it seems like a reasonable extension. Then I can remove the instances where the current URI is confused with the referrer. Thanks, Norris

Norris: The javascript: protocol evaluation now occurs when AsyncRead/OpenInputStream is called, so you should be able to construct the channel, and then SetOwner before the evaluation occurs.

Turns out this blocks my fix for 34769 which requires me to stop setting the referrer as the original uri on the channel in nsWebShell. I broke one of the smoketests I think because the JS protocol handler expects to call GetOriginalURI on the passed in channel and it wants to get the referrer. Now that I'm not setting it, this breaks and bad things happen. Looks like we need to set the owner on the channel on the JS side in this case?

Norris is out this week (till Monday 4/10). If this is blocking, and is needed for an M15 stopper, then we need some external assistance (mscott??). If this is not blocking an M15 stopper, this needs to be pushed to M16 (so that we can branch for M15 checkpoint).

A fix for this (along with bugs 33803 and 28387) is forthcoming, but it isn't ready yet. Marking as M16.

Bulk reassigning most of norris's bugs to mstoltz.

Problems with the trust boundary for javscript: URLs. Marking M17.

promoting because of dependency from 39089.

mstoltz, what's the plan for M17, and can I help? /be

Brendan, Yes...thanks for the offer. I have a patch created by Norris (I'll post it here) which apparently fixes this problem for most cases, but it's not complete. Could you take a look at this patch and see what else is required?

Putting on [nsbeta2+][6/01] radar. This work must be done by 06/01 or we may pull this for PR2. Please work with brendan to get in ASAP. Thanks!

Assessment: Providing backward compatibility with DOM0 JS 1.1 code on the web is a critical goal for the browser to be a viable product. javascript: URLs are fairly widely used even by beginning JS programmers and are common in the JS 1.1 code that's predominant on the web. Until we are executing the JavaScript code in javascript: URLs on legacy web pages, we won't even be detecting the other backward compatibility bugs we must have. We must fix this for nsbeta2 if we are to have any hope of finding the other b.c. bugs that will be exposed by executing the code in <A HREF="javascript: ..."> URLs in time to fix them for FCS. Clearing [nsbeta2+][6/01] to trigger re-evaluation. Recommend [nsbeta2+] stopper.

Putting on [nsbeta2+][dogfood-] radar. Does not need a fix ASAP for daily work, but we should fix this for beta2.

I haven't had the opportunity to work on this, so Norris's patch is the most recent solution I have. Thanks for looking at this.

I haven't had the opportunity to work on this, so Norris's patch is the most recent solution I have. Thanks for looking at this.

*** Bug 38537 has been marked as a duplicate of this bug. ***

*** Bug 34667 has been marked as a duplicate of this bug. ***

*** Bug 36745 has been marked as a duplicate of this bug. ***

*** Bug 40396 has been marked as a duplicate of this bug. ***

*** Bug 41395 has been marked as a duplicate of this bug. ***

Clearing [dogfood-] for reevaluation. This bug prevents me from searching talkback, and that's dogfood for me.

Putting on [dogfood+] radar.

ETA is 6/9 (tomorrow).

*** Bug 41919 has been marked as a duplicate of this bug. ***

*** Bug 42002 has been marked as a duplicate of this bug. ***

Fix in hand; will check in on Monday.

Mitch, can you attach the complete fix? Thanks, /be

Yeah, I'd like to see this before you commit it.

*** Bug 33940 has been marked as a duplicate of this bug. ***

+ + NS_IMETHODIMP nsDocShell::GetCurrentDocumentOwner(nsISupports** aOwner) + { + nsresult rv; + *aOwner = nsnull; + nsCOMPtr<nsIDocumentViewer> docv(do_QueryInterface(mContentViewer)); + if (!docv) return NS_ERROR_FAILURE; + nsIDocument* doc; + rv = docv->GetDocument(doc); Why not nsCOMPtr<nsIDocument> doc; rv = docv->GetDocument(getter_AddRefs(doc)); instead of manually ref-counting doc? + if (NS_FAILED(rv) || !doc) return NS_ERROR_FAILURE; + nsCOMPtr<nsIPrincipal> principal; + rv = doc->GetPrincipal(getter_AddRefs(principal)); + NS_IF_RELEASE(doc); I'd use an nsCOMPtr for doc, but if you don't, then this should be NS_RELEASE(doc); because you've already tested for !doc and returned error above. + else + { + nsCOMPtr<nsIStreamIOChannel> ioChannel(do_QueryInterface(channel)); + if(ioChannel) // Might be a javascript: URL load, need to set owner + { + static const char jsSchemeName[] = "javascript"; + char* scheme; + aURI->GetScheme(&scheme); + if (PL_strcasecmp(scheme, jsSchemeName) == 0) + channel->SetOwner(aOwner); + } + else + { // Also set owner for data: URLs + nsCOMPtr<nsIDataChannel> dataChannel(do_QueryInterface(channel)); + if (dataChannel) + channel->SetOwner(aOwner); + } + } Ick -- why not always set owner here, whether or not the scheme is "javascript"? Can we not unify cases here and just always call channel->SetOwner? What goes wrong with other channel impls if we do (I don't see any error propagation here, so it must be unwanted side effects)? ! ! /* ! The owner of the load, that is, the entity responsible for ! causing the load to occur. This should be a nsIPrincipal typically. ! */ ! attribute nsISupports owner; ! }; Not your file, I know -- and you're no doubt imitating the prevailing comment style (which is The Right Thing, aka When In Rome), but I thought I'd plunk for /** ... */ doc comments here, in case you're up for reforming that file, or bugging its owner to use javadoc comments. Thanks, every bit helps. With another few rounds of review, and if this tests out well, I think it should go in M16. Cc'ing leaf. /be

what's the eta? this looks like something i'd want applied to a clean pull of the branch and tested thoroughly before checking in, if at all. I want to release this week.

Brendan, In response to your comments in the bug: >Why not > > nsCOMPtr<nsIDocument> doc; > rv = docv->GetDocument(getter_AddRefs(doc)); > instead of manually ref-counting doc? nsIDocumentViewer::GetDocument() takes a nsIDocument*& parameter, not nsIDocument**. nsCOMPtr didn't seem to work here. Is there a way to pass a getter_AddRefs(nsCOMPtr<...>) as a nsIDocument*& ? As for the ugly protocol-specific code which strcmps for "javascript," here's my dilemma: The vast majority of protocols should not inherit their owner from the loading page, they should initially derive an owner from their own URL using GetCodebasePrincipal. This currently happens in nsDocument::GetPrincipal() for cases where a document does not get an owner from the channel that loaded it. If I were to set owner for all channels in DocShell, every page would inherit its owner from its referring page (in the case of link clicks anyway) and this is incorrect; only javascript: and data: URLs should do this right now. The other way to accomplish this that I could think of was to add a function to nsIChannel, something like "SetReferringOwner()" which would set the owner for javascript: and data: and do nothing for all other channel types. I wanted to avoid changing nsIChannel since this interface is used all over and I'm trying to "tread lightly" on the code at this stage of the game. I agree that protocol-specific code in DocShell is ugly, but it vastly reduces the number of files which need to be changed, and anyway we're already doing that sort of thing here in DocShell with http URLs. Can you think of an alternate solution?

Yes, there is a way to use |nsCOMPtr| in this case. Apply the |*| operator, like so nsCOMPtr<nsIDocument> doc; nsresult rv = docv->GetDocument(*getter_AddRefs(doc)); See the examples at <http://www.mozilla.org/projects/xpcom/nsCOMPtr.html#ref_getter_AddRefs> The first code box there shows some declarations. |GetFoo2| illustrates your situation. The side-by-side boxes below show raw-pointer versus |nsCOMPtr| clients of those interfaces.

I was thinking that there would be a default SetOwner impl that did nothing, and specialized overriding methods in javascript: and data: channel impls that did set mOwner. Then the default codebase-principals computation would use mOwner if set, otherwise it would use the document's URL. But this is no doubt too stupidly simple to work. What am I missing? /be

Subject: Re: Bug 31818 Date: Mon, 12 Jun 2000 17:48:51 -0700 From: Brendan Eich <brendan@meer.net> To: Mitchell Stoltz <mstoltz@netscape.com> CC: vidur@netscape.com, warren@netscape.com References: 1 , 2 , 3 I will take Vidur's suggestion and use rv = docv->GetDocument(*getter_AddRefs(doc)); Sorry I forgot about this, it's obvious in hindsight (if & works at all, it must work for *p such that &(*p) is p, but as a reference [never null]). Vidur and I considered the way you suggested, that is, making setOwner() a no-op for every other type of channel. There are two problems with this: the chrome protocol handler uses setOwner() to assign the system principal to chrome. This happens below the call to NS_NewURI() in docshell, which means that any subsequent setOwner() calls overwrite the system principal for chrome, and the whole world ends. Well, shoot. That means we're overloading the ambiguous term "owner". I suspect you're right, and we now need yet another channel method, say SetOriginatingURL or (your suggestion, but is it really specific to referrers?) SetReferringOwner. That's the only legitimate existing use of setOwner(), but since the chrome protocol creates various different kinds of channels, the assignment of the system principal can't be pushed any lower. So I think that way is out. Another crazy idea. At most one SetOwner per channel call is respected, after which the first URI set as the owner "sticks", and subsequent sets fail. This allows chrome to "preset" a trump card, even there is to be a later SetOwner call just in case a javascript: or data: link was clicked, or src=javascript: attribute set. But for content (as opposed to chrome) channels, the first and sticky set is the one from the docshell. The alternative, as I mentioned, is adding another function to nsIChannel. Do you think I should do this now, or stick to the ugly protocol-specific code in docshell? As I wrote, we should do the "right thing" in due course, not to fix this bug for m16, or even m17. For the short run, we should do the expedient thing. Your special cases for javascript: and data: are fine in that timeframe.

One other problem: There's no javascript: channel. the JS protocol creates a StreamIOChannel; that's why I had to use a strcmp in docshell. Is it worth creating an nsJSChannel merely to override SetOwner()?

Let's do the expedient thing (special case GetScheme strcasecmp) for now, but I would say for the long run, either implement nsIChannel for javascript: URLs, or enhance StreamIOChannel so its setOwner and getOwner call into some sub-interface that nsJSProtocolHandler.cpp can implement cheaply (i.e., without having to implement all of the channel methods). /be

*** Bug 37702 has been marked as a duplicate of this bug. ***

mstoltz, didn't you just check in the fix for this?

Fix is in (on the tip), marking FIXED.

concerns that this may not be completely fixed bug 44022

Discussed with mstoltz. He pointed out bug 30915 as a passing test case for the work done here. There are problems with javascript urls from the location bar but they should be filed in separate bugs. verified: WinNT 2000070508

*** Bug 33940 has been marked as a duplicate of this bug. ***