From e43aa011c6878363dc81f3afe9f029060accb761 Mon Sep 17 00:00:00 2001 From: ashwini-orchestral Date: Mon, 22 Feb 2021 14:09:06 +0000 Subject: [PATCH 01/10] Replaced yam.load with yaml.safe_load --- st2common/st2common/util/spec_loader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/st2common/util/spec_loader.py b/st2common/st2common/util/spec_loader.py index 8ab926330f..c01bde8d20 100644 --- a/st2common/st2common/util/spec_loader.py +++ b/st2common/st2common/util/spec_loader.py @@ -77,7 +77,7 @@ def load_spec(module_name, spec_file, allow_duplicate_keys=False): # 1. Check for duplicate keys if not allow_duplicate_keys: - yaml.load(spec_string, UniqueKeyLoader) + yaml.safe_load(spec_string) # 2. Generate actual spec spec = yaml.safe_load(spec_string) From 9a1647e28125dcb9abd2f37e4c702e3cd46181ce Mon Sep 17 00:00:00 2001 From: W Chan Date: Wed, 3 Mar 2021 05:15:29 +0000 Subject: [PATCH 02/10] Add the UniqueKeyLoader to the yaml SafeConstructor Add the UniqueKeyLoader to the yaml SafeConstructor so that duplicate key is checked on safe_load. We want to use safe_load since yaml.load should not be used to load data from untrusted source. --- st2common/st2common/cmd/validate_api_spec.py | 5 +++-- st2common/st2common/util/spec_loader.py | 16 ++++++++-------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/st2common/st2common/cmd/validate_api_spec.py b/st2common/st2common/cmd/validate_api_spec.py index 743b3e467a..a9dad76fa8 100644 --- a/st2common/st2common/cmd/validate_api_spec.py +++ b/st2common/st2common/cmd/validate_api_spec.py @@ -111,8 +111,9 @@ def main(): setup() try: - # 1. Validate there are no duplicates keys in the YAML file - spec_loader.load_spec('st2common', 'openapi.yaml.j2', allow_duplicate_keys=False) + # Validate there are no duplicates keys in the YAML file. + # The spec loader do not allow duplicate keys. + spec_loader.load_spec('st2common', 'openapi.yaml.j2') # 2. Validate schema (currently broken) # validate_spec() diff --git a/st2common/st2common/util/spec_loader.py b/st2common/st2common/util/spec_loader.py index c01bde8d20..dde6d54be1 100644 --- a/st2common/st2common/util/spec_loader.py +++ b/st2common/st2common/util/spec_loader.py @@ -20,6 +20,7 @@ import yaml from yaml.constructor import ConstructorError +from yaml.constructor import SafeConstructor from yaml.nodes import MappingNode try: @@ -72,16 +73,15 @@ def construct_mapping(self, node, deep=False): return mapping -def load_spec(module_name, spec_file, allow_duplicate_keys=False): - spec_string = generate_spec(module_name, spec_file) +# Add the check duplicate method above to the SafeConstructor so it is invoked by safe_load. +SafeConstructor.add_constructor( + yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG, UniqueKeyLoader.construct_mapping +) - # 1. Check for duplicate keys - if not allow_duplicate_keys: - yaml.safe_load(spec_string) - # 2. Generate actual spec - spec = yaml.safe_load(spec_string) - return spec +def load_spec(module_name, spec_file): + spec_string = generate_spec(module_name, spec_file) + return yaml.safe_load(spec_string) def generate_spec(module_name, spec_file): From 2451b2059082cf498f74cf08371ebb2b46e45e4a Mon Sep 17 00:00:00 2001 From: W Chan Date: Wed, 3 Mar 2021 06:15:41 +0000 Subject: [PATCH 03/10] Use yaml.add_constructor to add the constructor to SafeLoader --- st2common/st2common/util/spec_loader.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/st2common/st2common/util/spec_loader.py b/st2common/st2common/util/spec_loader.py index dde6d54be1..b3c7238abd 100644 --- a/st2common/st2common/util/spec_loader.py +++ b/st2common/st2common/util/spec_loader.py @@ -20,7 +20,6 @@ import yaml from yaml.constructor import ConstructorError -from yaml.constructor import SafeConstructor from yaml.nodes import MappingNode try: @@ -73,9 +72,11 @@ def construct_mapping(self, node, deep=False): return mapping -# Add the check duplicate method above to the SafeConstructor so it is invoked by safe_load. -SafeConstructor.add_constructor( - yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG, UniqueKeyLoader.construct_mapping +# Add UniqueKeyLoader to the yaml SafeLoader so it is invoked by safe_load. +yaml.add_constructor( + yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG, + UniqueKeyLoader.construct_mapping, + Loader=yaml.SafeLoader ) From 8f06095bbdaabbe4b3f07ac82d97b60b4c759ed4 Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 6 Mar 2021 00:10:24 +0000 Subject: [PATCH 04/10] Add unit tests for spec_loader Add unit tests to cover success case and failure where YAML has duplicate keys. --- st2common/tests/unit/test_spec_loader.py | 36 +++++++++++++++++++ st2tests/st2tests/fixtures/specs/__init__.py | 0 .../st2tests/fixtures/specs/openapi.yaml.j2 | 6 ++++ 3 files changed, 42 insertions(+) create mode 100644 st2common/tests/unit/test_spec_loader.py create mode 100644 st2tests/st2tests/fixtures/specs/__init__.py create mode 100644 st2tests/st2tests/fixtures/specs/openapi.yaml.j2 diff --git a/st2common/tests/unit/test_spec_loader.py b/st2common/tests/unit/test_spec_loader.py new file mode 100644 index 0000000000..a3e6c88eb7 --- /dev/null +++ b/st2common/tests/unit/test_spec_loader.py @@ -0,0 +1,36 @@ +# Copyright 2020 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. + +from __future__ import absolute_import + +import pkg_resources +import unittest2 +import yaml + +from st2common.util import spec_loader + + +class SpecLoaderTest(unittest2.TestCase): + + def test_spec_loader(self): + self.assertTrue(isinstance(spec_loader.load_spec("st2common", "openapi.yaml.j2"), dict)) + + def test_bad_spec_duplicate_keys(self): + self.assertRaisesRegex( + yaml.constructor.ConstructorError, + "found duplicate key \"swagger\"", + spec_loader.load_spec, + "st2tests.fixtures.specs", + "openapi.yaml.j2", + ) diff --git a/st2tests/st2tests/fixtures/specs/__init__.py b/st2tests/st2tests/fixtures/specs/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/st2tests/st2tests/fixtures/specs/openapi.yaml.j2 b/st2tests/st2tests/fixtures/specs/openapi.yaml.j2 new file mode 100644 index 0000000000..93ad611ab8 --- /dev/null +++ b/st2tests/st2tests/fixtures/specs/openapi.yaml.j2 @@ -0,0 +1,6 @@ +swagger: '2.0' +swagger: '3.0' + +info: + version: "1.0.0" + title: StackStorm API From 6ce51b325724cd0c64d8a779fa8417fd2ea4929e Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 6 Mar 2021 00:15:33 +0000 Subject: [PATCH 05/10] Remove unused import --- st2common/tests/unit/test_spec_loader.py | 1 - 1 file changed, 1 deletion(-) diff --git a/st2common/tests/unit/test_spec_loader.py b/st2common/tests/unit/test_spec_loader.py index a3e6c88eb7..56b2a9fcd6 100644 --- a/st2common/tests/unit/test_spec_loader.py +++ b/st2common/tests/unit/test_spec_loader.py @@ -14,7 +14,6 @@ from __future__ import absolute_import -import pkg_resources import unittest2 import yaml From d04a7792d76dcde2b7082634cb8458107bd1498d Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 6 Mar 2021 00:26:43 +0000 Subject: [PATCH 06/10] Change the error message in the UniqueKeyLoader --- st2common/st2common/util/spec_loader.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/st2common/st2common/util/spec_loader.py b/st2common/st2common/util/spec_loader.py index b3c7238abd..b89972b879 100644 --- a/st2common/st2common/util/spec_loader.py +++ b/st2common/st2common/util/spec_loader.py @@ -46,27 +46,29 @@ } +# Custom YAML loader that throw an exception on duplicate key. +# Credit: https://gist.github.com/pypt/94d747fe5180851196eb class UniqueKeyLoader(Loader): - """ - YAML loader which throws on a duplicate key. - """ def construct_mapping(self, node, deep=False): - if not isinstance(node, MappingNode): - raise ConstructorError(None, None, - "expected a mapping node, but found %s" % node.id, - node.start_mark) + if not isinstance(node, nodes.MappingNode): + raise constructor.ConstructorError( + None, None, "expected a mapping node, but found %s" % node.id, node.start_mark + ) mapping = {} for key_node, value_node in node.value: key = self.construct_object(key_node, deep=deep) try: hash(key) except TypeError as exc: - raise ConstructorError("while constructing a mapping", node.start_mark, - "found unacceptable key (%s)" % exc, key_node.start_mark) + raise constructor.ConstructorError( + "while constructing a mapping", + node.start_mark, + "found unacceptable key (%s)" % exc, + key_node.start_mark, + ) # check for duplicate keys if key in mapping: - raise ConstructorError("while constructing a mapping", node.start_mark, - "found duplicate key", key_node.start_mark) + raise constructor.ConstructorError('found duplicate key "%s"' % key_node.value) value = self.construct_object(value_node, deep=deep) mapping[key] = value return mapping From ba1c5c0b57c8fde6371064330f056f53b21ea579 Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 6 Mar 2021 00:32:26 +0000 Subject: [PATCH 07/10] Fix yaml imports in spec_loader --- st2common/st2common/util/spec_loader.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/st2common/st2common/util/spec_loader.py b/st2common/st2common/util/spec_loader.py index b89972b879..25bb354b76 100644 --- a/st2common/st2common/util/spec_loader.py +++ b/st2common/st2common/util/spec_loader.py @@ -18,9 +18,8 @@ import jinja2 import yaml - -from yaml.constructor import ConstructorError -from yaml.nodes import MappingNode +from yaml import constructor +from yaml import nodes try: from yaml import CLoader as Loader From 308f73002b84eb3a90698326d8f62844e13f0a60 Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 6 Mar 2021 00:34:26 +0000 Subject: [PATCH 08/10] Add changelog entry for refactoring spec_loader --- CHANGELOG.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 11ecb52105..0c1298fc29 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -4,6 +4,11 @@ Changelog in development -------------- +Fixed +~~~~~ + +* Refactor spec_loader util to use yaml.safe_load instead of yaml.load. (security) + Contributed by @ashwini-orchestral 3.4.0 - March 02, 2021 ---------------------- From dd649792b1b638ecbe82ea444d36d9935d8d5ac1 Mon Sep 17 00:00:00 2001 From: W Chan Date: Wed, 10 Mar 2021 01:04:35 +0000 Subject: [PATCH 09/10] Use yaml.load with CSafeLoader The method yaml.safe_load uses the python implementation of SafeLoader by default. Use the CSafeLoader if import is successful for better performance. This requires using the yaml.load method but pass the CSafeLoader. --- st2common/st2common/cmd/validate_api_spec.py | 2 +- st2common/st2common/util/spec_loader.py | 28 +++++++++++++------- st2common/tests/unit/test_spec_loader.py | 7 ++--- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/st2common/st2common/cmd/validate_api_spec.py b/st2common/st2common/cmd/validate_api_spec.py index 878d0ba5bc..01659be732 100644 --- a/st2common/st2common/cmd/validate_api_spec.py +++ b/st2common/st2common/cmd/validate_api_spec.py @@ -116,7 +116,7 @@ def main(): try: # Validate there are no duplicates keys in the YAML file. # The spec loader do not allow duplicate keys. - spec_loader.load_spec('st2common', 'openapi.yaml.j2') + spec_loader.load_spec("st2common", "openapi.yaml.j2") ret = 0 except Exception: diff --git a/st2common/st2common/util/spec_loader.py b/st2common/st2common/util/spec_loader.py index 4f763f6b2d..4ae8229ca1 100644 --- a/st2common/st2common/util/spec_loader.py +++ b/st2common/st2common/util/spec_loader.py @@ -18,13 +18,14 @@ import jinja2 import yaml -from yaml import constructor -from yaml import nodes try: - from yaml import CLoader as Loader + from yaml import CSafeLoader as SafeLoader except ImportError: - from yaml import Loader + from yaml import SafeLoader + +from yaml import constructor +from yaml import nodes import st2common.constants.pack import st2common.constants.action @@ -44,11 +45,14 @@ # Custom YAML loader that throw an exception on duplicate key. # Credit: https://gist.github.com/pypt/94d747fe5180851196eb -class UniqueKeyLoader(Loader): +class UniqueKeyLoader(SafeLoader): def construct_mapping(self, node, deep=False): if not isinstance(node, nodes.MappingNode): raise constructor.ConstructorError( - None, None, "expected a mapping node, but found %s" % node.id, node.start_mark + None, + None, + "expected a mapping node, but found %s" % node.id, + node.start_mark, ) mapping = {} for key_node, value_node in node.value: @@ -64,7 +68,9 @@ def construct_mapping(self, node, deep=False): ) # check for duplicate keys if key in mapping: - raise constructor.ConstructorError('found duplicate key "%s"' % key_node.value) + raise constructor.ConstructorError( + 'found duplicate key "%s"' % key_node.value + ) value = self.construct_object(value_node, deep=deep) mapping[key] = value return mapping @@ -74,13 +80,17 @@ def construct_mapping(self, node, deep=False): yaml.add_constructor( yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG, UniqueKeyLoader.construct_mapping, - Loader=yaml.SafeLoader + Loader=SafeLoader, ) def load_spec(module_name, spec_file): spec_string = generate_spec(module_name, spec_file) - return yaml.safe_load(spec_string) + + # The use of yaml.load and passing SafeLoader is the same as yaml.safe_load which + # makes the same call. The difference here is that we use CSafeLoader where possible + # to improve performance and yaml.safe_load uses the python implementation by default. + return yaml.load(spec_string, SafeLoader) def generate_spec(module_name, spec_file): diff --git a/st2common/tests/unit/test_spec_loader.py b/st2common/tests/unit/test_spec_loader.py index 56b2a9fcd6..a9b91d96d2 100644 --- a/st2common/tests/unit/test_spec_loader.py +++ b/st2common/tests/unit/test_spec_loader.py @@ -21,14 +21,15 @@ class SpecLoaderTest(unittest2.TestCase): - def test_spec_loader(self): - self.assertTrue(isinstance(spec_loader.load_spec("st2common", "openapi.yaml.j2"), dict)) + self.assertTrue( + isinstance(spec_loader.load_spec("st2common", "openapi.yaml.j2"), dict) + ) def test_bad_spec_duplicate_keys(self): self.assertRaisesRegex( yaml.constructor.ConstructorError, - "found duplicate key \"swagger\"", + 'found duplicate key "swagger"', spec_loader.load_spec, "st2tests.fixtures.specs", "openapi.yaml.j2", From 9b6cfbbd6b16cf8ba42dd5726baf1689aa27cfde Mon Sep 17 00:00:00 2001 From: W Chan Date: Wed, 10 Mar 2021 01:08:13 +0000 Subject: [PATCH 10/10] Update changelog entry --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8153091e5e..4de2c33ad7 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,7 +7,7 @@ in development Fixed ~~~~~ -* Refactor spec_loader util to use yaml.safe_load instead of yaml.load. (security) +* Refactor spec_loader util to use yaml.load with SafeLoader. (security) Contributed by @ashwini-orchestral Changed