Youtube plugin
-
I'm now getting around to overhauling this. On the server side, I'm able to talk to Youtube's v3 API, so I can get the title (and a bunch of other stuff). However, what do we want to see?
I know a lot of people just want a link, no embedding. That's too bad. But. I can certainly put a link in. Right now I have something working that does only this. Looks something like:
Do we want the same sort of thing we have embedded currently? Do we want it just to embed the player straight away? If the answer is what we have or just embedding the player, I should be able to handle that, but if we want something different...what? How?
What parameters should we support in the embedded player (
t,start,end, obviously). See here for inspirationI figure the link (like in the screenshot above) would keep whatever the user put in (or maybe randomly delete some if it detects the user is blakey, just to give us something to talk about). But to embed stuff, some mangling needs to be done, and I recall looking at those and deciding that stuff like allowing people to hide the controls is not something we'd want.
-
@boomzilla Pretty sure you already asked me this and I already answered it in a different thread.
-
@boomzilla said in Youtube plugin:
I know a lot of people just want a link, no embedding.
if you're doing this serverside......
i don't suppose you could have a user menu option to let them choose wether they want an embed or the link?
just a thought.....
-
@accalia said in Youtube plugin:
@boomzilla said in Youtube plugin:
I know a lot of people just want a link, no embedding.
if you're doing this serverside......
i don't suppose you could have a user menu option to let them choose wether they want an embed or the link?
just a thought.....
I don't mind the embed with the preview image (even though I'll never watch the embed). I wouldn't object to a useroption to not show the embed, especially for mobile users.
The title above the video as a clickable link to the Youtubez. Maybe also include the username of the GooTuber who posted it?
<a href="{{url}}" target="_blank">{{VideoTitle}} <br/> by <small>{{Username}}</small> </a> {{VideoEmbed}}- Be sure to run it through a sanitizer and HTML Encoder, as we learned the hard way from DickXSS
- Put sufficient classes on all the divs and other elements so people can stylish it however they want.
-
@Lorne-Kates said in Youtube plugin:
Be sure to run it through a sanitizer and HTML Encoder, as we learned the hard way from DickXSS
Hmm....@ben_lubar, I notice that you set the priority on your html cleaner plugin to other than the default 10. Relevant hooks for this:
{"hook": "filter:parse.raw", "method": "fixRaw", "priority": 4}, {"hook": "filter:parse.post", "method": "cleanPost", "priority": 6},Why did you pick those priorities? I guess I should set the priority of this to a lower number to make sure that html cleaner sees its output?
-
@Lorne-Kates said in Youtube plugin:
Maybe also include the username of the GooTuber who posted it?
Ah, yeah, maybe something like:
: Silat Suffian Bela Diri - Hidden Baiting Strategy
— Maul565
-
@boomzilla why the
:? It seems kind of unnecessary.
-
So, I've mainly left it as-is on the front end, at least for now. There are issues there, of course, I think with streaming posts killing the player, but for now I guess I'm fairly happy with what it's doing. The client side markup is basically the same (though technically the way it works now, the post actually has an iframe'd player that gets replaced by javascript.
The one remaining thing I need to do is to clean the title and channel text. I can't just process before @ben_lubar's html cleaner, because then that cleans everything. But I guess I really probably only need to worry about
<and>, no? Just escape all of those?Oh, one difference is that in the preview, instead of a clickable player, I show a smaller thumbnail. My logic there was that the preview is kind of small, so it's probably worth the miniaturization inconsistency in the name of space. So, preview looks like this:
...and the final post like this:
The calls to youtube to fetch the data requires an API key and I added an LRU cache. I set it up to start with a size of 100. I think there's a way to reduce the amount of data I'm fetching (and therefore storing, too). Probably not a huge amount, and maybe the cache size should be higher.
-
@boomzilla said in Youtube plugin:
I really probably only need to worry about
<and>, no?If you have anything in an attribute, you need to escape
"as well.
-
@ben_lubar said in Youtube plugin:
If you have anything in an attribute, you need to escape
"as well.And
&.
-
@boomzilla said in Youtube plugin:
There are issues there, of course, I think with streaming posts killing the player,
To fix that, you'd actually have to shove the playing video in its own iframe (like a pop-out dialog-y thing), outside of any DOM nodes that would be updated by streaming-in content. It's not hard code to write, but way more effort than any lazy-ass developers writing a forum system like this would.
-
-
@ben_lubar said in Youtube plugin:
If you have anything in an attribute, you need to escape " as well.
@anotherusername said in Youtube plugin:
And &.
Right.
-
@ben_lubar I… don't get it. I don't remember any posts that could kill you.
-
@asdf the post on the right in that screenshot would deal quite a bit of damage if you rammed into it at full speed.
-
@ben_lubar Guess I have to play again and try it out.
-
I've added duration and start / end to the text (if they exist).
Additionally, if you don't have a youtube API key, it just says "Youtube Video" for the title. It will give start / end information, but there's no duration:
Regular:
With start / end
-
I pushed the changes up into the
server-sidebranch:
GitHub - boomzillawtf/nodebb-plugin-youtube-embed at server-side
Embed Youtube Videos on your NodeBB Forum. Contribute to boomzillawtf/nodebb-plugin-youtube-embed development by creating an account on GitHub.
Would appreciate it if people could take a look. I'm a bit dubious about getting the maintainer to accept this stuff as a PR. We may need to fork.
-
@boomzilla Only problem I see is that your regex for
t=also matchesfoobart=.
Also, can't times be mixed minutes and seconds? So\d+is insufficient.
-
@error Ummm....hmmm....apparently I messed up the commit. Look again.
-
@boomzilla That's a big difference. Hm. Markup concatenation looks possibly susceptible to XSS.
-
Why can't we embed html5 video?
-
@error said in Youtube plugin:
Markup concatenation looks possibly susceptible to XSS.
It assumes that the original youtube link already went through @ben_lubar's html-cleaner. Should probably make that clear.
The only other source of text comes from the youtube video title and channel titles and I escaped the <s.
-
@boomzilla said in Youtube plugin:
The only other source of text comes from the youtube video title and channel titles and I escaped the <s.
Wasn't there a video with a XSS title that was used as proof-of-concept on Dipshore?
-
@Lorne-Kates said in Youtube plugin:
@boomzilla said in Youtube plugin:
The only other source of text comes from the youtube video title and channel titles and I escaped the <s.
Wasn't there a video with a XSS title that was used as proof-of-concept on Dipshore?
I remember when I caused fa-spin by linking to a github repository.
-
@Lorne-Kates said in Youtube plugin:
Wasn't there a video with a XSS title that was used as proof-of-concept on Dipshore?
That was kind of my inspiration.
-
@XanderTheGamer said in Youtube plugin:
Why can't we embed html5 video?
If you click the icon in the composer, it tells you the list of allowed HTML tags and attributes.
-
@ben_lubar that's a Jeff level answer.
-
@ben_lubar said in Youtube plugin:
@XanderTheGamer said in Youtube plugin:
Why can't we embed html5 video?
If you click the icon in the composer, it tells you the list of allowed HTML tags and attributes.
and where is this button in the composer?
vanilla theme, no stylish , no greasemonkey, no shenanigans.
-
The grey-on-white bit in the composer, obviously. Look how much it jumps out. Look at the affordance
-
@accalia said in Youtube plugin:
@ben_lubar said in Youtube plugin:
@XanderTheGamer said in Youtube plugin:
Why can't we embed html5 video?
If you click the icon in the composer, it tells you the list of allowed HTML tags and attributes.
and where is this button in the composer?
vanilla theme, no stylish , no greasemonkey, no shenanigans.
It's in the compose textbox. Just to the right of the word COMPOSE. As it's all grey and tiny it took me a minute to find the damn thing even though Ben had said it was in there somewhere. How the heck is that discoverable?
-
@Jaloopa said in Youtube plugin:
The grey-on-white bit in the composer, obviously.
......
@Jaloopa said in Youtube plugin:
Look how much it jumps out.
it doesn't.
at all.
@Cursorkeys said in Youtube plugin:
How the heck is that discoverable?
it isn't.
at all.
-
-
-
It occurred to me that I'm relying on @ben_lubar's html cleaner being around to clean up the original youtube link. I'm not sure what happens if that isn't being used, so it probably needs to be tested to prevent xss stuff.
I'm escaping any
<s in the video title and channel titles, which text is never used as a tag attribute, just between opening and closing<a>tags. There is some parsing done for the supported parameters, which are put into a data attribute in a tag. Maybe that's enough.
-
So, after some initial testing, I think it's OK, with or without the html cleaner. Basically, the markdown plugin has an "allow html" setting with a big warning. That's what @ben_lubar's html cleaner plugin is meant to help with.
If you allow html but don't use that, you're already allowing crap, with or without the youtube plugin.
The youtube plugin won't convert anything if you try to inject stuff in there, though. So no additional xss seems possible AFAICT with the youtube plugin, since it's escaping problematic stuff coming from the youtube API.
Unless someone else has any other xss ideas about this.
-
@boomzilla should I use the server-side branch or is there another branch I should be using?
-
@ben_lubar The server-side branch is the branch. I guess I should go make a pull request and see if anything happens.
If not, I guess it'll become a real fork.
-
@boomzilla said in Youtube plugin:
a pull reques
Server side by boomzillawtf · Pull Request #30 · a5mith/nodebb-plugin-youtube-lite
These changes simplify the client handling. The API calls are all done on the server (and cached with an LRU to minimize quota usage) with graceful degradation without an API key. Above the video, ...
-
I blame you guys:
Don't convert invalid video URLs. Also don't crash when trying to pro… · boomzillawtf/nodebb-plugin-youtube-embed@09a6e7b
…cess what the youtube API returns.
-
@accalia It's not obvious? Why it's like 3 or even 4 full pixels tall! Might as well be a 90' billboard by NodeBB styling standards.
-
@boomzilla said in Youtube plugin:
html cleaner
I might be missing something-- but WHY?
What is wrong with htmlencode or htmlentities or whatever your language uses?
<h3><%= htmlencode(result("title")) %></h3>Who cares about stripping anything out? It'll get converted into
 or whatever the equiv is. Let there be <!
-
@Lorne-Kates said in Youtube plugin:
@boomzilla said in Youtube plugin:
html cleaner
I might be missing something-- but WHY?
What is wrong with htmlencode or htmlentities or whatever your language uses?
We don't want <script> but we do want <abbr>.
-
OK, so now I've added tests for invalid
videoIds, too. What other stuff should be tested here? Since you guys aren't likely to install this, can you suggest other things to throw at it to break it?
-
@ben_lubar said in Youtube plugin:
the post on the right in that screenshot would deal quite a bit of damage if you rammed into it at full speed.
That's really a bit big to be considered a stream, though.
-
@ben_lubar said in Youtube plugin:
@Lorne-Kates said in Youtube plugin:
@boomzilla said in Youtube plugin:
html cleaner
I might be missing something-- but WHY?
What is wrong with htmlencode or htmlentities or whatever your language uses?
We don't want <script> but we do want <abbr>.
Seems like something a whitelist should be for.
-
@Lorne-Kates said in Youtube plugin:
@ben_lubar said in Youtube plugin:
@Lorne-Kates said in Youtube plugin:
@boomzilla said in Youtube plugin:
html cleaner
I might be missing something-- but WHY?
What is wrong with htmlencode or htmlentities or whatever your language uses?
We don't want <script> but we do want <abbr>.
Seems like something a whitelist should be for.
Which is exactly what htmlcleaner is. It parses HTML, strips out un-whitelisted attributes and turns un-whitelisted tags into plain text, and then encodes it back as HTML.
-
@boomzilla said in 🔥 Computer says no guacamole:
@error said in 🔥 Computer says no guacamole:
Good to see we got that YouTube OneBox thing sorted.
Yeah...the regex doesn't handle the www.youtube.com style URL along with a time stamp. It's on my list of stuff to do...
Oh, actually, it's the
#that it wasn't catching.
-
Oops.
Fix error handler in googleapi call. · boomzillawtf/nodebb-plugin-youtube-embed@9c1ed97
Embed Youtube Videos on your NodeBB Forum. Contribute to boomzillawtf/nodebb-plugin-youtube-embed development by creating an account on GitHub.
Paging @ben_lubar for an update.
-
Since the author isn't actively maintaining the plugin any more, I went ahead and forked it:
npm: nodebb-plugin-youtube-embed
NodeBB Youtube Embed Plugin. Latest version: 0.6.9, last published: 3 years ago. Start using nodebb-plugin-youtube-embed in your project by running `npm i nodebb-plugin-youtube-embed`. There are no other projects in the npm registry using nodebb-plugin-youtube-embed.
YouTube Embedded Players and Player Parameters | YouTube IFrame Player API | Google for Developers
