Closed Bug 394471 Opened 17 years ago Closed 17 years ago

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

Categories

(SeaMonkey :: UI Design, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: bzbarsky, Assigned: neil)

Details

Attachments

(1 file, 2 obsolete files)

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.
Hmm, I think this bug might depend on the suite version of bug 303181, which I can't actually find right now :s
That would make sense, yes.
Oh, that was bug 363179 which is now fixed, so I can go ahead with this.
No, wait, that only covered half of bug 303181...
(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.
Attached patch Proposed patch (obsolete) — Splinter Review
* 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
Assignee: jag → neil
Status: NEW → ASSIGNED
Attachment #280731 - Flags: superreview?(jag)
Attachment #280731 - Flags: review?(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.  :(
Attachment #280731 - Flags: review?(bzbarsky) → review-
Attached patch Addressed review comments (obsolete) — Splinter Review
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().
Attachment #280731 - Attachment is obsolete: true
Attachment #280784 - Flags: superreview?(jag)
Attachment #280784 - Flags: review?(bzbarsky)
Attachment #280731 - Flags: superreview?(jag)
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.
Attachment #280784 - Flags: review?(bzbarsky) → review-
Attached patch Fixed patchSplinter Review
Well, they always say you learn something new every day :-)
Attachment #280784 - Attachment is obsolete: true
Attachment #280790 - Flags: superreview?(jag)
Attachment #280790 - Flags: review?(bzbarsky)
Attachment #280784 - Flags: superreview?(jag)
Comment on attachment 280790 [details] [diff] [review]
Fixed patch

Looks good!
Attachment #280790 - Flags: review?(bzbarsky) → review+
Attachment #280790 - Flags: superreview?(jag) → superreview+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0alpha
Can someone please fix this error a.s.a.p:

-    throw "Load of " + url + " denied.";
+    throw "Load of " + aURI.spec + " denied.";
(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.";
 

(In reply to comment #14)
>make that:
>>+    throw "Load of " + aURI + " denied.";
Done.
You need to log in before you can comment on or make changes to this bug.