Comments

jadhavdevendra created an issue. See original summary.

jadhavdevendra’s picture

Status: Active » Needs review
jadhavdevendra’s picture

Issue summary: View changes
PA robot’s picture

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

jadhavdevendra’s picture

Can someone review and make it RTBC or needs work?

tatarbj’s picture

Assigned: Unassigned » tatarbj

Let me start it and get back to you @jadhavdevendra in a few hours!
Cheers,
Balazs.

tatarbj’s picture

Assigned: tatarbj » Unassigned
Status: Needs review » Needs work

Hi @jadhavdevendra,
i've reviewed the module and found security issue with the tpl file where $width and $height are not sanitized and open for XSS.
When you create a new video embed style and under the panopto video settings one of the textfield you use with this code snippet, the XSS issue occure and because of this, the module is not safe to be used.
"></iframe><script>alert('XSS')</script>
One possible solution could be to sanitize it before printing it out from a preprocess function, or even (that's a bit less nice) in the tpl file.
Currently the module also needs tests, that is definitely not a blocker issue, but for me personally took time to get how it could work, also had to manually modify the code to accept this url from panopto site: https://demo.hosted.panopto.com/Panopto/Pages/Viewer.aspx?id=31b3bd0f-32... (basically the domain had to be added in video_embed_panopto_video_embed_handler_info() as it should have been done in an info_alter() - i don't really know how big can be the use case for this purpose, but as an improvement could be to implement an admin interface where new domains can be added and will be handled here.
Also a hook_help() should be implemented to show the README.md's content on the website.
Let me know if you need more help by my side, but basically the first issue is a blocker to accept the application.
Bests,
Balazs.

jadhavdevendra’s picture

Status: Needs work » Needs review

@tatarbj I really appreciate you taking time out and helping reviewing the module.

  1. Added sanitization function filter_xss before passing the variable to template.
  2. Admin interface to add domains is already present. You need 7.x-2.0-beta11 version of video_embed_field module for that. The configuration section in README.md file have instructions to add different domain.
  3. Added hook_help.
tatarbj’s picture

Status: Needs review » Reviewed & tested by the community

Hi @jadhavdevendra,
thanks for the required changes to be implemented, i've checked it and again tested the module and i think we are good to go!
Nice work!
Bests,
Balazs.

apaderno’s picture

Assigned: Unassigned » apaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

apaderno’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.