sharpchi / moodle-filter_syntaxhighlighter

14 stars 11 forks source link

New feature:Fetch code from a repository #5

Closed cuococarlos closed 3 years ago

cuococarlos commented 5 years ago

We have developed a new feature.

Motivation: We need to fetch code from a repository and then apply the syntax highlighter filter.

How does it work: We added a validation to check if the given string has an URL format, if so, we do a GET call and replace the string with the response.

smarbos commented 5 years ago

Works great!

sharpchi commented 5 years ago

Hey guys, thanks for your idea and code. I have a couple of issues with it as it stands.

It fetches any url anywhere

If I had some html with an image tag, what would happen? e.g. <img src="http://www.mysite.com/image.png"/>

It downloads anything

Your issue item talks about downloading from a repository, but there's nothing in the code that guarantees or limits sources. I'd hate for the plugin to become an attack opportunity.

If you could address those two issues and some example tests to check that they've been addressed, that would be great. Many thanks Mark

cuococarlos commented 5 years ago

Ok @sharpchi , We will work in these issues and come back to you later.

cuococarlos commented 5 years ago

@sharpchi We did these upgrades regarding your comments: 1) Added a check before fetching, ensuring that it is only a valid URL, without HTML tags or random text. 2) Added a checkbox in the plugin configuration to enable "external sources". This feature is disabled by default and will only fetch from Github or Gitlab repos.

The webtool used to test the regex was: https://www.phpliveregex.com/#tab-preg-match The regex used is the one proposed by @imme_emosl in this url:https://mathiasbynens.be/demo/url-regex, wich looks like the best option for us.

Note: The accepted url without "external sources" enabled are :

Greetings ! :smiley:

smarbos commented 5 years ago

@sharpchi what do you think? Would you like to merge this changes? Les us know if you thing another changes are needed. We understand that maybe you don't want to include this functionality and in that case we were thinking of releasing our code as a fork of your project. Let us know what you think. Regards, Damian.

sharpchi commented 3 years ago

Closing this as an alternative method to do this was suggested.

smarbos commented 3 years ago

Closing this as an alternative method to do this was suggested.

Hi @sharpchi! What is the alternative method?

sharpchi commented 3 years ago

@smarbos You put in a request to Moodle to accept your version of the plugin, they didn't want two near identical plugins, and I didn't want my plugin fetching data, so the suggestion was that you write a filter that fetches the data, then pass the results to my filter. That way you get to control the fetching, and my plugin manages the display. I can't find the comments in tracker.moodle.org right now. But those were the comments that were made.