Page MenuHomePhabricator
Create Task
Maniphest T11047

Entering wrong or empty paths renders configtool unusable
Open, WishlistPublic
Actions

Description

I am currently testing phabricator and found out that it's possible to enter config-values that break the config script.

Steps to reproduce:

  • Install Sprint-extension as described here: https://github.com/wikimedia/phabricator-extensions-Sprint
  • Try to temporarily deactivate sprint extension by giving it an empty path: ./config set load-libraries '{"sprint":""}'
  • Now try to set any config variable e.g.: ./config set maniphest.points '{"enabled":false}'
  • The tool will throw an Exception:

[2016-05-27 08:54:41] EXCEPTION: (PhutilBootloaderException) Include of '/__phutil_library_init__.php' failed! at [<phutil>/src/moduleutils/PhutilBootloader.php:209]
arcanist(head=master, ref.master=2234c8cacc21), phabricator(head=master, ref.master=5b77b86ffbf9), phutil(head=master, ref.master=5eaf0a9f5a35)

#0 PhutilBootloader::loadLibrary(string) called at [<phutil>/src/moduleutils/core.php:12]
#1 phutil_load_library(string) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:202]
#2 PhabricatorEnv::buildConfigurationSourceStack() called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:95]
#3 PhabricatorEnv::initializeCommonEnvironment() called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:75]
#4 PhabricatorEnv::initializeScriptEnvironment() called at [<phabricator>/scripts/__init_script__.php:21]
#5 init_phabricator_script() called at [<phabricator>/scripts/__init_script__.php:24]
#6 require_once(string) called at [<phabricator>/scripts/setup/manage_config.php:5]

From a users perspective, I think, that a wrong config-value should never break the config-tool itself. I should always be able to undo whatever my incompetence leads me to.
From a developers perspective I see, that the backlog does not pierce the Sprint-Extension's code, so I assume this could be a general Problem. (I think it's not even neccessary to install the Sprint extension in the first place. Added the step for the sake of completeness)

Related Objects

Event Timeline

• grindhold created this task.May 27 2016, 8:58 AM
Herald added a subscriber: qgil. · View Herald TranscriptMay 27 2016, 8:58 AM
• grindhold updated the task description. (Show Details)May 27 2016, 9:01 AM
hach-que added a subscriber: hach-que.May 27 2016, 11:56 AM

Extensions are a really advanced feature, and you will get almost no support from upstream here if you have them installed. Or another way, anyone who is installing extensions should know Phabricator well enough to know how to fix this issue manually (hand editing local.json).

epriestley triaged this task as Wishlist priority.May 27 2016, 12:29 PM
epriestley added a subscriber: epriestley.

This needs to be the default behavior. For example, if you update lib-important-security/ and the new version has a bug which causes it to fatal, Phabricator must also fatal. It is not acceptable to just skip the extension and start without loading a library that may have important security/policy code an install could rely on, because this could make the install vulnerable to security or policy threats which you expect to be protected from.

Even in the narrow case of bin/config, I think we can't skip extensions because there are so many things they may be changing at very low levels that we can't trust that program behavior is correct/expected if we skip one.

In arc, we react to this class of errors by giving the user better instructions for fixing it. We could do that here, as well.

We could also proactively test values of this configuration setting before allowing them to be written, to make sure that the next operation won't immediately fail. But all bets are off if you configure Phabricator with a working library, then break the library (perhaps by updating it).

As @hach-que mentions, you can manually edit phabricator/conf/local/local.json to remove the reference to this library.

qgil removed a subscriber: qgil.May 30 2016, 11:21 AM
• grindhold added a comment.Jun 8 2016, 3:39 PM

maybe it would be an option to at least identify lethal arguments for commands and warn the user if he enters some. like rm does when typing rm -rf / without --no-preserve-root.

epriestley added a comment.Jun 8 2016, 3:41 PM
In T11047#179388, @grindhold wrote:

maybe it would be an option to at least identify lethal arguments for commands and warn the user if he enters some. like rm does when typing rm -rf / without --no-preserve-root.

Yes, that's what I meant by this:

We could also proactively test values of this configuration setting before allowing them to be written, to make sure that the next operation won't immediately fail.

chad added a project: Config.Jun 8 2016, 4:37 PM
Phabricator · Built by Phacility