248052 - HTTP should not depend on nsIScriptSecurityManager

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Darin Fisher]

HTTP should not depend on nsIScriptSecurityManager.  Currently, we call
CheckLoadURI when we process redirects.  We should not call this directly. 
Instead, we should implement some sort of global redirect observer API.  Then
the script security manager could use that API to force a global redirect policy.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

-> me

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

note: I can't remove caps from REQUIRES yet, because digest auth uses it (nsISignatureVerifier, nsICryptoHash)

Comment 3

[Darin Fisher]

Comment on attachment 201950 [details] [diff] [review]
patch

>Index: caps/src/nsScriptSecurityManager.cpp

>+NS_IMETHODIMP
>+nsScriptSecurityManager::OnChannelRedirect(nsIChannel* oldChannel, 
>+                                           nsIChannel* newChannel,
>+                                           PRUint32 redirFlags)
>+{
>+    nsCOMPtr<nsIURI> oldURI, newURI;
>+    oldChannel->GetURI(getter_AddRefs(oldURI));
>+    newChannel->GetURI(getter_AddRefs(newURI));
>+
>+    if (!oldURI || !newURI)
>+        return NS_ERROR_UNEXPECTED;

use NS_ENSURE_STATE?


>Index: caps/src/nsSecurityManagerFactory.cpp

>+      NS_GLOBAL_REDIRECT_OBSERVER_CONTRACTID,

NS_GLOBAL_CHANNELEVENTSINK_CONTRACTID might be a better name since it
is easier to remember :-)


I can imagine extensions wanting to observe redirects of all channels
as well, so it would almost make better sense to do this via a XPCOM
category.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

yeah, a category would be nice, but I think we should do that in another bug. (or maybe an nsIObserverService notification?)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201995 [details] [diff] [review]
patch v2 

Hmm... So with this approach a random extension can accidentally disable this by overriding the contract?  That seems undesirable...

I don't think we can pass both URIs to nsIObserver's Notify() method in a reasonable way either.

So perhaps we should indeed just do that XPCOM category?

Comment 6

[Darin Fisher]

> Hmm... So with this approach a random extension can accidentally disable this
> by overriding the contract?  That seems undesirable...

Sure, I guess someone looking for a way to intercept global redirects might be tempted into that trap.  Documentation could be the answer too.


> I don't think we can pass both URIs to nsIObserver's Notify() method in a
> reasonable way either.

I think biesi and I talked about a way to expose the previous channel (and flags) as a property on the new channel, so that it would be possible to observe the redirect more easily from nsIWebProgressListener::OnStateChange.
The same approach could be used with nsIObserver/nsIObserverService.


> So perhaps we should indeed just do that XPCOM category?

Perhaps.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  So if we're going to work on a more generic solution, do we still want to do this for now?  I assume we'll remove this contract once we have those other solutions in hand?

Comment 8

[Darin Fisher]

> OK.  So if we're going to work on a more generic solution, do we still want to
> do this for now?  I assume we'll remove this contract once we have those other
> solutions in hand?

As I said above, I don't mind if this goes in now.  It's better than what we have today, and no worse IMO.  It's really up to biesi how he wants to proceed given everything we've just discussed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201995 [details] [diff] [review]
patch v2 

OK, sr=bzbarsky

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

I checked this in for now. I, too, think this is better than what we have today, and especially I want to avoid the caps/js/xpconnect dependency that bug 312760 would otherwise introduce. I'm not that concerned about extensions disabling this specific security check, since they can screw the browser in so many other ways.

filed bug 315598 as a followup

Checking in caps/include/nsScriptSecurityManager.h;
/cvsroot/mozilla/caps/include/nsScriptSecurityManager.h,v  <--  nsScriptSecurityManager.h
new revision: 1.95; previous revision: 1.94
done
Checking in caps/src/nsScriptSecurityManager.cpp;
/cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v  <--  nsScriptSecurityManager.cpp
new revision: 1.278; previous revision: 1.277
done
Checking in caps/src/nsSecurityManagerFactory.cpp;
/cvsroot/mozilla/caps/src/nsSecurityManagerFactory.cpp,v  <--  nsSecurityManagerFactory.cpp
new revision: 1.42; previous revision: 1.41
done
Checking in netwerk/build/nsNetCID.h;
/cvsroot/mozilla/netwerk/build/nsNetCID.h,v  <--  nsNetCID.h
new revision: 1.52; previous revision: 1.51
done
Checking in netwerk/protocol/http/src/Makefile.in;
/cvsroot/mozilla/netwerk/protocol/http/src/Makefile.in,v  <--  Makefile.in
new revision: 1.70; previous revision: 1.69
done
Checking in netwerk/protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChannel.cpp
new revision: 1.264; previous revision: 1.263
done