In the Config class, Config::$name is supposed to be restricted in length and character set. But this is never checked until you try to save() the object, at which point it's too late to do anything about it.

This means it's possible to create and use a Config object with an invalid name. So for example getName() would return the invalid name, and setName() could be used to set a new invalid name. Any code using this object would be dealing with an invalid name. And the only place the name is checked is when you save() the object, which may be way downstream from where the Config was created.

A better way to do this would be to ensure that Config::$name always contains a valid value - we should never be able to construct a Config object with an invalid name, and we should never be able to setName() to an invalid name. In other words, validateName() should be called in the constructor and in setName(), not in save().

This can be fixed with a few lines of code. I also added some documentation comments to explain what constitutes a valid name, because currently that's only described in the code.

Files: 
CommentFileSizeAuthor
#29 config-validate-name-2150803-29.patch7.6 KBwillzyx
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch config-validate-name-2150803-29.patch. Unable to apply patch. See the log in the details link for more information. View
#29 interdiff-26-29.txt626 byteswillzyx
#22 interdiff-19-22.txt2.98 KBwillzyx
#19 config-validate-name-2150803-19.patch2.32 KBdeepakaryan1988
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,949 pass(es), 4 fail(s), and 0 exception(s). View
#16 config-validate-name-15.patch2.32 KBashutoshsngh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,949 pass(es), 4 fail(s), and 0 exception(s). View
#14 config-validate-name-14.patch1.94 KBashutoshsngh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,950 pass(es), 4 fail(s), and 0 exception(s). View
#11 config-validate-name-11.patch2.25 KBtadityar
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch config-validate-name-11.patch. Unable to apply patch. See the log in the details link for more information. View
#9 config-validate-name-9.patch2.25 KBtadityar
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,471 pass(es). View
#5 config-validate-name-6.patch1.45 KBtadityar
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,192 pass(es). View
#3 config-validate-name-4.patch4.18 KBtadityar
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/Config.php. View
config-validate-name.patch1.85 KBTR
PASSED: [[SimpleTest]]: [MySQL] 59,489 pass(es). View

Comments

TR’s picture

config-validate-name.patch queued for re-testing.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

tadityar’s picture

Status: Needs work » Needs review
FileSize
4.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/Config.php. View

re-rolled the patch

Status: Needs review » Needs work

The last submitted patch, 3: config-validate-name-4.patch, failed testing.

tadityar’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,192 pass(es). View

re-roll again

penyaskito’s picture

Status: Needs review » Needs work

Patch #5 is not included the validation at setters (probably missing other things too).

tadityar’s picture

@penyaskito the function setName($name) and validateName($name) doesn't exist in /core/lib/Drupal/Core/Config/Config.php anymore. What should I do? include them again?

swentel’s picture

The functions are in ConfigBase now. Not sure about moving it, although it probably can't hurt.

tadityar’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,471 pass(es). View

re-roll with Updated the ConfigBase file

penyaskito’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/ConfigBase.php
@@ -78,6 +78,8 @@ public function getName() {
+  	// Validate the configuration object.
+  	static::validateName($name);

These have tabs instead of spaces. Check https://www.drupal.org/coding-standards#indenting

Otherwise looks good to me.

tadityar’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch config-validate-name-11.patch. Unable to apply patch. See the log in the details link for more information. View

My bad didn't notice that.. corrected the indentation :)

Status: Needs review » Needs work

The last submitted patch, 11: config-validate-name-11.patch, failed testing.

ashutoshsngh’s picture

Status: Needs work » Needs review
FileSize
1.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,950 pass(es), 4 fail(s), and 0 exception(s). View

Path won't work,re-rolled the patch.

Status: Needs review » Needs work

The last submitted patch, 14: config-validate-name-14.patch, failed testing.

ashutoshsngh’s picture

Status: Needs work » Needs review
FileSize
2.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,949 pass(es), 4 fail(s), and 0 exception(s). View

Missed one change in config.php file.And also fixed indent issues.

Status: Needs review » Needs work

The last submitted patch, 16: config-validate-name-15.patch, failed testing.

ashutoshsngh’s picture

It's working fine on my local i don't know why it's failing here.

deepakaryan1988’s picture

FileSize
2.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,949 pass(es), 4 fail(s), and 0 exception(s). View

Re-rolling the patch in comment #9

deepakaryan1988’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: config-validate-name-2150803-19.patch, failed testing.

willzyx’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
5.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,948 pass(es). View

I think we need to change also \Drupal\Tests\Core\Config\ImmutableConfigTest

TR’s picture

Status: Needs review » Needs work

Now that Config has been refactored, the validation should be done in the base class (ConfigBase) constructor, that way derived classes will inherit this functionality. It makes no sense to defer the validation to the child class constructor when the setName/getName/validateName functions are all in ConfigBase.

The patch also needs a new test case where Config::setName() is used to try to set an invalid name, to show that this patch does the right thing and to make sure this continues to work in the future.

@willzyk: How are your changes to ImmutableConfigTest.php related to this issue?

willzyx’s picture

@willzyk: How are your changes to ImmutableConfigTest.php related to this issue?

These changes are needed because in ImmutableConfigTest::setUp() the code below trigger the Config constructor passing an invalid name: 'test'

$this->config = new ImmutableConfig('test', $storage, $event_dispatcher, $typed_config);

Now that Config has been refactored, the validation should be done in the base class (ConfigBase) constructor, that way derived classes will inherit this functionality.

so the plan is to add the constructor to abstract class ConfigBase? anyhow in this way derived classes with their own constructor (almost all) will not inherit this functionality automatically and should explicitly invoke the parent constructor

TR’s picture

These changes are needed because in ImmutableConfigTest::setUp() the code below trigger the Config constructor passing an invalid name: 'test'

OK, thanks for clarifying that.

so the plan is to add the constructor to abstract class ConfigBase? anyhow in this way derived classes with their own constructor (almost all) will not inherit this functionality automatically and should explicitly invoke the parent constructor

Well, yes. The alternative is just to assume that everyone who subclasses ConfigBase will know that they *have* to provide a subclass constructor, whether they need it or not, *and* have to implement the validation in that subclass constructor. We don't have any way to reliably convey this message, let alone enforce it.

You're correct that in PHP if a subclass provides its own constructor then it's responsible for invoking the parent constructor if it wants the parent to be initialized properly. This should be well-known (and second-nature) to anyone writing OO code and implementing a __construct() method in a subclass.

willzyx’s picture

Status: Needs work » Needs review
FileSize
6.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,949 pass(es), 1 fail(s), and 0 exception(s). View
2.07 KB

I have taken into account #25 but I'm not convinced at all.

Moreover the changes that we are thinking to make could have a huge impact and create performance regressions so probably better have feedback from maintainers.

Status: Needs review » Needs work

The last submitted patch, 26: config-validate-name-2150803-26.patch, failed testing.

TR’s picture

could have a huge impact and create performance regressions

Please explain why you think this is a possibility - have you done any benchmarking? How many times is setName() normally called in a page load?

The fact that this patch has already exposed a core error - use of an invalid config name in ImmutableConfigTest - demonstrates my original claim that currently "... it's possible to create and use a Config object with an invalid name".

willzyx’s picture

FileSize
626 bytes
7.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch config-validate-name-2150803-29.patch. Unable to apply patch. See the log in the details link for more information. View

fixed tests

How many times is setName() normally called in a page load?

the problem is not ::setName() but the constructor that is invoked for every config object and performs the validation for all configs, even those that do not need it (eg saved ones). My concern for performance may be overdone (constructor + ::setName() are called ~50 times for every page load as authenticated user on homepage in my installation) but make some benchmark may be useful

willzyx’s picture

Status: Needs work » Needs review
alexpott’s picture

Validating the name on construct is unnecessarily expensive. Validating in the constructor means that all config names are validated even when we load existing configuration that has already been validated.

TR’s picture

I would argue that if there is a subset of Config objects that have already passed through validation, then that subset should be represented by a subclass of Config. The validation upon construction could then be bypassed in the constructor of the subclass, as an optimization, if warranted by performance benchmarks. To allow invalid objects to be constructed and used and not checked until you try to save them is really not an option IMO. But can you really say for sure that a config read from the file system has already been validated, and that no one has renamed it?

As an aside, it's not really clear to me why we make these specific validation requirements in the first place - why a maximum length of 250 characters? Isn't that totally arbitrary, or perhaps tied to just one specific storage type? There doesn't seem to be any documentation about why those checks are in there.

Status: Needs review » Needs work

The last submitted patch, 29: config-validate-name-2150803-29.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.