Eliminate CheckLoadURI() and CheckLoadURIStr()

RESOLVED FIXED in mozilla17

Status

()

Core
Security: CAPS
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: bz, Assigned: mounir)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-needed})

Trunk
mozilla17
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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).
Blocks: 193255
Blocks: 253761
Blocks: 293973
Blocks: 330102
Depends on: 342484
Depends on: 342485
Depends on: 342486
Depends on: 342487
Depends on: 342489
Depends on: 348559
Flags: blocking1.9?
No longer blocks: 330102
Keywords: dev-doc-needed
Depends on: 370719
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Depends on: 408328
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
QA Contact: caps
(Assignee)

Comment 1

5 years ago
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.
Assignee: dveditz → mounir
Status: NEW → ASSIGNED
Attachment #642852 - Flags: review?(jonas)
(Assignee)

Comment 2

5 years ago
Created attachment 642858 [details] [diff] [review]
Remove CheckLoadURIStr()

This is now using CheckLoadURI{Str,}WithPrincipal instead.
Attachment #642852 - Attachment is obsolete: true
Attachment #642852 - Flags: review?(jonas)
Attachment #642858 - Flags: review?(jonas)
(Assignee)

Comment 3

5 years ago
Created attachment 642864 [details] [diff] [review]
Remove CheckLoadURI()

Justin, can you review nsDocShell changes?
Attachment #642864 - Flags: review?(justin.lebar+bug)
(Assignee)

Updated

5 years ago
Attachment #642864 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Blocks: 758258
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 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.
Attachment #642864 - Flags: review?(jonas) → review+
>@@ -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.
Attachment #642864 - Flags: review?(justin.lebar+bug) → review+
Attachment #642858 - Flags: review?(jonas) → review+
(Assignee)

Comment 7

5 years ago
try: https://tbpl.mozilla.org/?tree=Try&rev=d00fd7009533
(Assignee)

Updated

5 years ago
Attachment #642858 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #642864 - Flags: checkin+
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla17

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/913bd4954d7b
https://hg.mozilla.org/mozilla-central/rev/34fbd30f4f08
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Keywords: addon-compat
You need to log in before you can comment on or make changes to this bug.