Security vulnerabilities which do not need a security advisory. For example, security issues in projects which do not have security advisory coverage, or forward-porting a change already disclosed in a security advisory. See Drupal’s security advisory policy for details.Be careful publicly disclosing security vulnerabilities! Use the “Report a security vulnerability” link in the project page’s sidebar. See how to report a security issue for details.

D8MI

(Drupal 8 Multilingual Initiative) is the tag used by the multilingual initiative to mark core issues (and some contributed module issues). For versions other than Drupal 8, use the i18n (Internationalization) tag on issues which involve or affect multilingual / multinational support. That is preferred over Translation.

Needs tests

The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Problem/motivation

There are common APIs to access configuration across different parts of Drupal:

1. FormBase::config() proxies your call to the configuration factory.
2. ConfigFormBase::config() does the same but ...

The behaviour is very different. ConfigFormBase::config() will disable all overrides with the assumption that you deal with a configuration form to display and save non-overriden configuration. The original behavior of FormBase::config() is overriden, so there is no direct way to access configuration with all overrides applied.

Let's say you have an admin form that sends an email when a setting is updated. It sends to the site email address. You override that of course in $global['conf']['system.site']['mail'] = 'localdev@example.com'; so your customer will not get the emails from your developer site. You don't want this local override to get into the active configuration so that it is not applied when you edit admin settings is good. However in the form that sends email, it will also not be applied for sending the email. So pooof, you sent emails to your customer even though there is nothing evident in your code, that the config() method that in the other class applied overrides did not apply overrides in this class.

This is an example that some people mentioned, but I suspect there are probably other examples that I did not think of yet.

Proposed resolution

A. Make global overrides apply all the time. This would make local dev overrides end up in active configuration so not sure developers want to need to review those changes all the time?
B. Make the API explicit about which config methods return possibly overriden values and which return raw original values all the time. (Note that this may have side effects for how we solve entity listings, see #2136559: Config entity admin lists show configuration with overrides (eg. localized)).
C. Decide this is not a big problem. We are not concerned with dev environment emails from admin forms and other possible side effects that may come.

Comments

In Drupal 8 if you put something in global $conf, that overrides your config by default but it may be ignored if the code specifically asked for an "override free context" or if there are translations for the value, those would trump global overrides. It would be important to get this right and I think there are people using global $conf as a security enforcement system (beyond admin permissions), which is NOT currently possible in Drupal 8 due to how it is applied. I think the way it works now makes sense for use cases where you want a soft fallback default that cannot be edited, but if we consider global $conf overrides as a security measure, we should somehow support that use case too, or maybe restore global $conf to be an enforcement system like it was in Drupal 7 and not have soft overrides or have them in a different way.

Being able to override settings per-hostname is extremely important for many developers and many sites. The settings.php $conf array is how I ensure that production sites have the correct production-worthy settings enabled while my local install has all caching disabled, uses alternative API keys, etc. This must be possible. How can I help? :-)

@DamienMcKenna: yeah, so overriding for different host names is an interesting use case. Do you mean overriding conditionally in settings.php or just using different settings.php files in different environments? global $conf is still used and applied EXCEPT if the code is in an "override free" environment which is only applied for admin forms. So if you go edit your site setup, your $conf overriden values will not be present there and not saved. Which is logical if you use logic in settings.php to apply to different domains, but probably irrelevant if you just use static settings.php overrides on different setups.

Different values for a "true" multisite install, e.g. different API keys for different Mailchimp accounts.

Different values for different instances of the same site, e.g. API keys for the production vs dev Acquia Network accounts to avoid corrupting the production search index :) and different performance tuning settings.

I've had a blog post kicking around in my head for 2+ years on how I handle variables, I really need to write it down..

So the question is if its a bigger problem if those overrides creep into settings forms and get saved as original config on sites, or if its a bigger problem if they don't creep into forms but then they are not applied when editing those config (so eg. if the global override changes the theme that would not be possible on the admin editing form). Which is the bigger problem? :)

I have done many of the same things as @DamienMcKenna - I think the API key use case is one to consider for sure - how to we ensure that "production only" config doesn't leak to staging and vice versa (you don't want to get your real and testing Paypal API keys mixed up, trust me!). If there is not already a good way to ensure this, $conf might be a reasonable solution.

I have also used the case of having multiple hostnames all using sites/default (and the same database/files etc) and using $conf to switch out some key variables (frontpage, language, site name etc) based on the hostname. This is all stuff I could have done (less efficiently) with using sites/hostname or other means however.

Another use case to consider is for automated hosting setups - allowing the host to set the database and reverse proxy configuration dynamically (as Acquia does, for instance), and if necessary change it - it seems preferable if this can be done without needing to trigger a config reload or other operation that may have side effects.

Another use case to consider is for automated hosting setups - allowing the host to set the database and reverse proxy configuration dynamically (as Acquia does, for instance), and if necessary change it - it seems preferable if this can be done without needing to trigger a config reload or other operation that may have side effects.

Anything that can't actually be stored in the database/CMI is in $settings now - this distinguishes between things that are exclusively configured in settings.php vs. overrides. There's likely a grey area where something is used after CMI becomes available but you don't ever want a UI for it, I'd lean towards $settings for those too.

I think it's OK that conf overrides don't show up in forms and aren't saved back, that's arguably a bug fix compared to Drupal 7, there's two things that could use clarifying though:

1. Should we warn people on the form somewhere that values are overridden in $conf and their changes won't take effect?

2. We should put a huge warning on the 'no context' context to make it clear that $conf overrides are disabled for that context. I can't really think of a reason to use it except for the forms and maybe the new drush vget.

@Gábor: With my dev workflow, nothing is changed via the admin UI on production, so these overridden settings being exported to the config files elsewhere doesn't matter, the changes won't be committed unless I've reviewed every single line and given it my blessing.

I do address a similar issue over in Domain Access, where I have a small hook that can be used to declare which admin forms contain domain-sensitive values. In an ideal world, the config system might detect these hard overrides and flag them for the user via a message.

That would, however, involve some trickery to match $conf values with form config values. Perhaps if we disallowed putting "loose" $conf variables in settings.php and instead allowed for a standard function call for overrides:

I don't think there is a common element in config forms, etc. views, node types and site settings don't have much in common, also form structure is not related to config structure (as in views, or even the 403/404/front path config is entirely differently presented on the form vs. the config file). It was a point of the CMI initiative *not* to deal with automated form generation. D8MI needs to generate such forms, so we have http://drupal.org/project/config_translation which was so far deemed not a fit for Drupal 8 core. There it is possible to mark elements, because all forms are autogenerated from the schema/data combined. I don't see a reliable way to mark elements in Drupal forms overall where original config is edited.

I think it is a big win to disable form elements where the admin's changes would not make any difference. It is not appropriate IMO to offer a non-overidden value for editing without telling the user whats going on. His edit is likely not to make any difference. We have always done it that way and it has always sucked :(

If config_translation project gets us there, perhaps we should consider it for core.

@moshe: I'm not sure such programatic backreferences are possible. In Drupal 7, system_settings_form() pages were known to store their values in variables and their form elements were named equal to the variables. So you could look at the form element name and put on warnings if it was overridden. Eg. variable module used this to put on notes if the variable was multilingual.

In Drupal 8, there is no central store like variables, and it is not really known where in config would the value be stored. So while you can get the form element name, you don't really know where in config it would end up. For example the user account settings page uses two config files to store settings, one for "base" settings like anonymous user name or whether registration is allowed and one for all the user email subjects and bodies. All are edited on one form altogether. Menus are config entities and the menu editing form merges editing the menu itself with menu items, so it is quicker for the user. So to be able to tell for specific form fields whether they are overridden (globally or in some language), we'd need to have this information on each form element. That is lots of added info to form structures, that is not there now. And then finally, the question of how we generally disable an element comes up. There are potentially complex form elements. Eg. if the site logo upload field would have an inline preview of the logo, how would you make that form element disabled? If you enforce date formats to be in a specific setup, then multiple admin pages would be entirely disabled / pointless. You could try to add new formats, but that would not work.

The config initiative specifically opted to not deal with automated form generation and wanted to leave forms as one-off implementations on top of config. Later config entities somewhat structured this for whatever is implemented through config entities, so you kind of have some structure (and config vs. form relation somewhat programatically discoverable), but still then the form item vs. config key relation does not necessarily work. See menus.

What config translation (contrib) module does it takes the config schema, that describes the structure of the config and generates translation forms based on that. It totally ignores the original forms for config, because it just cannot pull out fields from the various views forms or from the middle of a config entity or cherry-pick elements from different levels from the structure of the user account settings form. So that does not even attempt to do this relation, because it looks pointless. Dries spelled out earlier he sees this module as a parallel form API and not a fit for Drupal 8. Also, this module needs to build up its own relation system between admin paths and config keys, so it can put translate tabs on the right places for example (and go and alter random forms of various structures to put in translate operations to summary tables).

I guess we need an "override free" state so that we don't save translated values of site slogan and config like that. Can't we just neuter the LocaleConfigOverride in this case? In my world, all conf overrides would *always* be in effect, even during admin forms. This preserves D7 behavior and satisfies the security concern described by @pounard.

Yes, it is totally our decision, whether we want to make global overrides always apply for example, if those showing up in forms is a smaller issue vs. those not being applied on some admin forms. If we add a feature so overrides can optionally apply in override free contexts too, then one can do logic based overrides (such as domain based overrides in contrib) with a system separate from global $conf and it is their choice if it applies in this context or not. We will need a better name for this context then too :)

@David: sounds like the reason indeed. Looks like one more +1 from you to make global overrides stick. (Considering them creeping into forms not a bug like many people noted but a feature like many other people noted). Here is a possible solution. Will probably not pass tests yet.

1. I kept the override-free context, since it is used in internal config operations to access the original values for import & staging (see code in core/config.inc exclusively, which is not even touched in the patch). With this patch that is the only place where that is used.
3. Added the capability to force overrides. The default override handler will take any override either forced or not. The free context will not take any override, even if forced (may be a possibility for misunderstanding :) and there is a forced only context, which only takes forced values.
2. Added a config.context.only.forced on the DIC to expose this and made the admin forms use it. Made global overrides forced.

I guess we need an "override free" state so that we don't save translated values of site slogan and config like that. Can't we just neuter the LocaleConfigOverride in this case? In my world, all conf overrides would *always* be in effect, even during admin forms. This preserves D7 behavior and satisfies the security concern described by @pounard.

Domain module will also use this override system. The overrides of domain module are conditional on which domain you are on, so saving them back to the original config might be an issue or at least confusing. There are unconditional overrides like sometimes global $conf (although I've seen conditions in settings.php files when setting config values in $conf), and there are dynamic overrides like translations or domain overrides. Looks like we want some of them apply to forms but not others. So the question is where is that decision made. The patch I proposed puts that decision with the override event listener and only the global overrides decide to "apply globally" in the patch. The domain overrides could then decide in contrib to apply globally (except when truly no overrides apply in internal config operations) or not.

It helps if I follow the new interface definition in implementations :)

Also as for why the overrides creeping in to forms and therefore current config is a problem, if you have eg. domain module, depending on which domain you save your admin form, the config saved will be different, if the domain conditional config override gets into the form. That will be confusing when you stage / update site config from development, since your live site will show changes while it just flip-flops between different domain based values. The config diff / staging system as well as overrides were built with the idea that the saved configuration is the canonical value and overrides are not used as "even more canonical".

Anybody still interested in resolving this? It has been raised to critical, a solution has been proposed and then no feedback for over two weeks. If that is as much as we care, then I don't see how this would be critical?

i've tried to think of something useful to say about this issue and patch a couple of times, but never quite managed it.

implementation details aside, i'm ok with values set in settings.php being special, so i think i'm ok with the patch in #24. the context system is already insanely hard to reason about, so i don't think another layer of special casing makes much difference, and seems to solve a real use case or ten.

honestly, i'd still rather we rip out the context system, and bake in special handling for settings.php and locale. that would get us more features than D7 (first class locale support), without making it insane.

Per my review with @YesCT, this patch is likely in need of a complete rewrite. Maybe @Cottser can help?

There are three files with issues: core/modules/system/system.admin.inc, core/modules/user/user.admin.inc, and core/lib/Drupal/Core/CoreBundle.php. User.admin.inc in particular had ~1,700 lines deleted from it, and as was mentioned above CoreBundle.php was completely removed.

We attempted to find the issue that deleted CoreBundle.php with the command: git log -S'class CoreBundle extends Bundle'. The intention was to find the commit where that particular line was removed, which would tell us when the file was deleted. The result was:

$ git apply drupal-n1934152-39-conf_ftw.patch
error: patch failed: core/lib/Drupal/Core/Config/Config.php:275
error: core/lib/Drupal/Core/Config/Config.php: patch does not apply
error: core/lib/Drupal/Core/Config/Context/ConfigContext.php: No such file or directory
error: core/lib/Drupal/Core/Config/Context/ContextInterface.php: No such file or directory
error: core/lib/Drupal/Core/Config/Context/FreeConfigContext.php: No such file or directory
error: core/lib/Drupal/Core/EventSubscriber/ConfigGlobalOverrideSubscriber.php: No such file or directory

Figure out override priorities and whether we want global overrides to stick (settings.php overrides don't work on all pages)

» Figure out if we want global config overrides to stick (settings.php overrides don't work on all pages)

Updated the issue summary! The situation now in short is that global overrides always trump every other override, see ConfigFactory::get(), but no override is applied if in override-free mode, such as in admin forms, see ConfigFactory::setOverrideState(). So this later situation is in question in this issue.

public function setOverrideState($state) {
$this->useOverrides = $state;
return $this;
}
public function get($name) {
if ($config = $this->loadMultiple(array($name))) {
return $config[$name];
}
else {
$cache_key = $this->getCacheKey($name);
// If the config object has been deleted it will already exist in the
// cache but self::loadMultiple does not return such objects.
// @todo Explore making ConfigFactory a listener to the config.delete
// event to reset the static cache when this occurs.
if (!isset($this->cache[$cache_key])) {
// If the configuration object does not exist in the configuration
// storage or static cache create a new object and add it to the static
// cache.
$this->cache[$cache_key] = new Config($name, $this->storage, $this->eventDispatcher, $this->typedConfigManager);
if ($this->useOverrides) {
// Get and apply any overrides.
$overrides = $this->loadOverrides(array($name));
if (isset($overrides[$name])) {
$this->cache[$cache_key]->setModuleOverride($overrides[$name]);
}
// Apply any settings.php overrides.
if (isset($GLOBALS['config'][$name])) {
$this->cache[$cache_key]->setSettingsOverride($GLOBALS['config'][$name]);
}
}
}
return $this->cache[$cache_key];
}
}

Yes, settings.php overrides must win. I am not sure how is this even a question but having access to settings.php is usually rarer than having access to the UI so it should win.

We discussed the UI half with timplunkett and the conclusion I arrived to is as follows:

The overrides should not get into CMI because a development override shouldn't get into the export to bleed into production.

However, this leads to situations where you change a value in the UI and it doesn't take.

The desired resolution for this, in my opinion, is to show the overridden value in place of the edit element and in the description add "This value is overridden, the original value is X click here to edit it. The change will be saved into the database and can be exported but it won't be visible as long as it is overridden." and then the user clearly knows why his edit won't take.

Yeah that as an idea is great (to show if a value is overridden) *however* configuration does not have a consistent formbuilder API at all for anything outside the form class itself to inject generic messages and/or disable elements. Configuration does not even use standardized widgets so we can tell a widget to disable itself (eg. parts of the views UI or a block configuration or whatnot). So long as each form needs to deal with any part of it needing to be disabled and print that message, it becomes a huge pain.

Finally, even if all forms would deal with that and display such message, it may or may not be true. There is no indication as to whether the global override is static or its dynamic, ie. it may or may not apply at certain cases and the db stored value may or may not apply then with appropriate conditions. Eg. if config is overridden for anonymous users, the admin will never see the override on the UI and the changes will be possible to be saved, but not taken for anonymous users. Same if the config is overriden for authenticated only, the config cannot be changed in your scheme even if it would change how anonymous behaves.

One of the things pointed out in the issue summary is that global settings overrides are used for all kinds of fun things, eg. site sections / context dependent settings which comes with various conditions.

In short I don't think we can do anything about this on the UI since there is no control over how the UIs are created in the first place. AFAIS the only question is whether we apply those overrides for the UIs too, which seems to be people saying NO, since it would save back to actual config and with the assumption that the global settings overrides are environment specific (another interesting assumption about what people use their global overrides for :D), we don't want them to end up in that config.

OTOH now the entity loaded in upcasting in admin contexts and all config loaded on forms is override-free and therefore if you wanted to apply overrides to eg. configuration of how the form is built or how the form acts when submitted that is impossible to do now because the overrides don't apply. Eg. if the admin form sends an email when changed and the email address is overridden in the local context so the live monitoring is not spammed, this will not work because admin forms don't apply overrides you know.

This issue was opened for security considerations. I don't have a good example, but surely there are people who are more creative than I am :D

eg. configuration of how the form is built or how the form acts when submitted that is impossible to do now because the overrides don't apply. Eg. if the admin form sends an email when changed and the email address is overridden in the local context so the live monitoring is not spammed, this will not work because admin forms don't apply overrides you know.

Hmmm... we should only be using override-free configuration for what is displayed & configured using the form - not for how form acts.

Hmmm... we should only be using override-free configuration for what is displayed & configured using the form - not for how form acts.

Yeah that sounds good as a principle. However:

1. In practice since we have all that code doing $this->config(), it is very easy to use $this->config() for whatever builds the form or reacts to the submission as well.
2. OTOH any API calls to get data for forms (ie. not using $this->config() to get data but some API) may get us overridden config, since our override state altering is entirely encapsulated in ConfigFormBase::config().

So forms may still get overridden config where they did not want (2) and they would naturally use $this->config() for auxiliary config access even for things where overrides should be applied (1). I think the distinction is where overrides should be applied is a big enough can of worms that the multiple use cases of global overrides just complicates more.

So looks like it would be down to developers understanding the whole override mess for them to make informed choices, no? We can make parts of the API easier to identify where they belong. Eg. we can rename ConfigFormBase::config() to rawConfig() or something along those lines? Not sure how to make it apparent that admin upcast entities are also raw but all other API access will not yield raw config and that for behavioural code in the form one should not use raw config.

+1 to making it more explicit through renaming the method - great idea.

FormBase implements config() too - so I think the ConfigFormBase should actually throw an exception if that is called. And then we can have two methods, not sure of their names... ConfigForEditing() and ConfigForUsing() so that the caller knows the explicit choices they are making.

Here is a draft for what that may look like with rawConfig() and liveConfig() as method names. I just did a quick phpstorm refactor so there may be false positives or missing pieces. Just posting to keep the discussion going. The substantial change is in ConfigFormBase.php

it also has getOriginal() which is the (not yet modified from) stored version of the config with or without overrides :) So we should somehow look for terminology harmony here and if we want to introduce something else here, then apply it to elsewhere where it makes sense.

liveConfig() would be used on any config that is read to construct the form and/or to react to the form being submitted outside of saving the config itself. That is if you are editing config A, you may use config B to construct the form or react to saving it. I'm not sure replacing config() methods on base classes with liveConfig() for consistency is a good idea in and of itself though.

One of those patches where using git to reroll is significantly easier: the patch applied on top of d0d3e533615df7539a54cac995b50e75f18d1597 then I merged on top of HEAD and there was a single, easy to fix conflict.

Your patch contained unrelated stuff, like that MenuSettingsForm. I redid the patch from scratch with the following:

- same changes in ConfigFormBase and FormBase
- phpstorm refactor to rename config() to rawConfig()
- I removed the config() from ConfigFormBase since it was not in use anymore

The goal of this issue to let config edit forms access raw config by default. So long as you are not **editing** your config, the primary use case is you want to access it with all overrides applied, in fact that distinction was the goal of this issue. So I am not sure why would content entity forms need this at all? They don't currently have a config() method and if they would have one, they would expect to get values with overrides (they are not editing that config).

Note that 82 also renames the config() method to rawConfig() in FormBase, which is silly, that is a side effect of the overzealous phpstorm refactor tool :) We should not do that of course. Here is another more intelligently ran refactor that keeps config() on FormBase intact. No interdiff because I recreated the refactor with phpstorm again. The actual changes are only in ConfigFormBase the others are refactor (search and replace).

I don't think its a smart idea to work on more code for this unless we even agree we want something like this. Sounds like an easy waste of time for someone. The tests will not make the approach any easier to review or provide feedback on. We still need feedback on this. I don't contest that it needs tests once we agree on even doing this and doing it this way. Therefore moving back to needs review for feedback.

@dawehner: Thanks for the feedback. I definitely agree a more semantic exception is in order. I will work on that as soon as there is some agreement we want this kind of change to happen :) You also identified the reasons for the two new methods very well. I think overriddenConfig() may be a bit confusing, as we don't really know if there are any overrides, there may be... on an English only site that does not use domain access, og, etc. there will not be anything that may even override config. So that was the reason for liveConfig(). But I don't have very strong feelings on that, overriddenConfig() is fine if others believe that is fine too :)

I really think having global overrides from settings.php is important. We use it for all kinds of things such as disabling cache, css/js aggregation locally, API keys on live etc.

Didn't Acquia just use this global override approach to switch out the DB abstraction for the recent security issue? Yes you might do that differently in D8 but I think that shows that a way to trump all other configuration from the settings file is very handy in many cases.

@benjy: right, this patch does not make them applied less, it just makes it obvious when overrides are used vs. when they are not, so people can consciously use what they need to. It is important that when you edit config, you can edit the original values and that experience is not obscured by overrides (for domain, language, etc). However for anything else on the page, such as eg. labels of the forms for editing the config should apply overrides. Eg. if you are editing a view in French, the UI should be French (eg. labels of fields) but if the view is originally English, you should edit the English view not a (partial) translation.

Prior to this patch it is easy to just use config() and depending on whether your class was extending ConfigFormBase or not, it would behave VERY differently. The goal of this patch is to make that behaviour difference apparent.