Repository: devopshq/artifactory

Issue:
Property values are not URL quoted for matrix parameters
This only affects matrix parameters as far as I can tell; we should not quote parameters that are set after the fact because those go through a POST.

Because property values aren't encoded/quoted, it can cause undesirable characters to make it into properties, or cause the properties to be truncated. For example if your value contains a question mark `?`, it seems to get stripped somewhere; this character plus any characters after it do not end up in the property value.

If your value contains `%` followed by any hex value, the resulting character with that code will be inserted into the property value instead of the literal, for example if your value contains `%0` you'll end up with a null character in the value.

Example repros (can be done with the stock Artifactory OSS container):

```python
from artifactory import ArtifactoryPath

with open('/tmp/delme', 'w'):
    pass

a = Artifactorypath('http://localhost:8082/artifactory/example-repo-local', auth=('admin', 'password))

props = {
    'test1': 'a?b',
    'test2': 'a%3Fb',
    'test3': 'a%0b',
}

a.deploy_file('/tmp/delme', parameters=props)
```

---

I believe this should be fixable with [`urllib3.quote`](https://docs.python.org/3/library/urllib.parse.html#urllib.parse.quote) being applied to each version.

If this is an acceptable fix, I might be able to put up a PR for it.

If the position of the library is that callers are responsible for encoding their property values, then we must also make that explicit (because if callers quote their values now, and later the library adds quoting, it will double encode and ruin the values).

For the same reason, if we do opt to do quoting in the library, we should call it out as a breaking change.

Discussion:
briantist: @allburov would appreciate you approving the CI run so I can work on the integration tests 🙏 
briantist: I now realize the integration tests are not run in CI 🤦 I tried to run them locally but without a license key it seems impossible :(
I'd rather not sign up for a trial right now...

I'm not sure how you want to proceed on that. I can make guesses on changes to the integration tests but it seems dangerous to do so.

Are you able to run integration and either make the changes in my branch, or show me the warnings/errors and I can try to make changes (this will probably only work if small changes are needed, our back-and-forth will be somewhat slow I think)
allburov: @briantist sorry, I don't have an Artifactory license too right now :( 

@beliaev-maksim Could you help us with running integration tests for the PR?
beliaev-maksim: @allburov I also lost access to artifactory.. 
briantist: Re: integration tests: of course I would prefer to have coverage in them, but I also don't believe they will be failing right now. I think they will simply show warnings until v0.10.0.

If we can get a license that would be great, but if not, I don't think I should modify the tests without someone being able to run them; greater chance that I will break something than help.
briantist: Hi @allburov & @beliaev-maksim  just wanted to check in on this
allburov: It affects the actions too https://github.com/devopshq/artifactory/issues/419


kohodono: @allburov checked on 7.31.13 - it works!

Patch:
diff --git a/artifactory.py b/artifactory.py
index ae87d22..754bcd0 100755
--- a/artifactory.py
+++ b/artifactory.py
@@ -36,6 +36,7 @@ import re
 import sys
 import urllib.parse
 from itertools import islice
+from warnings import warn
 
 import dateutil.parser
 import requests
@@ -315,21 +316,31 @@ class HTTPResponseWrapper(object):
         return int(self.getheader("content-length"))
 
 
-def encode_matrix_parameters(parameters):
+def encode_matrix_parameters(parameters, quote_parameters):
     """
     Performs encoding of url matrix parameters from dictionary to
     a string.
     See http://www.w3.org/DesignIssues/MatrixURIs.html for specs.
+    If quote_parameters is true, then apply URL quoting to the values and the parameter names.
     """
     result = []
 
     for param in iter(sorted(parameters)):
-        if isinstance(parameters[param], (list, tuple)):
-            value = f";{param}=".join(parameters[param])
+        raw_value = parameters[param]
+
+        resolved_param = urllib.parse.quote(param) if quote_parameters else param
+
+        if isinstance(raw_value, (list, tuple)):
+            values = (
+                [urllib.parse.quote(v) for v in raw_value]
+                if quote_parameters
+                else raw_value
+            )
+            value = f";{resolved_param}=".join(values)
         else:
-            value = parameters[param]
+            value = urllib.parse.quote(raw_value) if quote_parameters else raw_value
 
-        result.append("=".join((param, value)))
+        result.append("=".join((resolved_param, value)))
 
     return ";".join(result)
 
@@ -1131,6 +1142,7 @@ class _ArtifactoryAccessor:
         explode_archive_atomic=None,
         checksum=None,
         by_checksum=False,
+        quote_parameters=None,  # TODO: v0.10.0: change default to True
     ):
         """
         Uploads a given file-like object
@@ -1148,8 +1160,18 @@ class _ArtifactoryAccessor:
         :param explode_archive_atomic: (bool) if True, archive will be exploded in an atomic operation upon deployment
         :param checksum: sha1Value or sha256Value
         :param by_checksum: (bool) if True, deploy artifact by checksum, default False
+        :param quote_parameters: (bool) if True, apply URL quoting to matrix parameter names and values,
+            default False until v0.10.0
         """
 
+        if quote_parameters is None:
+            warn(
+                "The current default value of quote_parameters (False) will change to True in v0.10.0.\n"
+                "To ensure consistent behavior and remove this warning, explicitly set a value for quote_parameters.\n"
+                "For more details see https://github.com/devopshq/artifactory/issues/408."
+            )
+            quote_parameters = False
+
         if fobj and by_checksum:
             raise ArtifactoryException("Either fobj or by_checksum, but not both")
 
@@ -1159,7 +1181,9 @@ class _ArtifactoryAccessor:
         url = str(pathobj)
 
         matrix_parameters = (
-            f";{encode_matrix_parameters(parameters)}" if parameters else None
+            f";{encode_matrix_parameters(parameters, quote_parameters=quote_parameters)}"
+            if parameters
+            else None
         )
         headers = {}
 
@@ -1790,7 +1814,11 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
         sha256 = hashlib.sha256(data).hexdigest()
 
         fobj = io.BytesIO(data)
-        self.deploy(fobj, md5=md5, sha1=sha1, sha256=sha256)
+        self.deploy(fobj, md5=md5, sha1=sha1, sha256=sha256, quote_parameters=False)
+        # TODO: v0.10.0 - possibly remove quote_parameters explicit setting
+        # Because this call never has parameters, it should not matter what it's set to.
+        # In this version, we set it explicitly to avoid the warning.
+        # In 0.10.0 or later, we can either keep it explicitly set to False, or remove it entirely.
         return len(data)
 
     def write_text(self, data, encoding="utf-8", errors="strict"):
@@ -1935,6 +1963,7 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
         parameters={},
         explode_archive=None,
         explode_archive_atomic=None,
+        quote_parameters=None,
     ):
         """
         Upload the given file object to this path
@@ -1948,6 +1977,7 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
             parameters=parameters,
             explode_archive=explode_archive,
             explode_archive_atomic=explode_archive_atomic,
+            quote_parameters=quote_parameters,
         )
 
     def deploy_file(
@@ -1959,6 +1989,7 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
         parameters={},
         explode_archive=False,
         explode_archive_atomic=False,
+        quote_parameters=None,
     ):
         """
         Upload the given file to this path
@@ -1981,6 +2012,7 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
                 parameters=parameters,
                 explode_archive=explode_archive,
                 explode_archive_atomic=explode_archive_atomic,
+                quote_parameters=quote_parameters,
             )
 
     def deploy_by_checksum(
@@ -1989,6 +2021,7 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
         sha256=None,
         checksum=None,
         parameters={},
+        quote_parameters=None,
     ):
         """
         Deploy an artifact to the specified destination by checking if the
@@ -2007,10 +2040,17 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
             checksum=checksum,
             by_checksum=True,
             parameters=parameters,
+            quote_parameters=quote_parameters,
         )
 
     def deploy_deb(
-        self, file_name, distribution, component, architecture, parameters={}
+        self,
+        file_name,
+        distribution,
+        component,
+        architecture,
+        parameters={},
+        quote_parameters=None,
     ):
         """
         Convenience method to deploy .deb packages
@@ -2021,6 +2061,7 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
         component -- repository component (e.g. 'main')
         architecture -- package architecture (e.g. 'i386')
         parameters -- attach any additional metadata
+        quote_parameters -- URL quote parameter values and names
         """
         params = {
             "deb.distribution": distribution,
@@ -2029,7 +2070,9 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
         }
         params.update(parameters)
 
-        self.deploy_file(file_name, parameters=params)
+        self.deploy_file(
+            file_name, parameters=params, quote_parameters=quote_parameters
+        )
 
     def copy(self, dst, suppress_layouts=False, fail_fast=False, dry_run=False):
         """
@@ -2107,7 +2150,17 @@ class ArtifactoryPath(pathlib.Path, PureArtifactoryPath):
                 return
 
             with self.open() as fobj:
-                dst.deploy(fobj, md5=stat.md5, sha1=stat.sha1, sha256=stat.sha256)
+                dst.deploy(
+                    fobj,
+                    md5=stat.md5,
+                    sha1=stat.sha1,
+                    sha256=stat.sha256,
+                    quote_parameters=False,
+                )
+                # TODO: v0.10.0 - possibly remove quote_parameters explicit setting
+                # Because this call never has parameters, it should not matter what it's set to.
+                # In this version, we set it explicitly to avoid the warning.
+                # In 0.10.0 or later, we can either keep it explicitly set to False, or remove it entirely.
 
     def move(self, dst, suppress_layouts=False, fail_fast=False, dry_run=False):
         """


Test patch:
diff --git a/tests/unit/test_artifactory_path.py b/tests/unit/test_artifactory_path.py
index 607d8fc..e4ef608 100644
--- a/tests/unit/test_artifactory_path.py
+++ b/tests/unit/test_artifactory_path.py
@@ -8,6 +8,8 @@ import dateutil
 import responses
 from responses.matchers import json_params_matcher
 from responses.matchers import query_param_matcher
+from responses.matchers import query_string_matcher
+from urllib3.util import parse_url
 
 import artifactory
 from artifactory import ArtifactoryPath
@@ -22,16 +24,23 @@ class UtilTest(unittest.TestCase):
     def test_matrix_encode(self):
         params = {"foo": "bar", "qux": "asdf"}
 
-        s = artifactory.encode_matrix_parameters(params)
+        s = artifactory.encode_matrix_parameters(params, quote_parameters=False)
 
         self.assertEqual(s, "foo=bar;qux=asdf")
 
         params = {"baz": ["bar", "quux"], "foo": "asdf"}
 
-        s = artifactory.encode_matrix_parameters(params)
+        s = artifactory.encode_matrix_parameters(params, quote_parameters=False)
 
         self.assertEqual(s, "baz=bar;baz=quux;foo=asdf")
 
+        # Test with quoting
+        params = {"b?az": ["b%ar", "quux"], "foo?%0": "a/s&d%f?"}
+
+        s = artifactory.encode_matrix_parameters(params, quote_parameters=True)
+
+        self.assertEqual(s, "b%3Faz=b%25ar;b%3Faz=quux;foo%3F%250=a/s%26d%25f%3F")
+
     def test_escape_chars(self):
         s = artifactory.escape_chars("a,b|c=d")
         self.assertEqual(s, r"a\,b\|c\=d")
@@ -895,60 +904,119 @@ class ArtifactoryPathTest(ClassSetup):
         constructed_url = (
             "http://artifactory.local/artifactory" "/api/storage" "/libs-release-local"
         )
-        responses.add(
-            responses.GET,
-            constructed_url,
-            status=200,
-            json=self.dir_stat,
-        )
-
-        matrix_parameters = "deb.architecture=amd64;deb.component=contrib;deb.distribution=dist1;deb.distribution=dist2"
 
         # file is required to calculate checksums
         with tempfile.NamedTemporaryFile(mode="w") as file:
             test_file = pathlib.Path(file.name)
             file.write("I am a test file")
 
-            constructed_url = f"{path}{test_file.name};{matrix_parameters}"
-            responses.add(
-                responses.PUT, constructed_url, status=200, match_querystring=True
-            )
-
-            path.deploy_file(
-                test_file,
-                explode_archive=True,
-                explode_archive_atomic=True,
-                parameters={
-                    "deb.architecture": "amd64",
-                    "deb.component": "contrib",
-                    "deb.distribution": ["dist1", "dist2"],
-                },
-            )
-
-        request_url = responses.calls[1].request.url
-        self.assertEqual(request_url, constructed_url)
-
-        # verify that all headers are present and checksums are calculated properly
-        headers = responses.calls[1].request.headers
-        self.assertIn("X-Checksum-Md5", headers)
-        self.assertEqual(headers["X-Checksum-Md5"], "d41d8cd98f00b204e9800998ecf8427e")
-
-        self.assertIn("X-Checksum-Sha1", headers)
-        self.assertEqual(
-            headers["X-Checksum-Sha1"], "da39a3ee5e6b4b0d3255bfef95601890afd80709"
-        )
-
-        self.assertIn("X-Checksum-Sha256", headers)
-        self.assertEqual(
-            headers["X-Checksum-Sha256"],
-            "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
-        )
-
-        self.assertIn("X-Explode-Archive", headers)
-        self.assertEqual(headers["X-Explode-Archive"], "true")
-
-        self.assertIn("X-Explode-Archive-Atomic", headers)
-        self.assertEqual(headers["X-Explode-Archive-Atomic"], "true")
+            # can't use pytest.mark.parametrize with unittest classes
+            for i, quote_params in enumerate((None, True, False)):
+                responses.add(
+                    responses.GET,
+                    constructed_url,
+                    status=200,
+                    json=self.dir_stat,
+                )
+
+                static_matrix_parameters = "deb.architecture=amd64;deb.component=contrib;deb.distribution=dist1;deb.distribution=dist2"
+                if quote_params:
+                    expected_properties = {
+                        "deb.architecture": "amd64",
+                        "deb.component": "contrib",
+                        "deb.distribution": ["dist1", "dist2"],
+                        "propA": "a%3Fb",
+                        "prop%253FB": "a%250b",
+                        "prop%3FC": "see",
+                    }
+                    matrix_parameters = f"{static_matrix_parameters};prop%253FB=a%250b;prop%3FC=see;propA=a%3Fb"
+                else:
+                    expected_properties = {
+                        "deb.architecture": "amd64",
+                        "deb.component": "contrib",
+                        "deb.distribution": ["dist1", "dist2"],
+                        "propA": "a?b",
+                        "prop%3FB": "a%0b",
+                        "prop?C": "see",
+                    }
+                    matrix_parameters = (
+                        f"{static_matrix_parameters};prop%3FB=a%0b;prop?C=see;propA=a?b"
+                    )
+
+                item_constructed_url = f"{path}{test_file.name};{matrix_parameters}"
+                responses.add(responses.PUT, item_constructed_url, status=200)
+
+                path.deploy_file(
+                    test_file,
+                    explode_archive=True,
+                    explode_archive_atomic=True,
+                    parameters={
+                        "deb.architecture": "amd64",
+                        "deb.component": "contrib",
+                        "deb.distribution": ["dist1", "dist2"],
+                        "propA": "a?b",
+                        "prop%3FB": "a%0b",
+                        "prop?C": "see",
+                    },
+                    quote_parameters=quote_params,
+                )
+
+                responses.remove(responses.GET, constructed_url)
+                responses.add(
+                    responses.GET,
+                    constructed_url,
+                    status=200,
+                    match=[query_string_matcher("properties")],
+                    json=dict(properties=expected_properties),
+                )
+
+                props = path.properties
+                assert props == expected_properties
+
+                # TODO: v0.10.0 - None test will not be needed, warn test will not be needed
+                if quote_params is None:
+                    self.assertWarnsRegex(
+                        UserWarning,
+                        r"^The current default value of quote_parameters \(False\) will change to True in v0\.10\.0\.\n"
+                        r"To ensure consistent behavior and remove this warning, explicitly set a value for quote_parameters.\n"
+                        r"For more details see https://github\.com/devopshq/artifactory/issues/408\.$",
+                    )
+
+                # We are in a for loop, each iteration makes 3 mocked requests,
+                # and the one we want to do all these assertions on is the middle one.
+                request_index = (i * 3) + 1
+                request_url = responses.calls[request_index].request.url
+                # the reason we need to call parse_url here is because Responses calls it:
+                # https://github.com/getsentry/responses/blob/master/responses/__init__.py#L306
+                # and it ends up changing parts of the URL that it thinks are escaped; namely
+                # it changes %0b in our test URL to %0B and so the assertion ends up failing.
+                expected_url = parse_url(item_constructed_url).url
+                self.assertEqual(request_url, expected_url)
+
+                # verify that all headers are present and checksums are calculated properly
+                headers = responses.calls[request_index].request.headers
+                self.assertIn("X-Checksum-Md5", headers)
+                self.assertEqual(
+                    headers["X-Checksum-Md5"], "d41d8cd98f00b204e9800998ecf8427e"
+                )
+
+                self.assertIn("X-Checksum-Sha1", headers)
+                self.assertEqual(
+                    headers["X-Checksum-Sha1"],
+                    "da39a3ee5e6b4b0d3255bfef95601890afd80709",
+                )
+
+                self.assertIn("X-Checksum-Sha256", headers)
+                self.assertEqual(
+                    headers["X-Checksum-Sha256"],
+                    "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+                )
+
+                self.assertIn("X-Explode-Archive", headers)
+                self.assertEqual(headers["X-Explode-Archive"], "true")
+
+                self.assertIn("X-Explode-Archive-Atomic", headers)
+                self.assertEqual(headers["X-Explode-Archive-Atomic"], "true")
 
     def test_deploy_by_checksum_sha1(self):
         """
@@ -962,7 +1030,10 @@ class ArtifactoryPathTest(ClassSetup):
                 json=self.file_stat_without_modification_date,
                 status=200,
             )
-            self.path.deploy_by_checksum(sha1=self.sha1)
+            self.path.deploy_by_checksum(sha1=self.sha1, quote_parameters=True)
+            # TODO: v0.10.0 - remove quote_parameters explicit setting
+            # These tests should allow the default value of the underlying method to be used.
+            # In this version, we set it explicitly to avoid the warning.
 
             self.assertEqual(len(rsps.calls), 1)
             self.assertEqual(rsps.calls[0].request.url, self.artifact_url)
@@ -984,7 +1055,10 @@ class ArtifactoryPathTest(ClassSetup):
                 json=self.file_stat_without_modification_date,
                 status=200,
             )
-            self.path.deploy_by_checksum(sha256=self.sha256)
+            self.path.deploy_by_checksum(sha256=self.sha256, quote_parameters=True)
+            # TODO: v0.10.0 - remove quote_parameters explicit setting
+            # These tests should allow the default value of the underlying method to be used.
+            # In this version, we set it explicitly to avoid the warning.
 
             self.assertEqual(len(rsps.calls), 1)
             self.assertEqual(rsps.calls[0].request.url, self.artifact_url)
@@ -1006,7 +1080,10 @@ class ArtifactoryPathTest(ClassSetup):
                 json=self.file_stat_without_modification_date,
                 status=200,
             )
-            self.path.deploy_by_checksum(checksum=self.sha1)
+            self.path.deploy_by_checksum(checksum=self.sha1, quote_parameters=True)
+            # TODO: v0.10.0 - remove quote_parameters explicit setting
+            # These tests should allow the default value of the underlying method to be used.
+            # In this version, we set it explicitly to avoid the warning.
 
             self.assertEqual(len(rsps.calls), 1)
             self.assertEqual(rsps.calls[0].request.url, self.artifact_url)
@@ -1023,7 +1100,10 @@ class ArtifactoryPathTest(ClassSetup):
                 json=self.file_stat_without_modification_date,
                 status=200,
             )
-            self.path.deploy_by_checksum(checksum=self.sha256)
+            self.path.deploy_by_checksum(checksum=self.sha256, quote_parameters=True)
+            # TODO: v0.10.0 - remove quote_parameters explicit setting
+            # These tests should allow the default value of the underlying method to be used.
+            # In this version, we set it explicitly to avoid the warning.
 
             self.assertEqual(len(rsps.calls), 1)
             self.assertEqual(rsps.calls[0].request.url, self.artifact_url)
@@ -1046,7 +1126,12 @@ class ArtifactoryPathTest(ClassSetup):
                 status=400,
             )
             with self.assertRaises(ArtifactoryException) as context:
-                self.path.deploy_by_checksum(sha1=f"{self.sha1}invalid")
+                self.path.deploy_by_checksum(
+                    sha1=f"{self.sha1}invalid", quote_parameters=True
+                )
+                # TODO: v0.10.0 - remove quote_parameters explicit setting
+                # These tests should allow the default value of the underlying method to be used.
+                # In this version, we set it explicitly to avoid the warning.
 
             self.assertEqual(str(context.exception), "Checksum values not provided")
 
@@ -1092,9 +1177,7 @@ class ArtifactoryPathTest(ClassSetup):
             file.write("I am a test file")
 
             constructed_url = f"{path}{test_file.name};{matrix_parameters}"
-            responses.add(
-                responses.PUT, constructed_url, status=200, match_querystring=True
-            )
+            responses.add(responses.PUT, constructed_url, status=200)
 
             path.deploy_deb(
                 test_file,
@@ -1102,7 +1185,11 @@ class ArtifactoryPathTest(ClassSetup):
                 component="contrib",
                 architecture="amd64",
                 parameters={"z.additional": "param"},
+                quote_parameters=True,
             )
+            # TODO: v0.10.0 - remove quote_parameters explicit setting
+            # These tests should allow the default value of the underlying method to be used.
+            # In this version, we set it explicitly to avoid the warning.
 
         request_url = responses.calls[1].request.url
         self.assertEqual(request_url, constructed_url)