Skip to content
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

Fix implicit conversion of data from YAML files #40

Merged
merged 1 commit into from
May 28, 2019
Merged

Fix implicit conversion of data from YAML files #40

merged 1 commit into from
May 28, 2019

Conversation

negillett
Copy link
Member

@negillett negillett commented May 22, 2019

To maintain data integrety when loading YAML files, loading is now done
using BaseLoader in stead of SafeLoader. Fixes #34.

Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't the best approach as it's covering the issue only for that specific example of conversion to float, but there are other kinds of conversions we should be avoiding too.

Another couple of examples of surprising behavior during load:

>>> import yaml
>>> yaml.load('foo: yes')
{'foo': True}
>>> yaml.load('foo: 0123')
{'foo': 83}

# with BaseLoader instead, always get strings
>>> yaml.load('foo: 0123', Loader=yaml.BaseLoader)
{'foo': '0123'}
>>> yaml.load('foo: yes', Loader=yaml.BaseLoader)
{'foo': 'yes'}

Boolean is contrived, but I don't think it's outside the realm of possibility that there could be a stream named something like 0123 at some point, seeing as streams may be based on upstream version numbers and those can be anything.

It seems like it should be using BaseLoader to disable the conversions entirely.

As I understand it, there was a concern that replacing safe_load (which uses SafeLoader) with BaseLoader would make the code less safe. I don't think this is true, based on docs at https://pyyaml.org/wiki/PyYAMLDocumentation the hierarchy seems to be:

  • Loader - "supports all predefined tags and may construct an arbitrary Python object", i.e. completely unsafe
  • SafeLoader - "supports only standard YAML tags", safe as it can't construct arbitrary Python objects. Also enables the implicit conversions although the docs don't point this out too clearly.
  • BaseLoader - "does not resolve or support any tags", i.e. even safer than SafeLoader. It's documented as returning "only basic Python objects: lists, dictionaries and Unicode strings."

To maintain data integrety when loading YAML files, loading is now done
using BaseLoader in stead of SafeLoader.
@negillett negillett requested a review from rohanpm May 23, 2019 12:40
@negillett negillett changed the title Fix trimming of trailing zeros on decmials Fix implicit conversion of data from YAML files May 23, 2019
@negillett negillett merged commit 8d75037 into master May 28, 2019
@negillett negillett deleted the 6936 branch May 28, 2019 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ubiconfig can load broken data for certain fields due to implicit YAML types
4 participants