statamic / v2-hub

Statamic 2 - Feature Requests and Bug Reports
https://statamic.com
95 stars 5 forks source link

Variables in system.yaml handled by .env file are overwritten with value #2485

Open bendeca opened 4 years ago

bendeca commented 4 years ago

Setting a variable in the system.yaml config to e.g. '{env:APP_URL}' causes the value to be overwritten with the value of the current .env file. This only happened to me in the system.yaml config and only when editing the license key through cp/licensing.

But looking at the code, I can see that it could happen with any yaml config file.

To Reproduce Steps to reproduce the behavior:

  1. create a .env file in your web root directory with the content TEST=foo
  2. set a param in system.yaml to {env:TEST} e.g. php_max_memory_limit: '{env:TEST}'
  3. Edit the statamic license in the control panel at the url cp/licensing and save !!! this bug does not appear when editing the license key in the config menu > settings > system image
  4. check your system.yaml: It should say:

php_max_memory_limit: foo

Expected behavior variables handled by .env file should never be overwritten via the control panel.

Environment details (please complete the following information):

FIX The problem lies within statamic/core/Config/Settings.php

The logic is pretty much copied from statamic/core/Http/Controllers/SettingsController.php[58-63] This is also the function that gets called when editing the license key from the settings > system

Fixed content of statamic/core/Config/Settings.php:

<?php

namespace Statamic\Config;

use Statamic\API\File;
use Statamic\API\YAML;

class Settings extends Config
{
    /**
     * Save the config
     *
     * @return void
     */
    public function save()
    {
        foreach ($this->config as $file => $data) {
            // Don't save if it hasn't changed.
            if (array_get($this->original, $file) === $data) {
                continue;
            }

            // Remove environment managed vars from what was submitted, and replace them with their current values.
            // They aren't editable in the CP but will be submitted (possibly incorrectly) anyway.
            $environmentVars = array_keys($this->env()[$file] ?: []);
            $data = array_except($data, $environmentVars);
            $environmentValues = array_only(YAML::parse(File::get(settings_path("$file.yaml"))), $environmentVars);
            $data = array_merge($data, $environmentValues);

            File::put(settings_path("$file.yaml"), YAML::dump($data));
        }
    }
}
studio1902 commented 4 years ago

Yes, this is dangerous. I use a .env value for the url of the site in my system.yaml. Then I add my license key through the CP and this get's overwritten with the current local url. If I deploy that the site is broken. I've had that happen multiple times.

philipboomy commented 4 years ago

When I save the cache settings page it overwrites my static_caching_exclude causing big issues for contact form. This is when I use env vars to set the cache settings.

Probably related to the above so thats why I write it here.

stache_always_update: '{env:STACHE_ALWAYS_UPDATE}'
static_caching_enabled: '{env:STATIC_CACHING_ENABLED}'
stache_lock_enabled: true
stache_lock_wait_length: 30
static_caching_lock_hold_length: 0
static_caching_type: '{env:STATIC_CACHING_TYPE}'
static_caching_ignore_query_strings: '{env:STATIC_CACHING_IGNORE_QUERY_STRINGS}'
static_caching_file_path: public/static
static_caching_invalidation: all
static_caching_exclude:
  - /contact-us