Spring-cloud-config: Config server resolves placeholders incorrectly for JSON and YAML endpoints

[rlynch]

I am seeing problems with the resolvePlaceholder functionality in Spring Cloud Config Server. A set of examples is best. First here is an example yml file I have in my git repo....

jsk:
  servlet:
    healthcheck:
      dirToWatch: '${catalina.home:.}/conf/_health'

When I call the server using the path used by the Spring Client the correct results are returned with the ${catalina.home:.} placeholder not being resolved.

_http://localhost:8888/sp_boot_sample/foo/ryan_test_

{
  "name": "sp_boot_sample",
  "profiles": [
    "foo"
  ],
  "label": "ryan_test",
  "version": "ed19d558ff52e08aeb72a517e46685a344389fd1",
  "propertySources": [
    {
      "name": "https://github.intuit.com/java-service-kit/sp-boot-sample-config/application.yml",
      "source": {
        "jsk.servlet.healthcheck.dirToWatch": "${catalina.home:.}/conf/_health"
      }
    }
  ]
}

It also behaives correctly when I request the configuration as properties with the resolvePlaceholder=false
_http://localhost:8888/ryan_test/sp_boot_sample-foo.properties?resolvePlaceholders=false_

jsk.servlet.healthcheck.dirToWatch: ${catalina.home:.}/conf/_health

YML and JSON is where we have problems since it is trying to replace the placeholder.

_http://localhost:8888/ryan_test/sp_boot_sample-foo.json?resolvePlaceholders=false_

{
  "jsk": {
    "servlet": {
      "healthcheck": {
        "dirToWatch": "./conf/_health"
      }
    }
  }
}

_http://localhost:8888/ryan_test/sp_boot_sample-foo.yml?resolvePlaceholders=false_

jsk:
  servlet:
    healthcheck:
      dirToWatch: ./conf/_health

The issue gets more problematic when you remove the resolvePlaceholders as the behavior becomes inconsistent.
YML
_http://localhost:8888/ryan_test/sp_boot_sample-foo.yml_

jsk:
  servlet:
    healthcheck:
      dirToWatch: ./conf/_health

JSON
_http://localhost:8888/ryan_test/sp_boot_sample-foo.json_

{
  "jsk": {
    "servlet": {
      "healthcheck": {
        "dirToWatch": "./conf/_health"
      }
    }
  }
}

Properties
_http://localhost:8888/ryan_test/sp_boot_sample-foo.properties_

jsk.servlet.healthcheck.dirToWatch: /private/var/folders/5g/0hmbzds942318msw3jxvvbp0nw22jg/T/tomcat.1297932675264632683.8888/conf/_health

To be honest I am not sure which one is correct. If the properties version is the correct one then it exposes a dangerous security vulnerability since any client could start examining the server's environment variables.

[ryanjbaxter]

I THINK this is a bug, I am able to reproduce the problem, but i am unfamiliar with the code. I also dont see any tests that test this specific scenario. I will check with the team next week and provide more info.

[rlynch]

Looking at the code this seems to be centralized to the EnvironmentController class. For the labelledProperties() method return there is a convertToProperties function which simply converts the environment to a map of properties but does not alter the value of the properties. For json and yml (labeledYaml() and labeledJson()) there is a convertToMap function which along with converting the structure of the environment to a nested map it also alters the property values....

        PropertiesConfigurationFactory<Map<String, Object>> factory = new PropertiesConfigurationFactory<>(
        //.... some structuring code....
        factory.setPropertySources(propertySources);
        factory.bindPropertiesToTarget();
        //... other stuff....

Its during the bindPropertiesToTarget that the placeholder resolution occurs regardless of if the resolvePlaceholders value is true or false. Because the convertToMap function alters the value of the properties and the convertToProperties function does not you get the inconsistant behavior. What I haven't determined is if there is a valid reason the convertToMap tries to alter the property values for a use case I am not thinking of.

I was going to look into this myself but I am having trouble running the unit tests as I am getting weird errors from Mockito.

[rlynch]

One last comment. It concerns me that we don't have the ability to completely eliminate place holder resolution on the server side. As it stands right now a malicious hacker could probe the server and potentially use it to retrieve any environment property set on the config server regardless of its relevance for outside configurations. This seems like a huge security vulnerability.

[ryanjbaxter]

That is my analysis as well, hopefully I will have more information for you soon.

[dsyer]

I think "huge security vulnerability" is a bit of an exaggeration (the config server should be secure in any reasonable environment, so only its authenticated clients can probe it, and only the ones that can write to its backend), but we can certainly correct this because it is not the intended behaviour. See f8fc4e19.