Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for "absolute" root syntax #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

m4dz
Copy link

@m4dz m4dz commented Jun 20, 2021

This PR introduces support with the "absolute" path syntax:

  • if the include path is specified in a relative syntax (e.g. ./L1/foo.md) or without any leading path (e.g. L1/foo.md), then the inclusion occurs the same way as yet
  • if the include path has a leading slash (e.g. /foo.md), the inclusion path is computed from the root path as declared in options.root

It allows to include files without having to know where the included files is located relatively to the parent file.

It may fix issue #13 by referencing the included file from the root directory by using this "absolute root" syntax.

@riegelTech
Copy link

@m4dz I think I heard you at Snow Camp, nice to comment your PR ;-)
However, as me mentionned in this PR #22 using absolute paths can be a huge security problem because a smart markdown writer can leak some system conf files (if the server isn' t properly configured).

So without challenging the value of your proposal, be sure that anyone that uses this trick will deeply understand what risk he takes.
Anyway, I let @camelaissani decide what is better for his module ;-)

@camelaissani
Copy link
Owner

@m4dz thank you for the contribution.

Actually I'm considering to forbid to include files with path starting by a /. As written by @riegelTech it might open a door to leak some files which are not intended to be included in a markdown. Using a / in the include path is a way to get rid of the rootdir and point anywhere in the system.

However your PR is interesting as it brings the support of relative paths which have not specified ./ at the begining (L1/L2/r.md with your code becomes ./L1/L2/r.md)

About issue #13, I'm not sure this PR will help. From what I understood what it is expected there is to change somehow the img url accordingly to the include path. IMO, it is not something that can be tackled in this plugin.

WDYT about modifying your PR to keep only the fix you made on the relative path?

@m4dz
Copy link
Author

m4dz commented Jun 28, 2021

Thanks for your feedback all!

I probably misdescribed this feature, so let me bring some light to it 😃

Currently, when you include a file with a path starting with a /, the plugin follows the unix convention and looks to the file from the filesystem root.

With this PR, any include path starting with a / is looked from the rootdir rather than the root of the filesystem. To me, it allows different things:

  1. being able to include files anywhere from the rootdir, without having to think on complex relative paths depending on the filesystem structure inside the rootdir, and independently where your original file is located (i.e. include /my/partials/file.md will always include the file located to [rootdir]/my/partials/file.md regardless the location of the file querying for inclusion)
  2. propose an alternative fix to Add a root scope protection option to increase module security. #22 as any file starting with a / is now looked from rootdir and not from /

(sorry for my misreference to #13, this PR is unrelated)

We can see it as a chroot system for the inclusion file lookup 🤩

Does it makes more sense? Or did I missed something in your reviews?

Thanks!

@m4dz
Copy link
Author

m4dz commented Jul 26, 2021

Hey @camelaissani, any news about this one? Tell me if you need some feedback from me 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants