[iacsha]

Validate the configuration map before persisting it (fixes #520)

Problem

PUT /api/server/configurationMap does not check the map it receives. The XML/JSON
body readers deserialize into a raw map, so the body can produce a null map, a null
key, a non-string key, or a null / non-ConfigurationProperty value.

DefaultConfigurationController.setConfigurationProperties persists the map before
it builds the in-memory copy:

if (persist) {
    saveConfigurationProperties(map);   // saved first
}
...
for (Entry<String, ConfigurationProperty> entry : map.entrySet()) {
    valueMap.put(entry.getKey(), entry.getValue().getValue());  // then throws NPE/CCE here

So a bad map is written to the CONFIGURATION table first, and the in-memory update
then throws. With configurationmap.location = database this leaves a row that breaks
the configuration map on the next startup — the request 500s but the damage is already
persisted.

Fix

• Add validateConfigurationMap(map): rejects a null map, and any entry with a
  non-string key or a null / non-ConfigurationProperty value, throwing
  ControllerException before anything else.
• Reorder setConfigurationProperties to validate → build the in-memory maps →
  persist last, so an invalid map can never reach the database.

The database-load path (loadDatabaseConfigPropsIfNecessary) already catches
ControllerException and logs it, so a server already bricked by this bug now starts
and skips the bad map instead of failing to load.

Tests

Added to DefaultConfigurationControllerTest:

• null map throws ControllerException
• entry with a null value throws
• non-string key throws
• a valid map is applied

./gradlew :server:test --tests "*DefaultConfigurationControllerTest*" → 19 tests, 0
failures.

Not included

The deeper fix — making the XML/JSON body readers honor the map's generic types so a
raw map can't be constructed in the first place — is left for a follow-up to keep this
change focused on preventing the persistence corruption.

[mgaffigan]

We need to use the same validation implementation on get as we do put, lest we get desynchronized.

Also: DCO

[iacsha]

Thanks @mgaffigan. DCO sign-off is fixed on all my open PRs.

On the get/put point: I think they already share one implementation. validateConfigurationMap is called at the top of setConfigurationProperties, and that method is the single path for all three ways a map comes in:

• the API PUT (ConfigurationServlet.setConfigurationMap)
• the database load in loadDatabaseConfigPropsIfNecessary (it deserializes the stored map and calls setConfigurationProperties(map, true))
• the file load in initialize() (setConfigurationProperties(configurationMap, false))

So a map read back from the database is checked by the same code that checks a PUT, and they can't drift apart.

One gap I did notice on the read side: if a bad map was already stored before this fix, the load path catches the ControllerException, logs it, and carries on with an empty map. A ClassCastException from the HashMap cast isn't caught at all. If that is closer to what you meant, I'm happy to add it here so a bad stored map fails clearly instead of falling back to empty. Let me know which way you'd like it.

[github-actions]

Test Results

  141 files  ±0    141 suites  ±0   3m 43s ⏱️ +44s
  923 tests +4    923 ✅ +4  0 💤 ±0  0 ❌ ±0 
1 055 runs  +4  1 049 ✅ +4  6 💤 ±0  0 ❌ ±0 

Results for commit 1747960. ± Comparison against base commit 892e33f.

[tonygermano]

This is a duplicate of #552 . I don't mind if you want to take over the fix, but please compare this to the existing PR to see if any part of it should be included in yours. It's still in a draft because it is stacked on top of #515 and was waiting for that one to merge to rebase. I don't remember exactly what's in it now, but I think it adds some tests that depend on 515 merging first.

Additionally, new contributors are being asked to sign the Eclipse Contributor Agreement. It is now described in section 8 of CONTRIBUTING.md. The failing check on all of your PRs should go away once you have completed that step.