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

Ideas for addArg #46

Open
cdp1337 opened this issue Dec 31, 2019 · 8 comments
Open

Ideas for addArg #46

cdp1337 opened this issue Dec 31, 2019 · 8 comments

Comments

@cdp1337
Copy link

cdp1337 commented Dec 31, 2019

It seems that addArg is doing more than it should, which is causing the code to be a little complicated. In my use case, I'm doing one of a few things, which vary greatly on how the logic needs to behave:

  • Basic key/value pairs - --file, $tarball
  • Arguments only, no values - --extract
  • Values only - $filename
  • Crazy redirects - -O > $destination

(If it wasn't obvious, my test case here is tar.) Since there's a lot going on, but not a lot of flexibility, would it make sense to have a few different methods for addArg, for various use cases.

As a list critique point, passing in '--file=', $tarball looks weird, I feel moving the option for a separator to an argument could make more sense.

Either way, I'm going to monkey patch a local copy I grabbed for a project I'm working on, and can submit my results if you're interested. Otherwise great looking class, the convenience of it handling the necessary proc_* operations is useful.

@cdp1337
Copy link
Author

cdp1337 commented Dec 31, 2019

For example:

/**
	 * Add a simple key to the argument list
	 *
	 * KEY IS NOT ESCAPED!
	 *
	 * @param string $key
	 *
	 * @return Command
	 */
    public function addArgKey(string $key) : self {
		$this->_args[] = $key;

		// Allow chaining
		return $this;
	}

	/**
	 * Add a simple value to the argument list
	 *
	 * This value is assumed to be untrusted, so it will be escaped.
	 *
	 * @param string|string[] $value
	 *
	 * @return Command
	 */
	public function addArgValue($value) : self {
		$useLocale = $this->locale !== null;

		if($useLocale) {
			$locale = setlocale(LC_CTYPE, 0);   // Returns current locale setting
			setlocale(LC_CTYPE, $this->locale);
		}

		if(is_array($value)) {
			$this->_args[] = implode(' ', array_map('escapeshellarg', $value));
		}
		else {
			$this->_args[] = escapeshellarg($value);
		}

		if($useLocale) {
			setlocale(LC_CTYPE, $locale);
		}

		// Allow chaining
		return $this;
	}

	/**
	 * Add a key/value pair to the argument list
	 *
	 * The key is assumed to be trusted, so it is NOT escaped,
	 * while the value is untrusted, and is escaped.
	 *
	 * @param string $key
	 * @param string|string[] $value
	 * @param string $seperator
	 * 
	 * @return Command
	 */
	public function addArgKeyValue(string $key, $value, string $seperator = '=') : self {
		$useLocale = $this->locale !== null;

		if($useLocale) {
			$locale = setlocale(LC_CTYPE, 0);   // Returns current locale setting
			setlocale(LC_CTYPE, $this->locale);
		}

		if(is_array($value)) {
			$this->_args[] = $key . $seperator . implode(' ', array_map('escapeshellarg', $value));
		}
		else {
			$this->_args[] = $key . $seperator . escapeshellarg($value);
		}

		if($useLocale) {
			setlocale(LC_CTYPE, $locale);
		}

		// Allow chaining
		return $this;
	}

Also yes, I only need to worry about supporting PHP 7.x and newer, so I prefer to use typecasting when applicable.

** edit - Separator is not spelled with an "e"... I'm a sysadmin, not a speller.

@cdp1337
Copy link
Author

cdp1337 commented Dec 31, 2019

$cmd = new Command('tar');
$cmd->addArgKey('--' . $this->_compressionFlag);
$cmd->addArgKeyValue('--file', $this->_file->getLocalFilename());
$cmd->addArgKey('--list');

// to extract a single file, this is NOT to be confused with --extract=..., 
// because extract and the list of files are separate commands with tar
$cmd->addArgKey('--extract');
$cmd->addArgValue($filename);

// or for redirecting stdout to a file
$cmd->addArgKey('-O > ' . escapeshellarg($destination));
tar --gzip --file='/tmp/localhost-web/package/content-3.1.0.tar.gz' --list
tar --gzip --file='/tmp/localhost-web/package/content-3.1.0.tar.gz' --extract 'components/content/component.xml' -O
tar --gzip --file='/tmp/localhost-web/package/content-3.1.0.tar.gz' --extract 'components/content/CHANGELOG' -O
tar --gzip --file='/tmp/localhost-web/package/content-3.1.0.tar.gz' --extract 'components/content/assets/images/logos/content.png' -O > '/home/charlie/public_html/coreplus-full/files/public/packagerepo-logos/component-content-3.1.0/content.png'

Example command results with this idea. The stdout redirect was the only one where I had to do manual work outside the standard workflow, as the class doesn't seem to support redirecting stdout to a file, and I don't want the webserver to attempt to load a 2GB database dump into memory, (for example).

@cdp1337
Copy link
Author

cdp1337 commented Jan 1, 2020

^^ cdp1337@a952a78

@mikehaertl
Copy link
Owner

@cdp1337 Thanks for your suggestions.

Let me first admit, that the interface of this library is quite flawed: I had the misconception that somehow --some-option=bla can be split into 2 pieces, key and value, and only the value must be escaped. This lead to a security issue as described in #44 because the key was not escaped. This is fixed, but now this interface no longer makes sense (and it probably never really did).

I actually think what we should really do is to remove this separation of key and value and treat any argument as a single string and leave it up to the user what this string represents. And we should by default escape the complete argument because it doesn't matter if you escape the complete argument:

  • --file='/some/path' is equivalent to
  • '--file=/some/path'

So instead of adding even more methods to deal with this mess, what do you think about a simplified interface like this?

$cmd->addArg((string) $arg, $escape = true);
$cmd->addArgs((array) $args, $escape = true);

In your case you could then use it like:

$cmd = new Command('tar');
$cmd->addArgs([
    '--gzip',
    '--file=' . $filename,
]); // properly escaped
$cmd->addArg('-0 > ', false); // not escaped
$cmd->addArg($destination); // properly escaped

This would of course break BC, so I'd create a 2.0.0 release.

@Kirill89 CC you here as you brought up the issue. Would you agree that this is a cleaner approach? IMO it leaves no questions about what's happening. And maybe you can confirm that '--x=y' is the same as --x='y' when passing shell args.

@Kirill89
Copy link
Contributor

Kirill89 commented Jan 3, 2020

@mikehaertl, yes as I understand, '--x=y' is the same as --x='y' and the same as '--x'='y'. Personally, I like the approach you suggested. Looks like it covers all cases listed by @cdp1337:

// tar '--gzip' '--file=/content-3.1.0.tar.gz' '--list'
$cmd->addArgs([
    '--gzip',
    '--file=/content-3.1.0.tar.gz',
    '--list',
]); 
// tar '--gzip' '--file=/content-3.1.0.tar.gz' '--extract' 'component.xml' '-O'
$cmd->addArgs([
    '--gzip',
    '--file=/content-3.1.0.tar.gz',
    '--extract',
    'component.xml',
    '-O'
]); 
// tar '--gzip' '--file=/content-3.1.0.tar.gz' '--extract' 'component.xml' '-O' > '/component.xml'
$cmd->addArgs([
    '--gzip',
    '--file=/content-3.1.0.tar.gz',
    '--extract',
    'component.xml',
    '-O'
]);
$cmd->addArg('>', false);
$cmd->addArg('/component.xml');

@Kirill89
Copy link
Contributor

Kirill89 commented Jan 3, 2020

BTW you don't even need two functions (addArgs and addArg). You can check if first argument of addArg is an array.

@mikehaertl
Copy link
Owner

mikehaertl commented Jan 3, 2020

Yes, I also considered that but thought it would be cleaner to have 2 methods to really leave no magic. But as you also bring this up I think we could also go with 1 method only.

I'd also clean up the constructor:

 // Passed to escapeshellcmd() by default
$cmd = new Command('/some/cmd --option=' . $_POST['option']);

// Disable escaping
$cmd = new Command('/some/cmd > /other/cmd', false);

@cdp1337
Copy link
Author

cdp1337 commented Jan 3, 2020

Escaping the key just seems weird to me, but I guess if it works.

Also I concur with your thoughts on 2 methods; allows for

addArg(string $argument, bool $escape = true)
addArgs(array $arguments, bool $escape = true)

(Assuming you want to enforce support for PHP 7.0 and newer.)

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

3 participants