diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e063fbff18..d6c8bac63d 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -22,6 +22,7 @@ Redis 8.6 Fixed ~~~~~ +* Fix key value pairs with a ``/`` in the key name failing to be set, retrieved and deleted via the API and the CLI. ``st2client`` now URL encodes the key name in request URLs and the API router URL decodes path variables after route matching instead of before. #4789 * Fix ``TypeError`` when displaying help for actions whose parameters have no ``description`` key. #6375 * Fix utf-8 encode before checking paramter max size #6352 * Fix stuck running workflow tasks #6398 (by @guzzijones12@gmail.com) diff --git a/st2client/st2client/models/core.py b/st2client/st2client/models/core.py index 633e57911f..2a40103119 100644 --- a/st2client/st2client/models/core.py +++ b/st2client/st2client/models/core.py @@ -69,6 +69,15 @@ def parse_api_response(response: requests.models.Response) -> dict: return data +def _quote_url_path_segment(value): + """ + Quote a single URL path segment (a resource id or name) so a value which + contains characters such as "/" stays inside one path segment and reaches + the API intact. Key value pair names, for example, may contain "/". + """ + return urllib.parse.quote(str(value), safe="") + + class Resource(object): # An alias to use for the resource if different than the class name. @@ -253,7 +262,10 @@ def get_all(self, **kwargs): @add_auth_token_to_kwargs_from_env def get_by_id(self, id, **kwargs): - url = "/%s/%s" % (self.resource.get_url_path_name(), id) + url = "/%s/%s" % ( + self.resource.get_url_path_name(), + _quote_url_path_segment(id), + ) response = self.client.get(url, **kwargs) if response.status_code == http_client.NOT_FOUND: return None @@ -276,12 +288,16 @@ def get_property(self, id_, property_name, self_deserialize=True, **kwargs): if kwargs: url = "/%s/%s/%s/?%s" % ( self.resource.get_url_path_name(), - id_, + _quote_url_path_segment(id_), property_name, urllib.parse.urlencode(kwargs), ) else: - url = "/%s/%s/%s/" % (self.resource.get_url_path_name(), id_, property_name) + url = "/%s/%s/%s/" % ( + self.resource.get_url_path_name(), + _quote_url_path_segment(id_), + property_name, + ) if token: response = self.client.get(url, token=token) @@ -377,7 +393,10 @@ def create(self, instance, **kwargs): @add_auth_token_to_kwargs_from_env def update(self, instance, **kwargs): - url = "/%s/%s" % (self.resource.get_url_path_name(), instance.id) + url = "/%s/%s" % ( + self.resource.get_url_path_name(), + _quote_url_path_segment(instance.id), + ) response = self.client.put(url, instance.serialize(), **kwargs) if response.status_code != http_client.OK: self.handle_error(response) @@ -386,7 +405,10 @@ def update(self, instance, **kwargs): @add_auth_token_to_kwargs_from_env def delete(self, instance, **kwargs): - url = "/%s/%s" % (self.resource.get_url_path_name(), instance.id) + url = "/%s/%s" % ( + self.resource.get_url_path_name(), + _quote_url_path_segment(instance.id), + ) response = self.client.delete(url, **kwargs) if response.status_code not in [ @@ -401,7 +423,10 @@ def delete(self, instance, **kwargs): @add_auth_token_to_kwargs_from_env def delete_action(self, instance, remove_files, **kwargs): - url = "/%s/%s" % (self.resource.get_url_path_name(), instance.id) + url = "/%s/%s" % ( + self.resource.get_url_path_name(), + _quote_url_path_segment(instance.id), + ) payload = {"remove_files": remove_files} response = self.client.delete(url, data=orjson.dumps(payload), **kwargs) if response.status_code not in [ @@ -437,7 +462,10 @@ def clone( @add_auth_token_to_kwargs_from_env def delete_by_id(self, instance_id, **kwargs): - url = "/%s/%s" % (self.resource.get_url_path_name(), instance_id) + url = "/%s/%s" % ( + self.resource.get_url_path_name(), + _quote_url_path_segment(instance_id), + ) response = self.client.delete(url, **kwargs) if response.status_code not in [ http_client.OK, @@ -906,7 +934,7 @@ def get_by_name(self, name, **kwargs): url = "/%s/%s/?%s" % ( self.resource.get_url_path_name(), - name, + _quote_url_path_segment(name), urllib.parse.urlencode(params), ) diff --git a/st2client/tests/unit/test_models.py b/st2client/tests/unit/test_models.py index 31e762eb81..cdd5406860 100644 --- a/st2client/tests/unit/test_models.py +++ b/st2client/tests/unit/test_models.py @@ -488,3 +488,45 @@ def test_resource_get_by_name(self): actual = resource.serialize() expected = json.loads(json.dumps(base.RESOURCES[0])) self.assertEqual(actual, expected) + + def test_keyvalue_get_by_name_with_slash_in_name(self): + mgr = models.KeyValuePairResourceManager( + models.KeyValuePair, base.FAKE_ENDPOINT + ) + response = base.FakeResponse( + json.dumps({"name": "foo/bar", "value": "value1"}), 200, "OK" + ) + with mock.patch.object( + httpclient.HTTPClient, "get", mock.MagicMock(return_value=response) + ) as mock_get: + resource = mgr.get_by_name("foo/bar") + + self.assertEqual(resource.name, "foo/bar") + self.assertEqual(mock_get.call_args[0][0], "/keys/foo%2Fbar/?") + + def test_keyvalue_update_with_slash_in_name(self): + mgr = models.KeyValuePairResourceManager( + models.KeyValuePair, base.FAKE_ENDPOINT + ) + instance = models.KeyValuePair(name="foo/bar", value="value1") + response = base.FakeResponse( + json.dumps({"name": "foo/bar", "value": "value1"}), 200, "OK" + ) + with mock.patch.object( + httpclient.HTTPClient, "put", mock.MagicMock(return_value=response) + ) as mock_put: + mgr.update(instance) + + self.assertEqual(mock_put.call_args[0][0], "/keys/foo%2Fbar") + + def test_keyvalue_delete_by_id_with_slash_in_name(self): + mgr = models.KeyValuePairResourceManager( + models.KeyValuePair, base.FAKE_ENDPOINT + ) + response = base.FakeResponse("", 204, "NO CONTENT") + with mock.patch.object( + httpclient.HTTPClient, "delete", mock.MagicMock(return_value=response) + ) as mock_delete: + mgr.delete_by_id("foo/bar") + + self.assertEqual(mock_delete.call_args[0][0], "/keys/foo%2Fbar") diff --git a/st2common/st2common/router.py b/st2common/st2common/router.py index 75eece396a..fb3b02c974 100644 --- a/st2common/st2common/router.py +++ b/st2common/st2common/router.py @@ -269,11 +269,14 @@ def add_spec(self, spec, transforms): ) def match(self, req): - # NOTE: webob.url_unquote doesn't work correctly under Python 3 when paths contain non-ascii - # characters. That method supposed to handle Python 2 and Python 3 compatibility, but it - # doesn't work correctly under Python 3. + # NOTE: The routes are matched against the URL encoded path and the + # extracted path variables are URL decoded afterwards. Decoding the + # whole path before matching would turn an escaped "/" ("%2F") inside + # a single path variable value (for example a key value pair name) + # into a real path separator, so such requests could never match + # their route. try: - path = urllib.parse.unquote(req.path) + path = req.path except Exception as e: # This exception being thrown indicates that the URL / path contains bad or incorrectly # URL escaped characters. Instead of returning this stack track + 500 error to the @@ -316,6 +319,14 @@ def match(self, req): method = path_vars.pop("_api_method") endpoint = self.spec["paths"][path][method] + # NOTE: webob.url_unquote doesn't work correctly under Python 3 when paths contain non-ascii + # characters. That method supposed to handle Python 2 and Python 3 compatibility, but it + # doesn't work correctly under Python 3, so urllib.parse.unquote is used instead. + path_vars = { + name: urllib.parse.unquote(value) if isinstance(value, str) else value + for (name, value) in six.iteritems(path_vars) + } + return endpoint, path_vars def __call__(self, req): diff --git a/st2common/tests/unit/test_router.py b/st2common/tests/unit/test_router.py new file mode 100644 index 0000000000..bf39677424 --- /dev/null +++ b/st2common/tests/unit/test_router.py @@ -0,0 +1,63 @@ +# Copyright 2026 The StackStorm Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + +import mock + +from st2common.router import Router + + +class RouterMatchTestCase(unittest.TestCase): + def _get_router(self): + router = Router(auth=False) + router.spec = { + "paths": { + "/api/v1/keys/{name}": { + "get": {"operationId": "st2api.controllers.v1.keyvalue:get_one"} + } + } + } + mapper = router.routes.submapper( + _api_path="/api/v1/keys/{name}", + _api_method="get", + conditions={"method": ["GET"]}, + ) + mapper.connect(None, "/v1/keys/{name}") + return router + + def _match(self, path): + request = mock.Mock() + request.path = path + request.environ = {"REQUEST_METHOD": "GET"} + return self._get_router().match(request) + + def test_match_plain_name(self): + endpoint, path_vars = self._match("/v1/keys/key1") + self.assertEqual(path_vars, {"name": "key1"}) + + def test_match_name_with_escaped_slash(self): + # A key value pair name may contain "/". The client URL encodes it as + # "%2F" and the router must hand the decoded name to the controller + # instead of failing to match the route. + endpoint, path_vars = self._match("/v1/keys/foo%2Fbar") + self.assertEqual(path_vars, {"name": "foo/bar"}) + + def test_match_name_with_escaped_space(self): + endpoint, path_vars = self._match("/v1/keys/foo%20bar") + self.assertEqual(path_vars, {"name": "foo bar"}) + + +if __name__ == "__main__": + unittest.main()