occ command interprets leading dash values as keys

MichaIng commented 3 years ago

How to use GitHub

Please use the 👍 reaction to show that you are affected by the same issue.
Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
Subscribe to receive notifications on status change and new comments.

Steps to reproduce

Install current Nextcloud files

Run:

occ maintenance:install --no-interaction --database 'mysql' --database-name 'nextcloud' --database-user 'root' --database-pass '<password>' --admin-user 'admin' --admin-pass '-passwordWithLeadingDash' --data-dir '/path/to/ncdata'

Expected behaviour

The input argument that follows --admin-pass (or any other key that requires a value) is interpreted as value and not as another key.

Actual behaviour

The "--admin-pass" option requires a value.

Or if --database-name has the leading dash, as it does not require a value:

The "-p" option does not exist.

Server configuration

Operating system: irrelevant

Web server: irrelevant

Database: MariaDB v10.3+

PHP version: 7.0+

Nextcloud version: 21.0.0

Updated from an older Nextcloud/ownCloud or fresh install: fresh

Where did you install Nextcloud from: https://download.nextcloud.com/server/releases/latest.tar.bz2

Investigations

I couldn't find the getOption function definition, or where exactly the input is parsed and the error generated, so I'm not sure if this can be easily accomplished or not.
The options are added here: https://github.com/nextcloud/server/blob/master/core/Command/Maintenance/Install.php#L62-L77 They get a VALUE_REQUIRED or VALUE_OPTIONAL flag, which probably can be used to decide whether to take the next argument as value for that option or as new key.
Strange is that the --database-pass value is marked as optional which does not make much sense to me: Either the key is given, then a value must be present, or the key is not given, e.g. in case of SQLite where no password is required, or if an interactive input is wanted. Or what is the purpose when the key is given but no value?

Another thing I recognised which is slightly related: https://github.com/nextcloud/server/blob/master/core/Command/Maintenance/Install.php#L130-L146

First $dbPass = $input->getOption('database-pass'); is used to get the password.
Then the same is done again, if ($input->hasParameterOption('--database-pass')), hence if a value is given, or if the key itself is given?
I guess the second one can be removed (as it has no effect) while the first one is required in any case to not have an undeclared variable?

szaimen commented 3 years ago

cc @nextcloud/server-triage

artonge commented 3 years ago

It is also a problem with certificates that begins with -----BEGIN CERTIFICATE----- like in this use case:

php occ config:app:set user_saml idp-x509cert --value " -----BEGIN CERTIFICATE----- ......"

The only workaround I found is to prepend a space before the argument like in my example. I the case of OP, this would give something like --admin-pass ' -passwordWithLeadingDash', but I am not sure it would work as intended as the space might be interpreted as a password's character...

melato commented 2 years ago

Anyone who uses "occ maintenance:install" with generated passwords that might have "-" will easily run into this bug, as I have. The workaround is to generate passwords that don't begin with a dash, so I removed dashes from my password-generating code.

I'm using: Nextcloud version: 23.0.3 PHP 7.4.28

blizzz commented 2 years ago

this is normal/common behaviour. leading dashes have to be escaped.

MichaIng commented 2 years ago

Escaping is done via slash?

blizzz commented 2 years ago

yes, for example:

$ ack '\--headless' --type=php lib/private/
lib/private/Preview/Office.php
56:             $defaultParameters = ' -env:UserInstallation=file://' . escapeshellarg($tmpDir . '/owncloud-' . \OC_Util::getInstanceId() . '/') . ' --headless --nologo --nofirststartwizard --invisible --norestore --convert-to png --outdir ';

$ ack '--headless' --type=php lib/private/
Unknown option: headless
ack: Invalid option on command line

melato commented 2 years ago

In the example you gave, the argument --headless needs to be escaped because ack does not know if it should expect an option or a program argument at that point, since both are expected there. Another common mechanism is to put "--" in the command line to indicate that any arguments following the dashes are not option names but program arguments. In cases of options that expect arguments, the expected behavior is to allow option arguments with leading dashes, without escaping. Examples:

sort -t - a.txt
grep -e --headless a.txt
grep -- --headless a.txt

"man grep" even documents for the -e option that: This option can be used to protect a pattern beginning with “-”.

melato commented 2 years ago

The solution is to use the --admin-pass="password" version of the options, as documented in occ help maintenance:install

Usage:
  maintenance:install [options]

Options:
      --database=DATABASE                            Supported database type [default: "sqlite"]
      --database-name=DATABASE-NAME                  Name of the database
      --database-host=DATABASE-HOST                  Hostname of the database [default: "localhost"]
      --database-port=DATABASE-PORT                  Port the database is listening on
      --database-user=DATABASE-USER                  User name to connect to the database
      --database-pass[=DATABASE-PASS]                Password of the database user
      --database-table-space[=DATABASE-TABLE-SPACE]  Table space of the database (oci only)
      --admin-user=ADMIN-USER                        User name of the admin account [default: "admin"]
      --admin-pass=ADMIN-PASS                        Password of the admin account
      --admin-email[=ADMIN-EMAIL]                    E-Mail of the admin account
      --data-dir=DATA-DIR                            Path to data directory

With this usage, there is no problem with leading dashes in passwords or any other option arguments.

However, the manual incorrectly uses the defective version of the options:

$ cd /var/www/nextcloud/
$ sudo -u www-data php occ  maintenance:install --database \
"mysql" --database-name "nextcloud"  --database-user "root" --database-pass \
"password" --admin-user "admin" --admin-pass "password"
Nextcloud is not installed - only a limited number of commands are available
Nextcloud was successfully installed

I suggest not changing the code, but changing the manual to use the --option=value version of the command line.

MichaIng commented 2 years ago

Ah wow, that actually makes sense, as usually the long from of CLI options uses equal sign for the value while short forms use spaces, even that parsers are usually relaxed about that.

Makes sense to adjust the docs then and keep it as common/known limitation of space-separated key-value pairs for CLI in general.

MichaIng commented 1 year ago

PR up for the documentation change: https://github.com/nextcloud/documentation/pull/9571

If I'm not mistaken, the error output of the occ command needs to be adjusted as well.

MichaIng commented 1 year ago

Hmm, generally in case of an argument error, the command is shown with --option value syntax, while help outputs show it with --option=value syntax. This is inconsistent, and the latter should be shown in all cases:

# ncc config:app:get -J

  The "-J" option does not exist.

config:app:get [--output [OUTPUT]] [--default-value [DEFAULT-VALUE]] [--] <app> <name>

# ncc help config:app:get
Description:
  Get an app config value

Usage:
  config:app:get [options] [--] <app> <name>

Arguments:
  app                                  Name of the app
  name                                 Name of the config to get

Options:
      --output[=OUTPUT]                Output format (plain, json or json_pretty, default is plain) [default: "plain"]
      --default-value[=DEFAULT-VALUE]  If no default value is set and the config does not exist, the command will exit with 1
  -h, --help                           Display this help message
  -q, --quiet                          Do not output any message
  -V, --version                        Display this application version
      --ansi                           Force ANSI output
      --no-ansi                        Disable ANSI output
  -n, --no-interaction                 Do not ask any interactive question
      --no-warnings                    Skip global warnings, show command output only
  -v|vv|vvv, --verbose                 Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug

Funnily, this particular error is exactly caused by the printed syntax, while with = the output would be actually helpful and solve the issue 😄.

I just couldn't find it in Nextcloud code, so I guess this is part of Symfony console command component? At least the option does not exist. error is: https://github.com/symfony/console/search?q=option+does+not+exist&type=code

MichaIng commented 1 year ago

Reopened as of above inconsistency with option does not exist output.

nextcloud / server