search

WPTT / theme-sniffer

Theme Sniffer plugin using sniffs.
MIT License
270 stars 3 forks source link

Usage of the iframe false positive? #143

Open danyj opened 5 years ago

danyj commented 5 years ago

Is this false positive or? It says Usage of the iframe HTML element is prohibited

This is the line that triggers it https://github.com/Themezly/Creatus/blob/WP-Org-Prep/assets/js/thz.site.js#L3114

but the iframe element is needed for magnific popup to display all video sources.

https://github.com/dimsemenov/Magnific-Popup/blob/master/src/js/iframe.js#L27

timelsass commented 5 years ago

The iframe check happens upstream in the repository WPTRT/WPThemeReview so any bugs should be reported there. I believe this check is stating that themes are not responsible for content, in which case the code would best be added to a plugin that provides that functionality.

dingo-d commented 5 years ago

As far as I'm aware we are not allowing iframes in themes. This was added in the old theme check plugin, so I'm not sure the theme would pass the upload phase if it had it.

https://make.wordpress.org/themes/handbook/review/required/theme-check-plugin/

@joyously @carolinan @jocastaneda @justintadlock comments?

pattonwebz commented 5 years ago

Themes have not been allowed to add iframes in the past. This is mostly to prevent them loading in external content that could be dangerous. Exceptions might be made in certain situations but as a general rule iframes are not allowed.

If the videos loaded by this theme into the iframe are urls that the user has input themselves then this might be one of the exceptions to the general rule but I really don't see any way of us sniffing around it.

The sniff should probably still flag all instances of iframe for farther inspection.

danyj commented 5 years ago

Just so we are clear , we are talking about ligtboxes here like tb_iframe, not an inclusion of iframe or anything else. Here https://github.com/brainstormforce/astra/blob/35c7845e566ae6c4c619ad8a5abd4badd589d315/inc/core/class-astra-admin-settings.php#L293

and same scripts Magnific Popup and Fitvid are already in the themes repo.

https://github.com/oceanwp/oceanwp/blob/master/assets/js/third/magnific-popup.js#L1572 https://github.com/oceanwp/oceanwp/blob/0ea55c592fc757366083140b9739539f9d870b77/assets/js/devs/fitvids.js#L36

There is no other way for lightbox to display any 3rd party service video without it.

pattonwebz commented 5 years ago

@danyj I am aware of the kinds frames this Issue was opened about and do understand how they differ from the normal kinds of 'iframes are not allowed' that the rule is for. That is why I said that these kinds of things may be an exception to the general rule (also noting that I believe it would be a trouble to discount these kinds of frames in the sniff).

I was just adding some notes to this issue for future reference - sorry if I was unclear about that.