Ensuring that requesting to update the metadata with values that do not serialize as a JSON object raises an exception in the client

[sjulian4]

This PR is intended to fix the issue where trying to update the metadata with update_metadata() with a parameter that is not of the right type (will serialize as a JSON object) allows for the modification but then causes server errors when trying to access. To fix this issue, this PR checks to see if the metadata does not match the dictionary type when it contains a value with calling update_metadata(metadata=value).

This issue was initially brought up in #1226.

Checklist

• Add a Changelog entry
• Add the ticket number which this PR closes to the comment section

[checkmarx-gh-ast-us-povs]

Checkmarx One – Scan Summary & Details – 8581f35f-c068-4429-bf0d-df87498310e6

---

New Issues (1)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		Use_of_Broken_or_Risky_Cryptographic_Algorithm	tiled/adapters/sql.py: 135	details In , the application protects sensitive data using a cryptographic algorithm, hexdigest, that is considered weak or even trivially broken, in /tile... ID: QCeC25%2Bsll4ISwokN0xaq%2FL%2FHa4%3D Attack Vector

---

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	Use_of_Broken_or_Risky_Cryptographic_Algorithm	tiled/adapters/sql.py: 215

---

Communicate with Checkmarx by submitting a PR comment with @Checkmarx followed by one of the supported commands. Learn about the supported commands here.

[danielballan]

Thanks for this fix! Indeed, it looks like we accept any JSON-serializable type, where we should insist on only things that serialize as a JSON object.

In Python, I've been trained to be suspicious of isinstance checks because they can overly constrain the user. We don't need something that is literally an instance of dict; we need something that implements a dict-like interface (and will therefore serialize as a JSON object).

Here's a real example of a dict-like object in the Python standard library that is not a dict and would fail this check, but actually works fine:

In [1]: from tiled.client import simple

In [2]: import collections

In [3]: c = simple()
Tiled version 0.2.10
http://127.0.0.1:43145/api/v1?api_key=3a3359cdbc50ed74

In [4]: d1 = {'a': 1}

In [5]: d2 = {'b': 2}

In [6]: d = collections.ChainMap(d1, d2)

In [7]: d
Out[7]: ChainMap({'a': 1}, {'b': 2})

In [8]: isinstance(d, dict)
Out[8]: False

In [9]: x = c.write_array([1,2,3], key='x')

In [10]: x.update_metadata(metadata=d)

In [11]: x
Out[11]: <ArrayClient shape=(3,) chunks=((3,)) dtype=int64>

In [12]: x.metadata
Out[12]: {'a': 1, 'b': 2}

In [13]: c['x'].metadata
Out[13]: {'a': 1, 'b': 2}

In [15]: import collections.abc

One path is to check against the abstract base case (abc) for "mappings" which is broader than dict.

In [16]: isinstance(d, collections.abc.Mapping)
Out[16]: True

[danielballan]

Sorry, I should have mentioned in my previous comment: Would you please add a test to verify the new behavior for a range of invalid inputs? Look for existing tests that exercise update_metadata using git grep update_metadata, and check that the expected type of error is raised using pytest.raises.

[sjulian4]

Hi! Thank you for looking it over! I added the pytests, let me know if there is anything else I should change.