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

Minor fixes #1

Merged
merged 3 commits into from
Sep 23, 2019
Merged

Minor fixes #1

merged 3 commits into from
Sep 23, 2019

Conversation

NickeZ
Copy link
Contributor

@NickeZ NickeZ commented Sep 19, 2019

Hey, I updated the dependencies, added support for images and changed a few minor things.

@NickeZ NickeZ force-pushed the minor-fixes branch 3 times, most recently from 42d97ad to 6a42844 Compare September 20, 2019 09:06
@NickeZ NickeZ force-pushed the minor-fixes branch 2 times, most recently from 0b798c8 to 6805625 Compare September 20, 2019 09:23
@razorheadfx
Copy link
Owner

Thank you for the PR! Sorry it took me so long to respond.

I'm a little conflicted on serving the working directory by default.
Pro:

  • Pretty handy for serving other content images/etc
  • No need to run an additional server (like https for other static stuff

Con:

  • Users can now unknowingly expose directories they do not wish to serve i.e. when starting with the --host <ip here> option.
cd /home/my_user_name
grup my_md_files/example.md
# now everyone can read everything in your home directory

Not sure how bad it could be since we're not serving a directory listing in index.html

How would you feel about an additional flag enabling that behaviour and a warning if using it with a non-localhost address?

@NickeZ
Copy link
Contributor Author

NickeZ commented Sep 23, 2019

Hey thanks for the comments!

I totally agree with your sentiment that serving arbitrary files are bad. Perhaps a better implementation would be to ignore the built-in support in simple_server and only serve files relative to the md-file and perhaps have a whitelist of file extensions?

I think the current implementation in this PR is even broken. If you launch grup like your example it probably won't find images since the paths would be wrong...

Unfortunately I don't see myself continuing work on this PR since I already worked a lot on my async/await branch. So feel free to close it.

@razorheadfx razorheadfx merged commit 1d580b2 into razorheadfx:master Sep 23, 2019
@NickeZ NickeZ deleted the minor-fixes branch September 23, 2019 19:19
@razorheadfx
Copy link
Owner

Thank you for pitching in!

I totally agree with your sentiment that serving arbitrary files are bad. Perhaps a better implementation would be to ignore the built-in support in simple_server and only serve files relative to the md-file and perhaps have a whitelist of file extensions?

I like your the whitelisting approach for image files.
We could also approach it by identifying the images paths in the markdown AST, and specifically whitelist those, but the AST provided by comrak is pretty painful to work with (at least it was the last time i tried to do fancy stuff with it).

I think the current implementation in this PR is even broken. If you launch grup like your example it probably won't find images since the paths would be wrong...
Yeah i'll sort that out

Unfortunately I don't see myself continuing work on this PR since I already worked a lot on my async/await branch. So feel free to close it.
I'm gonna make 0.2.1 release after fixing #4 and then we can use your async-await impl for the 0.3.0 once async-await becomes stable.
Sound good?

@NickeZ
Copy link
Contributor Author

NickeZ commented Sep 23, 2019

Sound good?

Yeah, it seems like async/await is coming in November. I'll port anything you do on the master branch to my async-await branch.

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.

None yet

2 participants