Skip to content
This repository was archived by the owner on Jun 25, 2020. It is now read-only.

Enforce that version is a string to prevent exceptions in the case of an all-integer version ID #119

Closed
asaba96 wants to merge 4 commits into from
Closed

Enforce that version is a string to prevent exceptions in the case of an all-integer version ID #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cab0259
fixed type issue with versions, if the version id is an integer
asaba96 Feb 10, 2018
0c2bd9c
fixed issue with None version
asaba96 Feb 10, 2018
b12689d
Added test for checking version is a str
asaba96 Feb 10, 2018
9c44843
style fixes and comments
asaba96 Oct 8, 2018
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
6 changes: 5 additions & 1 deletion src/wstool/config_yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,11 @@ def get_path_spec_from_yaml(yaml_dict):
elif key == "uri":
uri = value
elif key == "version":
version = value
# VCs tools expects version to be
# string; otherwise, all integer
# versions will break application
if value is not None:
version = str(value)
Copy link
Copy Markdown
Contributor

@tkruse tkruse Oct 4, 2018

Choose a reason for hiding this comment

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

Additionally, you could add str(arg).strip in the place the exception happened in vcstools, which should not be vulnerable to this. Possibly check other places we use strip without being careful about the type.

Copy link
Copy Markdown
Author

@asaba96 asaba96 Oct 8, 2018

Choose a reason for hiding this comment

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

So I addressed the issue here because it seemed to be caused due to the underlying PyYaml versions having different behavior in the way it returned a specific yawl value. So I could fix all exceptions related to strip or I could enforce that version is a string, which already something something above the file parsing layer already expected, so I assume there are other parts that depend on the same expectation of type.

Copy link
Copy Markdown
Contributor

@tkruse tkruse Oct 9, 2018

Choose a reason for hiding this comment

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

Sure, here is a good place for a fix. vcstools is a library in it's own right (potentially to be used by other projects), so if you have time, you could also make it less vulnerable, but it's a different story from this PR.

Copy link
Copy Markdown
Author

@asaba96 asaba96 Oct 9, 2018

Choose a reason for hiding this comment

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

Cool, I will take a look then to see what I can do with vcstools when I get a chance.

else:
raise MultiProjectException(
"Unknown key %s in %s" % (key, yaml_dict))
Expand Down
14 changes: 14 additions & 0 deletions test/local/test_config_yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,20 @@ def test_original_syntax_scm(self):
self.assertEqual(uri, wrap.get_uri())
self.assertEqual({scmtype: {'local-name': local_name, 'uri': uri}}, wrap.get_legacy_yaml())

# version is a number
local_name = 'common_rosdeps'
version = 1234
uri = 'https://kforge.ros.org/common/rosdepcore'
scmtype = 'hg'
struct = {scmtype: {'local-name': local_name, 'version': version, 'uri': uri}}
expected_struct = {scmtype: {'local-name': local_name, 'version': str(version), 'uri': uri}}
wrap = get_path_spec_from_yaml(struct)
self.assertEqual(scmtype, wrap.get_scmtype())
self.assertEqual(scmtype, wrap.get_legacy_type())
self.assertIsInstance(wrap.get_version(), str)
self.assertEqual(uri, wrap.get_uri())
self.assertEqual(expected_struct, wrap.get_legacy_yaml())

# no version
local_name = 'common_rosdeps'
version = None
Expand Down