Skip to content

Add support for "absolute" root syntax#23

Open
m4dz wants to merge 1 commit into
camelaissani:masterfrom
m4dz:absolute-path-support
Open

Add support for "absolute" root syntax#23
m4dz wants to merge 1 commit into
camelaissani:masterfrom
m4dz:absolute-path-support

Conversation

@m4dz
Copy link
Copy Markdown

@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
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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