Using custom HTML in video embed code doesn't work

[lkraav]

https://github.com/Automattic/sensei/blob/version/2.4.0/includes/class-sensei-course.php#L3074 is broken.

Original issue #1613

Original fix #1631

Re-broken by adding wp_kses() at https://github.com/Automattic/sensei/pull/2296/files#diff-c0922c04ec6aa809fbd289ed7aabc9d7R3158 in #2296

Currently, Sensei()->settings->get( 'sensei_video_embed_html_sanitization_disable' ) is meaningless, because final wp_kses() call will not allow much custom HTML to pass.

EDIT same for lessons:

https://github.com/Automattic/sensei/blob/version/2.4.0/includes/class-sensei-frontend.php#L1020

UPDATE:
Work to be done for this issue can be found in this comment.

[donnapep]

I'm not a fan of the sensei_video_embed_html_sanitization_disable and would like to eliminate it eventually. It feels irresponsible on our part to allow for even the possibility of having security issues on a site.

Which elements are not working in the video embed fields? I'd rather that we explicitly add those that are not working as part of the existing wp_kses call, than remove escaping altogether.

[lkraav]

Which elements are not working in the video embed fields?

I'm not sure this is feasible. In our case, it's not only the video - it's also a complete transcript / chapter display system (mostly divs, but also paragraphs, custom icon elements, etc)

This is just us, I bet Sensei user surface worldwide can have more variations of elements.

I was looking for $allowed_html to be filterable, alas it currently isn't.

Allow for even the possibility of having security issues on a site.

🤷‍♂️ I don't know if this is really securing anything significant at the end of the day?

Alternatively, we could relatively easily sidestep using Sensei hooks to render our video display system directly in theme template however we wish, only keeping Sensei embed video meta box for data storage model.

But still, as-is, Sensei's "Remove security" option is broken. Do y'all have telemetry data on how much this option is even used? Perhaps could just remove it and related logic, and we all move on however?

[donnapep]

It seems there are a couple of issues here:

1. The necessity of the Disable HTML security setting, which sounds like a separate issue not to be tackled here.
2. The allowed HTML in the video embed fields for both courses and lessons is too restrictive.

2 seems like it could be solved by merging wp_kses_allowed_html( 'post' ) and $allowed_html, and also adding a filter for anyone who wants to add more elements.

[bhavik-dreamz]

i have updated code you can check here #2950 , please wait for approving this,