Last Comment Bug 327244 - Eliminate CheckLoadURI() and CheckLoadURIStr()
: Eliminate CheckLoadURI() and CheckLoadURIStr()
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla17
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 324464 327242 342484 342485 342486 342487 342489 348559 370719 408328
Blocks: clu 253761 JEP/caps 758258
  Show dependency treegraph
 
Reported: 2006-02-14 22:01 PST by Boris Zbarsky [:bz]
Modified: 2012-10-31 15:12 PDT (History)
20 users (show)
jonas: blocking1.9-
reed: wanted1.9+
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove CheckLoadURIStr() (9.39 KB, patch)
2012-07-16 20:28 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Remove CheckLoadURIStr() (9.35 KB, patch)
2012-07-16 21:07 PDT, Mounir Lamouri (:mounir)
jonas: review+
mounir: checkin+
Details | Diff | Splinter Review
Remove CheckLoadURI() (18.94 KB, patch)
2012-07-16 23:07 PDT, Mounir Lamouri (:mounir)
justin.lebar+bug: review+
jonas: review+
mounir: checkin+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2006-02-14 22:01:35 PST
Callers should just use CheckLoadURIWithPrincipal() instead.  As things stand, we do weird things when a document URI doesn't match the principal's URI, for example.

To fix this we need to make the relevant CAPS stuff scriptable (bug 327242) and then expose nsIPrincipal for documents, nodes, and possibly browsers (sorta the same problem as bug 324464).
Comment 1 Mounir Lamouri (:mounir) 2012-07-16 20:28:56 PDT
Created attachment 642852 [details] [diff] [review]
Remove CheckLoadURIStr()

Basic patch that change the two remaining calls of CheckLoadURIStr() to use CheckLoadURI() by simply creating URIs from the strings.
Comment 2 Mounir Lamouri (:mounir) 2012-07-16 21:07:56 PDT
Created attachment 642858 [details] [diff] [review]
Remove CheckLoadURIStr()

This is now using CheckLoadURI{Str,}WithPrincipal instead.
Comment 3 Mounir Lamouri (:mounir) 2012-07-16 23:07:46 PDT
Created attachment 642864 [details] [diff] [review]
Remove CheckLoadURI()

Justin, can you review nsDocShell changes?
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-17 00:34:36 PDT
Comment on attachment 642852 [details] [diff] [review]
Remove CheckLoadURIStr()

Review of attachment 642852 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/idl/nsIScriptSecurityManager.idl
@@ -117,5 @@
> -     * by scripts
> -     *
> -     * @deprecated Use checkLoadURIStrWithPrincipal instead of this function.
> -     */
> -    [deprecated] void checkLoadURIStr(in AUTF8String from, in AUTF8String uri,

Update the iid

::: content/base/src/contentAreaDropListener.js
@@ +89,5 @@
>        secMan.checkLoadURIStrWithPrincipal(sourceNode.nodePrincipal, uriString, flags);
> +    } else {
> +      secMan.checkLoadURI(ioService.newURI("file:///", null, null),
> +                          ioService.newURI(uriString, null, null),
> +                          flags);

Why not go all the way and create a principal and use checkLoadURIWithPrincipal here?

::: toolkit/content/nsDragAndDrop.js
@@ +591,5 @@
>        var sourceURI = sourceDoc ? sourceDoc.documentURI : "file:///";
>  
>        try {
> +        secMan.checkLoadURI(ioService.newURI(asourceURI, null, null),
> +                            draggedURI, nsIScriptSecurityManager.STANDARD);

You want to use checkLoadURIWithPrincipal here, and if sourceDoc is non-null use sourceDoc.nodePrincipal.

and 'asourceURI' should be 'sourceURI', no?
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-17 00:37:42 PDT
Comment on attachment 642864 [details] [diff] [review]
Remove CheckLoadURI()

Review of attachment 642864 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsIRefreshURI.idl
@@ +56,5 @@
>        *
>        * @param aBaseURI base URI to resolve refresh uri with.
>        * @param aHeader  The meta refresh header string.
>        */
> +    void setupRefreshURIFromHeader(in nsIURI aBaseURI, in nsIPrincipal principal, in ACString aHeader);

Update the documentation.
Comment 6 Justin Lebar (not reading bugmail) 2012-07-17 09:18:46 PDT
>@@ -5765,18 +5767,25 @@ NS_IMETHODIMP nsDocShell::SetupRefreshUR
>     nsresult rv;
>     nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aChannel, &rv));
>     if (NS_SUCCEEDED(rv)) {
>         nsCAutoString refreshHeader;
>         rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("refresh"),
>                                             refreshHeader);
> 
>         if (!refreshHeader.IsEmpty()) {
>+            nsCOMPtr<nsIScriptSecurityManager> secMan =
>+                do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
>+            NS_ENSURE_SUCCESS(rv, rv);
>+
>+            nsCOMPtr<nsIPrincipal> principal;
>+            secMan->GetChannelPrincipal(aChannel, getter_AddRefs(principal));
>+
>             SetupReferrerFromChannel(aChannel);
>-            rv = SetupRefreshURIFromHeader(mCurrentURI, refreshHeader);
>+            rv = SetupRefreshURIFromHeader(mCurrentURI, principal, refreshHeader);

If GetChannelPrincipal fails, we'll crash in SetupRefreshURIFromHeader.  I'm
not familiar with it, but looking at the code, it seems quite possible that the
call could fail.

>+++ b/docshell/base/nsIRefreshURI.idl

Update IID here too.
Comment 7 Mounir Lamouri (:mounir) 2012-07-18 13:37:28 PDT
try: https://tbpl.mozilla.org/?tree=Try&rev=d00fd7009533

Note You need to log in before you can comment on or make changes to this bug.