394471 - Can't "View image" moz-icon: images

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Load a file:/// directory listing
2)  Right-click on any non-folder icon in the listing
3)  Select "View image"

ACTUAL RESULTS:  Security check denies the load

EXPECTED RESULTS: urlSecurityCheck takes a flags argument (like Firefox's) and the "view image" code passes ALLOW_CHROME, just like regular image loads.

Comment 1

[[e-mail]]

Hmm, I think this bug might depend on the suite version of bug 303181, which I can't actually find right now :s

Comment 2

[Boris Zbarsky [:bzbarsky]]

That would make sense, yes.

Comment 3

[[e-mail]]

Oh, that was bug 363179 which is now fixed, so I can go ahead with this.

Comment 4

[[e-mail]]

No, wait, that only covered half of bug 303181...

Comment 5

[[e-mail]]

(In reply to comment #0)
>ACTUAL RESULTS:  Security check denies the load
But not only ours... View Image works by calling targetNode.ownerDocument.defaultView.open(targetNode.src, "_top") which throws a security error for moz-icon URLs.

Comment 6

[[e-mail]]

Attachment: Proposed patch

* Changed urlSecurityCheck to be a wrapper around checkLoadURIStrWithPrincipal
* Passed nsIScriptSecurityManager.ALLOW_CHROME when viewing an image
* Rewrote getTopWin's handling of potentially unsafe loads
* Got rid of getReferrer as we don't need the backward compatibility on trunk

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 280731 [details] [diff] [review]
Proposed patch

>Index: contentAreaUtils.js
> function openNewTabWindowOrExistingWith(aType, aURL, aDoc, aLoadInBackground)
> {
>   // Make sure we are allowed to open this url
>-  urlSecurityCheck(aURL, document);
>+  if (aDoc)
>+    urlSecurityCheck(aURL, aDoc.nodePrincipal,

Those first two args are backwards, no?

>Index: jar.mn

Not related to this bug, I assume?

>Index: openLocation.xul

Also not related?

>Index: utilityOverlay.js

>+        if (!opener || !isRestricted(url))
>+            topWindowOfType.loadURI(url);

So... isRestricted is kinda buggy.  It should be checking protocol flags, not scheme names.  Followup bug, if desired.

This whole function gives me the creeps in terms of losing track of who's doing loads, but not much I see we can do about it so far.  :(

Comment 8

[[e-mail]]

Attachment: Addressed review comments

Oops, so the bits of other files were unintentional, caused by me forgetting to save before attaching. Still, it gave me the chance to fix that caller and also I've taken the chance to beef up isRestricted().

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 280784 [details] [diff] [review]
Addressed review comments

>Index: utilityOverlay.js
>+    flags = Components.classes[kIOServiceProgID]
>+                      .getService(Components.interfaces.nsIIOService)
>+                      .getProtocolFlags(url.scheme);

I think you want to use nsINetUtil.URIChainHasFlags here, to handle nested URIs correctly.

Comment 10

[[e-mail]]

Attachment: Fixed patch

Well, they always say you learn something new every day :-)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 280790 [details] [diff] [review]
Fixed patch

Looks good!

Comment 12

[[e-mail]]

Fix checked in.

Comment 13

[mv_van_rantwijk]

Can someone please fix this error a.s.a.p:

-    throw "Load of " + url + " denied.";
+    throw "Load of " + aURI.spec + " denied.";

Comment 14

[mv_van_rantwijk]

(In reply to comment #13)
> Can someone please fix this error a.s.a.p:
> 
> -    throw "Load of " + url + " denied.";
> +    throw "Load of " + aURI.spec + " denied.";

Oops make that:
> +    throw "Load of " + aURI + " denied.";
 

Comment 15

[[e-mail]]

(In reply to comment #14)
>make that:
>>+    throw "Load of " + aURI + " denied.";
Done.