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

filenames with spaces not supported #8

Closed
iamjochem opened this issue Apr 6, 2016 · 5 comments
Closed

filenames with spaces not supported #8

iamjochem opened this issue Apr 6, 2016 · 5 comments

Comments

@iamjochem
Copy link

hi there,

filenames with spaces cause your tool to break because filename arguments are passed to pdf2htmlEX command line without quotes.

see here: https://github.com/fagbokforlaget/pdftohtmljs/blob/master/lib/pdftohtml.js#L58

given the way the cmd-line string is simply concatenated I assume this also consistutes a potential cmd injection vector (here is an example discussion regarding this security issue), I believe the following article offers a partial solution:

I'm not sure whether using require('child_process').execFile() helps with the spaces in file paths, it might be worth considering some kind of package geared towards escaping shell arguments

P.S. this is a drive-by comment, I'm reporting something I found during testing but I ended up not using this module - I won't be creating a PR, please don't ask :-)

@iapain
Copy link
Member

iapain commented May 16, 2016

@iamjochem simply put your file name inside quotes.

@iamjochem
Copy link
Author

sure that works but I don't think it's a good idea to put the responsibility of proper [cmd argument] escaping on the consumer of this module (effectively the consumer should not know that functionality is implemented using exec ... that is a leaky abstraction, also using quotes does not mitigate the potential command injection vulnerability

@iapain
Copy link
Member

iapain commented May 17, 2016

Right! If time permits I'll implement proper command execution. Thanks again for reporting.

@iamjochem
Copy link
Author

cool - I reopened this issue so that it can remind you every now and again ;-)

@iamjochem iamjochem reopened this May 17, 2016
@iapain
Copy link
Member

iapain commented Apr 18, 2018

@iamjochem as of v0.5.0 we now use spawn which is Command Injection safe. Also, filenames with space should also work v0.5.0 onwards.

@iapain iapain closed this as completed Apr 18, 2018
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

No branches or pull requests

2 participants