target="_top" allows framebusting by web content in iframes #83

Closed
lloyd opened this Issue Apr 28, 2011 · 14 comments

Projects

None yet

4 participants

@lloyd
Contributor
lloyd commented Apr 28, 2011

see tests/target_top

@CoderPuppy

I fixed it. Added a handler to windows.js which I made and got from taboca and modified.

@CoderPuppy CoderPuppy closed this Apr 28, 2011
@lloyd
Contributor
lloyd commented Apr 28, 2011

yo drewyoung1,

awesome! which commit is that change in? When you run ./chromeless tests/target_top can you verify that clicking on the link in the embedded iframe doesn't overwrite the parent?

@CoderPuppy

I didn't commit it yet. I am now. I also added new window support. And am
working on _blank s.

On Thu, Apr 28, 2011 at 3:15 PM, lloyd <
reply@reply.github.com>wrote:

yo drewyoung1,

awesome! which commit is that change in? When you run ./chromeless tests/target_top can you verify that clicking on the link in the embedded
iframe doesn't overwrite the parent?

Reply to this email directly or view it on GitHub:
#83 (comment)

@lloyd
Contributor
lloyd commented Apr 28, 2011

rad! Excited to get this merged! Reopening the issue till it's pulled in.

@lloyd lloyd reopened this Apr 28, 2011
@CoderPuppy

Do you know how to commit I don't?

On Thu, Apr 28, 2011 at 3:17 PM, Andrew Young drewyoung1@gmail.com wrote:

I didn't commit it yet. I am now. I also added new window support. And am
working on _blank s.

On Thu, Apr 28, 2011 at 3:15 PM, lloyd <
reply@reply.github.com>wrote:

yo drewyoung1,

awesome! which commit is that change in? When you run ./chromeless tests/target_top can you verify that clicking on the link in the embedded
iframe doesn't overwrite the parent?

Reply to this email directly or view it on GitHub:
#83 (comment)

@lloyd
Contributor
lloyd commented Apr 28, 2011

The best way is to "fork" the repo on github. You can then either look at github docs to figure out how to issue a "pull request", or you can just drop a link to your branch in this issue.

@CoderPuppy

I issued a pull request.

On Thu, Apr 28, 2011 at 3:30 PM, lloyd <
reply@reply.github.com>wrote:

The best way is to "fork" the repo on github. You can then either look at
github docs to figure out how to issue a "pull request", or you can just
drop a link to your branch in this issue.

Reply to this email directly or view it on GitHub:
#83 (comment)

@CoderPuppy

Here is my fork of Chromeless. It has the fix. And should have some others sometime later.

@lloyd
Contributor
lloyd commented Apr 29, 2011

I'll give this a look early next week if someone else doesn't beat me to it.

@lloyd
Contributor
lloyd commented May 3, 2011

I think the proper approach here may be twofold. It seems like we should cancel all navigation directed at the window which holds top level application code. This can probably be accomplished using a nsIContentPolicy.

The second phase would then be to intercept clicks inside content iframes and re-route them to frame that we believe to be the parent.

But still, I can't help but feel like we're monkey patching iframes from the outside. An end run around all of this, which may also solve some of our other issues, would be to intercept when iframes are added to the dom and replace them with containers that deeply resemble iframes to the application code, but have all the built in restrictions that we're trying to simulate here.

@taboca, you ever thought about this approach?

@taboca
Contributor
taboca commented May 3, 2011

It looks like nsBrowserAccess function from browser.js source of Fennec and Firefox is a sort of way of doing this, but I am feeling it only works for Chrome type windows.

@davidmurdoch davidmurdoch added a commit to davidmurdoch/chromeless that referenced this issue May 27, 2011
@davidmurdoch davidmurdoch Monkey Patching anchor elements with `target='_top'`. Possible partia…
…l fix for #83.
6dff767
@lloyd
Contributor
lloyd commented Jun 3, 2011

@davidmurdoch you're a genius! this patch behaves wonderfully! merging now...

@lloyd lloyd added a commit that closed this issue Jun 3, 2011
@lloyd lloyd merge impl of a content policy that blocks navigation after applicati…
…on page load. a very robust solution which closes #83 by @davidmurdoch
042489d
@lloyd lloyd closed this in 042489d Jun 3, 2011
@davidmurdoch
Contributor

haha. Its really just a partial fix.

What we REALLY need to do is dispatch events on the iframe element and let the main HTML code figure out what to do with it.

We would then be able to handle iframe navigation kinda like this:

someIframe.addEventListener("chromelessFrameNavigate", function(event){
   switch ( event.target ){
       case null : // null means the caller didn't not specify target
       case "_self" : 
       case "_top" : // _self and _top are the same
           return; // don't do anything as Chromeless will automatically load the uri in the frame anyway.
       case "_blank" :
           // browserObject object for handling browser tabs.
           browserObject.openNewTab({ uri:event.uri, parentWindow: this } );
       default:
           // obviously this logic isn't enough to properly decide what to do with a named target. Get over it. :-p
           var targetWindow = allOpenChromelessWindowsOrTabs( event.target );
           if( targetWindow && targetWindow.parentWindow === this ){
                browserObject.navigateWindow( { uri: event.uri, targetWindow: targetWindow } );
           }
           else {
               browserObject.openNewTab({ uri:event.uri, parentWindow: this } );
           }
           break;
   }
   return false; // maybe returning false would tell chromeless to NOT load the URI into the current frame.
}, false);

I really have no clue how to go about implementing events like this.

reference #83 and #73

p.s. please excuse my terrible naming conventions.

@lloyd
Contributor
lloyd commented Jun 3, 2011

oh, nice. I didn't realize #73 existed. let's take further discussion over there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment