Add a root scope protection option to increase module security.

[riegelTech]

Using this awesome module, I was able to leak a system file using the inclusion tag inside a MD file. I consider this as a security breach. However it's not a big deal to offer a security option to user.

I choose to add an option, disabled by default to not change deployed modules at update, but this can be challenged, backward compatibility vs security...

[coveralls]

Coverage remained the same at 100.0% when pulling 56c7ee1f3dd1eb9e587ffdf9a536058a6a87158b on riegelTech:feature/addRootScopeProtectionOption into def47e99e9a5a37b080ab210d6a853af61266362 on camelaissani:master.

[camelaissani]

Thank you for the contribution! I'm looking at your PR.

[camelaissani]

What do you think about using the rootdir also as the root scope and not introducing the rootScope option? IMO, these two options are redundant. However, I agree we should keep the rootScopeProtection option to activate/deactivate the checks.

Also, I think we should release a new major version of this plugin to have the rootScopeProtection set to true by default. The plugin should behave by default without vulnerabilities. What do you think?

[riegelTech]

I was afraid that keeping the only option rootdir will degrade the module flexibility, for example when I want to include a markdown that is in the parent directory of the first one that use the inclusion marker. Another way to solve this problem is to let the choice to define the option value of rootScope, but when not defined, use rootdir anyway to keep user safe. What's your opinion ?

However your proposal of a new version is reasonable and shows that you're really concerned by security problems, it sounds good to me !

[camelaissani]

The rootdir will stay flexible if the rootScopeProtection is set to false, won't it? Then all files above the rootdir might be accessible, right?

On the contrary, when the rootScopeProtection is set to true, then files above of the rootdir won't be accessible. Only files below the rootdir will be.

Am I miss something?

Also I see another check that could be added. What do you think to control the extension of the file to include?. Should we only accept .md extension? Or at least, give the possibility to the user to specify the allowed extensions via another option.

[riegelTech]

Hello, I'm facing a case where users can render markdowns that they drop anywhere inside a root folder that I know. So it is possible to have a markdown file in root/some/dir/some-md.md that includes a MD file that is in root/some/other-md.md .

In this case, rootdir option means something else than rootscope. I know this seems redundant but I love so much flexibility. That lead me to not restraint the files extension, because (if I'm right) you can include some HTML markup inside MD format, and you should do someting like that (not tested, but so powerful):

```JSON
!!!include(./myJson.json)!!!

Sorry, it is hard to escape such syntax, but I imagine you have the trick: include raw text content inside code blocks.

[camelaissani]

I am not sure I have fully understood your previous comment.

Let me explain my idea through two examples:

First set of options

{
  root: '/home/mdDocs',
  rootScopeProtection: false,
}

In this case there are not any restrictions on the paths (as today). The paths below are accepted:

/home/mdDocs/a.md
./a.md
../../var/xxxx.txt  (corresonding to /var/xxxx.txt)

No check done on the extension files as the allowedExtensions option is not set.

Second set of options:

{
  root: '/home/mdDocs',
  rootScopeProtection: true,
  allowedExtensions: ['.md', '.txt']
}

In this case only paths under the root dir are accepted:

./a.md
./L1/L2/a2.md

and the paths above the root dir or referencing another different root dir are not allowed:

/home/b.md
/a.md
../../var/xxxx.txt  (corresonding to /var/xxxx.txt)

As the allowedExtensions option is set, it will reject any files that don't match the extension list. The paths below are not allowed:

/home/mdDocs/a.conf
./script.sh

From these examples, I still don't see a need to add an additional rootscope option. I feel also that the plugin users might be confused by two options setting the same information: the root directory.

[riegelTech]

I totally understood your point, I just want to warn you about a case, in which you would provide security, but you are not able to predict where is the first document in the tree of the directories (see my example above).

So to mix security with flexibility you should support both of the two options. I propose a deal: next reply from you will definetly close the debate, as you are the one that have the final word 😉. Maybe I will be not fully satisfied with that but I think this is fair to accept your decisions.

About the extension restrictions, I have to reconsider my opinion: your last example seems very cool and easy to configure, nice proposal !

[camelaissani]

Could you elaborate your example? I really want to understand before taking any decision and be sure I'm not missing an important use case.

[riegelTech]

Hello,

To be clear, just start from my use case. Basically my application allows users to clone GIT repositories in the server's directory in order to visualize markdown files, in the form of my_application/repositories/a_cloned_repo/. Everything inside a_cloned_repo comes from the user, and the way he decides to organize its files.

So it is possible to get this kind organization:

• the templates (the files he wants to be included) are in my_application/repositories/a_cloned_repo/md_files/templates/
• the main files (the files he wants to include others) are in my_application/repositories/a_cloned_repo/md_files/main/

Now suppose a MD file that is filed under my_application/repositories/a_cloned_repo/md_files/main/first_page.md and includes a template that is filed under my_application/repositories/a_cloned_repo/md_files/templates/header.md.

The inclusion syntax of first_page.md should be ̀!!!include(../templates/header.md)!!!` .

First case, with a set of options like:

{
  root: 'my_application/repositories/a_cloned_repo/md_files/main',
  rootScopeProtection: true,
  allowedExtensions: ['.md', '.txt']
}

I have security but it will not allow to include my template file, as it is above rootdir

Second case, with a set of options like:

{
  root: 'my_application/repositories/a_cloned_repo/md_files/main',
  rootScopeProtection: false,
  allowedExtensions: ['.md', '.txt']
}

I cannot provide security, even if I'm sure that no file should be included above my_application/repositories/a_cloned_repo/

So to catch my case, the only way is to support the two options rootdir and rootScope.