Skip to content

Load YAML files safely #6897

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 16, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion readthedocs/config/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import yaml


__all__ = ('parse', 'ParseError')


Expand Down
20 changes: 20 additions & 0 deletions readthedocs/config/tests/test_yaml_loader.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import textwrap
from readthedocs.doc_builder.backends.mkdocs import yaml_load_safely


def test_yaml_load_safely():
expected = {
'int': 3,
'float': 3.0,
'unknown': None,
}

content = textwrap.dedent('''
int: 3
float: !!float 3
unknown: !!python/name:unknown_funtion
''')
data = yaml_load_safely(content)
assert data == expected
assert type(data['int']) == int
assert type(data['float']) == float
2 changes: 2 additions & 0 deletions readthedocs/config/utils.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
"""Shared functions for the config module."""

import yaml


def to_dict(value):
"""Recursively transform a class from `config.models` to a dict."""
Expand Down
30 changes: 28 additions & 2 deletions readthedocs/doc_builder/backends/mkdocs.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def get_final_doctype(self):
https://www.mkdocs.org/user-guide/configuration/#use_directory_urls
"""
with open(self.yaml_file, 'r') as f:
config = yaml.safe_load(f)
config = yaml_load_safely(f)
use_directory_urls = config.get('use_directory_urls', True)
return MKDOCS if use_directory_urls else MKDOCS_HTML

Expand All @@ -96,7 +96,7 @@ def load_yaml_config(self):
:raises: ``MkDocsYAMLParseError`` if failed due to syntax errors.
"""
try:
config = yaml.safe_load(open(self.yaml_file, 'r'))
config = yaml_load_safely(open(self.yaml_file, 'r'))

if not config:
raise MkDocsYAMLParseError(
Expand Down Expand Up @@ -324,3 +324,29 @@ class MkdocsJSON(BaseMkdocs):
type = 'mkdocs_json'
builder = 'json'
build_dir = '_build/json'


class SafeLoaderIgnoreUnknown(yaml.SafeLoader): # pylint: disable=too-many-ancestors

"""
YAML loader to ignore unknown tags.

Borrowed from https://stackoverflow.com/a/57121993
"""

def ignore_unknown(self, node): # pylint: disable=no-self-use, unused-argument
return None


SafeLoaderIgnoreUnknown.add_constructor(None, SafeLoaderIgnoreUnknown.ignore_unknown)


def yaml_load_safely(content):
"""
Uses ``SafeLoaderIgnoreUnknown`` loader to skip unknown tags.

When a YAML contains ``!!python/name:int`` it will complete ignore it an
return ``None`` for those fields instead of failing. We need this to avoid
executing random code, but still support these YAML files.
"""
return yaml.load(content, Loader=SafeLoaderIgnoreUnknown)
9 changes: 5 additions & 4 deletions readthedocs/rtd_tests/tests/test_doc_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from django_dynamic_fixture import get
from unittest.mock import patch

from readthedocs.config.utils import yaml_load_safely
from readthedocs.builds.models import Version
from readthedocs.doc_builder.backends.mkdocs import MkdocsHTML
from readthedocs.doc_builder.backends.sphinx import BaseSphinx
Expand Down Expand Up @@ -355,7 +356,7 @@ def test_append_conf_create_yaml(self, checkout_path, run):
# There is a mkdocs.yml file created
generated_yaml = os.path.join(tmpdir, 'mkdocs.yml')
self.assertTrue(os.path.exists(generated_yaml))
config = yaml.safe_load(open(generated_yaml))
config = yaml_load_safely(open(generated_yaml))
self.assertEqual(
config['docs_dir'],
os.path.join(tmpdir, 'docs'),
Expand Down Expand Up @@ -412,7 +413,7 @@ def test_append_conf_existing_yaml_on_root(self, checkout_path, run):

run.assert_called_with('cat', 'mkdocs.yml', cwd=mock.ANY)

config = yaml.safe_load(open(yaml_file))
config = yaml_load_safely(open(yaml_file))
self.assertEqual(
config['docs_dir'],
'docs',
Expand Down Expand Up @@ -502,7 +503,7 @@ def test_dont_override_theme(self, checkout_path, run):

run.assert_called_with('cat', 'mkdocs.yml', cwd=mock.ANY)

config = yaml.safe_load(open(yaml_file))
config = yaml_load_safely(open(yaml_file))
self.assertEqual(
config['theme_dir'],
'not-readthedocs',
Expand Down Expand Up @@ -606,7 +607,7 @@ def test_append_conf_existing_yaml_with_extra(self, checkout_path, run):

run.assert_called_with('cat', 'mkdocs.yml', cwd=mock.ANY)

config = yaml.safe_load(open(yaml_file))
config = yaml_load_safely(open(yaml_file))

self.assertEqual(
config['extra_css'],
Expand Down