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

occ command interprets leading dash values as keys #26109

Open
MichaIng opened this issue Mar 14, 2021 · 12 comments · Fixed by nextcloud/documentation#9571
Open

occ command interprets leading dash values as keys #26109

MichaIng opened this issue Mar 14, 2021 · 12 comments · Fixed by nextcloud/documentation#9571
Assignees
Labels
1. to develop Accepted and waiting to be taken care of enhancement feature: occ

Comments

@MichaIng
Copy link
Member

MichaIng commented Mar 14, 2021

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

  1. Install current Nextcloud files
  2. 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
Copy link
Contributor

szaimen commented Jun 25, 2021

cc @nextcloud/server-triage

@artonge
Copy link
Contributor

artonge commented Jun 28, 2021

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...

@szaimen szaimen added 1. to develop Accepted and waiting to be taken care of feature: occ and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap needs info labels Jun 29, 2021
@melato
Copy link

melato commented Apr 14, 2022

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
Copy link
Member

blizzz commented Apr 14, 2022

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

@MichaIng
Copy link
Member Author

Escaping is done via slash?

@blizzz
Copy link
Member

blizzz commented Apr 14, 2022

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
Copy link

melato commented Apr 15, 2022

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
Copy link

melato commented Apr 15, 2022

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
Copy link
Member Author

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 added a commit to nextcloud/documentation that referenced this issue Jan 20, 2023
The `--option=value` version of passing options to the `occ maintenance:install` command is now used in the documentation, instead of the `--option value` variant. This  solves issues with leading dashes in values, especially passwords. It also matches the `occ help maintenance:install` output.

Solves:
- nextcloud/server#26109
- #8190

Furthermore values are now single quoted, which is important to avoid variable expansion and special treatment of the backlash character in random passwords, and a doubled space was removed.

Signed-off-by: MichaIng <micha@dietpi.com>
@MichaIng
Copy link
Member Author

PR up for the documentation change: nextcloud/documentation#9571

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

@MichaIng
Copy link
Member Author

MichaIng commented Jan 20, 2023

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 MichaIng self-assigned this Jan 20, 2023
@MichaIng MichaIng added enhancement 3. to review Waiting for reviews and removed bug 1. to develop Accepted and waiting to be taken care of labels Jan 20, 2023
MichaIng added a commit to nextcloud/documentation that referenced this issue Mar 7, 2023
The `--option=value` version of passing options to the `occ maintenance:install` command is now used in the documentation, instead of the `--option value` variant. This  solves issues with leading dashes in values, especially passwords. It also matches the `occ help maintenance:install` output.

Solves:
- nextcloud/server#26109
- #8190

Furthermore values are now single quoted, which is important to avoid variable expansion and special treatment of the backlash character in random passwords, and a doubled space was removed.

Signed-off-by: MichaIng <micha@dietpi.com>
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this issue Apr 3, 2023
The `--option=value` version of passing options to the `occ maintenance:install` command is now used in the documentation, instead of the `--option value` variant. This  solves issues with leading dashes in values, especially passwords. It also matches the `occ help maintenance:install` output.

Solves:
- nextcloud/server#26109
- #8190

Furthermore values are now single quoted, which is important to avoid variable expansion and special treatment of the backlash character in random passwords, and a doubled space was removed.

Signed-off-by: MichaIng <micha@dietpi.com>
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this issue Apr 3, 2023
The `--option=value` version of passing options to the `occ maintenance:install` command is now used in the documentation, instead of the `--option value` variant. This  solves issues with leading dashes in values, especially passwords. It also matches the `occ help maintenance:install` output.

Solves:
- nextcloud/server#26109
- #8190

Furthermore values are now single quoted, which is important to avoid variable expansion and special treatment of the backlash character in random passwords, and a doubled space was removed.

Signed-off-by: MichaIng <micha@dietpi.com>
@MichaIng MichaIng reopened this Apr 3, 2023
@MichaIng MichaIng added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels Apr 3, 2023
@MichaIng
Copy link
Member Author

MichaIng commented Apr 3, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of enhancement feature: occ
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants