Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem
- The namespace part in a configuration object is essential for Drupal core to maintain modules + config objects, but the config system allows to save a config object without a namespace (e.g., just
$module
). - The configuration file name needs to be fewer than 255 characters, implying an object name of 250 characters or less, but the Configuration system does not enforce this requirement. (See #1186034: Length of configuration object name can be too small.)
Proposed solution
- Add a
ConfigObjectNameException
for invalid object names. - Validate configuration object names to reject names which do not contain at least one dot/period (".") or is over 250 characters.
- Perform validation on save, synch, and import operations to ensure that invalid objects are not created or imported from other sources.
Comment | File | Size | Author |
---|---|---|---|
#81 | config-1701014-81.patch | 9.25 KB | xjm |
#81 | interdiff.txt | 3.43 KB | xjm |
#75 | config-1701014-75.patch | 9.05 KB | xjm |
#75 | interdiff.txt | 4.43 KB | xjm |
#73 | config-1701014-73.patch | 6.97 KB | xjm |
Comments
Comment #1
sunComment #3
sunerr, what? Do we have malformed config object names in core already...?
Comment #4
sun#1: drupal8.config-save-namespace.1.patch queued for re-testing.
Comment #6
alexpott#1: drupal8.config-save-namespace.1.patch queued for re-testing.
Comment #7
sunI think this patch is actually RTBC.
The exception is not caught anywhere, but that's intended. The system should fail prominently if someone is trying to save a config object without a proper name including namespace.
Comment #8
gddDo we want to check when a module is installed to verify that it hasn't supplied an improperly named file as well? Possibly the same thing on import?
Comment #9
sunWell, this patch sorta is a catch-all.
The check is injected into the low-level Config::save() method, so it applies to everything; config(), module installation, import, etc.pp. If anything at any point in time is trying to save config with a bogus config object name, the system blows up.
Comment #10
gddRight but so far as I can see, default module installation doesn't actually call Config::save(). It calls config_sync_changes() which does direct read/write operations on the storage objects themselves.
Comment #11
sunHm. You're right.
Though then again, in order to have any config to import, that config must have been saved at some point, and at that point, this validation would be triggered.
The alternative would be to implant a similar check somewhere into the config_import* functions (though I'm not sure which location would be appropriate currently).
Comment #12
gddThat's not necessarily true, any module author can create any default config file by hand they want. I'm sure many people will make a 'settings.yml' at some point and expect it work. It certainly won't work as expected, its a bug ultimately. I'm not sure this is worth protecting against or not, but I wanted to think through all the places this might be needed.
I don't really know where the best place to protect against this would be either. My first inclination was to build it into the FileStorage object, but then we're putting business logic in the storage object and that is no good.
Comment #13
catchIf the eventual storage dispatcher just contains a list of storage objects to read and/or write to, then won't we eventually be able to use Config::save() there too?
Comment #14
sunWe don't want to rely on StorageDispatcher, since we'll #1671080: Remove StorageDispatcher to simplify configuration system
So I suggest to just simply implant the identical check into the config import functions.
Comment #15
disasm CreditAttribution: disasm commentedattached is a test for saving configs without a namespace.
Comment #17
disasm CreditAttribution: disasm commenteduploaded the wrong files. here's the correct ones.
Comment #18
disasm CreditAttribution: disasm commentedComment #19
tim.plunkettThe test makes sense, here's some stuff to clean up.
Why use sprintf? Why not format_string?
Same here.
Once again.
There should be a blank line between class and getInfo
Needs a space after 'if' before '('
Add a blank line between these lines
Needs a space after 'if' before '('
And another blank line after the method before the end of the class
Comment #20
sunWe're using sprintf() in exceptions to decouple the Config system from procedural code in Drupal core. One of the final goals is to move it into Drupal\Component.
Comment #21
disasm CreditAttribution: disasm commentedattached are patches with Tim's changes.
Comment #22
tim.plunkett@sun, that makes sense. Is there an issue? It needs to be documented on http://drupal.org/node/608166
Comment #23
star-szr#21: drupal-1701014-config_save_namespace-21-test-only.patch queued for re-testing.
Comment #24
disasm CreditAttribution: disasm commentedAfter viewing document Tim linked above, caught to more style issues, this time with catch not having a ' ' after it.
Attached are more patches.
Comment #25
star-szrThanks @disasm!
A few other minor things I noticed:
This comment should end with a period.
These could be single quotes.
There should be a space after the first argument and before 'No Namespace…'
The consensus seems to be that the assertion messages should end in a period.
The test only patch in #21 showed 0 passes but it looks like that's #1717868: PIFR uses variable_get() but needs config() in Drupal 8.
Comment #26
disasm CreditAttribution: disasm commentedattached is a patch and interdiff to 24
Comment #27
star-szrWe reviewed this in IRC, thanks again @disasm.
One other minor point that was discussed after reviewing #26 - the 'description' in getInfo() could be a bit broader, rather than describing the testNamespace() method.
Comment #28
sunThanks for moving this forward! :)
Almost done! Some last issues:
No longer used in this file and can be removed here.
I wonder whether we really need an entirely new test case class. Looks like we could just add the new test method to ConfigCRUDTest instead?
Doesn't seem to be used in this file, so can be removed.
We don't need to assert the exact exception message.
The common way to test exceptions is:
Comment #29
disasm CreditAttribution: disasm commentedSince I moved the test and changed the functionality of the pass/fail logic, I've attached the test-only as well this time. Full inventory:
test-only-patch (should fail)
patch (should pass)
interdiff to patch in 26.
Comment #30
disasm CreditAttribution: disasm commentedComment #31
sunThanks! Almost done!
Missing newline before new phpDoc block.
Let's remove the word "exception" in all of the messages (leaving only ConfigException).
Comment #32
disasm CreditAttribution: disasm commentedattached is patch fixing comments from 31.
Comment #33
disasm CreditAttribution: disasm commentedforgot the interdiff...
Comment #34
sunExcellent, thanks! :)
Comment #35
catchWhy are we copy/pasting the same code three times? Isn't there a single place this can be checked? Or a helper if not?
I've complained about sprintf() in Symfony method overrides where it was copied from there, and I don't like it being introduced here either. We already have format_string() and t() that do placeholder substitution in core, and encourage people to use those for translation, avoiding security mis-haps etc.. If we start using sprintf() in seemingly arbitrary places (even if it's just exceptions, but I'm sure there'll be other places eventually), then it's going to confuse that issue - likely in the direction of people just using raw sprintf() all over the place. Since this code doesn't have the get-out of being a copy/paste from Symfony that's not even executed and ought to be removed again at some point, I'm not going to let it go in like this. Let's use format_string() here, then open a separate issue to discuss just using sprintf() in Drupal components to avoid the procedural dependency, since it's got nothing to do with this issue at all.
Comment #36
disasm CreditAttribution: disasm commentedAttached is a patch that moves copy/pasted code into static function Config::checkNamespace($name).
Also, patch uses format_string as catch requested above.
Interdiff to #32 included.
Comment #37
disasm CreditAttribution: disasm commentedComment #38
tim.plunkettThis isn't needed (and has too many '\Config's anyway)
No need for the extra line.
The opening curly brace should be on the same line
Comment #39
disasm CreditAttribution: disasm commentedattached is patch with requested changes.
It looks like the failed test above was unrelated to this patch, but we'll see what happens when this one is tested.
Interdiff to #36.
Comment #40
sunThanks! Almost done :)
Let's order these alphabetically.
Missing newline between methods.
What do you think of renaming this to validateNamespace() and adjusting the phpDoc accordingly?
Comment #41
disasm CreditAttribution: disasm commentedattached are requested changes
Comment #42
disasm CreditAttribution: disasm commentedComment #44
disasm CreditAttribution: disasm commentedpatch failed to apply, here's a reroll.
Comment #45
disasm CreditAttribution: disasm commentedComment #46
sun#44: drupal-1701014-config_save_namespace-44.patch queued for re-testing.
Comment #47
sun#44: drupal-1701014-config_save_namespace-44.patch queued for re-testing.
Comment #49
boztek CreditAttribution: boztek commentedRerolled.
Comment #50
leschekfm CreditAttribution: leschekfm commentedThe patch looks fine for me.
Comment #51
sunExcellent, thanks! :)
Comment #52
catchChecking this in three different places still feels like too much - what's wrong with just a non-static function and checking in save()?
Comment #53
sunAs visible in the diff, it is not checked in save() only.
Comment #54
catchYes I know that. Why can't we only check it in save?
Comment #55
sunBecause this does not go through
save()
— not even through a Config object at all?Comment #56
xjmAdding in a length limitation per #1186034: Length of configuration object name can be too small.
Comment #57
xjmI updated the issue summary to reflect the current scope and answer @catch's question. We want to validate the names whenever we have new configuration, which means it needs to happen both on save and on synch. Fail early, fail often, fail well.
Comment #57.0
xjmUpdated issue summary.
Comment #58
xjmAttached merges in the change and test coverage from #1186034-16: Length of configuration object name can be too small.
Note that #52 raises a point: right now, the tests only assert the expected behavior on
save()
operations. If we want to make sure the validation also happens on synch and import, we should add test coverage for that. I haven't added that yet in this patch.Comment #61
xjmOops.
Comment #63
chx CreditAttribution: chx commentedWhile at it, could we please exclude
: ? * < > " | / \
characters as well?Comment #64
gddOne other thing we talked about in #1479454-82: Convert user roles to configurables is adding a trim() to all names.
Comment #65
gddThis at least gets through the installer. Haven't gone much further but figured I'd push it up.
Comment #66
tim.plunkettI think this should technically be
static::validateName($this->name);
, but I don't know if it mattersThe self:: should be static:::
Comment #68
gddThese fails seem to be from the following line in rest.module
$resources = config('rest')->get('resources');
This naming is obviously broken, and rest.module also provides no default config. #1858676: Rest module config is named improperly and no default config is supplied. has been opened to address this.
Comment #69
xjmI've downloaded this patch and issue to take a look at offline.
Comment #70
sun- Incorporated #1858676: Rest module config is named improperly and no default config is supplied..
- Moved constant definition in Config first (constants should always defined first).
- Fixed coding style in new validateName() method.
- Fixed test tries to catch unknown exception.
Comment #72
chx CreditAttribution: chx commented+ // Validate the configuration object name before saving.
+ $this->validateName($this->name);
This should be static:: not $this->
Comment #73
xjmI disagree; your "fix" is not related to any coding standard that has consensus, but I don't care enough to change it. ;) People complain about both formats and it's really a matter of preference.
Anyway, the attached:
Still to do: confirm that the exception gets thrown properly on import and synch (one assertion each should be sufficient).
Comment #74
damiankloip CreditAttribution: damiankloip commentedI don't think this should be changed to the class name? Shouldn't this be static::validateName() instead?
Small point, but the $config variable doesn't seem to be used. So this could just be config($name)->save(); on one line.
Nice, I like the idea of this. I'll remember that when I need to test exceptions :)
Also, I opened #1879774: Catch plugin exceptions for invalid views display plugins as we want to deal with display plugin errors instead I think. So we will want to change, or just remove the views validation in this patch maybe.
Comment #75
xjmIsn't this the same thing? I guess if someone wanted to subclass Config and override the method...? I changed it in any case.
OK.
I also added the two additional assertions per #73. In the process of so doing I noticed that
config_sync_changes()
is not ever called in core withoutconfig_import_invoke_owner()
being called first; do we still need to validate the name in both?Comment #76
damiankloip CreditAttribution: damiankloip commentedNo, not the same thing, it's not internal to the class then, as it's hardcoding the Config class. I don't think we do this anywhere else (or hopefully not!) That's how we would call this from outside the class. So I think it kind of has to be changed ;)
Patch is looking good.
Comment #77
xjmDo we also need to change these, then?
Comment #78
xjmOkay, apparently #77 isn't a problem and I just don't understand things very well. I had to re-read #76. It seems a little weird to have some procedural and some OO code around this, but that isn't in scope for this issue.
Comment #79
gddIt's really nice seeing this documented with references now.
We will probably need a followup to make sure that not only do we have a namespace, but that it is an installed module/theme, because that will likely become codified when #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API is sorted out.
I'm happy, thanks all!
Comment #80
sunThis patch looks also great to me.
Not sure on disallowing forward-slashes yet, since config context was supposed to use subdirectories for overrides, which might need to be included in the name in some form, but for now, it's absolutely better to disallow everything, so let's definitely move forward with this patch!
Thanks all!
Comment #81
xjm@webchick asked for clarification about how the filenames in the last two assertions were invalid, and my discussion made it apparent that the naming and documentation of the test module was unclear, so the attached patch makes the names more self-explanatory and adds docs.
Comment #82
webchickLooks great, thanks!
Committed and pushed to 8.x.
Comment #84
xjm