Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting targetOriginDefault from frame content page. #199

Closed
andrej2k opened this issue May 12, 2015 · 10 comments
Closed

Setting targetOriginDefault from frame content page. #199

andrej2k opened this issue May 12, 2015 · 10 comments

Comments

@andrej2k
Copy link

Hi,

Is it possible to set targetOriginDefault parameter of iframeResizer.contentWindow script from iframe content page without enabling public methods?

Thanks,

Cheers, Andrej

@davidjbradshaw
Copy link
Owner

Hi,

Currently you can only set this on the sendMessage method. By default the parent checks messages come from the correct domain, but the iFrame doesn't have an option to restrict what page contains it currently.

It's an option on sendMessage as you could be sending data that needs to be more secure than the size of the page.

Allowing options to be set in the iFrame is on my todo list for version 3.

D.

@andrej2k
Copy link
Author

Hi,

Thanks for the quick response David.

Page size could be also sensitive information if we are talking about financial data i.e. you could guess if a user has some type of investments or not.

So, +1 to have possibility to set targetOrigin option from iframe content.

Another improvement would be to set origin option (or list of origins) for isMessageFromIFrame function in iframeResizer.js. Currently, we could only use iframe.src as origin or switch checkOrigin to false. Do you plan to add something like this also?
If you think it is useful feature, I could send patch or pull request.

Andrej

@davidjbradshaw
Copy link
Owner

I'd be happy to take a patch that allowed an array of allowed origins to be passed to the checkOrigin property. So allowed values became true, false and [array].

@andrej2k
Copy link
Author

Please check if following implementation is acceptable:

diff --git a/src/iframeResizer.js b/src/iframeResizer.js
index 411474f..77a4de2 100644
--- a/src/iframeResizer.js
+++ b/src/iframeResizer.js
@@ -171,10 +171,22 @@
                                origin     = event.origin,
                                remoteHost = messageData.iframe.src.split('/').slice(0,3).join('/');

-                       if (settings[iframeID].checkOrigin) {
-                               log(' Checking connection is from: '+remoteHost);
+                       var checkOrigin = settings[iframeID].checkOrigin;
+                       if (checkOrigin) {
+                               var checkOriginFunction;
+                               if (checkOrigin.constructor === Array) {
+                                       checkOriginFunction = function() {
+                                               log(' Checking connection is from list of origins: ' + checkOrigin);
+                                               return $.inArray(origin, checkOrigin) > -1;
+                                       }
+                               } else {
+                                       checkOriginFunction = function() {
+                                               log(' Checking connection is from: '+remoteHost);
+                                               return origin == remoteHost;
+                                       }
+                               }

-                               if ((''+origin !== 'null') && (origin !== remoteHost)) {
+                               if ((''+origin !== 'null') && !checkOriginFunction()) {
                                        throw new Error(
                                                'Unexpected message received from: ' + origin +
                                                ' for ' + messageData.iframe.id +

@davidjbradshaw
Copy link
Owner

Hi,

Looks good, but the $.inArray creates a jQuery dependancy and jQuery is strictly optional with this library. Have a look at this.

http://youmightnotneedjquery.com/#index_of

and then we would need to add a polyfil to the IE8 docs.

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf

Think it would be better to have one checkOriginFunction which can do both checks, rather than polymorphic construction which is going to be slow. Something like this maybe (Although I've not tested it).

checkAllowedOrigin(){
    function checkList(){
        log(' Checking connection is from list of origins: ' + checkOrigin);
        return checkOrigin.indexOf(origin) > -1;        
    }

    function checkSingle(){
        log(' Checking connection is from: '+remoteHost);
        return origin == remoteHost;        
    }

    return checkOrigin.constructor === Array ? checkList() : checkSingle();
}

Also it would be great if you could add a test for this.

Dave.

@andrej2k
Copy link
Author

Hi David,

I understand you concern regarding JQuery dependency. Here is updated patch. Regarding tests, it's hard to provide end-to-end test for such feature, because event.origin is null for file:// protocol and will be different for any other environment. We could somehow mock the data.origin but that will complicate the main code.

Index: src/iframeResizer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/iframeResizer.js    (revision d0965cca37f477a49f860286f6b1bde8bc9124d2)
+++ src/iframeResizer.js    (revision )
@@ -166,20 +166,39 @@
            messageData[dimension]=''+size;
        }

+       function checkAllowedOrigin(origin, checkOrigin){
+           function checkList(){
+               log(' Checking connection is from list of origins: ' + checkOrigin);
+               var i;
+               for (i = 0; i < checkOrigin.length; i++) {
+                   if (checkOrigin[i] === origin) {
+                       return true;
+                   }
+               }
+               return false;
+           }
+
+           function checkSingle(){
+               log(' Checking connection is from: '+remoteHost);
+               return origin == remoteHost;
+           }
+
+           return checkOrigin.constructor === Array ? checkList() : checkSingle();
+       }
+
        function isMessageFromIFrame(){
            var
                origin     = event.origin,
                remoteHost = messageData.iframe.src.split('/').slice(0,3).join('/');

-           if (settings[iframeID].checkOrigin) {
-               log(' Checking connection is from: '+remoteHost);
-
-               if ((''+origin !== 'null') && (origin !== remoteHost)) {
+           var checkOrigin = settings[iframeID].checkOrigin;
+           if (checkOrigin) {
+               if ((''+origin !== 'null') && !checkAllowedOrigin(origin, checkOrigin)) {
                    throw new Error(
                        'Unexpected message received from: ' + origin +
                        ' for ' + messageData.iframe.id +
                        '. Message was: ' + event.data +
-                       '. This error can be disabled by adding the checkOrigin: false option.'
+                       '. This error can be disabled by adding the checkOrigin: false option or providing of array of trusted domains.'
                    );
                }
            }
Index: README.md
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- README.md   (revision d0965cca37f477a49f860286f6b1bde8bc9124d2)
+++ README.md   (revision )
@@ -82,9 +82,9 @@
 ### checkOrigin

    default: true
-   type:    boolean
+   type:    boolean || array

-When set to true, only allow incoming messages from the domain listed in the `src` property of the iFrame tag. If your iFrame navigates between different domains, ports or protocols; then you will need to disable this option.
+When set to true, only allow incoming messages from the domain listed in the `src` property of the iFrame tag. If your iFrame navigates between different domains, ports or protocols; then you will need to provide an array of domains or disable this option.

 ### enableInPageLinks

@davidjbradshaw
Copy link
Owner

That's very hard to read in a ticket. Could you please make a pull request for it.

@andrej2k
Copy link
Author

Sure

@paulbors
Copy link

Feel free to use and add Selenium IDE tests to the project in order to
fully end-to-end test your code change.

On Thu, May 14, 2015 at 4:45 PM, Andrej Golcov [email protected]
wrote:

Sure


Reply to this email directly or view it on GitHub
#199 (comment)
.

~ Thank you,
Paul Bors

@davidjbradshaw
Copy link
Owner

Option added to v3.0.0.

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

No branches or pull requests

3 participants